Merge pull request #2028 from mfranczy/image-compatibility-nfr

Refactoring of image compatibility node validator

pkg/client-nfd/compat/node-validator/node-validator.go

@@ -18,6 +18,7 @@ package nodevalidator
 
 import (
 	"context"
+	"fmt"
 	"slices"
 	"sort"
 
@@ -62,12 +63,12 @@ func New(opts ...NodeValidatorOpts) nodeValidator {
 func (nv *nodeValidator) Execute(ctx context.Context) ([]*CompatibilityStatus, error) {
 	spec, err := nv.artifactClient.FetchCompatibilitySpec(ctx)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to fetch compatibility spec: %w", err)
 	}
 
 	for _, s := range nv.sources {
 		if err := s.Discover(); err != nil {
-			return nil, err
+			return nil, fmt.Errorf("error during discovery of source %s: %w", s.Name(), err)
 		}
 	}
 	features := source.GetAllFeatures()
@@ -84,7 +85,7 @@ func (nv *nodeValidator) Execute(ctx context.Context) ([]*CompatibilityStatus, e
 			if err != nil {
 				return nil, err
 			}
-			compat.Rules = append(compat.Rules, evaluateRuleStatus(&r, ruleOut.MatchStatus))
+			compat.Rules = append(compat.Rules, nv.evaluateRuleStatus(&r, ruleOut.MatchStatus))
 
 			// Add the 'rule.matched' feature for backreference functionality
 			features.InsertAttributeFeatures(nfdv1alpha1.RuleBackrefDomain, nfdv1alpha1.RuleBackrefFeature, ruleOut.Labels)
@@ -96,18 +97,58 @@ func (nv *nodeValidator) Execute(ctx context.Context) ([]*CompatibilityStatus, e
 	return compats, nil
 }
 
-func evaluateRuleStatus(rule *nfdv1alpha1.Rule, matchStatus *nodefeaturerule.MatchStatus) ProcessedRuleStatus {
+func (nv *nodeValidator) evaluateRuleStatus(rule *nfdv1alpha1.Rule, matchStatus *nodefeaturerule.MatchStatus) ProcessedRuleStatus {
-	var matchedFeatureTerms nfdv1alpha1.FeatureMatcher
 	out := ProcessedRuleStatus{Name: rule.Name, IsMatch: matchStatus.IsMatch}
 
-	evaluateFeatureMatcher := func(featureMatcher, matchedFeatureTerms nfdv1alpha1.FeatureMatcher) []MatchedExpression {
+	matchedFeatureTerms := nfdv1alpha1.FeatureMatcher{}
-		out := []MatchedExpression{}
+	if m := matchStatus.MatchFeatureStatus; m != nil {
+		matchedFeatureTerms = m.MatchedFeaturesTerms
+	}
+	out.MatchedExpressions = nv.matchFeatureExpressions(rule.MatchFeatures, matchedFeatureTerms)
+
+	for i, matchAnyElem := range rule.MatchAny {
+		matchedFeatureTermsAny := nfdv1alpha1.FeatureMatcher{}
+		if t := matchStatus.MatchAny[i].MatchedFeaturesTerms; t != nil {
+			matchedFeatureTermsAny = t
+		}
+		matchedExpressions := nv.matchFeatureExpressions(matchAnyElem.MatchFeatures, matchedFeatureTermsAny)
+		out.MatchedAny = append(out.MatchedAny, MatchAnyElem{MatchedExpressions: matchedExpressions})
+	}
+
+	return out
+}
+
+func (nv *nodeValidator) matchFeatureExpressions(featureMatcher, matchedFeatureTerms nfdv1alpha1.FeatureMatcher) []MatchedExpression {
+	var out []MatchedExpression
+
 	for _, term := range featureMatcher {
 		if term.MatchExpressions != nil {
+			out = append(out, nv.matchExpressions(term, matchedFeatureTerms)...)
+		}
+		if term.MatchName != nil {
+			out = append(out, nv.matchName(term, matchedFeatureTerms))
+		}
+	}
+
+	// For reproducible output sort by name, feature, expression.
+	sort.Slice(out, func(i, j int) bool {
+		if out[i].Feature != out[j].Feature {
+			return out[i].Feature < out[j].Feature
+		}
+		if out[i].Name != out[j].Name {
+			return out[i].Name < out[j].Name
+		}
+		return out[i].Expression.String() < out[j].Expression.String()
+	})
+
+	return out
+}
+
+func (nodeValidator) matchExpressions(term nfdv1alpha1.FeatureMatcherTerm, matchedFeatureTerms nfdv1alpha1.FeatureMatcher) []MatchedExpression {
+	var out []MatchedExpression
+
 	for name, exp := range *term.MatchExpressions {
 		isMatch := false
-
-					// Check if the expression matches
 		for _, processedTerm := range matchedFeatureTerms {
 			if term.Feature != processedTerm.Feature || processedTerm.MatchExpressions == nil {
 				continue
@@ -126,11 +167,13 @@ func evaluateRuleStatus(rule *nfdv1alpha1.Rule, matchStatus *nodefeaturerule.Mat
 			IsMatch:     isMatch,
 		})
 	}
-			}
 
-			if term.MatchName != nil {
+	return out
+}
+
+func (nodeValidator) matchName(term nfdv1alpha1.FeatureMatcherTerm, matchedFeatureTerms nfdv1alpha1.FeatureMatcher) MatchedExpression {
 	isMatch := false
-				for _, processedTerm := range matchStatus.MatchedFeaturesTerms {
+	for _, processedTerm := range matchedFeatureTerms {
 		if term.Feature != processedTerm.Feature || processedTerm.MatchName == nil {
 			continue
 		}
@@ -139,46 +182,13 @@ func evaluateRuleStatus(rule *nfdv1alpha1.Rule, matchStatus *nodefeaturerule.Mat
 			break
 		}
 	}
-				out = append(out, MatchedExpression{
+	return MatchedExpression{
 		Feature:     term.Feature,
 		Name:        "",
 		Expression:  term.MatchName,
 		MatcherType: MatchNameType,
 		IsMatch:     isMatch,
-				})
 	}
-		}
-
-		// For reproducible output sort by name, feature, expression.
-		sort.Slice(out, func(i, j int) bool {
-			if out[i].Feature != out[j].Feature {
-				return out[i].Feature < out[j].Feature
-			}
-			if out[i].Name != out[j].Name {
-				return out[i].Name < out[j].Name
-			}
-			return out[i].Expression.String() < out[j].Expression.String()
-		})
-
-		return out
-	}
-
-	if matchFeatures := rule.MatchFeatures; matchFeatures != nil {
-		if matchStatus.MatchFeatureStatus != nil {
-			matchedFeatureTerms = matchStatus.MatchFeatureStatus.MatchedFeaturesTerms
-		}
-		out.MatchedExpressions = evaluateFeatureMatcher(matchFeatures, matchedFeatureTerms)
-	}
-
-	for i, matchAnyElem := range rule.MatchAny {
-		if matchStatus.MatchAny[i].MatchedFeaturesTerms != nil {
-			matchedFeatureTerms = matchStatus.MatchAny[i].MatchedFeaturesTerms
-		}
-		matchedExpressions := evaluateFeatureMatcher(matchAnyElem.MatchFeatures, matchedFeatureTerms)
-		out.MatchedAny = append(out.MatchedAny, MatchAnyElem{MatchedExpressions: matchedExpressions})
-	}
-
-	return out
 }
 
 // NodeValidatorOpts applies certain options to the node validator.

pkg/client-nfd/compat/node-validator/node-validator_test.go

@@ -34,16 +34,46 @@ func init() {
 	fs.SetConfig(fs.NewConfig())
 }
 
-func TestNodeValidator(t *testing.T) {
+func buildDefaultSpec(rules []v1alpha1.Rule) *compatv1alpha1.Spec {
-	ctx := context.Background()
+	return &compatv1alpha1.Spec{
-
-	Convey("With a single compatibility set that contains flags, attributes and instances", t, func() {
-		spec := &compatv1alpha1.Spec{
 		Version: compatv1alpha1.Version,
 		Compatibilties: []compatv1alpha1.Compatibility{
 			{
 				Description: "Fake compatibility",
-					Rules: []v1alpha1.Rule{
+				Rules:       rules,
+			},
+		},
+	}
+}
+
+func buildDefaultExpectedOutput(status []ProcessedRuleStatus) []*CompatibilityStatus {
+	return []*CompatibilityStatus{
+		{
+			Description: "Fake compatibility",
+			Rules:       status,
+		},
+	}
+}
+
+func assertOutput(ctx context.Context, spec *compatv1alpha1.Spec, expectedOutput []*CompatibilityStatus) {
+	validator := New(
+		WithArgs(&Args{}),
+		WithArtifactClient(newMock(ctx, spec)),
+		WithSources(map[string]source.FeatureSource{fake.Name: source.GetFeatureSource(fake.Name)}),
+	)
+	output, err := validator.Execute(ctx)
+
+	So(err, ShouldBeNil)
+	So(output, ShouldEqual, expectedOutput)
+}
+
+func TestNodeValidator(t *testing.T) {
+	ctx := context.Background()
+
+	Convey("With a single compatibility set", t, func() {
+
+		Convey("That contains flag which results in match", func() {
+			spec := buildDefaultSpec([]v1alpha1.Rule{
 				{
 					Name: "fake_1",
 					MatchFeatures: v1alpha1.FeatureMatcher{
@@ -53,6 +83,29 @@ func TestNodeValidator(t *testing.T) {
 						},
 					},
 				},
+			})
+
+			expectedOutput := buildDefaultExpectedOutput([]ProcessedRuleStatus{
+				{
+					Name:    "fake_1",
+					IsMatch: true,
+					MatchedExpressions: []MatchedExpression{
+						{
+							Feature:     "fake.flag",
+							Name:        "",
+							Expression:  &v1alpha1.MatchExpression{Op: v1alpha1.MatchInRegexp, Value: v1alpha1.MatchValue{"^flag"}},
+							MatcherType: MatchNameType,
+							IsMatch:     true,
+						},
+					},
+				},
+			})
+
+			assertOutput(ctx, spec, expectedOutput)
+		})
+
+		Convey("That contains flags and attribute which result in mismatch", func() {
+			spec := buildDefaultSpec([]v1alpha1.Rule{
 				{
 					Name: "fake_2",
 					MatchFeatures: v1alpha1.FeatureMatcher{
@@ -70,84 +123,9 @@ func TestNodeValidator(t *testing.T) {
 						},
 					},
 				},
-						{
+			})
-							Name: "fake_3",
-							MatchFeatures: v1alpha1.FeatureMatcher{
-								{
-									Feature: "fake.instance",
-									MatchExpressions: &v1alpha1.MatchExpressionSet{
-										"name":   &v1alpha1.MatchExpression{Op: v1alpha1.MatchIn, Value: v1alpha1.MatchValue{"instance_1"}},
-										"attr_1": &v1alpha1.MatchExpression{Op: v1alpha1.MatchIn, Value: v1alpha1.MatchValue{"true"}},
-									},
-								},
-								{
-									Feature: "fake.instance",
-									MatchExpressions: &v1alpha1.MatchExpressionSet{
-										"name":   &v1alpha1.MatchExpression{Op: v1alpha1.MatchIn, Value: v1alpha1.MatchValue{"instance_2"}},
-										"attr_1": &v1alpha1.MatchExpression{Op: v1alpha1.MatchIn, Value: v1alpha1.MatchValue{"false"}},
-									},
-								},
-							},
-						},
-						{
-							Name: "fake_4",
-							MatchAny: []v1alpha1.MatchAnyElem{
-								{
-									MatchFeatures: v1alpha1.FeatureMatcher{
-										{
-											Feature: "fake.instance",
-											MatchExpressions: &v1alpha1.MatchExpressionSet{
-												"name": &v1alpha1.MatchExpression{Op: v1alpha1.MatchIn, Value: v1alpha1.MatchValue{"instance_1"}},
-											},
-										},
-									},
-								},
-								{
-									MatchFeatures: v1alpha1.FeatureMatcher{
-										{
-											Feature: "fake.instance",
-											MatchExpressions: &v1alpha1.MatchExpressionSet{
-												"name": &v1alpha1.MatchExpression{Op: v1alpha1.MatchIn, Value: v1alpha1.MatchValue{"instance_unknown"}},
-											},
-										},
-									},
-								},
-							},
-						},
-						{
-							Name: "fake_5",
-							MatchFeatures: v1alpha1.FeatureMatcher{
-								{
-									Feature: "unknown.unknown",
-									MatchExpressions: &v1alpha1.MatchExpressionSet{
-										"name": &v1alpha1.MatchExpression{Op: v1alpha1.MatchIn, Value: v1alpha1.MatchValue{"instance_1"}},
-									},
-								},
-							},
-						},
-					},
-				},
-			},
-		}
 
-		// The output contains expressions in alphabetical order over the feature, name and expression string.
+			expectedOutput := buildDefaultExpectedOutput([]ProcessedRuleStatus{
-		expectedOutput := []*CompatibilityStatus{
-			{
-				Description: "Fake compatibility",
-				Rules: []ProcessedRuleStatus{
-					{
-						Name:    "fake_1",
-						IsMatch: true,
-						MatchedExpressions: []MatchedExpression{
-							{
-								Feature:     "fake.flag",
-								Name:        "",
-								Expression:  &v1alpha1.MatchExpression{Op: v1alpha1.MatchInRegexp, Value: v1alpha1.MatchValue{"^flag"}},
-								MatcherType: MatchNameType,
-								IsMatch:     true,
-							},
-						},
-					},
 				{
 					Name:    "fake_2",
 					IsMatch: false,
@@ -168,6 +146,35 @@ func TestNodeValidator(t *testing.T) {
 						},
 					},
 				},
+			})
+
+			assertOutput(ctx, spec, expectedOutput)
+		})
+
+		Convey("That contains instances which results in mismatch", func() {
+			spec := buildDefaultSpec([]v1alpha1.Rule{
+				{
+					Name: "fake_3",
+					MatchFeatures: v1alpha1.FeatureMatcher{
+						{
+							Feature: "fake.instance",
+							MatchExpressions: &v1alpha1.MatchExpressionSet{
+								"name":   &v1alpha1.MatchExpression{Op: v1alpha1.MatchIn, Value: v1alpha1.MatchValue{"instance_1"}},
+								"attr_1": &v1alpha1.MatchExpression{Op: v1alpha1.MatchIn, Value: v1alpha1.MatchValue{"true"}},
+							},
+						},
+						{
+							Feature: "fake.instance",
+							MatchExpressions: &v1alpha1.MatchExpressionSet{
+								"name":   &v1alpha1.MatchExpression{Op: v1alpha1.MatchIn, Value: v1alpha1.MatchValue{"instance_2"}},
+								"attr_1": &v1alpha1.MatchExpression{Op: v1alpha1.MatchIn, Value: v1alpha1.MatchValue{"false"}},
+							},
+						},
+					},
+				},
+			})
+
+			expectedOutput := buildDefaultExpectedOutput([]ProcessedRuleStatus{
 				{
 					Name:    "fake_3",
 					IsMatch: false,
@@ -202,6 +209,41 @@ func TestNodeValidator(t *testing.T) {
 						},
 					},
 				},
+			})
+
+			assertOutput(ctx, spec, expectedOutput)
+		})
+
+		Convey("That contains instances which results in match", func() {
+			spec := buildDefaultSpec([]v1alpha1.Rule{
+				{
+					Name: "fake_4",
+					MatchAny: []v1alpha1.MatchAnyElem{
+						{
+							MatchFeatures: v1alpha1.FeatureMatcher{
+								{
+									Feature: "fake.instance",
+									MatchExpressions: &v1alpha1.MatchExpressionSet{
+										"name": &v1alpha1.MatchExpression{Op: v1alpha1.MatchIn, Value: v1alpha1.MatchValue{"instance_1"}},
+									},
+								},
+							},
+						},
+						{
+							MatchFeatures: v1alpha1.FeatureMatcher{
+								{
+									Feature: "fake.instance",
+									MatchExpressions: &v1alpha1.MatchExpressionSet{
+										"name": &v1alpha1.MatchExpression{Op: v1alpha1.MatchIn, Value: v1alpha1.MatchValue{"instance_unknown"}},
+									},
+								},
+							},
+						},
+					},
+				},
+			})
+
+			expectedOutput := buildDefaultExpectedOutput([]ProcessedRuleStatus{
 				{
 					Name:    "fake_4",
 					IsMatch: true,
@@ -230,6 +272,27 @@ func TestNodeValidator(t *testing.T) {
 						},
 					},
 				},
+			})
+
+			assertOutput(ctx, spec, expectedOutput)
+		})
+
+		Convey("That contains spec with zero matches which results in mismatch", func() {
+			spec := buildDefaultSpec([]v1alpha1.Rule{
+				{
+					Name: "fake_5",
+					MatchFeatures: v1alpha1.FeatureMatcher{
+						{
+							Feature: "unknown.unknown",
+							MatchExpressions: &v1alpha1.MatchExpressionSet{
+								"name": &v1alpha1.MatchExpression{Op: v1alpha1.MatchIn, Value: v1alpha1.MatchValue{"instance_1"}},
+							},
+						},
+					},
+				},
+			})
+
+			expectedOutput := buildDefaultExpectedOutput([]ProcessedRuleStatus{
 				{
 					Name:    "fake_5",
 					IsMatch: false,
@@ -243,19 +306,11 @@ func TestNodeValidator(t *testing.T) {
 						},
 					},
 				},
-				},
+			})
-			},
-		}
 
-		validator := New(
+			assertOutput(ctx, spec, expectedOutput)
-			WithArgs(&Args{}),
+		})
-			WithArtifactClient(newMock(ctx, spec)),
-			WithSources(map[string]source.FeatureSource{fake.Name: source.GetFeatureSource(fake.Name)}),
-		)
-		output, err := validator.Execute(ctx)
 
-		So(err, ShouldBeNil)
-		So(output, ShouldEqual, expectedOutput)
 	})
 
 	Convey("With multiple compatibility sets", t, func() {