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

feat(configmap-reloader): introduce flag reload-authorization-header #7258

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RouxAntoine
Copy link

@RouxAntoine RouxAntoine commented Jan 6, 2025

Introduce flag in the configmap reloader to pass username and password for basic authentication to call prometheus reload endpoint.

Description

The purpose is to support a secured prometheus with basic auth.

As far as I know this solve the issues #5836 and #2358 .

Type of change

What type of changes does your code introduce to the Prometheus operator? Put an x in the box that apply.

  • CHANGE (fix or feature that would cause existing functionality to not work as expected)
  • FEATURE (non-breaking change which adds functionality)
  • BUGFIX (non-breaking change which fixes an issue)
  • ENHANCEMENT (non-breaking change which improves existing functionality)
  • NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Verification

Please check the Prometheus-Operator testing guidelines for recommendations about automated tests.

Changelog entry

Please put a one-line changelog entry below. This will be copied to the changelog file during the release process.

* [FEATURE] Introduce flag `reload-basic-username` and `reload-basic-password` to pass as basic auth credential when calling prometheus reload endpoint. #7258
This new flag is optional and default to no header

@RouxAntoine
Copy link
Author

Waiting for merge of thanos-io/thanos#8042

@RouxAntoine RouxAntoine force-pushed the feature/prometheus-config-reloader-authenticated branch from bf58206 to 0cf5ef6 Compare January 7, 2025 20:04
@pull-request-size pull-request-size bot added size/M and removed size/S labels Jan 7, 2025
@RouxAntoine RouxAntoine marked this pull request as ready for review January 7, 2025 20:06
@RouxAntoine RouxAntoine requested a review from a team as a code owner January 7, 2025 20:06
@RouxAntoine
Copy link
Author

After discussion and recommandation on the thanos pull request thanos-io/thanos#8042 (comment) I change my first try to a new meaning with round tripper.
in this proposition there is now two new flag one for username and one for password.

@RouxAntoine RouxAntoine force-pushed the feature/prometheus-config-reloader-authenticated branch 2 times, most recently from 403702a to de2a844 Compare January 7, 2025 20:17
@@ -105,6 +106,9 @@ func main() {
reloadURL := app.Flag("reload-url", "URL to trigger the configuration").
Default("http://127.0.0.1:9090/-/reload").URL()

reloadBasicUsername := app.Flag("reload-basic-username", "basic username passed as Authorization Header with Basic auth schema").Default("").String()
reloadBasicPassword := app.Flag("reload-basic-password", "basic password passed as Authorization Header with Basic auth schema").Default("").String()
Copy link
Contributor

Choose a reason for hiding this comment

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

password should be read from a file.

Copy link
Author

@RouxAntoine RouxAntoine Jan 11, 2025

Choose a reason for hiding this comment

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

In my case I don't need it (because I pass flag with helm chart and terraform secret in extraArgs https://github.com/prometheus-community/helm-charts/blob/main/charts/prometheus/values.yaml#L75).

Does there is a security problem behind this constraint ?

In case I would use a file (with extraConfigmapMounts or extraVolumeMounts) does something like a plain file path containing a user:password is sufficient or a more advanced json for exemple with

{
   "user": "",
   "password": ""
}

?

@RouxAntoine
Copy link
Author

After my pull request I find out this one
#7004
which seem similar and wider, is it then a good idea to continue mine ?

@simonpasquier
Copy link
Contributor

#7004 fell through the cracks of my review backlog but yes it would accomplish the same thing and is already in good shape.

@RouxAntoine
Copy link
Author

RouxAntoine commented Jan 15, 2025

Do you have some EAT for merge of #7004 please ?
I just ask to know if for my personal use, I need to go ahead with my forked version meanwhile or if release is plan soon and then I can wait

… pass username and password for basic authentication use during call to prometheus reload endpoint

Signed-off-by: RouxAntoine <[email protected]>
@RouxAntoine RouxAntoine force-pushed the feature/prometheus-config-reloader-authenticated branch from b109b33 to 7fb8913 Compare March 1, 2025 19:10
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.

2 participants