From bc9a41c68cecc2a9e466712835f0570dc9b9d3a1 Mon Sep 17 00:00:00 2001 From: Mark Wolfe Date: Thu, 23 Jan 2025 10:28:38 +1100 Subject: [PATCH 1/5] Introduce http client profile option for custom configuration --- api/client.go | 4 ++++ clicommand/agent_start.go | 12 +++++++----- clicommand/global.go | 12 ++++++++++++ internal/agenthttp/client.go | 18 ++++++++++++++++++ 4 files changed, 41 insertions(+), 5 deletions(-) diff --git a/api/client.go b/api/client.go index 9803437b2d..f4eea09c4a 100644 --- a/api/client.go +++ b/api/client.go @@ -42,6 +42,9 @@ type Config struct { // If true, only HTTP2 is disabled DisableHTTP2 bool + // http client profile to use for the client + HTTPClientProfile string + // If true, requests and responses will be dumped and set to the logger DebugHTTP bool @@ -91,6 +94,7 @@ func NewClient(l logger.Logger, conf Config) *Client { agenthttp.WithAuthToken(conf.Token), agenthttp.WithAllowHTTP2(!conf.DisableHTTP2), agenthttp.WithTLSConfig(conf.TLSConfig), + agenthttp.WithHTTPClientProfile(conf.HTTPClientProfile), ), conf: conf, } diff --git a/clicommand/agent_start.go b/clicommand/agent_start.go index c4633b970e..8f96f64ebb 100644 --- a/clicommand/agent_start.go +++ b/clicommand/agent_start.go @@ -183,11 +183,12 @@ type AgentStartConfig struct { NoMultipartArtifactUpload bool `cli:"no-multipart-artifact-upload"` // API config - DebugHTTP bool `cli:"debug-http"` - TraceHTTP bool `cli:"trace-http"` - Token string `cli:"token" validate:"required"` - Endpoint string `cli:"endpoint" validate:"required"` - NoHTTP2 bool `cli:"no-http2"` + DebugHTTP bool `cli:"debug-http"` + TraceHTTP bool `cli:"trace-http"` + Token string `cli:"token" validate:"required"` + Endpoint string `cli:"endpoint" validate:"required"` + NoHTTP2 bool `cli:"no-http2"` + HTTPClientProfile string `cli:"http-client-profile"` // Deprecated NoSSHFingerprintVerification bool `cli:"no-automatic-ssh-fingerprint-verification" deprecated-and-renamed-to:"NoSSHKeyscan"` @@ -721,6 +722,7 @@ var AgentStartCommand = cli.Command{ NoHTTP2Flag, DebugHTTPFlag, TraceHTTPFlag, + HTTPClientProfileFlag, // Global flags NoColorFlag, diff --git a/clicommand/global.go b/clicommand/global.go index 08902d98b0..cbcbee0938 100644 --- a/clicommand/global.go +++ b/clicommand/global.go @@ -43,6 +43,13 @@ var ( EnvVar: "BUILDKITE_AGENT_ENDPOINT", } + HTTPClientProfileFlag = cli.StringFlag{ + Name: "http-client-profile", + Usage: "Enable a http client profile, either standard, gcp or stdlib", + Value: "standard", + EnvVar: "BUILDKITE_AGENT_HTTP_CLIENT_PROFILE", + } + NoHTTP2Flag = cli.BoolFlag{ Name: "no-http2", Usage: "Disable HTTP2 when communicating with the Agent API.", @@ -309,6 +316,11 @@ func loadAPIClientConfig(cfg any, tokenField string) api.Config { conf.DisableHTTP2 = noHTTP2.(bool) } + httpClientProfile, err := reflections.GetField(cfg, "HTTPClientProfile") + if err == nil { + conf.HTTPClientProfile = httpClientProfile.(string) + } + return conf } diff --git a/internal/agenthttp/client.go b/internal/agenthttp/client.go index 6cead16292..e8d3664457 100644 --- a/internal/agenthttp/client.go +++ b/internal/agenthttp/client.go @@ -26,6 +26,18 @@ func NewClient(opts ...ClientOption) *http.Client { opt(&conf) } + switch conf.HTTPClientProfile { + case "stdlib": + return &http.Client{ + Timeout: conf.Timeout, + Transport: &authenticatedTransport{ + Bearer: conf.Bearer, + Token: conf.Token, + Delegate: http.DefaultTransport, + }, + } + } + cacheKey := transportCacheKey{ AllowHTTP2: conf.AllowHTTP2, TLSConfig: conf.TLSConfig, @@ -65,6 +77,9 @@ func WithAllowHTTP2(a bool) ClientOption { return func(c *clientConfig) { func WithTimeout(d time.Duration) ClientOption { return func(c *clientConfig) { c.Timeout = d } } func WithNoTimeout(c *clientConfig) { c.Timeout = 0 } func WithTLSConfig(t *tls.Config) ClientOption { return func(c *clientConfig) { c.TLSConfig = t } } +func WithHTTPClientProfile(p string) ClientOption { + return func(c *clientConfig) { c.HTTPClientProfile = p } +} type ClientOption = func(*clientConfig) @@ -122,6 +137,9 @@ type clientConfig struct { // optional TLS configuration primarily used for testing TLSConfig *tls.Config + + // HTTPClientProfile profile to use for the http client + HTTPClientProfile string } // The underlying http.Transport is cached, mainly so that multiple clients with From b53db528afe86bb7ae4aeedc9da00efc42fdba8f Mon Sep 17 00:00:00 2001 From: Mark Wolfe Date: Thu, 23 Jan 2025 12:52:32 +1100 Subject: [PATCH 2/5] Add log message to indicate what profile is used and tls config support --- clicommand/agent_start.go | 2 ++ internal/agenthttp/client.go | 11 ++++++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/clicommand/agent_start.go b/clicommand/agent_start.go index 8f96f64ebb..bfdc330411 100644 --- a/clicommand/agent_start.go +++ b/clicommand/agent_start.go @@ -1120,6 +1120,8 @@ var AgentStartCommand = cli.Command{ l.Info("Agents will disconnect after %d seconds of inactivity", agentConf.DisconnectAfterIdleTimeout) } + l.Info("Using http client profile: %s", cfg.HTTPClientProfile) + if len(cfg.AllowedRepositories) > 0 { agentConf.AllowedRepositories = make([]*regexp.Regexp, 0, len(cfg.AllowedRepositories)) for _, v := range cfg.AllowedRepositories { diff --git a/internal/agenthttp/client.go b/internal/agenthttp/client.go index e8d3664457..412cf8e940 100644 --- a/internal/agenthttp/client.go +++ b/internal/agenthttp/client.go @@ -26,14 +26,23 @@ func NewClient(opts ...ClientOption) *http.Client { opt(&conf) } + // http client profile is used to switch between different http client implementations + // - stdlib: uses the standard library http client switch conf.HTTPClientProfile { case "stdlib": + // Base any modifications on the default transport. + transport := http.DefaultTransport.(*http.Transport).Clone() + + if conf.TLSConfig != nil { + transport.TLSClientConfig = conf.TLSConfig + } + return &http.Client{ Timeout: conf.Timeout, Transport: &authenticatedTransport{ Bearer: conf.Bearer, Token: conf.Token, - Delegate: http.DefaultTransport, + Delegate: transport, }, } } From 5b03bac733b6fc65d2b406626abad40e0718222d Mon Sep 17 00:00:00 2001 From: Mark Wolfe Date: Thu, 23 Jan 2025 14:37:30 +1100 Subject: [PATCH 3/5] Validate the selected http client profile and keep a list --- clicommand/agent_start.go | 5 +++++ clicommand/global.go | 4 ++-- internal/agenthttp/client.go | 6 ++++++ 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/clicommand/agent_start.go b/clicommand/agent_start.go index bfdc330411..677920d841 100644 --- a/clicommand/agent_start.go +++ b/clicommand/agent_start.go @@ -26,6 +26,7 @@ import ( "github.com/buildkite/agent/v3/api" "github.com/buildkite/agent/v3/core" "github.com/buildkite/agent/v3/internal/agentapi" + "github.com/buildkite/agent/v3/internal/agenthttp" "github.com/buildkite/agent/v3/internal/awslib" awssigner "github.com/buildkite/agent/v3/internal/cryptosigner/aws" "github.com/buildkite/agent/v3/internal/experiments" @@ -1122,6 +1123,10 @@ var AgentStartCommand = cli.Command{ l.Info("Using http client profile: %s", cfg.HTTPClientProfile) + if !slices.Contains(agenthttp.ValidHTTPClientProfiles, cfg.HTTPClientProfile) { + l.Fatal("HTTP client profile %s is not in list of valid profiles: %v", cfg.HTTPClientProfile, agenthttp.ValidHTTPClientProfiles) + } + if len(cfg.AllowedRepositories) > 0 { agentConf.AllowedRepositories = make([]*regexp.Regexp, 0, len(cfg.AllowedRepositories)) for _, v := range cfg.AllowedRepositories { diff --git a/clicommand/global.go b/clicommand/global.go index cbcbee0938..9d72b47b7a 100644 --- a/clicommand/global.go +++ b/clicommand/global.go @@ -45,8 +45,8 @@ var ( HTTPClientProfileFlag = cli.StringFlag{ Name: "http-client-profile", - Usage: "Enable a http client profile, either standard, gcp or stdlib", - Value: "standard", + Usage: "Enable a http client profile, either default or stdlib", + Value: "default", EnvVar: "BUILDKITE_AGENT_HTTP_CLIENT_PROFILE", } diff --git a/internal/agenthttp/client.go b/internal/agenthttp/client.go index 412cf8e940..01623db1a3 100644 --- a/internal/agenthttp/client.go +++ b/internal/agenthttp/client.go @@ -11,6 +11,10 @@ import ( "golang.org/x/net/http2" ) +var ( + ValidHTTPClientProfiles = []string{"stdlib", "default"} +) + // NewClient creates a HTTP client. Note that the default timeout is 60 seconds; // for some use cases (e.g. artifact operations) use [WithNoTimeout]. func NewClient(opts ...ClientOption) *http.Client { @@ -47,6 +51,8 @@ func NewClient(opts ...ClientOption) *http.Client { } } + // fall back to the default http client profile + cacheKey := transportCacheKey{ AllowHTTP2: conf.AllowHTTP2, TLSConfig: conf.TLSConfig, From 5303dc10fa315e83d3b32f8ea086e84e3cc75d20 Mon Sep 17 00:00:00 2001 From: Mark Wolfe Date: Tue, 28 Jan 2025 16:51:28 +1100 Subject: [PATCH 4/5] Update internal/agenthttp/client.go Better description of the valid http client profiles. Co-authored-by: Josh Deprez --- internal/agenthttp/client.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/agenthttp/client.go b/internal/agenthttp/client.go index 01623db1a3..90120e7555 100644 --- a/internal/agenthttp/client.go +++ b/internal/agenthttp/client.go @@ -12,6 +12,7 @@ import ( ) var ( + // ValidHTTPClientProfiles lists accepted values for Config.HTTPClientProfile. ValidHTTPClientProfiles = []string{"stdlib", "default"} ) From d6787eea6fe8b0fc2ce834a4d1cebb0228f58898 Mon Sep 17 00:00:00 2001 From: Mark Wolfe Date: Fri, 31 Jan 2025 14:07:51 +1100 Subject: [PATCH 5/5] I now ensure unsupported http client options log a fatal error --- clicommand/agent_start.go | 8 ++++++-- internal/agenthttp/client.go | 7 +++++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/clicommand/agent_start.go b/clicommand/agent_start.go index 677920d841..338e90dc8b 100644 --- a/clicommand/agent_start.go +++ b/clicommand/agent_start.go @@ -1123,8 +1123,12 @@ var AgentStartCommand = cli.Command{ l.Info("Using http client profile: %s", cfg.HTTPClientProfile) - if !slices.Contains(agenthttp.ValidHTTPClientProfiles, cfg.HTTPClientProfile) { - l.Fatal("HTTP client profile %s is not in list of valid profiles: %v", cfg.HTTPClientProfile, agenthttp.ValidHTTPClientProfiles) + if !slices.Contains(agenthttp.ValidClientProfiles, cfg.HTTPClientProfile) { + l.Fatal("HTTP client profile %s is not in list of valid profiles: %v", cfg.HTTPClientProfile, agenthttp.ValidClientProfiles) + } + + if cfg.HTTPClientProfile == agenthttp.ClientProfileStdlib && cfg.NoHTTP2 { + l.Fatal("NoHTTP2 is not supported with the standard library (%s) HTTP client profile, use GODEBUG see https://pkg.go.dev/net/http#hdr-HTTP_2", agenthttp.ClientProfileStdlib) } if len(cfg.AllowedRepositories) > 0 { diff --git a/internal/agenthttp/client.go b/internal/agenthttp/client.go index 90120e7555..8447489496 100644 --- a/internal/agenthttp/client.go +++ b/internal/agenthttp/client.go @@ -12,8 +12,11 @@ import ( ) var ( + ClientProfileDefault = "default" + ClientProfileStdlib = "stdlib" + // ValidHTTPClientProfiles lists accepted values for Config.HTTPClientProfile. - ValidHTTPClientProfiles = []string{"stdlib", "default"} + ValidClientProfiles = []string{ClientProfileDefault, ClientProfileStdlib} ) // NewClient creates a HTTP client. Note that the default timeout is 60 seconds; @@ -34,7 +37,7 @@ func NewClient(opts ...ClientOption) *http.Client { // http client profile is used to switch between different http client implementations // - stdlib: uses the standard library http client switch conf.HTTPClientProfile { - case "stdlib": + case ClientProfileStdlib: // Base any modifications on the default transport. transport := http.DefaultTransport.(*http.Transport).Clone()