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

Fix x-forwarded-prefix annotation #3786

Merged
merged 3 commits into from
Mar 31, 2019

Conversation

alexkursell
Copy link
Contributor

@alexkursell alexkursell commented Feb 20, 2019

What this PR does / why we need it: With the now mandatory use of regexps in paths when using rewrite-target, the x-forwarded-prefix annotation is broken.

Before 0.22, setting the annotation to true would set the x-forwarded-prefix header value to the path value. However, now that the path always includes a capture group, doing this no longer makes sense.

This PR changes the type of the x-forwarded-prefix annotation from bool to string, and changes things so that the value of the x-forwarded-prefix header is just equal to the value of the annotation (if the annotation value exists and is non-empty).

This is actually the behaviour that is explained by the docs (see here). My guess is that when this was changed to copying the path value into the header, the docs were never updated.

Which issue this PR fixes: fixes #3733, #3670, #3132

This would be a breaking change, so we should probably think about how migrating would work, as well as if there is a better way to do this.

@k8s-ci-robot 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 Feb 20, 2019
@aledbf aledbf added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Feb 20, 2019
@aledbf
Copy link
Member

aledbf commented Mar 20, 2019

so we should probably think about how migrating would work, as well as if there is a better way to do this.

@alexkursell can we use the new lint plugin feature to also provide examples of migration?

@alexkursell
Copy link
Contributor Author

alexkursell commented Mar 21, 2019

so we should probably think about how migrating would work, as well as if there is a better way to do this.

@alexkursell can we use the new lint plugin feature to also provide examples of migration?

@aledbf We can add a lint that warns on the use of x-forwarded-prefix with boolean values that links to this issue. Once the lint command is merged I'll add the lint to this PR.

@alexkursell
Copy link
Contributor Author

@aledbf I've added a lint for this change:

✗ example-ingress1
  - The x-forwarded-prefix annotation value is a boolean instead of a string
      Lint added for version 0.24.0
      https://github.com/kubernetes/ingress-nginx/issues/3786

@ElvinEfendi
Copy link
Member

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 31, 2019
@aledbf
Copy link
Member

aledbf commented Mar 31, 2019

/lgtm

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

aledbf commented Mar 31, 2019

@alexkursell thanks!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, alexkursell, ElvinEfendi

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:

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

@aledbf aledbf removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Mar 31, 2019
@k8s-ci-robot k8s-ci-robot merged commit b87cc5a into kubernetes:master Mar 31, 2019
@alexkursell alexkursell deleted the rewrite-x-forwarded-prefix branch April 1, 2019 14:10
@Sudharma
Copy link

Sudharma commented Apr 9, 2019

if I have ingress with path as below (with re-write path nginx.ingress.kubernetes.io/rewrite-target: /$1) and with x-forward-prefix annotation as true. For my service with path /path/api shall result in the correct forward prefix /path/api instead of /path/?(.*) if I am correct

paths:
         - path: path/?(.*)
           backend:
             serviceName: test 
             servicePort:  8086 

@alexkursell
Copy link
Contributor Author

alexkursell commented Apr 9, 2019

@Sudharma with this change the x-forwarded-prefix header is just set to whatever the annotation value is. So if you have nginx.ingress.kubernetes.io/x-forwarded-prefix: /foo/bar then the value of the header would always be /foo/bar, for any path that you access via that ingress.

@Sudharma
Copy link

Sudharma commented Apr 9, 2019

well but thats more hard-coded, Is it not possible to have it /foo/$1/ which is like rewrite? because If I have multiple backends and each has its own angular app and would want to have the suffix accordingly.

@alexkursell
Copy link
Contributor Author

Is it not possible to have it /foo/$1/ which is like rewrite?

This is not possible right now. If you need a different header value for different backends you will need to create different ingresses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rewrite-target with new 0.22 syntax sets x-forwarded-prefix header with unexpected capture group suffix
5 participants