change: make -error-on-replace more cooperating (#233)

* change: make `-error-on-replace` more cooperating With this change, the proxy wouldn't necessary return an error if `-error-on-replace` is set and the incoming query has a label matcher overlapping with the enforced label. For instance when the enforced label is `namespace="foo"`, the following expressions aren't rejected anymore: - `up{namespace!="bar"}` - `up{namespace=~"foo|bar"}` - `up{namespace!~"bar"}` The following expressions are rejected as before: - `up{namespace=""}` - `up{namespace="bar"}` - `up{namespace=~"bar|fred"}` - `up{namespace!~"foo"}` Signed-off-by: Simon Pasquier <[email protected]> * Address squat's comments Signed-off-by: Simon Pasquier <[email protected]> --------- Signed-off-by: Simon Pasquier <[email protected]>
prometheus-community · Jul 30, 2024 · a177f41 · a177f41
1 parent b7b2c74
commit a177f41
Show file tree

Hide file tree

Showing 3 changed files with 773 additions and 16 deletions.
diff --git a/README.md b/README.md
@@ -157,7 +157,7 @@ prom-label-proxy \
 
 > :warning: The above feature is experimental. Be careful when using this option, it may expose sensitive metrics if you use a too permissive expression.
 
-To error out when the query already contains a label matcher that differs from the one the proxy would inject, you can use the `-error-on-replace` option. For example:
+To error out when the query already contains a label matcher that conflicts with the one the proxy would inject, you can use the `-error-on-replace` option. For example:
 
 ```
 prom-label-proxy \

diff --git a/injectproxy/enforce.go b/injectproxy/enforce.go
@@ -152,30 +152,150 @@ func (ms PromQLEnforcer) EnforceNode(node parser.Node) error {
 	return nil
 }
 
-// EnforceMatchers appends the configured label matcher if not present.
-// If the label matcher that is to be injected is present (by labelname) but
-// different (either by match type or value) the behavior depends on the
-// errorOnReplace variable and the enforced matcher(s):
-// * if errorOnReplace is true, an error is returned,
-// * if errorOnReplace is false and the label matcher type is '=', the existing matcher is silently replaced.
-// * otherwise the existing matcher is preserved.
+// EnforceMatchers appends the enforced label matcher(s) to the list of matchers
+// if not already present.
+//
+// If the label matcher that is to be injected is present (by labelname), the
+// behavior depends on the errorOnReplace variable and the enforced matcher(s):
+// * If errorOnReplace is false
+//   - And the label matcher type is '=', the existing matcher is silently
+//     discarded whatever is the original value.
+//   - Otherwise the existing matcher is preserved.
+//
+// * if errorOnReplace is true
+//   - And the label matcher and the enforced matcher are disjoint, the function returns an error.
+//   - Otherwise the existing matcher is preserved.
 func (ms PromQLEnforcer) EnforceMatchers(targets []*labels.Matcher) ([]*labels.Matcher, error) {
 	var res []*labels.Matcher
 
 	for _, target := range targets {
-		if matcher, ok := ms.labelMatchers[target.Name]; ok {
-			// matcher.String() returns something like "labelfoo=value"
-			if ms.errorOnReplace && matcher.String() != target.String() {
-				return res, fmt.Errorf("%w: label matcher value %q conflicts with injected value %q", ErrIllegalLabelMatcher, matcher.String(), target.String())
+		matcher, ok := ms.labelMatchers[target.Name]
+		if !ok {
+			res = append(res, target)
+			continue
+		}
+
+		if ms.errorOnReplace {
+			var ok bool
+
+			// Ensure that the expression's matcher combined with the
+			// enforced matchers can return some result. If the combined
+			// matchers return no result, the function returns an error.
+			//
+			// For instance, when the enforced matcher is 'tenant="bar"':
+			// * and the expression's selector is 'tenant="foo"' then the
+			// result is always empty.
+			// * and the expression's selector is 'tenant!="foo"' then the
+			// matchers don't conflict.
+			switch matcher.Type {
+
+			case labels.MatchEqual:
+				switch target.Type {
+				case labels.MatchEqual:
+					ok = matcher.Value == target.Value
+				case labels.MatchNotEqual:
+					ok = matcher.Value != target.Value
+				case labels.MatchRegexp:
+					ok = target.Matches(matcher.Value)
+				case labels.MatchNotRegexp:
+					ok = target.Matches(matcher.Value)
+				}
+
+			case labels.MatchNotEqual:
+				switch target.Type {
+				case labels.MatchEqual:
+					ok = target.Value == "" || matcher.Matches(target.Value)
+				case labels.MatchNotEqual:
+					ok = true
+				case labels.MatchRegexp:
+					frm, _ := labels.NewFastRegexMatcher(target.Value)
+					ok = (frm == nil || len(frm.SetMatches()) == 0)
+					if !ok {
+						for _, sm := range frm.SetMatches() {
+							if sm != matcher.Value {
+								ok = true
+								break
+							}
+						}
+					}
+				case labels.MatchNotRegexp:
+					ok = true
+				}
+
+			case labels.MatchRegexp:
+				frm, _ := labels.NewFastRegexMatcher(matcher.Value)
+				switch target.Type {
+				case labels.MatchEqual:
+					ok = matcher.Matches(target.Value)
+				case labels.MatchNotEqual:
+					if frm != nil {
+						for _, sm := range frm.SetMatches() {
+							if target.Matches(sm) {
+								ok = true
+								break
+							}
+						}
+					}
+					ok = ok || (target.Value == "" && !matcher.Matches(""))
+				case labels.MatchRegexp:
+					if frm != nil {
+						for _, sm := range frm.SetMatches() {
+							if target.Matches(sm) {
+								ok = true
+								break
+							}
+						}
+					}
+				case labels.MatchNotRegexp:
+					if frm != nil {
+						for _, sm := range frm.SetMatches() {
+							if target.Matches(sm) {
+								ok = true
+								break
+							}
+						}
+					}
+					ok = ok || (target.Value == "" && !matcher.Matches(""))
+				}
+
+			case labels.MatchNotRegexp:
+				switch target.Type {
+				case labels.MatchEqual:
+					ok = target.Value != "" || !matcher.Matches("")
+				case labels.MatchNotEqual:
+					ok = true
+				case labels.MatchRegexp:
+					frm, _ := labels.NewFastRegexMatcher(target.Value)
+					if frm != nil {
+						for _, sm := range frm.SetMatches() {
+							if matcher.Matches(sm) {
+								ok = true
+								break
+							}
+						}
+					}
+					ok = ok && !(target.Value == "" && matcher.Matches(""))
+					ok = ok && target.Value != matcher.Value
+				case labels.MatchNotRegexp:
+					ok = true
+				}
 			}
 
-			// Drop the existing matcher only if the enforced matcher is an
-			// equal matcher.
-			if matcher.Type == labels.MatchEqual {
-				continue
+			if !ok {
+				return res, fmt.Errorf("%w: label matcher %q conflicts with injected matcher %q", ErrIllegalLabelMatcher, target.String(), matcher.String())
 			}
 		}
 
+		// Always drop the expression matcher if:
+		// * the enforced matcher is an equal matcher because it will be
+		// added after iterating on all the expression's matchers.
+		// * or it is equal to the enforced matcher.
+		// In both cases, the enforced matcher will be added after
+		// iterating on all the expression's matchers.
+		if matcher.Type == labels.MatchEqual || matcher.String() == target.String() {
+			continue
+		}
+
 		res = append(res, target)
 	}