Repository: kubernetes-sigs/node-feature-discovery
Pull request #1670 (2024-04-23 07:30:47)

apis/nfd: no error on ops that never match
Return false (i.e. "did not match") but no error when evaluating a match expression against a "flag" type feature (which don't have any associated value, just the name) if a MatchOp that never matches is used.

This is preparation for supporting multi-type features, i.e. one feature, like "cpu.cpuid", having e.g. "flag" and "attribute" type features.

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/1670#" 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":[]} -->
netlify[bot]: ### <span aria-hidden="true">👷</span> Deploy Preview for *kubernetes-sigs-nfd* processing.


|  Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 9e796ae74e358580d58f7cd9fbec814e8cf84e3c |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/662763aa69cde5000808ee25 |
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1670?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 39.49%. Comparing base [(`81bdf1d`)](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/commit/81bdf1dadb626eaefd3cbed987116839fbe14ba0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) to head [(`9e796ae`)](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1670?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs).

<details><summary>Additional details and impacted files</summary>


[![Impacted file tree graph](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1670/graphs/tree.svg?width=650&height=150&src=pr&token=mEZgtJQ1vg&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs)](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1670?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs)

```diff
@@            Coverage Diff             @@
##           master    #1670      +/-   ##
==========================================
- Coverage   39.55%   39.49%   -0.07%     
==========================================
  Files          80       80              
  Lines        6848     6841       -7     
==========================================
- Hits         2709     2702       -7     
  Misses       3884     3884              
  Partials      255      255              
```


| [Files](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1670?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) | Coverage Δ | |
|---|---|---|
| [pkg/apis/nfd/nodefeaturerule/expression.go](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1670?src=pr&el=tree&filepath=pkg%2Fapis%2Fnfd%2Fnodefeaturerule%2Fexpression.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs#diff-cGtnL2FwaXMvbmZkL25vZGVmZWF0dXJlcnVsZS9leHByZXNzaW9uLmdv) | `71.48% <100.00%> (-0.83%)` | :arrow_down: |

</details>
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1670?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 39.49%. Comparing base [(`81bdf1d`)](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/commit/81bdf1dadb626eaefd3cbed987116839fbe14ba0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) to head [(`9e796ae`)](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1670?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs).

<details><summary>Additional details and impacted files</summary>


[![Impacted file tree graph](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1670/graphs/tree.svg?width=650&height=150&src=pr&token=mEZgtJQ1vg&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs)](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1670?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs)

```diff
@@            Coverage Diff             @@
##           master    #1670      +/-   ##
==========================================
- Coverage   39.55%   39.49%   -0.07%     
==========================================
  Files          80       80              
  Lines        6848     6841       -7     
==========================================
- Hits         2709     2702       -7     
  Misses       3884     3884              
  Partials      255      255              
```


| [Files](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1670?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) | Coverage Δ | |
|---|---|---|
| [pkg/apis/nfd/nodefeaturerule/expression.go](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1670?src=pr&el=tree&filepath=pkg%2Fapis%2Fnfd%2Fnodefeaturerule%2Fexpression.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs#diff-cGtnL2FwaXMvbmZkL25vZGVmZWF0dXJlcnVsZS9leHByZXNzaW9uLmdv) | `71.48% <100.00%> (-0.83%)` | :arrow_down: |

</details>

Patch:
diff --git a/pkg/apis/nfd/nodefeaturerule/expression.go b/pkg/apis/nfd/nodefeaturerule/expression.go
index fbc1e1e56..be7ca3bbb 100644
--- a/pkg/apis/nfd/nodefeaturerule/expression.go
+++ b/pkg/apis/nfd/nodefeaturerule/expression.go
@@ -67,7 +67,7 @@ func evaluateMatchExpression(m *nfdv1alpha1.MatchExpression, valid bool, value i
 		return !valid, nil
 	}
 
-	if valid {
+	if valid && value != nil {
 		value := fmt.Sprintf("%v", value)
 		switch m.Op {
 		case nfdv1alpha1.MatchIn:
@@ -161,18 +161,10 @@ func evaluateMatchExpression(m *nfdv1alpha1.MatchExpression, valid bool, value i
 
 // evaluateMatchExpressionKeys evaluates the MatchExpression against a set of keys.
 func evaluateMatchExpressionKeys(m *nfdv1alpha1.MatchExpression, name string, keys map[string]nfdv1alpha1.Nil) (bool, error) {
-	matched := false
-
 	_, ok := keys[name]
-	switch m.Op {
-	case nfdv1alpha1.MatchAny:
-		matched = true
-	case nfdv1alpha1.MatchExists:
-		matched = ok
-	case nfdv1alpha1.MatchDoesNotExist:
-		matched = !ok
-	default:
-		return false, fmt.Errorf("invalid Op %q when matching keys", m.Op)
+	matched, err := evaluateMatchExpression(m, ok, nil)
+	if err != nil {
+		return false, err
 	}
 
 	if klogV := klog.V(3); klogV.Enabled() {


Test patch:
diff --git a/pkg/apis/nfd/nodefeaturerule/expression-api_test.go b/pkg/apis/nfd/nodefeaturerule/expression-api_test.go
index d33ad355b..e03e94187 100644
--- a/pkg/apis/nfd/nodefeaturerule/expression-api_test.go
+++ b/pkg/apis/nfd/nodefeaturerule/expression-api_test.go
@@ -43,35 +43,70 @@ func TestMatchKeys(t *testing.T) {
 	}
 
 	tcs := []TC{
-		{output: O{}, result: assert.True, err: assert.Nil},
-
-		{input: I{}, output: O{}, result: assert.True, err: assert.Nil},
-
-		{input: I{"foo": {}}, output: O{}, result: assert.True, err: assert.Nil},
-
-		{mes: `
+		{
+			name:   "empty expression and nil input",
+			output: O{},
+			result: assert.True,
+			err:    assert.Nil,
+		},
+		{
+			name:   "empty expression and empty input",
+			input:  I{},
+			output: O{},
+			result: assert.True,
+			err:    assert.Nil,
+		},
+		{
+			name:   "empty expression with non-empty input",
+			input:  I{"foo": {}},
+			output: O{},
+			result: assert.True,
+			err:    assert.Nil,
+		},
+		{
+			name: "expressions match",
+			mes: `
 foo: { op: DoesNotExist }
 bar: { op: Exists }
 `,
 			input:  I{"bar": {}, "baz": {}, "buzz": {}},
 			output: O{{"Name": "bar"}, {"Name": "foo"}},
-			result: assert.True, err: assert.Nil},
-
-		{mes: `
+			result: assert.True,
+			err:    assert.Nil,
+		},
+		{
+			name: "expression does not match",
+			mes: `
 foo: { op: DoesNotExist }
 bar: { op: Exists }
 `,
 			input:  I{"foo": {}, "bar": {}, "baz": {}},
 			output: nil,
-			result: assert.False, err: assert.Nil},
-
-		{mes: `
+			result: assert.False,
+			err:    assert.Nil,
+		},
+		{
+			name: "op that never matches",
+			mes: `
 foo: { op: In, value: ["bar"] }
 bar: { op: Exists }
 `,
 			input:  I{"bar": {}, "baz": {}},
 			output: nil,
-			result: assert.False, err: assert.NotNil},
+			result: assert.False,
+			err:    assert.Nil,
+		},
+		{
+			name: "error in expression",
+			mes: `
+foo: { op: Exists, value: ["bar"] }
+bar: { op: Exists }
+`,
+			input:  I{"bar": {}},
+			output: nil,
+			result: assert.False,
+			err:    assert.NotNil,
+		},
 	}
 
 	for _, tc := range tcs {
diff --git a/pkg/apis/nfd/nodefeaturerule/expression_test.go b/pkg/apis/nfd/nodefeaturerule/expression_test.go
index 931cd72b9..8d3a77372 100644
--- a/pkg/apis/nfd/nodefeaturerule/expression_test.go
+++ b/pkg/apis/nfd/nodefeaturerule/expression_test.go
@@ -171,15 +171,15 @@ func TestEvaluateMatchExpressionKeys(t *testing.T) {
 		{name: "7", op: nfdv1alpha1.MatchDoesNotExist, key: "foo", input: I{"bar": {}}, result: assert.True, err: assert.Nil},
 		{name: "8", op: nfdv1alpha1.MatchDoesNotExist, key: "foo", input: I{"bar": {}, "foo": {}}, result: assert.False, err: assert.Nil},
 
-		// All other ops should return an error
-		{name: "9", op: nfdv1alpha1.MatchIn, values: V{"foo"}, key: "foo", result: assert.False, err: assert.NotNil},
-		{name: "10", op: nfdv1alpha1.MatchNotIn, values: V{"foo"}, key: "foo", result: assert.False, err: assert.NotNil},
-		{name: "11", op: nfdv1alpha1.MatchInRegexp, values: V{"foo"}, key: "foo", result: assert.False, err: assert.NotNil},
-		{name: "12", op: nfdv1alpha1.MatchGt, values: V{"1"}, key: "foo", result: assert.False, err: assert.NotNil},
-		{name: "13", op: nfdv1alpha1.MatchLt, values: V{"1"}, key: "foo", result: assert.False, err: assert.NotNil},
-		{name: "14", op: nfdv1alpha1.MatchGtLt, values: V{"1", "10"}, key: "foo", result: assert.False, err: assert.NotNil},
-		{name: "15", op: nfdv1alpha1.MatchIsTrue, key: "foo", result: assert.False, err: assert.NotNil},
-		{name: "16", op: nfdv1alpha1.MatchIsFalse, key: "foo", result: assert.False, err: assert.NotNil},
+		// All other ops should be nop (and return false) for "key" features
+		{name: "9", op: nfdv1alpha1.MatchIn, values: V{"foo"}, key: "foo", result: assert.False, err: assert.Nil},
+		{name: "10", op: nfdv1alpha1.MatchNotIn, values: V{"foo"}, key: "foo", result: assert.False, err: assert.Nil},
+		{name: "11", op: nfdv1alpha1.MatchInRegexp, values: V{"foo"}, key: "foo", result: assert.False, err: assert.Nil},
+		{name: "12", op: nfdv1alpha1.MatchGt, values: V{"1"}, key: "foo", result: assert.False, err: assert.Nil},
+		{name: "13", op: nfdv1alpha1.MatchLt, values: V{"1"}, key: "foo", result: assert.False, err: assert.Nil},
+		{name: "14", op: nfdv1alpha1.MatchGtLt, values: V{"1", "10"}, key: "foo", result: assert.False, err: assert.Nil},
+		{name: "15", op: nfdv1alpha1.MatchIsTrue, key: "foo", result: assert.False, err: assert.Nil},
+		{name: "16", op: nfdv1alpha1.MatchIsFalse, key: "foo", result: assert.False, err: assert.Nil},
 	}
 
 	for _, tc := range tcs {