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

POC: Setting upstream vhost for nginx. #719

Merged
merged 1 commit into from Sep 10, 2017
Merged

POC: Setting upstream vhost for nginx. #719

merged 1 commit into from Sep 10, 2017

Conversation

mtanski
Copy link
Contributor

@mtanski mtanski commented May 15, 2017

In the current k8 ingress there's no way to control "Host" header sent to upstream server; it's always the configured vhost.

It's desirable to support custom Host header when proxying for a number of cases. One such case is forwarding to an Service that points to an external host that expect a host paramater (like AWS S3 buckets). There's a number of others.

By default nginx set Host to the upstreams domain name (and not the one passed in) like K8 always forces. This provided an escape hatch to this behavior.

This is a proof of concept PR for review, feedback. I can incorporate all of that; I just need to make sure I'm going down the right path.

@k8s-ci-robot
Copy link
Contributor

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://github.com/kubernetes/kubernetes/wiki/CLA-FAQ 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.


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.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label May 15, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@mtanski
Copy link
Contributor Author

mtanski commented May 18, 2017

Stuck in getting the CLA signed due to LF CLA issues. But in the mean time I would appreciate the review of the work/direction.

@aledbf
Copy link
Member

aledbf commented May 25, 2017

@mtanski apologies for the delay. This feature makes sense in many scenarios.
What you are doing is ok. Please finish the annotation parser so we can test this

@mtanski
Copy link
Contributor Author

mtanski commented May 26, 2017

@aledbf I'm going to be away for a week; so I'll make those changes when I'm back and solicit feedback then.

@mtanski
Copy link
Contributor Author

mtanski commented Jul 20, 2017

Have signed the CLA (org & employee). This was delayed due to some LF internal issue that prevented my org for being able to sign the CLA.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 20, 2017
@mtanski
Copy link
Contributor Author

mtanski commented Aug 24, 2017

Finally had a chance to get back to this. I have rebased this against master (for ez merging) and added the missing annotation implementation file.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 25, 2017
@mtanski
Copy link
Contributor Author

mtanski commented Sep 9, 2017

@aledbf any chance to get this into upstream?

@aledbf
Copy link
Member

aledbf commented Sep 9, 2017

@mtanski please rebase

In the current k8 ingress there's no way to control "Host" header sent
to upstream server; it's always the configured vhost.

It's desirable to support custom Host header when proxying for a number
of cases. One such case is forwarding to an Service that points to an
external host that expect a host paramater (like AWS S3 buckets).
There's a number of others.

By default nginx set Host to the upstreams domain name (and not the one
passed in) like K8 always forces. This provided an escape hatch to this
behavior.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 43.688% when pulling 2120aab on adfin:upstream-vhost into 7eb2b81 on kubernetes:master.

@mtanski
Copy link
Contributor Author

mtanski commented Sep 10, 2017

@aledbf here ya go

@aledbf aledbf self-assigned this Sep 10, 2017
@aledbf
Copy link
Member

aledbf commented Sep 10, 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 Sep 10, 2017
@aledbf
Copy link
Member

aledbf commented Sep 10, 2017

@mtanski thanks!

@aledbf aledbf merged commit f647914 into kubernetes:master Sep 10, 2017
@rikatz
Copy link
Contributor

rikatz commented Oct 1, 2017

@mtanski Can we just update the docs with this new directive? Thanks!!!

@mtanski
Copy link
Contributor Author

mtanski commented Oct 1, 2017

@rikatz Yes. I will send a PR this week; most likely in the 2nd half.

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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants