Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce http client profile option for custom configuration #3165

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions api/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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,
}
Expand Down
23 changes: 18 additions & 5 deletions clicommand/agent_start.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -180,11 +181,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"`
Expand Down Expand Up @@ -702,6 +704,7 @@ var AgentStartCommand = cli.Command{
NoHTTP2Flag,
DebugHTTPFlag,
TraceHTTPFlag,
HTTPClientProfileFlag,

// Global flags
NoColorFlag,
Expand Down Expand Up @@ -1099,6 +1102,16 @@ 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 !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 {
agentConf.AllowedRepositories = make([]*regexp.Regexp, 0, len(cfg.AllowedRepositories))
for _, v := range cfg.AllowedRepositories {
Expand Down
12 changes: 12 additions & 0 deletions clicommand/global.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,13 @@ var (
EnvVar: "BUILDKITE_AGENT_ENDPOINT",
}

HTTPClientProfileFlag = cli.StringFlag{
Name: "http-client-profile",
Usage: "Enable a http client profile, either default or stdlib",
Value: "default",
EnvVar: "BUILDKITE_AGENT_HTTP_CLIENT_PROFILE",
}

NoHTTP2Flag = cli.BoolFlag{
Name: "no-http2",
Usage: "Disable HTTP2 when communicating with the Agent API.",
Expand Down Expand Up @@ -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
}

Expand Down
37 changes: 37 additions & 0 deletions internal/agenthttp/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,14 @@ import (
"golang.org/x/net/http2"
)

var (
ClientProfileDefault = "default"
ClientProfileStdlib = "stdlib"

// ValidHTTPClientProfiles lists accepted values for Config.HTTPClientProfile.
ValidClientProfiles = []string{ClientProfileDefault, ClientProfileStdlib}
)

// 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 {
Expand All @@ -26,6 +34,29 @@ 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 ClientProfileStdlib:
// Base any modifications on the default transport.
transport := http.DefaultTransport.(*http.Transport).Clone()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell https://go-review.googlesource.com/c/net/+/617655 is merged but not released. So using the default option would re-introduce the bug we fixed in #3005?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@patrobinson Given 1.24 is coming out soon, with 1.24rc3 coming out a few days ago, I think I am going to wait rather than add this workaround.

I have confirmed this fix is bundled with 1.24 in https://github.com/golang/go/blob/release-branch.go1.24/src/net/http/h2_bundle.go#L7834-L7840 🤞🏻.


if conf.TLSConfig != nil {
transport.TLSClientConfig = conf.TLSConfig
}

return &http.Client{
Timeout: conf.Timeout,
Transport: &authenticatedTransport{
Bearer: conf.Bearer,
Token: conf.Token,
Delegate: transport,
},
}
}

// fall back to the default http client profile

cacheKey := transportCacheKey{
AllowHTTP2: conf.AllowHTTP2,
TLSConfig: conf.TLSConfig,
Expand Down Expand Up @@ -65,6 +96,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)

Expand Down Expand Up @@ -122,6 +156,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
Expand Down