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

config: Made sure config is generatable: Added unsafe option for Secrets. #1804

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

Conversation

bwplotka
Copy link
Member

@bwplotka bwplotka commented Mar 18, 2019

Hello 👋

Essentially the problem is that we define our Infrastructure using Golang code. This means that we directly import alertmanager configuration, we fill the config structure and generate YAML at the end that will be used to in configmap later on.

The current implementation masks secrets on rendering time for UI.

In this PR I added and unsafe flag so user of this struct can choose if it should be rendered or not.

Alternatively we can think of any other way of filling secrets, like env vars? But I don't think it's currently possible.

Any thoughts?

Signed-off-by: Bartek Plotka bwplotka@gmail.com

@brian-brazil
Copy link
Contributor

This means that we directly import alertmanager configuration

The question is if this is something we want to support. This (and all the others) is an internal library, intended for use only by the AM.

Alternatively we can think of any other way of filling secrets, like env vars?

Env vars are not a secure place to put secrets.

@bwplotka
Copy link
Member Author

bwplotka commented Mar 18, 2019

That makes sense. I can see this being quite not typical use case (:

I hope there will be more users like as though over time as this (generating directly from struct) is really good way to make sure your infrastracture/configuration is up to date with alertmananger version.

The question is if this is something we want to support. This (and all the others) is an internal library, intended for use only by the AM.

Then it's question to AM maintainers. (: Are you interested to support this? Otherwise I will vendor the generatable configuration in some other repository which will be quite painful for me and community (if someone has similar interests as us)

@bwplotka
Copy link
Member Author

Some build errors I need to fix still

@brian-brazil
Copy link
Contributor

Then it's question to AM maintainers.

It's a question for the entire project, as we use secrets in at least 4 repos.

@bwplotka
Copy link
Member Author

Good point 👍 consistency.

@bwplotka bwplotka force-pushed the generatable-config branch 3 times, most recently from 9b2e819 to e14d046 Compare March 20, 2019 15:30
@dak1n1
Copy link

dak1n1 commented Apr 10, 2019

This PR is going to be very useful for my work, since I'm creating an operator that writes the Alertmanager config for OpenShift clusters. (Work-in-progress code lives here, if anyone wants to see what I'm doing https://github.com/dak1n1/configure-alertmanager-operator/blob/initial_create/pkg/controller/secret/secret_controller.go ).

I think vendoring this library will be useful for SRE teams who need to dynamically generate the Alertmanager config based on a set of conditions. For example, the presence of a Pager Duty secret in my cluster means my operator will generate the necessary Alertmanager configs to use Pager Duty.

@bwplotka
Copy link
Member Author

Happy to rebase, if needed. We use this on prod already to generate our infra definitions directly from Golang.

@stuartnelson3
Copy link
Contributor

This (and all the others) is an internal library, intended for use only by the AM.

I think this bears repeating. This is an unintended use of the internal types, and it also is being requested from very few users.

I'm currently against adding this since it benefits fairly few users, makes it much easier to accidentally expose secrets, and is a feature that would be better decided upon by the entire org (do we want to expose config types that can be used to generate a valid config?).

…ets.

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
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.

None yet

4 participants