Add annotations for proxy_pass customization #2343

Capitrium · 2018-04-13T14:05:02Z

What this PR does / why we need it:

Integration with the linkerd service mesh requires customization of the proxy_pass address in the nginx template. This PR adds the following annotations to allow users to customize the proxy_pass address:

nginx.ingress.kubernetes.io/proxy-pass-address: string
nginx.ingress.kubernetes.io/proxy-pass-port: string
nginx.ingress.kubernetes.io/proxy-to-local-node: bool

The proxy-pass-address and proxy-pass-port annotations allow users to set proxy_pass to a linkerd address, so that traffic is passed to the service mesh as in the basic nginx-linkerd example.

If proxy-to-local-node is true, the proxy_pass address will be set to the value parsed by os.Getenv("NODE_NAME"), which can be set to the name of the node using the downward API. This is useful when running linkerd as a daemonset, ensuring that traffic reaching an nginx ingress controller pod will be forwarded to the linkerd pod running on the same node; when using linkerd in a linker-to-linker configuration with TLS enabled, this ensures traffic does not leave the node unencrypted.

antoineco · 2018-04-17T08:01:54Z

internal/ingress/annotations/annotations.go

 	RateLimit            ratelimit.Config
 	Redirect             redirect.Config
 	Rewrite              rewrite.Config
 	SecureUpstream       secureupstream.Config
 	ServerSnippet        string
 	ServiceUpstream      bool
+	ServiceNameAsVhost   bool


Instead of introducing a new annotation type for service-name-as-vhost, could it somehow be consolidated with upstream-vhost?

It could - I'm not sure whether you mean adding the service-name-as-host annotation onto the existing upstreamVhost type using a Config struct (the same way other multi-annotation types are handled), or simply removing the service-name-as-vhost annotation and replacing {{ if $location.ServiceUpstreamAsVhost }} with something like {{ if (and (not (empty $location.UpstreamVhost)) (not (empty $location.ProxyPass.Address))) }} in the nginx template to replicate the functionality, but either one is doable.

I didn't have a very precise idea in mind and was rather opening the discussion to try improving the customization of the Host header (if possible) right away. I believe there is a better way than "if, elsif, else" to approach this.

I can roughly imagine something similar to the affinity feature, where you select a mode (eg. affinity: cookie) and configure it via fine-grained options (eg. session-cookie-name: jsessionid).

Here the mode could be an enum of preserve (preserve request Host, the default), service (what you implemented) or custom (the equivalent of the current upstream-vhost annotation).

@aledbf do you have an opinion on that?

@antoineco Turns out I actually don't need the service-name-as-vhost annotation, as it's easier to integrate with linkerd by adding a custom header (via custom nginx template or configuration snippet) and then having linkerd route on that instead of trying to change the Host header.

antoineco · 2018-04-18T16:40:55Z

internal/ingress/annotations/proxypass/main.go

+	ProxyToLocalNode bool `json:"proxyToLocalNode"`
+}
+
+// Equal tests for equality between two Redirect types


"Redirect" -> "proxyPass config"

antoineco · 2018-04-18T16:45:35Z

rootfs/etc/nginx/template/nginx.tmpl

@@ -1059,7 +1059,9 @@ stream {
            proxy_set_header       X-Service-Name     $service_name;
            {{ end }}

-            {{ if not (empty $location.Backend) }}
+            {{ if (or (not (empty $location.ProxyPass.Address)) (not (empty $location.ProxyPass.Port))) }}


empty is optional for primitive types -> if or $x $y

antoineco · 2018-04-18T20:27:41Z

internal/ingress/controller/controller.go

@@ -481,6 +482,7 @@ func (n *NGINXController) getBackendServers(ingresses []*extensions.Ingress) ([]
 						CorsConfig:           anns.CorsConfig,
 						ExternalAuth:         anns.ExternalAuth,
 						Proxy:                anns.Proxy,
+						ProxyPass:			  anns.ProxyPass,


Is this indentation a GitHub glitch or did you forget running gofmt?

Whoops - forgot to run gofmt, as I'm used to running it via make 😛

antoineco · 2018-04-18T20:56:10Z

A squash wouldn't hurt and you will have to run make code-generator because the template has changed, otherwise LGTM 👍

antoineco · 2018-04-19T06:18:30Z

Oops, looks like the changes to nginx.conf.tmpl are gone 😨

antoineco

Please add corresponding changes to the tmpl file and run make code-generator.

antoineco · 2018-04-19T11:37:58Z

internal/ingress/controller/template/template.go

@@ -404,6 +404,14 @@ func buildProxyPass(host string, b interface{}, loc interface{}, dynamicConfigur
 		}
 	}

+	if location.ProxyPass.Address != "" || location.ProxyPass.Port != "" {


Would you ever use a port alone?

I figured that the address could be omitted if ProxyToLocalNode was set along with the port.

Capitrium · 2018-04-19T13:23:03Z

@antoineco I realized last night right before squashing that buildProxyPass also sets some config values for the rewrite annotations, so the proxy-pass annotations were breaking any rewrite annotations that were set on the same ingress when that check was done directly in the template.

Moving that logic into the buildProxyPass function allows the proxy-pass and rewrite annotations to work together, and also removes the need to run make code-generator since the template has no longer changed. Sorry for the confusion!

antoineco · 2018-04-20T17:50:23Z

@Capitrium tests are failing, can you try rebasing on master?

k8s-ci-robot · 2018-04-20T18:06:11Z

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.

antoineco · 2018-04-20T18:11:32Z

@Capitrium I think an actual git rebase master would make your life easier 😄
(you probably have to git reset --hard <previous-commit-sha> first, check your git reflog)

Capitrium · 2018-04-20T18:24:33Z

@antoineco When you say <previous-commit-sha>, is that the sha of the last commit before the branch was opened, the original added proxy-pass override annotations commit, or the commit from before I ran the git pull --rebase upstream master that originally screwed up the branch? Not entirely sure what I've done to the history here...

antoineco · 2018-04-20T18:29:15Z

The one before the screwup, in theory this is the second entry from the top in your reflog.

Capitrium · 2018-04-20T18:32:46Z

Ok thanks - think I've fixed it now 😆

ElvinEfendi · 2018-04-20T18:35:33Z

@Capitrium could you also document the new annotations in https://github.com/kubernetes/ingress-nginx/blob/master/docs/user-guide/annotations.md?

codecov-io · 2018-04-20T18:40:30Z

Codecov Report

Merging #2343 into master will decrease coverage by <.01%.
The diff coverage is 41.17%.

@@            Coverage Diff             @@
##           master    #2343      +/-   ##
==========================================
- Coverage   41.61%   41.61%   -0.01%     
==========================================
  Files          74       75       +1     
  Lines        5291     5325      +34     
==========================================
+ Hits         2202     2216      +14     
- Misses       2792     2805      +13     
- Partials      297      304       +7

Impacted Files	Coverage Δ
internal/ingress/controller/template/template.go	`64.69% <0%> (-0.84%)`	⬇️
internal/ingress/controller/controller.go	`2.23% <0%> (-0.01%)`	⬇️
internal/ingress/types_equals.go	`12.41% <0%> (-0.09%)`	⬇️
internal/ingress/annotations/annotations.go	`80.95% <100%> (+0.3%)`	⬆️
internal/ingress/annotations/proxypass/main.go	`56.52% <56.52%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52e7302...45ca274. Read the comment docs.

Capitrium · 2018-04-20T19:51:45Z

@ElvinEfendi docs are updated now.
@antoineco a few of the e2e tests are failing now, any ideas on what I can do to fix that? Doesn't seem like there are any other changes to rebase in from master.

ElvinEfendi · 2018-04-21T03:56:55Z

@Capitrium #2395 should help with test flakiness

antoineco · 2018-04-23T19:24:32Z

upstream-default-server-http is just a internal upstream name, it doesn't exist, in the background nginx effectively proxies to l5d.linkerd.svc.cluster.local:4140 if you make it proxy to the l5d.linkerd Service (and not an isolated service Endpoint). There is already an annotation for it, it's called service-upstream.
By default ingress-nginx preserves the Host header. The only reason why it didn't work in your test is because you set upstream-vhost, but you shall not.

I think I have to pass that issue to someone else, because I'm obviously missing an important point here.

antoineco · 2018-04-23T19:25:37Z

/assign @aledbf
(sorry!)

aledbf · 2018-04-23T20:34:14Z

@Capitrium tomorrow I will take a look at this PR.

This change should not be required. We already have users running Istio just adding the annotations nginx.ingress.kubernetes.io/service-upstream: "true" and
nginx.ingress.kubernetes.io/upstream-vhost: svc.ns.svc.cluster.local.
You should not require to point the upstream itself to the mesh itself.

Capitrium · 2018-04-23T21:28:35Z

@antoineco No worries, I appreciate the effort!
@aledbf Thanks for your help on this. The nginx.ingress.kubernetes.io/service-upstream and nginx.ingress.kubernetes.io/upstream-vhost do seem to work, but only if the backing service does not require rewrites. Consider the following ingress as an example:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  annotations:
    kubernetes.io/ingress.class: nginx
    nginx.ingress.kubernetes.io/auth-signin: https://$host/oauth2/sign_in
    nginx.ingress.kubernetes.io/auth-url: https://$host/oauth2/auth
    nginx.ingress.kubernetes.io/configuration-snippet: |
      proxy_set_header X-Linkerd-Host '$service_name.$namespace';
      more_clear_input_headers 'l5d-ctx-*' 'l5d-dtab' 'l5d-sample';
    nginx.ingress.kubernetes.io/rewrite-target: /
    nginx.ingress.kubernetes.io/service-upstream: "true"
    nginx.ingress.kubernetes.io/upstream-vhost: l5d.linkerd.svc.cluster.local:4140
  name: server
  namespace: default
spec:
  rules:
  - host: server.testing.example.com
    http:
      paths:
      - backend:
          serviceName: test-server
          servicePort: http
        path: /
      - backend:
          serviceName: l5d-tcp-admin
          servicePort: http
        path: /l5d
      - backend:
          serviceName: echoserver
          servicePort: http
        path: /test
  tls:
  - hosts:
    - server.testing.example.com
    secretName: server-tls

Requests to the server.testing.example.com/l5d/metrics endpoint results in a 504 Gateway Timeout.
If we use the proxy-pass annotations added in this PR instead of the existing service-upstream and upstream-vhost annotations, the requests succeed.

Additionally, using upstream-vhost breaks authentication via oauth2_proxy, with requests to the authenticating service (which in my case is Google) returning a 400 Invalid Request; not sure if that's a known issue with combining the upstream-vhost and auth-signin/auth-url annotations.

antoineco · 2018-04-24T07:33:59Z

nginx.ingress.kubernetes.io/service-upstream and nginx.ingress.kubernetes.io/upstream-vhost do seem to work, but only if the backing service does not require rewrites

That's a very interesting point we may want to look into.

Regarding the PR itself, excluding the absence of E2E tests it looks good to me.

k8s-ci-robot · 2018-05-01T13:11:50Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Capitrium
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: aledbf

Assign the PR to them by writing /assign @aledbf in a comment when ready.

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

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

Capitrium · 2018-05-01T17:28:14Z

/retest

Edit: guess that didn't trigger a retest... 😛The dynamic configuration e2e tests seem a little flaky, I've run them several times on my local minikube env and they all usually pass but I've seen the odd failure/flake reported.

Capitrium · 2018-05-02T15:01:49Z

@aledbf e2e tests have been added and the build is passing, I think the only thing left is to determine whether this change is actually needed - I believe that it is, given the issues that I've previously mentioned when using rewrites with the service-upstream/upstream-vhost annotations, and when using upstream-vhost with the auth-signin/auth-url annotations.

Capitrium · 2018-06-01T19:26:57Z

another month, another merge, another set of build errors...

I have the proxy_pass e2e tests passing when running locally, but there are some errors on other tests, and I had to remove the --publish-service flag from the test ingress controller manifest to get things running in minikube (on OSX). Unfortunately I don't have time to dig further into those e2e-test errors at the moment, but I'm confident they're caused by a problem with the test harness as opposed to the actual tests.

@aledbf friendly ping: would love to get this looked at again soon if possible! I've also spoken to the Linkerd folks, and they've indicated that Istio and Linkerd are substantially different and agree that the service-upstream/upstream-vhost are not necessarily a one-size-fits-all solution for integrating nginx with any service mesh.

aledbf · 2018-06-04T14:03:33Z

... are not necessarily a one-size-fits-all solution for integrating nginx with any service mesh.

That's a problem. First, we cannot start adding code for all the service meshes available but more importantly, it should not be required to specify any special configuration to the mesh. From my point of view that removes any benefit we can get from it and makes things unnecessarily complex.

I believe that it is, given the issues that I've previously mentioned when using rewrites with the service-upstream/upstream-vhost annotations, and when using upstream-vhost with the auth-signin/auth-url annotations.

Please try to see this issue from my point of view. If we add this (in the current state) we need to add a mesh in the e2e tests and create scenarios only for this. Sadly the ingress-nginx is a community effort project with personal time from everyone involved, there is no full-time people working on it so this cannot be done.

Capitrium · 2018-06-04T14:41:41Z

... are not necessarily a one-size-fits-all solution for integrating nginx with any service mesh.

That's a problem. First, we cannot start adding code for all the service meshes available but more importantly, it should not be required to specify any special configuration to the mesh. From my point of view that removes any benefit we can get from it and makes things unnecessarily complex.

While I disagree with you here, I'm not sure this is entirely relevant. I feel like we're getting away from what this PR actually does: it allows users to set a custom proxy_pass value using ingress annotations. I don't see a need to be concerned with why users may need that, or how it makes things "unnecessarily complex" given the vast array of customization annotations already present.

I believe that it is, given the issues that I've previously mentioned when using rewrites with the service-upstream/upstream-vhost annotations, and when using upstream-vhost with the auth-signin/auth-url annotations.

Please try to see this issue from my point of view. If we add this (in the current state) we need to add a mesh in the e2e tests and create scenarios only for this. Sadly the ingress-nginx is a community effort project with personal time from everyone involved, there is no full-time people working on it so this cannot be done.

Sorry, I'm a bit confused here: why do you think we need to add a service mesh to the e2e framework to allow users to set proxy_pass with some annotations? The reason I require that functionality may be to get linkerd working, but I don't see why we'd need to add a service mesh to test that we can set proxy_pass. The controller also currently supports Istio, yet I don't see Istio referenced anywhere in the current e2e framework. Can you please clarify this point?

TLDR: Don't worry about Linkerd or supporting other service meshes 😄 - let's reduce the use case here to setting a custom proxy_pass dynamically per-ingress. Is there a security or technical concern with allowing that?

Capitrium · 2018-06-06T14:15:00Z

@aledbf Not sure if you saw my follow-up questions above, just summarizing them here:

Why would we need to add a service mesh to the e2e test framework in order to test setting a custom proxy_pass value?
Is there a technical or security concern with allowing users to set a custom proxy_pass value?

aledbf · 2018-06-06T14:41:45Z

Is there a technical or security concern with allowing users to set a custom proxy_pass value?

Yes, basically you are reimplementing Service type=ExternalName using annotations in Ingress

Please check this example #629 (comment)

Capitrium · 2018-06-06T18:33:38Z

@aledbf thanks for the link - I can see how there is overlap between being able to set an arbitrary proxy_pass and the existing support for ExternalName services.

However, it doesn't look like a service with an ExternalName of $NODE_NAME would be supported (or make any sense), and what I really need from the controller is the ability to send traffic to an upstream service located on the same node without modifying the request - any ideas on the best way to move forward here? Maybe allowing setting proxy_pass to $NODE_NAME without allowing arbitrary endpoints? (although validation of $NODE_NAME becomes a problem...)

aledbf · 2018-06-06T18:52:46Z

However, it doesn't look like a service with an ExternalName of $NODE_NAME would be supported

I think we could add support for externalTrafficPolicy: Local in the controller but to be honest it makes no sense to assume there's an upstream server available in the node

Capitrium · 2018-06-06T19:00:28Z

it makes no sense to assume there's an upstream server available in the node

Unless the upstream server is deployed as a daemonset, right?

aledbf · 2018-06-15T04:29:25Z

Unless the upstream server is deployed as a daemonset, right?

Right. Just in case the ingress controller is not aware of how a service deployed

fejta-bot · 2018-09-13T05:20:42Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2018-10-13T05:44:01Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

fejta-bot · 2018-11-12T06:31:03Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot · 2018-11-12T06:31:10Z

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

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 Apr 13, 2018

antoineco reviewed Apr 17, 2018

View reviewed changes

Capitrium changed the title ~~Add annotations for proxy_pass customization and per-service Host headers~~ Add annotations for proxy_pass customization Apr 18, 2018

antoineco suggested changes Apr 18, 2018

View reviewed changes

antoineco reviewed Apr 18, 2018

View reviewed changes

antoineco approved these changes Apr 18, 2018

View reviewed changes

antoineco suggested changes Apr 19, 2018

View reviewed changes

aledbf assigned antoineco Apr 19, 2018

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

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 20, 2018

k8s-ci-robot assigned aledbf Apr 23, 2018

aledbf unassigned antoineco Apr 23, 2018

Capitrium added 3 commits June 1, 2018 13:30

added proxy_pass override annotations

77cdece

document proxy_pass override annotations

8ff4e69

added e2e tests for proxy_pass annotations

206aa6d

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 13, 2018

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 13, 2018

k8s-ci-robot closed this Nov 12, 2018

Add annotations for proxy_pass customization #2343

Add annotations for proxy_pass customization #2343

Conversation

Capitrium commented Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoineco Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoineco Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoineco commented Apr 18, 2018

antoineco commented Apr 19, 2018

antoineco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Capitrium commented Apr 19, 2018

antoineco commented Apr 20, 2018

k8s-ci-robot commented Apr 20, 2018

antoineco commented Apr 20, 2018 • edited Loading

Capitrium commented Apr 20, 2018

antoineco commented Apr 20, 2018

Capitrium commented Apr 20, 2018

ElvinEfendi commented Apr 20, 2018

codecov-io commented Apr 20, 2018 • edited Loading

Codecov Report

Capitrium commented Apr 20, 2018

ElvinEfendi commented Apr 21, 2018

antoineco commented Apr 23, 2018 • edited Loading

antoineco commented Apr 23, 2018

aledbf commented Apr 23, 2018

Capitrium commented Apr 23, 2018

antoineco commented Apr 24, 2018

k8s-ci-robot commented May 1, 2018

Capitrium commented May 1, 2018 • edited Loading

Capitrium commented May 2, 2018

Capitrium commented Jun 1, 2018

aledbf commented Jun 4, 2018

Capitrium commented Jun 4, 2018 • edited Loading

Capitrium commented Jun 6, 2018

aledbf commented Jun 6, 2018 • edited Loading

Capitrium commented Jun 6, 2018

aledbf commented Jun 6, 2018

Capitrium commented Jun 6, 2018

aledbf commented Jun 15, 2018

fejta-bot commented Sep 13, 2018

fejta-bot commented Oct 13, 2018

fejta-bot commented Nov 12, 2018

k8s-ci-robot commented Nov 12, 2018

Capitrium commented Apr 13, 2018 •

edited

Loading

antoineco Apr 17, 2018 •

edited

Loading

antoineco Apr 18, 2018 •

edited

Loading

antoineco commented Apr 20, 2018 •

edited

Loading

codecov-io commented Apr 20, 2018 •

edited

Loading

antoineco commented Apr 23, 2018 •

edited

Loading

Capitrium commented May 1, 2018 •

edited

Loading

Capitrium commented Jun 4, 2018 •

edited

Loading

aledbf commented Jun 6, 2018 •

edited

Loading