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

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),
+				},
+				},
 			},
 		},
 	}