-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Fix --disable-catch-all #3698
Fix --disable-catch-all #3698
Conversation
// CreatesCatchAll returns whether or not an ingress creates a catch-all backend | ||
func CreatesCatchAll(spec extensions.IngressSpec) bool { | ||
return spec.Backend != nil && len(spec.Rules) == 0 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about implementing the whole logic here - including disableCatchAll
flag check here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed but I'd change the function name to something like IsAllowedIngress
. wdyt?
@@ -617,6 +617,11 @@ func New(checkOCSP bool, | |||
return store | |||
} | |||
|
|||
// CreatesCatchAll returns whether or not an ingress creates a catch-all backend | |||
func CreatesCatchAll(spec extensions.IngressSpec) bool { | |||
return spec.Backend != nil && len(spec.Rules) == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure there is no case where len(spec.Rules) > 0
and it's still a catch-all Ingress?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That condition is checked at https://github.com/kubernetes/ingress-nginx/blob/master/internal/ingress/controller/controller.go#L952
LGTM, please squash your commits into one before we merge this. |
c3dcfd4
to
ca74960
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexkursell, ElvinEfendi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
The
--disable-catch-all
flag that was recently added in #3586 is supposed to disallow ingresses that create a catch all server. Normally, a catch all server is created only if an ingress both defines a default backend and has no rules, however the flag disallows any ingress that just defines a default backend. The result is that ingresses like this are blocked by the flag:This PR tightens the circumstances under which the flag causes an ingress to be ignored from
spec.Backend != nil
tospec.Backend != nil && len(spec.Rules) == 0
, as well as adding a test for the new behaviour.cc: @ElvinEfendi