fix(kuma-cp): using policy name with "." causes hash to be inserted i…

…n the wrong place on the zone (#8240)

Since we allow a dot in the policy name, we can't use functions like CoreNameToK8sName that try to parse the name and make assumptions based on .<namespace> suffix.

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

pkg/core/resources/model/resource.go

@@ -10,9 +10,7 @@ import (
 	"k8s.io/kube-openapi/pkg/validation/spec"
 
 	common_api "github.com/kumahq/kuma/api/common/v1alpha1"
-	"github.com/kumahq/kuma/api/mesh/v1alpha1"
 	"github.com/kumahq/kuma/pkg/kds/hash"
-	util_k8s "github.com/kumahq/kuma/pkg/util/k8s"
 )
 
 const (
@@ -289,6 +287,18 @@ func (a ByMeta) Less(i, j int) bool {
 
 func (a ByMeta) Swap(i, j int) { a[i], a[j] = a[j], a[i] }
 
+const (
+	// K8sNamespaceComponent identifies the namespace component of a resource name on Kubernetes.
+	// The value is considered a part of user-facing Kuma API and should not be changed lightly.
+	// The value has a format of a Kubernetes label name.
+	K8sNamespaceComponent = "k8s.kuma.io/namespace"
+
+	// K8sNameComponent identifies the name component of a resource name on Kubernetes.
+	// The value is considered a part of user-facing Kuma API and should not be changed lightly.
+	// The value has a format of a Kubernetes label name.
+	K8sNameComponent = "k8s.kuma.io/name"
+)
+
 type ResourceType string
 
 // ResourceNameExtensions represents an composite resource name in environments
@@ -324,15 +334,21 @@ type ResourceMeta interface {
 // IsReferenced check if `refMeta` references with `refName` the entity `resourceMeta`
 // This is required because in multi-zone policies may have names different from the name they are defined with.
 func IsReferenced(refMeta ResourceMeta, refName string, resourceMeta ResourceMeta) bool {
+	if refMeta.GetMesh() != resourceMeta.GetMesh() {
+		return false
+	}
+
 	if len(refMeta.GetNameExtensions()) == 0 {
 		return equalNames(refMeta.GetMesh(), refName, resourceMeta.GetName())
 	}
 
-	if ns := refMeta.GetNameExtensions()[v1alpha1.KubeNamespaceTag]; ns != "" {
-		return equalNames(refMeta.GetMesh(), util_k8s.K8sNamespacedNameToCoreName(refName, ns), resourceMeta.GetName())
+	nsRef := refMeta.GetNameExtensions()[K8sNamespaceComponent]
+	nsRes := refMeta.GetNameExtensions()[K8sNamespaceComponent]
+	if nsRef == "" || nsRef != nsRes {
+		return false
 	}
 
-	return false
+	return equalNames(refMeta.GetMesh(), refName, resourceMeta.GetNameExtensions()[K8sNameComponent])
 }
 
 func equalNames(mesh, n1, n2 string) bool {

pkg/core/resources/model/resource_test.go

@@ -1,12 +1,17 @@
 package model_test
 
 import (
+	"fmt"
 	"reflect"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 
+	"github.com/kumahq/kuma/pkg/core/resources/model"
+	"github.com/kumahq/kuma/pkg/kds/hash"
+	"github.com/kumahq/kuma/pkg/plugins/common/k8s"
 	policies_api "github.com/kumahq/kuma/pkg/plugins/policies/meshaccesslog/api/v1alpha1"
+	test_model "github.com/kumahq/kuma/pkg/test/resources/model"
 )
 
 var _ = Describe("Resource", func() {
@@ -22,3 +27,62 @@ var _ = Describe("Resource", func() {
 		Expect(reflect.ValueOf(obj.GetSpec()).IsNil()).To(BeFalse())
 	})
 })
+
+var _ = Describe("IsReferenced", func() {
+	Context("Universal", func() {
+		meta := func(mesh, name string) *test_model.ResourceMeta {
+			return &test_model.ResourceMeta{Mesh: mesh, Name: name}
+		}
+		It("should return true when t1 is referencing route-1", func() {
+			Expect(model.IsReferenced(meta("m1", "t1"), "route-1", meta("m1", "route-1"))).To(BeTrue())
+		})
+		It("should return false when t1 is referencing route-2", func() {
+			Expect(model.IsReferenced(meta("m1", "t1"), "route-2", meta("m1", "route-1"))).To(BeFalse())
+		})
+		It("should return false when meshes are different", func() {
+			Expect(model.IsReferenced(meta("m1", "t1"), "route-1", meta("m2", "route-1"))).To(BeFalse())
+		})
+	})
+	Context("Kubernetes", func() {
+		meta := func(mesh, name string) *test_model.ResourceMeta {
+			return &test_model.ResourceMeta{
+				Mesh:           mesh,
+				Name:           fmt.Sprintf("%s.foo", name),
+				NameExtensions: k8s.ResourceNameExtensions("foo", name),
+			}
+		}
+		It("should return true when t1 is referencing route-1", func() {
+			Expect(model.IsReferenced(meta("m1", "t1"), "route-1", meta("m1", "route-1"))).To(BeTrue())
+		})
+		It("should return false when t1 is referencing route-2", func() {
+			Expect(model.IsReferenced(meta("m1", "t1"), "route-2", meta("m1", "route-1"))).To(BeFalse())
+		})
+		It("should return false when meshes are different", func() {
+			Expect(model.IsReferenced(meta("m1", "t1"), "route-1", meta("m2", "route-1"))).To(BeFalse())
+		})
+	})
+	Context("Kubernetes Zone", func() {
+		meta := func(mesh, name string) *test_model.ResourceMeta {
+			return &test_model.ResourceMeta{
+				Mesh:           mesh,
+				Name:           fmt.Sprintf("%s.foo", hash.SyncedNameInZone(mesh, name)),
+				NameExtensions: k8s.ResourceNameExtensions("foo", hash.SyncedNameInZone(mesh, name)),
+			}
+		}
+		It("should return true when t1 is referencing route-1", func() {
+			Expect(model.IsReferenced(meta("m1", "t1"), "route-1", meta("m1", "route-1"))).To(BeTrue())
+		})
+		It("should return true when route name has max allowed length", func() {
+			longRouteName := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" +
+				"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" +
+				"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaab"
+			Expect(model.IsReferenced(meta("m1", "t1"), longRouteName, meta("m1", longRouteName))).To(BeTrue())
+		})
+		It("should return false when t1 is referencing route-2", func() {
+			Expect(model.IsReferenced(meta("m1", "t1"), "route-2", meta("m1", "route-1"))).To(BeFalse())
+		})
+		It("should return false when meshes are different", func() {
+			Expect(model.IsReferenced(meta("m1", "t1"), "route-1", meta("m2", "route-1"))).To(BeFalse())
+		})
+	})
+})

pkg/kds/context/context.go

@@ -201,6 +201,10 @@ func AddHashSuffix(r model.Resource) (model.Resource, error) {
 	}
 
 	newObj := r.Descriptor().NewObject()
+
+	// When syncing mesh-scoped resources from Global to Zone, the only possible namespace on Global is system namespace.
+	// We always trim system namespace in RemoveK8sSystemNamespaceSuffixFromPluginOriginatedResourcesMapper,
+	// that's why r.GetMeta().GetName() never has a namespace suffix, so we can safely call SyncedNameInZone with it.
 	newObj.SetMeta(util.NewResourceMeta(hash.SyncedNameInZone(r.GetMeta().GetMesh(), r.GetMeta().GetName()), r.GetMeta().GetMesh()))
 	_ = newObj.SetSpec(r.GetSpec())
 

pkg/kds/context/context_test.go

@@ -450,6 +450,7 @@ var _ = Describe("Context", func() {
 			name                       string
 			expectedName               string
 			isResourcePluginOriginated bool
+			scope                      model.ResourceScope
 		}
 
 		genConfig := func(caseCfg config) kuma_cp.Config {
@@ -470,6 +471,7 @@ var _ = Describe("Context", func() {
 			descriptor := model.ResourceTypeDescriptor{
 				IsPluginOriginated: given.isResourcePluginOriginated,
 				Resource:           core_mesh.NewCircuitBreakerResource(),
+				Scope:              given.scope,
 			}
 
 			meta := &test_model.ResourceMeta{Name: given.name}
@@ -501,6 +503,7 @@ var _ = Describe("Context", func() {
 				},
 				name:         "foo.custom-namespace",
 				expectedName: "foo-zxw6c95d42zfz9cc",
+				scope:        model.ScopeMesh,
 			}),
 			Entry("shouldn't be removed when store type is kubernetes "+
 				"and resource isn't plugin originated", testCase{
@@ -509,28 +512,9 @@ var _ = Describe("Context", func() {
 					storeType:          config_store.KubernetesStore,
 					k8sSystemNamespace: "custom-namespace",
 				},
-				name:         "foo.custom-namespace",
-				expectedName: "foo-zxw6c95d42zfz9cc.custom-namespace",
-			}),
-			Entry("shouldn't be removed when store type is not kubernetes",
-				testCase{
-					isResourcePluginOriginated: true,
-					config: config{
-						storeType:          config_store.PostgresStore,
-						k8sSystemNamespace: "custom-namespace",
-					},
-					name:         "foo.custom-namespace",
-					expectedName: "foo-zxw6c95d42zfz9cc.custom-namespace",
-				}),
-			Entry("shouldn't be removed when suffix is not k8s system "+
-				"namespace", testCase{
-				isResourcePluginOriginated: true,
-				config: config{
-					storeType:          config_store.KubernetesStore,
-					k8sSystemNamespace: "kuma-system",
-				},
-				name:         "foo.custom-namespace",
-				expectedName: "foo-zxw6c95d42zfz9cc.custom-namespace",
+				name:         "foo.default",
+				expectedName: "foo.default",
+				scope:        model.ScopeGlobal,
 			}),
 		)
 	})

pkg/kds/hash/hash.go

@@ -7,19 +7,13 @@ import (
 
 	"k8s.io/apimachinery/pkg/util/rand"
 	k8s_strings "k8s.io/utils/strings"
-
-	util_k8s "github.com/kumahq/kuma/pkg/util/k8s"
 )
 
 // SyncedNameInZone returns the resource's name after syncing from Global to Zone.
 // It creates a new name by adding a hash suffix constructed from the 'mesh' and
 // the original 'name'.
 func SyncedNameInZone(mesh, name string) string {
-	if n, ns, err := util_k8s.CoreNameToK8sName(name); err == nil {
-		return util_k8s.K8sNamespacedNameToCoreName(addSuffix(n, hash(mesh, n)), ns)
-	} else {
-		return addSuffix(name, hash(mesh, name))
-	}
+	return addSuffix(name, hash(mesh, name))
 }
 
 func addSuffix(name, hash string) string {

pkg/plugins/common/k8s/names.go

@@ -5,16 +5,6 @@ import (
 )
 
 const (
-	// k8sNamespaceComponent identifies the namespace component of a resource name on Kubernetes.
-	// The value is considered a part of user-facing Kuma API and should not be changed lightly.
-	// The value has a format of a Kubernetes label name.
-	k8sNamespaceComponent = "k8s.kuma.io/namespace"
-
-	// k8sNameComponent identifies the name component of a resource name on Kubernetes.
-	// The value is considered a part of user-facing Kuma API and should not be changed lightly.
-	// The value has a format of a Kubernetes label name.
-	k8sNameComponent = "k8s.kuma.io/name"
-
 	// K8sMeshDefaultsGenerated identifies that default resources for mesh were successfully generated
 	K8sMeshDefaultsGenerated = "k8s.kuma.io/mesh-defaults-generated"
 
@@ -27,7 +17,7 @@ const (
 
 func ResourceNameExtensions(namespace, name string) core_model.ResourceNameExtensions {
 	return core_model.ResourceNameExtensions{
-		k8sNamespaceComponent: namespace,
-		k8sNameComponent:      name,
+		core_model.K8sNamespaceComponent: namespace,
+		core_model.K8sNameComponent:      name,
 	}
 }

pkg/plugins/resources/k8s/mapper.go

@@ -5,7 +5,6 @@ import (
 
 	"github.com/kumahq/kuma/pkg/core/resources/model"
 	k8s_model "github.com/kumahq/kuma/pkg/plugins/resources/k8s/native/pkg/model"
-	util_k8s "github.com/kumahq/kuma/pkg/util/k8s"
 )
 
 type ResourceMapperFunc func(resource model.Resource, namespace string) (k8s_model.KubernetesObject, error)
@@ -33,21 +32,13 @@ func NewInferenceMapper(systemNamespace string, kubeFactory KubeFactory) Resourc
 			return nil, err
 		}
 		if rs.Scope() == k8s_model.ScopeNamespace {
-			name, ns, err := util_k8s.CoreNameToK8sName(resource.GetMeta().GetName())
-			if err != nil {
-				// if the original resource doesn't look like a kubernetes name ("name"."namespace")`, just use the default namespace.
-				// this is in the case where someone calls this on a universal cluster. Exporting is a use-case for this.
-				ns = systemNamespace
-				name = resource.GetMeta().GetName()
-			}
 			if namespace != "" { // If the user is forcing the namespace accept it.
-				ns = namespace
+				rs.SetNamespace(namespace)
+			} else {
+				rs.SetNamespace(systemNamespace)
 			}
-			rs.SetName(name)
-			rs.SetNamespace(ns)
-		} else {
-			rs.SetName(resource.GetMeta().GetName())
 		}
+		rs.SetName(resource.GetMeta().GetName())
 		rs.SetMesh(resource.GetMeta().GetMesh())
 		rs.SetCreationTimestamp(v1.NewTime(resource.GetMeta().GetCreationTime()))
 		rs.SetSpec(resource.GetSpec())

pkg/plugins/resources/k8s/mapper_test.go

@@ -34,19 +34,6 @@ var _ = Describe("KubernetesStore template", func() {
 			Expect(res.GetNamespace()).To(Equal("core-ns"))
 		})
 
-		It("works with namespace in name on inference", func() {
-			in := core_mtp.NewMeshTrafficPermissionResource()
-			in.SetMeta(&rest_v1alpha1.ResourceMeta{Name: "foo.namespace", Mesh: "default"})
-
-			mapper := k8s.NewInferenceMapper("core-ns", &k8s.SimpleKubeFactory{KubeTypes: kubeTypes})
-			res, err := mapper(in, "")
-
-			Expect(err).ToNot(HaveOccurred())
-			Expect(res.GetMesh()).To(Equal("default"))
-			Expect(res.GetName()).To(Equal("foo"))
-			Expect(res.GetNamespace()).To(Equal("namespace"))
-		})
-
 		It("works passing namespace on inference", func() {
 			in := core_mtp.NewMeshTrafficPermissionResource()
 			in.SetMeta(&rest_v1alpha1.ResourceMeta{Name: "foo", Mesh: "default"})