From fa9a5c020042723837a848ddd3a65768fcb73e6e Mon Sep 17 00:00:00 2001 From: MikeAlejoBR Date: Thu, 16 Jan 2025 19:01:12 +0100 Subject: [PATCH] fix: API Client is unable to contact Sources Apparently the "URL.String()" method escapes the special characters from the paths by default, which makes the calls to fetch internal authentications to fail, because the URL contains brackets. Also, we were attempting to read the body after it was closed in the retry attempts, which was causing errors too. By moving it to the retries loop we fulfill two purposes: 1. Draining the body completely for a new retry and connection reuse. 2. Reading the body to be able to be used in log statements or to be marshalled. RHCLOUD-36959 --- logger/logger_context.go | 5 ++-- sources/api_client.go | 64 +++++++++++++++++++++------------------- 2 files changed, 36 insertions(+), 33 deletions(-) diff --git a/logger/logger_context.go b/logger/logger_context.go index 06ad476..14be374 100644 --- a/logger/logger_context.go +++ b/logger/logger_context.go @@ -3,7 +3,6 @@ package logger import ( "context" "net/http" - "net/url" "github.com/sirupsen/logrus" ) @@ -168,8 +167,8 @@ func WithHttpMethod(ctx context.Context, httpMethod string) context.Context { } // WithURL creates a new context by copying the given context and appending the URL to it. -func WithURL(ctx context.Context, url *url.URL) context.Context { - return context.WithValue(ctx, urlCtxKey, url.String()) +func WithURL(ctx context.Context, url string) context.Context { + return context.WithValue(ctx, urlCtxKey, url) } // WithHTTPHeaders creates a new context by copying the given context and appending the HTTP headers to it. diff --git a/sources/api_client.go b/sources/api_client.go index 7a142fa..f8b7b53 100644 --- a/sources/api_client.go +++ b/sources/api_client.go @@ -54,7 +54,7 @@ func NewSourcesClient(config *config.SuperKeyWorkerConfig) *sourcesClient { return &sourcesClient{ baseV20InternalUrl: &url.URL{ Host: fmt.Sprintf("%s:%d", config.SourcesHost, config.SourcesPort), - Path: "/internal/v2.0/", + Path: "/internal/v2.0", Scheme: config.SourcesScheme, }, baseV31URL: &url.URL{ @@ -82,13 +82,13 @@ func (sc *sourcesClient) CreateAuthentication(ctx context.Context, authData *Aut ctx = l.WithResourceType(ctx, sourcesAuthentication.ResourceType) ctx = l.WithResourceId(ctx, strconv.FormatInt(sourcesAuthentication.ResourceID, 10)) - var createdAuthentication model.AuthenticationResponse - err := sc.sendRequest(ctx, http.MethodPost, createAuthenticationUrl, authData, sourcesAuthentication, createdAuthentication) + var createdAuthentication *model.AuthenticationResponse = nil + err := sc.sendRequest(ctx, http.MethodPost, createAuthenticationUrl, authData, sourcesAuthentication, &createdAuthentication) if err != nil { return nil, fmt.Errorf("error while creating authentication: %w", err) } - return &createdAuthentication, nil + return createdAuthentication, nil } func (sc *sourcesClient) CreateApplicationAuthentication(ctx context.Context, authData *AuthenticationData, appAuthCreateRequest *model.ApplicationAuthenticationCreateRequest) error { @@ -155,14 +155,20 @@ func (sc *sourcesClient) sendRequest(ctx context.Context, httpMethod string, url requestBody = bytes.NewBuffer(tmp) } + // Build the URL for the request. Unfortunately, we cannot simply use "url.String()" because it does escape the + // special path characters, and for calls like getting the internal authentication, it makes the Sources API to + // return a "Not found" response. Things like "[]" get escaped and therefore the URL does not match Sources' + // router, which causes issues. + urlRaw := fmt.Sprintf("%s://%s:%s%s", url.Scheme, url.Hostname(), url.Port(), url.Path) + // Create the request. Apparently a nil "*bytes.Buffer" counts as a body, which in turn makes the code panic when // creating a new request. That is why we add another "if" statement to guard us against that. var request *http.Request var err error if requestBody != nil { - request, err = http.NewRequestWithContext(ctx, httpMethod, url.String(), requestBody) + request, err = http.NewRequestWithContext(ctx, httpMethod, urlRaw, requestBody) } else { - request, err = http.NewRequestWithContext(ctx, httpMethod, url.String(), nil) + request, err = http.NewRequestWithContext(ctx, httpMethod, urlRaw, nil) } if err != nil { @@ -174,38 +180,42 @@ func (sc *sourcesClient) sendRequest(ctx context.Context, httpMethod string, url // Add the logging fields to the context. ctx = l.WithHttpMethod(ctx, httpMethod) - ctx = l.WithURL(ctx, url) + ctx = l.WithURL(ctx, urlRaw) ctx = l.WithHTTPHeaders(ctx, request.Header) // Perform the actual request. var response *http.Response + var responseBody []byte for attempt := 0; attempt < sc.config.SourcesRequestsMaxAttempts; attempt++ { response, err = http.DefaultClient.Do(request) // The "err" check is to avoid nil dereference errors, since if we attempt checking for the status code - // directly when an error has occurred, the "response" struct might be nil. - if err == nil && sc.isStatusCodeFamilyOf2xx(response.StatusCode) { - break - } - - // When there are no errors but the status code is not the expected one, we attempt to drain the body so that - // the default client can reuse the connection, and then we close the body to avoid memory leaks. - if err == nil && !sc.isStatusCodeFamilyOf2xx(response.StatusCode) { - _, drainErr := io.Copy(io.Discard, response.Body) - - if drainErr != nil { - l.LogWithContext(ctx).Warnf("Unable to drain response body. The connection will not be reused by the default HTTP client: %s", drainErr) + // or attempt reading the response body when an error has occurred, the "response" struct might be nil. + if err == nil { + // Declare an error variable to avoid shadowing the response body one. + var readErr error + + // Read the response body every time to ensure that the body is completely drained when retrying, or that + // it is available if it needs to be printed or used elsewhere. Draining the body is important so that the + // connection can be reused. + responseBody, readErr = io.ReadAll(response.Body) + if readErr != nil { + return fmt.Errorf(`failed to read response body: %w`, readErr) } + // Make sure to close the body to avoid memory leaks. if closeErr := response.Body.Close(); closeErr != nil { l.LogWithContext(ctx).Errorf("Failed to close incoming response's body: %s", closeErr) } - l.LogWithContext(ctx).Debugf(`Unexpected status code received. Want "2xx", got "%d"`, response.StatusCode) - continue - } - - if err != nil { + // When the status code is "2xx", we can simply exit the loop. Otherwise, we need to keep retrying until we + // deplete the attempts. + if sc.isStatusCodeFamilyOf2xx(response.StatusCode) { + break + } else { + l.LogWithContext(ctx).WithField("response_body", responseBody).Debugf(`Unexpected status code received. Want "2xx", got "%d"`, response.StatusCode) + } + } else { l.LogWithContext(ctx).Warn("Failed to send request. Retrying...") l.LogWithContext(ctx).Debugf("Failed to send request. Retrying... Cause: %s", err) } @@ -216,12 +226,6 @@ func (sc *sourcesClient) sendRequest(ctx context.Context, httpMethod string, url return fmt.Errorf("failed to send request: %w", err) } - // Always read the response body, in case we need to return it in an error or marshal it to a struct. - responseBody, err := io.ReadAll(response.Body) - if err != nil { - return fmt.Errorf(`failed to read response body: %w`, err) - } - // Make sure that the status code is a "2xx" one. if !sc.isStatusCodeFamilyOf2xx(response.StatusCode) { return fmt.Errorf(`unexpected status code received. Want "2xx", got "%d". Response body: %s`, response.StatusCode, string(responseBody))