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.

Sign up for GitHub

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

Jump to bottom

[kube-prometheus-stack] allow to create secrets for basic auth usage #1193

Merged
merged 2 commits into from
Aug 2, 2021
Merged

[kube-prometheus-stack] allow to create secrets for basic auth usage #1193

merged 2 commits into from
Aug 2, 2021

Conversation

thirdeyenick
Copy link
Contributor

What this PR does / why we need it:

This PR allows to pass hashed credentials to the helm chart. Those credentials will be stored in a k8s secret. That k8s secret can then be referenced to support basic authentication in ingress resources. The nginx ingress controller has that feature for example.

Although Prometheus does support basic authentication itself, the prometheus-operator project doesn't seem to allow to configure this via the Prometheus CRD.

Which issue this PR fixes

Special notes for your reviewer:

Checklist

  • DCO signed
  • Chart Version bumped
  • Title of the PR starts with chart name (e.g. [prometheus-couchdb-exporter])

@gkarthiks
Copy link
Member

Thank you for your contribution @thirdeyenick.

Though I understand the need for this, I have the slightest hesitation on this PR. IMHO, one shouldn't do a basic auth like this. Rather the system should be integrated with an IDP. Because when done something like a secret file with the creds, wouldn't this be insecure?

I would defer this PR for the rest of the owners @bismarck @gianrubio @scottrigby @vsliouniaev @Xtigyro

Thank you @thirdeyenick for your patience and for helping us in defining the best solution for the community.

@thirdeyenick
Copy link
Contributor Author

Hi,

Though I understand the need for this, I have the slightest hesitation on this PR. IMHO, one shouldn't do a basic auth like this. Rather the system should be integrated with an IDP. Because when done something like a secret file with the creds, wouldn't this be insecure?

It for sure depends on the implementation of the ingress controller, which kind of credentials are needed in the k8s secret.
Nginx Ingress supports bcrypt for example, so you just push a bcrypt hashed password into the helm values. I wouldn't consider this as insecure. But for sure: using the PLAIN scheme (so username and password in plaintext) would be really insecure. It depends on the user which password scheme will be used.

monotek
monotek previously requested changes Jul 24, 2021
View reviewed changes
Copy link
Member

@monotek monotek left a comment

Choose a reason for hiding this comment

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

I would suggest to go for a more general solution like "extraSecrets".

@thirdeyenick
Copy link
Contributor Author

I would suggest to go for a more general solution like "extraSecrets".

I like the idea and did some changes. Should now be more generic.

@thirdeyenick thirdeyenick requested a review from monotek July 26, 2021 08:39
gkarthiks
gkarthiks previously approved these changes Jul 26, 2021
View reviewed changes
Copy link
Member

@gkarthiks gkarthiks left a comment

Choose a reason for hiding this comment

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

lgtm

@thirdeyenick
Copy link
Contributor Author

Anything to do for me still?

@gkarthiks
Copy link
Member

Anything to do for me still?

nope, just waiting for @monotek approval. Since he requested a change and Github is waiting for it. I don't want to dismiss his review.

@thirdeyenick
allow to create a secret for basic auth
1d46f99 
This allows to create a secret which can be used for basic
authentication in ingresses

Signed-off-by: nick <nick@nine.ch>
@thirdeyenick
allow for a more generic usage
645fe60 
Signed-off-by: nick <nick@nine.ch>
@shiron-babi
Copy link

This feature is very useful, thanks for the hard work!

Any estimation when it will be merged ?

@gkarthiks gkarthiks dismissed monotek’s stale review August 2, 2021 15:18

All requested changes are implemented. Dismissing to get the merge button enabled.

@gkarthiks gkarthiks merged commit 23335a9 into prometheus-community:main Aug 2, 2021
langecode pushed a commit to neticdk/helm-charts that referenced this pull request Aug 12, 2021
@thirdeyenick @langecode
[kube-prometheus-stack] allow to create secrets for basic auth usage (p…
e095caf 
…rometheus-community#1193)

* allow to create a secret for basic auth

This allows to create a secret which can be used for basic
authentication in ingresses

Signed-off-by: nick <nick@nine.ch>

* allow for a more generic usage

Signed-off-by: nick <nick@nine.ch>
Signed-off-by: Thor Anker Kvisgård Lange <tal@netic.dk>
@ArchiFleKs ArchiFleKs mentioned this pull request Aug 31, 2021
Closed
3 tasks
QuentinBisson pushed a commit to giantswarm/prometheus-community-helm-charts-upstream that referenced this pull request Oct 5, 2021
@thirdeyenick @QuentinBisson
[kube-prometheus-stack] allow to create secrets for basic auth usage (p…
04faee8 
…rometheus-community#1193)

* allow to create a secret for basic auth

This allows to create a secret which can be used for basic
authentication in ingresses

Signed-off-by: nick <nick@nine.ch>

* allow for a more generic usage

Signed-off-by: nick <nick@nine.ch>
Signed-off-by: QuentinBisson <quentin@giantswarm.io>
@heriet heriet mentioned this pull request Feb 6, 2022
Merged
3 tasks
stamzid pushed a commit to Unstructured-IO/prometheus-community-helm-charts that referenced this pull request Mar 3, 2023
@thirdeyenick
[kube-prometheus-stack] allow to create secrets for basic auth usage (p…
a51f89c 
…rometheus-community#1193)

* allow to create a secret for basic auth

This allows to create a secret which can be used for basic
authentication in ingresses

Signed-off-by: nick <nick@nine.ch>

* allow for a more generic usage

Signed-off-by: nick <nick@nine.ch>
@manidharanupoju24
Copy link

Can this be used without ingress?

@manidharanupoju24 manidharanupoju24 mentioned this pull request May 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gkarthiks gkarthiks gkarthiks approved these changes

@bismarck bismarck Awaiting requested review from bismarck

@gianrubio gianrubio Awaiting requested review from gianrubio gianrubio is a code owner

@scottrigby scottrigby Awaiting requested review from scottrigby scottrigby is a code owner

@vsliouniaev vsliouniaev Awaiting requested review from vsliouniaev

@Xtigyro Xtigyro Awaiting requested review from Xtigyro Xtigyro is a code owner

@monotek monotek Awaiting requested review from monotek

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Configuration for Basic Authentication ( prometheus/alertmanager)
5 participants
@thirdeyenick @gkarthiks @shiron-babi @manidharanupoju24 @monotek