Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions webhooks/networking/ingress_validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ func (v *ingressValidator) ValidateCreate(ctx context.Context, obj runtime.Objec

func (v *ingressValidator) ValidateUpdate(ctx context.Context, obj runtime.Object, oldObj runtime.Object) error {
ing := obj.(*networking.Ingress)
// skip validation for ingresses being deleted to allow finalizer removal when IngressClass is already gone.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're not really checking if the IngressClass is gone here. If the intended logic is to allow Ingress deletion to proceed when IngressClass is already gone, then adding a check for a NotFoundError is probably better.
https://github.com/aydosman/aws-load-balancer-controller/blob/5d4c3e1288ccfa840027d12c028ca3651c18de9d/webhooks/networking/ingress_validator.go#L90-L94

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zac-nixon Thank you; I was going for the simplest solution. I will look at this at some point this week.

if !ing.DeletionTimestamp.IsZero() {
return nil
}
oldIng := oldObj.(*networking.Ingress)
if skip, err := v.checkIngressClass(ctx, ing); skip || err != nil {
v.metricsCollector.ObserveWebhookValidationError(apiPathValidateNetworkingIngress, "checkIngressClass")
Expand Down
81 changes: 81 additions & 0 deletions webhooks/networking/ingress_validator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1171,3 +1171,84 @@ func Test_ingressValidator_checkIngressAnnotationConditions(t *testing.T) {
})
}
}

func Test_ingressValidator_ValidateUpdate_deletingIngress(t *testing.T) {
now := metav1.Now()
tests := []struct {
name string
ing *networking.Ingress
oldIng *networking.Ingress
wantErr bool
}{
{
name: "ingress being deleted with missing IngressClass should be allowed",
ing: &networking.Ingress{
ObjectMeta: metav1.ObjectMeta{
Namespace: "ns-1",
Name: "ing-1",
DeletionTimestamp: &now,
Finalizers: []string{"ingress.k8s.aws/resources"},
},
Spec: networking.IngressSpec{
IngressClassName: awssdk.String("deleted-ingress-class"),
},
},
oldIng: &networking.Ingress{
ObjectMeta: metav1.ObjectMeta{
Namespace: "ns-1",
Name: "ing-1",
},
Spec: networking.IngressSpec{
IngressClassName: awssdk.String("deleted-ingress-class"),
},
},
wantErr: false,
},
{
name: "ingress not being deleted with missing IngressClass should be denied",
ing: &networking.Ingress{
ObjectMeta: metav1.ObjectMeta{
Namespace: "ns-1",
Name: "ing-1",
},
Spec: networking.IngressSpec{
IngressClassName: awssdk.String("deleted-ingress-class"),
},
},
oldIng: &networking.Ingress{
ObjectMeta: metav1.ObjectMeta{
Namespace: "ns-1",
Name: "ing-1",
},
Spec: networking.IngressSpec{
IngressClassName: awssdk.String("other-class"),
},
},
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ctx := context.Background()
k8sSchema := runtime.NewScheme()
clientgoscheme.AddToScheme(k8sSchema)
elbv2api.AddToScheme(k8sSchema)
k8sClient := testclient.NewClientBuilder().WithScheme(k8sSchema).Build()
mockMetricsCollector := lbcmetrics.NewMockCollector()
v := &ingressValidator{
annotationParser: annotations.NewSuffixAnnotationParser(annotations.AnnotationPrefixIngress),
classAnnotationMatcher: ingress.NewDefaultClassAnnotationMatcher("alb"),
classLoader: ingress.NewDefaultClassLoader(k8sClient, false),
manageIngressesWithoutIngressClass: false,
logger: logr.New(&log.NullLogSink{}),
metricsCollector: mockMetricsCollector,
}
err := v.ValidateUpdate(ctx, tt.ing, tt.oldIng)
if tt.wantErr {
assert.Error(t, err)
} else {
assert.NoError(t, err)
}
})
}
}