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

Add annotation to allow use of service ClusterIP for NGINX upstream. #981

Merged
merged 1 commit into from
Jul 19, 2017

Conversation

chrismoos
Copy link
Contributor

This change adds a new annotation ingress.kubernetes.io/service-upstream that when set to true, uses a service's Cluster IP and port as the single upstream server, rather than the full list of service endpoints. This may address #257.

There are now no NGINX configuration reloads necessary when a service's pods come and go.

This helps with things like zero-downtime deployments and other issues arising from NGINX configuration reloads for upstream server changes.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 16, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@chrismoos
Copy link
Contributor Author

@aledbf I see there is a good amount of activity around NGINX ingress and configuration reloads for upstream changes (kubernetes-retired/contrib#1140), hopefully this change is worth exploring. I'm not sure when ingress development started but now kube-proxy is not really used and everything is done via iptables, so performance shouldn't be an issue. I'm not sure how frequently iptables is updated (versus the endpoints in the ingress controller) so that might also be relevant.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 44.104% when pulling 70753b978007721290dc8dc3739a655cb72025d6 on chrismoos:service_upstream into 82e75ee on kubernetes:master.

@aledbf
Copy link
Member

aledbf commented Jul 17, 2017

@aledbf
Copy link
Member

aledbf commented Jul 17, 2017

@chrismoos I really don't like this because introduces another variable (kube-proxy).
If you are willing to provide support in the long term for this feature I'm ok to merge this.

@chrismoos
Copy link
Contributor Author

@aledbf I'll document the annotation with a warning that sticky sessions will not work. As for the retries -- I believe it will still exhibit the same retry logic (but instead against only 1 upstream, which will just cause another random pod to be selected via iptables), see: https://forum.nginx.org/read.php?2,152071,152136#msg-152136

I'm personally not too worried about adding kube-proxy into the mix as that's exactly what's used in the case of a lot of external load balancers (which use nodePort), but I'll let you guys decide as you've definitely got more experience than me.

I don't mind providing support on this one.

@chrismoos
Copy link
Contributor Author

@aledbf Updated documentation with warning about sticky sessions.

@aledbf
Copy link
Member

aledbf commented Jul 18, 2017

@chrismoos one small change. Add a mention that in case of errors, no retries will be made because there is only one server in the nginx upstream.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 44.026% when pulling 525570eb3f6f63ed04614865b108b7db75c4dd06 on chrismoos:service_upstream into bfd60ac on kubernetes:master.

@chrismoos
Copy link
Contributor Author

@aledbf Done. Documentation has been updated w/ those issues pointed out.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 44.04% when pulling 666bcca on chrismoos:service_upstream into bc6e584 on kubernetes:master.

@aledbf aledbf self-assigned this Jul 19, 2017
@aledbf
Copy link
Member

aledbf commented Jul 19, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 19, 2017
@aledbf
Copy link
Member

aledbf commented Jul 19, 2017

@chrismoos thanks!

@aledbf aledbf merged commit fbb96f4 into kubernetes:master Jul 19, 2017
@michaelajr
Copy link

michaelajr commented Mar 8, 2019

When I use the annotation I still see all the Pod IPs as endpoints. Shouldn't I see the Service's ClusterIp?

I see this:

Host  Path  Backends
  ----  ----  --------
  *     *     testapp:443 (192.168.103.2:443,192.168.203.66:443,192.168.5.3:443)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants