refactor(controller): replace http step timeoutSeconds with timeout (a…

…kuity#3066)

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>

docs/docs/35-references/10-promotion-steps.md

@@ -1301,7 +1301,7 @@ with a wide variety of external services.
 | `queryParams[].value` | `string` | Y | The value of the query parameter. The provided value will automatically be URL-encoded if necessary. |
 | `body` | `string` | N | The body of the request. |
 | `insecureSkipTLSVerify` | `boolean` | N | Indicates whether to bypass TLS certificate verification when making the request. Setting this to `true` is highly discouraged. |
-| `timeoutSeconds` | `number` | N | The maximum number of seconds to wait for a request to complete. If this is not specified, the default timeout is 10 seconds. The minimum permissible value is `1` and the maximum is `60`. _This is the timeout for an individual HTTP request. If a request is retried, each attempt is independently subject to this timeout._ |
+| `timeout` | `string` | N | A string representation of the maximum time interval to wait for a request to complete. _This is the timeout for an individual HTTP request. If a request is retried, each attempt is independently subject to this timeout._ See Go's [`time` package docs](https://pkg.go.dev/time#ParseDuration) for a description of the accepted format. |
 | `successExpression` | `string` | N | An [expr-lang] expression that can evaluate the response to determine success. If this is left undefined and `failureExpression` _is_ defined, the default success criteria will be the inverse of the specified failure criteria. If both are left undefined, success is `true` when the HTTP status code is `2xx`. If `successExpression` and `failureExpression` are both defined and both evaluate to `true`, the failure takes precedence. Note that this expression should _not_ be offset by `${{` and `}}`. See examples for more details. |
 | `failureExpression` | `string` | N | An [expr-lang] expression that can evaluate the response to determine failure. If this is left undefined and `successExpression` _is_ defined, the default failure criteria will be the inverse of the specified success criteria. If both are left undefined, failure is `true` when the HTTP status code is _not_ `2xx`. If `successExpression` and `failureExpression` are both defined and both evaluate to `true`, the failure takes precedence. Note that this expression should _not_ be offset by `${{` and `}}`. See examples for more details. |
 | `outputs` | `[]object` | N | A list of rules for extracting outputs from the HTTP response. These are only applied to responses deemed successful. |

internal/directives/http_requester.go

@@ -76,7 +76,12 @@ func (h *httpRequester) runPromotionStep(
 		return PromotionStepResult{Status: kargoapi.PromotionPhaseErrored},
 			fmt.Errorf("error building HTTP request: %w", err)
 	}
-	resp, err := h.getClient(cfg).Do(req)
+	client, err := h.getClient(cfg)
+	if err != nil {
+		return PromotionStepResult{Status: kargoapi.PromotionPhaseErrored},
+			fmt.Errorf("error creating HTTP client: %w", err)
+	}
+	resp, err := client.Do(req)
 	if err != nil {
 		return PromotionStepResult{Status: kargoapi.PromotionPhaseErrored},
 			fmt.Errorf("error sending HTTP request: %w", err)
@@ -138,23 +143,25 @@ func (h *httpRequester) buildRequest(cfg HTTPConfig) (*http.Request, error) {
 	return req, nil
 }
 
-func (h *httpRequester) getClient(cfg HTTPConfig) *http.Client {
+func (h *httpRequester) getClient(cfg HTTPConfig) (*http.Client, error) {
 	httpTransport := cleanhttp.DefaultTransport()
 	if cfg.InsecureSkipTLSVerify {
 		httpTransport.TLSClientConfig = &tls.Config{
 			InsecureSkipVerify: true, // nolint: gosec
 		}
 	}
-	var timeoutSeconds int64
-	if cfg.TimeoutSeconds != nil {
-		timeoutSeconds = *cfg.TimeoutSeconds
-	} else {
-		timeoutSeconds = 10
+	timeout := 10 * time.Second
+	if cfg.Timeout != "" {
+		var err error
+		if timeout, err = time.ParseDuration(cfg.Timeout); err != nil {
+			// Input is validated, so this really should not happen
+			return nil, fmt.Errorf("error parsing timeout: %w", err)
+		}
 	}
 	return &http.Client{
 		Transport: httpTransport,
-		Timeout:   time.Duration(timeoutSeconds) * time.Second,
-	}
+		Timeout:   timeout,
+	}, nil
 }
 
 func (h *httpRequester) buildExprEnv(resp *http.Response) (map[string]any, error) {

internal/directives/http_requester_test.go

@@ -125,21 +125,12 @@ func Test_httpRequester_validate(t *testing.T) {
 			},
 		},
 		{
-			name: "timeoutSeconds < 1",
+			name: "invalid timeout",
 			config: Config{
-				"timeoutSeconds": 0,
+				"timeout": "invalid",
 			},
 			expectedProblems: []string{
-				"timeoutSeconds: Must be greater than or equal to 1",
-			},
-		},
-		{
-			name: "timeoutSeconds > 60",
-			config: Config{
-				"timeoutSeconds": 61,
-			},
-			expectedProblems: []string{
-				"timeoutSeconds: Must be less than or equal to 60",
+				"timeout: Does not match pattern",
 			},
 		},
 		{
@@ -182,6 +173,35 @@ func Test_httpRequester_validate(t *testing.T) {
 				"outputs.0.fromExpression: String length must be greater than or equal to 1",
 			},
 		},
+		{
+			name: "valid kitchen sink",
+			config: Config{
+				"method": "GET",
+				"url":    "https://example.com",
+				"headers": []Config{{
+					"name":  "Accept",
+					"value": "application/json",
+				}},
+				"queryParams": []Config{{
+					"name":  "foo",
+					"value": "bar",
+				}},
+				"insecureSkipTLSVerify": true,
+				"timeout":               "30s",
+				"successExpression":     "response.status == 200",
+				"failureExpression":     "response.status == 404",
+				"outputs": []Config{
+					{
+						"name":           "fact1",
+						"fromExpression": "response.body.facts[0]",
+					},
+					{
+						"name":           "fact2",
+						"fromExpression": "response.body.facts[1]",
+					},
+				},
+			},
+		},
 	}
 
 	r := newHTTPRequester()
@@ -381,11 +401,12 @@ func Test_httpRequester_getClient(t *testing.T) {
 	testCases := []struct {
 		name       string
 		cfg        HTTPConfig
-		assertions func(*testing.T, *http.Client)
+		assertions func(*testing.T, *http.Client, error)
 	}{
 		{
 			name: "without insecureSkipTLSVerify",
-			assertions: func(t *testing.T, client *http.Client) {
+			assertions: func(t *testing.T, client *http.Client, err error) {
+				require.NoError(t, err)
 				require.NotNil(t, client)
 				transport, ok := client.Transport.(*http.Transport)
 				require.True(t, ok)
@@ -397,20 +418,30 @@ func Test_httpRequester_getClient(t *testing.T) {
 			cfg: HTTPConfig{
 				InsecureSkipTLSVerify: true,
 			},
-			assertions: func(t *testing.T, client *http.Client) {
+			assertions: func(t *testing.T, client *http.Client, err error) {
+				require.NoError(t, err)
 				require.NotNil(t, client)
 				transport, ok := client.Transport.(*http.Transport)
 				require.True(t, ok)
 				require.NotNil(t, transport.TLSClientConfig)
 				require.True(t, transport.TLSClientConfig.InsecureSkipVerify)
 			},
 		},
+		{
+			name: "with invalid timeout",
+			cfg: HTTPConfig{
+				Timeout: "invalid",
+			},
+			assertions: func(t *testing.T, _ *http.Client, err error) {
+				require.ErrorContains(t, err, "error parsing timeout")
+			},
+		},
 	}
 	h := &httpRequester{}
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {
-			client := h.getClient(testCase.cfg)
-			testCase.assertions(t, client)
+			client, err := h.getClient(testCase.cfg)
+			testCase.assertions(t, client, err)
 		})
 	}
 }

internal/directives/schemas/http-config.json

@@ -95,11 +95,10 @@
       "type": "boolean",
       "description": "Whether to skip TLS verification when making the request. (Not recommended.)"
     },
-    "timeoutSeconds": {
-      "type": "integer",
-      "minimum": 1,
-      "maximum": 60,
-      "description": "The number of seconds to wait for the request to complete. If not specified, the default is 10 seconds."
+    "timeout": {
+      "type": "string",
+      "pattern": "(?:\\d+(ns|us|µs|ms|s|m|h))+",
+      "description": "The maximum time to wait for the request to complete. If not specified, the default is 10 seconds."
     },
     "outputs": {
       "type": "array",

ui/src/gen/directives/http-config.json

@@ -112,11 +112,10 @@
    "type": "boolean",
    "description": "Whether to skip TLS verification when making the request. (Not recommended.)"
   },
-  "timeoutSeconds": {
-   "type": "integer",
-   "minimum": 1,
-   "maximum": 60,
-   "description": "The number of seconds to wait for the request to complete. If not specified, the default is 10 seconds."
+  "timeout": {
+   "type": "string",
+   "pattern": "(?:\\d+(ns|us|µs|ms|s|m|h))+",
+   "description": "The maximum time to wait for the request to complete. If not specified, the default is 10 seconds."
   },
   "outputs": {
    "type": "array",