From 5329f926b64e5cfeffd9146a48d46a66e41bdcdb Mon Sep 17 00:00:00 2001
From: Timothy Messier <tim.messier@gmail.com>
Date: Wed, 8 Jan 2025 10:07:09 -0500
Subject: [PATCH] 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       |  7 ++++
 api/client.go      | 11 +++---
 api/client_test.go | 89 ++++++++++++++++++++++++++++++++++++++++++++++
 api/go.mod         |  1 +
 4 files changed, 102 insertions(+), 6 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index a6e7f247d7..10ee49b3ba 100644
--- a/CHANGELOG.md
+++ b/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.
diff --git a/api/client.go b/api/client.go
index 3a71fd57ee..8cc58c4ad8 100644
--- a/api/client.go
+++ b/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
 }
 
diff --git a/api/client_test.go b/api/client_test.go
index 757aef94b0..cf5022af7c 100644
--- a/api/client_test.go
+++ b/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"),
+			))
+	}
+}
diff --git a/api/go.mod b/api/go.mod
index fd2eed8bde..ec6ee8d787 100644
--- a/api/go.mod
+++ b/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