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

…#4887)

CHANGELOG.md

@@ -141,6 +141,10 @@ Adding a new version? You'll need three changes:
  [#4813](https:/Kong/kubernetes-ingress-controller/pull/4813)
 - Fixed an incorrect watch, set in UDPRoute controller watching UDProute status updates.
  [#4835](https:/Kong/kubernetes-ingress-controller/pull/4835)
+- Credentials Secrets that are not referenced by any KongConsumer but violate the KongConsumer
+ basic level validation (invalid credential type or missing required fields) are now rejected
+ by the admission webhook.
+ [#4887](https:/Kong/kubernetes-ingress-controller/pull/4887)
 
 ### Changed
 

hack/deploy-admission-controller.sh

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

internal/admission/handler.go

@@ -252,13 +252,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 logr.Logger
  SecretGetter kongstate.SecretGetter
+ ConsumerGetter ConsumerGetter
  ManagerClient client.Client
  AdminAPIServicesProvider AdminAPIServicesProvider
  ParserFeatures parser.FeatureFlags
@@ -75,6 +81,7 @@ func NewKongHTTPValidator(
  return KongHTTPValidator{
  Logger: logger,
  SecretGetter: &managerClientSecretGetter{managerClient: managerClient},
+ ConsumerGetter: &managerClientConsumerGetter{managerClient: managerClient},
  ManagerClient: managerClient,
  AdminAPIServicesProvider: servicesProvider,
  ParserFeatures: parserFeatures,
@@ -229,48 +236,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
@@ -462,17 +467,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) {
@@ -521,10 +524,6 @@ func (validator KongHTTPValidator) validatePluginAgainstGatewaySchema(ctx contex
  return "", nil
 }
 
-// -----------------------------------------------------------------------------
-// Private - Manager Client Secret Getter
-// -----------------------------------------------------------------------------
-
 type managerClientSecretGetter struct {
  managerClient client.Client
 }
@@ -536,3 +535,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

@@ -6,11 +6,14 @@ import (
  "net/http"
  "testing"
 
+ "github.com/go-logr/logr"
  "github.com/go-logr/zapr"
  "github.com/kong/go-kong/kong"
  "github.com/samber/lo"
+ "github.com/stretchr/testify/assert"
  "github.com/stretchr/testify/require"
  "go.uber.org/zap"
+ corev1 "k8s.io/api/core/v1"
  apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
  metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
@@ -555,3 +558,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: logr.Discard(),
+ }
+
+ 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
+}