Use apimachinery conventions for ingress validation errors

pkg/annotations/parser.go

@@ -3,9 +3,10 @@ package annotations
 import (
 	"encoding/json"
 	"fmt"
-	"github.com/pkg/errors"
 	"strconv"
 	"strings"
+
+	"github.com/pkg/errors"
 )
 
 type ParseOptions struct {
@@ -49,8 +50,8 @@ type Parser interface {
 	ParseStringSliceAnnotation(annotation string, value *[]string, annotations map[string]string, opts ...ParseOption) bool
 
 	// ParseJSONAnnotation parses json value into the given interface
-	// returns true if the annotation exists and parser error if any
-	ParseJSONAnnotation(annotation string, value interface{}, annotations map[string]string, opts ...ParseOption) (bool, error)
+	// returns true and the matched annotation key if the annotation exists and parser error if any
+	ParseJSONAnnotation(annotation string, value interface{}, annotations map[string]string, opts ...ParseOption) (bool, string, error)
 
 	// ParseStringMapAnnotation parses comma separated key=value pairs into a map
 	// returns true if the annotation exists
@@ -115,16 +116,16 @@ func (p *suffixAnnotationParser) ParseStringSliceAnnotation(annotation string, v
 	return true
 }
 
-func (p *suffixAnnotationParser) ParseJSONAnnotation(annotation string, value interface{}, annotations map[string]string, opts ...ParseOption) (bool, error) {
+func (p *suffixAnnotationParser) ParseJSONAnnotation(annotation string, value interface{}, annotations map[string]string, opts ...ParseOption) (bool, string, error) {
 	raw := ""
 	exists, matchedKey := p.parseStringAnnotation(annotation, &raw, annotations, opts...)
 	if !exists {
-		return false, nil
+		return false, "", nil
 	}
 	if err := json.Unmarshal([]byte(raw), value); err != nil {
-		return true, errors.Wrapf(err, "failed to parse json annotation, %v: %v", matchedKey, raw)
+		return true, matchedKey, errors.Wrapf(err, "failed to parse json annotation, %v: %v", matchedKey, raw)
 	}
-	return true, nil
+	return true, matchedKey, nil
 }
 
 func (p *suffixAnnotationParser) ParseStringMapAnnotation(annotation string, value *map[string]string, annotations map[string]string, opts ...ParseOption) (bool, error) {

pkg/annotations/parser_test.go

@@ -2,8 +2,9 @@ package annotations
 
 import (
 	"errors"
-	"github.com/stretchr/testify/assert"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
 func Test_annotationParser_ParseStringAnnotation(t *testing.T) {
@@ -379,7 +380,7 @@ func Test_serviceAnnotationParser_ParseJSONAnnotation(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			parser := NewSuffixAnnotationParser(tt.prefix)
 			value := objStruct{}
-			exists, err := parser.ParseJSONAnnotation(tt.suffix, &value, tt.annotations, tt.opts...)
+			exists, _, err := parser.ParseJSONAnnotation(tt.suffix, &value, tt.annotations, tt.opts...)
 			if tt.wantError {
 				assert.True(t, err != nil)
 			} else {
@@ -434,7 +435,7 @@ func Test_serviceAnnotationParser_ParseBoolAnnotation(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			parser := NewSuffixAnnotationParser(tt.prefix)
 			value := false
-			exists, err := parser.ParseJSONAnnotation(tt.suffix, &value, tt.annotations, tt.opts...)
+			exists, _, err := parser.ParseJSONAnnotation(tt.suffix, &value, tt.annotations, tt.opts...)
 			if tt.wantError {
 				assert.True(t, err != nil)
 			} else {

pkg/ingress/auth_config_builder.go

@@ -2,6 +2,7 @@ package ingress
 
 import (
 	"context"
+
 	"github.com/pkg/errors"
 	"sigs.k8s.io/aws-load-balancer-controller/pkg/annotations"
 )
@@ -95,7 +96,7 @@ func (b *defaultAuthConfigBuilder) buildAuthType(_ context.Context, svcAndIngAnn
 
 func (b *defaultAuthConfigBuilder) buildAuthIDPConfigCognito(_ context.Context, svcAndIngAnnotations map[string]string) (*AuthIDPConfigCognito, error) {
 	authIDP := AuthIDPConfigCognito{}
-	exists, err := b.annotationParser.ParseJSONAnnotation(annotations.IngressSuffixAuthIDPCognito, &authIDP, svcAndIngAnnotations)
+	exists, _, err := b.annotationParser.ParseJSONAnnotation(annotations.IngressSuffixAuthIDPCognito, &authIDP, svcAndIngAnnotations)
 	if err != nil {
 		return nil, err
 	}
@@ -107,7 +108,7 @@ func (b *defaultAuthConfigBuilder) buildAuthIDPConfigCognito(_ context.Context,
 
 func (b *defaultAuthConfigBuilder) buildAuthIDPConfigOIDC(_ context.Context, svcAndIngAnnotations map[string]string) (*AuthIDPConfigOIDC, error) {
 	authIDP := AuthIDPConfigOIDC{}
-	exists, err := b.annotationParser.ParseJSONAnnotation(annotations.IngressSuffixAuthIDPOIDC, &authIDP, svcAndIngAnnotations)
+	exists, _, err := b.annotationParser.ParseJSONAnnotation(annotations.IngressSuffixAuthIDPOIDC, &authIDP, svcAndIngAnnotations)
 	if err != nil {
 		return nil, err
 	}

pkg/ingress/class_loader.go

@@ -25,9 +25,12 @@ const (
 	defaultClassAnnotation = "ingressclass.kubernetes.io/is-default-class"
 )
 
-// ErrInvalidIngressClass is an sentinel error that represents the IngressClass configuration for Ingress is invalid.
+// ErrInvalidIngressClass is a sentinel error that represents the IngressClass configuration for Ingress is invalid.
 var ErrInvalidIngressClass = errors.New("invalid ingress class")
 
+// ErrIngressClassNotFound is a sentinel error that indicates the IngressClass for an Ingress is not found.
+var ErrIngressClassNotFound = errors.New("ingress class not found")
+
 // ClassLoader loads IngressClass configurations for Ingress.
 type ClassLoader interface {
 	// Load loads the ClassConfiguration for Ingress with IngressClassName.
@@ -89,7 +92,7 @@ func (l *defaultClassLoader) Load(ctx context.Context, ing *networking.Ingress)
 	ingClass := &networking.IngressClass{}
 	if err := l.client.Get(ctx, ingClassKey, ingClass); err != nil {
 		if apierrors.IsNotFound(err) {
-			return ClassConfiguration{}, fmt.Errorf("%w: %v", ErrInvalidIngressClass, err.Error())
+			return ClassConfiguration{}, fmt.Errorf("%w: %v", ErrIngressClassNotFound, err.Error())
 		}
 		return ClassConfiguration{}, err
 	}

pkg/ingress/class_loader_test.go

@@ -243,7 +243,7 @@ func Test_defaultClassLoader_Load(t *testing.T) {
 					},
 				},
 			},
-			wantErr: errors.New("invalid ingress class: ingressclasses.networking.k8s.io \"awesome-class\" not found"),
+			wantErr: errors.New("ingress class not found: ingressclasses.networking.k8s.io \"awesome-class\" not found"),
 		},
 		{
 			name: "when IngressClass found and belong to other controller",

pkg/ingress/enhanced_backend_builder.go

@@ -160,7 +160,7 @@ func (b *defaultEnhancedBackendBuilder) Build(ctx context.Context, ing *networki
 func (b *defaultEnhancedBackendBuilder) buildConditions(_ context.Context, ingAnnotation map[string]string, svcName string) ([]RuleCondition, error) {
 	var conditions []RuleCondition
 	annotationKey := fmt.Sprintf("conditions.%v", svcName)
-	_, err := b.annotationParser.ParseJSONAnnotation(annotationKey, &conditions, ingAnnotation)
+	_, _, err := b.annotationParser.ParseJSONAnnotation(annotationKey, &conditions, ingAnnotation)
 	if err != nil {
 		return nil, err
 	}
@@ -176,7 +176,7 @@ func (b *defaultEnhancedBackendBuilder) buildConditions(_ context.Context, ingAn
 func (b *defaultEnhancedBackendBuilder) buildActionViaAnnotation(ctx context.Context, ingAnnotation map[string]string, svcName string) (Action, error) {
 	action := Action{}
 	annotationKey := fmt.Sprintf("actions.%v", svcName)
-	exists, err := b.annotationParser.ParseJSONAnnotation(annotationKey, &action, ingAnnotation)
+	exists, _, err := b.annotationParser.ParseJSONAnnotation(annotationKey, &action, ingAnnotation)
 	if err != nil {
 		return Action{}, err
 	}

pkg/ingress/group_loader.go

@@ -145,7 +145,7 @@ func (m *defaultGroupLoader) checkGroupMembershipType(ctx context.Context, group
 		// tolerate ErrInvalidIngressClass for Ingresses that hasn't belongs to the group yet.
 		// for Ingresses that was belongs to the group, we error out to avoid remove the Ingress from IngressGroup since the IngressClass might be accidentally modified.
 		// see https://github.com/kubernetes-sigs/aws-load-balancer-controller/issues/2731
-		if errors.Is(err, ErrInvalidIngressClass) {
+		if errors.Is(err, ErrInvalidIngressClass) || errors.Is(err, ErrIngressClassNotFound) {
 			if hasGroupFinalizer {
 				return groupMembershipTypeNone, ClassifiedIngress{}, errors.Wrapf(err, "Ingress has invalid IngressClass configuration")
 			}

pkg/ingress/group_loader_test.go

@@ -1704,7 +1704,7 @@ func Test_defaultGroupLoader_loadGroupIDIfAnyHelper(t *testing.T) {
 			},
 			wantClassifiedIng: ClassifiedIngress{},
 			wantGroupID:       nil,
-			wantErr:           errors.New("invalid ingress class: ingressclasses.networking.k8s.io \"ing-class\" not found"),
+			wantErr:           errors.New("ingress class not found: ingressclasses.networking.k8s.io \"ing-class\" not found"),
 		},
 		{
 			name: "ingress isn't matched by controller's class",
@@ -2051,7 +2051,7 @@ func Test_defaultGroupLoader_classifyIngress(t *testing.T) {
 				},
 				IngClassConfig: ClassConfiguration{},
 			},
-			wantErr: errors.New("invalid ingress class: ingressclasses.networking.k8s.io \"ing-class\" not found"),
+			wantErr: errors.New("ingress class not found: ingressclasses.networking.k8s.io \"ing-class\" not found"),
 		},
 		{
 			name: "no class specified - manageIngressesWithoutIngressClass is set",

webhooks/networking/ingress_validator.go

@@ -4,11 +4,11 @@ import (
 	"context"
 	"fmt"
 
-	awssdk "github.com/aws/aws-sdk-go/aws"
 	"github.com/go-logr/logr"
 	"github.com/pkg/errors"
 	networking "k8s.io/api/networking/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 	"sigs.k8s.io/aws-load-balancer-controller/pkg/annotations"
 	"sigs.k8s.io/aws-load-balancer-controller/pkg/config"
 	"sigs.k8s.io/aws-load-balancer-controller/pkg/ingress"
@@ -58,19 +58,12 @@ func (v *ingressValidator) ValidateCreate(ctx context.Context, obj runtime.Objec
 	if skip, err := v.checkIngressClass(ctx, ing); skip || err != nil {
 		return err
 	}
-	if err := v.checkIngressClassAnnotationUsage(ing, nil); err != nil {
-		return err
-	}
-	if err := v.checkGroupNameAnnotationUsage(ing, nil); err != nil {
-		return err
-	}
-	if err := v.checkIngressClassUsage(ctx, ing, nil); err != nil {
-		return err
-	}
-	if err := v.checkIngressAnnotationConditions(ing); err != nil {
-		return err
-	}
-	return nil
+	allErrs := field.ErrorList{}
+	allErrs = append(allErrs, v.checkIngressClassAnnotationUsage(ing, nil)...)
+	allErrs = append(allErrs, v.checkGroupNameAnnotationUsage(ing, nil)...)
+	allErrs = append(allErrs, v.checkIngressAnnotationConditions(ing)...)
+
+	return allErrs.ToAggregate()
 }
 
 func (v *ingressValidator) ValidateUpdate(ctx context.Context, obj runtime.Object, oldObj runtime.Object) error {
@@ -79,19 +72,11 @@ func (v *ingressValidator) ValidateUpdate(ctx context.Context, obj runtime.Objec
 	if skip, err := v.checkIngressClass(ctx, ing); skip || err != nil {
 		return err
 	}
-	if err := v.checkIngressClassAnnotationUsage(ing, oldIng); err != nil {
-		return err
-	}
-	if err := v.checkGroupNameAnnotationUsage(ing, oldIng); err != nil {
-		return err
-	}
-	if err := v.checkIngressClassUsage(ctx, ing, oldIng); err != nil {
-		return err
-	}
-	if err := v.checkIngressAnnotationConditions(ing); err != nil {
-		return err
-	}
-	return nil
+	allErrs := field.ErrorList{}
+	allErrs = append(allErrs, v.checkIngressClassAnnotationUsage(ing, oldIng)...)
+	allErrs = append(allErrs, v.checkGroupNameAnnotationUsage(ing, oldIng)...)
+	allErrs = append(allErrs, v.checkIngressAnnotationConditions(ing)...)
+	return allErrs.ToAggregate()
 }
 
 func (v *ingressValidator) ValidateDelete(ctx context.Context, obj runtime.Object) error {
@@ -105,7 +90,10 @@ func (v *ingressValidator) checkIngressClass(ctx context.Context, ing *networkin
 	}
 	classConfiguration, err := v.classLoader.Load(ctx, ing)
 	if err != nil {
-		return false, err
+		if errors.Is(err, ingress.ErrIngressClassNotFound) {
+			return false, field.NotFound(field.NewPath("spec", "ingressClassName"), ing.Spec.IngressClassName)
+		}
+		return false, field.Forbidden(field.NewPath("spec", "ingressClassName"), err.Error())
 	}
 	if classConfiguration.IngClass != nil {
 		return classConfiguration.IngClass.Spec.Controller != ingress.IngressClassControllerALB, nil
@@ -116,7 +104,7 @@ func (v *ingressValidator) checkIngressClass(ctx context.Context, ing *networkin
 // checkIngressClassAnnotationUsage checks the usage of kubernetes.io/ingress.class annotation.
 // kubernetes.io/ingress.class annotation cannot be set to the ingress class for this controller once disabled,
 // so that we enforce users to use spec.ingressClassName in Ingress and IngressClass resource instead.
-func (v *ingressValidator) checkIngressClassAnnotationUsage(ing *networking.Ingress, oldIng *networking.Ingress) error {
+func (v *ingressValidator) checkIngressClassAnnotationUsage(ing *networking.Ingress, oldIng *networking.Ingress) (allErrs field.ErrorList) {
 	if !v.disableIngressClassAnnotation {
 		return nil
 	}
@@ -135,15 +123,17 @@ func (v *ingressValidator) checkIngressClassAnnotationUsage(ing *networking.Ingr
 		}
 	}
 	if !usedInOldIng && usedInNewIng {
-		return errors.Errorf("new usage of `%s` annotation is forbidden", annotations.IngressClass)
+		fieldPath := field.NewPath("metadata", "annotations").Key(annotations.IngressClass)
+		allErrs = append(allErrs, field.Forbidden(fieldPath, "new usage is forbidden"))
 	}
-	return nil
+
+	return allErrs
 }
 
 // checkGroupNameAnnotationUsage checks the usage of "group.name" annotation.
 // "group.name" annotation cannot be set once disabled,
 // so that we enforce users to use spec.group in IngressClassParams resource instead.
-func (v *ingressValidator) checkGroupNameAnnotationUsage(ing *networking.Ingress, oldIng *networking.Ingress) error {
+func (v *ingressValidator) checkGroupNameAnnotationUsage(ing *networking.Ingress, oldIng *networking.Ingress) (allErrs field.ErrorList) {
 	if !v.disableIngressGroupAnnotation {
 		return nil
 	}
@@ -162,68 +152,39 @@ func (v *ingressValidator) checkGroupNameAnnotationUsage(ing *networking.Ingress
 
 	if usedInNewIng {
 		if !usedInOldIng || (newGroupName != oldGroupName) {
-			return errors.Errorf("new usage of `%s/%s` annotation is forbidden", annotations.AnnotationPrefixIngress, annotations.IngressSuffixGroupName)
+			fieldPath := field.NewPath("metadata", "annotations").Key(annotations.AnnotationPrefixIngress + "/" + annotations.IngressSuffixGroupName)
+			allErrs = append(allErrs, field.Forbidden(fieldPath, "new usage is forbidden"))
 		}
 	}
-	return nil
-}
-
-// checkIngressClassUsage checks the usage of "ingressClassName" field.
-// if ingressClassName is mutated, it must refer to a existing & valid IngressClass.
-func (v *ingressValidator) checkIngressClassUsage(ctx context.Context, ing *networking.Ingress, oldIng *networking.Ingress) error {
-	usedInNewIng := false
-	usedInOldIng := false
-	newIngressClassName := ""
-	oldIngressClassName := ""
-
-	if ing.Spec.IngressClassName != nil {
-		usedInNewIng = true
-		newIngressClassName = awssdk.StringValue(ing.Spec.IngressClassName)
-	}
-	if oldIng != nil && oldIng.Spec.IngressClassName != nil {
-		usedInOldIng = true
-		oldIngressClassName = awssdk.StringValue(oldIng.Spec.IngressClassName)
-	}
-
-	if usedInNewIng {
-		if !usedInOldIng || (newIngressClassName != oldIngressClassName) {
-			_, err := v.classLoader.Load(ctx, ing)
-			if err != nil {
-				return err
-			}
-		}
-	}
-	return nil
+	return allErrs
 }
 
 // checkGroupNameAnnotationUsage checks the validity of "conditions.${conditions-name}" annotation.
-func (v *ingressValidator) checkIngressAnnotationConditions(ing *networking.Ingress) error {
+func (v *ingressValidator) checkIngressAnnotationConditions(ing *networking.Ingress) (allErrs field.ErrorList) {
 	for _, rule := range ing.Spec.Rules {
 		if rule.HTTP == nil {
 			continue
 		}
 		for _, path := range rule.HTTP.Paths {
 			var conditions []ingress.RuleCondition
 			annotationKey := fmt.Sprintf("conditions.%v", path.Backend.Service.Name)
-			_, err := v.annotationParser.ParseJSONAnnotation(annotationKey, &conditions, ing.Annotations)
+			_, matchedKey, err := v.annotationParser.ParseJSONAnnotation(annotationKey, &conditions, ing.Annotations)
+			fieldPath := field.NewPath("metadata", "annotations").Key(matchedKey)
 			if err != nil {
-				return err
+				allErrs = append(allErrs, field.Invalid(fieldPath, ing.Annotations[matchedKey], err.Error()))
+				continue
 			}
 
 			for _, condition := range conditions {
 				if err := condition.Validate(); err != nil {
-					return fmt.Errorf("ignoring Ingress %s/%s since invalid alb.ingress.kubernetes.io/conditions.%s annotation: %w",
-						ing.Namespace,
-						ing.Name,
-						path.Backend.Service.Name,
-						err,
-					)
+					allErrs = append(allErrs, field.Invalid(fieldPath, ing.Annotations[matchedKey], err.Error()))
+					continue
 				}
 			}
 		}
 	}
 
-	return nil
+	return allErrs
 }
 
 // +kubebuilder:webhook:path=/validate-networking-v1-ingress,mutating=false,failurePolicy=fail,groups=networking.k8s.io,resources=ingresses,verbs=create;update,versions=v1,name=vingress.elbv2.k8s.aws,sideEffects=None,matchPolicy=Equivalent,webhookVersions=v1,admissionReviewVersions=v1beta1