From 3f7792dac32747c4f0d2312b9701d3b928d15cbe Mon Sep 17 00:00:00 2001 From: Jakub Warczarek Date: Mon, 16 Dec 2024 11:42:40 +0100 Subject: [PATCH] feat(KongPlugin): generate K8s events for missing grant --- internal/dataplane/kongstate/customentity.go | 4 +- internal/dataplane/kongstate/kongstate.go | 142 ++++++++++-------- .../dataplane/kongstate/kongstate_test.go | 4 +- internal/gatewayapi/contraints.go | 13 +- test/envtest/configerrorevent_envtest_test.go | 22 ++- 5 files changed, 116 insertions(+), 69 deletions(-) diff --git a/internal/dataplane/kongstate/customentity.go b/internal/dataplane/kongstate/customentity.go index 179ec2fdf90..5536ea673b5 100644 --- a/internal/dataplane/kongstate/customentity.go +++ b/internal/dataplane/kongstate/customentity.go @@ -178,7 +178,7 @@ func (ks *KongState) FillCustomEntities( } // Fetch relations between plugins and services/routes/consumers and store the pointer to translated Kong entities. // Used for fetching entity referred by a custom entity and fill the ID of referred entity. - pluginRels := ks.getPluginRelatedEntitiesRef(s, logger) + pluginRels := ks.getPluginRelatedEntitiesRef(s, logger, failuresCollector) for _, entity := range entities { // reject the custom entity if its type is in "known" entity types that are already processed. @@ -333,7 +333,7 @@ func findCustomEntityRelatedPlugin(logger logr.Logger, cacheStore store.Storer, Namespace: parentRefNamespace, Name: parentRef.Name, } - paretRefNamespace, err := extractReferredPluginNamespace(logger, cacheStore, k8sEntity, nn) + paretRefNamespace, err := extractReferredPluginNamespaceIfAllowed(logger, cacheStore, k8sEntity, nn) if err != nil { return "", false, err } diff --git a/internal/dataplane/kongstate/kongstate.go b/internal/dataplane/kongstate/kongstate.go index 370c4817970..addc3c64b27 100644 --- a/internal/dataplane/kongstate/kongstate.go +++ b/internal/dataplane/kongstate/kongstate.go @@ -3,6 +3,7 @@ package kongstate import ( "bytes" "crypto/sha256" + "errors" "fmt" "strconv" "strings" @@ -378,7 +379,9 @@ type NamespacedKongPlugin struct { Name string } -func (ks *KongState) getPluginRelations(cacheStore store.Storer, log logr.Logger) map[string]util.ForeignRelations { +func (ks *KongState) getPluginRelations( + cacheStore store.Storer, log logr.Logger, failuresCollector *failures.ResourceFailuresCollector, +) map[string]util.ForeignRelations { // KongPlugin key (KongPlugin's name:namespace) to corresponding associations pluginRels := map[string]util.ForeignRelations{} @@ -400,9 +403,13 @@ func (ks *KongState) getPluginRelations(cacheStore store.Storer, log logr.Logger // // Code in buildPlugins() will combine plugin associations into // multi-entity plugins within the local namespace - namespace, err := extractReferredPluginNamespace(log, cacheStore, referrer, plugin) + namespace, err := extractReferredPluginNamespaceIfAllowed(log, cacheStore, referrer, plugin) if err != nil { - log.Error(err, "could not bind requested plugin", "plugin", plugin.Name, "namespace", plugin.Namespace) + if errors.As(err, &referredPluginNotAllowedError{}) { + failuresCollector.PushResourceFailure(err.Error(), referrer) + } else { + log.Error(err, "could not bind requested plugin", "plugin", plugin.Name, "namespace", plugin.Namespace) + } return } @@ -489,58 +496,6 @@ type pluginReference struct { Name string } -func isRemotePluginReferenceAllowed(log logr.Logger, s store.Storer, r pluginReference) error { - // remote plugin. considered part of this namespace if a suitable ReferenceGrant exists - grants, err := s.ListReferenceGrants() - if err != nil { - return fmt.Errorf("could not retrieve ReferenceGrants from store when building plugin relations map: %w", err) - } - allowed := gatewayapi.GetPermittedForReferenceGrantFrom( - log, - gatewayapi.ReferenceGrantFrom{ - Group: gatewayapi.Group(r.Referrer.GetObjectKind().GroupVersionKind().Group), - Kind: gatewayapi.Kind(r.Referrer.GetObjectKind().GroupVersionKind().Kind), - Namespace: gatewayapi.Namespace(r.Referrer.GetNamespace()), - }, - grants, - ) - - // TODO https://github.com/Kong/kubernetes-ingress-controller/issues/6000 - // Our reference checking code wasn't originally designed to handle multiple object types and relationships and - // wasn't designed with much guidance for future usage. It expects something akin to the BackendRef section of an - // HTTPRoute or similar, since that's what it was originally designed for. A future simplified model should probably - // work with source and target client.Object resources derived from the particular resources' reference specs, as - // those reference formats aren't necessarily consistent. We should possibly push for a standard upstream utility as - // part of https://github.com/kubernetes/enhancements/issues/3766 if it proceeds further, as this is can be difficult - // to wrap your head around when deep in the code that's checking an individual use case. A standard model for "these - // are the inputs and outputs for a reference grant check, derive them from your spec" should help avoid inconsistency - // in check implementation. - - // we don't have a full plugin resource here for the grant checker, so we build a fake one with the correct - // name and namespace. - pluginReference := gatewayapi.PluginLabelReference{ - Namespace: &r.Namespace, - Name: r.Name, - } - - // Because we should check whether it is allowed to refer FROM the referrer TO the plugin here, - // we should put the referrer on the "target" and the plugin on the "backendRef". - - log.V(logging.TraceLevel).Info("requested grant to plugins", - "from-namespace", r.Referrer.GetNamespace(), - "from-group", r.Referrer.GetObjectKind().GroupVersionKind().Group, - "from-kind", r.Referrer.GetObjectKind().GroupVersionKind().Kind, - "to-namespace", r.Namespace, - "to-name", r.Name, - ) - - if !gatewayapi.NewRefCheckerForKongPlugin(log, r.Referrer, pluginReference).IsRefAllowedByGrant(allowed) { - return fmt.Errorf("no grant found for %s in %s to plugin %s in %s requested remote KongPlugin bind", - r.Referrer.GetObjectKind().GroupVersionKind().Kind, r.Referrer.GetNamespace(), r.Name, r.Namespace) - } - return nil -} - func buildPlugins( logger logr.Logger, s store.Storer, @@ -691,7 +646,7 @@ func (ks *KongState) FillPlugins( s store.Storer, failuresCollector *failures.ResourceFailuresCollector, ) { - ks.Plugins = buildPlugins(log, s, failuresCollector, ks.getPluginRelations(s, log)) + ks.Plugins = buildPlugins(log, s, failuresCollector, ks.getPluginRelations(s, log, failuresCollector)) } // FillIDs iterates over the KongState and fills in the ID field for each entity @@ -819,15 +774,21 @@ func getServiceIDFromPluginRels( // // TODO: refactor the building of plugin related entities and share the result between here and building plugins: // https://github.com/Kong/kubernetes-ingress-controller/issues/6115 -func (ks *KongState) getPluginRelatedEntitiesRef(cacheStore store.Storer, log logr.Logger) PluginRelatedEntitiesRefs { +func (ks *KongState) getPluginRelatedEntitiesRef( + cacheStore store.Storer, log logr.Logger, failuresCollector *failures.ResourceFailuresCollector, +) PluginRelatedEntitiesRefs { pluginRels := PluginRelatedEntitiesRefs{ RelatedEntities: map[string]RelatedEntitiesRef{}, RouteAttachedService: map[string]*Service{}, } addRelation := func(referrer client.Object, plugin k8stypes.NamespacedName, entity any) { - namespace, err := extractReferredPluginNamespace(log, cacheStore, referrer, plugin) + namespace, err := extractReferredPluginNamespaceIfAllowed(log, cacheStore, referrer, plugin) if err != nil { - log.Error(err, "could not bind requested plugin", "plugin", plugin.Name, "namespace", plugin.Namespace) + if errors.As(err, &referredPluginNotAllowedError{}) { + failuresCollector.PushResourceFailure(err.Error(), referrer) + } else { + log.Error(err, "could not bind requested plugin", "plugin", plugin.Name, "namespace", plugin.Namespace) + } return } pluginKey := namespace + ":" + plugin.Name @@ -882,7 +843,15 @@ func (ks *KongState) getPluginRelatedEntitiesRef(cacheStore store.Storer, log lo return pluginRels } -func extractReferredPluginNamespace( +type referredPluginNotAllowedError struct { + pluginReference gatewayapi.PluginLabelReference +} + +func (e referredPluginNotAllowedError) Error() string { + return fmt.Sprintf("no grant found to referenced %q plugin in the requested remote KongPlugin bind", e.pluginReference) +} + +func extractReferredPluginNamespaceIfAllowed( log logr.Logger, cacheStore store.Storer, referrer client.Object, plugin k8stypes.NamespacedName, ) (string, error) { // There are 2 types of KongPlugin references: local and remote. @@ -911,3 +880,56 @@ func extractReferredPluginNamespace( } return plugin.Namespace, nil } + +func isRemotePluginReferenceAllowed(log logr.Logger, s store.Storer, r pluginReference) error { + // remote plugin. considered part of this namespace if a suitable ReferenceGrant exists + grants, err := s.ListReferenceGrants() + if err != nil { + return fmt.Errorf("could not retrieve ReferenceGrants from store when building plugin relations map: %w", err) + } + allowed := gatewayapi.GetPermittedForReferenceGrantFrom( + log, + gatewayapi.ReferenceGrantFrom{ + Group: gatewayapi.Group(r.Referrer.GetObjectKind().GroupVersionKind().Group), + Kind: gatewayapi.Kind(r.Referrer.GetObjectKind().GroupVersionKind().Kind), + Namespace: gatewayapi.Namespace(r.Referrer.GetNamespace()), + }, + grants, + ) + + // TODO https://github.com/Kong/kubernetes-ingress-controller/issues/6000 + // Our reference checking code wasn't originally designed to handle multiple object types and relationships and + // wasn't designed with much guidance for future usage. It expects something akin to the BackendRef section of an + // HTTPRoute or similar, since that's what it was originally designed for. A future simplified model should probably + // work with source and target client.Object resources derived from the particular resources' reference specs, as + // those reference formats aren't necessarily consistent. We should possibly push for a standard upstream utility as + // part of https://github.com/kubernetes/enhancements/issues/3766 if it proceeds further, as this is can be difficult + // to wrap your head around when deep in the code that's checking an individual use case. A standard model for "these + // are the inputs and outputs for a reference grant check, derive them from your spec" should help avoid inconsistency + // in check implementation. + + // we don't have a full plugin resource here for the grant checker, so we build a fake one with the correct + // name and namespace. + pluginReference := gatewayapi.PluginLabelReference{ + Namespace: &r.Namespace, + Name: r.Name, + } + + // Because we should check whether it is allowed to refer FROM the referrer TO the plugin here, + // we should put the referrer on the "target" and the plugin on the "backendRef". + + log.V(logging.TraceLevel).Info("requested grant to plugins", + "from-namespace", r.Referrer.GetNamespace(), + "from-group", r.Referrer.GetObjectKind().GroupVersionKind().Group, + "from-kind", r.Referrer.GetObjectKind().GroupVersionKind().Kind, + "to-namespace", r.Namespace, + "to-name", r.Name, + ) + + if !gatewayapi.NewRefCheckerForKongPlugin(log, r.Referrer, pluginReference).IsRefAllowedByGrant(allowed) { + return referredPluginNotAllowedError{ + pluginReference: pluginReference, + } + } + return nil +} diff --git a/internal/dataplane/kongstate/kongstate_test.go b/internal/dataplane/kongstate/kongstate_test.go index a4a62095321..b2866c0b8ee 100644 --- a/internal/dataplane/kongstate/kongstate_test.go +++ b/internal/dataplane/kongstate/kongstate_test.go @@ -677,7 +677,7 @@ func TestGetPluginRelations(t *testing.T) { t.Run(tt.name, func(t *testing.T) { store, err := store.NewFakeStore(store.FakeObjects{}) require.NoError(t, err) - computedPluginRelations := tt.data.inputState.getPluginRelations(store, logr.Discard()) + computedPluginRelations := tt.data.inputState.getPluginRelations(store, logr.Discard(), nil) if diff := cmp.Diff(tt.data.expectedPluginRelations, computedPluginRelations); diff != "" { t.Fatal("expected value differs from actual, see the human-readable diff:", diff) } @@ -819,7 +819,7 @@ func BenchmarkGetPluginRelations(b *testing.B) { b.ResetTimer() for i := 0; i < b.N; i++ { - fr := ks.getPluginRelations(store, logr.Discard()) + fr := ks.getPluginRelations(store, logr.Discard(), nil) _ = fr } } diff --git a/internal/gatewayapi/contraints.go b/internal/gatewayapi/contraints.go index c52e0f3ed5e..6008b5f8c99 100644 --- a/internal/gatewayapi/contraints.go +++ b/internal/gatewayapi/contraints.go @@ -1,6 +1,10 @@ package gatewayapi -import "sigs.k8s.io/controller-runtime/pkg/client" +import ( + "fmt" + + "sigs.k8s.io/controller-runtime/pkg/client" +) type HostnameT interface { Hostname | string @@ -33,3 +37,10 @@ type PluginLabelReference struct { Namespace *string Name string } + +func (plr PluginLabelReference) String() string { + if plr.Namespace == nil { + return plr.Name + } + return fmt.Sprintf("%s:%s", *plr.Namespace, plr.Name) +} diff --git a/test/envtest/configerrorevent_envtest_test.go b/test/envtest/configerrorevent_envtest_test.go index 7342e30fa43..fa513be7d21 100644 --- a/test/envtest/configerrorevent_envtest_test.go +++ b/test/envtest/configerrorevent_envtest_test.go @@ -66,7 +66,7 @@ func TestConfigErrorEventGenerationInMemoryMode(t *testing.T) { "konghq.com/protocol": "tcp", "konghq.com/path": "/aitmatov", // Referencing non-existent KongPlugins. - "konghq.com/plugins": "foo,bar", + "konghq.com/plugins": "foo,bar,n1:p1", } service.Namespace = ns.Name require.NoError(t, ctrlClient.Create(ctx, service)) @@ -102,7 +102,7 @@ func TestConfigErrorEventGenerationInMemoryMode(t *testing.T) { } t.Logf("got %d events", len(events.Items)) - const numberOfExpectedEvents = 7 + const numberOfExpectedEvents = 8 matches := make([]bool, numberOfExpectedEvents) matches[0] = lo.ContainsBy(events.Items, func(e corev1.Event) bool { return e.Type == corev1.EventTypeWarning && @@ -154,6 +154,13 @@ func TestConfigErrorEventGenerationInMemoryMode(t *testing.T) { e.InvolvedObject.Name == ingress.Name && e.Message == `referenced KongPlugin or KongClusterPlugin "baz" does not exist` }) + matches[7] = lo.ContainsBy(events.Items, func(e corev1.Event) bool { + return e.Type == corev1.EventTypeWarning && + e.Reason == dataplane.KongConfigurationTranslationFailedEventReason && + e.InvolvedObject.Kind == "Service" && + e.InvolvedObject.Name == service.Name && + e.Message == `no grant found to referenced "n1:p1" plugin in the requested remote KongPlugin bind` + }) if lo.Count(matches, true) != numberOfExpectedEvents { t.Logf("not all events matched: %+v", matches) return false @@ -311,7 +318,7 @@ func TestConfigErrorEventGenerationDBMode(t *testing.T) { Annotations: map[string]string{ annotations.IngressClassKey: ingressClassName, // Referencing non-existent KongPlugin. - "konghq.com/plugins": "foo", + "konghq.com/plugins": "foo, n1:p1", }, }, Username: "donenbai", @@ -342,7 +349,7 @@ func TestConfigErrorEventGenerationDBMode(t *testing.T) { } t.Logf("got %d events", len(events.Items)) - const numberOfExpectedEvents = 2 + const numberOfExpectedEvents = 3 matches := make([]bool, numberOfExpectedEvents) matches[0] = lo.ContainsBy(events.Items, func(e corev1.Event) bool { return e.Type == corev1.EventTypeWarning && @@ -358,6 +365,13 @@ func TestConfigErrorEventGenerationDBMode(t *testing.T) { e.InvolvedObject.Name == consumer.Name && e.Message == `referenced KongPlugin or KongClusterPlugin "foo" does not exist` }) + matches[2] = lo.ContainsBy(events.Items, func(e corev1.Event) bool { + return e.Type == corev1.EventTypeWarning && + e.Reason == dataplane.KongConfigurationTranslationFailedEventReason && + e.InvolvedObject.Kind == "KongConsumer" && + e.InvolvedObject.Name == consumer.Name && + e.Message == `no grant found to referenced "n1:p1" plugin in the requested remote KongPlugin bind` + }) if lo.Count(matches, true) != numberOfExpectedEvents { t.Logf("not all events matched: %+v", matches) return false