feat: Add azuread oauth support to remote write #6037

adinhodovic · 2023-10-22T14:35:26Z

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.

Fixes #6032. Adds azure ad oauth support to remote write.

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.

Add azuread oauth support to remote write

pkg/assets/store.go

pkg/apis/monitoring/v1/prometheus_types.go

pkg/assets/store.go

nicolastakashi · 2023-10-23T20:31:34Z

pkg/prometheus/promcfg.go

+				if err != nil {
+					return yaml.MapItem{}, fmt.Errorf("the provided Azure OAuth clientId is invalid")
+				}
+				matched := tenantIDre.MatchString(spec.AzureAD.OAuth.TenantID)


I'm a bit torn if we should validate this on the operator or not, because if MS change this we have to change the operator too.

But!
If we think it's valuable, I'd prefer to add this annotation // +kubebuilder:validation:Pattern to the struct and let the validation on the CRD side.

WDYT?

Moved validation to the CRD side. I'd keep it probably since prometheus itself does the same validation? Invalid id -> prometheus startup issues?

I'm fine with either though!

pkg/apis/monitoring/v1/prometheus_types.go

ArthurSens

Thanks for the PR! Looking at "API Conventions" in our CONTRIBUTING.md we could make some improvements to the API definition.

Mostly around Optionals vs. Required

pkg/apis/monitoring/v1/prometheus_types.go

pkg/prometheus/promcfg_test.go

ArthurSens · 2023-10-27T19:33:17Z

pkg/prometheus/promcfg.go

+			if spec.AzureAD.ManagedIdentity.ClientID == "" && spec.AzureAD.OAuth == nil {
+				return yaml.MapItem{}, fmt.Errorf("must provide Azure Managed Identity or Azure OAuth in the Azure AD config")
+			}
+			if spec.AzureAD.ManagedIdentity.ClientID != "" && spec.AzureAD.OAuth != nil {
+				return yaml.MapItem{}, fmt.Errorf("cannot provide both Azure Managed Identity and Azure OAuth in the Azure AD config")
+			}


I wonder if there isn't a more appropriate place for the validation? The method signature is only generateRemoteWriteConfig and doesn't mention any kind of validation. I also don't see other remote write options being validated here.

Sorry for giving such a vague answer for this one, at the moment I can't remember where exactly we validate remote-write, but I have a feeling we do it elsewhere 😅

by validation I mean, we don't return errors here because we've already checked it beforehand

Thanks in general for all the feedback! Specifically this comment -> I've moved it to the ValidateRemoteWriteSpec function and reverted the signature of the generateRemoteWriteConfig to no error returns. Makes sense.

ArthurSens · 2023-10-27T19:33:44Z

pkg/prometheus/promcfg.go

+				if !cg.WithMinimumVersion("2.48.0").IsCompatible() {
+					return yaml.MapItem{}, fmt.Errorf("Azure OAuth is only supported in Prometheus 2.48.0 and later")
+				}
+				if spec.AzureAD.OAuth.ClientID == "" {
+					return yaml.MapItem{}, fmt.Errorf("clientId must be provided in the Azure AD OAuth config")
+				}
+				if spec.AzureAD.OAuth.TenantID == "" {
+					return yaml.MapItem{}, fmt.Errorf("tenantId must be provided in the Azure AD OAuth config")
+				}
+				_, err := uuid.Parse(spec.AzureAD.OAuth.ClientID)
+				if err != nil {
+					return yaml.MapItem{}, fmt.Errorf("the provided Azure OAuth clientId is invalid")
+				}


pkg/apis/monitoring/v1/prometheus_types.go

ArthurSens

Some very small nits!

pkg/prometheus/testdata/RemoteWriteConfig_v2.48.0_1.golden

ArthurSens · 2023-11-10T20:31:27Z

pkg/prometheus/operator.go

@@ -96,8 +97,7 @@ func NewTLSAssetSecret(p monitoringv1.PrometheusInterface, labels map[string]str
 	}
 }

-// ValidateRemoteWriteSpec checks that mutually exclusive configurations are not
-// included in the Prometheus remoteWrite configuration section.
+// ValidateRemoteWriteSpec validates the RemoteWriteSpec.


Suggested change

// ValidateRemoteWriteSpec validates the RemoteWriteSpec.

// ValidateRemoteWriteSpec checks that mutually exclusive configurations are not

// included in the Prometheus remoteWrite configuration section, while validating

// RemoteWriteSpec child fields.

I'm not sure about this one, I think it makes sense to keep the previous comment and extend with the extra functionality... I struggling to find a good comment though 😅

That sounds good :)

simonpasquier · 2023-11-16T13:54:46Z

Prometheus v2.48.0 is out!

simonpasquier

Nice! Just a few minor comments otherwise LGTM.

simonpasquier · 2023-11-17T08:42:17Z

pkg/apis/monitoring/v1/prometheus_types.go

+	OAuth *AzureOAuth `json:"oauth,omitempty"`
+}
+
+// AzureOAuth is used to store azure oauth config values.


Suggested change

// AzureOAuth is used to store azure oauth config values.

// AzureOAuth defines the Azure OAuth settings.

simonpasquier · 2023-11-17T08:47:48Z

pkg/apis/monitoring/v1/prometheus_types.go

@@ -1246,8 +1246,28 @@ type AzureAD struct {
 	// +optional
 	Cloud *string `json:"cloud,omitempty"`
 	// ManagedIdentity defines the Azure User-assigned Managed identity.
+	// Cannot be set at the same time as `oAuth`.


Suggested change

// Cannot be set at the same time as `oAuth`.

// Cannot be set at the same time as `oauth`.

pkg/apis/monitoring/v1/prometheus_types.go

simonpasquier · 2023-11-17T08:51:05Z

pkg/apis/monitoring/v1/prometheus_types.go

+// AzureOAuth is used to store azure oauth config values.
+// +k8s:openapi-gen=true
+type AzureOAuth struct {
+	// ClientID is the clientId of the azure active directory application that is being used to authenticate.


Suggested change

// ClientID is the clientId of the azure active directory application that is being used to authenticate.

// `clientID` is the clientId of the Azure Active Directory application that is being used to authenticate.

simonpasquier · 2023-11-17T08:54:46Z

pkg/apis/monitoring/v1/prometheus_types.go

+	// ClientID is the clientId of the azure active directory application that is being used to authenticate.
+	// +required
+	ClientID string `json:"clientId"`
+	// ClientSecret is the clientSecret of the azure active directory application that is being used to authenticate.


Suggested change

// ClientSecret is the clientSecret of the azure active directory application that is being used to authenticate.

// `clientSecret` specifies a key of a Secret containing the client secret of the Azure Active Directory application that is being used to authenticate.

simonpasquier · 2023-11-17T08:55:15Z

pkg/apis/monitoring/v1/prometheus_types.go

+	// ClientSecret is the clientSecret of the azure active directory application that is being used to authenticate.
+	// +required
+	ClientSecret v1.SecretKeySelector `json:"clientSecret"`
+	// TenantID is the tenantId of the azure active directory application that is being used to authenticate.


Suggested change

// TenantID is the tenantId of the azure active directory application that is being used to authenticate.

// tenantID is the tenant ID of the Azure Active Directory application that is being used to authenticate.

pkg/apis/monitoring/v1/prometheus_types.go

simonpasquier · 2023-11-17T09:02:07Z

pkg/prometheus/operator.go

+			if spec.AzureAD.OAuth.ClientID == "" {
+				return fmt.Errorf("clientId must be provided in the Azure AD OAuth config")
+			}
+			if spec.AzureAD.OAuth.TenantID == "" {
+				return fmt.Errorf("tenantId must be provided in the Azure AD OAuth config")
+			}


already validated at the CRD level

Suggested change

if spec.AzureAD.OAuth.ClientID == "" {

return fmt.Errorf("clientId must be provided in the Azure AD OAuth config")

}

if spec.AzureAD.OAuth.TenantID == "" {

return fmt.Errorf("tenantId must be provided in the Azure AD OAuth config")

}

pkg/prometheus/promcfg_test.go

Signed-off-by: adinhodovic <hodovicadin@gmail.com>

simonpasquier

All good! I've spotted one small typo but we can merge it now and you can fix it later.

simonpasquier · 2023-11-17T14:23:39Z

pkg/apis/monitoring/v1/prometheus_types.go

+	// `clientSecret` specifies a key of a Secret containing the client secret of the Azure Active Directory application that is being used to authenticate.
+	// +required
+	ClientSecret v1.SecretKeySelector `json:"clientSecret"`
+	// `tenantID` is the tenant ID of the Azure Active Directory application that is being used to authenticate.


Suggested change

// `tenantID` is the tenant ID of the Azure Active Directory application that is being used to authenticate.

// `tenantId` is the tenant ID of the Azure Active Directory application that is being used to authenticate.

adinhodovic requested a review from a team as a code owner October 22, 2023 14:35

pull-request-size bot added the size/L label Oct 22, 2023

adinhodovic force-pushed the add-azuread-oauth branch from 1a5e0d5 to 960100c Compare October 22, 2023 14:48

heylongdacoder reviewed Oct 22, 2023

View reviewed changes

pkg/assets/store.go Outdated Show resolved Hide resolved

adinhodovic force-pushed the add-azuread-oauth branch 2 times, most recently from 3db7fcd to 48fa1d8 Compare October 22, 2023 23:17

nicolastakashi reviewed Oct 23, 2023

View reviewed changes

pkg/apis/monitoring/v1/prometheus_types.go Outdated Show resolved Hide resolved

nicolastakashi reviewed Oct 23, 2023

View reviewed changes

pkg/apis/monitoring/v1/prometheus_types.go Outdated Show resolved Hide resolved

nicolastakashi reviewed Oct 23, 2023

View reviewed changes

pkg/assets/store.go Outdated Show resolved Hide resolved

nicolastakashi reviewed Oct 23, 2023

View reviewed changes

adinhodovic force-pushed the add-azuread-oauth branch from 48fa1d8 to f434330 Compare October 25, 2023 11:37

nicolastakashi reviewed Oct 25, 2023

View reviewed changes

pkg/apis/monitoring/v1/prometheus_types.go Outdated Show resolved Hide resolved

ArthurSens reviewed Oct 27, 2023

View reviewed changes

pkg/apis/monitoring/v1/prometheus_types.go Outdated Show resolved Hide resolved

pkg/apis/monitoring/v1/prometheus_types.go Outdated Show resolved Hide resolved

adinhodovic force-pushed the add-azuread-oauth branch 2 times, most recently from e58b401 to 8b5ecb9 Compare October 28, 2023 11:32

ArthurSens reviewed Nov 10, 2023

View reviewed changes

pull-request-size bot added size/XXL and removed size/L labels Nov 13, 2023

adinhodovic force-pushed the add-azuread-oauth branch from 5d7e70b to dd4230f Compare November 13, 2023 10:07

pull-request-size bot added size/L and removed size/XXL labels Nov 13, 2023

adinhodovic force-pushed the add-azuread-oauth branch 4 times, most recently from 332ec32 to 77814de Compare November 13, 2023 10:29

ArthurSens approved these changes Nov 16, 2023

View reviewed changes

simonpasquier reviewed Nov 17, 2023

View reviewed changes

feat: Add azuread oauth support to remote write

7f22f69

Signed-off-by: adinhodovic <hodovicadin@gmail.com>

adinhodovic force-pushed the add-azuread-oauth branch from 77814de to 7f22f69 Compare November 17, 2023 12:11

simonpasquier approved these changes Nov 17, 2023

View reviewed changes

simonpasquier merged commit b6b6e29 into prometheus-operator:main Nov 17, 2023
17 checks passed

ArthurSens mentioned this pull request Nov 30, 2023

Cut release 0.70 #6134

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add azuread oauth support to remote write #6037

feat: Add azuread oauth support to remote write #6037

adinhodovic commented Oct 22, 2023

nicolastakashi Oct 23, 2023

adinhodovic Oct 25, 2023

ArthurSens left a comment

ArthurSens Oct 27, 2023

ArthurSens Oct 27, 2023

adinhodovic Oct 28, 2023

ArthurSens Oct 27, 2023

ArthurSens left a comment

ArthurSens Nov 10, 2023

adinhodovic Nov 13, 2023

simonpasquier commented Nov 16, 2023

simonpasquier left a comment

simonpasquier Nov 17, 2023

simonpasquier Nov 17, 2023

simonpasquier Nov 17, 2023

simonpasquier Nov 17, 2023

simonpasquier Nov 17, 2023

simonpasquier Nov 17, 2023

simonpasquier left a comment

simonpasquier Nov 17, 2023

adinhodovic Nov 17, 2023

	// AzureOAuth is used to store azure oauth config values.
	// AzureOAuth defines the Azure OAuth settings.

	// Cannot be set at the same time as `oAuth`.
	// Cannot be set at the same time as `oauth`.

	// ClientID is the clientId of the azure active directory application that is being used to authenticate.
	// `clientID` is the clientId of the Azure Active Directory application that is being used to authenticate.

	// ClientSecret is the clientSecret of the azure active directory application that is being used to authenticate.
	// `clientSecret` specifies a key of a Secret containing the client secret of the Azure Active Directory application that is being used to authenticate.

	// TenantID is the tenantId of the azure active directory application that is being used to authenticate.
	// tenantID is the tenant ID of the Azure Active Directory application that is being used to authenticate.

	// `tenantID` is the tenant ID of the Azure Active Directory application that is being used to authenticate.
	// `tenantId` is the tenant ID of the Azure Active Directory application that is being used to authenticate.

feat: Add azuread oauth support to remote write #6037

feat: Add azuread oauth support to remote write #6037

Conversation

adinhodovic commented Oct 22, 2023

Description

Type of change

Changelog entry

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArthurSens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArthurSens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonpasquier commented Nov 16, 2023

simonpasquier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonpasquier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment