feat: added validation for prometheusRule size (#6478) #6606

yp969803 · 2024-05-18T21:56:49Z

Description

fixes: #6478
Reject PrometheusRule objects that generate > 1MB configmap

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

added unit test

Changelog entry

)

yp969803 · 2024-05-18T22:09:01Z

@simonpasquier can u review the pr

simonpasquier

In fact the original issue is about the internal hardcoded limit of the operator which is 512KiB (1MiB/2):

prometheus-operator/pkg/prometheus/server/rules.go

Lines 36 to 40 in 7e18b82

    
           // The maximum `Data` size of a ConfigMap seems to differ between 
        
           // environments. This is probably due to different meta data sizes which count 
        
           // into the overall maximum size of a ConfigMap. Thereby lets leave a 
        
           // large buffer. 
        
           var maxConfigMapDataSize = int(float64(v1.MaxSecretSize) * 0.5)

IMHO the first things to do would be

better advertise this limit in the API documentation.
have a descriptive error message stating the operator limit and the actual value.

I'd also rather see the check in ValidateRule() since it would help users that don't run the admission webhook.

Eventually we may want to make the 512MB limit configurable and/or increase the default limit. Unfortunately #1591 has little details on why 50% of 1MiB was picked up.

simonpasquier · 2024-05-21T09:46:39Z

pkg/admission/testdata/badRulesLargeSize.golden

In this specific case, I'd avoid storing a large file in the Git repository.

yp969803 · 2024-05-27T04:54:00Z

@simonpasquier done the above changes

yp969803 · 2024-08-26T18:04:12Z

@simonpasquier @ArthurSens @slashpai the pr is ready to merge.

simonpasquier

I would remove the size check from each operator because it becomes redundant with ValidateRule(). The (minor) change would be a generated rule file could be slightly above the limit in case the namespace label enforcer is enabled but it should be safe (the 512KB limit is arbitrary anyway).

prometheus-operator/pkg/prometheus/server/rules.go

Lines 206 to 214 in 0cf01e5

    
           //check if none of the rule files is too large for a single ConfigMap 
        
           for filename, file := range ruleFiles { 
        
           	if len(file) > maxConfigMapDataSize { 
        
           		return nil, fmt.Errorf( 
        
           			"rule file '%v' is too large for a single Kubernetes ConfigMap", 
        
           			filename, 
        
           		) 
        
           	} 
        
           }

simonpasquier · 2024-08-28T08:53:16Z

pkg/admission/admission.go

@@ -233,6 +233,14 @@ func (a *Admission) validatePrometheusRules(ar v1.AdmissionReview) *v1.Admission
 		return toAdmissionResponseFailure(errUnmarshalRules, prometheusRuleResource, []error{err})
 	}

+	// Check if the size of prometheusRule exceeds 512KB (1048576 Bytes).


ValidateRule() which is called below will also check the total size hence we don't have to do it explicitly here (bonus point: it would be based on the spec size, not the raw payload).

simonpasquier · 2024-08-28T08:54:47Z

pkg/operator/rules.go

+	maxSize := 524288
+	promRuleSize := len(content)
+	if promRuleSize > maxSize {
+		return []error{fmt.Errorf("prometheusRules exceeded by %d bytes, maximum limit is 512KB", promRuleSize-maxSize)}


I'd rather print the current size and max limit.

Suggested change

return []error{fmt.Errorf("prometheusRules exceeded by %d bytes, maximum limit is 512KB", promRuleSize-maxSize)}

return []error{fmt.Errorf("the length of rendered Prometheus Rule is %d bytes which is above the maximum limit of %d bytes", promRuleSize, maxSize)}

simonpasquier · 2024-08-28T08:56:31Z

pkg/operator/rules.go

@@ -158,6 +158,14 @@ func ValidateRule(promRuleSpec monitoringv1.PrometheusRuleSpec) []error {
 	if err != nil {
 		return []error{fmt.Errorf("failed to marshal content: %w", err)}
 	}
+
+	// Check if the size of prometheusRule exceeds 512KB (1048576 Bytes).
+	maxSize := 524288


I would move the maxConfigMapDataSize consts from the pkg/prometheus/server and pkg/thanos packages to pkg/operator and use it here.

yp969803 · 2024-08-29T14:24:14Z

@simonpasquier done the changes

simonpasquier

Ideally we'd need an e2e test but it can be added later.

simonpasquier · 2024-08-29T14:56:26Z

pkg/operator/rules.go

@@ -158,6 +164,13 @@ func ValidateRule(promRuleSpec monitoringv1.PrometheusRuleSpec) []error {
 	if err != nil {
 		return []error{fmt.Errorf("failed to marshal content: %w", err)}
 	}
+
+	// Check if the size of prometheusRule exceeds 512KB (1048576 Bytes).


Suggested change

// Check if the size of prometheusRule exceeds 512KB (1048576 Bytes).

// Check if the serialized rules exceed our internal limit.

simonpasquier · 2024-08-29T14:59:03Z

pkg/prometheus/server/rules.go

@@ -206,7 +200,7 @@ func (c *Operator) selectRuleNamespaces(p *monitoringv1.Prometheus) ([]string, e
 func makeRulesConfigMaps(p *monitoringv1.Prometheus, ruleFiles map[string]string, opts ...operator.ObjectOption) ([]v1.ConfigMap, error) {
 	//check if none of the rule files is too large for a single ConfigMap
 	for filename, file := range ruleFiles {
-		if len(file) > maxConfigMapDataSize {
+		if len(file) > operator.MaxConfigMapDataSize {


the block shouldn't be needed anymore since the operator should have called ValidateRule() before.

Suggested change

if len(file) > operator.MaxConfigMapDataSize {

yp969803 · 2024-08-30T14:26:19Z

@simonpasquier done the changes

pkg/prometheus/server/rules_test.go

simonpasquier · 2024-09-02T09:05:53Z

pkg/thanos/rules.go

@@ -207,7 +201,7 @@ func (o *Operator) selectRuleNamespaces(p *monitoringv1.ThanosRuler) ([]string,
 func makeRulesConfigMaps(t *monitoringv1.ThanosRuler, ruleFiles map[string]string, opts ...operator.ObjectOption) ([]v1.ConfigMap, error) {
 	//check if none of the rule files is too large for a single ConfigMap
 	for filename, file := range ruleFiles {
-		if len(file) > maxConfigMapDataSize {
+		if len(file) > operator.MaxConfigMapDataSize {


same remark than before: promRuleSelector.Select() will prevent "too big" rules from reaching here so we can remove this check.

yp969803 · 2024-09-13T14:05:17Z

@simonpasquier done the changes

simonpasquier

Thanks!

feat: added validation for prometheusRule size (prometheus-operator#6478

f11a914

)

yp969803 requested a review from a team as a code owner May 18, 2024 21:56

pull-request-size bot added the size/S label May 18, 2024

simonpasquier reviewed May 21, 2024

View reviewed changes

rfac: added size limit in ValidateRule

2943f32

github-actions bot added the stale label Jul 27, 2024

yp969803 added 4 commits August 26, 2024 22:48

patch: make generate

00f8d9e

Merge branch 'main' into issue6478

3ece7bb

rfac: admission.go

5e9af1f

rfac: admission.go warn logger

15d0a11

github-actions bot removed the stale label Aug 27, 2024

simonpasquier reviewed Aug 28, 2024

View reviewed changes

rfac: removed raw prometheusRuleSize check

2e67971

pull-request-size bot added size/M and removed size/S labels Aug 29, 2024

simonpasquier reviewed Aug 29, 2024

View reviewed changes

rfac: prometheus server rules.go

ce13db6

rfac: removed ShouldErrorOnTooLargeRuleFile unit test

cc7eb7b

simonpasquier reviewed Sep 2, 2024

View reviewed changes

chore: added prometheusRule size unit test

92f78c1

rfac: golint solved

58c2a60

simonpasquier approved these changes Sep 16, 2024

View reviewed changes

simonpasquier merged commit 4f36476 into prometheus-operator:main Sep 16, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added validation for prometheusRule size (#6478) #6606

feat: added validation for prometheusRule size (#6478) #6606

yp969803 commented May 18, 2024

yp969803 commented May 18, 2024

simonpasquier left a comment

simonpasquier May 21, 2024

yp969803 commented May 27, 2024

yp969803 commented Aug 26, 2024 •

edited

Loading

simonpasquier left a comment

simonpasquier Aug 28, 2024

simonpasquier Aug 28, 2024

simonpasquier Aug 28, 2024

yp969803 commented Aug 29, 2024

simonpasquier left a comment

simonpasquier Aug 29, 2024

simonpasquier Aug 29, 2024

yp969803 commented Aug 30, 2024

simonpasquier Sep 2, 2024

yp969803 commented Sep 13, 2024

simonpasquier left a comment

	// The maximum `Data` size of a ConfigMap seems to differ between
	// environments. This is probably due to different meta data sizes which count
	// into the overall maximum size of a ConfigMap. Thereby lets leave a
	// large buffer.
	var maxConfigMapDataSize = int(float64(v1.MaxSecretSize) * 0.5)

	//check if none of the rule files is too large for a single ConfigMap
	for filename, file := range ruleFiles {
	if len(file) > maxConfigMapDataSize {
	return nil, fmt.Errorf(
	"rule file '%v' is too large for a single Kubernetes ConfigMap",
	filename,
	)
	}
	}

	return []error{fmt.Errorf("prometheusRules exceeded by %d bytes, maximum limit is 512KB", promRuleSize-maxSize)}
	return []error{fmt.Errorf("the length of rendered Prometheus Rule is %d bytes which is above the maximum limit of %d bytes", promRuleSize, maxSize)}

	// Check if the size of prometheusRule exceeds 512KB (1048576 Bytes).
	// Check if the serialized rules exceed our internal limit.

feat: added validation for prometheusRule size (#6478) #6606

feat: added validation for prometheusRule size (#6478) #6606

Conversation

yp969803 commented May 18, 2024

Description

Type of change

Verification

Changelog entry

yp969803 commented May 18, 2024

simonpasquier left a comment

Choose a reason for hiding this comment

simonpasquier May 21, 2024

Choose a reason for hiding this comment

yp969803 commented May 27, 2024

yp969803 commented Aug 26, 2024 • edited Loading

simonpasquier left a comment

Choose a reason for hiding this comment

simonpasquier Aug 28, 2024

Choose a reason for hiding this comment

simonpasquier Aug 28, 2024

Choose a reason for hiding this comment

simonpasquier Aug 28, 2024

Choose a reason for hiding this comment

yp969803 commented Aug 29, 2024

simonpasquier left a comment

Choose a reason for hiding this comment

simonpasquier Aug 29, 2024

Choose a reason for hiding this comment

simonpasquier Aug 29, 2024

Choose a reason for hiding this comment

yp969803 commented Aug 30, 2024

simonpasquier Sep 2, 2024

Choose a reason for hiding this comment

yp969803 commented Sep 13, 2024

simonpasquier left a comment

Choose a reason for hiding this comment

yp969803 commented Aug 26, 2024 •

edited

Loading