feat(creds) remove kongCredType field support (#5856)

Remove support for the kongCredType field in credential Secrets. Only
honor the konghq.com/credential label.

Add a konghq.com/plugin-config label. This label indicates that a Secret
contains plugin configuration and should be validated against its
referrers when updated.

Add objectSelectors to the Secret blocks in the admission webhook
definition. Admission will skip any Secrets without a label indicating
they should be used by KIC.

Add a "konghq.com/validate" label for other Secrets (currently plugin
configuration) that require admission checks.

Split the webhook manifest into the generated manifest and additional
rule patches. Kubebuilder limitations require writing your own rules to
use objectSelectors.

Remove the kubebuilder Secret hook generation directive and document the
workaround.

Refactor the envtest runner to build a webhook manifest via Kustomize,
rather than reading a static manifest.

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, ""
 	}