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

use AlertmanagerConfigAllowList for alertmanager secret informer #4455

Merged
merged 3 commits into from
Feb 2, 2022
Merged

use AlertmanagerConfigAllowList for alertmanager secret informer #4455

merged 3 commits into from
Feb 2, 2022

Conversation

william-tran
Copy link
Contributor

@william-tran william-tran commented Dec 16, 2021 •
edited by simonpasquier

Description

Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request.
If it fixes a bug or resolves a feature request, be sure to link to that issue.

I want to use the operator to handle PrometheusRule CRDs in many namespaces but only handle Prometheus CRDs in one namespace. I saw the default RBAC in the helm chart was far too broad and wrote my own, having secrets operations as Roles instead of ClusterRoles, attached only to the namespaces where I want to set up prometheus instances. I noticed after setting --namespaces and --prometheus-instance-selector to different values, that list/watch of secrets was being attempted in the namespaces under --namespaces, with access denied.

Fixes #3544 (and #3298 in the case --namespaces is unset, which results in attempting to watch all secrets)

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.)

Changelog entry

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

Added `-alertmanager-config-namespaces` CLI flag to the operator.

@william-tran william-tran requested a review from a team as a code owner December 16, 2021 03:22
@simonpasquier
Copy link
Contributor

This secrets informer exists for AlertmanagerConfig custom resources which themselves can reference secret keys (for defining receiver's credentials for instance). These resources live in the namespaces defined by --namespaces and not --alertmanager-instance-namespaces which is why the informer is set up this way.
Now the operator should probably have another secrets informer watching for alertmanager-instance-namespaces to reconcile changes to the base Alertmanager configuration's secret...
cc @philipgough

@william-tran
Copy link
Contributor Author

william-tran commented Dec 16, 2021 •
edited

These resources live in the namespaces defined by --namespaces

Ahh I see now with the Alertmanager CRD vs AlertmanagerConfig which can reference secrets e.g. https://prometheus-operator.dev/docs/operator/api/#slackconfig apiURL

To address tighter scoping of secrets permissions, we could add an option alertmanager-config-namespaces. This could cater to those who setup alertmanager config centrally versus having anyone provide that config.

@simonpasquier
Copy link
Contributor

To address tighter scoping of secrets permissions, we could add an option alertmanager-config-namespaces

I wouldn't be opposed to this. To be consistent with the other flags, the logic should be:

  • use --alertmanager-config-namespaces if not empty.
  • otherwise use --namespaces (as before).

@fpetkovski @paulfantom thoughts?

@philipgough
Copy link
Contributor

@simonpasquier Sounds reasonable to me. Will wait for others to chime in.

@fpetkovski
Copy link
Contributor

This suggestion makes sense to me as well 👍

@paulfantom
Copy link
Member

Simon's suggestion sounds good to me.

@william-tran
Copy link
Contributor Author

Great, I'll get to work on this and will have something by end of week.

@william-tran william-tran force-pushed the alertmanager-secret-informer branch 3 times, most recently from b7a584f to 7c3c80d Compare January 13, 2022 04:23
@william-tran william-tran changed the title use AlertmanagerAllowList for alertmanager secret informer use AlertmanagerConfigAllowList for alertmanager secret informer Jan 13, 2022
simonpasquier
simonpasquier approved these changes Jan 14, 2022
View reviewed changes
Copy link
Contributor

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

lgtm

@william-tran
Copy link
Contributor Author

@simonpasquier is there a release we can expect this in?

@fpetkovski
Copy link
Contributor

@william-tran some CI jobs are failing, you should be able to fix them with make format generate.

@fpetkovski
Copy link
Contributor

@simonpasquier okay with you to merge?

@william-tran
Copy link
Contributor Author

The check docs error seems unrelated to this PR:

https://github.com/prometheus-operator/prometheus-operator/runs/5004881958?check_suite_focus=true#step:5:254

error: 		ADOPTERS.md:32: "https://www.clyso.com/en" not accessible even after retry; status code 0: Get "https://www.clyso.com/en": unexpected EOF

@philipgough
Copy link
Contributor

@william-tran - if you rebase on top of main that check should resolve. It is not required however so we can merge as is.

dependabot bot and others added 3 commits January 31, 2022 13:07
@dependabot
build(deps): bump github.com/prometheus/client_golang
5b99e24 
Bumps [github.com/prometheus/client_golang](https://github.com/prometheus/client_golang) from 1.12.0 to 1.12.1.
- [Release notes](https://github.com/prometheus/client_golang/releases)
- [Changelog](https://github.com/prometheus/client_golang/blob/main/CHANGELOG.md)
- [Commits](prometheus/client_golang@v1.12.0...v1.12.1)

---
updated-dependencies:
- dependency-name: github.com/prometheus/client_golang
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Add a new flag alertmanager-config-namespaces to watch
cfc67d8 
AlertmanagerConfig namespaces require Secrets list/watch, so separating
this config from general --namespaces provides better control over which
namespaces to allow Secrets RBAC access.

Fixes prometheus-operator#3544

Signed-off-by: Will Tran <will@autonomic.ai>
Use AlertmanagerConfigAllowList for alrtCfgInfs and nsAlrtCfgInf as well
3d8f8d4 
Signed-off-by: Will Tran <will@autonomic.ai>
@william-tran
Copy link
Contributor Author

@philipgough done

@simonpasquier simonpasquier merged commit 023feec into prometheus-operator:main Feb 2, 2022
@simonpasquier
Copy link
Contributor

thanks!

0robustus1 pushed a commit to 0robustus1/prometheus-operator that referenced this pull request Feb 22, 2022
@dependabot @0robustus1
use AlertmanagerConfigAllowList for alertmanager secret informer (pro…
bc1a208 
…metheus-operator#4455)

* build(deps): bump github.com/prometheus/client_golang

Bumps [github.com/prometheus/client_golang](https://github.com/prometheus/client_golang) from 1.12.0 to 1.12.1.
- [Release notes](https://github.com/prometheus/client_golang/releases)
- [Changelog](https://github.com/prometheus/client_golang/blob/main/CHANGELOG.md)
- [Commits](prometheus/client_golang@v1.12.0...v1.12.1)

---
updated-dependencies:
- dependency-name: github.com/prometheus/client_golang
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* Add a new flag alertmanager-config-namespaces to watch

AlertmanagerConfig namespaces require Secrets list/watch, so separating
this config from general --namespaces provides better control over which
namespaces to allow Secrets RBAC access.

Fixes prometheus-operator#3544

Signed-off-by: Will Tran <will@autonomic.ai>

* Use AlertmanagerConfigAllowList for alrtCfgInfs and nsAlrtCfgInf as well

Signed-off-by: Will Tran <will@autonomic.ai>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@simonpasquier simonpasquier mentioned this pull request Mar 7, 2022
Merged
5 tasks
@eyenx eyenx mentioned this pull request Mar 17, 2022
Merged
6 tasks
@stevo-f3 stevo-f3 mentioned this pull request Jul 14, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fpetkovski fpetkovski fpetkovski approved these changes

@simonpasquier simonpasquier simonpasquier approved these changes

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

Successfully merging this pull request may close these issues.

Support and document tighter RBAC permissions for Operator
5 participants
@william-tran @simonpasquier @philipgough @fpetkovski @paulfantom