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.
kubernetes-sigs · Apr 23, 2024 · fbb7303 · fbb7303
1 parent 81bdf1d
commit fbb7303
Show file tree

Hide file tree

Showing 3 changed files with 62 additions and 35 deletions.
diff --git 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.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() {

diff --git 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 {