feat(creds) remove kongCredType field support

CHANGELOG.md

@@ -87,6 +87,22 @@ Adding a new version? You'll need three changes:
 
 ## Unreleased
 
+### Breaking changes
+
+- Removed support for the deprecated `kongCredType` Secret field. If you have
+  not previously [updated to the credential label](https://docs.konghq.com/kubernetes-ingress-controller/latest/guides/migrate/credential-kongcredtype-label/)
+  you must do so before upgrading to this version. This removal includes an
+  update to the webhook configuration that checks only Secrets with
+  `konghq.com/credential` or `konghq.com/validate` labels (for Secrets that
+  contain plugin configuration). This filter improves performance and
+  reliability by not checking Secrets the controller will never use. Users that
+  wish to defer adding `konghq.com/validate` to Secrets with plugin
+  configuration can set the `ingressController.admissionWebhook.filterSecrets`
+  chart values.yaml key to `false`. Doing so does not benefit from the
+  performance benefits, however, so labeling plugin configuration Secrets and
+  enabling the filter is recommended as soon as is convenient.
+  [#5856](https://github.com/Kong/kubernetes-ingress-controller/pull/5856)
+
 ### Added
 
 - Add a `/debug/config/raw-error` endpoint to the config dump diagnostic

config/default/kustomization.yaml

@@ -18,7 +18,9 @@ bases:
 - ../manager
 # [WEBHOOK] To enable webhook, uncomment all the sections with [WEBHOOK] prefix including the one in
 # crd/kustomization.yaml
-#- ../webhook
+# NOTE we enable this to allow patching the generated webhook. We do not have the crd/kustomization.yaml config
+# enabled, as we don't use the mutating hooks in there currently.
+- ../webhook
 # [CERTMANAGER] To enable cert-manager, uncomment all sections with 'CERTMANAGER'. 'WEBHOOK' components are required.
 #- ../certmanager
 # [PROMETHEUS] To enable prometheus monitor, uncomment all sections with 'PROMETHEUS'.

config/webhook/additional_secret_hooks.yaml

@@ -0,0 +1,58 @@
+# https://github.com/kubernetes-sigs/controller-tools/issues/553
+# controller-tools, and by extension kubebuilder, do not support specifying objectSelector,
+# which we need for the Secret rules.
+apiVersion: admissionregistration.k8s.io/v1
+kind: ValidatingWebhookConfiguration
+metadata:
+  name: validating-webhook-configuration
+webhooks:
+- admissionReviewVersions:
+  - v1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: system
+      path: /
+  failurePolicy: Fail
+  matchPolicy: Equivalent
+  name: secrets.credentials.validation.ingress-controller.konghq.com
+  objectSelector:
+    matchExpressions:
+    - key: "konghq.com/credential"
+      operator: "Exists"
+  rules:
+  - apiGroups:
+    - ""
+    apiVersions:
+    - v1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - secrets
+  sideEffects: None
+- admissionReviewVersions:
+  - v1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: system
+      path: /
+  failurePolicy: Fail
+  matchPolicy: Equivalent
+  name: secrets.plugins.validation.ingress-controller.konghq.com
+  objectSelector:
+    matchExpressions:
+    - key: "konghq.com/validate"
+      operator: "Exists"
+  rules:
+  - apiGroups:
+    - ""
+    apiVersions:
+    - v1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - secrets
+  sideEffects: None

config/webhook/kustomization.yaml

@@ -0,0 +1,7 @@
+apiVersion: kustomize.config.k8s.io/v1beta1
+kind: Kustomization
+resources:
+- manifests.yaml
+
+patchesStrategicMerge:
+- additional_secret_hooks.yaml

config/webhook/manifests.yaml

@@ -195,27 +195,6 @@ webhooks:
     resources:
     - kongvaults
   sideEffects: None
-- admissionReviewVersions:
-  - v1
-  clientConfig:
-    service:
-      name: webhook-service
-      namespace: system
-      path: /
-  failurePolicy: Fail
-  matchPolicy: Equivalent
-  name: secrets.validation.ingress-controller.konghq.com
-  rules:
-  - apiGroups:
-    - ""
-    apiVersions:
-    - v1
-    operations:
-    - CREATE
-    - UPDATE
-    resources:
-    - secrets
-  sideEffects: None
 - admissionReviewVersions:
   - v1
   clientConfig:

internal/admission/handler.go

@@ -15,6 +15,7 @@ import (
 	"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/gatewayapi"
+	"github.com/kong/kubernetes-ingress-controller/v3/internal/labels"
 	"github.com/kong/kubernetes-ingress-controller/v3/internal/util"
 	kongv1 "github.com/kong/kubernetes-ingress-controller/v3/pkg/apis/configuration/v1"
 	kongv1alpha1 "github.com/kong/kubernetes-ingress-controller/v3/pkg/apis/configuration/v1alpha1"
@@ -256,7 +257,10 @@ func (h RequestHandler) handleKongClusterPlugin(
 	return responseBuilder.Allowed(ok).WithMessage(message).Build(), nil
 }
 
-// +kubebuilder:webhook:verbs=create;update,groups=core,resources=secrets,versions=v1,name=secrets.validation.ingress-controller.konghq.com,path=/,webhookVersions=v1,matchPolicy=equivalent,mutating=false,failurePolicy=fail,sideEffects=None,admissionReviewVersions=v1
+// NOTE this handler _does not_ use a kubebuilder directive, as our Secret handling requires webhook features
+// kubebuilder does not support (objectSelector). Instead, config/webhook/additional_secret_hooks.yaml includes
+// handwritten webhook rules that we patch into the webhook manifest.
+// See https://github.com/kubernetes-sigs/controller-tools/issues/553 for further info.
 
 func (h RequestHandler) handleSecret(
 	ctx context.Context,
@@ -271,24 +275,36 @@ func (h RequestHandler) handleSecret(
 
 	switch request.Operation {
 	case admissionv1.Update, admissionv1.Create:
-		// 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 {
+		// credential secrets
+		if credType, err := util.ExtractKongCredentialType(&secret); err == nil && credType != "" {
 			ok, message := h.Validator.ValidateCredential(ctx, secret)
 			if !ok {
 				return responseBuilder.Allowed(ok).WithMessage(message).Build(), nil
 			}
 		}
 
-		ok, message, err := h.checkReferrersOfSecret(ctx, &secret)
+		// TODO https://github.com/Kong/kubernetes-ingress-controller/issues/5876
+		// This catch-all block handles Secrets referenced by KongPlugin and KongClusterPlugin configuration. As of 3.2,
+		// these Secrets should use a "konghq.com/validate: plugin" label, but the original unfiltered behavior is still
+		// supported. It is slated for removal in 4.0. Once it is removed (or if we add additional Secret validation cases
+		// other than "plugin") this needs to change to a case that only applies if the valdiate label is present with the
+		// "plugin" value, probably using a 'switch validate := secret.Labels[labels.ValidateLabel]; labels.ValidateType(validate)'
+		// statement.
+		ok, count, message, err := h.checkReferrersOfSecret(ctx, &secret)
+		if count > 0 {
+			if secret.Labels[labels.ValidateLabel] != string(labels.PluginValidate) {
+				h.Logger.Info("Warning: Secret used in Kong(Cluster)Plugin, but missing 'konghq.com/validate: plugin' label."+
+					"This label will be required in a future release", "namespace", secret.Namespace, "name", secret.Name)
+			}
+		}
 		if err != nil {
 			return responseBuilder.Allowed(false).WithMessage(fmt.Sprintf("failed to validate other objects referencing the secret: %v", err)).Build(), err
 		}
 		if !ok {
 			return responseBuilder.Allowed(false).WithMessage(message).Build(), nil
 		}
 
+		// no reference found in the blanket block, this is some random unrelated Secret and KIC should ignore it.
 		return responseBuilder.Allowed(true).Build(), nil
 
 	default:
@@ -298,45 +314,49 @@ func (h RequestHandler) handleSecret(
 
 // checkReferrersOfSecret validates all referrers (KongPlugins and KongClusterPlugins) of the secret
 // and rejects the secret if it generates invalid configurations for any of the referrers.
-func (h RequestHandler) checkReferrersOfSecret(ctx context.Context, secret *corev1.Secret) (bool, string, error) {
+func (h RequestHandler) checkReferrersOfSecret(ctx context.Context, secret *corev1.Secret) (bool, int, string, error) {
 	referrers, err := h.ReferenceIndexers.ListReferrerObjectsByReferent(secret)
 	if err != nil {
-		return false, "", fmt.Errorf("failed to list referrers of secret: %w", err)
+		return false, 0, "", fmt.Errorf("failed to list referrers of secret: %w", err)
 	}
 
+	count := 0
 	for _, obj := range referrers {
 		gvk := obj.GetObjectKind().GroupVersionKind()
 		if gvk.Group == kongv1.GroupVersion.Group && gvk.Version == kongv1.GroupVersion.Version && gvk.Kind == KindKongPlugin {
+			count++
 			plugin := obj.(*kongv1.KongPlugin)
 			ok, message, err := h.Validator.ValidatePlugin(ctx, *plugin, []*corev1.Secret{secret})
 			if err != nil {
-				return false, "", fmt.Errorf("failed to run validation on KongPlugin %s/%s: %w",
+				return false, count, "", fmt.Errorf("failed to run validation on KongPlugin %s/%s: %w",
 					plugin.Namespace, plugin.Name, err,
 				)
 			}
 			if !ok {
-				return false,
+				return false, count,
 					fmt.Sprintf("Change on secret will generate invalid configuration for KongPlugin %s/%s: %s",
 						plugin.Namespace, plugin.Name, message,
 					), nil
 			}
 		}
 		if gvk.Group == kongv1.GroupVersion.Group && gvk.Version == kongv1.GroupVersion.Version && gvk.Kind == KindKongClusterPlugin {
+			count++
 			plugin := obj.(*kongv1.KongClusterPlugin)
 			ok, message, err := h.Validator.ValidateClusterPlugin(ctx, *plugin, []*corev1.Secret{secret})
 			if err != nil {
-				return false, "", fmt.Errorf("failed to run validation on KongClusterPlugin %s: %w",
+				return false, count, "", fmt.Errorf("failed to run validation on KongClusterPlugin %s: %w",
 					plugin.Name, err,
 				)
 			}
 			if !ok {
-				return false, fmt.Sprintf("Change on secret will generate invalid configuration for KongClusterPlugin %s: %s",
-					plugin.Name, message,
-				), nil
+				return false, count,
+					fmt.Sprintf("Change on secret will generate invalid configuration for KongClusterPlugin %s: %s",
+						plugin.Name, message,
+					), nil
 			}
 		}
 	}
-	return true, "", nil
+	return true, count, "", nil
 }
 
 // +kubebuilder:webhook:verbs=create;update,groups=gateway.networking.k8s.io,resources=gateways,versions=v1;v1beta1,name=gateways.validation.ingress-controller.konghq.com,path=/,webhookVersions=v1,matchPolicy=equivalent,mutating=false,failurePolicy=fail,sideEffects=None,admissionReviewVersions=v1

internal/admission/handler_test.go

@@ -19,6 +19,7 @@ import (
 
 	"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/labels"
 	"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"
@@ -220,6 +221,9 @@ func TestHandleSecret(t *testing.T) {
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: "default",
 					Name:      "plugin-conf",
+					Labels: map[string]string{
+						labels.ValidateLabel: "plugin",
+					},
 				},
 			},
 			referrers: []client.Object{
@@ -235,6 +239,9 @@ func TestHandleSecret(t *testing.T) {
 					TypeMeta: kongClusterPluginTypeMeta,
 					ObjectMeta: metav1.ObjectMeta{
 						Name: "cluster-plugin-0",
+						Labels: map[string]string{
+							labels.ValidateLabel: "plugin",
+						},
 					},
 					PluginName: "test-plugin",
 				},
@@ -249,6 +256,9 @@ func TestHandleSecret(t *testing.T) {
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: "default",
 					Name:      "plugin-conf",
+					Labels: map[string]string{
+						labels.ValidateLabel: "plugin",
+					},
 				},
 			},
 			referrers: []client.Object{
@@ -274,6 +284,9 @@ func TestHandleSecret(t *testing.T) {
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: "default",
 					Name:      "plugin-conf",
+					Labels: map[string]string{
+						labels.ValidateLabel: "plugin",
+					},
 				},
 			},
 			referrers: []client.Object{

internal/admission/validation/consumers/credentials/validation.go

@@ -19,9 +19,8 @@ import (
 // ValidateCredentials performs basic validation on a credential secret given
 // the Kubernetes secret which contains credentials data.
 func ValidateCredentials(secret *corev1.Secret) error {
-	credentialType, credentialSource := util.ExtractKongCredentialType(secret)
-
-	if credentialSource == util.CredentialTypeAbsent {
+	credentialType, err := util.ExtractKongCredentialType(secret)
+	if err != nil {
 		// this shouldn't occur, since we check this earlier in the admission controller's handleSecret function, but
 		// checking here also in case a refactor removes that
 		return fmt.Errorf("secret has no credential type, add a %s label", labels.CredentialTypeLabel)
@@ -123,12 +122,9 @@ type Index map[string]map[string]map[string]struct{}
 // and will validate it for both normal structure validation and for
 // unique key constraint violations.
 func (cs Index) ValidateCredentialsForUniqueKeyConstraints(secret *corev1.Secret) error {
-	credentialType, credentialSource := util.ExtractKongCredentialType(secret)
-	if credentialSource == util.CredentialTypeAbsent {
-		return fmt.Errorf(
-			"secret has no credential type, add a %s label",
-			labels.CredentialTypeLabel,
-		)
+	credentialType, err := util.ExtractKongCredentialType(secret)
+	if err != nil {
+		return fmt.Errorf("secret has no credential type, add a %s label", labels.CredentialTypeLabel)
 	}
 
 	// the additional key/values are optional, but must be validated

internal/admission/validation/consumers/credentials/validation_test.go

@@ -176,21 +176,6 @@ func TestValidateCredentials(t *testing.T) {
 			},
 			wantErr: fmt.Errorf("some fields were invalid due to missing data: rsa_public_key, key, secret"),
 		},
-		{
-			// TODO https://github.com/Kong/kubernetes-ingress-controller/issues/4853 to be removed after deprecation window
-			name: "valid credential with deprectated field",
-			secret: &corev1.Secret{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "secret",
-					Namespace: "default",
-				},
-				Data: map[string][]byte{
-					"key":          []byte("little-rabbits-be-good"),
-					"kongCredType": []byte("key-auth"),
-				},
-			},
-			wantErr: nil,
-		},
 		{
 			name: "invalid credential type",
 			secret: &corev1.Secret{

internal/admission/validation/consumers/credentials/vars.go

@@ -6,11 +6,7 @@ import "k8s.io/apimachinery/pkg/util/sets"
 // Validation - Vars
 // -----------------------------------------------------------------------------
 
-// TypeKey indicates the key in a consumer secret which identifies the type
-// of credential that is being provided for the consumer.
-const TypeKey = "kongCredType"
-
-// SupportedTypes indicates all the "kongCredType"s which are supported for KongConsumer credentials.
+// SupportedTypes indicates all the Kong credential types which are supported for KongConsumer credentials.
 var SupportedTypes = sets.NewString(
 	"basic-auth",
 	"hmac-auth",

internal/admission/validator.go

@@ -272,8 +272,11 @@ func (validator KongHTTPValidator) ValidateConsumerGroup(
 // else it returns false with the error message. If an error happens during
 // validation, error is returned.
 func (validator KongHTTPValidator) ValidateCredential(ctx context.Context, secret corev1.Secret) (bool, string) {
-	// If the secret doesn't specify a credential type (either by label or the secret's key) it's not a credentials secret.
-	if _, s := util.ExtractKongCredentialType(&secret); s == util.CredentialTypeAbsent {
+	// If the secret doesn't specify a credential type it's not a credentials secret. We shouldn't actually reach this
+	// codepath in practice because such secrets will be filtered out by the webhook secrets objectSelector and ignored.
+	// However, installs could potentially use an outdated webhook definition. Prior to 3.2 we only filtered in code and
+	// used a blanket selector.
+	if _, err := util.ExtractKongCredentialType(&secret); err != nil {
 		return true, ""
 	}