[Bugfix] Fix HTTP Client NPE (#1683)

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

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
 	}

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

pkg/util/http/client_mod.go

@@ -23,35 +23,24 @@ package http
 import (
 	"crypto/tls"
 	"net/http"
+
+	"github.com/arangodb/kube-arangodb/pkg/util/mod"
 )
 
-type Mod[T any] func(in T)
-
-type TransportMod Mod[*http.Transport]
-
-type TransportTLSMod Mod[*tls.Config]
-
-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...)
 	}
 }

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)
+	})
+}

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)
+		}
+	}
+}