From f881ffcd1e28371def1cc24a41690f114b0c204b Mon Sep 17 00:00:00 2001 From: ajanikow <12255597+ajanikow@users.noreply.github.com> Date: Fri, 19 Jul 2024 09:53:54 +0000 Subject: [PATCH] [Bugfix] Fix HTTP Client NPE --- CHANGELOG.md | 1 + pkg/util/http/client.go | 12 ++- pkg/util/http/client_configuration.go | 46 +++++---- pkg/util/http/client_mod.go | 23 ++--- pkg/util/http/client_test.go | 133 ++++++++++++++++++++++++++ pkg/util/mod/mod.go | 31 ++++++ 6 files changed, 203 insertions(+), 43 deletions(-) create mode 100644 pkg/util/http/client_test.go create mode 100644 pkg/util/mod/mod.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 98b64f0fd..a2d2e6d01 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - (Bugfix) Adjust Prometheus Monitor labels - (Feature) Expose HTTP Client Config - (Bugfix) MarkedToRemove Condition Check +- (Bugfix) Fix HTTP Client NPE ## [1.2.41](https://github.com/arangodb/kube-arangodb/tree/1.2.41) (2024-05-24) - (Maintenance) Bump Prometheus API Version diff --git a/pkg/util/http/client.go b/pkg/util/http/client.go index b787253b6..a03ca7df1 100644 --- a/pkg/util/http/client.go +++ b/pkg/util/http/client.go @@ -24,18 +24,20 @@ import ( "crypto/tls" "crypto/x509" "net/http" + + "github.com/arangodb/kube-arangodb/pkg/util/mod" ) -func RoundTripper(mods ...TransportMod) http.RoundTripper { - df := append([]TransportMod{ +func RoundTripper(mods ...mod.Mod[*http.Transport]) http.RoundTripper { + df := append([]mod.Mod[*http.Transport]{ configuration.DefaultTransport, }, mods...) return Transport(df...) } -func RoundTripperWithShortTransport(mods ...TransportMod) http.RoundTripper { - df := append([]TransportMod{ +func RoundTripperWithShortTransport(mods ...mod.Mod[*http.Transport]) http.RoundTripper { + df := append([]mod.Mod[*http.Transport]{ configuration.ShortTransport, }, mods...) @@ -46,7 +48,7 @@ func Insecure(in *tls.Config) { in.InsecureSkipVerify = true } -func WithRootCA(ca *x509.CertPool) TransportTLSMod { +func WithRootCA(ca *x509.CertPool) mod.Mod[*tls.Config] { return func(in *tls.Config) { in.RootCAs = ca } diff --git a/pkg/util/http/client_configuration.go b/pkg/util/http/client_configuration.go index 826198d37..60e82b679 100644 --- a/pkg/util/http/client_configuration.go +++ b/pkg/util/http/client_configuration.go @@ -47,19 +47,23 @@ const ( defaultTransportExpectContinueTimeout = 1 * time.Second ) -var configuration = configurationObject{ - TransportKeepAlive: defaultTransportKeepAlive, - TransportForceAttemptHTTP2: defaultTransportForceAttemptHTTP2, - TransportMaxIdleConns: defaultTransportMaxIdleConns, - TransportDialTimeout: defaultTransportDialTimeout, - TransportKeepAliveTimeout: defaultTransportKeepAliveTimeout, - TransportKeepAliveTimeoutShort: defaultTransportKeepAliveTimeoutShort, - TransportIdleConnTimeout: defaultTransportIdleConnTimeout, - TransportIdleConnTimeoutShort: defaultTransportIdleConnTimeoutShort, - TransportTLSHandshakeTimeout: defaultTransportTLSHandshakeTimeout, - TransportExpectContinueTimeout: defaultTransportExpectContinueTimeout, +func newConfiguration() configurationObject { + return configurationObject{ + TransportKeepAlive: defaultTransportKeepAlive, + TransportForceAttemptHTTP2: defaultTransportForceAttemptHTTP2, + TransportMaxIdleConns: defaultTransportMaxIdleConns, + TransportDialTimeout: defaultTransportDialTimeout, + TransportKeepAliveTimeout: defaultTransportKeepAliveTimeout, + TransportKeepAliveTimeoutShort: defaultTransportKeepAliveTimeoutShort, + TransportIdleConnTimeout: defaultTransportIdleConnTimeout, + TransportIdleConnTimeoutShort: defaultTransportIdleConnTimeoutShort, + TransportTLSHandshakeTimeout: defaultTransportTLSHandshakeTimeout, + TransportExpectContinueTimeout: defaultTransportExpectContinueTimeout, + } } +var configuration = newConfiguration() + type configurationObject struct { TransportKeepAlive bool TransportForceAttemptHTTP2 bool @@ -82,19 +86,19 @@ func (c *configurationObject) Init(cmd *cobra.Command) error { f := cmd.PersistentFlags() - f.BoolVar(&configuration.TransportKeepAlive, "http1.keep-alive", defaultTransportKeepAlive, "If false, disables HTTP keep-alives and will only use the connection to the server for a single HTTP request") - f.BoolVar(&configuration.TransportForceAttemptHTTP2, "http1.force-attempt-http2", defaultTransportForceAttemptHTTP2, "controls whether HTTP/2 is enabled") + f.BoolVar(&c.TransportKeepAlive, "http1.keep-alive", defaultTransportKeepAlive, "If false, disables HTTP keep-alives and will only use the connection to the server for a single HTTP request") + f.BoolVar(&c.TransportForceAttemptHTTP2, "http1.force-attempt-http2", defaultTransportForceAttemptHTTP2, "controls whether HTTP/2 is enabled") - f.IntVar(&configuration.TransportMaxIdleConns, "http1.transport.max-idle-conns", defaultTransportMaxIdleConns, "Maximum number of idle (keep-alive) connections across all hosts. Zero means no limit") + f.IntVar(&c.TransportMaxIdleConns, "http1.transport.max-idle-conns", defaultTransportMaxIdleConns, "Maximum number of idle (keep-alive) connections across all hosts. Zero means no limit") - f.DurationVar(&configuration.TransportDialTimeout, "http1.transport.dial-timeout", defaultTransportDialTimeout, "Maximum amount of time a dial will wait for a connect to complete") - f.DurationVar(&configuration.TransportKeepAliveTimeout, "http1.transport.keep-alive-timeout", defaultTransportKeepAliveTimeout, "Interval between keep-alive probes for an active network connection") - f.DurationVar(&configuration.TransportKeepAliveTimeoutShort, "http1.transport.keep-alive-timeout-short", defaultTransportKeepAliveTimeoutShort, "Interval between keep-alive probes for an active network connection") + f.DurationVar(&c.TransportDialTimeout, "http1.transport.dial-timeout", defaultTransportDialTimeout, "Maximum amount of time a dial will wait for a connect to complete") + f.DurationVar(&c.TransportKeepAliveTimeout, "http1.transport.keep-alive-timeout", defaultTransportKeepAliveTimeout, "Interval between keep-alive probes for an active network connection") + f.DurationVar(&c.TransportKeepAliveTimeoutShort, "http1.transport.keep-alive-timeout-short", defaultTransportKeepAliveTimeoutShort, "Interval between keep-alive probes for an active network connection") - f.DurationVar(&configuration.TransportIdleConnTimeout, "http1.transport.idle-conn-timeout", defaultTransportIdleConnTimeout, "Maximum amount of time an idle (keep-alive) connection will remain idle before closing itself. Zero means no limit") - f.DurationVar(&configuration.TransportIdleConnTimeoutShort, "http1.transport.idle-conn-timeout-short", defaultTransportIdleConnTimeoutShort, "Maximum amount of time an idle (keep-alive) connection will remain idle before closing itself. Zero means no limit") - f.DurationVar(&configuration.TransportTLSHandshakeTimeout, "http1.transport.tls-handshake-timeout", defaultTransportTLSHandshakeTimeout, "Maximum amount of time to wait for a TLS handshake. Zero means no timeout") - f.DurationVar(&configuration.TransportExpectContinueTimeout, "http1.transport.except-continue-timeout", defaultTransportExpectContinueTimeout, "") + f.DurationVar(&c.TransportIdleConnTimeout, "http1.transport.idle-conn-timeout", defaultTransportIdleConnTimeout, "Maximum amount of time an idle (keep-alive) connection will remain idle before closing itself. Zero means no limit") + f.DurationVar(&c.TransportIdleConnTimeoutShort, "http1.transport.idle-conn-timeout-short", defaultTransportIdleConnTimeoutShort, "Maximum amount of time an idle (keep-alive) connection will remain idle before closing itself. Zero means no limit") + f.DurationVar(&c.TransportTLSHandshakeTimeout, "http1.transport.tls-handshake-timeout", defaultTransportTLSHandshakeTimeout, "Maximum amount of time to wait for a TLS handshake. Zero means no timeout") + f.DurationVar(&c.TransportExpectContinueTimeout, "http1.transport.except-continue-timeout", defaultTransportExpectContinueTimeout, "") if err := f.MarkHidden("http1.transport.except-continue-timeout"); err != nil { return err diff --git a/pkg/util/http/client_mod.go b/pkg/util/http/client_mod.go index c16c9beb5..b69cd1c15 100644 --- a/pkg/util/http/client_mod.go +++ b/pkg/util/http/client_mod.go @@ -23,35 +23,24 @@ package http import ( "crypto/tls" "net/http" -) - -type Mod[T any] func(in T) -type TransportMod Mod[*http.Transport] - -type TransportTLSMod Mod[*tls.Config] + "github.com/arangodb/kube-arangodb/pkg/util/mod" +) -func Transport(mods ...TransportMod) http.RoundTripper { +func Transport(mods ...mod.Mod[*http.Transport]) http.RoundTripper { var c http.Transport - for _, m := range mods { - if m == nil { - continue - } - m(&c) - } + mod.Exec[*http.Transport](&c, mods...) return &c } -func WithTransportTLS(mods ...TransportTLSMod) TransportMod { +func WithTransportTLS(mods ...mod.Mod[*tls.Config]) mod.Mod[*http.Transport] { return func(in *http.Transport) { if in.TLSClientConfig == nil { in.TLSClientConfig = &tls.Config{} } - for _, mod := range mods { - mod(in.TLSClientConfig) - } + mod.Exec(in.TLSClientConfig, mods...) } } diff --git a/pkg/util/http/client_test.go b/pkg/util/http/client_test.go new file mode 100644 index 000000000..0fe1edabd --- /dev/null +++ b/pkg/util/http/client_test.go @@ -0,0 +1,133 @@ +// +// DISCLAIMER +// +// Copyright 2024 ArangoDB GmbH, Cologne, Germany +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// Copyright holder is ArangoDB GmbH, Cologne, Germany +// + +package http + +import ( + "net/http" + "testing" + "time" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/require" +) + +func resetConfig() { + configuration = newConfiguration() +} + +func execCommand(t *testing.T, args ...string) configurationObject { + config := newConfiguration() + + cmd := &cobra.Command{ + Run: func(cmd *cobra.Command, args []string) { + + }, + } + + require.NoError(t, config.Init(cmd)) + + cmd.SetArgs(args) + + require.NoError(t, cmd.Execute()) + + return config +} + +func Test_ClientSettings(t *testing.T) { + t.Run("Ensure nil function is handled", func(t *testing.T) { + defer resetConfig() + + require.NotNil(t, RoundTripper()) + }) + + t.Run("Ensure default settings", func(t *testing.T) { + defer resetConfig() + + def := newConfiguration() + + evaluated := execCommand(t) + + require.Equal(t, def, evaluated) + }) + + t.Run("Ensure default settings overriden", func(t *testing.T) { + defer resetConfig() + + def := newConfiguration() + + evaluated := execCommand(t, "--http1.keep-alive=false") + + require.NotEqual(t, def, evaluated) + + evaluated.TransportKeepAlive = true + require.Equal(t, def, evaluated) + }) + + t.Run("Ensure normal client", func(t *testing.T) { + evaluated := execCommand(t) + + transport := Transport(evaluated.DefaultTransport) + + c, ok := transport.(*http.Transport) + require.True(t, ok) + + require.Equal(t, c.DisableKeepAlives, !evaluated.TransportKeepAlive) + require.Equal(t, c.IdleConnTimeout, evaluated.TransportIdleConnTimeout) + }) + + t.Run("Ensure short client", func(t *testing.T) { + evaluated := execCommand(t) + + transport := Transport(evaluated.ShortTransport) + + c, ok := transport.(*http.Transport) + require.True(t, ok) + + require.Equal(t, c.DisableKeepAlives, !evaluated.TransportKeepAlive) + require.Equal(t, c.IdleConnTimeout, evaluated.TransportIdleConnTimeoutShort) + }) + + t.Run("Ensure normal client with mods", func(t *testing.T) { + evaluated := execCommand(t, "--http1.transport.idle-conn-timeout=1h") + + transport := Transport(evaluated.DefaultTransport) + + c, ok := transport.(*http.Transport) + require.True(t, ok) + + require.Equal(t, c.DisableKeepAlives, !evaluated.TransportKeepAlive) + require.Equal(t, c.IdleConnTimeout, evaluated.TransportIdleConnTimeout) + require.Equal(t, c.IdleConnTimeout, time.Hour) + }) + + t.Run("Ensure short client with mods", func(t *testing.T) { + evaluated := execCommand(t, "--http1.transport.idle-conn-timeout-short=6h") + + transport := Transport(evaluated.ShortTransport) + + c, ok := transport.(*http.Transport) + require.True(t, ok) + + require.Equal(t, c.DisableKeepAlives, !evaluated.TransportKeepAlive) + require.Equal(t, c.IdleConnTimeout, evaluated.TransportIdleConnTimeoutShort) + require.Equal(t, c.IdleConnTimeout, 6*time.Hour) + }) +} diff --git a/pkg/util/mod/mod.go b/pkg/util/mod/mod.go new file mode 100644 index 000000000..2d5ba3944 --- /dev/null +++ b/pkg/util/mod/mod.go @@ -0,0 +1,31 @@ +// +// DISCLAIMER +// +// Copyright 2024 ArangoDB GmbH, Cologne, Germany +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// Copyright holder is ArangoDB GmbH, Cologne, Germany +// + +package mod + +type Mod[T any] func(in T) + +func Exec[T any](in T, mods ...Mod[T]) { + for _, mod := range mods { + if mod != nil { + mod(in) + } + } +}