From fbb7303562574283d9abcb03bc14cd78b0809a4f Mon Sep 17 00:00:00 2001 From: Markus Lehtonen Date: Wed, 17 Apr 2024 15:28:58 +0300 Subject: [PATCH] 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. --- .../nodefeaturerule/expression-api_test.go | 63 ++++++++++++++----- pkg/apis/nfd/nodefeaturerule/expression.go | 16 ++--- .../nfd/nodefeaturerule/expression_test.go | 18 +++--- 3 files changed, 62 insertions(+), 35 deletions(-) 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.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() { 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 {