fix(api-server): return 400 on invalid resource name

Before we were returning 500 when resource was not in the format `<name>.<namespace>` We now return a typed error in the store and have the api-server map the error to a 400 Fix #5705 Fix #4985 Signed-off-by: Charly Molter <[email protected]>
kumahq · Jan 17, 2023 · e1a8a78 · e1a8a78
1 parent 9648bc6
commit e1a8a78
Show file tree

Hide file tree

Showing 5 changed files with 55 additions and 1 deletion.
diff --git a/pkg/core/resources/store/store.go b/pkg/core/resources/store/store.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"io"
+	"reflect"
 	"strings"
 
 	"github.com/pkg/errors"
@@ -145,3 +146,15 @@ func IsResourceNotFound(err error) bool {
 func IsResourcePreconditionFailed(err error) bool {
 	return err != nil && strings.HasPrefix(err.Error(), "Resource precondition failed")
 }
+
+type PreconditionError struct {
+	Reason string
+}
+
+func (a *PreconditionError) Error() string {
+	return "invalid format: " + a.Reason
+}
+
+func (a *PreconditionError) Is(err error) bool {
+	return reflect.TypeOf(a) == reflect.TypeOf(err)
+}
diff --git a/pkg/core/rest/errors/error_handler.go b/pkg/core/rest/errors/error_handler.go
@@ -22,6 +22,13 @@ func HandleError(response *restful.Response, err error, title string) {
 		handleNotFound(title, response)
 	case store.IsResourcePreconditionFailed(err):
 		handlePreconditionFailed(title, response)
+	case errors.Is(err, &store.PreconditionError{}):
+		var err2 *store.PreconditionError
+		errors.As(err, &err2)
+		WriteError(response, 400, types.Error{
+			Title:   "Bad Request",
+			Details: err2.Reason,
+		})
 	case err == store.ErrorInvalidOffset:
 		handleInvalidOffset(title, response)
 	case manager.IsMeshNotFound(err):

diff --git a/pkg/plugins/resources/k8s/store.go b/pkg/plugins/resources/k8s/store.go
@@ -205,11 +205,18 @@ func (s *KubernetesStore) List(ctx context.Context, rs core_model.ResourceList,
 }
 
 func k8sNameNamespace(coreName string, scope k8s_model.Scope) (string, string, error) {
+	if coreName == "" {
+		return "", "", &store.PreconditionError{Reason: "name can't be empty"}
+	}
 	switch scope {
 	case k8s_model.ScopeCluster:
 		return coreName, "", nil
 	case k8s_model.ScopeNamespace:
-		return util_k8s.CoreNameToK8sName(coreName)
+		name, ns, err := util_k8s.CoreNameToK8sName(coreName)
+		if err != nil {
+			return "", "", &store.PreconditionError{Reason: err.Error()}
+		}
+		return name, ns, nil
 	default:
 		return "", "", errors.Errorf("unknown scope %s", scope)
 	}

diff --git a/pkg/plugins/resources/k8s/store_test.go b/pkg/plugins/resources/k8s/store_test.go
@@ -14,6 +14,8 @@ import (
 	core_mesh "github.com/kumahq/kuma/pkg/core/resources/apis/mesh"
 	core_model "github.com/kumahq/kuma/pkg/core/resources/model"
 	"github.com/kumahq/kuma/pkg/core/resources/store"
+	"github.com/kumahq/kuma/pkg/plugins/policies/meshtrace/api/v1alpha1"
+	v1alpha1_k8s "github.com/kumahq/kuma/pkg/plugins/policies/meshtrace/k8s/v1alpha1"
 	"github.com/kumahq/kuma/pkg/plugins/resources/k8s"
 	mesh_k8s "github.com/kumahq/kuma/pkg/plugins/resources/k8s/native/api/v1alpha1"
 	k8s_registry "github.com/kumahq/kuma/pkg/plugins/resources/k8s/native/pkg/registry"
@@ -73,6 +75,7 @@ var _ = Describe("KubernetesStore", func() {
 		kubeTypes := k8s_registry.NewTypeRegistry()
 		Expect(kubeTypes.RegisterObjectType(&mesh_proto.TrafficRoute{}, &mesh_k8s.TrafficRoute{})).To(Succeed())
 		Expect(kubeTypes.RegisterObjectType(&mesh_proto.Mesh{}, &mesh_k8s.Mesh{})).To(Succeed())
+		Expect(kubeTypes.RegisterObjectType(&v1alpha1.MeshTrace{}, &v1alpha1_k8s.MeshTrace{})).To(Succeed())
 		Expect(kubeTypes.RegisterListType(&mesh_proto.TrafficRoute{}, &mesh_k8s.TrafficRouteList{})).To(Succeed())
 		Expect(kubeTypes.RegisterListType(&mesh_proto.Mesh{}, &mesh_k8s.MeshList{})).To(Succeed())
 
@@ -435,6 +438,24 @@ var _ = Describe("KubernetesStore", func() {
 			Expect(err).To(MatchError(store.ErrorResourceNotFound(core_mesh.TrafficRouteType, name, mesh)))
 		})
 
+		It("should return an error if namespaced resource is not in the right format", func() {
+
+			// when
+			err := ks.Get(context.Background(), v1alpha1.NewMeshTraceResource(), store.GetByKey(name, mesh))
+
+			// then
+			Expect(err.Error()).To(ContainSubstring("must include namespace after the dot"))
+		})
+
+		It("should return an error if resource name is empty", func() {
+
+			// when
+			err := ks.Get(context.Background(), v1alpha1.NewMeshTraceResource(), store.GetByKey("", mesh))
+
+			// then
+			Expect(err.Error()).To(Equal("invalid format: name can't be empty"))
+		})
+
 		It("should return an existing resource", func() {
 			// setup
 			expected := backend.ParseYAML(fmt.Sprintf(`

diff --git a/test/e2e_env/kubernetes/inspect/inspect.go b/test/e2e_env/kubernetes/inspect/inspect.go
@@ -28,6 +28,12 @@ func Inspect() {
 		Expect(kubernetes.Cluster.DeleteMesh(meshName)).To(Succeed())
 	})
 
+	It("should return bad request on invalid name(#4985)", func() {
+		_, err := kubernetes.Cluster.GetKumactlOptions().RunKumactlAndGetOutput("inspect", "dataplane", "-m", meshName, "dummy-name", "--type=config-dump")
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring(`Bad Request (name "dummy-name" must include namespace after the dot, ex. "name.namespace")`))
+	})
+
 	It("should return envoy config_dump", func() {
 		// Synchronize on the dataplanes coming up.
 		Eventually(func(g Gomega) {