Repository: kubernetes-sigs/node-feature-discovery
Pull request #1468 (2023-11-22 08:41:22)

apis/nfd: fix multiple matcher terms targeting the same feature
Fix NodeFeatureRule templating in cases where multiple matchFeatures terms are targeting the same feature. Previously, only matched feature elements from the last matcher terms were used as the input to the template. However, the input should contain all matched elements from all matcher terms.

For example, consider the example rule snippet below:

```yaml
  ...
  labelsTemplate: |
    {{ range .pci.device }}vendor.io/pci-device.{{ .class }}-{{ .device }}=exists
    {{ end }}
  matchFeatures:
    - feature: pci.device
      matchExpressions:
        class: {op: InRegexp, value: ["^03"]}
        vendor: {op: In, value: ["1234"]}
    - feature: pci.device
      matchExpressions:
        class: {op: InRegexp, value: ["^12"]}
```

This rule matches if both a pci device of class `03` from vendor `1234` exists and a pci device of class `12` (from any vendor) exists. Previously, the template would only generate labels from the devices in class `12` (as that's the last term). With this patch the template creates device labels from devices in both classes `03` and `12`.

netlify[bot]: ### <span aria-hidden="true">👷</span> Deploy Preview for *kubernetes-sigs-nfd* processing.


|  Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | d78cadbff9fe359335be674c31c5aeecaf9b3799 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/655dbeb5be05e400089147a9 |
k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/node-feature-discovery/pull/1468#" title="Author self-approved">marquiz</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fnode-feature-discovery).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/node-feature-discovery/blob/master/OWNERS)~~ [marquiz]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
marquiz: /assign @ArangoGutierrez 

Patch:
diff --git a/pkg/apis/nfd/v1alpha1/expression.go b/pkg/apis/nfd/v1alpha1/expression.go
index 026a8ef52..1d997efc0 100644
--- a/pkg/apis/nfd/v1alpha1/expression.go
+++ b/pkg/apis/nfd/v1alpha1/expression.go
@@ -288,16 +288,15 @@ func (m *MatchExpressionSet) MatchKeys(keys map[string]Nil) (bool, error) {
 	return matched, err
 }
 
-// MatchedKey holds one matched key.
-type MatchedKey struct {
-	Name string
-}
+// MatchedElement holds one matched Instance.
+// +k8s:deepcopy-gen=false
+type MatchedElement map[string]string
 
 // MatchGetKeys evaluates the MatchExpressionSet against a set of keys and
 // returns all matched keys or nil if no match was found. Note that an empty
 // MatchExpressionSet returns a match with an empty slice of matched features.
-func (m *MatchExpressionSet) MatchGetKeys(keys map[string]Nil) (bool, []MatchedKey, error) {
-	ret := make([]MatchedKey, 0, len(*m))
+func (m *MatchExpressionSet) MatchGetKeys(keys map[string]Nil) (bool, []MatchedElement, error) {
+	ret := make([]MatchedElement, 0, len(*m))
 
 	for n, e := range *m {
 		match, err := e.MatchKeys(n, keys)
@@ -307,10 +306,8 @@ func (m *MatchExpressionSet) MatchGetKeys(keys map[string]Nil) (bool, []MatchedK
 		if !match {
 			return false, nil, nil
 		}
-		ret = append(ret, MatchedKey{Name: n})
+		ret = append(ret, MatchedElement{"Name": n})
 	}
-	// Sort for reproducible output
-	sort.Slice(ret, func(i, j int) bool { return ret[i].Name < ret[j].Name })
 	return true, ret, nil
 }
 
@@ -320,17 +317,11 @@ func (m *MatchExpressionSet) MatchValues(values map[string]string) (bool, error)
 	return matched, err
 }
 
-// MatchedValue holds one matched key-value pair.
-type MatchedValue struct {
-	Name  string
-	Value string
-}
-
 // MatchGetValues evaluates the MatchExpressionSet against a set of key-value
 // pairs and returns all matched key-value pairs. Note that an empty
 // MatchExpressionSet returns a match with an empty slice of matched features.
-func (m *MatchExpressionSet) MatchGetValues(values map[string]string) (bool, []MatchedValue, error) {
-	ret := make([]MatchedValue, 0, len(*m))
+func (m *MatchExpressionSet) MatchGetValues(values map[string]string) (bool, []MatchedElement, error) {
+	ret := make([]MatchedElement, 0, len(*m))
 
 	for n, e := range *m {
 		match, err := e.MatchValues(n, values)
@@ -340,10 +331,8 @@ func (m *MatchExpressionSet) MatchGetValues(values map[string]string) (bool, []M
 		if !match {
 			return false, nil, nil
 		}
-		ret = append(ret, MatchedValue{Name: n, Value: values[n]})
+		ret = append(ret, MatchedElement{"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 true, ret, nil
 }
 
@@ -355,15 +344,12 @@ func (m *MatchExpressionSet) MatchInstances(instances []InstanceFeature) (bool,
 	return len(v) > 0, err
 }
 
-// MatchedInstance holds one matched Instance.
-type MatchedInstance map[string]string
-
 // MatchGetInstances evaluates the MatchExpressionSet against a set of instance
 // features, each of which is an individual set of key-value pairs
 // (attributes). A slice containing all matching instances is returned. An
 // empty (non-nil) slice is returned if no matching instances were found.
-func (m *MatchExpressionSet) MatchGetInstances(instances []InstanceFeature) ([]MatchedInstance, error) {
-	ret := []MatchedInstance{}
+func (m *MatchExpressionSet) MatchGetInstances(instances []InstanceFeature) ([]MatchedElement, error) {
+	ret := []MatchedElement{}
 
 	for _, i := range instances {
 		if match, err := m.MatchValues(i.Attributes); err != nil {
diff --git a/pkg/apis/nfd/v1alpha1/rule.go b/pkg/apis/nfd/v1alpha1/rule.go
index 53b9ad188..3ac58658b 100644
--- a/pkg/apis/nfd/v1alpha1/rule.go
+++ b/pkg/apis/nfd/v1alpha1/rule.go
@@ -155,7 +155,7 @@ func (r *Rule) executeVarsTemplate(in matchedFeatures, out map[string]string) er
 
 type matchedFeatures map[string]domainMatchedFeatures
 
-type domainMatchedFeatures map[string]interface{}
+type domainMatchedFeatures map[string][]MatchedElement
 
 func (e *MatchAnyElem) match(features *Features) (bool, matchedFeatures, error) {
 	return e.MatchFeatures.match(features)
@@ -175,30 +175,26 @@ func (m *FeatureMatcher) match(features *Features) (bool, matchedFeatures, error
 			nameSplit = []string{featureName, ""}
 		}
 
-		if _, ok := matches[nameSplit[0]]; !ok {
-			matches[nameSplit[0]] = make(domainMatchedFeatures)
+		dom := nameSplit[0]
+		nam := nameSplit[1]
+		if _, ok := matches[dom]; !ok {
+			matches[dom] = make(domainMatchedFeatures)
 		}
 
 		var isMatch bool
+		var matchedElems []MatchedElement
 		var err error
 		if f, ok := features.Flags[featureName]; ok {
-			m, v, e := term.MatchExpressions.MatchGetKeys(f.Elements)
-			isMatch = m
-			err = e
-			matches[nameSplit[0]][nameSplit[1]] = v
+			isMatch, matchedElems, err = term.MatchExpressions.MatchGetKeys(f.Elements)
 		} else if f, ok := features.Attributes[featureName]; ok {
-			m, v, e := term.MatchExpressions.MatchGetValues(f.Elements)
-			isMatch = m
-			err = e
-			matches[nameSplit[0]][nameSplit[1]] = v
+			isMatch, matchedElems, err = term.MatchExpressions.MatchGetValues(f.Elements)
 		} else if f, ok := features.Instances[featureName]; ok {
-			v, e := term.MatchExpressions.MatchGetInstances(f.Elements)
-			isMatch = len(v) > 0
-			err = e
-			matches[nameSplit[0]][nameSplit[1]] = v
+			matchedElems, err = term.MatchExpressions.MatchGetInstances(f.Elements)
+			isMatch = len(matchedElems) > 0
 		} else {
 			return false, nil, fmt.Errorf("feature %q not available", featureName)
 		}
+		matches[dom][nam] = append(matches[dom][nam], matchedElems...)
 
 		if err != nil {
 			return false, nil, err
diff --git a/pkg/apis/nfd/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/nfd/v1alpha1/zz_generated.deepcopy.go
index 971600f9d..b114b9427 100644
--- a/pkg/apis/nfd/v1alpha1/zz_generated.deepcopy.go
+++ b/pkg/apis/nfd/v1alpha1/zz_generated.deepcopy.go
@@ -276,57 +276,6 @@ func (in MatchValue) DeepCopy() MatchValue {
 	return *out
 }
 
-// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
-func (in MatchedInstance) DeepCopyInto(out *MatchedInstance) {
-	{
-		in := &in
-		*out = make(MatchedInstance, len(*in))
-		for key, val := range *in {
-			(*out)[key] = val
-		}
-	}
-}
-
-// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new MatchedInstance.
-func (in MatchedInstance) DeepCopy() MatchedInstance {
-	if in == nil {
-		return nil
-	}
-	out := new(MatchedInstance)
-	in.DeepCopyInto(out)
-	return *out
-}
-
-// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
-func (in *MatchedKey) DeepCopyInto(out *MatchedKey) {
-	*out = *in
-}
-
-// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new MatchedKey.
-func (in *MatchedKey) DeepCopy() *MatchedKey {
-	if in == nil {
-		return nil
-	}
-	out := new(MatchedKey)
-	in.DeepCopyInto(out)
-	return out
-}
-
-// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
-func (in *MatchedValue) DeepCopyInto(out *MatchedValue) {
-	*out = *in
-}
-
-// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new MatchedValue.
-func (in *MatchedValue) DeepCopy() *MatchedValue {
-	if in == nil {
-		return nil
-	}
-	out := new(MatchedValue)
-	in.DeepCopyInto(out)
-	return out
-}
-
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *Nil) DeepCopyInto(out *Nil) {
 	*out = *in


Test patch:
diff --git a/pkg/apis/nfd/v1alpha1/expression_test.go b/pkg/apis/nfd/v1alpha1/expression_test.go
index aeb5f3638..694871fbe 100644
--- a/pkg/apis/nfd/v1alpha1/expression_test.go
+++ b/pkg/apis/nfd/v1alpha1/expression_test.go
@@ -290,8 +290,8 @@ func TestMatchValues(t *testing.T) {
 
 func TestMESMatchKeys(t *testing.T) {
 	type I = map[string]api.Nil
-	type MK = api.MatchedKey
-	type O = []MK
+	type ME = api.MatchedElement
+	type O = []ME
 	type TC struct {
 		mes    string
 		input  I
@@ -312,7 +312,7 @@ foo: { op: DoesNotExist }
 bar: { op: Exists }
 `,
 			input:  I{"bar": {}, "baz": {}, "buzz": {}},
-			output: O{MK{Name: "bar"}, MK{Name: "foo"}},
+			output: O{ME{"Name": "bar"}, ME{"Name": "foo"}},
 			result: assert.Truef, err: assert.Nilf},
 
 		{mes: `
@@ -351,8 +351,8 @@ bar: { op: Exists }
 
 func TestMESMatchValues(t *testing.T) {
 	type I = map[string]string
-	type MV = api.MatchedValue
-	type O = []MV
+	type ME = api.MatchedElement
+	type O = []ME
 	type TC struct {
 		mes    string
 		input  I
@@ -382,7 +382,7 @@ bar: { op: In, value: ["val", "wal"] }
 baz: { op: Gt, value: ["10"] }
 `,
 			input:  I{"foo": "1", "bar": "val", "baz": "123", "buzz": "light"},
-			output: O{MV{Name: "bar", Value: "val"}, MV{Name: "baz", Value: "123"}, MV{Name: "foo", Value: "1"}},
+			output: O{ME{"Name": "bar", "Value": "val"}, ME{"Name": "baz", "Value": "123"}, ME{"Name": "foo", "Value": "1"}},
 			result: assert.Truef, err: assert.Nilf},
 
 		{mes: `
@@ -413,8 +413,8 @@ baz: { op: Gt, value: ["10"] }
 
 func TestMESMatchInstances(t *testing.T) {
 	type I = api.InstanceFeature
-	type MI = api.MatchedInstance
-	type O = []MI
+	type ME = api.MatchedElement
+	type O = []ME
 	type A = map[string]string
 	type TC struct {
 		mes    string
diff --git a/pkg/apis/nfd/v1alpha1/rule_test.go b/pkg/apis/nfd/v1alpha1/rule_test.go
index 7314c293a..97f99137e 100644
--- a/pkg/apis/nfd/v1alpha1/rule_test.go
+++ b/pkg/apis/nfd/v1alpha1/rule_test.go
@@ -246,6 +246,13 @@ func TestTemplating(t *testing.T) {
 							"attr-2": "val-200",
 						},
 					},
+					{
+						Attributes: map[string]string{
+							"attr-1": "1000",
+							"attr-2": "val-2000",
+							"attr-3": "3000",
+						},
+					},
 				},
 			},
 		},
@@ -290,6 +297,14 @@ var-2=
 					"attr-1": MustCreateMatchExpression(MatchLt, "100"),
 				},
 			},
+			FeatureMatcherTerm{
+				Feature: "domain_1.if_1",
+				MatchExpressions: MatchExpressionSet{
+					"attr-1": MustCreateMatchExpression(MatchExists),
+					"attr-2": MustCreateMatchExpression(MatchExists),
+					"attr-3": MustCreateMatchExpression(MatchExists),
+				},
+			},
 		},
 	}
 
@@ -309,8 +324,9 @@ var-2=
 		"vf-key-1": "vf-val-1",
 		"vf-bar":   "vf-",
 		// From if_1 template
-		"if-1_val-2":   "present",
-		"if-10_val-20": "present",
+		"if-1_val-2":       "present",
+		"if-10_val-20":     "present",
+		"if-1000_val-2000": "present",
 	}
 	expectedVars := map[string]string{
 		"var-1": "var-val-1",