feat: propagate translation failures for KongPlugin and KongClusterPl…

…ugin (#4428)
Kong · Jul 31, 2023 · 04c465e · 04c465e
1 parent 1e260dd
commit 04c465e
Show file tree

Hide file tree

Showing 5 changed files with 158 additions and 33 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -138,6 +138,11 @@ Adding a new version? You'll need three changes:
   `Programmed` condition of an object when `kubectl get` is used.
   [#4425](https://github.com/Kong/kubernetes-ingress-controller/pull/4425)
   [#4423](https://github.com/Kong/kubernetes-ingress-controller/pull/4423)
+- Parser instead of logging errors for invalid `KongPlugin` or `KongClusterPlugin`
+  configuration, will now propagate a translation failure that will result
+  in the `Programmed` condition of the object being set to `False` and an
+  event being emitted.
+  [#4428](https://github.com/Kong/kubernetes-ingress-controller/pull/4428)
 
 ### Changed
 

diff --git a/internal/dataplane/kongstate/kongstate.go b/internal/dataplane/kongstate/kongstate.go
@@ -289,21 +289,42 @@ func (ks *KongState) getPluginRelations() map[string]util.ForeignRelations {
 	return pluginRels
 }
 
-func buildPlugins(log logrus.FieldLogger, s store.Storer, pluginRels map[string]util.ForeignRelations) []Plugin {
+func buildPlugins(
+	log logrus.FieldLogger,
+	s store.Storer,
+	failuresCollector *failures.ResourceFailuresCollector,
+	pluginRels map[string]util.ForeignRelations,
+) []Plugin {
 	var plugins []Plugin
 
 	for pluginIdentifier, relations := range pluginRels {
 		identifier := strings.Split(pluginIdentifier, ":")
 		namespace, kongPluginName := identifier[0], identifier[1]
-		plugin, err := getPlugin(s, namespace, kongPluginName)
+		k8sPlugin, k8sClusterPlugin, err := getKongPluginOrKongClusterPlugin(s, namespace, kongPluginName)
 		if err != nil {
 			log.WithFields(logrus.Fields{
 				"kongplugin_name":      kongPluginName,
 				"kongplugin_namespace": namespace,
-			}).WithError(err).Errorf("failed to fetch KongPlugin")
+			}).WithError(err).Errorf("failed to fetch KongPlugin resource")
 			continue
 		}
 
+		var plugin Plugin
+		if k8sPlugin != nil {
+			plugin, err = kongPluginFromK8SPlugin(s, *k8sPlugin)
+			if err != nil {
+				failuresCollector.PushResourceFailure(err.Error(), k8sPlugin)
+				continue
+			}
+		}
+		if k8sClusterPlugin != nil {
+			plugin, err = kongPluginFromK8SClusterPlugin(s, *k8sClusterPlugin)
+			if err != nil {
+				failuresCollector.PushResourceFailure(err.Error(), k8sClusterPlugin)
+				continue
+			}
+		}
+
 		for _, rel := range relations.GetCombinations() {
 			plugin := plugin.DeepCopy()
 			// ID is populated because that is read by decK and in_memory
@@ -389,8 +410,12 @@ func globalPlugins(log logrus.FieldLogger, s store.Storer) ([]Plugin, error) {
 	return plugins, nil
 }
 
-func (ks *KongState) FillPlugins(log logrus.FieldLogger, s store.Storer) {
-	ks.Plugins = buildPlugins(log, s, ks.getPluginRelations())
+func (ks *KongState) FillPlugins(
+	log logrus.FieldLogger,
+	s store.Storer,
+	failuresCollector *failures.ResourceFailuresCollector,
+) {
+	ks.Plugins = buildPlugins(log, s, failuresCollector, ks.getPluginRelations())
 }
 
 // FillIDs iterates over the KongState and fills in the ID field for each entity

diff --git a/internal/dataplane/kongstate/util.go b/internal/dataplane/kongstate/util.go
@@ -78,37 +78,34 @@ func getKongIngressFromObjAnnotations(
 	return nil, nil
 }
 
-// getPlugin constructs a plugins from a KongPlugin resource.
-func getPlugin(s store.Storer, namespace, name string) (Plugin, error) {
-	var plugin Plugin
-	k8sPlugin, err := s.GetKongPlugin(namespace, name)
-	if err != nil {
-		// if no namespaced plugin definition, then
-		// search for cluster level-plugin definition
-		if errors.As(err, &store.ErrNotFound{}) {
-			clusterPlugin, err := s.GetKongClusterPlugin(name)
-			// not found
-			if errors.As(err, &store.ErrNotFound{}) {
-				return plugin, errors.New(
-					"no KongPlugin or KongClusterPlugin was found")
-			}
-			if err != nil {
-				return plugin, err
-			}
-			if clusterPlugin.PluginName == "" {
-				return plugin, fmt.Errorf("invalid empty 'plugin' property")
+// getKongPluginOrKongClusterPlugin fetches a KongPlugin or KongClusterPlugin (as fallback) from the store.
+// If both are not found, an error is returned.
+func getKongPluginOrKongClusterPlugin(s store.Storer, namespace, name string) (
+	*configurationv1.KongPlugin,
+	*configurationv1.KongClusterPlugin,
+	error,
+) {
+	plugin, pluginErr := s.GetKongPlugin(namespace, name)
+	if pluginErr != nil {
+		if !errors.As(pluginErr, &store.ErrNotFound{}) {
+			return nil, nil, fmt.Errorf("failed fetching KongPlugin: %w", pluginErr)
+		}
+
+		// If KongPlugin is not found, try to fetch KongClusterPlugin.
+		clusterPlugin, err := s.GetKongClusterPlugin(name)
+		if err != nil {
+			if !errors.As(err, &store.ErrNotFound{}) {
+				return nil, nil, fmt.Errorf("failed fetching KongClusterPlugin: %w", err)
 			}
-			plugin, err = kongPluginFromK8SClusterPlugin(s, *clusterPlugin)
-			return plugin, err
+
+			// Both KongPlugin and KongClusterPlugin are not found.
+			return nil, nil, fmt.Errorf("no KongPlugin or KongClusterPlugin was found for %s/%s", namespace, name)
 		}
-	}
-	// ignore plugins with no name
-	if k8sPlugin.PluginName == "" {
-		return plugin, fmt.Errorf("invalid empty 'plugin' property")
+
+		return nil, clusterPlugin, nil
 	}
 
-	plugin, err = kongPluginFromK8SPlugin(s, *k8sPlugin)
-	return plugin, err
+	return plugin, nil, nil
 }
 
 func kongPluginFromK8SClusterPlugin(

diff --git a/internal/dataplane/parser/parser.go b/internal/dataplane/parser/parser.go
@@ -233,7 +233,7 @@ func (p *Parser) BuildKongConfig() KongConfigBuildingResult {
 	}
 
 	// process annotation plugins
-	result.FillPlugins(p.logger, p.storer)
+	result.FillPlugins(p.logger, p.storer, p.failuresCollector)
 	for i := range result.Plugins {
 		p.registerSuccessfullyParsedObject(result.Plugins[i].K8sParent)
 	}

diff --git a/test/envtest/programmed_condition_envtest_test.go b/test/envtest/programmed_condition_envtest_test.go
@@ -6,6 +6,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/require"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	k8stypes "k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -38,6 +39,7 @@ func TestKongCRDs_ProgrammedCondition(t *testing.T) {
 		objects                     []client.Object
 		getExpectedObjectConditions func(ctrlClient client.Client) ([]metav1.Condition, error)
 		expectedProgrammedStatus    metav1.ConditionStatus
+		expectedProgrammedReason    kongv1.ConditionReason
 	}{
 		{
 			name: "valid KongConsumer",
@@ -65,6 +67,7 @@ func TestKongCRDs_ProgrammedCondition(t *testing.T) {
 				return consumer.Status.Conditions, nil
 			},
 			expectedProgrammedStatus: metav1.ConditionTrue,
+			expectedProgrammedReason: kongv1.ReasonProgrammed,
 		},
 		{
 			name: "KongConsumer referencing non-existent secret",
@@ -93,6 +96,7 @@ func TestKongCRDs_ProgrammedCondition(t *testing.T) {
 				return consumer.Status.Conditions, nil
 			},
 			expectedProgrammedStatus: metav1.ConditionFalse,
+			expectedProgrammedReason: kongv1.ReasonInvalid,
 		},
 		{
 			name: "valid KongConsumerGroup",
@@ -119,6 +123,7 @@ func TestKongCRDs_ProgrammedCondition(t *testing.T) {
 				return consumerGroup.Status.Conditions, nil
 			},
 			expectedProgrammedStatus: metav1.ConditionTrue,
+			expectedProgrammedReason: kongv1.ReasonProgrammed,
 		},
 		{
 			name: "valid KongPlugin",
@@ -155,6 +160,52 @@ func TestKongCRDs_ProgrammedCondition(t *testing.T) {
 				return plugin.Status.Conditions, nil
 			},
 			expectedProgrammedStatus: metav1.ConditionTrue,
+			expectedProgrammedReason: kongv1.ReasonProgrammed,
+		},
+		{
+			name: "invalid KongPlugin",
+			objects: []client.Object{
+				&kongv1.KongPlugin{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:        "invalid-kong-plugin",
+						Namespace:   ns.Name,
+						Annotations: map[string]string{annotations.IngressClassKey: annotations.DefaultIngressClass},
+					},
+					PluginName: "plugin",
+					// Specifying both Config and ConfigFrom is invalid.
+					Config: apiextensionsv1.JSON{Raw: []byte(`{"key": "value"}`)},
+					ConfigFrom: &kongv1.ConfigSource{
+						SecretValue: kongv1.SecretValueFromSource{
+							Secret: "secret",
+							Key:    "key",
+						},
+					},
+				},
+				&kongv1.KongConsumer{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "consumer-for-invalid-plugin",
+						Namespace: ns.Name,
+						Annotations: map[string]string{
+							annotations.IngressClassKey:                           annotations.DefaultIngressClass,
+							annotations.AnnotationPrefix + annotations.PluginsKey: "invalid-kong-plugin",
+						},
+					},
+					Username: "foo",
+				},
+			},
+			getExpectedObjectConditions: func(ctrlClient client.Client) ([]metav1.Condition, error) {
+				var plugin kongv1.KongPlugin
+				err := ctrlClient.Get(ctx, k8stypes.NamespacedName{
+					Name:      "invalid-kong-plugin",
+					Namespace: ns.Name,
+				}, &plugin)
+				if err != nil {
+					return nil, err
+				}
+				return plugin.Status.Conditions, nil
+			},
+			expectedProgrammedStatus: metav1.ConditionFalse,
+			expectedProgrammedReason: kongv1.ReasonInvalid,
 		},
 		{
 			name: "valid KongClusterPlugin",
@@ -189,6 +240,52 @@ func TestKongCRDs_ProgrammedCondition(t *testing.T) {
 				return clusterPlugin.Status.Conditions, nil
 			},
 			expectedProgrammedStatus: metav1.ConditionTrue,
+			expectedProgrammedReason: kongv1.ReasonProgrammed,
+		},
+		{
+			name: "invalid KongClusterPlugin",
+			objects: []client.Object{
+				&kongv1.KongPlugin{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:        "invalid-kong-cluster-plugin",
+						Namespace:   ns.Name,
+						Annotations: map[string]string{annotations.IngressClassKey: annotations.DefaultIngressClass},
+					},
+					PluginName: "plugin",
+					// Specifying both Config and ConfigFrom is invalid.
+					Config: apiextensionsv1.JSON{Raw: []byte(`{"key": "value"}`)},
+					ConfigFrom: &kongv1.ConfigSource{
+						SecretValue: kongv1.SecretValueFromSource{
+							Secret: "secret",
+							Key:    "key",
+						},
+					},
+				},
+				&kongv1.KongConsumer{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "consumer-for-invalid-cluster-plugin",
+						Namespace: ns.Name,
+						Annotations: map[string]string{
+							annotations.IngressClassKey:                           annotations.DefaultIngressClass,
+							annotations.AnnotationPrefix + annotations.PluginsKey: "invalid-kong-cluster-plugin",
+						},
+					},
+					Username: "foo",
+				},
+			},
+			getExpectedObjectConditions: func(ctrlClient client.Client) ([]metav1.Condition, error) {
+				var plugin kongv1.KongPlugin
+				err := ctrlClient.Get(ctx, k8stypes.NamespacedName{
+					Name:      "invalid-kong-cluster-plugin",
+					Namespace: ns.Name,
+				}, &plugin)
+				if err != nil {
+					return nil, err
+				}
+				return plugin.Status.Conditions, nil
+			},
+			expectedProgrammedStatus: metav1.ConditionFalse,
+			expectedProgrammedReason: kongv1.ReasonInvalid,
 		},
 	}
 
@@ -208,6 +305,7 @@ func TestKongCRDs_ProgrammedCondition(t *testing.T) {
 				if !conditions.Contain(
 					cs,
 					conditions.WithType(string(kongv1.ConditionProgrammed)),
+					conditions.WithReason(string(tc.expectedProgrammedReason)),
 					conditions.WithStatus(tc.expectedProgrammedStatus),
 				) {
 					t.Logf("Programmed condition not found, actual: %v", cs)