Skip to content

Conversation

@gtenev
Copy link
Contributor

@gtenev gtenev commented Aug 22, 2019

In certain use-cases when calculating the prefix (the initial value
of the new cache key) we need to have the scheme, host and port in
their original form from the request URI, i.e. when hosting.config
is used the cache key is expected to contain a valid URI authority
element used for volume selection.

More details about the new parameter and its functionality can be
found in doc/admin-guide/plugins/cachekey.en.rst

@gtenev gtenev added the Plugins label Aug 22, 2019
@gtenev gtenev added this to the 10.0.0 milestone Aug 22, 2019
@gtenev gtenev requested a review from d2r August 22, 2019 22:30
@gtenev gtenev self-assigned this Aug 22, 2019
@gtenev
Copy link
Contributor Author

gtenev commented Aug 22, 2019

This patch fixes a problem found by @knutsel while using cachekey plugin and hosting.config where volume selection failed since the cachekey by design always generates a cache key in a path-like format and removes the URI scheme and authority elements in their original form (which are expected in this case).

I would like to have this PR back ported to 9.x as a "bug fix" for this broken use-case which would allow cachekey plugin to be used with hosting.config without cumbersome workarounds in the cachekey configs (i.e. like using cacheurl compatibility mode and custom delimiters, see cachekey plugin documentation for more details)

@gtenev gtenev requested review from ezelkow1 and mlibbey August 22, 2019 23:06
@gtenev
Copy link
Contributor Author

gtenev commented Aug 28, 2019

An offline chat with @ezelkow1 about his experiments with this new feature made me realize that an arbitrary combining of all prefix related parameters could also yield a valid host name in the final resulting cache key, depending on the static value provided (--static-prefix) or the success of the regex capture/replace operations (--capture-prefix and --capture-prefix-uri) requested by the config. Added a tweak to cover those cases.

@bryancall
Copy link
Contributor

[approve ci autest]

ezelkow1
ezelkow1 previously approved these changes Aug 28, 2019
Copy link
Member

@ezelkow1 ezelkow1 left a comment

Choose a reason for hiding this comment

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

Looks good, tested here with various scenarios. After the last fix push it seems to work as advertised

In certain use-cases when calculating the prefix (the initial value
of the new cache key) we need to have the scheme, host and port in
their original form from the request URI, i.e. when hosting.config
is used the cache key is expected to contain a valid URI authority
element used for volume selection.

More details about the new parameter and its functionality can be
found in doc/admin-guide/plugins/cachekey.en.rst
@gtenev gtenev merged commit 0f86efc into apache:master Aug 28, 2019
@zwoop
Copy link
Contributor

zwoop commented Aug 30, 2019

Cherry-picked to v9.0.x branch.

@zwoop
Copy link
Contributor

zwoop commented Mar 26, 2020

Cherry-picked to 8.1.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants