apis/nfd: empty match expression set returns no features for templates

This patch changes a rare corner case of custom label rules with an
empty set of matchexpressions. The patch removes a special case where an
empty match expression set matched everything and returned all feature
elements for templates to consume. With this patch the match expression
set logically evaluates all expressions in the set and returns all
matches - if there are no expressions there are no matches and no
matched features are returned. However, the overall match result
(determining if "non-template" labels will be created) in this special
case will be "true" as before as none of the zero match expressions
failed.

The former behavior was somewhat illogical and counterintuitive: having
1 to N expressions matched and returned 1 to N features (at most), but,
having 0 expressions always matched everything and returned all
features. This was some leftover proof-of-concept functionality (for
some possible future extensions) that should have been removed before
merging.

docs/advanced/customization-guide.md

@@ -420,10 +420,6 @@ element whose name matches the `<key>`. However, for *instance* features all
 MatchExpressions are evaluated against the attributes of each instance
 separately.
 
-A special case of an empty `matchExpressions` field matches everything, i.e.
-matches/returns all elements of the feature. This makes it possible to write
-[templates](#templating) that run over all discovered features.
-
 #### MatchAny
 
 The `.matchAny` field is a list of of [`matchFeatures`](#matchfeatures)
@@ -624,27 +620,6 @@ would be executed on matcher#1 as well as on matcher#2 and/or matcher#3
 these separate expansions would be created, i.e. the end result would be a
 union of all the individual expansions.
 
-A special case of an empty `matchExpressions` field matches everything, i.e.
-matches/returns all elements of the feature. This makes it possible to write
-[templates](#templating) that run over all discovered features.
-
-Consider the following example:
-<!-- {% raw %} -->
-
-```yaml
-    labelsTemplate: |
-      {{ range .network.device }}net-{{ .name }}.speed-mbps={{ .speed }}
-      {{ end }}
-    matchFeatures:
-      - feature: network.device
-        matchExpressions: null
-```
-
-<!-- {% endraw %} -->
-This will create individual
-`feature.node.kubernetes.io/net-<if-name>.speed-mbpx=<speed-in-mbps>` label for
-each (physical) network device of the system.
-
 Rule templates use the Golang [text/template](https://pkg.go.dev/text/template)
 package and all its built-in functionality (e.g. pipelines and functions) can
 be used. An example template taking use of the built-in `len` function,

pkg/apis/nfd/v1alpha1/expression.go

@@ -314,8 +314,8 @@ func (m *MatchExpression) UnmarshalJSON(data []byte) error {
 
 // MatchKeys evaluates the MatchExpressionSet against a set of keys.
 func (m *MatchExpressionSet) MatchKeys(keys map[string]feature.Nil) (bool, error) {
-	v, err := m.MatchGetKeys(keys)
-	return v != nil, err
+	matched, _, err := m.MatchGetKeys(keys)
+	return matched, err
 }
 
 // MatchedKey holds one matched key.
@@ -328,35 +328,28 @@ type MatchedKey struct {
 // empty MatchExpressionSet returns all existing keys are returned. Note that
 // an empty MatchExpressionSet and an empty set of keys returns an empty slice
 // which is not nil and is treated as a match.
-func (m *MatchExpressionSet) MatchGetKeys(keys map[string]feature.Nil) ([]MatchedKey, error) {
+func (m *MatchExpressionSet) MatchGetKeys(keys map[string]feature.Nil) (bool, []MatchedKey, error) {
 	ret := make([]MatchedKey, 0, m.Len())
 
-	// An empty rule matches all existing keys
-	if m.Len() == 0 {
-		for n := range keys {
-			ret = append(ret, MatchedKey{Name: n})
-		}
-	}
-
 	for n, e := range (*m).Expressions {
 		match, err := e.MatchKeys(n, keys)
 		if err != nil {
-			return nil, err
+			return false, nil, err
 		}
 		if !match {
-			return nil, nil
+			return false, nil, nil
 		}
 		ret = append(ret, MatchedKey{Name: n})
 	}
 	// Sort for reproducible output
 	sort.Slice(ret, func(i, j int) bool { return ret[i].Name < ret[j].Name })
-	return ret, nil
+	return true, ret, nil
 }
 
 // MatchValues evaluates the MatchExpressionSet against a set of key-value pairs.
 func (m *MatchExpressionSet) MatchValues(values map[string]string) (bool, error) {
-	v, err := m.MatchGetValues(values)
-	return v != nil, err
+	matched, _, err := m.MatchGetValues(values)
+	return matched, err
 }
 
 // MatchedValue holds one matched key-value pair.
@@ -370,29 +363,22 @@ type MatchedValue struct {
 // MatchExpressionSet returns all existing key-value pairs. Note that an empty
 // MatchExpressionSet and an empty set of values returns an empty non-nil map
 // which is treated as a match.
-func (m *MatchExpressionSet) MatchGetValues(values map[string]string) ([]MatchedValue, error) {
+func (m *MatchExpressionSet) MatchGetValues(values map[string]string) (bool, []MatchedValue, error) {
 	ret := make([]MatchedValue, 0, m.Len())
 
-	// An empty rule matches all existing values
-	if m.Len() == 0 {
-		for n, v := range values {
-			ret = append(ret, MatchedValue{Name: n, Value: v})
-		}
-	}
-
 	for n, e := range (*m).Expressions {
 		match, err := e.MatchValues(n, values)
 		if err != nil {
-			return nil, err
+			return false, nil, err
 		}
 		if !match {
-			return nil, nil
+			return false, nil, nil
 		}
 		ret = append(ret, MatchedValue{Name: n, Value: values[n]})
 	}
 	// Sort for reproducible output
 	sort.Slice(ret, func(i, j int) bool { return ret[i].Name < ret[j].Name })
-	return ret, nil
+	return true, ret, nil
 }
 
 // MatchInstances evaluates the MatchExpressionSet against a set of instance

pkg/apis/nfd/v1alpha1/expression_test.go

@@ -306,7 +306,7 @@ func TestMESMatchKeys(t *testing.T) {
 
 		{input: I{}, output: O{}, result: assert.Truef, err: assert.Nilf},
 
-		{input: I{"foo": {}}, output: O{MK{Name: "foo"}}, result: assert.Truef, err: assert.Nilf},
+		{input: I{"foo": {}}, output: O{}, result: assert.Truef, err: assert.Nilf},
 
 		{mes: `
 foo: { op: DoesNotExist }
@@ -339,11 +339,12 @@ bar: { op: Exists }
 			t.Fatalf("failed to parse data of test case #%d (%v): %v", i, tc, err)
 		}
 
-		out, err := mes.MatchGetKeys(tc.input)
+		res, out, err := mes.MatchGetKeys(tc.input)
+		tc.result(t, res, "test case #%d (%v) failed", i, tc)
 		assert.Equalf(t, tc.output, out, "test case #%d (%v) failed", i, tc)
 		tc.err(t, err, "test case #%d (%v) failed", i, tc)
 
-		res, err := mes.MatchKeys(tc.input)
+		res, err = mes.MatchKeys(tc.input)
 		tc.result(t, res, "test case #%d (%v) failed", i, tc)
 		tc.err(t, err, "test case #%d (%v) failed", i, tc)
 	}
@@ -366,7 +367,7 @@ func TestMESMatchValues(t *testing.T) {
 
 		{input: I{}, output: O{}, result: assert.Truef, err: assert.Nilf},
 
-		{input: I{"foo": "bar"}, output: O{MV{Name: "foo", Value: "bar"}}, result: assert.Truef, err: assert.Nilf},
+		{input: I{"foo": "bar"}, output: O{}, result: assert.Truef, err: assert.Nilf},
 
 		{mes: `
 foo: { op: Exists }
@@ -400,11 +401,12 @@ baz: { op: Gt, value: ["10"] }
 			t.Fatalf("failed to parse data of test case #%d (%v): %v", i, tc, err)
 		}
 
-		out, err := mes.MatchGetValues(tc.input)
+		res, out, err := mes.MatchGetValues(tc.input)
+		tc.result(t, res, "test case #%d (%v) failed", i, tc)
 		assert.Equalf(t, tc.output, out, "test case #%d (%v) failed", i, tc)
 		tc.err(t, err, "test case #%d (%v) failed", i, tc)
 
-		res, err := mes.MatchValues(tc.input)
+		res, err = mes.MatchValues(tc.input)
 		tc.result(t, res, "test case #%d (%v) failed", i, tc)
 		tc.err(t, err, "test case #%d (%v) failed", i, tc)
 	}

pkg/apis/nfd/v1alpha1/rule.go

@@ -44,21 +44,21 @@ func (r *Rule) Execute(features feature.Features) (RuleOutput, error) {
 		// Logical OR over the matchAny matchers
 		matched := false
 		for _, matcher := range r.MatchAny {
-			if m, err := matcher.match(features); err != nil {
+			if isMatch, matches, err := matcher.match(features); err != nil {
 				return RuleOutput{}, err
-			} else if m != nil {
+			} else if isMatch {
 				matched = true
-				utils.KlogDump(4, "matches for matchAny "+r.Name, "  ", m)
+				utils.KlogDump(4, "matches for matchAny "+r.Name, "  ", matches)
 
 				if r.labelsTemplate == nil {
 					// No templating so we stop here (further matches would just
 					// produce the same labels)
 					break
 				}
-				if err := r.executeLabelsTemplate(m, labels); err != nil {
+				if err := r.executeLabelsTemplate(matches, labels); err != nil {
 					return RuleOutput{}, err
 				}
-				if err := r.executeVarsTemplate(m, vars); err != nil {
+				if err := r.executeVarsTemplate(matches, vars); err != nil {
 					return RuleOutput{}, err
 				}
 			}
@@ -70,17 +70,17 @@ func (r *Rule) Execute(features feature.Features) (RuleOutput, error) {
 	}
 
 	if len(r.MatchFeatures) > 0 {
-		if m, err := r.MatchFeatures.match(features); err != nil {
+		if isMatch, matches, err := r.MatchFeatures.match(features); err != nil {
 			return RuleOutput{}, err
-		} else if m == nil {
+		} else if !isMatch {
 			klog.V(2).Infof("rule %q did not match", r.Name)
 			return RuleOutput{}, nil
 		} else {
-			utils.KlogDump(4, "matches for matchFeatures "+r.Name, "  ", m)
-			if err := r.executeLabelsTemplate(m, labels); err != nil {
+			utils.KlogDump(4, "matches for matchFeatures "+r.Name, "  ", matches)
+			if err := r.executeLabelsTemplate(matches, labels); err != nil {
 				return RuleOutput{}, err
 			}
-			if err := r.executeVarsTemplate(m, vars); err != nil {
+			if err := r.executeVarsTemplate(matches, vars); err != nil {
 				return RuleOutput{}, err
 			}
 		}
@@ -148,60 +148,60 @@ type matchedFeatures map[string]domainMatchedFeatures
 
 type domainMatchedFeatures map[string]interface{}
 
-func (e *MatchAnyElem) match(features map[string]*feature.DomainFeatures) (matchedFeatures, error) {
+func (e *MatchAnyElem) match(features map[string]*feature.DomainFeatures) (bool, matchedFeatures, error) {
 	return e.MatchFeatures.match(features)
 }
 
-func (m *FeatureMatcher) match(features map[string]*feature.DomainFeatures) (matchedFeatures, error) {
-	ret := make(matchedFeatures, len(*m))
+func (m *FeatureMatcher) match(features map[string]*feature.DomainFeatures) (bool, matchedFeatures, error) {
+	matches := make(matchedFeatures, len(*m))
 
 	// Logical AND over the terms
 	for _, term := range *m {
 		split := strings.SplitN(term.Feature, ".", 2)
 		if len(split) != 2 {
-			return nil, fmt.Errorf("invalid feature %q: must be <domain>.<feature>", term.Feature)
+			return false, nil, fmt.Errorf("invalid feature %q: must be <domain>.<feature>", term.Feature)
 		}
 		domain := split[0]
 		// Ignore case
 		featureName := strings.ToLower(split[1])
 
 		domainFeatures, ok := features[domain]
 		if !ok {
-			return nil, fmt.Errorf("unknown feature source/domain %q", domain)
+			return false, nil, fmt.Errorf("unknown feature source/domain %q", domain)
 		}
 
-		if _, ok := ret[domain]; !ok {
-			ret[domain] = make(domainMatchedFeatures)
+		if _, ok := matches[domain]; !ok {
+			matches[domain] = make(domainMatchedFeatures)
 		}
 
-		var m bool
-		var e error
+		var isMatch bool
+		var err error
 		if f, ok := domainFeatures.Keys[featureName]; ok {
-			v, err := term.MatchExpressions.MatchGetKeys(f.Elements)
-			m = len(v) > 0
-			e = err
-			ret[domain][featureName] = v
+			m, v, e := term.MatchExpressions.MatchGetKeys(f.Elements)
+			isMatch = m
+			err = e
+			matches[domain][featureName] = v
 		} else if f, ok := domainFeatures.Values[featureName]; ok {
-			v, err := term.MatchExpressions.MatchGetValues(f.Elements)
-			m = len(v) > 0
-			e = err
-			ret[domain][featureName] = v
+			m, v, e := term.MatchExpressions.MatchGetValues(f.Elements)
+			isMatch = m
+			err = e
+			matches[domain][featureName] = v
 		} else if f, ok := domainFeatures.Instances[featureName]; ok {
-			v, err := term.MatchExpressions.MatchGetInstances(f.Elements)
-			m = len(v) > 0
-			e = err
-			ret[domain][featureName] = v
+			v, e := term.MatchExpressions.MatchGetInstances(f.Elements)
+			isMatch = len(v) > 0
+			err = e
+			matches[domain][featureName] = v
 		} else {
-			return nil, fmt.Errorf("%q feature of source/domain %q not available", featureName, domain)
+			return false, nil, fmt.Errorf("%q feature of source/domain %q not available", featureName, domain)
 		}
 
-		if e != nil {
-			return nil, e
-		} else if !m {
-			return nil, nil
+		if err != nil {
+			return false, nil, err
+		} else if !isMatch {
+			return false, nil, nil
 		}
 	}
-	return ret, nil
+	return true, matches, nil
 }
 
 type templateHelper struct {

pkg/apis/nfd/v1alpha1/rule_test.go

@@ -82,6 +82,17 @@ func TestRule(t *testing.T) {
 	assert.Nilf(t, err, "unexpected error: %v", err)
 	assert.Equal(t, r1.Labels, m.Labels, "empty matcher should have matched empty features")
 
+	// Test empty MatchExpressions
+	r1.MatchFeatures = FeatureMatcher{
+		FeatureMatcherTerm{
+			Feature:          "domain-1.kf-1",
+			MatchExpressions: MatchExpressionSet{},
+		},
+	}
+	m, err = r1.Execute(f)
+	assert.Nilf(t, err, "unexpected error: %v", err)
+	assert.Equal(t, r1.Labels, m.Labels, "empty match expression set mathces anything")
+
 	// Match "key" features
 	m, err = r2.Execute(f)
 	assert.Nilf(t, err, "unexpected error: %v", err)
@@ -325,8 +336,11 @@ var-2=
 			// We need at least one matcher to match to execute the template.
 			// Use a simple empty matchexpression set to match anything.
 			FeatureMatcherTerm{
-				Feature:          "domain_1.kf_1",
-				MatchExpressions: MatchExpressionSet{},
+				Feature: "domain_1.kf_1",
+				MatchExpressions: MatchExpressionSet{Expressions: Expressions{
+					"key-a": MustCreateMatchExpression(MatchExists),
+				},
+				},
 			},
 		},
 	}