prometheus: Add AWS sigv4 auth to Alertmanager endpoints #6036
Conversation
| @@ -1518,13 +1518,8 @@ func (c *Operator) createOrUpdateConfigurationSecret(ctx context.Context, p *mon | |||
| } | |||
|
|
|||
| if p.Spec.Alerting != nil { | |||
| for i, am := range p.Spec.Alerting.Alertmanagers { | |||
| if err := store.AddBasicAuth(ctx, p.GetNamespace(), am.BasicAuth, fmt.Sprintf("alertmanager/auth/%d", i)); err != nil { | |||
There was a problem hiding this comment.
I don't get the reason why we move this logic from this place to the pkg/prometheus/store.go
There was a problem hiding this comment.
I noticed some similar functions in the pkg/prometheus/store.go, such as AddRemoteWritesToStore, so I moved it to this file to standardize it. 😄
There was a problem hiding this comment.
Looks good to me :)
Indeed a lot of asset store code is already present in the store package
There was a problem hiding this comment.
AddRemoteWritesToStore exists in pkg/prometheus/store.go because it's used by both the server and agent controllers, unlike the Alertmanager validation which is only relevant for the server. It's not a big deal to move it but wasn't strictly required.
Fixes: prometheus-operator#6033 Signed-off-by: Wen Long <heylongdacoder@gmail.com>
heylongdacoder
force-pushed
the
issue-6033
branch
from
f671125
to
8cc5434
Compare
|
e2e test failed due to the operator crash. Made this changes to fix it (https://github.com/prometheus-operator/prometheus-operator/compare/f6711256e47695ef95a77276e4f5a8983f82a221..8cc54344f9195680a176298b9ea33e3e3cbdb952) 🙏 |
|
|
||
| cfg = cg.WithMinimumVersion("2.26.0").AppendMapItem(cfg, "sigv4", sigV4) | ||
| } | ||
| cfg = cg.WithMinimumVersion("2.26.0").addSigv4ToYaml(cfg, fmt.Sprintf("remoteWrite/%d", i), store, spec.Sigv4) |
There was a problem hiding this comment.
Copy-paste mistake with remoteWrite? 😅
There was a problem hiding this comment.
Hi @ArthurSens, thanks for the review! Nope, it is not a mistake. Since we can now configure sigv4 in both the alertmanagerEndpoints and remoteWrite configurations, I have added a new addSigv4ToYaml function and updated this section to use this new function as well. 😄
| @@ -1518,13 +1518,8 @@ func (c *Operator) createOrUpdateConfigurationSecret(ctx context.Context, p *mon | |||
| } | |||
|
|
|||
| if p.Spec.Alerting != nil { | |||
| for i, am := range p.Spec.Alerting.Alertmanagers { | |||
| if err := store.AddBasicAuth(ctx, p.GetNamespace(), am.BasicAuth, fmt.Sprintf("alertmanager/auth/%d", i)); err != nil { | |||
There was a problem hiding this comment.
Looks good to me :)
Indeed a lot of asset store code is already present in the store package
| @@ -1518,13 +1518,8 @@ func (c *Operator) createOrUpdateConfigurationSecret(ctx context.Context, p *mon | |||
| } | |||
|
|
|||
| if p.Spec.Alerting != nil { | |||
| for i, am := range p.Spec.Alerting.Alertmanagers { | |||
| if err := store.AddBasicAuth(ctx, p.GetNamespace(), am.BasicAuth, fmt.Sprintf("alertmanager/auth/%d", i)); err != nil { | |||
There was a problem hiding this comment.
AddRemoteWritesToStore exists in pkg/prometheus/store.go because it's used by both the server and agent controllers, unlike the Alertmanager validation which is only relevant for the server. It's not a big deal to move it but wasn't strictly required.
Description
Fixes: #6033
Type of change
What type of changes does your code introduce to the Prometheus operator? Put an
xin 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.