fix: run basic credentials validation despite relation with consumers (…

…#4887) (#4904)

CHANGELOG.md

@@ -124,12 +124,12 @@ Adding a new version? You'll need three changes:
   [#4608](https://github.com/Kong/kubernetes-ingress-controller/pull/4608)
 - Do not parse error body when failed to get response from reloading declarative
   configurations to produce proper error log in such situations,
-  [#4666](https://github.com/Kong/kubernetes-ingress-controller/pull/4666) 
+  [#4666](https://github.com/Kong/kubernetes-ingress-controller/pull/4666)
 - Set type meta of objects when adding them to caches and reference indexers
   to ensure that indexes of objects in reference indexers have correct object
-  kind. This ensures referece relations of objects are stored and indexed 
+  kind. This ensures referece relations of objects are stored and indexed
   correctly.
-  [#4663](https://github.com/Kong/kubernetes-ingress-controller/pull/4663) 
+  [#4663](https://github.com/Kong/kubernetes-ingress-controller/pull/4663)
 - Display Service ports on generated Kong services, instead of a static default
   value. This change is cosmetic only.
   [#4503](https://github.com/Kong/kubernetes-ingress-controller/pull/4503)
@@ -150,7 +150,7 @@ Adding a new version? You'll need three changes:
   [#4641](https://github.com/Kong/kubernetes-ingress-controller/issues/4641)
   [#4643](https://github.com/Kong/kubernetes-ingress-controller/issues/4643)
 - Fix `Licenses` and `ConsumerGroups` missing in sanitized copies of Kong configuration.
-  [#4710](https://github.com/Kong/kubernetes-ingress-controller/pull/4710
+  [#4710](https://github.com/Kong/kubernetes-ingress-controller/pull/4710)
 
 ## [2.11.1]
 
@@ -203,7 +203,7 @@ Adding a new version? You'll need three changes:
   [#4211](https://github.com/Kong/kubernetes-ingress-controller/pull/4211)
 - Assign priorities to routes translated from Ingresses when parser translate
   them to expression based Kong routes. The assigning method is basically the
-  same as in Kong gateway's `traditional_compatible` router, except that 
+  same as in Kong gateway's `traditional_compatible` router, except that
   `regex_priority` field in Kong traditional route is not supported. This
   method is adopted to keep the compatibility with traditional router on
   maximum effort.
@@ -213,7 +213,7 @@ Adding a new version? You'll need three changes:
   [specification on priorities of matches in `HTTPRoute`][httproute-specification].
   [#4296](https://github.com/Kong/kubernetes-ingress-controller/pull/4296)
   [#4434](https://github.com/Kong/kubernetes-ingress-controller/pull/4434)
-- Assign priorities to routes translated from GRPCRoutes when the parser translates 
+- Assign priorities to routes translated from GRPCRoutes when the parser translates
   them to expression based Kong routes. The priority order follows the
   [specification on match priorities in GRPCRoute][grpcroute-specification].
   [#4364](https://github.com/Kong/kubernetes-ingress-controller/pull/4364)
@@ -230,7 +230,7 @@ Adding a new version? You'll need three changes:
   in terms of accepting data-plane traffic, but are ready to accept configuration
   updates. The controller will now send configuration to such Gateways and will
   actively monitor their readiness for accepting configuration updates.
-  [#4368](https://github.com/Kong/kubernetes-ingress-controller/pull/4368
+  [#4368](https://github.com/Kong/kubernetes-ingress-controller/pull/4368)
 - `KongConsumer`, `KongConsumerGroup` `KongPlugin`, and `KongClusterPlugin` CRDs were extended with
   `Status.Conditions` field. It will contain the `Programmed` condition describing
   whether an object was successfully translated into Kong entities and sent to Kong.
@@ -265,11 +265,11 @@ Adding a new version? You'll need three changes:
 - Changed the Gateway's readiness probe in all-in-one manifests from `/status`
   to `/status/ready`. Gateways will be considered ready only after an initial
   configuration is applied by the controller.
-  [#4368](https://github.com/Kong/kubernetes-ingress-controller/pull/4368
-- When translating to expression based Kong routes, annotations to specify 
+  [#4368](https://github.com/Kong/kubernetes-ingress-controller/pull/4368)
+- When translating to expression based Kong routes, annotations to specify
   protocols are translated to `protocols` field of the result Kong route,
-  instead of putting the conditions to match protocols inside expressions. 
-  [#4422](https://github.com/Kong/kubernetes-ingress-controller/pull/4422) 
+  instead of putting the conditions to match protocols inside expressions.
+  [#4422](https://github.com/Kong/kubernetes-ingress-controller/pull/4422)
 
 ### Fixed
 
@@ -285,7 +285,7 @@ Adding a new version? You'll need three changes:
 - `Gateway` can now correctly update `AttachedRoutes` even if there are more
   than 100 `HttpRoute`s.
   [#4458](https://github.com/Kong/kubernetes-ingress-controller/pull/4458)
- 
+
 [gojson]: https://github.com/goccy/go-json
 [httproute-specification]: https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io/v1beta1.HTTPRoute
 [grpcroute-specification]:  https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io/v1alpha2.GRPCRouteRule

hack/deploy-admission-controller.sh

@@ -58,6 +58,7 @@ webhooks:
     apiVersions:
     - 'v1'
     operations:
+    - CREATE
     - UPDATE
     resources:
     - secrets

internal/admission/handler.go

@@ -246,13 +246,8 @@ func (h RequestHandler) handleSecret(
 		return responseBuilder.Allowed(true).Build(), nil
 	}
 
-	// secrets are only validated on update because they must be referenced by a
-	// managed consumer in order for us to validate them, and because credentials
-	// validation also happens at the consumer side of the reference so a
-	// credentials secret can not be referenced without being validated.
-
 	switch request.Operation { //nolint:exhaustive
-	case admissionv1.Update:
+	case admissionv1.Update, admissionv1.Create:
 		ok, message, err := h.Validator.ValidateCredential(ctx, secret)
 		if err != nil {
 			return nil, err

internal/admission/validator.go

@@ -48,11 +48,17 @@ type AdminAPIServicesProvider interface {
 	GetRoutesService() (kong.AbstractRouteService, bool)
 }
 
+// ConsumerGetter is an interface for retrieving KongConsumers.
+type ConsumerGetter interface {
+	ListAllConsumers(ctx context.Context) ([]kongv1.KongConsumer, error)
+}
+
 // KongHTTPValidator implements KongValidator interface to validate Kong
 // entities using the Admin API of Kong.
 type KongHTTPValidator struct {
 	Logger                   logrus.FieldLogger
 	SecretGetter             kongstate.SecretGetter
+	ConsumerGetter           ConsumerGetter
 	ManagerClient            client.Client
 	AdminAPIServicesProvider AdminAPIServicesProvider
 	ParserFeatures           parser.FeatureFlags
@@ -77,6 +83,7 @@ func NewKongHTTPValidator(
 	return KongHTTPValidator{
 		Logger:                   logger,
 		SecretGetter:             &managerClientSecretGetter{managerClient: managerClient},
+		ConsumerGetter:           &managerClientConsumerGetter{managerClient: managerClient},
 		ManagerClient:            managerClient,
 		AdminAPIServicesProvider: servicesProvider,
 		ParserFeatures:           parserFeatures,
@@ -234,48 +241,46 @@ func (validator KongHTTPValidator) ValidateCredential(
 	ctx context.Context,
 	secret corev1.Secret,
 ) (bool, string, error) {
-	// if the secret doesn't contain a type key it's not a credentials secret
+	// If the secret doesn't contain a type key it's not a credentials secret.
 	_, ok := secret.Data[credsvalidation.TypeKey]
 	if !ok {
 		return true, "", nil
 	}
 
-	// credentials are only validated if they are referenced by a managed consumer
-	// in the namespace, as such we pull a list of all consumers from the cached
-	// client to determine if the credentials are referenced.
+	// If we know it's a credentials secret, we can ensure its base-level validity.
+	if err := credsvalidation.ValidateCredentials(&secret); err != nil {
+		return false, fmt.Sprintf("%s: %s", ErrTextConsumerCredentialValidationFailed, err), nil
+	}
+
+	// Credentials are validated further for unique key constraints only if they are referenced by a managed consumer
+	// in the namespace, as such we pull a list of all consumers from the cached client to determine
+	// if the credentials are referenced.
 	managedConsumers, err := validator.listManagedConsumers(ctx)
 	if err != nil {
 		return false, ErrTextConsumerUnretrievable, err
 	}
 
-	// verify whether this secret is referenced by any managed consumer
+	// Verify whether this secret is referenced by any managed consumer.
 	managedConsumersWithReferences := listManagedConsumersReferencingCredentialsSecret(secret, managedConsumers)
 	if len(managedConsumersWithReferences) == 0 {
-		// if no managed consumers reference this secret, its considered
-		// unmanaged and we don't validate it unless it becomes referenced
-		// by a managed consumer at a later time.
+		// If no managed consumers reference this secret, its considered unmanaged, and we don't validate it
+		// unless it becomes referenced by a managed consumer at a later time.
 		return true, "", nil
 	}
 
-	// now that we know at least one managed consumer is referencing this
-	// secret we perform the base-level credentials secret validation.
-	if err := credsvalidation.ValidateCredentials(&secret); err != nil {
-		return false, ErrTextConsumerCredentialValidationFailed, err
-	}
-
-	// if base-level validation passes we move on to create an index of
-	// all managed credentials so that we can verify that the updates to
-	// this secret are not in violation of any unique key constraints.
+	// If base-level validation passed and the credential is referenced by a consumer,
+	// we move on to create an index of all managed credentials so that we can verify that
+	// the updates to this secret are not in violation of any unique key constraints.
 	ignoreSecrets := map[string]map[string]struct{}{secret.Namespace: {secret.Name: {}}}
 	credentialsIndex, err := globalValidationIndexForCredentials(ctx, validator.ManagerClient, managedConsumers, ignoreSecrets)
 	if err != nil {
 		return false, ErrTextConsumerCredentialValidationFailed, err
 	}
 
-	// the index is built, now validate that the newly updated secret
+	// The index is built, now validate that the newly updated secret
 	// is not in violation of any constraints.
 	if err := credentialsIndex.ValidateCredentialsForUniqueKeyConstraints(&secret); err != nil {
-		return false, ErrTextConsumerCredentialValidationFailed, err
+		return false, fmt.Sprintf("%s: %s", ErrTextConsumerCredentialValidationFailed, err), nil
 	}
 
 	return true, "", nil
@@ -467,17 +472,15 @@ func (noOpRoutesValidator) Validate(_ context.Context, _ *kong.Route) (bool, str
 // -----------------------------------------------------------------------------
 
 func (validator KongHTTPValidator) listManagedConsumers(ctx context.Context) ([]*kongv1.KongConsumer, error) {
-	// gather a list of all consumers from the cached client
-	consumers := &kongv1.KongConsumerList{}
-	if err := validator.ManagerClient.List(ctx, consumers, &client.ListOptions{
-		Namespace: corev1.NamespaceAll,
-	}); err != nil {
-		return nil, err
+	// Gather a list of all consumers from the cached client.
+	consumers, err := validator.ConsumerGetter.ListAllConsumers(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("failed to list consumers: %w", err)
 	}
 
-	// reduce the consumer set to consumers managed by this controller
+	// Reduce the consumer set to consumers managed by this controller.
 	managedConsumers := make([]*kongv1.KongConsumer, 0)
-	for _, consumer := range consumers.Items {
+	for _, consumer := range consumers {
 		consumer := consumer
 		if !validator.ingressClassMatcher(&consumer.ObjectMeta, annotations.IngressClassKey,
 			annotations.ExactClassMatch) {
@@ -526,10 +529,6 @@ func (validator KongHTTPValidator) validatePluginAgainstGatewaySchema(ctx contex
 	return "", nil
 }
 
-// -----------------------------------------------------------------------------
-// Private - Manager Client Secret Getter
-// -----------------------------------------------------------------------------
-
 type managerClientSecretGetter struct {
 	managerClient client.Client
 }
@@ -541,3 +540,17 @@ func (m *managerClientSecretGetter) GetSecret(namespace, name string) (*corev1.S
 		Name:      name,
 	}, secret)
 }
+
+type managerClientConsumerGetter struct {
+	managerClient client.Client
+}
+
+func (m *managerClientConsumerGetter) ListAllConsumers(ctx context.Context) ([]kongv1.KongConsumer, error) {
+	consumers := &kongv1.KongConsumerList{}
+	if err := m.managerClient.List(ctx, consumers, &client.ListOptions{
+		Namespace: corev1.NamespaceAll,
+	}); err != nil {
+		return nil, err
+	}
+	return consumers.Items, nil
+}

internal/admission/validator_test.go

@@ -9,7 +9,9 @@ import (
 	"github.com/kong/go-kong/kong"
 	"github.com/samber/lo"
 	"github.com/sirupsen/logrus"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
@@ -565,3 +567,60 @@ func TestKongHTTPValidator_ValidateConsumerGroup(t *testing.T) {
 }
 
 func fakeClassMatcher(*metav1.ObjectMeta, string, annotations.ClassMatching) bool { return true }
+
+func TestKongHTTPValidator_ValidateCredential(t *testing.T) {
+	testCases := []struct {
+		name            string
+		secret          corev1.Secret
+		wantOK          bool
+		wantMessage     string
+		wantErrContains string
+	}{
+		{
+			name: "valid key-auth credential with no consumers gets accepted",
+			secret: corev1.Secret{
+				Data: map[string][]byte{
+					"kongCredType": []byte("key-auth"),
+					"key":          []byte("my-key"),
+				},
+			},
+			wantOK: true,
+		},
+		{
+			name: "invalid key-auth credential with no consumers gets rejected",
+			secret: corev1.Secret{
+				Data: map[string][]byte{
+					"kongCredType": []byte("key-auth"),
+					// missing key
+				},
+			},
+			wantOK:      false,
+			wantMessage: fmt.Sprintf("%s: %s", ErrTextConsumerCredentialValidationFailed, "invalid credentials secret, no data present"),
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			validator := KongHTTPValidator{
+				ConsumerGetter:           fakeConsumerGetter{},
+				AdminAPIServicesProvider: fakeServicesProvider{},
+				ingressClassMatcher:      fakeClassMatcher,
+				Logger:                   logrus.New(),
+			}
+
+			ok, msg, err := validator.ValidateCredential(context.Background(), tc.secret)
+			require.NoError(t, err)
+			assert.Equal(t, tc.wantOK, ok)
+			assert.Equal(t, tc.wantMessage, msg)
+		})
+	}
+}
+
+type fakeConsumerGetter struct {
+	consumers []kongv1.KongConsumer
+}
+
+func (f fakeConsumerGetter) ListAllConsumers(context.Context) ([]kongv1.KongConsumer, error) {
+	return f.consumers, nil
+}