Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not validate ingresses with unknown ingress class in admission webhook endpoint #8221

Merged
merged 1 commit into from
Feb 6, 2022
Merged
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
6 changes: 6 additions & 0 deletions internal/ingress/controller/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,12 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error {
return nil
}

// Do not attempt to validate an ingress that's not meant to be controlled by the current instance of the controller.
if ingressClass, err := n.store.GetIngressClass(ing, n.cfg.IngressClassConfiguration); ingressClass == "" {
klog.Warningf("ignoring ingress %v in %v based on annotation %v: %v", ing.Name, ing.ObjectMeta.Namespace, ingressClass, err)

This comment was marked as resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

What if err != nil?

Copy link
Member Author

Choose a reason for hiding this comment

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

GetIngressClass is kinda strange, err actually going to be != nil as long as ingressClass == ""

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed, unless some day we change GetIngressClass and the function returns a strange value due to...reasons...and an error and as we are just validating the IngressClass we end into a panic :P

I agree with you that right now this is too much, I'm afraid of us in a future doing something that relies on this error, not checking and getting a BSOD in ingress controller

return nil
}

if n.cfg.Namespace != "" && ing.ObjectMeta.Namespace != n.cfg.Namespace {
klog.Warningf("ignoring ingress %v in namespace %v different from the namespace watched %s", ing.Name, ing.ObjectMeta.Namespace, n.cfg.Namespace)
return nil
Expand Down
4 changes: 4 additions & 0 deletions internal/ingress/controller/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ type fakeIngressStore struct {
configuration ngx_config.Configuration
}

func (fakeIngressStore) GetIngressClass(ing *networking.Ingress, icConfig *ingressclass.IngressClassConfiguration) (string, error) {
return "nginx", nil
}

func (fis fakeIngressStore) GetBackendConfiguration() ngx_config.Configuration {
return fis.configuration
}
Expand Down
3 changes: 3 additions & 0 deletions internal/ingress/controller/store/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,9 @@ type Storer interface {

// Run initiates the synchronization of the controllers
Run(stopCh chan struct{})

// GetIngressClass validates given ingress against ingress class configuration and returns the ingress class.
GetIngressClass(ing *networkingv1.Ingress, icConfig *ingressclass.IngressClassConfiguration) (string, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

a bit afraid here of expanding the interface, but we gotta solve this :) so lgtm

}

// EventType type of event associated with an informer
Expand Down
29 changes: 29 additions & 0 deletions test/e2e/admission/admission.go
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,12 @@ var _ = framework.IngressNginxDescribe("[Serial] admission controller", func() {
assert.NotNil(ginkgo.GinkgoT(), err, "creating an ingress with invalid configuration should return an error")
}
})

ginkgo.It("should not return an error for an invalid Ingress when it has unknown class", func() {
out, err := createIngress(f.Namespace, invalidV1IngressWithOtherClass)
assert.Equal(ginkgo.GinkgoT(), "ingress.networking.k8s.io/extensions-invalid-other created\n", out)
assert.Nil(ginkgo.GinkgoT(), err, "creating an invalid ingress with unknown class using kubectl")
})
})

func uninstallChart(f *framework.Framework) error {
Expand Down Expand Up @@ -270,6 +276,29 @@ spec:
port:
number: 80
---
`
invalidV1IngressWithOtherClass = `
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
name: extensions-invalid-other
annotations:
nginx.ingress.kubernetes.io/configuration-snippet: |
invalid directive
spec:
ingressClassName: nginx-other
rules:
- host: extensions-invalid
http:
paths:
- path: /
pathType: Prefix
backend:
service:
name: echo
port:
number: 80
---
`
)

Expand Down