Fix for "url.Error: Delete http://f8tenant:80/api/tenant?remove=false…

…: EOF" (#2092)

The expected response code is `204`, not `200`

* apply status code range check
* use the response status to produce an error
* handle decode errors and empty JSON-API errors
* support other error, including with empty msg

Fixes openshiftio/openshift.io#3526

Signed-off-by: Xavier Coulon <[email protected]>

account/init_tenant.go

@@ -82,9 +82,9 @@ func UpdateTenant(ctx context.Context, config tenantConfig) error {
 }
 
 // CleanTenant cleans out a tenant in oso.
-func CleanTenant(ctx context.Context, config tenantConfig, remove bool) error {
+func CleanTenant(ctx context.Context, config tenantConfig, remove bool, options ...configuration.HTTPClientOption) error {
 
-	c, err := createClient(ctx, config)
+	c, err := createClient(ctx, config, options...)
 	if err != nil {
 		return err
 	}
@@ -95,14 +95,16 @@ func CleanTenant(ctx context.Context, config tenantConfig, remove bool) error {
 	}
 	defer rest.CloseResponse(res)
 
-	if res.StatusCode != http.StatusOK {
+	// operation failed for some reason
+	if res.StatusCode < 200 || res.StatusCode >= 300 {
 		jsonErr, err := c.DecodeJSONAPIErrors(res)
-		if err == nil {
-			if len(jsonErr.Errors) > 0 {
-				return errors.NewInternalError(ctx, fmt.Errorf(jsonErr.Errors[0].Detail))
-			}
+		if err == nil && len(jsonErr.Errors) > 0 {
+			return errors.FromStatusCode(res.StatusCode, jsonErr.Errors[0].Detail)
 		}
+		// if failed to decode the response body into a JSON-API error, or if the JSON-API error was empty
+		return errors.FromStatusCode(res.StatusCode, "unknown error")
 	}
+	// operation succeeded
 	return nil
 }
 

account/init_tenant_test.go

@@ -58,3 +58,64 @@ func TestShowTenant(t *testing.T) {
 		assert.IsType(t, errors.NotFoundError{}, err)
 	})
 }
+
+func TestCleanTenant(t *testing.T) {
+
+	// given
+	r, err := testrecorder.New(
+		"../test/data/account/delete_tenant",
+		testrecorder.WithJWTMatcher("../test/jwt/public_key.pem"),
+	)
+	require.NoError(t, err)
+	defer r.Stop()
+	config := TenantConfig{
+		url: "http://tenant",
+	}
+
+	t.Run("ok", func(t *testing.T) {
+		// given
+		ctx, err := testjwt.NewJWTContext("bcdd0b29-123d-11e8-a8bc-b69930b94f5c")
+		require.NoError(t, err)
+		// when
+		err = account.CleanTenant(ctx, config, false, configuration.WithRoundTripper(r.Transport))
+		// then
+		require.NoError(t, err)
+	})
+
+	t.Run("failure", func(t *testing.T) {
+
+		t.Run("internal server error", func(t *testing.T) {
+			// given
+			ctx, err := testjwt.NewJWTContext("83fdcae2-634f-4a52-958a-f723cb621700")
+			require.NoError(t, err)
+			// when
+			err = account.CleanTenant(ctx, config, false, configuration.WithRoundTripper(r.Transport))
+			// then
+			require.Error(t, err)
+			assert.IsType(t, errors.InternalError{}, err)
+		})
+
+		t.Run("other error with a message", func(t *testing.T) {
+			// given
+			ctx, err := testjwt.NewJWTContext("2610c5dc-d700-4b86-b979-2b103e0b1144")
+			require.NoError(t, err)
+			// when
+			err = account.CleanTenant(ctx, config, false, configuration.WithRoundTripper(r.Transport))
+			// then
+			require.Error(t, err)
+			assert.IsType(t, errors.UnauthorizedError{}, err)
+		})
+
+		t.Run("other error without a message", func(t *testing.T) {
+			// given
+			ctx, err := testjwt.NewJWTContext("73a3b0ce-4917-44db-9979-90b1219ca2c6")
+			require.NoError(t, err)
+			// when
+			err = account.CleanTenant(ctx, config, false, configuration.WithRoundTripper(r.Transport))
+			// then
+			require.Error(t, err)
+			assert.IsType(t, errors.InternalError{}, err)
+		})
+	})
+
+}

errors/errors.go

@@ -3,8 +3,10 @@ package errors
 import (
 	"context"
 	"fmt"
+	"net/http"
 
 	"errors"
+
 	errs "github.com/pkg/errors"
 )
 
@@ -19,6 +21,25 @@ const (
 	ErrInternalDatabase = "database_error"
 )
 
+// FromStatusCode returns an error from the given HTTP status code, using the message and args
+func FromStatusCode(statusCode int, format string, args ...interface{}) error {
+	msg := fmt.Sprintf(format, args...)
+	switch statusCode {
+	case http.StatusNotFound:
+		return NewNotFoundErrorFromString(msg)
+	case http.StatusBadRequest:
+		return NewBadParameterErrorFromString(msg)
+	case http.StatusConflict:
+		return NewVersionConflictError(msg)
+	case http.StatusUnauthorized:
+		return NewUnauthorizedError(msg)
+	case http.StatusForbidden:
+		return NewForbiddenError(msg)
+	default:
+		return NewInternalErrorFromString(msg)
+	}
+}
+
 type simpleError struct {
 	message string
 }

test/data/account/delete_tenant.yaml

@@ -0,0 +1,50 @@
+---
+version: 1
+interactions:
+- request:
+    headers:
+      sub: ["bcdd0b29-123d-11e8-a8bc-b69930b94f5c"] # will be compared against the `sub` claim in the incoming request's token
+    url: http://tenant/api/tenant?remove=false
+    method: DELETE
+  response:
+    status: 204 No Content
+    code: 204
+- request:
+    headers:
+      sub: ["83fdcae2-634f-4a52-958a-f723cb621700"] # will be compared against the `sub` claim in the incoming request's token
+    url: http://tenant/api/tenant?remove=false
+    method: DELETE
+  response:
+    status: 500 Internal Server Error
+    code: 500
+    body: '{
+      "errors":[{
+          "code": "500",
+          "detail": "unable to fetch cluster",
+          "id": "83fdcae2-634f-4a52-958a-f723cb621700"
+        }]
+      }'
+- request:
+    headers:
+      sub: ["2610c5dc-d700-4b86-b979-2b103e0b1144"] # will be compared against the `sub` claim in the incoming request's token
+    url: http://tenant/api/tenant?remove=false
+    method: DELETE
+  response:
+    status: 401 Unauthorized
+    code: 401
+    body: '{
+      "errors":[{
+          "code": "401",
+          "detail": "unauthorized",
+          "id": "2610c5dc-d700-4b86-b979-2b103e0b1144"
+        }]
+      }'
+- request:
+    headers:
+      sub: ["73a3b0ce-4917-44db-9979-90b1219ca2c6"] # will be compared against the `sub` claim in the incoming request's token
+    url: http://tenant/api/tenant?remove=false
+    method: DELETE
+  response:
+    status: 418 I'm a teapot
+    code: 418
+    # no response body, on purpose