From 3734560825176c435285468d31e549b2efb1c275 Mon Sep 17 00:00:00 2001 From: Amin Salarkia Date: Wed, 23 Oct 2024 16:27:15 +0200 Subject: [PATCH 1/2] fix(kuberpult-cli): add timeout to the args Ref: SRX-880CZF --- cli/pkg/cli_utils/http.go | 12 ++++++------ cli/pkg/cmd/cli.go | 1 + cli/pkg/cmd/handlers.go | 2 +- cli/pkg/cmd/parsing.go | 13 +++++++++++++ cli/pkg/cmd/parsing_test.go | 23 +++++++++++++++++++++++ cli/pkg/deployments/deployments.go | 2 +- cli/pkg/locks/lock.go | 2 +- cli/pkg/release/release.go | 2 +- cli/pkg/releasetrain/releasetrain.go | 2 +- 9 files changed, 48 insertions(+), 11 deletions(-) diff --git a/cli/pkg/cli_utils/http.go b/cli/pkg/cli_utils/http.go index 90ae5a1ea..af5ac1785 100644 --- a/cli/pkg/cli_utils/http.go +++ b/cli/pkg/cli_utils/http.go @@ -28,10 +28,10 @@ const ( HttpDefaultTimeout = 180 ) -func doRequest(request *http.Request) (*http.Response, []byte, error) { +func doRequest(request *http.Request, timeout int) (*http.Response, []byte, error) { //exhaustruct:ignore client := &http.Client{ - Timeout: HttpDefaultTimeout * time.Second, + Timeout: time.Duration(timeout) * time.Second, // We don't want to follow redirects. If we get a redirect, we want to return the original status code. CheckRedirect: func(req *http.Request, via []*http.Request) error { return http.ErrUseLastResponse @@ -52,10 +52,10 @@ func doRequest(request *http.Request) (*http.Response, []byte, error) { return resp, body, nil } -func IssueHttpRequest(req http.Request, retries uint64) error { +func IssueHttpRequest(req http.Request, retries uint64, timeout int) error { var i uint64 for i = 0; i < retries+1; i++ { - response, body, err := doRequest(&req) + response, body, err := doRequest(&req, timeout) if err != nil { log.Printf("error issuing http request: %v\n", err) } else if response.StatusCode != http.StatusCreated && response.StatusCode != http.StatusOK { @@ -73,8 +73,8 @@ func IssueHttpRequest(req http.Request, retries uint64) error { return fmt.Errorf("could not perform a successful call to kuberpult") } -func IssueHttpRequestWithBodyReturn(req http.Request) ([]byte, error) { - response, body, err := doRequest(&req) +func IssueHttpRequestWithBodyReturn(req http.Request, timeout int) ([]byte, error) { + response, body, err := doRequest(&req, timeout) if err != nil { return nil, err } else if response.StatusCode != http.StatusCreated && response.StatusCode != http.StatusOK { diff --git a/cli/pkg/cmd/cli.go b/cli/pkg/cmd/cli.go index 0ed5382f6..cbf5d32f4 100644 --- a/cli/pkg/cmd/cli.go +++ b/cli/pkg/cmd/cli.go @@ -37,6 +37,7 @@ type kuberpultClientParameters struct { authorEmail *string iapToken *string dexToken *string + timeout uint64 } func RunCLI() ReturnCode { diff --git a/cli/pkg/cmd/handlers.go b/cli/pkg/cmd/handlers.go index 9ee6d4807..68ae1949f 100644 --- a/cli/pkg/cmd/handlers.go +++ b/cli/pkg/cmd/handlers.go @@ -74,7 +74,7 @@ func handleReleaseTrain(kpClientParams kuberpultClientParameters, args []string) requestParameters := kutil.RequestParameters{ Url: &kpClientParams.url, Retries: kpClientParams.retries, - HttpTimeout: cli_utils.HttpDefaultTimeout, + HttpTimeout: int(kpClientParams.timeout), } if err = releasetrain.HandleReleaseTrain(requestParameters, authParams, *parsedArgs); err != nil { diff --git a/cli/pkg/cmd/parsing.go b/cli/pkg/cmd/parsing.go index c4370d88f..b432cd30b 100644 --- a/cli/pkg/cmd/parsing.go +++ b/cli/pkg/cmd/parsing.go @@ -29,6 +29,7 @@ type commandLineArguments struct { authorEmail cli_utils.RepeatedString authorName cli_utils.RepeatedString retries cli_utils.RepeatedInt + timeout cli_utils.RepeatedInt } func readArgs(args []string) (*commandLineArguments, []string, error) { @@ -40,6 +41,7 @@ func readArgs(args []string) (*commandLineArguments, []string, error) { fs.Var(&cmdArgs.authorName, "author_name", "the name of the git author who eventually will write to the manifest repo (must be set at most once)") fs.Var(&cmdArgs.authorEmail, "author_email", "the email of the git author who eventially will write to the manifest repo (must be set at most once)") fs.Var(&cmdArgs.retries, "retries", "number of times the cli will retry http requests to kuberpult upon failure (must be set at most once) - default=3") + fs.Var(&cmdArgs.timeout, "timeout", "requests timeout seconds (must be set at most once) - default=180") if err := fs.Parse(args); err != nil { return nil, nil, fmt.Errorf("error while reading command line arguments, error: %w", err) @@ -63,6 +65,9 @@ func argsValid(cmdArgs *commandLineArguments) (bool, string) { if len(cmdArgs.retries.Values) > 1 { return false, "the --retries arg must be set at most once" } + if len(cmdArgs.timeout.Values) > 1 { + return false, "the --timeout arg must be set at most once" + } return true, "" } @@ -92,6 +97,14 @@ func convertToParams(cmdArgs *commandLineArguments) (*kuberpultClientParameters, } else { params.retries = DefaultRetries } + if len(cmdArgs.timeout.Values) == 1 { + if cmdArgs.timeout.Values[0] <= 0 { + return nil, fmt.Errorf("--timeout arg value must be positive") + } + params.timeout = uint64(cmdArgs.timeout.Values[0]) + } else { + params.timeout = cli_utils.HttpDefaultTimeout + } return ¶ms, nil } diff --git a/cli/pkg/cmd/parsing_test.go b/cli/pkg/cmd/parsing_test.go index fb4902538..ab34c630c 100644 --- a/cli/pkg/cmd/parsing_test.go +++ b/cli/pkg/cmd/parsing_test.go @@ -71,6 +71,7 @@ func TestParseArgs(t *testing.T) { expectedParams: &kuberpultClientParameters{ url: "something.somewhere", retries: DefaultRetries, + timeout: 180, }, }, { @@ -93,6 +94,7 @@ func TestParseArgs(t *testing.T) { expectedParams: &kuberpultClientParameters{ url: "something.somewhere", retries: DefaultRetries, + timeout: 180, }, expectedOther: []string{"potato", "--tomato"}, }, @@ -103,6 +105,7 @@ func TestParseArgs(t *testing.T) { url: "something.somewhere", authorName: ptrStr("john"), retries: DefaultRetries, + timeout: 180, }, expectedOther: []string{"subcommand", "--arg1", "val1", "etc", "etc"}, }, @@ -120,6 +123,7 @@ func TestParseArgs(t *testing.T) { url: "something.somewhere", authorEmail: ptrStr("john"), retries: DefaultRetries, + timeout: 180, }, expectedOther: []string{"subcommand", "--arg1", "val1", "etc", "etc"}, }, @@ -136,6 +140,7 @@ func TestParseArgs(t *testing.T) { expectedParams: &kuberpultClientParameters{ url: "something.somewhere", retries: DefaultRetries, + timeout: 180, }, expectedOther: []string{"potato", "--tomato"}, }, @@ -145,6 +150,7 @@ func TestParseArgs(t *testing.T) { expectedParams: &kuberpultClientParameters{ url: "something.somewhere", retries: 10, + timeout: 180, }, expectedOther: []string{"potato", "--tomato"}, }, @@ -155,6 +161,23 @@ func TestParseArgs(t *testing.T) { msg: "error while creating kuberpult client parameters, error: --retries arg value must be positive", }, }, + { + name: "overriding default timeout", + cmdArgs: "--url something.somewhere --retries 10 --timeout 7200 potato --tomato", + expectedParams: &kuberpultClientParameters{ + url: "something.somewhere", + retries: 10, + timeout: 7200, + }, + expectedOther: []string{"potato", "--tomato"}, + }, + { + name: "--timeout is negative", + cmdArgs: "--url something.somewhere --timeout -1 --author_email john subcommand --arg1 val1 etc etc", + expectedError: errMatcher{ + msg: "error while creating kuberpult client parameters, error: --timeout arg value must be positive", + }, + }, } for _, tc := range tcs { diff --git a/cli/pkg/deployments/deployments.go b/cli/pkg/deployments/deployments.go index 5d43b61c3..bb445bfb6 100644 --- a/cli/pkg/deployments/deployments.go +++ b/cli/pkg/deployments/deployments.go @@ -35,7 +35,7 @@ func HandleGetCommitDeployments(requestParams kutil.RequestParameters, authParam if err != nil { return fmt.Errorf("error while preparing HTTP request, error: %w", err) } - body, err := cli_utils.IssueHttpRequestWithBodyReturn(*req) + body, err := cli_utils.IssueHttpRequestWithBodyReturn(*req, requestParams.HttpTimeout) if err != nil { return fmt.Errorf("error while issuing HTTP request, error: %v", err) } diff --git a/cli/pkg/locks/lock.go b/cli/pkg/locks/lock.go index 2110c2799..338333990 100644 --- a/cli/pkg/locks/lock.go +++ b/cli/pkg/locks/lock.go @@ -112,7 +112,7 @@ func HandleLockRequest(requestParams kutil.RequestParameters, authParams kutil.A if err != nil { return fmt.Errorf("error while preparing HTTP request, error: %w", err) } - if err := cli_utils.IssueHttpRequest(*req, requestParams.Retries); err != nil { + if err := cli_utils.IssueHttpRequest(*req, requestParams.Retries, requestParams.HttpTimeout); err != nil { return fmt.Errorf("error while issuing HTTP request, error: %v", err) } return nil diff --git a/cli/pkg/release/release.go b/cli/pkg/release/release.go index 6b71d2e9f..ee946d6cb 100644 --- a/cli/pkg/release/release.go +++ b/cli/pkg/release/release.go @@ -48,7 +48,7 @@ func Release(requestParams kutil.RequestParameters, authParams kutil.Authenticat if err != nil { return fmt.Errorf("error while preparing HTTP request, error: %w", err) } - if err := cli_utils.IssueHttpRequest(*req, requestParams.Retries); err != nil { + if err := cli_utils.IssueHttpRequest(*req, requestParams.Retries, requestParams.HttpTimeout); err != nil { return fmt.Errorf("error while issuing HTTP request, error: %v", err) } return nil diff --git a/cli/pkg/releasetrain/releasetrain.go b/cli/pkg/releasetrain/releasetrain.go index d56ddd20f..f5f975300 100644 --- a/cli/pkg/releasetrain/releasetrain.go +++ b/cli/pkg/releasetrain/releasetrain.go @@ -45,7 +45,7 @@ func HandleReleaseTrain(requestParams kutil.RequestParameters, authParams kutil. if err != nil { return fmt.Errorf("error while preparing HTTP request, error: %w", err) } - if err := cli_utils.IssueHttpRequest(*req, requestParams.Retries); err != nil { + if err := cli_utils.IssueHttpRequest(*req, requestParams.Retries, requestParams.HttpTimeout); err != nil { return fmt.Errorf("error while issuing HTTP request, error: %v", err) } return nil From 7a96324056c2751e901ff8c1030fd8f0bbc26b3b Mon Sep 17 00:00:00 2001 From: Amin Salarkia Date: Thu, 24 Oct 2024 10:06:05 +0200 Subject: [PATCH 2/2] fix(cli): fix namings Ref: SRX-880CZF --- cli/pkg/cli_utils/http.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/cli/pkg/cli_utils/http.go b/cli/pkg/cli_utils/http.go index af5ac1785..282efaf49 100644 --- a/cli/pkg/cli_utils/http.go +++ b/cli/pkg/cli_utils/http.go @@ -28,10 +28,10 @@ const ( HttpDefaultTimeout = 180 ) -func doRequest(request *http.Request, timeout int) (*http.Response, []byte, error) { +func doRequest(request *http.Request, timeoutSeconds int) (*http.Response, []byte, error) { //exhaustruct:ignore client := &http.Client{ - Timeout: time.Duration(timeout) * time.Second, + Timeout: time.Duration(timeoutSeconds) * time.Second, // We don't want to follow redirects. If we get a redirect, we want to return the original status code. CheckRedirect: func(req *http.Request, via []*http.Request) error { return http.ErrUseLastResponse @@ -52,10 +52,10 @@ func doRequest(request *http.Request, timeout int) (*http.Response, []byte, erro return resp, body, nil } -func IssueHttpRequest(req http.Request, retries uint64, timeout int) error { +func IssueHttpRequest(req http.Request, retries uint64, timeoutSeconds int) error { var i uint64 for i = 0; i < retries+1; i++ { - response, body, err := doRequest(&req, timeout) + response, body, err := doRequest(&req, timeoutSeconds) if err != nil { log.Printf("error issuing http request: %v\n", err) } else if response.StatusCode != http.StatusCreated && response.StatusCode != http.StatusOK { @@ -73,8 +73,8 @@ func IssueHttpRequest(req http.Request, retries uint64, timeout int) error { return fmt.Errorf("could not perform a successful call to kuberpult") } -func IssueHttpRequestWithBodyReturn(req http.Request, timeout int) ([]byte, error) { - response, body, err := doRequest(&req, timeout) +func IssueHttpRequestWithBodyReturn(req http.Request, timeoutSeconds int) ([]byte, error) { + response, body, err := doRequest(&req, timeoutSeconds) if err != nil { return nil, err } else if response.StatusCode != http.StatusCreated && response.StatusCode != http.StatusOK {