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

Reuse headers from pulpcore::apache class #354

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Aug 27, 2024

We have the exact same code in both pulpcore::plugin::container as in pulpcore::apache. This reuses the variables rather than duplicating the logic.

Fixes: 21aa39e ("Fixes #37308 - set REMOTE_USER properly for pulpcore registry")

We have the exact same code in both pulpcore::plugin::container as in
pulpcore::apache. This reuses the variables rather than duplicating the
logic.

Fixes: 21aa39e ("Fixes #37308 - set REMOTE_USER properly for pulpcore registry")
@ekohl ekohl added the Enhancement New feature or request label Aug 27, 2024
@@ -27,7 +19,7 @@
'url' => "${pulpcore::apache::api_base_url}${registry_version_path}",
},
],
'request_headers' => $api_default_request_headers + $api_additional_request_headers,
'request_headers' => $pulpcore::apache::api_default_request_headers + $pulpcore::apache::api_additional_request_headers,
Copy link
Member Author

Choose a reason for hiding this comment

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

I debated introducing a new variable that has both these values, but decided against it to keep this smaller.

@ekohl ekohl merged commit 5b6d088 into theforeman:master Sep 3, 2024
22 checks passed
@ekohl ekohl deleted the reuse-apache-header-values branch September 3, 2024 12:25
@ekohl
Copy link
Member Author

ekohl commented Sep 3, 2024

When adding tests I figured out that at least there it's somehow different.

Redoing it, I first wrote tests prior to this patch (e3d1ddb, based on @evgeni's work elsewhere). Then reapplied this (bd1dbec) and fixed tests the (66b97fe).

Why does it result in an additional line when it should use the exact same variables? Still trying to figure that out.

@evgeni
Copy link
Member

evgeni commented Sep 3, 2024

Why does it result in an additional line when it should use the exact same variables? Still trying to figure that out.

Because the content of the variables are not identical!

The "global" ones:

$api_default_request_headers = [
"unset ${remote_user_environ_header}",
"set ${remote_user_environ_header} \"%{SSL_CLIENT_S_DN_CN}s\" env=SSL_CLIENT_S_DN_CN",
]
$api_additional_request_headers = $pulpcore::api_client_auth_cn_map.map |String $cn, String $pulp_user| {
"set ${remote_user_environ_header} \"${pulp_user}\" \"expr=%{SSL_CLIENT_S_DN_CN} == '${cn}'\""
}

The ones you removed here:

$api_default_request_headers = [
"unset ${pulpcore::apache::remote_user_environ_header}",
]
$api_additional_request_headers = $pulpcore::api_client_auth_cn_map.map |String $cn, String $pulp_user| {
"set ${pulpcore::apache::remote_user_environ_header} \"${pulp_user}\" \"expr=%{SSL_CLIENT_S_DN_CN} == '${cn}'\""
}

the global ones, that we now use have an additional

"set ${remote_user_environ_header} \"%{SSL_CLIENT_S_DN_CN}s\" env=SSL_CLIENT_S_DN_CN", 

@ekohl
Copy link
Member Author

ekohl commented Sep 3, 2024

I was so blindly staring at api_additional_request_headers that I didn't look too carefully at api_default_request_headers.

@ekohl
Copy link
Member Author

ekohl commented Sep 3, 2024

#356

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants