Skip to content
This repository has been archived by the owner on Apr 17, 2019. It is now read-only.

[nginx-ingress-controller] Readiness probe that works behind a CP lb #1749

Merged

Conversation

aledbf
Copy link
Contributor

@aledbf aledbf commented Sep 16, 2016

fixes #1507


This change is Reviewable

Copy link

@bprashanth bprashanth left a comment

Choose a reason for hiding this comment

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

awesome! thanks just the one nit

@@ -265,6 +265,12 @@ http {
{{ end }}

{{ if eq $server.Name "_" }}
# health checks in cloud providers require the use of port 80
location /ingress-controller-healthz {

Choose a reason for hiding this comment

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

can you make this the default, and allow people to config it through yaml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using a flag with /ingress-controller-healthz as default is ok?

Choose a reason for hiding this comment

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

any reason it shouldn't be a flag? if i have an ingress controller behind an ingress controller how would i modify it :)

@bprashanth
Copy link

Thanks LGTM, modifying title to note have wip

@bprashanth bprashanth changed the title WIP: [nginx-ingress-controller] Readiness probe that works behind a CP lb [nginx-ingress-controller] Readiness probe that works behind a CP lb Sep 16, 2016
@bprashanth bprashanth added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Sep 16, 2016
@bprashanth
Copy link

oh please add a todo to scrub incoming ingresses for the url passed through the arg (can come laters, or just file a bug so we can track it)

@aledbf
Copy link
Contributor Author

aledbf commented Sep 19, 2016

file a bug so we can track it)

done #1754

@test-foxish
Copy link

recomputing cla status...

@k8s-github-robot k8s-github-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 22, 2016
@bprashanth bprashanth added the lgtm Indicates that a PR is ready to be merged. label Sep 26, 2016
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 890cbb1 into kubernetes-retired:master Sep 26, 2016
@aledbf aledbf deleted the cloud-health-check branch September 27, 2016 02:57
aledbf pushed a commit to aledbf/contrib that referenced this pull request Nov 10, 2016
…heck

Automatic merge from submit-queue

[nginx-ingress-controller] Readiness probe that works behind a CP lb

fixes kubernetes-retired#1507
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Figure out readiness probe for nginx ingress that works behind a CP lb
6 participants