fix(api-server): improve error handling and return status (#7937)

- never return 412 (it was never appropriate
- create better error hierarchy to simplify handler
- use in some places errors.Is

Signed-off-by: Charly Molter <[email protected]>

pkg/api-server/testdata/resource_page-size-invalid.golden.json

@@ -6,14 +6,14 @@
  "invalid_parameters": [
   {
    "field": "size",
-   "reason": "Invalid format"
+   "reason": "invalid format"
   }
  ],
  "details": "Invalid page size",
  "causes": [
   {
    "field": "size",
-   "message": "Invalid format"
+   "message": "invalid format"
   }
  ]
 }

pkg/api-server/testdata/resource_page-size-too-large.golden.json

@@ -6,14 +6,14 @@
  "invalid_parameters": [
   {
    "field": "size",
-   "reason": "Invalid page size of 2000. Maximum page size is 1000"
+   "reason": "invalid page size of 2000. Maximum page size is 1000"
   }
  ],
  "details": "Invalid page size",
  "causes": [
   {
    "field": "size",
-   "message": "Invalid page size of 2000. Maximum page size is 1000"
+   "message": "invalid page size of 2000. Maximum page size is 1000"
   }
  ]
 }

pkg/api-server/types/errors.go

@@ -1,17 +1,24 @@
 package types
 
 import (
-	"strings"
-
-	"github.com/pkg/errors"
+	"fmt"
+	"reflect"
 )
 
-func NewMaxPageSizeExceeded(pageSize, limit int) error {
-	return errors.Errorf("Invalid page size of %d. Maximum page size is %d", pageSize, limit)
+type InvalidPageSizeError struct {
+	Reason string
+}
+
+func (a *InvalidPageSizeError) Error() string {
+	return a.Reason
 }
 
-func IsMaxPageSizeExceeded(err error) bool {
-	return strings.HasPrefix(err.Error(), "Invalid page size of")
+func (a *InvalidPageSizeError) Is(err error) bool {
+	return reflect.TypeOf(a) == reflect.TypeOf(err)
+}
+
+func NewMaxPageSizeExceeded(pageSize, limit int) error {
+	return &InvalidPageSizeError{Reason: fmt.Sprintf("invalid page size of %d. Maximum page size is %d", pageSize, limit)}
 }
 
-var InvalidPageSize = errors.New("Invalid page size")
+var InvalidPageSize = &InvalidPageSizeError{Reason: "invalid format"}

pkg/core/resources/manager/manager.go

@@ -179,7 +179,7 @@ func Upsert(ctx context.Context, manager ResourceManager, key model.ResourceKey,
 			return err
 		}
 		err := upsert(ctx)
-		if store.IsResourceConflict(err) || store.IsResourceAlreadyExists(err) {
+		if errors.Is(err, &store.ResourceConflictError{}) {
 			return retry.RetryableError(err)
 		}
 		return err

pkg/core/resources/registry/registry.go

@@ -26,16 +26,16 @@ func NewTypeRegistry() TypeRegistry {
 	}
 }
 
-type InvalidResourceType struct {
+type InvalidResourceTypeError struct {
 	ResType model.ResourceType
 }
 
-func (e *InvalidResourceType) Error() string {
+func (e *InvalidResourceTypeError) Error() string {
 	return fmt.Sprintf("invalid resource type %q", e.ResType)
 }
 
-func (e *InvalidResourceType) Is(target error) bool {
-	t, ok := target.(*InvalidResourceType)
+func (e *InvalidResourceTypeError) Is(target error) bool {
+	t, ok := target.(*InvalidResourceTypeError)
 	if !ok {
 		return false
 	}
@@ -49,7 +49,7 @@ type typeRegistry struct {
 func (t *typeRegistry) DescriptorFor(resType model.ResourceType) (model.ResourceTypeDescriptor, error) {
 	typDesc, ok := t.descriptors[resType]
 	if !ok {
-		return model.ResourceTypeDescriptor{}, &InvalidResourceType{ResType: resType}
+		return model.ResourceTypeDescriptor{}, &InvalidResourceTypeError{ResType: resType}
 	}
 	return typDesc, nil
 }

pkg/core/resources/store/store.go

@@ -2,13 +2,12 @@ package store
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"reflect"
 	"strings"
 
-	"github.com/pkg/errors"
-
 	"github.com/kumahq/kuma/pkg/core/resources/model"
 )
 
@@ -117,24 +116,31 @@ func (s *strictResourceStore) Close() error {
 	return nil
 }
 
-func ErrorResourceNotFound(rt model.ResourceType, name, mesh string) error {
-	return fmt.Errorf("Resource not found: type=%q name=%q mesh=%q", rt, name, mesh)
+type ResourceConflictError struct {
+	rType model.ResourceType
+	name  string
+	mesh  string
+	msg   string
 }
 
-func ErrorResourceAlreadyExists(rt model.ResourceType, name, mesh string) error {
-	return fmt.Errorf("Resource already exists: type=%q name=%q mesh=%q", rt, name, mesh)
+func (e *ResourceConflictError) Error() string {
+	return fmt.Sprintf("%s: type=%q name=%q mesh=%q", e.msg, e.rType, e.name, e.mesh)
 }
 
-func ErrorResourceConflict(rt model.ResourceType, name, mesh string) error {
-	return fmt.Errorf("Resource conflict: type=%q name=%q mesh=%q", rt, name, mesh)
+func (e *ResourceConflictError) Is(err error) bool {
+	return reflect.TypeOf(e) == reflect.TypeOf(err)
 }
 
-func IsResourceConflict(err error) bool {
-	return err != nil && strings.HasPrefix(err.Error(), "Resource conflict")
+func ErrorResourceAlreadyExists(rt model.ResourceType, name, mesh string) error {
+	return &ResourceConflictError{msg: "resource already exists", rType: rt, name: name, mesh: mesh}
 }
 
-func ErrorResourcePreconditionFailed(rt model.ResourceType, name, mesh string) error {
-	return fmt.Errorf("Resource precondition failed: type=%q name=%q mesh=%q", rt, name, mesh)
+func ErrorResourceConflict(rt model.ResourceType, name, mesh string) error {
+	return &ResourceConflictError{msg: "resource conflict", rType: rt, name: name, mesh: mesh}
+}
+
+func ErrorResourceNotFound(rt model.ResourceType, name, mesh string) error {
+	return fmt.Errorf("Resource not found: type=%q name=%q mesh=%q", rt, name, mesh)
 }
 
 var ErrorInvalidOffset = errors.New("invalid offset")
@@ -143,12 +149,35 @@ func IsResourceNotFound(err error) bool {
 	return err != nil && strings.HasPrefix(err.Error(), "Resource not found")
 }
 
-func IsResourcePreconditionFailed(err error) bool {
-	return err != nil && strings.HasPrefix(err.Error(), "Resource precondition failed")
+// AssertionError
+type AssertionError struct {
+	msg string
+	err error
+}
+
+func ErrorResourceAssertion(msg string, rt model.ResourceType, name, mesh string) error {
+	return &AssertionError{
+		msg: fmt.Sprintf("%s: type=%q name=%q mesh=%q", msg, rt, name, mesh),
+	}
+}
+
+func (e *AssertionError) Unwrap() error {
+	return e.err
+}
+
+func (e *AssertionError) Error() string {
+	msg := "store assertion failed"
+	if e.msg != "" {
+		msg += " " + e.msg
+	}
+	if e.err != nil {
+		msg += fmt.Sprintf("error: %s", e.err)
+	}
+	return msg
 }
 
-func IsResourceAlreadyExists(err error) bool {
-	return err != nil && strings.HasPrefix(err.Error(), "Resource already exists")
+func (e *AssertionError) Is(err error) bool {
+	return reflect.TypeOf(e) == reflect.TypeOf(err)
 }
 
 type PreconditionError struct {

pkg/core/rest/errors/error_handler.go

@@ -2,10 +2,10 @@ package errors
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	"github.com/emicklei/go-restful/v3"
-	"github.com/pkg/errors"
 	"go.opentelemetry.io/otel/trace"
 
 	api_server_types "github.com/kumahq/kuma/pkg/api-server/types"
@@ -32,44 +32,12 @@ func HandleError(ctx context.Context, response *restful.Response, err error, tit
 			Title:  title,
 			Detail: "Not found",
 		}
-	case errors.Is(err, &rest.InvalidResourceError{}):
+	case errors.Is(err, &rest.InvalidResourceError{}) || errors.Is(err, &registry.InvalidResourceTypeError{}) || errors.Is(err, &store.PreconditionError{}):
 		kumaErr = &types.Error{
 			Status: 400,
 			Title:  "Bad Request",
 			Detail: err.Error(),
 		}
-	case errors.Is(err, &registry.InvalidResourceType{}):
-		kumaErr = &types.Error{
-			Status: 400,
-			Title:  "Bad Request",
-			Detail: err.Error(),
-		}
-	case store.IsResourcePreconditionFailed(err):
-		kumaErr = &types.Error{
-			Status: 412,
-			Title:  title,
-			Detail: "Precondition Failed",
-		}
-	case errors.Is(err, &store.PreconditionError{}):
-		var err2 *store.PreconditionError
-		errors.As(err, &err2)
-		kumaErr = &types.Error{
-			Status: 400,
-			Title:  "Bad Request",
-			Detail: err2.Reason,
-		}
-	case err == store.ErrorInvalidOffset:
-		kumaErr = &types.Error{
-			Status: 400,
-			Title:  title,
-			Detail: "Invalid offset",
-			InvalidParameters: []types.InvalidParameter{
-				{
-					Field:  "offset",
-					Reason: "Invalid format",
-				},
-			},
-		}
 	case manager.IsMeshNotFound(err):
 		kumaErr = &types.Error{
 			Status: 400,
@@ -94,27 +62,27 @@ func HandleError(ctx context.Context, response *restful.Response, err error, tit
 				Reason: violation.Message,
 			})
 		}
-	case api_server_types.IsMaxPageSizeExceeded(err):
+	case err == store.ErrorInvalidOffset:
 		kumaErr = &types.Error{
 			Status: 400,
 			Title:  title,
-			Detail: "Invalid page size",
+			Detail: "Invalid offset",
 			InvalidParameters: []types.InvalidParameter{
 				{
-					Field:  "size",
-					Reason: err.Error(),
+					Field:  "offset",
+					Reason: "Invalid format",
 				},
 			},
 		}
-	case err == api_server_types.InvalidPageSize:
+	case errors.Is(err, &api_server_types.InvalidPageSizeError{}):
 		kumaErr = &types.Error{
 			Status: 400,
 			Title:  title,
 			Detail: "Invalid page size",
 			InvalidParameters: []types.InvalidParameter{
 				{
 					Field:  "size",
-					Reason: "Invalid format",
+					Reason: err.Error(),
 				},
 			},
 		}
@@ -130,7 +98,7 @@ func HandleError(ctx context.Context, response *restful.Response, err error, tit
 			Title:  "Method not Allowed",
 			Detail: err.Error(),
 		}
-	case errors.Is(err, &Conflict{}):
+	case errors.Is(err, &Conflict{}) || errors.Is(err, &store.ResourceConflictError{}):
 		kumaErr = &types.Error{
 			Status: 409,
 			Title:  "Conflict",

pkg/insights/resyncer.go

@@ -519,8 +519,8 @@ func (r *resyncer) createOrUpdateServiceInsight(
 			// Mesh no longer exist so we cannot upsert the insight for it.
 			return nil, false
 		}
-		if store.IsResourceConflict(err) {
-			log.V(1).Info("ServiceInsight was updated in other place. Retrying")
+		if errors.Is(err, &store.ResourceConflictError{}) {
+			log.V(1).Info(err.Error())
 			return nil, false
 		}
 		return err, false
@@ -677,8 +677,8 @@ func (r *resyncer) createOrUpdateMeshInsight(
 			// Mesh no longer exist so we cannot upsert the insight for it.
 			return nil, false
 		}
-		if store.IsResourceConflict(err) {
-			log.V(1).Info("MeshInsight was updated in other place. Retrying")
+		if errors.Is(err, &store.ResourceConflictError{}) {
+			log.V(1).Info(err.Error())
 			return nil, false
 		}
 		return err, false

pkg/kds/server/status_sink.go

@@ -2,6 +2,7 @@ package server
 
 import (
 	"context"
+	"errors"
 	"time"
 
 	"github.com/go-logr/logr"
@@ -85,7 +86,7 @@ func (s *zoneInsightSink) Start(ctx context.Context, stop <-chan struct{}) {
 		}
 
 		if err := s.store.Upsert(gracefulCtx, zone, currentState); err != nil {
-			if store.IsResourceConflict(err) || store.IsResourceAlreadyExists(err) {
+			if errors.Is(err, &store.ResourceConflictError{}) {
 				log.V(1).Info("failed to flush ZoneInsight because it was updated in other place. Will retry in the next tick", "zone", zone)
 			} else {
 				log.Error(err, "failed to flush zone status", "zone", zone)

pkg/metrics/store/store.go

@@ -2,6 +2,7 @@ package metrics
 
 import (
 	"context"
+	"errors"
 
 	"github.com/prometheus/client_golang/prometheus"
 
@@ -49,7 +50,7 @@ func (m *MeteredStore) Update(ctx context.Context, resource model.Resource, opti
 		m.metric.WithLabelValues("update", string(resource.Descriptor().Name)).Observe(core.Now().Sub(start).Seconds())
 	}()
 	err := m.delegate.Update(ctx, resource, optionsFunc...)
-	if store.IsResourceConflict(err) {
+	if errors.Is(err, &store.ResourceConflictError{}) {
 		m.conflicts.WithLabelValues(string(resource.Descriptor().Name)).Inc()
 	}
 	return err

pkg/metrics/store/store_test.go

@@ -89,8 +89,7 @@ var _ = Describe("Metered Store", func() {
 		err = store.Update(context.Background(), anotherMesh)
 
 		// then
-		Expect(err).To(HaveOccurred())
-		Expect(core_store.IsResourceConflict(err)).To(BeTrue())
+		Expect(err).To(MatchError(&core_store.ResourceConflictError{}))
 		Expect(test_metrics.FindMetric(metrics, "store_conflicts", "resource_type", "Mesh").GetCounter().GetValue()).To(Equal(1.0))
 	})
 })

pkg/plugins/ca/builtin/manager.go

@@ -108,7 +108,7 @@ func (b *builtinCaManager) create(ctx context.Context, mesh core_model.Resource,
 		},
 	}
 	if err := b.secretManager.Create(ctx, certSecret, core_store.CreateWithOwner(mesh), core_store.CreateBy(certSecretResKey(meshName, backend.Name))); err != nil {
-		if !core_store.IsResourceAlreadyExists(err) {
+		if !errors.Is(err, &core_store.ResourceConflictError{}) {
 			return err
 		}
 		log.V(1).Info("CA certificate already exists. Nothing to create", "mesh", meshName, "backend", backend.Name)
@@ -120,7 +120,7 @@ func (b *builtinCaManager) create(ctx context.Context, mesh core_model.Resource,
 		},
 	}
 	if err := b.secretManager.Create(ctx, keySecret, core_store.CreateWithOwner(mesh), core_store.CreateBy(keySecretResKey(meshName, backend.Name))); err != nil {
-		if !core_store.IsResourceAlreadyExists(err) {
+		if !errors.Is(err, &core_store.ResourceConflictError{}) {
 			return err
 		}
 		log.V(1).Info("CA secret key already exists. Nothing to create", "mesh", meshName, "backend", backend.Name)

pkg/plugins/config/k8s/store_test.go

@@ -184,8 +184,7 @@ var _ = Describe("KubernetesStore", func() {
 			err = s.Update(context.Background(), config)
 
 			// then
-			Expect(err).To(HaveOccurred())
-			Expect(err.Error()).To(Equal(`Resource conflict: type="Config" name="kuma-internal-config" mesh=""`))
+			Expect(err).To(MatchError(&core_store.ResourceConflictError{}))
 		})
 	})