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

[THREESCALE-852] Split upstream policy phases so it can match the original path #690

Merged
merged 6 commits into from
May 9, 2018

Conversation

davidor
Copy link
Contributor

@davidor davidor commented May 4, 2018

This PR solves the same problem that #689 solves but in a different way.

This PR modifies the Upstream policy so the matching of rules is done in the rewrite phase instead of the content one. This allows us to combine this policy with the URL rewriting one. Before this change, the upstream policy ran on the content phase, whereas the URL rewriting one ran on the rewrite phase. This means that the upstream policy always took into account the rewritten path instead of the original one, regardless of the position of the policies in the chain.

I opened a new PR instead of fixing the previous one because I think @nmasse-itix might need it until we merge this.

Ref: https://issues.jboss.org/browse/THREESCALE-852

This way this policy will be easier to combine with others, for example
the URL rewriting policy.

Before this change, the upstream policy ran on the content phase,
whereas the URL rewriting one ran on the rewrite phase. This means
that the upstream policy always took into account the rewritten path
instead of the original one, regardless of the position of the policies
in the chain. This commit fixes the issue.
@davidor davidor force-pushed the split-upstream-policy-phases branch from 53756c2 to 7889cba Compare May 4, 2018 10:21
@davidor davidor requested review from mikz and mayorova May 4, 2018 10:21
context.upstream_changed = true
return change_upstream(rule.url)
elseif ngx.config.debug then
context.new_upstream = rule.url
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point we should introduce policy local context per request for these variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes 👍

@mikz mikz merged commit 0eb94ce into master May 9, 2018
@mikz mikz deleted the split-upstream-policy-phases branch May 9, 2018 07:58
mikz added a commit that referenced this pull request May 11, 2018
[THREESCALE-852] Split upstream policy phases so it can match the original path
@mikz mikz mentioned this pull request May 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants