From 63c22551df210143c8297f96c65afbafbea8f049 Mon Sep 17 00:00:00 2001 From: Markus Lehtonen Date: Wed, 22 Nov 2023 09:45:48 +0200 Subject: [PATCH] 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: ... 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. --- pkg/apis/nfd/v1alpha1/expression.go | 36 ++++--------- pkg/apis/nfd/v1alpha1/expression_test.go | 16 +++--- pkg/apis/nfd/v1alpha1/rule.go | 26 ++++------ pkg/apis/nfd/v1alpha1/rule_test.go | 20 +++++++- .../nfd/v1alpha1/zz_generated.deepcopy.go | 51 ------------------- 5 files changed, 48 insertions(+), 101 deletions(-) diff --git a/pkg/apis/nfd/v1alpha1/expression.go b/pkg/apis/nfd/v1alpha1/expression.go index 026a8ef529..1d997efc0f 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/expression_test.go b/pkg/apis/nfd/v1alpha1/expression_test.go index aeb5f36380..694871fbed 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.go b/pkg/apis/nfd/v1alpha1/rule.go index 53b9ad188b..3ac58658bc 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/rule_test.go b/pkg/apis/nfd/v1alpha1/rule_test.go index 7314c293a0..97f99137e8 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", diff --git a/pkg/apis/nfd/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/nfd/v1alpha1/zz_generated.deepcopy.go index 971600f9d7..b114b94277 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