Feat: set Azure Manage Identity Client ID default to empty value #7815
Conversation
simonpasquier
left a comment
simonpasquier
left a comment
There was a problem hiding this comment.
I think that we need to update validateRemoteWriteSpec() (in pkg/prometheus/operator.go) to fail when the field is empty and we're on Prometheus < v3.5.0.
Update as suggested Co-authored-by: Simon Pasquier <[email protected]>
pkg/prometheus/promcfg.go
Outdated
| {Key: "client_id", Value: spec.AzureAD.ManagedIdentity.ClientID}, | ||
| }}, | ||
| ) | ||
| if cg.version.GTE(semver.MustParse("3.5.0")) { |
There was a problem hiding this comment.
I don't think that we need to check the version here: the controller should have validated before that an empty client id is supported by the prometheus version. We should only check for nil-ness.
There was a problem hiding this comment.
@simonpasquier Just want to confirm. I need to add the version check in validateRemoteWriteSpec() in pkg/prometheus/operator.go and just skip the version check here. Am I correct?
Update as suggested Co-authored-by: Simon Pasquier <[email protected]>
|
@simonpasquier Updated. Please review |
pkg/prometheus/operator.go
Outdated
| // Reference: | ||
| // https://github.com/prometheus/prometheus/blob/main/docs/configuration/configuration.md#remote_write | ||
| func validateRemoteWriteSpec(spec monitoringv1.RemoteWriteSpec) error { | ||
| func validateRemoteWriteSpec(spec monitoringv1.RemoteWriteSpec, version semver.Version, componentName ComponentName) error { |
There was a problem hiding this comment.
The way we do it currently is that fields which are not (yet) supported in Thanos are reset in pkg/thanos/operator.go to avoid "polluting" pkg/prometheus with dual concerns. It would be good to preserve this approach and move the validation to the Thanos controller.
There was a problem hiding this comment.
@simonpasquier Sure, will update it later.
|
@simonpasquier From my understanding, we should do the validation for this field in pkg/thanos/operator.go and reset it before calling the validation in pkg/prometheus, am I correct? |
|
@simonpasquier Updated, please review and suggest. Thank you! |
simonpasquier
force-pushed
the
feat/azure-relax-validate-client-id
branch
from
e2af71c to
3d9dd4c
Compare
Signed-off-by: Simon Pasquier <[email protected]>
simonpasquier
force-pushed
the
feat/azure-relax-validate-client-id
branch
from
3d9dd4c to
caebd98
Compare
|
@simonpasquier thanks for updating the branch. Do you think it has anything else to do with this issue? |
|
It's ready to merge but we need someone else than me to approve :) |
3 participants
Description
Due to the new feature introduced in the Prometheus 3.5.0, the field client_id under Azure AD Managed Identity in Remote Write is allowed to be the blank support system assigned managed identity. This PR changes the ClientID field to be optional and will place the empty string in the config when the field is not being set.
Fixes #7562
Type of change
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
Unit Testing
Changelog entry