fix(admission): use Ignore failure policy in tests (same as charts) (…

…#5063) (#5078)

(cherry picked from commit a364f35)

Co-authored-by: Grzegorz Burzyński <[email protected]>

CHANGELOG.md

@@ -142,6 +142,13 @@ Nothing yet.
   They must be migrated to annotations. `upstream` field is deprecated - it's recommended
   to migrate its settings to the new `KongUpstreamPolicy` resource.
   [#5022](https://github.com/Kong/kubernetes-ingress-controller/pull/5022)
+- Fixed `HTTPRoute` and `KongConsumer` admission webhook validators to properly
+  signal validation failures, resulting in returning responses with `AdmissionResponse`
+  filled instead of 500 status codes. It will make them work as expected in cases where
+  the `ValidatingWebhookConfiguration` has `failurePolicy: Ignore`.
+  This will enable validations of `HTTPRoute` and `KongConsumer` that were previously only
+  accidentally effective with `failurePolicy: Fail`, thus it can be considered a breaking change.
+  [#5063](https://github.com/Kong/kubernetes-ingress-controller/pull/5063)
 
 ### Fixed
 

internal/admission/handler.go

@@ -251,10 +251,7 @@ func (h RequestHandler) handleSecret(
 
 	switch request.Operation {
 	case admissionv1.Update, admissionv1.Create:
-		ok, message, err := h.Validator.ValidateCredential(ctx, secret)
-		if err != nil {
-			return nil, err
-		}
+		ok, message := h.Validator.ValidateCredential(ctx, secret)
 		return responseBuilder.Allowed(ok).WithMessage(message).Build(), nil
 	default:
 		return nil, fmt.Errorf("unknown operation %q", string(request.Operation))
@@ -293,7 +290,6 @@ func (h RequestHandler) handleHTTPRoute(
 	if err != nil {
 		return nil, err
 	}
-
 	return responseBuilder.Allowed(ok).WithMessage(message).Build(), nil
 }
 

internal/admission/server_test.go

@@ -59,8 +59,8 @@ func (v KongFakeValidator) ValidateClusterPlugin(
 	return v.Result, v.Message, v.Error
 }
 
-func (v KongFakeValidator) ValidateCredential(_ context.Context, _ corev1.Secret) (bool, string, error) {
-	return v.Result, v.Message, v.Error
+func (v KongFakeValidator) ValidateCredential(context.Context, corev1.Secret) (bool, string) {
+	return v.Result, v.Message
 }
 
 func (v KongFakeValidator) ValidateGateway(_ context.Context, _ gatewayapi.Gateway) (bool, string, error) {

internal/admission/validation/gateway/httproute.go

@@ -34,7 +34,7 @@ func ValidateHTTPRoute(
 ) (bool, string, error) {
 	// validate that no unsupported features are in use
 	if err := validateHTTPRouteFeatures(httproute, parserFeatures); err != nil {
-		return false, "HTTPRoute spec did not pass validation", err
+		return false, fmt.Sprintf("HTTPRoute spec did not pass validation: %s", err), nil
 	}
 
 	// perform Gateway validations for the HTTPRoute (e.g. listener validation, namespace validation, e.t.c.)
@@ -45,24 +45,25 @@ func ValidateHTTPRoute(
 		// determine the parentRef for this gateway
 		parentRef, err := getParentRefForHTTPRouteGateway(httproute, gateway)
 		if err != nil {
-			return false, "Couldn't determine parentRefs for httproute", err
+			return false, fmt.Sprintf("Couldn't determine parentRefs for httproute: %s", err), nil
 		}
 
 		// gather the relevant gateway listeners for the httproute
 		listeners, err := getListenersForHTTPRouteValidation(parentRef.SectionName, gateway)
 		if err != nil {
-			return false, "Couldn't find gateway listeners for httproute", err
+			return false, fmt.Sprintf("Couldn't find gateway listeners for httproute: %s", err), nil
 		}
 
 		// perform validation of this route against it's linked gateway listeners
 		for _, listener := range listeners {
 			if err := validateHTTPRouteListener(listener); err != nil {
-				return false, "HTTPRoute linked Gateway listeners did not pass validation", err
+				return false, fmt.Sprintf("HTTPRoute linked Gateway listeners did not pass validation: %s", err), nil
 			}
 		}
 	}
 
-	return validateWithKongGateway(ctx, routesValidator, parserFeatures, httproute)
+	ok, msg := validateWithKongGateway(ctx, routesValidator, parserFeatures, httproute)
+	return ok, msg, nil
 }
 
 // -----------------------------------------------------------------------------
@@ -188,7 +189,7 @@ func getListenersForHTTPRouteValidation(sectionName *gatewayapi.SectionName, gat
 
 func validateWithKongGateway(
 	ctx context.Context, routesValidator routeValidator, parserFeatures parser.FeatureFlags, httproute *gatewayapi.HTTPRoute,
-) (bool, string, error) {
+) (bool, string) {
 	// Translate HTTPRoute to Kong Route object(s) that can be sent directly to the Admin API for validation.
 	// Use KIC parser that works both for traditional and expressions based routes.
 	var kongRoutes []kong.Route
@@ -211,23 +212,23 @@ func validateWithKongGateway(
 		}
 	}
 	if len(errMsgs) > 0 {
-		return false, validationMsg(errMsgs), nil
+		return false, validationMsg(errMsgs)
 	}
 	// Validate by using feature of Kong Gateway.
 	for _, kg := range kongRoutes {
 		kg := kg
 		ok, msg, err := routesValidator.Validate(ctx, &kg)
 		if err != nil {
-			return false, fmt.Sprintf("Unable to validate HTTPRoute schema: %s", err.Error()), nil
+			return false, fmt.Sprintf("Unable to validate HTTPRoute schema: %s", err.Error())
 		}
 		if !ok {
 			errMsgs = append(errMsgs, msg)
 		}
 	}
 	if len(errMsgs) > 0 {
-		return false, validationMsg(errMsgs), nil
+		return false, validationMsg(errMsgs)
 	}
-	return true, "", nil
+	return true, ""
 }
 
 func validationMsg(errMsgs []string) string {

internal/admission/validation/gateway/httproute_test.go

@@ -2,7 +2,6 @@ package gateway
 
 import (
 	"context"
-	"fmt"
 	"testing"
 
 	"github.com/kong/go-kong/kong"
@@ -59,8 +58,7 @@ func TestValidateHTTPRoute(t *testing.T) {
 				},
 			}},
 			valid:         false,
-			validationMsg: "Couldn't determine parentRefs for httproute",
-			err:           fmt.Errorf("no parentRef matched gateway default/testing-gateway"),
+			validationMsg: "Couldn't determine parentRefs for httproute: no parentRef matched gateway default/testing-gateway",
 		},
 		{
 			msg: "if you use sectionname to attach to a non-existent gateway listener, it fails validation",
@@ -98,8 +96,7 @@ func TestValidateHTTPRoute(t *testing.T) {
 				},
 			}},
 			valid:         false,
-			validationMsg: "Couldn't find gateway listeners for httproute",
-			err:           fmt.Errorf("sectionname referenced listener listener-that-doesnt-exist was not found on gateway default/testing-gateway"),
+			validationMsg: "Couldn't find gateway listeners for httproute: sectionname referenced listener listener-that-doesnt-exist was not found on gateway default/testing-gateway",
 		},
 		{
 			msg: "if the provided gateway has NO listeners, the HTTPRoute fails validation",
@@ -126,8 +123,7 @@ func TestValidateHTTPRoute(t *testing.T) {
 				},
 			}},
 			valid:         false,
-			validationMsg: "Couldn't find gateway listeners for httproute",
-			err:           fmt.Errorf("no listeners could be found for gateway default/testing-gateway"),
+			validationMsg: "Couldn't find gateway listeners for httproute: no listeners could be found for gateway default/testing-gateway",
 		},
 		{
 			msg: "parentRefs which omit the namespace pass validation in the same namespace",
@@ -200,8 +196,7 @@ func TestValidateHTTPRoute(t *testing.T) {
 				},
 			}},
 			valid:         false,
-			validationMsg: "HTTPRoute linked Gateway listeners did not pass validation",
-			err:           fmt.Errorf("HTTPRoute not supported by listener http-alternate"),
+			validationMsg: "HTTPRoute linked Gateway listeners did not pass validation: HTTPRoute not supported by listener http-alternate",
 		},
 		{
 			msg: "if an HTTPRoute is using queryparams matching it fails validation due to only supporting expression router",
@@ -253,8 +248,7 @@ func TestValidateHTTPRoute(t *testing.T) {
 				},
 			}},
 			valid:         false,
-			validationMsg: "HTTPRoute spec did not pass validation",
-			err:           fmt.Errorf("queryparam matching is supported with expression router only"),
+			validationMsg: "HTTPRoute spec did not pass validation: queryparam matching is supported with expression router only",
 		},
 		{
 			msg: "we don't support any group except core kubernetes for backendRefs",
@@ -311,8 +305,7 @@ func TestValidateHTTPRoute(t *testing.T) {
 				},
 			}},
 			valid:         false,
-			validationMsg: "HTTPRoute spec did not pass validation",
-			err:           fmt.Errorf("example is not a supported group for httproute backendRefs, only core is supported"),
+			validationMsg: "HTTPRoute spec did not pass validation: example is not a supported group for httproute backendRefs, only core is supported",
 		},
 		{
 			msg: "we don't support any core kind except Service for backendRefs",
@@ -368,17 +361,18 @@ func TestValidateHTTPRoute(t *testing.T) {
 				},
 			}},
 			valid:         false,
-			validationMsg: "HTTPRoute spec did not pass validation",
-			err:           fmt.Errorf("Pod is not a supported kind for httproute backendRefs, only Service is supported"),
+			validationMsg: "HTTPRoute spec did not pass validation: Pod is not a supported kind for httproute backendRefs, only Service is supported",
 		},
 	} {
-		// Passed routesValidator is irrelevant for the above test cases.
-		valid, validMsg, err := ValidateHTTPRoute(
-			context.Background(), mockRoutesValidator{}, parser.FeatureFlags{}, tt.route, tt.gateways...,
-		)
-		assert.Equal(t, tt.valid, valid, tt.msg)
-		assert.Equal(t, tt.validationMsg, validMsg, tt.msg)
-		assert.Equal(t, tt.err, err, tt.msg)
+		t.Run(tt.msg, func(t *testing.T) {
+			// Passed routesValidator is irrelevant for the above test cases.
+			valid, validMsg, err := ValidateHTTPRoute(
+				context.Background(), mockRoutesValidator{}, parser.FeatureFlags{}, tt.route, tt.gateways...,
+			)
+			assert.Equal(t, tt.valid, valid, tt.msg)
+			assert.Equal(t, tt.validationMsg, validMsg, tt.msg)
+			assert.Equal(t, tt.err, err, tt.msg)
+		})
 	}
 }
 

internal/admission/validator.go

@@ -32,7 +32,7 @@ type KongValidator interface {
 	ValidateConsumerGroup(ctx context.Context, consumerGroup kongv1beta1.KongConsumerGroup) (bool, string, error)
 	ValidatePlugin(ctx context.Context, plugin kongv1.KongPlugin) (bool, string, error)
 	ValidateClusterPlugin(ctx context.Context, plugin kongv1.KongClusterPlugin) (bool, string, error)
-	ValidateCredential(ctx context.Context, secret corev1.Secret) (bool, string, error)
+	ValidateCredential(ctx context.Context, secret corev1.Secret) (bool, string)
 	ValidateGateway(ctx context.Context, gateway gatewayapi.Gateway) (bool, string, error)
 	ValidateHTTPRoute(ctx context.Context, httproute gatewayapi.HTTPRoute) (bool, string, error)
 	ValidateIngress(ctx context.Context, ingress netv1.Ingress) (bool, string, error)
@@ -125,7 +125,7 @@ func (validator KongHTTPValidator) ValidateConsumer(
 	// credentials so that the consumers credentials references can be validated.
 	managedConsumers, err := validator.listManagedConsumers(ctx)
 	if err != nil {
-		return false, ErrTextConsumerUnretrievable, err
+		return false, fmt.Sprintf("failed to fetch managed KongConsumers from cache: %s", err), nil
 	}
 
 	// retrieve the consumer's credentials secrets to validate them with the index
@@ -136,14 +136,14 @@ func (validator KongHTTPValidator) ValidateConsumer(
 		secret, err := validator.SecretGetter.GetSecret(consumer.Namespace, secretName)
 		if err != nil {
 			if apierrors.IsNotFound(err) {
-				return false, ErrTextConsumerCredentialSecretNotFound, err
+				return false, fmt.Sprintf("%s: %s", ErrTextConsumerCredentialSecretNotFound, err), nil
 			}
 			return false, ErrTextFailedToRetrieveSecret, err
 		}
 
 		// do the basic credentials validation
 		if err := credsvalidation.ValidateCredentials(secret); err != nil {
-			return false, ErrTextConsumerCredentialValidationFailed, err
+			return false, fmt.Sprintf("%s: %s", ErrTextConsumerCredentialValidationFailed, err), nil
 		}
 
 		// if valid, store it so we can index it for upcoming constraints validation
@@ -164,15 +164,15 @@ func (validator KongHTTPValidator) ValidateConsumer(
 	// testing them against themselves.
 	credentialsIndex, err := globalValidationIndexForCredentials(ctx, validator.ManagerClient, managedConsumers, ignoredSecrets)
 	if err != nil {
-		return false, ErrTextConsumerCredentialValidationFailed, err
+		return false, fmt.Sprintf("%s: %s", ErrTextConsumerCredentialValidationFailed, err), nil
 	}
 
 	// validate the consumer's credentials against the index of all managed
 	// credentials to ensure they're not in violation of any unique constraints.
 	for _, secret := range credentials {
 		// do the unique constraints validation of the credentials using the credentials index
 		if err := credentialsIndex.ValidateCredentialsForUniqueKeyConstraints(secret); err != nil {
-			return false, ErrTextConsumerCredentialValidationFailed, err
+			return false, fmt.Sprintf("%s: %s", ErrTextConsumerCredentialValidationFailed, err), nil
 		}
 	}
 
@@ -232,34 +232,31 @@ func (validator KongHTTPValidator) ValidateConsumerGroup(
 // are present in it or not. If valid, it returns true with an empty string,
 // 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, error) {
+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 {
-		return true, "", nil
+		return true, ""
 	}
 
 	// 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
+		return false, fmt.Sprintf("%s: %s", ErrTextConsumerCredentialValidationFailed, err)
 	}
 
 	// 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
+		return false, fmt.Sprintf("failed to fetch managed KongConsumers from cache: %s", err)
 	}
 
 	// 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.
-		return true, "", nil
+		return true, ""
 	}
 
 	// If base-level validation passed and the credential is referenced by a consumer,
@@ -268,16 +265,16 @@ func (validator KongHTTPValidator) ValidateCredential(
 	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
+		return false, fmt.Sprintf("%s: %s", ErrTextConsumerCredentialValidationFailed, err)
 	}
 
 	// 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, fmt.Sprintf("%s: %s", ErrTextConsumerCredentialValidationFailed, err), nil
+		return false, fmt.Sprintf("%s: %s", ErrTextConsumerCredentialValidationFailed, err)
 	}
 
-	return true, "", nil
+	return true, ""
 }
 
 // ValidatePlugin checks if k8sPlugin is valid. It does so by performing
@@ -404,13 +401,19 @@ func (validator KongHTTPValidator) ValidateHTTPRoute(
 			Namespace: namespace,
 			Name:      string(parentRef.Name),
 		}, &gateway); err != nil {
-			return false, fmt.Sprintf("Couldn't retrieve referenced gateway %s/%s", namespace, parentRef.Name), err
+			if apierrors.IsNotFound(err) {
+				return false, fmt.Sprintf("referenced gateway %s/%s not found", namespace, parentRef.Name), nil
+			}
+			return false, "", err
 		}
 
 		// pull the referenced GatewayClass object from the Gateway
 		gatewayClass := gatewayapi.GatewayClass{}
 		if err := validator.ManagerClient.Get(ctx, client.ObjectKey{Name: string(gateway.Spec.GatewayClassName)}, &gatewayClass); err != nil {
-			return false, fmt.Sprintf("Couldn't retrieve referenced gatewayclass %s", gateway.Spec.GatewayClassName), err
+			if apierrors.IsNotFound(err) {
+				return false, fmt.Sprintf("referenced gatewayclass %s not found", gateway.Spec.GatewayClassName), nil
+			}
+			return false, "", err
 		}
 
 		// determine ultimately whether the Gateway is managed by this controller implementation

internal/admission/validator_test.go

@@ -664,8 +664,7 @@ func TestKongHTTPValidator_ValidateCredential(t *testing.T) {
 				Logger:                   logr.Discard(),
 			}
 
-			ok, msg, err := validator.ValidateCredential(context.Background(), tc.secret)
-			require.NoError(t, err)
+			ok, msg := validator.ValidateCredential(context.Background(), tc.secret)
 			assert.Equal(t, tc.wantOK, ok)
 			assert.Equal(t, tc.wantMessage, msg)
 		})

test/integration/httproute_webhook_test.go

@@ -62,7 +62,7 @@ func commonHTTPRouteValidationTestCases(
 					},
 				},
 			},
-			WantCreateErrSubstring: `Gateway.gateway.networking.k8s.io \"fake-gateway\" not found`,
+			WantCreateErrSubstring: `fake-gateway not found`,
 		},
 		{
 			Name: "an invalid httproute will pass validation if it's not linked to a managed controller (it's not ours)",

test/integration/webhook_test.go

@@ -611,7 +611,7 @@ func ensureAdmissionRegistration(ctx context.Context, t *testing.T, namespace, c
 			Webhooks: []admregv1.ValidatingWebhook{
 				{
 					Name:                    "validations.kong.konghq.com",
-					FailurePolicy:           lo.ToPtr(admregv1.Fail),
+					FailurePolicy:           lo.ToPtr(admregv1.Ignore),
 					SideEffects:             lo.ToPtr(admregv1.SideEffectClassNone),
 					AdmissionReviewVersions: []string{"v1beta1", "v1"},
 					Rules:                   rules,