Fix 503 error generation on empty endpoints #3628

0robustus1 · 2019-01-04T14:06:38Z

What this PR does / why we need it:

It provides two tests (minimal example) to illustrate a problem when encountering a service with no endpoints. One expects a generated 503 response in this case by the ingress-nginx.
But if such a change happened during runtime (e.g. replica-count changed from 2 to 0)
a 502 or 504 is returned instead.

This issue seems to have been outlined by the following github issues:

I also added an attempt at fixing the issues (by removing the backend from the lua cache when there are no endpoints, instead of skipping it).

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

Special notes for your reviewer:

k8s-ci-robot · 2019-01-04T15:08:21Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

aledbf · 2019-01-04T23:00:45Z

@0robustus1 please sign the CLA

ElvinEfendi · 2019-01-04T23:45:20Z

test/e2e/lua/dynamic_configuration.go

+		Get(f.IngressController.HTTPURL).
+		Set("Host", host).
+		End()
+	Expect(errs).Should(BeEmpty())


please reuse runRequest here to keep things DRY

ElvinEfendi · 2019-01-04T23:45:51Z

test/e2e/lua/dynamic_configuration.go

@@ -189,6 +249,24 @@ func ensureRequest(f *framework.Framework, host string) {
 	Expect(resp.StatusCode).Should(Equal(http.StatusOK))
 }

+func ensureRequestWithStatus(f *framework.Framework, host string, statusCode int) {


now that we have this helper we can refactor ensureRequest to use this function

ElvinEfendi · 2019-01-04T23:50:37Z

test/e2e/lua/dynamic_configuration.go

@@ -92,6 +92,60 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() {
 			Expect(nginxConfig).Should(Equal(newNginxConfig))
 		})

+		It("handles endpoints only changes (down scaling of replicas)", func() {


can you explain why is this necessary given you have https://github.com/kubernetes/ingress-nginx/pull/3628/files#diff-8700c46a8379c603b5e41ccbaff6b71eR124?

Seems like this is an unnecessary remnant from a first version of the test. I removed it.

ElvinEfendi · 2019-01-04T23:54:13Z

test/e2e/lua/dynamic_configuration.go

+			ensureRequestWithStatus(f, "foo.com", 503)
+		})
+
+		It("handles endpoints only changes consistently (down scaling of replicas vs. empty service)", func() {


what about

It("returns 503 when a service is created without replicas or replicas scaled down to zero at runtime", function(...)

ElvinEfendi · 2019-01-07T12:05:46Z

LGTM. Please sign the CLA and squash commits into one before we merge.

aledbf · 2019-01-07T20:34:18Z

/check-cla

aledbf · 2019-01-08T22:55:06Z

@0robustus1 friendly ping. I want to include this PR in 0.22 (last PR) but I am not sure if you can sign the CLA and squash the commits. Please let me know :)

0robustus1 · 2019-01-09T07:44:36Z

@aledbf i squashed the commit. Regarding the CLA we're setting up CCLA which is why it is taking a little while; we should get it sorted this week. Sorry for that.

0robustus1 · 2019-01-10T16:40:33Z

It turns out the CCLA setup on our side takes a little bit longer than originally expected. I'll keep you posted on the developments. (Sorry again)

ElvinEfendi · 2019-01-18T15:44:56Z

@0robustus1 any update with the CLA process?

0robustus1 · 2019-01-18T16:10:43Z

Sadly not yet.

* related to: * kubernetes#3070 * kubernetes#3335 * add a 503 test * test a service that starts out empty (a.k.a. ingress-nginx controller (re-)start) * test scaling up (should route traffic accordingly) * test scaling down to empty service * use custom deployments for scaling test. * provide a fix by updating the lua table (cache) of the configured backends to unset the backend if there are no endpoints available.

aledbf · 2019-02-03T13:06:49Z

/lgtm
/approve

aledbf · 2019-02-03T13:06:55Z

@0robustus1 thanks!

If there are no replicas defined, do not wait around for the respective endpoints, since none are expected.

aledbf · 2019-02-03T19:59:32Z

/lgtm

k8s-ci-robot · 2019-02-03T19:59:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 0robustus1, aledbf

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~~ [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 the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 4, 2019

k8s-ci-robot requested review from antoineco and ElvinEfendi January 4, 2019 14:07

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jan 4, 2019

aledbf assigned ElvinEfendi Jan 4, 2019

ElvinEfendi reviewed Jan 4, 2019

View reviewed changes

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 7, 2019

0robustus1 force-pushed the fix-503-error-generation-on-empty-endpoints branch from 8c46e85 to b1bd660 Compare January 9, 2019 07:40

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 9, 2019

0robustus1 force-pushed the fix-503-error-generation-on-empty-endpoints branch from b1bd660 to e489f70 Compare January 9, 2019 07:43

0robustus1 force-pushed the fix-503-error-generation-on-empty-endpoints branch from e489f70 to 6cbd918 Compare January 9, 2019 09:19

ElvinEfendi mentioned this pull request Jan 18, 2019

Ingress returns response if there are no endpoints for service #3523

Closed

ElvinEfendi mentioned this pull request Jan 19, 2019

fix removing endpoints from balancing if there is no endpoints #3681

Closed

0robustus1 force-pushed the fix-503-error-generation-on-empty-endpoints branch from 6cbd918 to 018a1e4 Compare February 3, 2019 10:44

k8s-ci-robot removed the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Feb 3, 2019

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 3, 2019

k8s-ci-robot assigned aledbf Feb 3, 2019

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 3, 2019

do not wait for endpoints that shouldn't exist

16dcace

If there are no replicas defined, do not wait around for the respective endpoints, since none are expected.

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 3, 2019

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

k8s-ci-robot merged commit 3db1dc1 into kubernetes:master Feb 3, 2019

aledbf mentioned this pull request Feb 21, 2019

ingress nginx still routes traffic to Unready pod #3789

Closed

aledbf mentioned this pull request Mar 20, 2019

Ingress resolves old svc #3911

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 503 error generation on empty endpoints #3628

Fix 503 error generation on empty endpoints #3628

0robustus1 commented Jan 4, 2019

k8s-ci-robot commented Jan 4, 2019

aledbf commented Jan 4, 2019

ElvinEfendi Jan 4, 2019

ElvinEfendi Jan 4, 2019

ElvinEfendi Jan 4, 2019

0robustus1 Jan 7, 2019

ElvinEfendi Jan 4, 2019

ElvinEfendi commented Jan 7, 2019 •

edited

Loading

aledbf commented Jan 7, 2019

aledbf commented Jan 8, 2019 •

edited

Loading

0robustus1 commented Jan 9, 2019

0robustus1 commented Jan 10, 2019

ElvinEfendi commented Jan 18, 2019

0robustus1 commented Jan 18, 2019

aledbf commented Feb 3, 2019

aledbf commented Feb 3, 2019

aledbf commented Feb 3, 2019

k8s-ci-robot commented Feb 3, 2019

Fix 503 error generation on empty endpoints #3628

Fix 503 error generation on empty endpoints #3628

Conversation

0robustus1 commented Jan 4, 2019

k8s-ci-robot commented Jan 4, 2019

aledbf commented Jan 4, 2019

ElvinEfendi Jan 4, 2019

Choose a reason for hiding this comment

ElvinEfendi Jan 4, 2019

Choose a reason for hiding this comment

ElvinEfendi Jan 4, 2019

Choose a reason for hiding this comment

0robustus1 Jan 7, 2019

Choose a reason for hiding this comment

ElvinEfendi Jan 4, 2019

Choose a reason for hiding this comment

ElvinEfendi commented Jan 7, 2019 • edited Loading

aledbf commented Jan 7, 2019

aledbf commented Jan 8, 2019 • edited Loading

0robustus1 commented Jan 9, 2019

0robustus1 commented Jan 10, 2019

ElvinEfendi commented Jan 18, 2019

0robustus1 commented Jan 18, 2019

aledbf commented Feb 3, 2019

aledbf commented Feb 3, 2019

aledbf commented Feb 3, 2019

k8s-ci-robot commented Feb 3, 2019

ElvinEfendi commented Jan 7, 2019 •

edited

Loading

aledbf commented Jan 8, 2019 •

edited

Loading