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
[WIP] Feat: Add clusterTLSConfigs to the Alertmanager CRD #6489
base: main
Are you sure you want to change the base?
[WIP] Feat: Add clusterTLSConfigs to the Alertmanager CRD #6489
Conversation
Questions:
|
Requesting review: @simonpasquier @ArthurSens @nicolastakashi @slashpai. Some concepts are still not clear to me, especially stuff that is not directly related to the code itself but the cluster tls config in general. I think the review will help a lot. Kindly bear with me, if I am not up to speed! |
pkg/alertmanager/statefulset.go
Outdated
// Mount web config and web TLS credentials as volumes. | ||
// We always mount the web config file for versions greater than 0.22.0. | ||
// With this we avoid redeploying alertmanager when reconfiguring between | ||
// HTTP and HTTPS and vice-versa. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that the comment applies here because an empty mTLS config file isn't allowed. So we'll have to redeploy the pods if the spec changes from no-TLS to TLS (and vice-versa).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, so we cannot mount an empty volume like we do with web tls. So here, we only mount the config and redeploy the pod depending on whether there is mTLS config or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// If the am version >= 0.24.0, create a new secret for the mTLS configuration.
// The secret volume is always created and mounted, even if the secret is empty.
// But, empty volume cannot be passed as the path in the argument for the cluster TLS configuration.
// So, we only pass the argument `--cluster.tls-config` if the ClusterTLSConfig field is not nil.
if c.Client != nil { | ||
if err := c.Client.Validate(); err != nil { | ||
return fmt.Errorf("am mtls: client config, %w", err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After c.Client.Validate(), we should check that the cert and key fields are valid (not empty) configmap/secret references.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added some checks. However, I do not quite understand why this does not apply for web TLS. Could you briefly touch upon that?
…manager statefulset
reuse existing code and remove unwanted structs
…f certs and keyhs
3a43bfe
to
7347499
Compare
Background: Problem with De-duplication: Alternative Approach: |
Description
This PR adds mTLS config to AlertManager, allowing user to configure gossip between alert managers using mTLS for authentication.
Closes: #4241
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.