fix(api): Correctly apply BOUNDARY_MAX_RETRIES env var (#5385)

The api.Client was first checking for the environment variable, and then
resetting the max retries to 2, thus overriding the environment variable
value. In addition, the cli would set max retries to zero if the
environment variable was not set. This would mean that by default the
max retries would be zero, and if the environment variable was set, the
max retries would be 2, regardless of the actual value for the
environment variable.

This fixes the order in which the environment variables are read, to
ensure they take precedence.

CHANGELOG.md

@@ -4,6 +4,13 @@ Canonical reference for changes, improvements, and bugfixes for Boundary.
 
 ## Next
 
+### Bug fixes
+
+* Fix bug in applying BOUNDARY_MAX_RETRIES for boundary cli. Previously
+  setting this environment variable would result in a max retries of 2,
+  regardless of the value set.
+  ([PR](https://github.com/hashicorp/boundary/pull/5385)).
+
 ### New and Improved
 
 * Introduces soft-delete for users within the client cache.

api/client.go

@@ -172,12 +172,6 @@ func DefaultConfig() (*Config, error) {
 		TLSConfig:  &TLSConfig{},
 	}
 
-	// We read the environment now; after DefaultClient returns we can override
-	// values from command line flags, which should take precedence.
-	if err := config.ReadEnvironment(); err != nil {
-		return config, fmt.Errorf("failed to read environment: %w", err)
-	}
-
 	transport := config.HttpClient.Transport.(*http.Transport)
 	transport.TLSHandshakeTimeout = 10 * time.Second
 	transport.TLSClientConfig = &tls.Config{
@@ -188,6 +182,11 @@ func DefaultConfig() (*Config, error) {
 	config.MaxRetries = 2
 	config.Headers = make(http.Header)
 
+	// Read from environment last to ensure it takes precedence.
+	if err := config.ReadEnvironment(); err != nil {
+		return config, fmt.Errorf("failed to read environment: %w", err)
+	}
+
 	return config, nil
 }
 

api/client_test.go

@@ -4,11 +4,17 @@
 package api
 
 import (
+	"crypto/tls"
 	"math"
+	"net/http"
 	"os"
 	"strconv"
 	"testing"
+	"time"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	"github.com/hashicorp/go-cleanhttp"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -157,3 +163,86 @@ func TestReadEnvironmentMaxRetries(t *testing.T) {
 		})
 	}
 }
+
+func TestDefaultConfig(t *testing.T) {
+	tests := []struct {
+		name    string
+		envvars map[string]string
+		want    *Config
+	}{
+		{
+			name:    "noEnvVarsSet",
+			envvars: nil,
+			want: &Config{
+				Addr:               "http://127.0.0.1:9200",
+				Token:              "",
+				RecoveryKmsWrapper: nil,
+				HttpClient: func() *http.Client {
+					client := cleanhttp.DefaultPooledClient()
+					client.Transport.(*http.Transport).TLSClientConfig = &tls.Config{
+						MinVersion: tls.VersionTLS12,
+					}
+					return client
+				}(),
+				TLSConfig:        &TLSConfig{},
+				Headers:          map[string][]string{},
+				MaxRetries:       2,
+				Timeout:          time.Second * 60,
+				Backoff:          RateLimitLinearJitterBackoff,
+				CheckRetry:       nil,
+				Limiter:          nil,
+				OutputCurlString: false,
+				SRVLookup:        false,
+			},
+		},
+		{
+			name: "maxRetries",
+			envvars: map[string]string{
+				"BOUNDARY_MAX_RETRIES": "5",
+			},
+			want: &Config{
+				Addr:               "http://127.0.0.1:9200",
+				Token:              "",
+				RecoveryKmsWrapper: nil,
+				HttpClient: func() *http.Client {
+					client := cleanhttp.DefaultPooledClient()
+					client.Transport.(*http.Transport).TLSClientConfig = &tls.Config{
+						MinVersion: tls.VersionTLS12,
+					}
+					return client
+				}(),
+				TLSConfig:        &TLSConfig{},
+				Headers:          map[string][]string{},
+				MaxRetries:       5,
+				Timeout:          time.Second * 60,
+				Backoff:          RateLimitLinearJitterBackoff,
+				CheckRetry:       nil,
+				Limiter:          nil,
+				OutputCurlString: false,
+				SRVLookup:        false,
+			},
+		},
+	}
+	for _, tt := range tests {
+		for k, v := range tt.envvars {
+			os.Setenv(k, v)
+		}
+		t.Cleanup(func() {
+			for k := range tt.envvars {
+				os.Unsetenv(k)
+			}
+		})
+
+		c, err := DefaultConfig()
+		require.NoError(t, err)
+
+		assert.Empty(t,
+			cmp.Diff(tt.want, c,
+				cmpopts.IgnoreUnexported(http.Transport{}, tls.Config{}),
+				// Ignore fields that are functions, since cmp.Diff can't
+				// correctly compare them if they are non-nil.
+				cmpopts.IgnoreFields(Config{}, "Backoff"),
+				cmpopts.IgnoreFields(http.Transport{}, "Proxy", "DialContext"),
+			))
+	}
+}

api/go.mod

@@ -3,6 +3,7 @@ module github.com/hashicorp/boundary/api
 go 1.23.3
 
 require (
+	github.com/google/go-cmp v0.6.0
 	github.com/hashicorp/boundary/sdk v0.0.48
 	github.com/hashicorp/go-cleanhttp v0.5.2
 	github.com/hashicorp/go-kms-wrapping/v2 v2.0.16