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

Generalize Rewrite Block Creation and Deprecate AddBaseUrl (not backwards compatible) #3174

Merged
merged 1 commit into from Jan 2, 2019

Conversation

Projects
@zrdaley
Copy link
Contributor

zrdaley commented Oct 3, 2018

What this PR does / why we need it:

When the rewrite-target annotation is used and the written path uses regex and does not end in / (ex. foo/bar/.+), rewrite does not work as expected. This provides a regex check in BuildProxyPass and uses a custom rewrite block if the use-regex annotation is true for a location.

UPDATE
In addition to the above, this PR deprecates the AddBaseUrl annotation.

Problem

An ingress is created with use-regex="true" and rewrite-target="/new" and the path /foo/bar/.+.

This results in the following rewrite blocks for that location:

rewrite (?i)/foo/bar/.+/(.*) /new/$1 break;
rewrite (?i)/foo/bar/.+$ /new break;
...

Therefore a request to /foo/bar/bar will go to /new instead of /new/bar as a user may expect.

Solution

Check if a use-regex is true for a location when building the proxy pass.

An ingress is created with use-regex="true" and rewrite-target="/new/$1" and the path /foo/bar/(.+).

This results in the following rewrite blocks for that location:

rewrite (?i)/foo/bar/(.+) /new/$1 break;
...

Now a request to /foo/bar/bar will go to /new/bar.

@aledbf

This comment has been minimized.

Copy link
Member

aledbf commented Oct 4, 2018

/lgtm
/hold

@ElvinEfendi

This comment has been minimized.

Copy link
Member

ElvinEfendi commented Oct 4, 2018

The way I understand this is it enforces using regexp group references explicitly in rewrite-target annotation when use-regex is true.

So given an ingress with regexp path enabled and rewrite-target is "/new/$1". And there are two paths
foo/bar/(.+)
and
foo/baz

rewrite (?i)/foo/bar/(.+) /new/$1 break;
and
rewrite (?i)/foo/baz /new/$1 break;

will be generated accordingly. This means foo/baz/anything will be rewritten to /new/, which means breaking existing expectations.

I don't know what's the best solution here, maybe we should stop trying to make the controller smart about generating the correct rewrite syntax and let the user take are of that. Because to me it seems like impossible to find a common ground. Therefore it would make sense to let user do whatever they want. That would also simplify controller logic. Nginx rewrite directive has two required arguments:

rewrite regex replacement [flag];

So given that whenever rewrite target is used we enforce regepx path matching, I'd delete all the existing logic where we try to come up with correct arguments to rewrite and let the user take care of it the way she wants.

For example the simplest case when you want to rewrite foo/bar to /new you would need to set rewrite-target to /new/$1 and configure the path as (?i)/foo/bar/(.*). Then the controller would take the path and target as is and generate

rewrite (?i)/foo/bar/(.*) /new/$1 break;

this is obviously not backward compatible, but I really dislike the complexity around the current solution, and this solution will make it simpler to reason about.

@zrdaley

This comment has been minimized.

Copy link
Contributor Author

zrdaley commented Oct 4, 2018

@ElvinEfendi I completely agree.

Giving the user control of whether or not to append parameters to rewritten paths in all cases makes way more sense. Defaulting to enforcing this policy, especially now that regex is enabled, seems counterintuitive.

@zrdaley zrdaley force-pushed the Shopify:rewrite-regex branch from 2b46733 to 51552fa Oct 4, 2018

@k8s-ci-robot k8s-ci-robot added size/L and removed lgtm size/M labels Oct 4, 2018

@zrdaley zrdaley force-pushed the Shopify:rewrite-regex branch from c6de981 to 726fbc2 Oct 4, 2018

@aledbf

This comment has been minimized.

Copy link
Member

aledbf commented Oct 8, 2018

@zrdaley please squash the commits

@aledbf aledbf added this to In Progress in 0.21.0 Oct 8, 2018

@@ -139,14 +138,13 @@ proxy_pass http://upstream-name;
false,
false,
false,
true},
false},

This comment has been minimized.

Copy link
@ElvinEfendi

ElvinEfendi Oct 8, 2018

Member

Why is this false, when there's rewrite enforceRegex is always true, no?

@ElvinEfendi

This comment has been minimized.

Copy link
Member

ElvinEfendi commented Oct 8, 2018

/hold

@zrdaley please update PR title and description accordingly now that we pivoted.

--

This change is not backward compatible and it is going to break every single rewrite configuration.
We should find some way to at least not fail silently (this is what's going to happen the PR as is).

The logic at https://github.com/kubernetes/ingress-nginx/pull/3174/files#diff-652deabba49a2a87d4c5a79700dfa1e3R331 does not make much sense with regexp path support and specially after this PR. With this PR users will have to have something like (.*) in their path to do rewrite with $1, which is going to render (?<baseuri>.*) useless. IMO we should completely drop support for AddBaseURL, the app has every knowledge to make that decision.

This test https://github.com/kubernetes/ingress-nginx/pull/3174/files#diff-780db26d3e78669defcf119bfb8a9b64R43 now is not so useful. We should make sure it tests that path suffix (/something/i-am-suffix) gets added to the target path prefix (/) when proxying.

@aledbf

This comment has been minimized.

Copy link
Member

aledbf commented Oct 11, 2018

@zrdaley friendly ping

@zrdaley zrdaley changed the title Use custom rewrite block when regex is used Generalize Rewrite Block Creation (not backwards compatible) Oct 16, 2018

@zrdaley

This comment has been minimized.

Copy link
Contributor Author

zrdaley commented Oct 16, 2018

@ElvinEfendi I agree with the deprecation of AddBaseURL however, I think that maybe this should be addressed in a separate PR, following this one.

Also, is there an official way to notify users of backwards compatibility issues or will the change of name above be sufficient?

I am just going to remove this test https://github.com/kubernetes/ingress-nginx/pull/3174/files#diff-780db26d3e78669defcf119bfb8a9b64R43. This test https://github.com/kubernetes/ingress-nginx/pull/3174/files?utf8=%E2%9C%93&diff=unified#diff-780db26d3e78669defcf119bfb8a9b64R257 cover's suffix appending for both the index and all other path cases since there is no longer a special case for the index path.

@zrdaley zrdaley force-pushed the Shopify:rewrite-regex branch 2 times, most recently from 47f6cb1 to 4a6a6f5 Oct 16, 2018

@zrdaley zrdaley changed the title Generalize Rewrite Block Creation (not backwards compatible) [WIP] Generalize Rewrite Block Creation (not backwards compatible) Oct 16, 2018

@zrdaley zrdaley force-pushed the Shopify:rewrite-regex branch from 4a6a6f5 to 9fa7964 Oct 16, 2018

@zrdaley zrdaley changed the title [WIP] Generalize Rewrite Block Creation (not backwards compatible) Generalize Rewrite Block Creation (not backwards compatible) Oct 16, 2018

@mxey

This comment has been minimized.

Copy link
Contributor

mxey commented Jan 17, 2019

I am trying to figure out how to migrate my configuration for this. AFAICT, using regex in rewrite-target does not work in v0.21.0 even when enabling use-regex. How can I configure my Ingress resources to work with both v0.21.0 and v0.22.0? Changing them all the same time is not practical, because they belong to different applications in different repositories.

I was already surprised by this change, because the release notes do not mention breaking changes to rewrite-target.

@ElvinEfendi

This comment has been minimized.

Copy link
Member

ElvinEfendi commented Jan 17, 2019

Sorry for introducing poorly organized backward incompatible change! Right now only zero-downtime migration strategy I can think of is:

  1. Deploy version 0.22.0 in addition to your existing running ingress-nginx, where it watched to different ingress class than the existing one (check for --ingress-class
    in https://kubernetes.github.io/ingress-nginx/user-guide/cli-arguments/).

  2. Create another ingress resource in your app with the class that new ingress-nginx instance watches. This ingress manifest should have the correct changes applied to rewrite-target annotation and paths.

  3. Update DNS setting for the relevant domains to point to the IP address of the new ingress-nginx. (You can probably do other things here to avoid DNS changes. One things comes to my mind is to edit the service of old ingress-nginx and point it to new ingress-nginx pods).

  4. If you are OK with the new ingress class you can kill old ingress-nginx and delete old ingress manifests, if you wanna switch back to it then continue with 5.

  5. Upgrade old ingress-nginx.

  6. Update all the ingress manifest to have correct rewrite-target and paths.

  7. Switch the traffic back to updated old ingress.

Note that I have not tried the above migration in practice.

--

I was already surprised by this change, because the release notes do not mention breaking changes to rewrite-target.

Unfortunately this has been overlooked too! I'm going to make a PR to at least correct the changelog.

@mxey

This comment has been minimized.

Copy link
Contributor

mxey commented Jan 18, 2019

@ElvinEfendi Thanks for your reply :)

Right now only zero-downtime migration strategy I can think of is

Is there any hope we can implement something to ease the migration?

@ElvinEfendi

This comment has been minimized.

Copy link
Member

ElvinEfendi commented Jan 18, 2019

UPDATE: Please check #3174 (comment) on how to do the migration.

@mxey there's no plan to do so. To clarify the situation rewrite will still happen, but the suffix of path won't be included in the new path if you upgrade to 0.22.0 without changing rewrite-target annotation.

For example given:

rewrite-target: /blog
...
path: /

Requests to /<anything>?<anything> will always be rewritten to /blog. If this is acceptable for your app for up to few minutes then I'd stage the relevant changes to do ingress-nginx upgrade and adjustment to ingress manifests and then ship them at the same time (obviously it won't be atomically at the same time this is why I said if broken rewrite is tolerable by your apps).

Otherwise I don't have any better suggestion than #3174 (comment).

@ElvinEfendi

This comment has been minimized.

Copy link
Member

ElvinEfendi commented Jan 18, 2019

Actually I think there's a way to make this migration seamless!

Until 0.22.0 controller used to generate something like:

      rewrite "(?i)/<path>/(.*)" /<value of rewrite-target annotation>/$1 break;
      rewrite "(?i)/<path>$" /value of rewrite-target annotation/ break;

We can use configuration-snippet annotation to do the same manually. Let's say you have an ingress like below:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: web
  annotations:
    kubernetes.io/ingress.class: nginx
    nginx.ingress.kubernetes.io/rewrite-target: /new
spec:
  rules:
  - host: example.com
    http:
      paths:
      - backend:
          serviceName: http-svc
          servicePort: http
         path: /old

Before upgrading to 0.22.0, you can change that ingress into:

...
    kubernetes.io/ingress.class: nginx
    nginx.ingress.kubernetes.io/use-regex: "true"
    nginx.ingress.kubernetes.io/configuration-snippet: |
      rewrite "(?i)/old/(.*)" /new/$1 break;
      rewrite "(?i)/old$" /new/ break;
spec:
...
      - backend:
          serviceName: http-svc
          servicePort: http
         path: /old/?(.*)

Now this manifest will work as expected in both 0.21.0 as well as 0.22.0.

After this change you can upgrade ingress-nginx to 0.22.0. Once upgrade is completed, you can get rid of server snippet and use rewrite-target annotation again:

...
    kubernetes.io/ingress.class: nginx

    nginx.ingress.kubernetes.io/rewrite-target: /new/$1
spec:
...
      - backend:
          serviceName: http-svc
          servicePort: http
         path: /old/?(.*)
@brichins

This comment has been minimized.

Copy link

brichins commented Jan 30, 2019

Wow! This is a very rough migration. In my circumstances I'll need to get 20+ teams with a total of 270+ ingress resources to make this change and redeploy through each environment through their own build pipelines. This quickly becomes a nightmare. Any chance of providing a backwards compatible option in 0.23 that would support the new capture group but leave the slash rewrite possible? That would give users the chance of migrating to the new syntax without the extra hassle of using a configuration-snippet.

@ElvinEfendi

This comment has been minimized.

Copy link
Member

ElvinEfendi commented Jan 30, 2019

@brichins I share your sentiments but this is something we had to do (see #3174 (comment)) and there was not really a better migration path because the change is applied to poth path and annotation value.

I realize it's not ideal but what you can do is write a script that goes through all your ingress manifests and updates them according to #3174 (comment). I'd also look and see if all your apps require zero-downtime migration.

Open to any new suggestion.

@MarkDeckert

This comment has been minimized.

Copy link

MarkDeckert commented Jan 30, 2019

@brichins makes a good point about breaking changes in nginx-ingress. The project has moved beyond toy status and is really used now in larger organizations that have a lot of moving parts and workflows that demand soft migration paths. There should be utmost effort to not simultaneously deprecate and remove features in the same version.

FWIW, I actually have an ingress (or set of ingresses that share the same hostname) where the seamless workaround oddly doesn't work on just one particular path in 0.21.0 but does work in 0.22.0. So it looks like it's going to be hard cutover on that one service.

@avik-so

This comment has been minimized.

Copy link

avik-so commented Feb 4, 2019

As much as I appreciate the work you do here, this breaking change really reduced our confidence.
There was no deprecation warnings or anything to indicate to us that there would be breaking changes in a minor upgrade.

Please respect Semversioning standards.

MAJOR version when you make incompatible API changes,
MINOR version when you add functionality in a backwards-compatible manner, and
PATCH version when you make backwards-compatible bug fixes.

@Yannic92

This comment has been minimized.

Copy link

Yannic92 commented Feb 4, 2019

I'd say it was mentioned often enough that this change wasn't ideal, but required.
We should keep in mind that the version is still a 0.x.x
As we all know, breaking changes in 0 versions are very likely.
Yes this breaking change was uncomfortable, but I've got the feeling that this thread is getting toxic because people just release their frustration here.
@ElvinEfendi did his best to help us performing an upgrade as comfortable as possible and I'm really thankful for his work.
Let's stop beating a dead horse.
If you you really want to contribute to this project, provide a PR that allows a backwards compatible migration.

To avoid misunderstandings: I'm not a comitter on this project. Just someone who thinks, we complained enough.

@avik-so

This comment has been minimized.

Copy link

avik-so commented Feb 4, 2019

My apologies, it seems my comment is in the wrong place. The biggest problem here is that the stable helm chart points to a version with breaking changes, without any warnings, or a way to choose the actual stable version. I've put a comment there.

helm/charts#10861 (comment)

@zuzzas

This comment has been minimized.

Copy link

zuzzas commented Feb 4, 2019

I am quite confused about the implications of this PR. Do I even have to worry about anything if I do not specify use-regex=true anywhere? I also have no regex constructs in my rewrite-targets. Should I be worried?

@avik-so

This comment has been minimized.

Copy link

avik-so commented Feb 5, 2019

I am quite confused about the implications of this PR. Do I even have to worry about anything if I do not specify use-regex=true anywhere? I also have no regex constructs in my rewrite-targets. Should I be worried?

We use Azure AKS, and have nginx.ingress.kubernetes.io/rewrite-target: in our helm charts. We don't have use-regex=true in our chart anywhere, but this change broke our rewrite targets.

@Yannic92

This comment has been minimized.

Copy link

Yannic92 commented Feb 5, 2019

From https://github.com/kubernetes/ingress-nginx/tree/master/docs/examples/rewrite

!!! attention Starting in Version 0.22.0, ingress definitions using the annotation nginx.ingress.kubernetes.io/rewrite-target are not backwards compatible with previous versions. In Version 0.22.0 and beyond, any substrings within the request URI that need to be passed to the rewritten path must explicitly be defined in a capture group.

If I'm not getting this wrong, this means that as soon as you use rewrite-target you need to do this with regex and capture groups.

@zuzzas

This comment has been minimized.

Copy link

zuzzas commented Feb 5, 2019

Thank you!

And another question. Should I add use-regex annotation everywhere if I want to pass a part of the request URI to rewrite, since I can't do that now without using regex capture groups? Because documentation is ambiguous on that matter.

@Yannic92

This comment has been minimized.

Copy link

Yannic92 commented Feb 5, 2019

To my understanding the use-regex annotation became useless for locations using rewrite-target. I omitted it everywhere and still used regex in the path and rewrite-target. Maybe @ElvinEfendi could clarify?

Additionally, if the rewrite-target annotation is used on any Ingress for a given host, then the case insensitive regular expression location modifier will be enforced on ALL paths for a given host regardless of what Ingress they are defined on.

from: https://github.com/kubernetes/ingress-nginx/blob/master/docs/user-guide/nginx-configuration/annotations.md#use-regex

@ElvinEfendi

This comment has been minimized.

Copy link
Member

ElvinEfendi commented Feb 5, 2019

@Yannic92 you got it all correct! When using rewrite-target annotation use-regex is redundant.

ccojocar added a commit to ccojocar/jenkins-x-versions that referenced this pull request Feb 6, 2019

chore: lock the nginx-ingress controller to version 1.1.5
It seems that nginx-ingress 0.22.0 introduced some breaking changes. This version
is used starting with chart version 1.2.0.
kubernetes/ingress-nginx#3174 (comment)
@hannes-angst

This comment has been minimized.

Copy link

hannes-angst commented Feb 10, 2019

Minor note on the given examples: /foo/?(.*) will match all /foo(.*) prefixes. Depending on the location order /footab/(.*) will not be matched if /foo?/(.*) takes precedent.

@dshakey

This comment has been minimized.

Copy link

dshakey commented Apr 6, 2019

Having issues with latest version. I'm currently on 0.19. The doc examples say

1.rewrite.bar.com/something rewrites to rewrite.bar.com/
2. rewrite.bar.com/something/ rewrites to rewrite.bar.com/
3. rewrite.bar.com/something/new rewrites to rewrite.bar.com/new

But i want the rewrite to also rewrite all uri's in my app to something/. The app works off / and will redirect to /login and then /home

  1. rewrite.bar.com/something/ rewrites to rewrite.bar.com/something/login and rewrite.bar.com/something/home

How do I keep the prefix "something" with the new changes?

Would the app-root help here?

@zrdaley

This comment has been minimized.

Copy link
Contributor Author

zrdaley commented Apr 15, 2019

How do I keep the prefix "something" with the new changes?

@dshakey you will need to include "something" within the capturing group.

i.e. change the spec.rules.host.paths.path from /something/?(.*) to/(something/.*)

@RAbraham

This comment has been minimized.

Copy link

RAbraham commented Apr 17, 2019

How do I keep the prefix "something" with the new changes?

@dshakey you will need to include "something" within the capturing group.

i.e. change the spec.rules.host.paths.path from /something/?(.*) to/(something/.*)

@zrdaley do you mean spec.rules.http.paths.path?

@dshakey

This comment has been minimized.

Copy link

dshakey commented Apr 17, 2019

I figured it out. I needed to use proxy redirect with some regex to make the location path what I needed.

nginx.ingress.kubernetes.io/proxy-redirect-from: ~*(https?://[^:]+:\d+)(/something)?(/(?:something))?(.+)$
nginx.ingress.kubernetes.io/proxy-redirect-to: $1/something$4

@RAbraham

This comment has been minimized.

Copy link

RAbraham commented Apr 17, 2019

@dshakey could you share your code change. That would be much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.