Correct container plugin apache header rewrite #356
Conversation
In 5b6d088 it was assumed that `$api_default_request_headers` was the same, but it was slightly different. This corrects it and adds tests, which should have been added in the first place. These tests pass prior to the application and after, making sure there's no change. Fixes: 5b6d088 ("Reuse headers from pulpcore::apache class")
| @@ -9,6 +9,11 @@ | |||
| String $location_prefix = '/pulpcore_registry', | |||
| String $registry_version_path = '/v2/', | |||
| ) { | |||
| # This is like pulpcore::apache's value, but slightly different | |||
| $api_default_request_headers = [ | |||
There was a problem hiding this comment.
Do we really need this?
https://httpd.apache.org/docs/current/mod/mod_headers.html#requestheader says
set
The request header is set, replacing any previous header with this name
So while the content of the file is changed, the behavior should not?
(Unless the cn map is empty)
There was a problem hiding this comment.
The CN map contains "if" expressions. My understanding is the first line 100% certain clears it. Then IF the cert's CN equals something, it sets the header.
There was a problem hiding this comment.
Right, so the flow is:
- use
unsetto remove any header by that name, wherever it comes from - use
setto set it to the CN (if there is a CN!) - use
setto set it toadmin(if the expression matches, resetting a possibly previously set CN-based name)
I honestly see no reason (ever) to set the CN-based remote user, as such user just doesn't exist in Pulp (in our deployment)
There was a problem hiding this comment.
My vote: don't add this complexity (same variable name, different values) back in, it's confusing as hell.
Do the releases we need to do.
Drop CN-stuff in an 11.x
There was a problem hiding this comment.
Should we then include 11.x in the upcoming releases? I'd actually feel more comfortable if we only had the explicit unset and explicit mapping.
There was a problem hiding this comment.
fwiw, I tested the current (two-set) config with theforeman/forklift#1853 and it pushes containers fine, so 🤷♀️
There was a problem hiding this comment.
I'm leaning to merging this now as is because it keeps it minimal and then resolve it properly in the next release cycle.
|
Skipping per #358 (comment) |
In 5b6d088 it was assumed that
$api_default_request_headerswas the same, but it was slightly different. This corrects it and adds tests, which should have been added in the first place. These tests pass prior to the application and after, making sure there's no change.Fixes: 5b6d088 ("Reuse headers from pulpcore::apache class")