chore: Add checks for selectors in KubernetesSDConfig (#6177) #6359

yp969803 · 2024-03-02T18:37:21Z

Description

fix: #6177
additional checks on the role selectors vs. the main role added

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

Add checks for selectors in KubernetesSDConfig

yp969803 · 2024-03-02T18:37:43Z

@slashpai can you review the pr

slashpai

I haven't reviewed full but its a good start, could you please add a test as well in https://github.com/prometheus-operator/prometheus-operator/blob/main/pkg/prometheus/resource_selector_test.go#L1710?

slashpai · 2024-03-04T17:06:24Z

pkg/apis/monitoring/v1alpha1/scrapeconfig_types.go

@@ -354,6 +366,32 @@ type KubernetesSDConfig struct {
 	Selectors []K8SSelectorConfig `json:"selectors,omitempty"`
 }

+// ValidateSelectorRoles validate the roles based on main role.


move this to https://github.com/prometheus-operator/prometheus-operator/blob/main/pkg/prometheus/resource_selector.go

slashpai · 2024-03-04T17:07:23Z

pkg/prometheus/promcfg.go

@@ -2604,6 +2604,12 @@ func (cg *ConfigGenerator) generateScrapeConfig(
 	if len(sc.Spec.KubernetesSDConfigs) > 0 {
 		configs := make([][]yaml.MapItem, len(sc.Spec.KubernetesSDConfigs))
 		for i, config := range sc.Spec.KubernetesSDConfigs {
+
+			err := (&config).ValidateSelectorRoles()


validation should be called from validateKubernetesSDConfigs in https://github.com/prometheus-operator/prometheus-operator/blob/main/pkg/prometheus/resource_selector.go

yp969803 · 2024-03-05T20:05:13Z

@slashpai done the changes and added the tests

ArthurSens · 2024-03-05T20:17:27Z

pkg/prometheus/resource_selector.go

+	foundSelectorRoles := make(map[monitoringv1alpha1.Role]struct{})
+	allowedSelectors := map[monitoringv1alpha1.Role][]string{
+		RolePod:           {string(RolePod)},
+		RoleService:       {string(RoleService)},
+		RoleEndpointSlice: {string(RolePod), string(RoleService), string(RoleEndpointSlice)},
+		RoleEndpoint:      {string(RolePod), string(RoleService), string(RoleEndpoint)},
+		RoleNode:          {string(RoleNode)},
+		RoleIngress:       {string(RoleIngress)},
+	}
+
+	for _, selector := range c.Selectors {
+
+		if _, ok := foundSelectorRoles[selector.Role]; ok {
+			return fmt.Errorf("duplicated selector role: %s", selector.Role)
+		}


I have the impression that foundSelectorRoles is never populated, therefore line 858 will never return ok 🤔

If it returns ok then error will come, while if it returns !ok then it should pass, tests are also passing

I agree with Arthur, I also tried it as a simple go code here https://go.dev/play/p/Di7AaLX_iW-
currently it accepts the the role pod for role node which it shouldn't.

Take a look at validation here we need to do something similar https://github.com/prometheus/prometheus/blob/92544c00bf9d43eecd7161911d541a3fbe7af8cc/discovery/kubernetes/kubernetes.go#L204-L233

slashpai

Did you also test this it locally on a cluster?

slashpai · 2024-03-22T06:53:05Z

pkg/prometheus/promcfg.go

@@ -2604,6 +2604,7 @@ func (cg *ConfigGenerator) generateScrapeConfig(
 	if len(sc.Spec.KubernetesSDConfigs) > 0 {
 		configs := make([][]yaml.MapItem, len(sc.Spec.KubernetesSDConfigs))
 		for i, config := range sc.Spec.KubernetesSDConfigs {
+


nit: remove the blank line

slashpai · 2024-03-22T06:57:11Z

pkg/prometheus/resource_selector.go

@@ -830,13 +840,38 @@ func (rs *ResourceSelector) SelectScrapeConfigs(ctx context.Context, listFn List
 	return res, nil
 }

+// ValidateSelectorRoles validate the roles based on main role.
+func ValidateSelectorRoles(c monitoringv1alpha1.KubernetesSDConfig) error {


I don't think we need to have a function for this as it is not used any other place, better lets move the code to

prometheus-operator/pkg/prometheus/resource_selector.go

Lines 832 to 862 in 1e9c2bb

func (rs *ResourceSelector) validateKubernetesSDConfigs(ctx context.Context, sc *monitoringv1alpha1.ScrapeConfig) error {

for i, config := range sc.Spec.KubernetesSDConfigs {

configKey := fmt.Sprintf("scrapeconfig/%s/%s/kubernetessdconfig/%d", sc.GetNamespace(), sc.GetName(), i)

if err := rs.store.AddBasicAuth(ctx, sc.GetNamespace(), config.BasicAuth, configKey); err != nil {

return fmt.Errorf("[%d]: %w", i, err)

}

configAuthKey := fmt.Sprintf("scrapeconfig/auth/%s/%s/kubernetessdconfig/%d", sc.GetNamespace(), sc.GetName(), i)

if err := rs.store.AddSafeAuthorizationCredentials(ctx, sc.GetNamespace(), config.Authorization, configAuthKey); err != nil {

return fmt.Errorf("[%d]: %w", i, err)

}

if err := rs.store.AddOAuth2(ctx, sc.GetNamespace(), config.OAuth2, configKey); err != nil {

return fmt.Errorf("[%d]: %w", i, err)

}

if err := rs.store.AddSafeTLSConfig(ctx, sc.GetNamespace(), config.TLSConfig); err != nil {

return fmt.Errorf("[%d]: %w", i, err)

}

if err := validateProxyConfig(ctx, config.ProxyConfig, rs.store, sc.GetNamespace()); err != nil {

return fmt.Errorf("[%d]: %w", i, err)

}

if config.APIServer != nil && config.Namespaces != nil {

if ptr.Deref(config.Namespaces.IncludeOwnNamespace, false) {

return fmt.Errorf("[%d]: %w", i, errors.New("cannot use 'apiServer' and 'namespaces.ownNamespace' simultaneously"))

}

}

for _, s := range config.Selectors {

slashpai · 2024-03-22T07:20:56Z

pkg/prometheus/resource_selector.go

+	foundSelectorRoles := make(map[monitoringv1alpha1.Role]struct{})
+	allowedSelectors := map[monitoringv1alpha1.Role][]string{
+		RolePod:           {string(RolePod)},
+		RoleService:       {string(RoleService)},
+		RoleEndpointSlice: {string(RolePod), string(RoleService), string(RoleEndpointSlice)},
+		RoleEndpoint:      {string(RolePod), string(RoleService), string(RoleEndpoint)},
+		RoleNode:          {string(RoleNode)},
+		RoleIngress:       {string(RoleIngress)},
+	}
+
+	for _, selector := range c.Selectors {
+
+		if _, ok := foundSelectorRoles[selector.Role]; ok {
+			return fmt.Errorf("duplicated selector role: %s", selector.Role)
+		}


I agree with Arthur, I also tried it as a simple go code here https://go.dev/play/p/Di7AaLX_iW-
currently it accepts the the role pod for role node which it shouldn't.

Take a look at validation here we need to do something similar https://github.com/prometheus/prometheus/blob/92544c00bf9d43eecd7161911d541a3fbe7af8cc/discovery/kubernetes/kubernetes.go#L204-L233

slashpai · 2024-03-22T07:38:51Z

pkg/prometheus/resource_selector.go

}
+

nit: remove extra blank line

slashpai · 2024-03-22T07:38:55Z

pkg/prometheus/resource_selector.go

@@ -870,7 +909,9 @@ func (rs *ResourceSelector) validateKubernetesSDConfigs(ctx context.Context, sc
 			if _, err := labels.Parse(s.Label); err != nil {
 				return fmt.Errorf("[%d]: %w", i, err)
 			}
+


nit: remove extra blank line

simonpasquier · 2024-03-22T15:40:32Z

pkg/prometheus/resource_selector.go

+
+	for _, selector := range c.Selectors {
+
+		if _, ok := foundSelectorRoles[selector.Role]; ok {


We need to apply strings.ToLower() on the role value (e.g. prometheus-operator both "Pod" and "pod").

yp969803 · 2024-03-23T08:50:11Z

@slashpai done the changes

simonpasquier · 2024-03-27T10:07:12Z

pkg/prometheus/resource_selector.go

@@ -42,6 +42,16 @@ import (
 	"github.com/prometheus-operator/prometheus-operator/pkg/operator"
 )

+// The valid options for Role.
+const (


it would be good to have the consts defined in pkg/apis/monitoring/v1 directly.

@simonpasquier previously i have done the same, but @slashpai suggested me to put it in the resource_selector

Thats for validation methods 🙂
Since consts were also moved I was ok but I agree with Simon consts go better in types.go

Let's have a follow-up for this.

simonpasquier · 2024-03-27T10:26:33Z

pkg/prometheus/resource_selector.go

+			var allowed bool
+
+			for _, role := range allowedSelectors[config.Role] {
+				if role == strings.ToLower(string(s.Role)) {
+					allowed = true
+					break
+				}
+			}
+			if !allowed {
+				return fmt.Errorf("[%d] : %s role supports only %s selectors", i, config.Role, strings.Join(allowedSelectors[config.Role], ", "))
+			}


(suggestion) we could use https://pkg.go.dev/slices#Contains

simonpasquier · 2024-03-27T10:27:15Z

pkg/prometheus/resource_selector.go

+		}
+
+		for _, s := range config.Selectors {
+			if _, ok := allowedSelectors[config.Role]; !ok {


we need strings.ToLower too?

Suggested change

if _, ok := allowedSelectors[config.Role]; !ok {

if _, ok := allowedSelectors[strings.ToLower(config.Role)]; !ok {

@simonpasquier done

simonpasquier · 2024-03-29T15:51:51Z

pkg/prometheus/resource_selector.go

@@ -42,6 +42,16 @@ import (
 	"github.com/prometheus-operator/prometheus-operator/pkg/operator"
 )

+// The valid options for Role.
+const (


Let's have a follow-up for this.

simonpasquier · 2024-03-29T15:53:59Z

TestSelectScrapeConfigs/Kubernetes_SD_config_with_valid_label_and_field_selectors is failing

…rator#6177) chore: test added rfac: kubernetes sd role chore: cofig.Role to lowercase rfac: unit_test role_consts

yp969803 · 2024-04-02T18:03:24Z

@simonpasquier done the above changes, rfac the unit test

simonpasquier

LGTM

yp969803 requested a review from a team as a code owner March 2, 2024 18:37

pull-request-size bot added the size/M label Mar 2, 2024

slashpai requested changes Mar 4, 2024

View reviewed changes

yp969803 force-pushed the issue6177 branch from 618c01a to a37469f Compare March 5, 2024 20:04

ArthurSens reviewed Mar 5, 2024

View reviewed changes

slashpai reviewed Mar 22, 2024

View reviewed changes

simonpasquier reviewed Mar 22, 2024

View reviewed changes

yp969803 force-pushed the issue6177 branch from a37469f to 12de956 Compare March 23, 2024 08:49

simonpasquier reviewed Mar 27, 2024

View reviewed changes

yp969803 force-pushed the issue6177 branch from 12de956 to 64cb318 Compare March 27, 2024 21:30

simonpasquier approved these changes Mar 29, 2024

View reviewed changes

chore: Add checks for selectors in KubernetesSDConfig (prometheus-ope…

cf52179

…rator#6177) chore: test added rfac: kubernetes sd role chore: cofig.Role to lowercase rfac: unit_test role_consts

yp969803 force-pushed the issue6177 branch from 64cb318 to cf52179 Compare April 2, 2024 18:02

simonpasquier approved these changes Apr 3, 2024

View reviewed changes

slashpai approved these changes Apr 3, 2024

View reviewed changes

slashpai merged commit 745813d into prometheus-operator:main Apr 3, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Add checks for selectors in KubernetesSDConfig (#6177) #6359

chore: Add checks for selectors in KubernetesSDConfig (#6177) #6359

yp969803 commented Mar 2, 2024

yp969803 commented Mar 2, 2024

slashpai left a comment

slashpai Mar 4, 2024

slashpai Mar 4, 2024

yp969803 commented Mar 5, 2024

ArthurSens Mar 5, 2024

yp969803 Mar 7, 2024

slashpai Mar 22, 2024

slashpai left a comment

slashpai Mar 22, 2024

slashpai Mar 22, 2024

slashpai Mar 22, 2024

slashpai Mar 22, 2024

slashpai Mar 22, 2024

simonpasquier Mar 22, 2024

yp969803 commented Mar 23, 2024

simonpasquier Mar 27, 2024

yp969803 Mar 27, 2024

slashpai Mar 28, 2024

simonpasquier Mar 29, 2024

simonpasquier Mar 27, 2024

simonpasquier Mar 27, 2024

yp969803 Mar 27, 2024

simonpasquier Mar 29, 2024

simonpasquier commented Mar 29, 2024

yp969803 commented Apr 2, 2024

simonpasquier left a comment

	func (rs ResourceSelector) validateKubernetesSDConfigs(ctx context.Context, sc monitoringv1alpha1.ScrapeConfig) error {
	for i, config := range sc.Spec.KubernetesSDConfigs {
	configKey := fmt.Sprintf("scrapeconfig/%s/%s/kubernetessdconfig/%d", sc.GetNamespace(), sc.GetName(), i)
	if err := rs.store.AddBasicAuth(ctx, sc.GetNamespace(), config.BasicAuth, configKey); err != nil {
	return fmt.Errorf("[%d]: %w", i, err)
	}

	configAuthKey := fmt.Sprintf("scrapeconfig/auth/%s/%s/kubernetessdconfig/%d", sc.GetNamespace(), sc.GetName(), i)
	if err := rs.store.AddSafeAuthorizationCredentials(ctx, sc.GetNamespace(), config.Authorization, configAuthKey); err != nil {
	return fmt.Errorf("[%d]: %w", i, err)
	}

	if err := rs.store.AddOAuth2(ctx, sc.GetNamespace(), config.OAuth2, configKey); err != nil {
	return fmt.Errorf("[%d]: %w", i, err)
	}

	if err := rs.store.AddSafeTLSConfig(ctx, sc.GetNamespace(), config.TLSConfig); err != nil {
	return fmt.Errorf("[%d]: %w", i, err)
	}

	if err := validateProxyConfig(ctx, config.ProxyConfig, rs.store, sc.GetNamespace()); err != nil {
	return fmt.Errorf("[%d]: %w", i, err)
	}

	if config.APIServer != nil && config.Namespaces != nil {
	if ptr.Deref(config.Namespaces.IncludeOwnNamespace, false) {
	return fmt.Errorf("[%d]: %w", i, errors.New("cannot use 'apiServer' and 'namespaces.ownNamespace' simultaneously"))
	}
	}

	for _, s := range config.Selectors {


		for _, selector := range c.Selectors {

		if _, ok := foundSelectorRoles[selector.Role]; ok {

	if _, ok := allowedSelectors[config.Role]; !ok {
	if _, ok := allowedSelectors[strings.ToLower(config.Role)]; !ok {

chore: Add checks for selectors in KubernetesSDConfig (#6177) #6359

chore: Add checks for selectors in KubernetesSDConfig (#6177) #6359

Conversation

yp969803 commented Mar 2, 2024

Description

Type of change

Changelog entry

yp969803 commented Mar 2, 2024

slashpai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yp969803 commented Mar 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slashpai 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

yp969803 commented Mar 23, 2024

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

Choose a reason for hiding this comment

simonpasquier commented Mar 29, 2024

yp969803 commented Apr 2, 2024

simonpasquier left a comment

Choose a reason for hiding this comment