Repository: kubernetes-sigs/node-feature-discovery
Pull request #654 (2021-11-16 16:30:54)

source/custom: move rule matching to pkg/apis/nfd
Move the rule processing of matchFeatures and matchAny from
source/custom package over to pkg/apis/nfd, aiming to better integrity
and re-usability of the code. Does not change the API as such, just adds
more supportive functions.

Depends/builds on #653

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/654#" 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":[]} -->
zvonkok: #653 merged 
/unhold

Patch:
diff --git a/pkg/apis/nfd/v1alpha1/rule.go b/pkg/apis/nfd/v1alpha1/rule.go
new file mode 100644
index 000000000..fcb07a13a
--- /dev/null
+++ b/pkg/apis/nfd/v1alpha1/rule.go
@@ -0,0 +1,100 @@
+/*
+Copyright 2021 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1alpha1
+
+import (
+	"strings"
+
+	"fmt"
+
+	"sigs.k8s.io/node-feature-discovery/pkg/api/feature"
+)
+
+// Execute the rule against a set of input features.
+func (r *Rule) Execute(features map[string]*feature.DomainFeatures) (map[string]string, error) {
+	if len(r.MatchAny) > 0 {
+		// Logical OR over the matchAny matchers
+		matched := false
+		for _, matcher := range r.MatchAny {
+			if m, err := matcher.match(features); err != nil {
+				return nil, err
+			} else if m {
+				matched = true
+				break
+			}
+		}
+		if !matched {
+			return nil, nil
+		}
+	}
+
+	if len(r.MatchFeatures) > 0 {
+		if m, err := r.MatchFeatures.match(features); err != nil {
+			return nil, err
+		} else if !m {
+			return nil, nil
+		}
+	}
+
+	labels := make(map[string]string, len(r.Labels))
+	for k, v := range r.Labels {
+		labels[k] = v
+	}
+
+	return labels, nil
+}
+
+func (e *MatchAnyElem) match(features map[string]*feature.DomainFeatures) (bool, error) {
+	return e.MatchFeatures.match(features)
+}
+
+func (m *FeatureMatcher) match(features map[string]*feature.DomainFeatures) (bool, error) {
+	// Logical AND over the terms
+	for _, term := range *m {
+		split := strings.SplitN(term.Feature, ".", 2)
+		if len(split) != 2 {
+			return false, fmt.Errorf("invalid selector %q: must be <domain>.<feature>", term.Feature)
+		}
+		domain := split[0]
+		// Ignore case
+		featureName := strings.ToLower(split[1])
+
+		domainFeatures, ok := features[domain]
+		if !ok {
+			return false, fmt.Errorf("unknown feature source/domain %q", domain)
+		}
+
+		var m bool
+		var err error
+		if f, ok := domainFeatures.Keys[featureName]; ok {
+			m, err = term.MatchExpressions.MatchKeys(f.Elements)
+		} else if f, ok := domainFeatures.Values[featureName]; ok {
+			m, err = term.MatchExpressions.MatchValues(f.Elements)
+		} else if f, ok := domainFeatures.Instances[featureName]; ok {
+			m, err = term.MatchExpressions.MatchInstances(f.Elements)
+		} else {
+			return false, fmt.Errorf("%q feature of source/domain %q not available", featureName, domain)
+		}
+
+		if err != nil {
+			return false, err
+		} else if !m {
+			return false, nil
+		}
+	}
+	return true, nil
+}
diff --git a/source/custom/custom.go b/source/custom/custom.go
index 84b5a502f..cde8d167c 100644
--- a/source/custom/custom.go
+++ b/source/custom/custom.go
@@ -51,21 +51,7 @@ type LegacyRule struct {
 }
 
 type Rule struct {
-	Name          string            `json:"name"`
-	Labels        map[string]string `json:"labels"`
-	MatchFeatures FeatureMatcher    `json:"matchFeatures"`
-	MatchAny      []MatchAnyElem    `json:"matchAny"`
-}
-
-type MatchAnyElem struct {
-	MatchFeatures FeatureMatcher
-}
-
-type FeatureMatcher []FeatureMatcherTerm
-
-type FeatureMatcherTerm struct {
-	Feature          string
-	MatchExpressions nfdv1alpha1.MatchExpressionSet
+	nfdv1alpha1.Rule
 }
 
 type config []CustomRule
@@ -155,7 +141,7 @@ func (r *CustomRule) execute(features map[string]*feature.DomainFeatures) (map[s
 	}
 
 	if r.Rule != nil {
-		ruleOut, err := r.Rule.execute(features)
+		ruleOut, err := r.Rule.Execute(features)
 		if err != nil {
 			return nil, fmt.Errorf("failed to execute rule %s: %w", r.Rule.Name, err)
 		}
@@ -189,80 +175,6 @@ func (r *LegacyRule) execute(features map[string]*feature.DomainFeatures) (map[s
 	return map[string]string{r.Name: value}, nil
 }
 
-func (r *Rule) execute(features map[string]*feature.DomainFeatures) (map[string]string, error) {
-	if len(r.MatchAny) > 0 {
-		// Logical OR over the matchAny matchers
-		matched := false
-		for _, matcher := range r.MatchAny {
-			if m, err := matcher.match(features); err != nil {
-				return nil, err
-			} else if m {
-				matched = true
-				break
-			}
-		}
-		if !matched {
-			return nil, nil
-		}
-	}
-
-	if len(r.MatchFeatures) > 0 {
-		if m, err := r.MatchFeatures.match(features); err != nil {
-			return nil, err
-		} else if !m {
-			return nil, nil
-		}
-	}
-
-	labels := make(map[string]string, len(r.Labels))
-	for k, v := range r.Labels {
-		labels[k] = v
-	}
-
-	return labels, nil
-}
-
-func (e *MatchAnyElem) match(features map[string]*feature.DomainFeatures) (bool, error) {
-	return e.MatchFeatures.match(features)
-}
-
-func (m *FeatureMatcher) match(features map[string]*feature.DomainFeatures) (bool, error) {
-	// Logical AND over the terms
-	for _, term := range *m {
-		split := strings.SplitN(term.Feature, ".", 2)
-		if len(split) != 2 {
-			return false, fmt.Errorf("invalid selector %q: must be <domain>.<feature>", term.Feature)
-		}
-		domain := split[0]
-		// Ignore case
-		featureName := strings.ToLower(split[1])
-
-		domainFeatures, ok := features[domain]
-		if !ok {
-			return false, fmt.Errorf("unknown feature source/domain %q", domain)
-		}
-
-		var m bool
-		var err error
-		if f, ok := domainFeatures.Keys[featureName]; ok {
-			m, err = term.MatchExpressions.MatchKeys(f.Elements)
-		} else if f, ok := domainFeatures.Values[featureName]; ok {
-			m, err = term.MatchExpressions.MatchValues(f.Elements)
-		} else if f, ok := domainFeatures.Instances[featureName]; ok {
-			m, err = term.MatchExpressions.MatchInstances(f.Elements)
-		} else {
-			return false, fmt.Errorf("%q feature of source/domain %q not available", featureName, domain)
-		}
-
-		if err != nil {
-			return false, err
-		} else if !m {
-			return false, nil
-		}
-	}
-	return true, nil
-}
-
 func (m *LegacyMatcher) match() (bool, error) {
 	allRules := []legacyRule{
 		m.PciID,


Test patch:
diff --git a/source/custom/custom_test.go b/pkg/apis/nfd/v1alpha1/rule_test.go
similarity index 69%
rename from source/custom/custom_test.go
rename to pkg/apis/nfd/v1alpha1/rule_test.go
index 7d4f6f8d7..b4c574203 100644
--- a/source/custom/custom_test.go
+++ b/pkg/apis/nfd/v1alpha1/rule_test.go
@@ -14,14 +14,13 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package custom
+package v1alpha1
 
 import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"sigs.k8s.io/node-feature-discovery/pkg/api/feature"
-	nfdv1alpha1 "sigs.k8s.io/node-feature-discovery/pkg/apis/nfd/v1alpha1"
 )
 
 func TestRule(t *testing.T) {
@@ -32,32 +31,30 @@ func TestRule(t *testing.T) {
 		MatchFeatures: FeatureMatcher{
 			FeatureMatcherTerm{
 				Feature: "domain-1.kf-1",
-				MatchExpressions: nfdv1alpha1.MatchExpressionSet{
-					Expressions: nfdv1alpha1.Expressions{
-						"key-1": nfdv1alpha1.MustCreateMatchExpression(nfdv1alpha1.MatchExists),
-					},
+				MatchExpressions: MatchExpressionSet{
+					Expressions: Expressions{"key-1": MustCreateMatchExpression(MatchExists)},
 				},
 			},
 		},
 	}
 
 	// Test totally empty features
-	m, err := r1.execute(f)
+	m, err := r1.Execute(f)
 	assert.Nilf(t, err, "unexpected error: %v", err)
 	assert.Equal(t, r1.Labels, m, "empty matcher should have matched empty features")
 
-	_, err = r2.execute(f)
+	_, err = r2.Execute(f)
 	assert.Error(t, err, "matching agains a missing domain should have returned an error")
 
 	// Test empty domain
 	d := feature.NewDomainFeatures()
 	f["domain-1"] = d
 
-	m, err = r1.execute(f)
+	m, err = r1.Execute(f)
 	assert.Nilf(t, err, "unexpected error: %v", err)
 	assert.Equal(t, r1.Labels, m, "empty matcher should have matched empty features")
 
-	_, err = r2.execute(f)
+	_, err = r2.Execute(f)
 	assert.Error(t, err, "matching agains a missing feature type should have returned an error")
 
 	// Test empty feature sets
@@ -65,11 +62,11 @@ func TestRule(t *testing.T) {
 	d.Values["vf-1"] = feature.NewValueFeatures(nil)
 	d.Instances["if-1"] = feature.NewInstanceFeatures(nil)
 
-	m, err = r1.execute(f)
+	m, err = r1.Execute(f)
 	assert.Nilf(t, err, "unexpected error: %v", err)
 	assert.Equal(t, r1.Labels, m, "empty matcher should have matched empty features")
 
-	m, err = r2.execute(f)
+	m, err = r2.Execute(f)
 	assert.Nilf(t, err, "unexpected error: %v", err)
 	assert.Nil(t, m, "unexpected match")
 
@@ -79,17 +76,17 @@ func TestRule(t *testing.T) {
 	d.Instances["if-1"] = feature.NewInstanceFeatures([]feature.InstanceFeature{
 		*feature.NewInstanceFeature(map[string]string{"attr-1": "val-x"})})
 
-	m, err = r1.execute(f)
+	m, err = r1.Execute(f)
 	assert.Nilf(t, err, "unexpected error: %v", err)
 	assert.Equal(t, r1.Labels, m, "empty matcher should have matched empty features")
 
 	// Match "key" features
-	m, err = r2.execute(f)
+	m, err = r2.Execute(f)
 	assert.Nilf(t, err, "unexpected error: %v", err)
 	assert.Nil(t, m, "keys should not have matched")
 
 	d.Keys["kf-1"].Elements["key-1"] = feature.Nil{}
-	m, err = r2.execute(f)
+	m, err = r2.Execute(f)
 	assert.Nilf(t, err, "unexpected error: %v", err)
 	assert.Equal(t, r2.Labels, m, "keys should have matched")
 
@@ -99,20 +96,18 @@ func TestRule(t *testing.T) {
 		MatchFeatures: FeatureMatcher{
 			FeatureMatcherTerm{
 				Feature: "domain-1.vf-1",
-				MatchExpressions: nfdv1alpha1.MatchExpressionSet{
-					Expressions: nfdv1alpha1.Expressions{
-						"key-1": nfdv1alpha1.MustCreateMatchExpression(nfdv1alpha1.MatchIn, "val-1"),
-					},
+				MatchExpressions: MatchExpressionSet{
+					Expressions: Expressions{"key-1": MustCreateMatchExpression(MatchIn, "val-1")},
 				},
 			},
 		},
 	}
-	m, err = r3.execute(f)
+	m, err = r3.Execute(f)
 	assert.Nilf(t, err, "unexpected error: %v", err)
 	assert.Nil(t, m, "values should not have matched")
 
 	d.Values["vf-1"].Elements["key-1"] = "val-1"
-	m, err = r3.execute(f)
+	m, err = r3.Execute(f)
 	assert.Nilf(t, err, "unexpected error: %v", err)
 	assert.Equal(t, r3.Labels, m, "values should have matched")
 
@@ -122,20 +117,18 @@ func TestRule(t *testing.T) {
 		MatchFeatures: FeatureMatcher{
 			FeatureMatcherTerm{
 				Feature: "domain-1.if-1",
-				MatchExpressions: nfdv1alpha1.MatchExpressionSet{
-					Expressions: nfdv1alpha1.Expressions{
-						"attr-1": nfdv1alpha1.MustCreateMatchExpression(nfdv1alpha1.MatchIn, "val-1"),
-					},
+				MatchExpressions: MatchExpressionSet{
+					Expressions: Expressions{"attr-1": MustCreateMatchExpression(MatchIn, "val-1")},
 				},
 			},
 		},
 	}
-	m, err = r4.execute(f)
+	m, err = r4.Execute(f)
 	assert.Nilf(t, err, "unexpected error: %v", err)
 	assert.Nil(t, m, "instances should not have matched")
 
 	d.Instances["if-1"].Elements[0].Attributes["attr-1"] = "val-1"
-	m, err = r4.execute(f)
+	m, err = r4.Execute(f)
 	assert.Nilf(t, err, "unexpected error: %v", err)
 	assert.Equal(t, r4.Labels, m, "instances should have matched")
 
@@ -145,28 +138,24 @@ func TestRule(t *testing.T) {
 		MatchFeatures: FeatureMatcher{
 			FeatureMatcherTerm{
 				Feature: "domain-1.vf-1",
-				MatchExpressions: nfdv1alpha1.MatchExpressionSet{
-					Expressions: nfdv1alpha1.Expressions{
-						"key-1": nfdv1alpha1.MustCreateMatchExpression(nfdv1alpha1.MatchIn, "val-x"),
-					},
+				MatchExpressions: MatchExpressionSet{
+					Expressions: Expressions{"key-1": MustCreateMatchExpression(MatchIn, "val-x")},
 				},
 			},
 			FeatureMatcherTerm{
 				Feature: "domain-1.if-1",
-				MatchExpressions: nfdv1alpha1.MatchExpressionSet{
-					Expressions: nfdv1alpha1.Expressions{
-						"attr-1": nfdv1alpha1.MustCreateMatchExpression(nfdv1alpha1.MatchIn, "val-1"),
-					},
+				MatchExpressions: MatchExpressionSet{
+					Expressions: Expressions{"attr-1": MustCreateMatchExpression(MatchIn, "val-1")},
 				},
 			},
 		},
 	}
-	m, err = r5.execute(f)
+	m, err = r5.Execute(f)
 	assert.Nilf(t, err, "unexpected error: %v", err)
 	assert.Nil(t, m, "instances should not have matched")
 
-	r5.MatchFeatures[0].MatchExpressions.Expressions["key-1"] = nfdv1alpha1.MustCreateMatchExpression(nfdv1alpha1.MatchIn, "val-1")
-	m, err = r5.execute(f)
+	r5.MatchFeatures[0].MatchExpressions.Expressions["key-1"] = MustCreateMatchExpression(MatchIn, "val-1")
+	m, err = r5.Execute(f)
 	assert.Nilf(t, err, "unexpected error: %v", err)
 	assert.Equal(t, r5.Labels, m, "instances should have matched")
 
@@ -176,16 +165,14 @@ func TestRule(t *testing.T) {
 			MatchFeatures: FeatureMatcher{
 				FeatureMatcherTerm{
 					Feature: "domain-1.kf-1",
-					MatchExpressions: nfdv1alpha1.MatchExpressionSet{
-						Expressions: nfdv1alpha1.Expressions{
-							"key-na": nfdv1alpha1.MustCreateMatchExpression(nfdv1alpha1.MatchExists),
-						},
+					MatchExpressions: MatchExpressionSet{
+						Expressions: Expressions{"key-na": MustCreateMatchExpression(MatchExists)},
 					},
 				},
 			},
 		},
 	}
-	m, err = r5.execute(f)
+	m, err = r5.Execute(f)
 	assert.Nilf(t, err, "unexpected error: %v", err)
 	assert.Nil(t, m, "instances should not have matched")
 
@@ -194,16 +181,14 @@ func TestRule(t *testing.T) {
 			MatchFeatures: FeatureMatcher{
 				FeatureMatcherTerm{
 					Feature: "domain-1.kf-1",
-					MatchExpressions: nfdv1alpha1.MatchExpressionSet{
-						Expressions: nfdv1alpha1.Expressions{
-							"key-1": nfdv1alpha1.MustCreateMatchExpression(nfdv1alpha1.MatchExists),
-						},
+					MatchExpressions: MatchExpressionSet{
+						Expressions: Expressions{"key-1": MustCreateMatchExpression(MatchExists)},
 					},
 				},
 			},
 		})
-	r5.MatchFeatures[0].MatchExpressions.Expressions["key-1"] = nfdv1alpha1.MustCreateMatchExpression(nfdv1alpha1.MatchIn, "val-1")
-	m, err = r5.execute(f)
+	r5.MatchFeatures[0].MatchExpressions.Expressions["key-1"] = MustCreateMatchExpression(MatchIn, "val-1")
+	m, err = r5.Execute(f)
 	assert.Nilf(t, err, "unexpected error: %v", err)
 	assert.Equal(t, r5.Labels, m, "instances should have matched")
 }