Skip to content

Conversation

@mlibbey
Copy link
Contributor

@mlibbey mlibbey commented Aug 30, 2019

In ATS <=7, $f was the original host header. It isn't anymore.

In ATS <=7, $f was the original host header. It isn't anymore.
@zwoop
Copy link
Contributor

zwoop commented Aug 30, 2019

$f ? Regardless, I'm not sure we should change the docs, before understanding why this was changed.

@mlibbey
Copy link
Contributor Author

mlibbey commented Aug 31, 2019

This is the behavior of 5 released versions -- shouldn't we document its actual behavior? (I'd be fine with reverting this if our 8 release (8.0.7) finds and changes it back to the pre-8 behavior.

@bryancall
Copy link
Contributor

[approve ci clang-format]

Copy link
Contributor

@zwoop zwoop left a comment

Choose a reason for hiding this comment

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

Yeh, we "broke" this behavior... Remapping now happens before plugins are called.

@mlibbey mlibbey merged commit 9b1511b into apache:master Sep 12, 2019
@mlibbey mlibbey deleted the regexremapfix branch September 12, 2019 21:48
@zwoop
Copy link
Contributor

zwoop commented Sep 13, 2019

Cherry-picked to 8.0.x

@zwoop zwoop added this to the 8.0.6 milestone Sep 13, 2019
@gtenev
Copy link
Contributor

gtenev commented Sep 13, 2019

@mlibbey, @zwoop, could not look into this fast enough to squeeze in my twopence.

Plugin execution behavior changed in core after PR #4964 according to what was discussed on dev mailing list about "rewriting before instead of after only the first plugin (original behavior) or after all plugins are run (unsuccessful attempt to fix)".

The documentation for the old 5 releases was not correct even before PR #4964. It was correct only if the regex_remap plugin was the first plugin in the remap rule where it would get the original (source) request. If the plugin was 2nd in the remap rule $h would have returned the target hostname even for those old releases. This is the inconsistency PR #4964 fixes.

After PR #4964 the regex_remap behavior changed for the case when regex_remap is the first plugin but not for the rest of the cases. If we want to keep the old documentation true the plugin would need to be fixed (I already fixed a couple of other plugins at the time of PR #4964 ). The new behavior makes sense to me too so we can just update the docs in this case.

I also have a comment about the wording in this change. Instead of host as used in the "to" portion of the remap rule I would use something like the target or remapped request host header since it can be confused with $t which is the "to" portion of the remap rule in remap.config file and does not come from the target (remapped) request which can be different from what is written in the file itself (similarly it can be confused with $f if we decide to fix the plugin so $h would return the original request's host header)

HTH! Please let me know if more information needed!

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