From 797c6de68a2ed5b0d20f635b522976afb2e7fe6b Mon Sep 17 00:00:00 2001 From: nolancon Date: Wed, 29 May 2024 17:10:58 +0000 Subject: [PATCH] Simplify health check to ignore response --- .../healthcheck/healthcheck_controller.go | 20 ++++--------- .../healthcheck_controller_test.go | 28 ++++++------------- 2 files changed, 14 insertions(+), 34 deletions(-) diff --git a/internal/controller/providerconfig/healthcheck/healthcheck_controller.go b/internal/controller/providerconfig/healthcheck/healthcheck_controller.go index ff11eb38..7629666b 100644 --- a/internal/controller/providerconfig/healthcheck/healthcheck_controller.go +++ b/internal/controller/providerconfig/healthcheck/healthcheck_controller.go @@ -18,7 +18,6 @@ package healthcheck import ( "context" - "fmt" "net/http" "time" @@ -50,7 +49,6 @@ const ( errDeleteLCValidationBucket = "failed to delete lifecycle configuration validation bucket" errUpdateHealthStatus = "failed to update health status of provider config" errFailedHealthCheckReq = "failed to forward health check request" - errUnexpectedResp = "unexpected response from health check request: %s" healthCheckSuffix = "-health-check" @@ -199,23 +197,15 @@ func (c *Controller) doHealthCheck(ctx context.Context, providerConfig *apisv1al return doErr } + // We don't actually check the response code or body for the health check. + // This is because a HTTP Get request to RGW equates to a ListBuckets S3 request and + // it is possible that an authorisation error will occur, resulting in a 4XX error. + // Instead, we assume healthiness by simply making the connection successfully. - defer func() { - if err := resp.Body.Close(); err != nil { - c.log.Info("failed to close response reader after health check", "error", err.Error()) - } - }() - - if resp.StatusCode != http.StatusOK { - statusErr := errors.New(fmt.Sprintf(errUnexpectedResp, resp.Status)) - traces.SetAndRecordError(span, statusErr) - - return statusErr - } // Health check completed successfully, update status. providerConfig.Status.SetConditions(v1alpha1.HealthCheckSuccess()) - return nil + return resp.Body.Close() } // unpauseBuckets lists all buckets that exist on the given backend by using the custom diff --git a/internal/controller/providerconfig/healthcheck/healthcheck_controller_test.go b/internal/controller/providerconfig/healthcheck/healthcheck_controller_test.go index 857bb3fe..745e9d0b 100644 --- a/internal/controller/providerconfig/healthcheck/healthcheck_controller_test.go +++ b/internal/controller/providerconfig/healthcheck/healthcheck_controller_test.go @@ -18,8 +18,8 @@ package healthcheck import ( "context" - "fmt" "net/http" + "net/url" "testing" "time" @@ -59,6 +59,8 @@ func NewTestClient(fn RoundTripFunc) *http.Client { func TestReconcile(t *testing.T) { t.Parallel() backendName := "test-backend" + someErr := errors.New("some error") + urlErr := url.Error{Op: "Get", URL: "http:", Err: someErr} type fields struct { fakeS3Client func(*backendstorefakes.FakeS3Client) @@ -88,10 +90,7 @@ func TestReconcile(t *testing.T) { "ProviderConfig has been deleted": { fields: fields{ testHttpClient: NewTestClient(func(req *http.Request) (*http.Response, error) { - return &http.Response{ - StatusCode: http.StatusOK, - Status: http.StatusText(http.StatusOK), - }, nil + return &http.Response{}, nil }), fakeS3Client: func(fake *backendstorefakes.FakeS3Client) { // cleanup calls HeadBucket @@ -163,10 +162,7 @@ func TestReconcile(t *testing.T) { "ProviderConfig goes from healthy to unhealthy with bad request": { fields: fields{ testHttpClient: NewTestClient(func(req *http.Request) (*http.Response, error) { - return &http.Response{ - StatusCode: http.StatusBadRequest, - Status: http.StatusText(http.StatusBadRequest), - }, nil + return &http.Response{}, someErr }), providerConfig: &apisv1alpha1.ProviderConfig{ ObjectMeta: metav1.ObjectMeta{ @@ -195,7 +191,7 @@ func TestReconcile(t *testing.T) { }, want: want{ res: ctrl.Result{}, - err: errors.New(fmt.Sprintf(errUnexpectedResp, http.StatusText(http.StatusBadRequest))), + err: errors.Wrap(&urlErr, errFailedHealthCheckReq), pc: &apisv1alpha1.ProviderConfig{ ObjectMeta: metav1.ObjectMeta{ Name: backendName, @@ -208,7 +204,7 @@ func TestReconcile(t *testing.T) { ConditionedStatus: xpv1.ConditionedStatus{ Conditions: []xpv1.Condition{ v1alpha1.HealthCheckFail(). - WithMessage(fmt.Sprintf(errUnexpectedResp, http.StatusText(http.StatusBadRequest))), + WithMessage(errors.Wrap(&urlErr, errFailedHealthCheckReq).Error()), }, }, }, @@ -219,10 +215,7 @@ func TestReconcile(t *testing.T) { "ProviderConfig goes from unhealthy to healthy so its buckets should be unpaused": { fields: fields{ testHttpClient: NewTestClient(func(req *http.Request) (*http.Response, error) { - return &http.Response{ - StatusCode: http.StatusOK, - Status: http.StatusText(http.StatusOK), - }, nil + return &http.Response{}, nil }), providerConfig: &apisv1alpha1.ProviderConfig{ ObjectMeta: metav1.ObjectMeta{ @@ -353,10 +346,7 @@ func TestReconcile(t *testing.T) { "ProviderConfig goes from health check disabled to healthy so its buckets should be unpaused": { fields: fields{ testHttpClient: NewTestClient(func(req *http.Request) (*http.Response, error) { - return &http.Response{ - StatusCode: http.StatusOK, - Status: http.StatusText(http.StatusOK), - }, nil + return &http.Response{}, nil }), providerConfig: &apisv1alpha1.ProviderConfig{ ObjectMeta: metav1.ObjectMeta{