Add --disable-catch-all option to disable catch-all server #3586

wayt · 2018-12-19T22:05:22Z

What this PR does / why we need it:

This PR adds the possibility to disable catch-all Ingress.
It's turned off by default.

When turned on, all Ingress that defines a .spec.backend will be ignored on the store level.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

A bug with Ingress update handler has been found while working on this.
I did not fix it here as it's unrelated.

test/e2e/framework/deployment.go

ElvinEfendi · 2018-12-20T13:19:30Z

Test failure seems legit.

wayt · 2018-12-21T19:04:36Z

@ElvinEfendi it's all good 🎉

aledbf · 2018-12-26T23:22:22Z

A bug with Ingress update handler has been found while working on this.

Can you be more specific?

aledbf · 2018-12-26T23:22:27Z

/approve

aledbf · 2019-01-04T22:57:51Z

@ElvinEfendi ping

ElvinEfendi · 2019-01-07T14:54:26Z

internal/ingress/controller/store/store.go

 				klog.Infof("creating ingress %v based on annotation %v", curIng.Name, class.IngressKey)
 				recorder.Eventf(curIng, corev1.EventTypeNormal, "CREATE", fmt.Sprintf("Ingress %s/%s", curIng.Namespace, curIng.Name))
 			} else if validOld && !validCur {
 				klog.Infof("removing ingress %v based on annotation %v", curIng.Name, class.IngressKey)
+				// FIXME: this does not actually delete the Ingress resource.
+				// The existing one will be updated with latest changes and invalid ingress.class will be missed.
 				recorder.Eventf(curIng, corev1.EventTypeNormal, "DELETE", fmt.Sprintf("Ingress %s/%s", curIng.Namespace, curIng.Name))


@aledbf is this an intended behaviour of the controller to not ignore an ingress after it changes its class to something that's not handled by ingress-nginx?

No, this is a bug introduced when I added a copy of the Ingress with annotations. We should remove the ingress from the local store (store.listers.IngressWithAnnotation.Delete(oldIng))

Alright, we will fix it in a subsequent PR then.

👍 I'll have a look

ElvinEfendi · 2019-01-07T14:56:32Z

internal/ingress/controller/store/store.go

+					klog.Infof("ignoring update for catch-all ingress %v/%v because of --disable-catch-all", curIng.Namespace, curIng.Name)
+					// FIXME: this does not actually delete the Ingress resource.
+					// The existing one will be updated with latest changes.
+					recorder.Eventf(curIng, corev1.EventTypeNormal, "DELETE", fmt.Sprintf("Ingress %s/%s", curIng.Namespace, curIng.Name))


Why are you not deleting ingress here? To keep it consistent with the valid class behaviour? If so then why are you recording DELETE event rather than UPDATE?

This has the same behavior has the valid class check. It records a DELETE event, but don't actually delete it.

I wanted more feedback on this issue before trying any fix here.
I've quickly tested to call the DeleteFunc handler when needed here and it works as expected. But I'm not 100% sure this is the solution.

Also, I think this should be fixed in a 2nd PR (that can be merge before this one).

ElvinEfendi · 2019-01-07T15:08:21Z

test/e2e/settings/disable_catch_all.go

+		f.WaitForNginxServer("_", func(cfg string) bool {
+			return strings.Contains(cfg, `set $ingress_name ""`) &&
+				strings.Contains(cfg, `set $proxy_upstream_name "upstream-default-backend"`)
+		})


While this works, it leaks internals of Nginx configuration to the test. I know we do it in another e2e tests, but here we can avoid this and test the functionality by sending a request and asserting that the response is 404.

Similarly instead of https://github.com/kubernetes/ingress-nginx/pull/3586/files#diff-cb7b9b28ba2a89db95d178caee979d06R57 you can send a request and assert 200 response code.

ElvinEfendi · 2019-01-07T15:14:57Z

/lgtm
/approve

In the subsequent PR we should also consider testing this case https://github.com/kubernetes/ingress-nginx/pull/3586/files#diff-ece58b3bebec02a6d3173c8d8099d298R348.

k8s-ci-robot · 2019-01-07T15:15:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, ElvinEfendi, wayt

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:

~~OWNERS~~ [ElvinEfendi,aledbf]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 19, 2018

k8s-ci-robot requested review from aledbf and antoineco December 19, 2018 22:05

wayt force-pushed the disable-catch-all branch from 5df1e69 to 8fd7508 Compare December 19, 2018 22:06

ElvinEfendi reviewed Dec 20, 2018

View reviewed changes

test/e2e/framework/deployment.go Outdated Show resolved Hide resolved

wayt force-pushed the disable-catch-all branch 5 times, most recently from dc310a6 to d106817 Compare December 21, 2018 16:21

Add --disable-catch-all option to disable catch-all server

1678d99

wayt force-pushed the disable-catch-all branch from d106817 to 1678d99 Compare December 21, 2018 18:22

aledbf added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 26, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 26, 2018

aledbf assigned ElvinEfendi Dec 26, 2018

aledbf removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 4, 2019

ElvinEfendi reviewed Jan 7, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 7, 2019

k8s-ci-robot merged commit 8f57f95 into kubernetes:master Jan 7, 2019

ElvinEfendi deleted the disable-catch-all branch January 7, 2019 15:16

wayt mentioned this pull request Jan 7, 2019

Fix a bug in Ingress update handler #3633

Merged

wayt restored the disable-catch-all branch January 8, 2019 14:29

wayt deleted the disable-catch-all branch January 8, 2019 14:29

alexkursell mentioned this pull request Jan 25, 2019

Fix --disable-catch-all #3698

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --disable-catch-all option to disable catch-all server #3586

Add --disable-catch-all option to disable catch-all server #3586

wayt commented Dec 19, 2018

ElvinEfendi commented Dec 20, 2018

wayt commented Dec 21, 2018

aledbf commented Dec 26, 2018

aledbf commented Dec 26, 2018

aledbf commented Jan 4, 2019

ElvinEfendi Jan 7, 2019

aledbf Jan 7, 2019

ElvinEfendi Jan 7, 2019

wayt Jan 7, 2019

ElvinEfendi Jan 7, 2019

wayt Jan 7, 2019

ElvinEfendi Jan 7, 2019

ElvinEfendi Jan 7, 2019 •

edited

Loading

ElvinEfendi commented Jan 7, 2019

k8s-ci-robot commented Jan 7, 2019

Add --disable-catch-all option to disable catch-all server #3586

Add --disable-catch-all option to disable catch-all server #3586

Conversation

wayt commented Dec 19, 2018

ElvinEfendi commented Dec 20, 2018

wayt commented Dec 21, 2018

aledbf commented Dec 26, 2018

aledbf commented Dec 26, 2018

aledbf commented Jan 4, 2019

ElvinEfendi Jan 7, 2019

Choose a reason for hiding this comment

aledbf Jan 7, 2019

Choose a reason for hiding this comment

ElvinEfendi Jan 7, 2019

Choose a reason for hiding this comment

wayt Jan 7, 2019

Choose a reason for hiding this comment

ElvinEfendi Jan 7, 2019

Choose a reason for hiding this comment

wayt Jan 7, 2019

Choose a reason for hiding this comment

ElvinEfendi Jan 7, 2019

Choose a reason for hiding this comment

ElvinEfendi Jan 7, 2019 • edited Loading

Choose a reason for hiding this comment

ElvinEfendi commented Jan 7, 2019

k8s-ci-robot commented Jan 7, 2019

ElvinEfendi Jan 7, 2019 •

edited

Loading