diff --git a/docs/docs/35-references/10-promotion-steps.md b/docs/docs/35-references/10-promotion-steps.md index 4d2d3f98a..383147522 100644 --- a/docs/docs/35-references/10-promotion-steps.md +++ b/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. | diff --git a/internal/directives/http_requester.go b/internal/directives/http_requester.go index 91e1fba6a..6423e26ba 100644 --- a/internal/directives/http_requester.go +++ b/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) { diff --git a/internal/directives/http_requester_test.go b/internal/directives/http_requester_test.go index 8c60f9efe..f5bc8b320 100644 --- a/internal/directives/http_requester_test.go +++ b/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,7 +418,8 @@ 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) @@ -405,12 +427,21 @@ func Test_httpRequester_getClient(t *testing.T) { 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) }) } } diff --git a/internal/directives/schemas/http-config.json b/internal/directives/schemas/http-config.json index e39f72287..5b140a42f 100644 --- a/internal/directives/schemas/http-config.json +++ b/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", diff --git a/internal/directives/zz_config_types.go b/internal/directives/zz_config_types.go index 6108aa303..6a9f964de 100644 --- a/internal/directives/zz_config_types.go +++ b/internal/directives/zz_config_types.go @@ -321,9 +321,9 @@ type HTTPConfig struct { QueryParams []HTTPQueryParam `json:"queryParams,omitempty"` // An expression to evaluate to determine if the request was successful. SuccessExpression string `json:"successExpression,omitempty"` - // The number of seconds to wait for the request to complete. If not specified, the default - // is 10 seconds. - TimeoutSeconds *int64 `json:"timeoutSeconds,omitempty"` + // The maximum time to wait for the request to complete. If not specified, the default is 10 + // seconds. + Timeout string `json:"timeout,omitempty"` // The URL to send the HTTP request to. URL string `json:"url"` } diff --git a/ui/src/gen/directives/http-config.json b/ui/src/gen/directives/http-config.json index ab373da0f..34edcf6f8 100644 --- a/ui/src/gen/directives/http-config.json +++ b/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",