Skip to content

Conversation

@gtenev
Copy link
Contributor

@gtenev gtenev commented Feb 11, 2019

Rewriting the url before running all plugins instead of after
which would guarantee that:

  • all plugins would get the same TSRemapRequestInfo::reqiestUrl
    (first plugin in the chain would not be special)
  • all plugins would treat TSRemapRequestInfo::reqiestUrl the same
    way consistently as a remapped URL which makes the first plugin
    really not different from the rest
  • there would be a remapped URL default in case the remap rule had
    no plugins OR none of the plugins modifed the mapped URL

@gtenev gtenev added this to the 9.0.0 milestone Feb 11, 2019
@gtenev gtenev self-assigned this Feb 11, 2019
@gtenev gtenev added the WIP label Feb 11, 2019
@gtenev
Copy link
Contributor Author

gtenev commented Feb 11, 2019

This PR is related to Issue #4929, #4026, #2877 and PR #4531 and also sent an email to the dev mailing list to discuss the fix / proposal, reviving the existing email thread "[PROPOSAL] Rewrite url after running all remap plugins".

@zwoop
Copy link
Contributor

zwoop commented Feb 13, 2019

[approve ci autest]

@zwoop
Copy link
Contributor

zwoop commented Feb 13, 2019

We would need this for 8.x presumably ?

@gtenev
Copy link
Contributor Author

gtenev commented Feb 13, 2019

@zwoop this is still an incompatible change similarly to the previous attempt to fix this problem in PR #4531.

Although this PR hopes to make the plugin API more consistent from plugin developer's point of view (TSRemapRequestInfo::reqiestUrl usage specifically) it still breaks backwards compatibility (discussed in this email to the dev mailing list).

Also marked WIP since imagined we need to have an agreement on dev mailing list about "rewriting before instead of after all plugin are run".

Copy link
Member

@SolidWallOfCode SolidWallOfCode left a comment

Choose a reason for hiding this comment

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

I this makes the API more consistent, in that each plugin sees the remap results of the previous one, and treats the core as the "first" remap plugin. Otherwise, as @gtenev points out, the first remap plugin is still special in not seeing any previous remapping occur. This is philosophically more consistent with other such modifications, e.g. TLS certificates. The core does its thing, then plugins can modify that result.

@gtenev
Copy link
Contributor Author

gtenev commented Feb 21, 2019

[approve ci]

@gtenev
Copy link
Contributor Author

gtenev commented Feb 26, 2019

[approve ci]

Rewriting the url *before* running all plugins instead of *after*
which would guarantee that:
- all plugins would get the same TSRemapRequestInfo::reqiestUrl
  (first plugin in the chain would not be special)
- all plugins would treat TSRemapRequestInfo::reqiestUrl the same
  way consistently as a *remapped* URL which makes the first plugin
  really not different from the rest
- there would be a remapped URL default in case the remap rule had
  no plugins OR none of the plugins modifed the mapped URL

Also turning off url_sig and cookie_remap plugin unit-tests impacted
by this not backwards compatible change.
@gtenev
Copy link
Contributor Author

gtenev commented Feb 26, 2019

@SolidWallOfCode would you reiterate your approval? Dismissed your previous approval by pushing changes to the PR branch, disabling unit-tests for url_sig plugin (will be reenabled in PR #5073) and cookie_remap plugin (filed Issue #5073).

@SolidWallOfCode SolidWallOfCode self-requested a review February 26, 2019 20:48
@gtenev gtenev merged commit fdf7688 into apache:master Feb 26, 2019
@gtenev gtenev deleted the rewrite_url_before_running_plugins branch February 26, 2019 20:54
@bryancall
Copy link
Contributor

This is an incompatible change and can't go into 8.x

gtenev added a commit that referenced this pull request Apr 8, 2019
When the plugin does not use @pparam=pristineurl it should work
with the remapped url. The code already does this but the unit-test
still tests the signing of the pristine url in those specific use
cases. This is related to a behavior change with v9.0 where the
1st plugin gets the remapped url as a remap API requestUrl
regardless of its possition in the plugin chain (PR #4964).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants