fix and add unit tests on handleSecret

internal/admission/handler.go

@@ -245,28 +245,27 @@ func (h RequestHandler) handleSecret(
 	if err != nil {
 		return nil, err
 	}
-	// TODO so long as we still handle the deprecated field, this has to remain
-	// once the deprecated field is removed, we must replace this with a label filter in the webhook itself
-	// https://github.com/Kong/kubernetes-ingress-controller/issues/4853
-
-	if _, credentialTypeSource := util.ExtractKongCredentialType(&secret); credentialTypeSource == util.CredentialTypeAbsent {
-		// secret does not look like a credential resource in Kong
-		return responseBuilder.Allowed(true).Build(), nil
-	}
 
 	switch request.Operation {
 	case admissionv1.Update, admissionv1.Create:
-		ok, message := h.Validator.ValidateCredential(ctx, secret)
-		if !ok {
-			return responseBuilder.Allowed(ok).WithMessage(message).Build(), nil
+		// TODO so long as we still handle the deprecated field, this has to remain
+		// once the deprecated field is removed, we must replace this with a label filter in the webhook itself
+		// https://github.com/Kong/kubernetes-ingress-controller/issues/4853
+		if _, credentialTypeSource := util.ExtractKongCredentialType(&secret); credentialTypeSource != util.CredentialTypeAbsent {
+			ok, message := h.Validator.ValidateCredential(ctx, secret)
+			if !ok {
+				return responseBuilder.Allowed(ok).WithMessage(message).Build(), nil
+			}
 		}
 		referrers, err := h.ReferenceIndexers.ListReferrerObjectsByReferent(&secret)
 		if err != nil {
 			return responseBuilder.Allowed(false).WithMessage(err.Error()).Build(), err
 		}
 		for _, obj := range referrers {
 			gvk := obj.GetObjectKind().GroupVersionKind()
+			// REVIEW: Should we check version here? Seems that we do not need to support KongPlugin and KongClusterPlugin in other versions.
 			if gvk.Group == kongv1.GroupVersion.Group && gvk.Version == kongv1.GroupVersion.Version && gvk.Kind == "KongPlugin" {
+				// REVIEW: run type check here to avoid panic, although it should be unlikely to happen after checking the GVK?
 				plugin := obj.(*kongv1.KongPlugin)
 				ok, message, err := h.Validator.ValidatePlugin(ctx, *plugin, []*corev1.Secret{&secret})
 				if err != nil {

internal/admission/handler_test.go

@@ -4,21 +4,41 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"net/http"
 	"testing"
 
 	"github.com/go-logr/logr"
+	"github.com/go-logr/logr/testr"
 	"github.com/stretchr/testify/require"
 	admissionv1 "k8s.io/api/admission/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	k8stypes "k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/kong/kubernetes-ingress-controller/v3/internal/annotations"
+	ctrlref "github.com/kong/kubernetes-ingress-controller/v3/internal/controllers/reference"
+	"github.com/kong/kubernetes-ingress-controller/v3/internal/util"
 	kongv1 "github.com/kong/kubernetes-ingress-controller/v3/pkg/apis/configuration/v1"
 	kongv1beta1 "github.com/kong/kubernetes-ingress-controller/v3/pkg/apis/configuration/v1beta1"
 )
 
+var (
+	secretTypeMeta = metav1.TypeMeta{
+		APIVersion: "v1",
+		Kind:       "Secret",
+	}
+	kongPluginTypeMeta = metav1.TypeMeta{
+		APIVersion: kongv1.GroupVersion.String(),
+		Kind:       "KongPlugin",
+	}
+	kongClusterPluginTypeMeta = metav1.TypeMeta{
+		APIVersion: kongv1.GroupVersion.String(),
+		Kind:       "KongClusterPlugin",
+	}
+)
+
 func TestHandleKongIngress(t *testing.T) {
 	tests := []struct {
 		name         string
@@ -130,7 +150,6 @@ func TestHandleService(t *testing.T) {
 					Raw:    raw,
 				},
 			}
-
 			handler := RequestHandler{
 				Validator: validator,
 				Logger:    logr.Discard(),
@@ -145,3 +164,178 @@ func TestHandleService(t *testing.T) {
 		})
 	}
 }
+
+func TestHandleSecret(t *testing.T) {
+	testCases := []struct {
+		name             string
+		secret           *corev1.Secret
+		referrers        []client.Object
+		validatorOK      bool
+		validatorMessage string
+		validatorError   error
+		expectAllowed    bool
+		expectStatusCode int32
+		expectMessage    string
+		expectError      bool
+	}{
+		{
+			name: "secret used as a credential and passes the validation of credential",
+			secret: &corev1.Secret{
+				TypeMeta: secretTypeMeta,
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "default",
+					Name:      "credential-0",
+					Labels: map[string]string{
+						"konghq.com/credential": "true",
+					},
+				},
+				Data: map[string][]byte{},
+			},
+			validatorOK:   true,
+			expectAllowed: true,
+		},
+		{
+			name: "secret used as credential and fails the validation of credential",
+			secret: &corev1.Secret{
+				TypeMeta: secretTypeMeta,
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "default",
+					Name:      "credential-1",
+					Labels: map[string]string{
+						"konghq.com/credential": "true",
+					},
+				},
+				Data: map[string][]byte{},
+			},
+			validatorOK:      false,
+			validatorMessage: "invalid credential",
+			expectAllowed:    false,
+			expectStatusCode: http.StatusBadRequest,
+			expectMessage:    "invalid credential",
+		},
+		{
+			name: "secret used as KongPlugin config and KongClusterPlugin and passes validation of both CRDs",
+			secret: &corev1.Secret{
+				TypeMeta: secretTypeMeta,
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "default",
+					Name:      "plugin-conf",
+				},
+			},
+			referrers: []client.Object{
+				&kongv1.KongPlugin{
+					TypeMeta: kongPluginTypeMeta,
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: "default",
+						Name:      "plugin-0",
+					},
+					PluginName: "test-plugin",
+				},
+				&kongv1.KongClusterPlugin{
+					TypeMeta: kongClusterPluginTypeMeta,
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "cluster-plugin-0",
+					},
+					PluginName: "test-plugin",
+				},
+			},
+			validatorOK:   true,
+			expectAllowed: true,
+		},
+		{
+			name: "secret used as KongPlugin config and fails validation of KongPlugin",
+			secret: &corev1.Secret{
+				TypeMeta: secretTypeMeta,
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "default",
+					Name:      "plugin-conf",
+				},
+			},
+			referrers: []client.Object{
+				&kongv1.KongPlugin{
+					TypeMeta: kongPluginTypeMeta,
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: "default",
+						Name:      "plugin-0",
+					},
+					PluginName: "test-plugin",
+				},
+			},
+			validatorOK:      false,
+			validatorMessage: "invalid KongPlugin",
+			expectAllowed:    false,
+			expectStatusCode: http.StatusBadRequest,
+			expectMessage:    "Change on secret will generate invalid configuration for KongPlugin default/plugin-0: invalid KongPlugin",
+		},
+		{
+			name: "secret used as KongClusterPlugin config and fails validation of KongClusterPlugin",
+			secret: &corev1.Secret{
+				TypeMeta: secretTypeMeta,
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "default",
+					Name:      "plugin-conf",
+				},
+			},
+			referrers: []client.Object{
+				&kongv1.KongClusterPlugin{
+					TypeMeta: kongClusterPluginTypeMeta,
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "cluster-plugin-0",
+					},
+					PluginName: "test-plugin",
+				},
+			},
+			validatorOK:      false,
+			validatorMessage: "invalid KongClusterPlugin",
+			expectAllowed:    false,
+			expectStatusCode: http.StatusBadRequest,
+			expectMessage:    "Change on secret will generate invalid configuration for KongClusterPlugin cluster-plugin-0: invalid KongClusterPlugin",
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			validator := KongFakeValidator{
+				Result:  tc.validatorOK,
+				Message: tc.validatorMessage,
+				Error:   tc.validatorError,
+			}
+			raw, err := json.Marshal(tc.secret)
+			require.NoError(t, err)
+			request := admissionv1.AdmissionRequest{
+				Object: runtime.RawExtension{
+					Object: tc.secret,
+					Raw:    raw,
+				},
+				Operation: admissionv1.Update,
+			}
+
+			logger := testr.NewWithOptions(t, testr.Options{Verbosity: util.DebugLevel})
+			referenceIndexer := ctrlref.NewCacheIndexers(logger)
+
+			handler := RequestHandler{
+				Validator:         validator,
+				Logger:            logger,
+				ReferenceIndexers: referenceIndexer,
+			}
+			for _, obj := range tc.referrers {
+				err := handler.ReferenceIndexers.SetObjectReference(obj, tc.secret)
+				require.NoError(t, err)
+			}
+
+			responseBuilder := NewResponseBuilder(k8stypes.UID(""))
+			got, err := handler.handleSecret(context.Background(), request, responseBuilder)
+			if tc.expectError {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+			require.Equal(t, tc.expectAllowed, got.Allowed, "should return expected result of allowed")
+			require.Equal(t, tc.expectStatusCode, got.Result.Code)
+			if len(tc.expectMessage) > 0 {
+				require.Contains(t, got.Result.Message, tc.expectMessage)
+			}
+		})
+	}
+}

internal/controllers/reference/indexer.go

@@ -194,11 +194,11 @@ func (c CacheIndexers) ListReferencesByReferrer(referrer client.Object) ([]*Obje
 
 // ListReferencesByReferent lists all reference records referring to the same referent.
 func (c CacheIndexers) ListReferencesByReferent(referent client.Object) ([]*ObjectReference, error) {
-	referenctKey, err := objectKeyFunc(referent)
+	referentKey, err := objectKeyFunc(referent)
 	if err != nil {
 		return nil, err
 	}
-	refList, err := c.indexer.ByIndex(IndexNameReferent, referenctKey)
+	refList, err := c.indexer.ByIndex(IndexNameReferent, referentKey)
 	if err != nil {
 		return nil, err
 	}

internal/controllers/reference/indexer_test.go

@@ -262,3 +262,41 @@ func TestDeleteReferencesByReferrer(t *testing.T) {
 		})
 	}
 }
+
+func TestListReferencesByReferent(t *testing.T) {
+	testCases := []struct {
+		name          string
+		addReferrer   client.Object
+		addReferent   client.Object
+		checkReferent client.Object
+		objectNum     int
+	}{
+		{
+			name:          "has_referring_objects",
+			addReferrer:   testRefService1,
+			addReferent:   testRefSecret1,
+			checkReferent: testRefSecret1,
+			objectNum:     1,
+		},
+		{
+			name:          "has_no_referring_objects",
+			addReferrer:   testRefService1,
+			addReferent:   testRefSecret1,
+			checkReferent: testRefSecret2,
+			objectNum:     0,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			c := NewCacheIndexers(logr.Discard())
+			err := c.SetObjectReference(tc.addReferrer, tc.addReferent)
+			require.NoError(t, err, "should not return error on setting reference")
+
+			referrers, err := c.ListReferrerObjectsByReferent(tc.checkReferent)
+			require.NoError(t, err)
+			require.Len(t, referrers, tc.objectNum)
+		})
+	}
+}