Skip to content

Commit

Permalink
[Bugfix] Fix HTTP Client NPE (#1683)
Browse files Browse the repository at this point in the history
  • Loading branch information
ajanikow authored Jul 19, 2024
1 parent dc9782a commit 84e454c
Show file tree
Hide file tree
Showing 6 changed files with 203 additions and 43 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 7 additions & 5 deletions pkg/util/http/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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...)

Expand All @@ -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
}
Expand Down
46 changes: 25 additions & 21 deletions pkg/util/http/client_configuration.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
23 changes: 6 additions & 17 deletions pkg/util/http/client_mod.go
Original file line number Diff line number Diff line change
Expand Up @@ -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...)
}
}
133 changes: 133 additions & 0 deletions pkg/util/http/client_test.go
Original file line number Diff line number Diff line change
@@ -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)
})
}
31 changes: 31 additions & 0 deletions pkg/util/mod/mod.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
}

0 comments on commit 84e454c

Please sign in to comment.