From 4db1cb9ff223b55e49740a47973111c3b30af676 Mon Sep 17 00:00:00 2001 From: Lukasz Dziedziak Date: Tue, 3 Dec 2024 18:00:57 +0100 Subject: [PATCH] fix(meshtrafficpermission): nil pointer for autoreachableservice when no top targetRef (#12152) Signed-off-by: Lukasz Dziedziak --- .../backends/reachable_backend_refs_graph.go | 62 +++ .../reachable_backend_refs_graph_test.go | 442 ++++++++++++++++++ .../graph/reachable_services_graph.go | 2 +- .../graph/reachable_services_graph_test.go | 8 + 4 files changed, 513 insertions(+), 1 deletion(-) create mode 100644 pkg/plugins/policies/meshtrafficpermission/graph/backends/reachable_backend_refs_graph.go create mode 100644 pkg/plugins/policies/meshtrafficpermission/graph/reachable_backend_refs_graph_test.go diff --git a/pkg/plugins/policies/meshtrafficpermission/graph/backends/reachable_backend_refs_graph.go b/pkg/plugins/policies/meshtrafficpermission/graph/backends/reachable_backend_refs_graph.go new file mode 100644 index 000000000000..1f2d2f8a2b6d --- /dev/null +++ b/pkg/plugins/policies/meshtrafficpermission/graph/backends/reachable_backend_refs_graph.go @@ -0,0 +1,62 @@ +package backends + +import ( + "maps" + + mesh_proto "github.com/kumahq/kuma/api/mesh/v1alpha1" + "github.com/kumahq/kuma/pkg/core" + ms_api "github.com/kumahq/kuma/pkg/core/resources/apis/meshservice/api/v1alpha1" + core_model "github.com/kumahq/kuma/pkg/core/resources/model" + core_rules "github.com/kumahq/kuma/pkg/plugins/policies/core/rules" + mtp_api "github.com/kumahq/kuma/pkg/plugins/policies/meshtrafficpermission/api/v1alpha1" + graph_util "github.com/kumahq/kuma/pkg/plugins/policies/meshtrafficpermission/graph/util" +) + +var log = core.Log.WithName("rms-graph") + +func BuildRules(meshServices []*ms_api.MeshServiceResource, mtps []*mtp_api.MeshTrafficPermissionResource) map[core_model.TypedResourceIdentifier]core_rules.Rules { + rules := map[core_model.TypedResourceIdentifier]core_rules.Rules{} + for _, ms := range meshServices { + dpTags := maps.Clone(ms.Spec.Selector.DataplaneTags) + if origin, ok := core_model.ResourceOrigin(ms.GetMeta()); ok { + dpTags[mesh_proto.ResourceOriginLabel] = string(origin) + } + if ms.GetMeta().GetLabels() != nil && ms.GetMeta().GetLabels()[mesh_proto.ZoneTag] != "" { + dpTags[mesh_proto.ZoneTag] = ms.GetMeta().GetLabels()[mesh_proto.ZoneTag] + } + rl, ok, err := graph_util.ComputeMtpRulesForTags(dpTags, trimNotSupportedTags(mtps, dpTags)) + if err != nil { + log.Error(err, "service could not be matched. It won't be reached by any other service", "service", ms.Meta.GetName()) + continue + } + if !ok { + continue + } + rules[core_model.NewTypedResourceIdentifier(ms)] = rl + } + return rules +} + +// trimNotSupportedTags removes tags that are not available in MeshService.dpTags + kuma.io/origin and kuma.io/zone +func trimNotSupportedTags(mtps []*mtp_api.MeshTrafficPermissionResource, supportedTags map[string]string) []*mtp_api.MeshTrafficPermissionResource { + newMtps := make([]*mtp_api.MeshTrafficPermissionResource, len(mtps)) + for i, mtp := range mtps { + if mtp.Spec != nil && mtp.Spec.TargetRef != nil && len(mtp.Spec.TargetRef.Tags) > 0 { + filteredTags := map[string]string{} + for tag, val := range mtp.Spec.TargetRef.Tags { + if _, ok := supportedTags[tag]; ok { + filteredTags[tag] = val + } + } + if len(filteredTags) != len(mtp.Spec.TargetRef.Tags) { + mtp = &mtp_api.MeshTrafficPermissionResource{ + Meta: mtp.Meta, + Spec: mtp.Spec.DeepCopy(), + } + mtp.Spec.TargetRef.Tags = filteredTags + } + } + newMtps[i] = mtp + } + return newMtps +} diff --git a/pkg/plugins/policies/meshtrafficpermission/graph/reachable_backend_refs_graph_test.go b/pkg/plugins/policies/meshtrafficpermission/graph/reachable_backend_refs_graph_test.go new file mode 100644 index 000000000000..80f632fc95f3 --- /dev/null +++ b/pkg/plugins/policies/meshtrafficpermission/graph/reachable_backend_refs_graph_test.go @@ -0,0 +1,442 @@ +package graph_test + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + common_api "github.com/kumahq/kuma/api/common/v1alpha1" + mesh_proto "github.com/kumahq/kuma/api/mesh/v1alpha1" + meshservice_api "github.com/kumahq/kuma/pkg/core/resources/apis/meshservice/api/v1alpha1" + "github.com/kumahq/kuma/pkg/core/resources/model" + core_rules "github.com/kumahq/kuma/pkg/plugins/policies/core/rules" + "github.com/kumahq/kuma/pkg/plugins/policies/meshtrafficpermission/api/v1alpha1" + "github.com/kumahq/kuma/pkg/plugins/policies/meshtrafficpermission/graph" + "github.com/kumahq/kuma/pkg/plugins/policies/meshtrafficpermission/graph/backends" + "github.com/kumahq/kuma/pkg/test/resources/builders" +) + +var _ = Describe("Reachable Backends Graph", func() { + type testCase struct { + mtps []*v1alpha1.MeshTrafficPermissionResource + expectedFromAll map[string]struct{} + expectedConnections map[string]map[string]struct{} + } + + services := []*meshservice_api.MeshServiceResource{ + builders.MeshService(). + WithName("a"). + WithDataplaneTagsSelectorKV("app", "a", "k8s.kuma.io/namespace", "kuma-demo"). + Build(), + builders.MeshService(). + WithName("b"). + WithDataplaneTagsSelectorKV("app", "b", "k8s.kuma.io/namespace", "kuma-demo"). + Build(), + builders.MeshService(). + WithName("c"). + WithDataplaneTagsSelectorKV("app", "c", "k8s.kuma.io/namespace", "test"). + Build(), + builders.MeshService(). + WithName("d"). + WithDataplaneTagsSelectorKV("app", "d", "k8s.kuma.io/namespace", "prod", "version", "v1"). + Build(), + } + + fromAllServices := map[string]struct{}{"a": {}, "b": {}, "c": {}, "d": {}} + + DescribeTable("should check reachability of the graph", + func(given testCase) { + // when + g := graph.NewGraph( + map[string]core_rules.Rules{}, + backends.BuildRules(services, given.mtps), + ) + + // then + for _, from := range services { + for _, to := range services { + _, fromAll := given.expectedFromAll[to.Meta.GetName()] + _, conn := given.expectedConnections[from.Meta.GetName()][to.Meta.GetName()] + Expect(g.CanReachBackend( + map[string]string{"app": from.Meta.GetName()}, + model.NewTypedResourceIdentifier(to), + )).To(Equal(fromAll || conn)) + } + } + }, + Entry("allow all", testCase{ + mtps: []*v1alpha1.MeshTrafficPermissionResource{ + builders.MeshTrafficPermission(). + WithTargetRef(builders.TargetRefMesh()). + AddFrom(builders.TargetRefMesh(), v1alpha1.Allow). + Build(), + }, + expectedFromAll: fromAllServices, + }), + Entry("allow all no top targetRef", testCase{ + mtps: []*v1alpha1.MeshTrafficPermissionResource{ + builders.MeshTrafficPermission(). + AddFrom(builders.TargetRefMesh(), v1alpha1.Allow). + Build(), + }, + expectedFromAll: fromAllServices, + }), + Entry("deny all", testCase{ + mtps: []*v1alpha1.MeshTrafficPermissionResource{ + builders.MeshTrafficPermission(). + WithTargetRef(builders.TargetRefMesh()). + AddFrom(builders.TargetRefMesh(), v1alpha1.Deny). + Build(), + }, + expectedFromAll: map[string]struct{}{}, + }), + Entry("no MeshTrafficPermissions", testCase{ + mtps: []*v1alpha1.MeshTrafficPermissionResource{}, + expectedFromAll: map[string]struct{}{}, + }), + Entry("one connection Allow", testCase{ + mtps: []*v1alpha1.MeshTrafficPermissionResource{ + builders.MeshTrafficPermission(). + WithTargetRef(builders.TargetRefMeshSubset("app", "b")). + AddFrom(builders.TargetRefMeshSubset("app", "a"), v1alpha1.Allow). + Build(), + }, + expectedConnections: map[string]map[string]struct{}{ + "a": {"b": {}}, + }, + }), + Entry("AllowWithShadowDeny is treated as Allow", testCase{ + mtps: []*v1alpha1.MeshTrafficPermissionResource{ + builders.MeshTrafficPermission(). + WithTargetRef(builders.TargetRefMeshSubset("app", "b")). + AddFrom(builders.TargetRefMeshSubset("app", "a"), v1alpha1.AllowWithShadowDeny). + Build(), + }, + expectedConnections: map[string]map[string]struct{}{ + "a": {"b": {}}, + }, + }), + Entry("multiple allowed connections", testCase{ + mtps: []*v1alpha1.MeshTrafficPermissionResource{ + builders.MeshTrafficPermission(). + WithTargetRef(builders.TargetRefMeshSubset("app", "b")). + AddFrom(builders.TargetRefMeshSubset("app", "a"), v1alpha1.Allow). + Build(), + builders.MeshTrafficPermission(). + WithTargetRef(builders.TargetRefMeshSubset("app", "c")). + AddFrom(builders.TargetRefMeshSubset("app", "b"), v1alpha1.AllowWithShadowDeny). + Build(), + builders.MeshTrafficPermission(). + WithTargetRef(builders.TargetRefMeshSubset("app", "d")). + AddFrom(builders.TargetRefMesh(), v1alpha1.Allow). + Build(), + }, + expectedFromAll: map[string]struct{}{ + "d": {}, + }, + expectedConnections: map[string]map[string]struct{}{ + "a": {"b": {}}, + "b": {"c": {}}, + }, + }), + Entry("all allowed except one connection", testCase{ + mtps: []*v1alpha1.MeshTrafficPermissionResource{ + builders.MeshTrafficPermission(). + WithTargetRef(builders.TargetRefMesh()). + AddFrom(builders.TargetRefMesh(), v1alpha1.Allow). + Build(), + builders.MeshTrafficPermission(). + WithTargetRef(builders.TargetRefMeshSubset("app", "b")). + AddFrom(builders.TargetRefMeshSubset("app", "a"), v1alpha1.Deny). + Build(), + }, + expectedFromAll: map[string]struct{}{ + "a": {}, + "c": {}, + "d": {}, + }, + expectedConnections: map[string]map[string]struct{}{ + "c": {"b": {}}, + "d": {"b": {}}, + "b": {"b": {}}, + }, + }), + Entry("allow all but one service has restrictive mesh traffic permission", testCase{ + mtps: []*v1alpha1.MeshTrafficPermissionResource{ + builders.MeshTrafficPermission(). + WithTargetRef(builders.TargetRefMesh()). + AddFrom(builders.TargetRefMesh(), v1alpha1.Allow). + Build(), + builders.MeshTrafficPermission(). + WithTargetRef(builders.TargetRefMeshSubset("app", "b")). + AddFrom(builders.TargetRefMesh(), v1alpha1.Deny). + AddFrom(builders.TargetRefMeshSubset("app", "a"), v1alpha1.Allow). + Build(), + }, + expectedFromAll: map[string]struct{}{ + "a": {}, + "c": {}, + "d": {}, + }, + expectedConnections: map[string]map[string]struct{}{ + "a": {"b": {}}, + }, + }), + Entry("top level target ref MeshSubset with unsupported predefined tags selects all", testCase{ + mtps: []*v1alpha1.MeshTrafficPermissionResource{ + builders.MeshTrafficPermission(). + WithTargetRef(builders.TargetRefMeshSubset("kuma.io/zone", "east")). + AddFrom(builders.TargetRefMesh(), v1alpha1.Allow). + Build(), + }, + expectedFromAll: fromAllServices, + }), + Entry("top level target ref MeshServiceSubset of unsupported predefined tags selects all instances of the service", testCase{ + mtps: []*v1alpha1.MeshTrafficPermissionResource{ + builders.MeshTrafficPermission(). + WithTargetRef(builders.TargetRefMeshSubset("app", "a", "kuma.io/zone", "east")). + AddFrom(builders.TargetRefMesh(), v1alpha1.Allow). + Build(), + }, + expectedFromAll: map[string]struct{}{ + "a": {}, + }, + }), + Entry("equal subsets matching is preserved because of the names", testCase{ + mtps: []*v1alpha1.MeshTrafficPermissionResource{ + builders.MeshTrafficPermission(). + WithName("bbb"). + WithTargetRef(builders.TargetRefMeshSubset("kuma.io/zone", "east")). + AddFrom(builders.TargetRefMesh(), v1alpha1.Deny). + Build(), + builders.MeshTrafficPermission(). + WithName("aaa"). + WithTargetRef(builders.TargetRefMeshSubset("version", "v1")). + AddFrom(builders.TargetRefMesh(), v1alpha1.Allow). + Build(), + }, + expectedFromAll: fromAllServices, + }), + ) + + It("should work with service subsets in from", func() { + // given + services := []*meshservice_api.MeshServiceResource{ + builders.MeshService(). + WithName("a"). + WithDataplaneTagsSelectorKV("app", "a", "k8s.kuma.io/namespace", "kuma-demo"). + Build(), + } + mtps := []*v1alpha1.MeshTrafficPermissionResource{ + builders.MeshTrafficPermission(). + WithTargetRef(builders.TargetRefMesh()). + AddFrom(builders.TargetRefMeshSubset("app", "b", "version", "v1"), v1alpha1.Allow). + Build(), + } + + // when + g := graph.NewGraph( + map[string]core_rules.Rules{}, + backends.BuildRules(services, mtps), + ) + // then + Expect(g.CanReachBackend( + map[string]string{"app": "b", "version": "v1"}, + model.TypedResourceIdentifier{ + ResourceType: meshservice_api.MeshServiceType, + ResourceIdentifier: model.ResourceIdentifier{ + Name: "a", + Mesh: "default", + }, + }, + )).To(BeTrue()) + Expect(g.CanReachBackend( + map[string]string{mesh_proto.ServiceTag: "b", "version": "v2"}, + model.TypedResourceIdentifier{ + ResourceType: meshservice_api.MeshServiceType, + ResourceIdentifier: model.ResourceIdentifier{ + Name: "a", + Mesh: "default", + }, + }, + )).To(BeFalse()) + }) + + It("should work with mesh subset in from", func() { + services := []*meshservice_api.MeshServiceResource{ + builders.MeshService(). + WithName("a"). + WithDataplaneTagsSelectorKV("app", "a", "k8s.kuma.io/namespace", "kuma-demo"). + Build(), + } + mtps := []*v1alpha1.MeshTrafficPermissionResource{ + builders.MeshTrafficPermission(). + WithTargetRef(builders.TargetRefMesh()). + AddFrom(builders.TargetRefMeshSubset("kuma.io/zone", "east"), v1alpha1.Allow). + Build(), + } + + // when + g := graph.NewGraph( + map[string]core_rules.Rules{}, + backends.BuildRules(services, mtps), + ) + + // then + Expect(g.CanReachBackend( + map[string]string{"kuma.io/zone": "east"}, + model.TypedResourceIdentifier{ + ResourceType: meshservice_api.MeshServiceType, + ResourceIdentifier: model.ResourceIdentifier{ + Name: "a", + Mesh: "default", + }, + }, + )).To(BeTrue()) + Expect(g.CanReachBackend( + map[string]string{"kuma.io/zone": "west"}, + model.TypedResourceIdentifier{ + ResourceType: meshservice_api.MeshServiceType, + ResourceIdentifier: model.ResourceIdentifier{ + Name: "a", + Mesh: "default", + }, + }, + )).To(BeFalse()) + Expect(g.CanReachBackend( + map[string]string{"othertag": "other"}, + model.TypedResourceIdentifier{ + ResourceType: meshservice_api.MeshServiceType, + ResourceIdentifier: model.ResourceIdentifier{ + Name: "a", + Mesh: "default", + }, + }, + )).To(BeFalse()) + }) + + DescribeTable("top level subset should work with predefined tags", + func(targetRef common_api.TargetRef) { + // given + services := []*meshservice_api.MeshServiceResource{ + builders.MeshService(). + WithName("a"). + WithDataplaneTagsSelectorKV("app", "a", "k8s.kuma.io/namespace", "kuma-demo", "k8s.kuma.io/service-name", "a", "k8s.kuma.io/service-port", "1234"). + Build(), + builders.MeshService(). + WithName("b"). + WithDataplaneTagsSelectorKV("app", "b", "k8s.kuma.io/namespace", "not-matching", "k8s.kuma.io/service-name", "b", "k8s.kuma.io/service-port", "9999"). + Build(), + } + mtps := []*v1alpha1.MeshTrafficPermissionResource{ + builders.MeshTrafficPermission(). + WithTargetRef(targetRef). + AddFrom(builders.TargetRefMesh(), v1alpha1.Allow). + Build(), + } + + // when + g := graph.NewGraph( + map[string]core_rules.Rules{}, + backends.BuildRules(services, mtps), + ) + + // then + Expect(g.CanReachBackend( + map[string]string{"app": "b"}, + model.TypedResourceIdentifier{ + ResourceType: meshservice_api.MeshServiceType, + ResourceIdentifier: model.ResourceIdentifier{ + Name: "a", + Mesh: "default", + }, + }, + )).To(BeTrue()) + Expect(g.CanReachBackend( + map[string]string{"app": "a"}, + model.TypedResourceIdentifier{ + ResourceType: meshservice_api.MeshServiceType, + ResourceIdentifier: model.ResourceIdentifier{ + Name: "b", + Mesh: "default", + }, + }, + )).To(BeFalse()) // it's not selected by top-level target ref + }, + Entry("MeshSubset by kube namespace", builders.TargetRefMeshSubset(mesh_proto.KubeNamespaceTag, "kuma-demo")), + Entry("MeshSubset by kube service name", builders.TargetRefMeshSubset(mesh_proto.KubeServiceTag, "a")), + Entry("MeshSubset by kube service port", builders.TargetRefMeshSubset(mesh_proto.KubePortTag, "1234")), + Entry("MeshSubset by app and kube namespace", builders.TargetRefMeshSubset("app", "a", mesh_proto.KubeNamespaceTag, "kuma-demo")), + Entry("MeshSubset by app and kube service name", builders.TargetRefMeshSubset("app", "a", mesh_proto.KubeServiceTag, "a")), + Entry("MeshSubset by app and kube service port", builders.TargetRefMeshSubset("app", "a", mesh_proto.KubePortTag, "1234")), + ) + + It("should match only dp with specific labels", func() { + // given + services := []*meshservice_api.MeshServiceResource{ + builders.MeshService(). + WithName("a"). + WithDataplaneTagsSelectorKV("app", "a", "k8s.kuma.io/namespace", "kuma-demo", "k8s.kuma.io/service-name", "a", "k8s.kuma.io/service-port", "1234"). + Build(), + builders.MeshService(). + WithName("b"). + WithDataplaneTagsSelectorKV("app", "b", "k8s.kuma.io/namespace", "not-available"). + Build(), + } + + mtps := []*v1alpha1.MeshTrafficPermissionResource{ + builders.MeshTrafficPermission(). + WithTargetRef(builders.TargetRefMeshSubset(mesh_proto.KubeNamespaceTag, "kuma-demo")). + AddFrom(builders.TargetRefMesh(), v1alpha1.Allow). + Build(), + } + + // when + g := graph.NewGraph( + map[string]core_rules.Rules{}, + backends.BuildRules(services, mtps), + ) + + // then + Expect(g.CanReachBackend( + map[string]string{"app": "b"}, + model.TypedResourceIdentifier{ + ResourceType: meshservice_api.MeshServiceType, + ResourceIdentifier: model.ResourceIdentifier{ + Name: "a", + Mesh: "default", + }, + }, + )).To(BeTrue()) + Expect(g.CanReachBackend( + map[string]string{"app": "a"}, + model.TypedResourceIdentifier{ + ResourceType: meshservice_api.MeshServiceType, + ResourceIdentifier: model.ResourceIdentifier{ + Name: "b", + Mesh: "default", + }, + }, + )).To(BeFalse()) + }) + + It("should not modify MeshTrafficPermission passed to the func when replacing tags in subsets", func() { + // given + mtps := []*v1alpha1.MeshTrafficPermissionResource{ + builders.MeshTrafficPermission(). + WithTargetRef(builders.TargetRefMeshSubset("version", "v1")). + AddFrom(builders.TargetRefMesh(), v1alpha1.Allow). + Build(), + builders.MeshTrafficPermission(). + WithTargetRef(builders.TargetRefServiceSubset("a", "version", "v1")). + AddFrom(builders.TargetRefMesh(), v1alpha1.Allow). + Build(), + } + + // when + _ = backends.BuildRules(services, mtps) + + // then + Expect(mtps[0].Spec.TargetRef.Tags).NotTo(BeNil()) + Expect(mtps[1].Spec.TargetRef.Tags).NotTo(BeNil()) + }) +}) diff --git a/pkg/plugins/policies/meshtrafficpermission/graph/reachable_services_graph.go b/pkg/plugins/policies/meshtrafficpermission/graph/reachable_services_graph.go index 299c05039da6..ef3615fc42ef 100644 --- a/pkg/plugins/policies/meshtrafficpermission/graph/reachable_services_graph.go +++ b/pkg/plugins/policies/meshtrafficpermission/graph/reachable_services_graph.go @@ -154,7 +154,7 @@ func BuildGraph(services map[string]mesh_proto.SingleValueTagSet, mtps []*mtp_ap func trimNotSupportedTags(mtps []*mtp_api.MeshTrafficPermissionResource) []*mtp_api.MeshTrafficPermissionResource { newMtps := make([]*mtp_api.MeshTrafficPermissionResource, len(mtps)) for i, mtp := range mtps { - if len(mtp.Spec.TargetRef.Tags) > 0 { + if mtp.Spec != nil && mtp.Spec.TargetRef != nil && len(mtp.Spec.TargetRef.Tags) > 0 { filteredTags := map[string]string{} for tag, val := range mtp.Spec.TargetRef.Tags { if _, ok := SupportedTags[tag]; ok { diff --git a/pkg/plugins/policies/meshtrafficpermission/graph/reachable_services_graph_test.go b/pkg/plugins/policies/meshtrafficpermission/graph/reachable_services_graph_test.go index af94e81498f1..5b6cffef6a89 100644 --- a/pkg/plugins/policies/meshtrafficpermission/graph/reachable_services_graph_test.go +++ b/pkg/plugins/policies/meshtrafficpermission/graph/reachable_services_graph_test.go @@ -56,6 +56,14 @@ var _ = Describe("Reachable Services Graph", func() { }, expectedFromAll: fromAllServices, }), + Entry("allow all no top targetRef", testCase{ + mtps: []*v1alpha1.MeshTrafficPermissionResource{ + builders.MeshTrafficPermission(). + AddFrom(builders.TargetRefMesh(), v1alpha1.Allow). + Build(), + }, + expectedFromAll: fromAllServices, + }), Entry("deny all", testCase{ mtps: []*v1alpha1.MeshTrafficPermissionResource{ builders.MeshTrafficPermission().