Merge pull request #40 from Conjur-Enterprise/configurable_timeout

CNJR-7343: Configurable timeout

CHANGELOG.md

@@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Policy dry-run now returns deleted policy resources. (CNJR-6369)
 - Policy dry-run now returns updated policy resources. (CNJR-6111)
 - Policy dry-run now returns create policy resources. (CNJR-6110)
+- HTTP timeout is now configurable. [cyberark/conjur-api-go#150](https://github.com/cyberark/conjur-cli-go/issues/150)
 
 ## [0.12.7] - 2024-11-18
 

conjurapi/client.go

@@ -5,6 +5,7 @@ import (
 	"crypto/x509"
 	"fmt"
 	"io"
+	"net"
 	"net/http"
 	"os"
 	"time"
@@ -241,7 +242,10 @@ func createHttpClient(config Config) (*http.Client, error) {
 			return nil, err
 		}
 	} else {
-		httpClient = &http.Client{Timeout: time.Second * time.Duration(config.GetHttpTimeout())}
+		httpClient = &http.Client{
+			Transport: newHTTPTransport(),
+			Timeout:   time.Second * time.Duration(config.GetHttpTimeout()),
+		}
 	}
 	return httpClient, nil
 }
@@ -264,8 +268,15 @@ func newHTTPSClient(cert []byte, config Config) (*http.Client, error) {
 	}
 	//TODO: Test what happens if this cert is expired
 	//TODO: What if server cert is rotated
-	tr := &http.Transport{
-		TLSClientConfig: &tls.Config{RootCAs: pool},
-	}
+	tr := newHTTPTransport()
+	tr.TLSClientConfig = &tls.Config{RootCAs: pool}
 	return &http.Client{Transport: tr, Timeout: time.Second * time.Duration(config.GetHttpTimeout())}, nil
 }
+
+func newHTTPTransport() *http.Transport {
+	return &http.Transport{
+		DialContext: (&net.Dialer{
+			Timeout: time.Second * time.Duration(HTTPDailTimeout),
+		}).DialContext,
+	}
+}

conjurapi/client_test.go

@@ -486,18 +486,18 @@ func TestClient_HttpClientTimeoutValue(t *testing.T) {
 
 		assert.NoError(t, err)
 		require.NotNil(t, client)
-		assert.Equal(t, time.Second*time.Duration(HttpTimeoutDefaultValue), client.Timeout)
+		assert.Equal(t, time.Second*time.Duration(HTTPTimeoutDefaultValue), client.Timeout)
 	})
-	t.Run("Create HTTP client with no timeout", func(t *testing.T) {
-		config := Config{Account: "account", ApplianceURL: "http://appliance-url", HttpTimeout: -1}
+	t.Run("Create HTTP client with negative timeout", func(t *testing.T) {
+		config := Config{Account: "account", ApplianceURL: "http://appliance-url", HTTPTimeout: -1}
 		client, err := createHttpClient(config)
 
 		assert.NoError(t, err)
 		require.NotNil(t, client)
-		assert.Equal(t, time.Second*time.Duration(0), client.Timeout)
+		assert.Equal(t, time.Second*time.Duration(HTTPTimeoutDefaultValue), client.Timeout)
 	})
 	t.Run("Create HTTP client with specific timeout", func(t *testing.T) {
-		config := Config{Account: "account", ApplianceURL: "http://appliance-url", HttpTimeout: 5}
+		config := Config{Account: "account", ApplianceURL: "http://appliance-url", HTTPTimeout: 5}
 		client, err := createHttpClient(config)
 
 		assert.NoError(t, err)

conjurapi/config.go

@@ -5,6 +5,7 @@ import (
 	"os"
 	"path"
 	"runtime"
+	"strconv"
 	"strings"
 
 	"github.com/sirupsen/logrus"
@@ -14,7 +15,12 @@ import (
 )
 
 const (
-	HttpTimeoutDefaultValue = 10
+	// HTTPTimeoutDefaultValue is the default value for the HTTP client timeout
+	HTTPTimeoutDefaultValue = 60
+	// HTTPTimeoutMaxValue is the maximum value allowed for the HTTP client timeout
+	HTTPTimeoutMaxValue = 600
+	// HTTPDailTimeout is the default value for the DialTimeout in the HTTP client
+	HTTPDailTimeout = 10
 )
 
 var supportedAuthnTypes = []string{"authn", "ldap", "oidc", "jwt"}
@@ -31,7 +37,7 @@ type Config struct {
 	JWTHostID         string `yaml:"jwt_host_id,omitempty"`
 	JWTContent        string `yaml:"-"`
 	JWTFilePath       string `yaml:"jwt_file,omitempty"`
-	HttpTimeout       int    `yaml:"-"`
+	HTTPTimeout       int    `yaml:"http_timeout,omitempty"`
 }
 
 func (c *Config) IsHttps() bool {
@@ -61,6 +67,10 @@ func (c *Config) Validate() error {
 		errors = append(errors, "Must specify a JWT token when using JWT authentication")
 	}
 
+	if c.HTTPTimeout < 0 || c.HTTPTimeout > HTTPTimeoutMaxValue {
+		errors = append(errors, fmt.Sprintf("HTTPTimeout must be between 1 and %d seconds", HTTPTimeoutMaxValue))
+	}
+
 	if len(errors) == 0 {
 		return nil
 	} else if logging.ApiLog.Level == logrus.DebugLevel {
@@ -89,18 +99,19 @@ func (c *Config) BaseURL() string {
 }
 
 // The GetHttpTimeout function retrieves the Timeout value from the config struc.
-// If config.HttpTimeout is
-// - less than 0, GetHttpTimeout returns 0 (no timeout)
-// - equal to 0, GetHttpTimeout returns the default value (constant HttpTimeoutDefaultValue)
-// Otherwise, GetHttpTimeout returns the value of config.HttpTimeout
+// If config.HTTPTimeout is
+// - less than 0, GetHttpTimeout returns the default value (constant HTTPTimeoutDefaultValue)
+// - equal to 0, GetHttpTimeout assumes no value passed and returns the default value (constant HTTPTimeoutDefaultValue)
+// - grater than HTTPTimeoutMaxValue, GetHttpTimeout returns the default value (constant HTTPTimeoutDefaultValue)
+// Otherwise, GetHttpTimeout returns the value of config.HTTPTimeout
 func (c *Config) GetHttpTimeout() int {
 	switch {
-	case c.HttpTimeout < 0:
-		return 0
-	case c.HttpTimeout == 0:
-		return HttpTimeoutDefaultValue
+	case c.HTTPTimeout <= 0:
+		return HTTPTimeoutDefaultValue
+	case c.HTTPTimeout > HTTPTimeoutMaxValue:
+		return HTTPTimeoutDefaultValue
 	default:
-		return c.HttpTimeout
+		return c.HTTPTimeout
 	}
 }
 
@@ -111,6 +122,13 @@ func mergeValue(a, b string) string {
 	return a
 }
 
+func mergeInt(a, b int) int {
+	if b != 0 {
+		return b
+	}
+	return a
+}
+
 func (c *Config) merge(o *Config) {
 	c.ApplianceURL = mergeValue(c.ApplianceURL, o.ApplianceURL)
 	c.Account = mergeValue(c.Account, o.Account)
@@ -123,6 +141,7 @@ func (c *Config) merge(o *Config) {
 	c.JWTHostID = mergeValue(c.JWTHostID, o.JWTHostID)
 	c.JWTContent = mergeValue(c.JWTContent, o.JWTContent)
 	c.JWTFilePath = mergeValue(c.JWTFilePath, o.JWTFilePath)
+	c.HTTPTimeout = mergeInt(c.HTTPTimeout, o.HTTPTimeout)
 }
 
 func (c *Config) mergeYAML(filename string) error {
@@ -184,6 +203,7 @@ func (c *Config) mergeEnv() {
 		JWTContent:        os.Getenv("CONJUR_AUTHN_JWT_TOKEN"),
 		JWTFilePath:       os.Getenv("JWT_TOKEN_PATH"),
 		JWTHostID:         os.Getenv("CONJUR_AUTHN_JWT_HOST_ID"),
+		HTTPTimeout:       httpTimoutFromEnv(),
 	}
 
 	if os.Getenv("CONJUR_AUTHN_JWT_SERVICE_ID") != "" {
@@ -197,6 +217,22 @@ func (c *Config) mergeEnv() {
 	c.merge(&env)
 }
 
+func httpTimoutFromEnv() int {
+	timeoutStr, ok := os.LookupEnv("CONJUR_HTTP_TIMEOUT")
+	if !ok || len(timeoutStr) == 0 {
+		return 0
+	}
+	timeout, err := strconv.Atoi(timeoutStr)
+	if err != nil {
+		logging.ApiLog.Infof(
+			"Could not parse CONJUR_HTTP_TIMEOUT, using default value (%ds): %s",
+			HTTPTimeoutDefaultValue,
+			err)
+		timeout = HTTPTimeoutDefaultValue
+	}
+	return timeout
+}
+
 func (c *Config) applyDefaults() {
 	if isConjurCloudURL(c.ApplianceURL) && c.Account == "" {
 		logging.ApiLog.Info("Detected Conjur Cloud URL, setting 'Account' to 'conjur")
@@ -246,8 +282,8 @@ func LoadConfig() (Config, error) {
 
 func getSystemPath() string {
 	if runtime.GOOS == "windows" {
-		//No way to use SHGetKnownFolderPath()
-		//Hardcoding should be fine for now since CONJURRC is available
+		// No way to use SHGetKnownFolderPath()
+		// Hardcoding should be fine for now since CONJURRC is available
 		return "C:\\windows"
 	} else {
 		return "/etc"

conjurapi/config_test.go

@@ -124,6 +124,46 @@ func TestConfig_Validate(t *testing.T) {
 		assert.Contains(t, errString, "Must specify an ApplianceURL")
 		assert.Contains(t, errString, "config: &{Account:account ApplianceURL: ")
 	})
+
+	t.Run("Validates HTTP timeout", func(t *testing.T) {
+		t.Run("Return error for HTTPTimeout less than 0", func(t *testing.T) {
+			config := Config{
+				Account:      "account",
+				ApplianceURL: "appliance-url",
+				HTTPTimeout:  -1,
+			}
+
+			err := config.Validate()
+			assert.Error(t, err)
+
+			errString := err.Error()
+			assert.Contains(t, errString, "HTTPTimeout must be between 1 and 600 seconds")
+		})
+
+		t.Run("Return error for HTTPTimeout greater than 600", func(t *testing.T) {
+			config := Config{
+				Account:      "account",
+				ApplianceURL: "appliance-url",
+				HTTPTimeout:  601,
+			}
+
+			err := config.Validate()
+			assert.Error(t, err)
+
+			errString := err.Error()
+			assert.Contains(t, errString, "HTTPTimeout must be between 1 and 600 seconds")
+		})
+
+		t.Run("Return error for HTTPTimeout not set", func(t *testing.T) {
+			config := Config{
+				Account:      "account",
+				ApplianceURL: "appliance-url",
+			}
+
+			err := config.Validate()
+			assert.NoError(t, err)
+		})
+	})
 }
 
 func TestConfig_IsHttps(t *testing.T) {
@@ -164,6 +204,7 @@ func TestConfig_LoadFromEnv(t *testing.T) {
 		os.Setenv("CONJUR_AUTHN_TYPE", "ldap")
 		os.Setenv("CONJUR_SERVICE_ID", "service-id")
 		os.Setenv("CONJUR_CREDENTIAL_STORAGE", "keyring")
+		os.Setenv("CONJUR_HTTP_TIMEOUT", "99")
 
 		t.Run("Returns Config loaded with values from env", func(t *testing.T) {
 			config := &Config{}
@@ -175,6 +216,7 @@ func TestConfig_LoadFromEnv(t *testing.T) {
 				AuthnType:         "ldap",
 				ServiceID:         "service-id",
 				CredentialStorage: "keyring",
+				HTTPTimeout:       99,
 			})
 		})
 	})
@@ -377,6 +419,7 @@ appliance_url: test-appliance-url
 			NetRCPath:         "test-netrc-path",
 			SSLCert:           "test-cert",
 			CredentialStorage: "keyring",
+			HTTPTimeout:       100,
 		},
 		expected: `account: test-account
 appliance_url: test-appliance-url
@@ -385,6 +428,7 @@ cert_file: test-cert-path
 authn_type: oidc
 service_id: test-service-id
 credential_storage: keyring
+http_timeout: 100
 `,
 	},
 }
@@ -492,24 +536,29 @@ func TestConfig_GetHttpTimeout(t *testing.T) {
 		{
 			name:                "smaller than zero",
 			configHttpTimeout:   -1,
-			expectedHttpTimeout: 0,
+			expectedHttpTimeout: HTTPTimeoutDefaultValue,
 		},
 		{
 			name:                "equal to zero",
 			configHttpTimeout:   0,
-			expectedHttpTimeout: HttpTimeoutDefaultValue,
+			expectedHttpTimeout: HTTPTimeoutDefaultValue,
 		},
 		{
 			name:                "greater then zero",
 			configHttpTimeout:   5,
 			expectedHttpTimeout: 5,
 		},
+		{
+			name:                "greater than max",
+			configHttpTimeout:   HTTPTimeoutMaxValue + 1,
+			expectedHttpTimeout: HTTPTimeoutDefaultValue,
+		},
 	}
 
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {
 			config := Config{
-				HttpTimeout: testCase.configHttpTimeout,
+				HTTPTimeout: testCase.configHttpTimeout,
 			}
 
 			assert.Equal(t, testCase.expectedHttpTimeout, config.GetHttpTimeout())