From f44a8fa1e057fdd452ba6a6f477e02bacb97c412 Mon Sep 17 00:00:00 2001 From: Archish Thakkar Date: Fri, 26 Apr 2024 15:17:03 +0530 Subject: [PATCH 1/4] Lint fixes for api channel packages --- workhorse/internal/api/block.go | 2 +- workhorse/internal/api/block_test.go | 2 +- workhorse/internal/api/channel_settings.go | 18 ++++++++++--- .../internal/api/channel_settings_test.go | 26 +++++++++---------- 4 files changed, 29 insertions(+), 19 deletions(-) diff --git a/workhorse/internal/api/block.go b/workhorse/internal/api/block.go index f6eefcdf2ccf16..afcbc358319280 100644 --- a/workhorse/internal/api/block.go +++ b/workhorse/internal/api/block.go @@ -8,7 +8,7 @@ import ( "gitlab.com/gitlab-org/gitlab/workhorse/internal/helper/fail" ) -// Prevent internal API responses intended for gitlab-workhorse from +// Block method blocks internal API responses intended for gitlab-workhorse from // leaking to the end user func Block(h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/workhorse/internal/api/block_test.go b/workhorse/internal/api/block_test.go index a28dd8d203c909..46ced4b88903fd 100644 --- a/workhorse/internal/api/block_test.go +++ b/workhorse/internal/api/block_test.go @@ -41,7 +41,7 @@ func TestBlocker(t *testing.T) { upstreamBody := []byte(upstreamResponse) n, err := bl.Write(upstreamBody) require.NoError(t, err) - require.Equal(t, len(upstreamBody), n, "bytes written") + require.Len(t, upstreamBody, n, "bytes written") rw.Flush() diff --git a/workhorse/internal/api/channel_settings.go b/workhorse/internal/api/channel_settings.go index ed03b04a69be17..50d5560c9e58f4 100644 --- a/workhorse/internal/api/channel_settings.go +++ b/workhorse/internal/api/channel_settings.go @@ -1,3 +1,4 @@ +// Package api provides internal APIs for gitlab-workhorse. package api import ( @@ -10,13 +11,14 @@ import ( "github.com/gorilla/websocket" ) +// ChannelSettings holds the configuration settings for establishing a websocket channel. type ChannelSettings struct { // The channel provider may require use of a particular subprotocol. If so, // it must be specified here, and Workhorse must have a matching codec. Subprotocols []string // The websocket URL to connect to. - Url string + wsURL string // Any headers (e.g., Authorization) to send with the websocket request Header http.Header @@ -30,10 +32,12 @@ type ChannelSettings struct { MaxSessionTime int } +// URL parses the websocket URL in the ChannelSettings and returns a *url.URL. func (t *ChannelSettings) URL() (*url.URL, error) { - return url.Parse(t.Url) + return url.Parse(t.wsURL) } +// Dialer returns a websocket Dialer configured with the settings from ChannelSettings. func (t *ChannelSettings) Dialer() *websocket.Dialer { dialer := &websocket.Dialer{ Subprotocols: t.Subprotocols, @@ -48,6 +52,7 @@ func (t *ChannelSettings) Dialer() *websocket.Dialer { return dialer } +// Clone creates and returns a deep copy of the ChannelSettings instance. func (t *ChannelSettings) Clone() *ChannelSettings { // Doesn't clone the strings, but that's OK as strings are immutable in go cloned := *t @@ -58,10 +63,13 @@ func (t *ChannelSettings) Clone() *ChannelSettings { return &cloned } +// Dial establishes a websocket connection using the settings from ChannelSettings. +// It returns a websocket connection, an HTTP response, and an error if any. func (t *ChannelSettings) Dial() (*websocket.Conn, *http.Response, error) { - return t.Dialer().Dial(t.Url, t.Header) + return t.Dialer().Dial(t.wsURL, t.Header) } +// Validate checks if the ChannelSettings instance is valid. func (t *ChannelSettings) Validate() error { if t == nil { return fmt.Errorf("channel details not specified") @@ -83,6 +91,8 @@ func (t *ChannelSettings) Validate() error { return nil } +// IsEqual compares the current ChannelSettings with another ChannelSettings instance. +// It returns true if both instances are equal (or both nil), otherwise false. func (t *ChannelSettings) IsEqual(other *ChannelSettings) bool { if t == nil && other == nil { return true @@ -117,7 +127,7 @@ func (t *ChannelSettings) IsEqual(other *ChannelSettings) bool { } } - return t.Url == other.Url && + return t.wsURL == other.wsURL && t.CAPem == other.CAPem && t.MaxSessionTime == other.MaxSessionTime } diff --git a/workhorse/internal/api/channel_settings_test.go b/workhorse/internal/api/channel_settings_test.go index 4aa2c835579f21..f2287c454b6568 100644 --- a/workhorse/internal/api/channel_settings_test.go +++ b/workhorse/internal/api/channel_settings_test.go @@ -7,7 +7,7 @@ import ( func channel(url string, subprotocols ...string) *ChannelSettings { return &ChannelSettings{ - Url: url, + wsURL: url, Subprotocols: subprotocols, MaxSessionTime: 0, } @@ -104,19 +104,19 @@ func TestDialer(t *testing.T) { dialer = channel.Dialer() if dialer.TLSClientConfig == nil || dialer.TLSClientConfig.RootCAs == nil { - t.Fatalf("Custom CA certificates not recognised!") + t.Fatalf("Custom CA certificates not recognized!") } } func TestIsEqual(t *testing.T) { chann := channel("ws:", "foo") - chann_header2 := header(chann, "extra") - chann_header3 := header(chann) - chann_header3.Header.Add("Extra", "extra") + channHeader2 := header(chann, "extra") + channHeader3 := header(chann) + channHeader3.Header.Add("Extra", "extra") - chann_ca2 := ca(chann) - chann_ca2.CAPem = "other value" + channCa2 := ca(chann) + channCa2.CAPem = "other value" for i, tc := range []struct { channelA *ChannelSettings @@ -129,19 +129,19 @@ func TestIsEqual(t *testing.T) { {chann, chann, true}, {chann.Clone(), chann.Clone(), true}, {chann, channel("foo:"), false}, - {chann, channel(chann.Url), false}, + {chann, channel(chann.wsURL), false}, {header(chann), header(chann), true}, - {chann_header2, chann_header2, true}, - {chann_header3, chann_header3, true}, - {header(chann), chann_header2, false}, - {header(chann), chann_header3, false}, + {channHeader2, channHeader2, true}, + {channHeader3, channHeader3, true}, + {header(chann), channHeader2, false}, + {header(chann), channHeader3, false}, {header(chann), chann, false}, {chann, header(chann), false}, {ca(chann), ca(chann), true}, {ca(chann), chann, false}, {chann, ca(chann), false}, {ca(header(chann)), ca(header(chann)), true}, - {chann_ca2, ca(chann), false}, + {channCa2, ca(chann), false}, {chann, timeout(chann), false}, } { if actual := tc.channelA.IsEqual(tc.channelB); tc.expected != actual { -- GitLab From 6a97643f45c4a60384d923f57b5bd62149112086 Mon Sep 17 00:00:00 2001 From: Archish Date: Tue, 14 May 2024 14:32:19 +0530 Subject: [PATCH 2/4] Test fixes for auth checker --- workhorse/internal/api/channel_settings.go | 8 ++++---- workhorse/internal/channel/auth_checker_test.go | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/workhorse/internal/api/channel_settings.go b/workhorse/internal/api/channel_settings.go index 33ded5b6acbb92..6e8d2f13b07129 100644 --- a/workhorse/internal/api/channel_settings.go +++ b/workhorse/internal/api/channel_settings.go @@ -19,7 +19,7 @@ type ChannelSettings struct { Subprotocols []string // The websocket URL to connect to. - wsURL string + WsURL string // Any headers (e.g., Authorization) to send with the websocket request Header http.Header @@ -35,7 +35,7 @@ type ChannelSettings struct { // URL parses the websocket URL in the ChannelSettings and returns a *url.URL. func (t *ChannelSettings) URL() (*url.URL, error) { - return url.Parse(t.wsURL) + return url.Parse(t.WsURL) } // Dialer returns a websocket Dialer configured with the settings from ChannelSettings. @@ -72,7 +72,7 @@ func (t *ChannelSettings) Clone() *ChannelSettings { // Dial establishes a websocket connection using the settings from ChannelSettings. // It returns a websocket connection, an HTTP response, and an error if any. func (t *ChannelSettings) Dial() (*websocket.Conn, *http.Response, error) { - return t.Dialer().Dial(t.wsURL, t.Header) + return t.Dialer().Dial(t.WsURL, t.Header) } // Validate checks if the ChannelSettings instance is valid. @@ -133,7 +133,7 @@ func (t *ChannelSettings) IsEqual(other *ChannelSettings) bool { } } - return t.wsURL == other.wsURL && + return t.WsURL == other.WsURL && t.CAPem == other.CAPem && t.MaxSessionTime == other.MaxSessionTime } diff --git a/workhorse/internal/channel/auth_checker_test.go b/workhorse/internal/channel/auth_checker_test.go index b522cf5da1fff3..36a45b07c1e2c0 100644 --- a/workhorse/internal/channel/auth_checker_test.go +++ b/workhorse/internal/channel/auth_checker_test.go @@ -19,7 +19,7 @@ func checkerSeries(values ...*api.ChannelSettings) AuthCheckerFunc { } func TestAuthCheckerStopsWhenAuthFails(t *testing.T) { - template := &api.ChannelSettings{Url: "ws://example.com"} + template := &api.ChannelSettings{WsURL: "ws://example.com"} stopCh := make(chan error) series := checkerSeries(template, template, template) ac := NewAuthChecker(series, template, stopCh) @@ -35,9 +35,9 @@ func TestAuthCheckerStopsWhenAuthFails(t *testing.T) { } func TestAuthCheckerStopsWhenAuthChanges(t *testing.T) { - template := &api.ChannelSettings{Url: "ws://example.com"} + template := &api.ChannelSettings{WsURL: "ws://example.com"} changed := template.Clone() - changed.Url = "wss://example.com" + changed.WsURL = "wss://example.com" stopCh := make(chan error) series := checkerSeries(template, changed, template) ac := NewAuthChecker(series, template, stopCh) -- GitLab From 0cebeb2814ad295f22589d4c46435ffb032f103e Mon Sep 17 00:00:00 2001 From: Archish Date: Tue, 14 May 2024 14:57:06 +0530 Subject: [PATCH 3/4] Test fixes for channel settings --- workhorse/internal/api/channel_settings_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/workhorse/internal/api/channel_settings_test.go b/workhorse/internal/api/channel_settings_test.go index 37716d8ac95cf2..d33d78222d24eb 100644 --- a/workhorse/internal/api/channel_settings_test.go +++ b/workhorse/internal/api/channel_settings_test.go @@ -9,7 +9,7 @@ import ( func channel(url string, subprotocols ...string) *ChannelSettings { return &ChannelSettings{ - wsURL: url, + WsURL: url, Subprotocols: subprotocols, MaxSessionTime: 0, } @@ -146,7 +146,7 @@ func TestIsEqual(t *testing.T) { {chann, chann, true}, {chann.Clone(), chann.Clone(), true}, {chann, channel("foo:"), false}, - {chann, channel(chann.wsURL), false}, + {chann, channel(chann.WsURL), false}, {header(chann), header(chann), true}, {channHeader2, channHeader2, true}, {channHeader3, channHeader3, true}, -- GitLab From 27fbed07cb13d0ccb753f698fefa60e5dd75c4a7 Mon Sep 17 00:00:00 2001 From: Archish Date: Tue, 14 May 2024 15:02:35 +0530 Subject: [PATCH 4/4] Test fixes for channel settings --- workhorse/cmd/gitlab-workhorse/channel_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/workhorse/cmd/gitlab-workhorse/channel_test.go b/workhorse/cmd/gitlab-workhorse/channel_test.go index 11b414cc526c0d..6e8ad945fd2dc4 100644 --- a/workhorse/cmd/gitlab-workhorse/channel_test.go +++ b/workhorse/cmd/gitlab-workhorse/channel_test.go @@ -174,7 +174,7 @@ func webSocketHandler(upgrader *websocket.Upgrader, connCh chan connWithReq) htt func channelOkBody(remote *httptest.Server, header http.Header, subprotocols ...string) *api.Response { out := &api.Response{ Channel: &api.ChannelSettings{ - Url: websocketURL(remote.URL), + WsURL: websocketURL(remote.URL), Header: header, Subprotocols: subprotocols, MaxSessionTime: 0, -- GitLab