From f944783ba6c8e8c154fb0e3d93a651d2fdef621c Mon Sep 17 00:00:00 2001 From: Roma Erema Date: Tue, 21 Feb 2023 03:21:49 +0300 Subject: [PATCH] validation ingress condition annotation in validation webhook (#2735) --- pkg/ingress/config_types.go | 2 +- pkg/ingress/enhanced_backend_builder.go | 2 +- webhooks/networking/ingress_validator.go | 34 +++++ webhooks/networking/ingress_validator_test.go | 116 ++++++++++++++++++ 4 files changed, 152 insertions(+), 2 deletions(-) diff --git a/pkg/ingress/config_types.go b/pkg/ingress/config_types.go index d90d59aa3..6e2da610e 100644 --- a/pkg/ingress/config_types.go +++ b/pkg/ingress/config_types.go @@ -322,7 +322,7 @@ type RuleCondition struct { SourceIPConfig *SourceIPConditionConfig `json:"sourceIPConfig"` } -func (c *RuleCondition) validate() error { +func (c *RuleCondition) Validate() error { switch c.Field { case RuleConditionFieldHostHeader: if c.HostHeaderConfig == nil { diff --git a/pkg/ingress/enhanced_backend_builder.go b/pkg/ingress/enhanced_backend_builder.go index ea2392244..c9a0ba14f 100644 --- a/pkg/ingress/enhanced_backend_builder.go +++ b/pkg/ingress/enhanced_backend_builder.go @@ -165,7 +165,7 @@ func (b *defaultEnhancedBackendBuilder) buildConditions(_ context.Context, ingAn return nil, err } for _, condition := range conditions { - if err := condition.validate(); err != nil { + if err := condition.Validate(); err != nil { return nil, err } } diff --git a/webhooks/networking/ingress_validator.go b/webhooks/networking/ingress_validator.go index 7f1e85585..4397972b6 100644 --- a/webhooks/networking/ingress_validator.go +++ b/webhooks/networking/ingress_validator.go @@ -2,6 +2,7 @@ package networking import ( "context" + "fmt" awssdk "github.com/aws/aws-sdk-go/aws" "github.com/go-logr/logr" @@ -59,6 +60,9 @@ func (v *ingressValidator) ValidateCreate(ctx context.Context, obj runtime.Objec if err := v.checkIngressClassUsage(ctx, ing, nil); err != nil { return err } + if err := v.checkIngressAnnotationConditions(ing); err != nil { + return err + } return nil } @@ -74,6 +78,9 @@ func (v *ingressValidator) ValidateUpdate(ctx context.Context, obj runtime.Objec if err := v.checkIngressClassUsage(ctx, ing, oldIng); err != nil { return err } + if err := v.checkIngressAnnotationConditions(ing); err != nil { + return err + } return nil } @@ -164,6 +171,33 @@ func (v *ingressValidator) checkIngressClassUsage(ctx context.Context, ing *netw return nil } +// checkGroupNameAnnotationUsage checks the validity of "conditions.${conditions-name}" annotation. +func (v *ingressValidator) checkIngressAnnotationConditions(ing *networking.Ingress) error { + for _, rule := range ing.Spec.Rules { + 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) + if err != nil { + return err + } + + 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, + ) + } + } + } + } + + return nil +} + // +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 func (v *ingressValidator) SetupWithManager(mgr ctrl.Manager) { diff --git a/webhooks/networking/ingress_validator_test.go b/webhooks/networking/ingress_validator_test.go index aad3fa436..0419522a1 100644 --- a/webhooks/networking/ingress_validator_test.go +++ b/webhooks/networking/ingress_validator_test.go @@ -787,3 +787,119 @@ func Test_ingressValidator_checkIngressClassUsage(t *testing.T) { }) } } + +func Test_ingressValidator_checkIngressAnnotationConditions(t *testing.T) { + type fields struct { + disableIngressGroupAnnotation bool + } + type args struct { + ing *networking.Ingress + } + tests := []struct { + name string + fields fields + args args + wantErr error + }{ + { + name: "ingress has valid condition", + fields: fields{ + disableIngressGroupAnnotation: false, + }, + args: args{ + ing: &networking.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns-1", + Name: "ing-1", + Annotations: map[string]string{ + "alb.ingress.kubernetes.io/condition.svc-1": `[{"field":"query-string","queryStringConfig":{"values":[{"key":"paramA","value":"paramAValue"}]}}]`, + }, + }, + Spec: networking.IngressSpec{ + Rules: []networking.IngressRule{ + { + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{ + Paths: []networking.HTTPIngressPath{ + { + Path: "/ing-1-path", + Backend: networking.IngressBackend{ + Service: &networking.IngressServiceBackend{ + Name: "svc-1", + Port: networking.ServiceBackendPort{ + Name: "https", + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + wantErr: nil, + }, + { + name: "ingress has invalid condition", + fields: fields{ + disableIngressGroupAnnotation: false, + }, + args: args{ + ing: &networking.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns-1", + Name: "ing-1", + Annotations: map[string]string{ + "alb.ingress.kubernetes.io/conditions.svc-1": `[{"field":"query-string","queryStringConfig":{"values":[{"key":"paramA","value":""}]}}]`, + }, + }, + Spec: networking.IngressSpec{ + Rules: []networking.IngressRule{ + { + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{ + Paths: []networking.HTTPIngressPath{ + { + Path: "/ing-1-path", + Backend: networking.IngressBackend{ + Service: &networking.IngressServiceBackend{ + Name: "svc-1", + Port: networking.ServiceBackendPort{ + Name: "https", + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + wantErr: errors.New("ignoring Ingress ns-1/ing-1 since invalid alb.ingress.kubernetes.io/conditions.svc-1 annotation: invalid queryStringConfig: value cannot be empty"), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + annotationParser := annotations.NewSuffixAnnotationParser("alb.ingress.kubernetes.io") + classAnnotationMatcher := ingress.NewDefaultClassAnnotationMatcher("alb") + v := &ingressValidator{ + annotationParser: annotationParser, + classAnnotationMatcher: classAnnotationMatcher, + disableIngressGroupAnnotation: tt.fields.disableIngressGroupAnnotation, + logger: logr.Discard(), + } + err := v.checkIngressAnnotationConditions(tt.args.ing) + if tt.wantErr != nil { + assert.EqualError(t, err, tt.wantErr.Error()) + } else { + assert.NoError(t, err) + } + }) + } +}