Repository: kubernetes-sigs/node-feature-discovery
Pull request #1471 (2023-11-23 19:41:44)

nfd-master: predictable handling of unprefixed names
Make the handling of unprefixed names (of labels, annotations and
extended resources) well-defined and predictable. Previously the
resulting output was not predictable in case the same name was coming in
both the unprefixed and prefixed form, say unprefixed `foo=bar` coming from
one source (be it nfd-worker or NodeFeature(Rule)) and
`feature.node.kubernetes.io/foo=baz` from a NodeFeature(Rule).
Previously the output value was randomly either `bar` or `baz`.

This patch adds prefixes to all names early in the processing
"pipeline", preventing random name clashes later on.


Patch:
diff --git a/pkg/nfd-master/nfd-master.go b/pkg/nfd-master/nfd-master.go
index a6c9f9e3e..02dfdc604 100644
--- a/pkg/nfd-master/nfd-master.go
+++ b/pkg/nfd-master/nfd-master.go
@@ -465,7 +465,7 @@ func (m *nfdMaster) prune() error {
 		klog.InfoS("pruning node...", "nodeName", node.Name)
 
 		// Prune labels and extended resources
-		err := m.updateNodeObject(cli, node.Name, Labels{}, Annotations{}, Annotations{}, ExtendedResources{}, []corev1.Taint{})
+		err := m.updateNodeObject(cli, node.Name, Labels{}, Annotations{}, ExtendedResources{}, []corev1.Taint{})
 		if err != nil {
 			nodeUpdateFailures.Inc()
 			return fmt.Errorf("failed to prune node %q: %v", node.Name, err)
@@ -523,9 +523,6 @@ func (m *nfdMaster) updateMasterNode() error {
 func (m *nfdMaster) filterFeatureLabels(labels Labels, features *nfdv1alpha1.Features) (Labels, ExtendedResources) {
 	outLabels := Labels{}
 	for name, value := range labels {
-		// Add possibly missing default ns
-		name := addNs(name, nfdv1alpha1.FeatureLabelNs)
-
 		if value, err := m.filterFeatureLabel(name, value, features); err != nil {
 			klog.ErrorS(err, "ignoring label", "labelKey", name, "labelValue", value)
 			nodeLabelsRejected.Inc()
@@ -537,8 +534,7 @@ func (m *nfdMaster) filterFeatureLabels(labels Labels, features *nfdv1alpha1.Fea
 	// Remove labels which are intended to be extended resources
 	extendedResources := ExtendedResources{}
 	for extendedResourceName := range m.config.ResourceLabels {
-		// Add possibly missing default ns
-		extendedResourceName = addNs(extendedResourceName, nfdv1alpha1.FeatureLabelNs)
+		extendedResourceName := addNs(extendedResourceName, nfdv1alpha1.FeatureLabelNs)
 		if value, ok := outLabels[extendedResourceName]; ok {
 			if _, err := strconv.Atoi(value); err != nil {
 				klog.ErrorS(err, "bad label value encountered for extended resource", "labelKey", extendedResourceName, "labelValue", value)
@@ -563,6 +559,9 @@ func (m *nfdMaster) filterFeatureLabel(name, value string, features *nfdv1alpha1
 
 	// Check label namespace, filter out if ns is not whitelisted
 	ns, base := splitNs(name)
+	if ns == "" {
+		return "", fmt.Errorf("labels without namespace (prefix/) not allowed")
+	}
 	if ns != nfdv1alpha1.FeatureLabelNs && ns != nfdv1alpha1.ProfileLabelNs &&
 		!strings.HasSuffix(ns, nfdv1alpha1.FeatureLabelSubNsSuffix) && !strings.HasSuffix(ns, nfdv1alpha1.ProfileLabelSubNsSuffix) {
 		// If the namespace is denied, and not present in the extraLabelNs, label will be ignored
@@ -693,10 +692,8 @@ func (m *nfdMaster) SetLabels(c context.Context, r *pb.SetLabelsRequest) (*pb.Se
 			return &pb.SetLabelsReply{}, err
 		}
 
-		annotations := Annotations{}
-
 		// Create labels et al
-		if err := m.refreshNodeFeatures(cli, r.NodeName, annotations, r.GetLabels(), r.GetFeatures()); err != nil {
+		if err := m.refreshNodeFeatures(cli, r.NodeName, r.GetLabels(), r.GetFeatures()); err != nil {
 			nodeUpdateFailures.Inc()
 			return &pb.SetLabelsReply{}, err
 		}
@@ -760,16 +757,17 @@ func (m *nfdMaster) nfdAPIUpdateOneNode(nodeName string) error {
 
 	features := nfdv1alpha1.NewNodeFeatureSpec()
 
-	annotations := Annotations{}
-
 	if len(objs) > 0 {
 		// Merge in features
 		//
 		// NOTE: changing the rule api to support handle multiple objects instead
 		// of merging would probably perform better with lot less data to copy.
 		features = objs[0].Spec.DeepCopy()
+		features.Labels = addNsToMapKeys(features.Labels, nfdv1alpha1.FeatureLabelNs)
 		for _, o := range objs[1:] {
-			o.Spec.MergeInto(features)
+			s := o.Spec.DeepCopy()
+			s.Labels = addNsToMapKeys(s.Labels, nfdv1alpha1.FeatureLabelNs)
+			s.MergeInto(features)
 		}
 
 		klog.V(4).InfoS("merged nodeFeatureSpecs", "newNodeFeatureSpec", utils.DelayedDumper(features))
@@ -782,7 +780,7 @@ func (m *nfdMaster) nfdAPIUpdateOneNode(nodeName string) error {
 	if err != nil {
 		return err
 	}
-	if err := m.refreshNodeFeatures(cli, nodeName, annotations, features.Labels, &features.Features); err != nil {
+	if err := m.refreshNodeFeatures(cli, nodeName, features.Labels, &features.Features); err != nil {
 		return err
 	}
 
@@ -794,9 +792,6 @@ func (m *nfdMaster) nfdAPIUpdateOneNode(nodeName string) error {
 func filterExtendedResources(features *nfdv1alpha1.Features, extendedResources ExtendedResources) ExtendedResources {
 	outExtendedResources := ExtendedResources{}
 	for name, value := range extendedResources {
-		// Add possibly missing default ns
-		name = addNs(name, nfdv1alpha1.ExtendedResourceNs)
-
 		capacity, err := filterExtendedResource(name, value, features)
 		if err != nil {
 			klog.ErrorS(err, "failed to create extended resources", "extendedResourceName", name, "extendedResourceValue", value)
@@ -811,6 +806,9 @@ func filterExtendedResources(features *nfdv1alpha1.Features, extendedResources E
 func filterExtendedResource(name, value string, features *nfdv1alpha1.Features) (string, error) {
 	// Check if given NS is allowed
 	ns, _ := splitNs(name)
+	if ns == "" {
+		return "", fmt.Errorf("extended resource without namespace (prefix/) not allowed")
+	}
 	if ns != nfdv1alpha1.ExtendedResourceNs && !strings.HasSuffix(ns, nfdv1alpha1.ExtendedResourceSubNsSuffix) {
 		if ns == "kubernetes.io" || strings.HasSuffix(ns, ".kubernetes.io") {
 			return "", fmt.Errorf("namespace %q is not allowed", ns)
@@ -837,10 +835,8 @@ func filterExtendedResource(name, value string, features *nfdv1alpha1.Features)
 	return q.String(), nil
 }
 
-func (m *nfdMaster) refreshNodeFeatures(cli *kubernetes.Clientset, nodeName string, nfdAnnotations Annotations, labels map[string]string, features *nfdv1alpha1.Features) error {
-	if labels == nil {
-		labels = make(map[string]string)
-	}
+func (m *nfdMaster) refreshNodeFeatures(cli *kubernetes.Clientset, nodeName string, labels map[string]string, features *nfdv1alpha1.Features) error {
+	labels = addNsToMapKeys(labels, nfdv1alpha1.FeatureLabelNs)
 
 	crLabels, crAnnotations, crExtendedResources, crTaints := m.processNodeFeatureRule(nodeName, features)
 
@@ -860,7 +856,7 @@ func (m *nfdMaster) refreshNodeFeatures(cli *kubernetes.Clientset, nodeName stri
 	extendedResources = filterExtendedResources(features, extendedResources)
 
 	// Annotations
-	featureAnnotations := m.filterFeatureAnnotations(crAnnotations)
+	annotations := m.filterFeatureAnnotations(crAnnotations)
 
 	// Taints
 	var taints []corev1.Taint
@@ -868,7 +864,7 @@ func (m *nfdMaster) refreshNodeFeatures(cli *kubernetes.Clientset, nodeName stri
 		taints = filterTaints(crTaints)
 	}
 
-	err := m.updateNodeObject(cli, nodeName, labels, nfdAnnotations, featureAnnotations, extendedResources, taints)
+	err := m.updateNodeObject(cli, nodeName, labels, annotations, extendedResources, taints)
 	if err != nil {
 		klog.ErrorS(err, "failed to update node", "nodeName", nodeName)
 		return err
@@ -1015,13 +1011,13 @@ func (m *nfdMaster) processNodeFeatureRule(nodeName string, features *nfdv1alpha
 				continue
 			}
 			taints = append(taints, ruleOut.Taints...)
-			for k, v := range ruleOut.Labels {
+			for k, v := range addNsToMapKeys(ruleOut.Labels, nfdv1alpha1.FeatureLabelNs) {
 				labels[k] = v
 			}
-			for k, v := range ruleOut.ExtendedResources {
+			for k, v := range addNsToMapKeys(ruleOut.ExtendedResources, nfdv1alpha1.ExtendedResourceNs) {
 				extendedResources[k] = v
 			}
-			for k, v := range ruleOut.Annotations {
+			for k, v := range addNsToMapKeys(ruleOut.Annotations, nfdv1alpha1.FeatureAnnotationNs) {
 				annotations[k] = v
 			}
 
@@ -1040,7 +1036,7 @@ func (m *nfdMaster) processNodeFeatureRule(nodeName string, features *nfdv1alpha
 // updateNodeObject ensures the Kubernetes node object is up to date,
 // creating new labels and extended resources where necessary and removing
 // outdated ones. Also updates the corresponding annotations.
-func (m *nfdMaster) updateNodeObject(cli *kubernetes.Clientset, nodeName string, labels Labels, nfdAnnotations, featureAnnotations Annotations, extendedResources ExtendedResources, taints []corev1.Taint) error {
+func (m *nfdMaster) updateNodeObject(cli *kubernetes.Clientset, nodeName string, labels Labels, featureAnnotations Annotations, extendedResources ExtendedResources, taints []corev1.Taint) error {
 	if cli == nil {
 		return fmt.Errorf("no client is passed, client:  %v", cli)
 	}
@@ -1051,6 +1047,8 @@ func (m *nfdMaster) updateNodeObject(cli *kubernetes.Clientset, nodeName string,
 		return err
 	}
 
+	annotations := make(Annotations)
+
 	// Store names of labels in an annotation
 	if len(labels) > 0 {
 		labelKeys := make([]string, 0, len(labels))
@@ -1059,7 +1057,7 @@ func (m *nfdMaster) updateNodeObject(cli *kubernetes.Clientset, nodeName string,
 			labelKeys = append(labelKeys, strings.TrimPrefix(key, nfdv1alpha1.FeatureLabelNs+"/"))
 		}
 		sort.Strings(labelKeys)
-		nfdAnnotations[m.instanceAnnotation(nfdv1alpha1.FeatureLabelsAnnotation)] = strings.Join(labelKeys, ",")
+		annotations[m.instanceAnnotation(nfdv1alpha1.FeatureLabelsAnnotation)] = strings.Join(labelKeys, ",")
 	}
 
 	// Store names of extended resources in an annotation
@@ -1070,11 +1068,10 @@ func (m *nfdMaster) updateNodeObject(cli *kubernetes.Clientset, nodeName string,
 			extendedResourceKeys = append(extendedResourceKeys, strings.TrimPrefix(key, nfdv1alpha1.FeatureLabelNs+"/"))
 		}
 		sort.Strings(extendedResourceKeys)
-		nfdAnnotations[m.instanceAnnotation(nfdv1alpha1.ExtendedResourceAnnotation)] = strings.Join(extendedResourceKeys, ",")
+		annotations[m.instanceAnnotation(nfdv1alpha1.ExtendedResourceAnnotation)] = strings.Join(extendedResourceKeys, ",")
 	}
 
 	// Store feature annotations
-	annotations := make(Annotations)
 	if len(featureAnnotations) > 0 {
 		// Store names of feature annotations in an annotation
 		annotationKeys := make([]string, 0, len(featureAnnotations))
@@ -1083,18 +1080,12 @@ func (m *nfdMaster) updateNodeObject(cli *kubernetes.Clientset, nodeName string,
 			annotationKeys = append(annotationKeys, strings.TrimPrefix(key, nfdv1alpha1.FeatureAnnotationNs+"/"))
 		}
 		sort.Strings(annotationKeys)
-		nfdAnnotations[m.instanceAnnotation(nfdv1alpha1.FeatureAnnotationsTrackingAnnotation)] = strings.Join(annotationKeys, ",")
+		annotations[m.instanceAnnotation(nfdv1alpha1.FeatureAnnotationsTrackingAnnotation)] = strings.Join(annotationKeys, ",")
 		for k, v := range featureAnnotations {
 			annotations[k] = v
 		}
 	}
 
-	if len(nfdAnnotations) > 0 {
-		for k, v := range nfdAnnotations {
-			annotations[k] = v
-		}
-	}
-
 	// Create JSON patches for changes in labels and annotations
 	oldLabels := stringToNsNames(node.Annotations[m.instanceAnnotation(nfdv1alpha1.FeatureLabelsAnnotation)], nfdv1alpha1.FeatureLabelNs)
 	oldAnnotations := stringToNsNames(node.Annotations[m.instanceAnnotation(nfdv1alpha1.FeatureAnnotationsTrackingAnnotation)], nfdv1alpha1.FeatureAnnotationNs)
@@ -1294,6 +1285,25 @@ func (m *nfdMaster) configure(filepath string, overrides string) error {
 	return nil
 }
 
+// addNsToMapKeys creates a copy of a map with the namespace (prefix) added to
+// unprefixed keys. Prefixed keys in the input map will take presedence, i.e.
+// if the input contains both prefixed (say "prefix/name") and unprefixed
+// ("name") name the unprefixed key will be ignored.
+func addNsToMapKeys(in map[string]string, nsToAdd string) map[string]string {
+	out := make(map[string]string, len(in))
+	for k, v := range in {
+		if strings.Contains(k, "/") {
+			out[k] = v
+		} else {
+			fqn := path.Join(nsToAdd, k)
+			if _, ok := in[fqn]; !ok {
+				out[fqn] = v
+			}
+		}
+	}
+	return out
+}
+
 // addNs adds a namespace if one isn't already found from src string
 func addNs(src string, nsToAdd string) string {
 	if strings.Contains(src, "/") {
@@ -1413,11 +1423,11 @@ func (m *nfdMaster) filterFeatureAnnotations(annotations map[string]string) map[
 	outAnnotations := make(map[string]string)
 
 	for annotation, value := range annotations {
-		// Add possibly missing default ns
-		annotation := addNs(annotation, nfdv1alpha1.FeatureAnnotationNs)
-
 		ns, _ := splitNs(annotation)
-
+		if ns == "" {
+			klog.ErrorS(fmt.Errorf("annotations without namespace (prefix/) not allowed"), fmt.Sprintf("Ignoring annotation %s", annotation))
+			continue
+		}
 		// Check annotation namespace, filter out if ns is not whitelisted
 		if ns != nfdv1alpha1.FeatureAnnotationNs && !strings.HasSuffix(ns, nfdv1alpha1.FeatureAnnotationSubNsSuffix) {
 			// If the namespace is denied, and not present in the extraLabelNs, label will be ignored


Test patch:
diff --git a/pkg/nfd-master/nfd-master-internal_test.go b/pkg/nfd-master/nfd-master-internal_test.go
index 0686a49ee..e715cd666 100644
--- a/pkg/nfd-master/nfd-master-internal_test.go
+++ b/pkg/nfd-master/nfd-master-internal_test.go
@@ -183,7 +183,7 @@ func TestUpdateNodeObject(t *testing.T) {
 			mockAPIHelper.On("GetNode", mockClient, mockNodeName).Return(mockNode, nil).Twice()
 			mockAPIHelper.On("PatchNodeStatus", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(statusPatches))).Return(nil)
 			mockAPIHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(metadataPatches))).Return(nil)
-			err := mockMaster.updateNodeObject(mockClient, mockNodeName, fakeFeatureLabels, Annotations{}, fakeAnnotations, fakeExtResources, nil)
+			err := mockMaster.updateNodeObject(mockClient, mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources, nil)
 
 			Convey("Error is nil", func() {
 				So(err, ShouldBeNil)
@@ -193,7 +193,7 @@ func TestUpdateNodeObject(t *testing.T) {
 		Convey("When I fail to update the node with feature labels", func() {
 			expectedError := fmt.Errorf("no client is passed, client:  <nil>")
 			mockAPIHelper.On("GetClient").Return(nil, expectedError)
-			err := mockMaster.updateNodeObject(nil, mockNodeName, fakeFeatureLabels, Annotations{}, fakeAnnotations, fakeExtResources, nil)
+			err := mockMaster.updateNodeObject(nil, mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources, nil)
 
 			Convey("Error is produced", func() {
 				So(err, ShouldResemble, expectedError)
@@ -203,7 +203,7 @@ func TestUpdateNodeObject(t *testing.T) {
 		Convey("When I fail to get a mock client while updating feature labels", func() {
 			expectedError := fmt.Errorf("no client is passed, client:  <nil>")
 			mockAPIHelper.On("GetClient").Return(nil, expectedError)
-			err := mockMaster.updateNodeObject(nil, mockNodeName, fakeFeatureLabels, Annotations{}, fakeAnnotations, fakeExtResources, nil)
+			err := mockMaster.updateNodeObject(nil, mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources, nil)
 
 			Convey("Error is produced", func() {
 				So(err, ShouldResemble, expectedError)
@@ -214,7 +214,7 @@ func TestUpdateNodeObject(t *testing.T) {
 			expectedError := errors.New("fake error")
 			mockAPIHelper.On("GetClient").Return(mockClient, nil)
 			mockAPIHelper.On("GetNode", mockClient, mockNodeName).Return(nil, expectedError).Twice()
-			err := mockMaster.updateNodeObject(mockClient, mockNodeName, fakeFeatureLabels, Annotations{}, fakeAnnotations, fakeExtResources, nil)
+			err := mockMaster.updateNodeObject(mockClient, mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources, nil)
 
 			Convey("Error is produced", func() {
 				So(err, ShouldEqual, expectedError)
@@ -227,7 +227,7 @@ func TestUpdateNodeObject(t *testing.T) {
 			mockAPIHelper.On("GetNode", mockClient, mockNodeName).Return(mockNode, nil).Twice()
 			mockAPIHelper.On("PatchNodeStatus", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(statusPatches))).Return(nil)
 			mockAPIHelper.On("PatchNode", mockClient, mockNodeName, mock.Anything).Return(expectedError).Twice()
-			err := mockMaster.updateNodeObject(mockClient, mockNodeName, fakeFeatureLabels, Annotations{}, fakeAnnotations, fakeExtResources, nil)
+			err := mockMaster.updateNodeObject(mockClient, mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources, nil)
 
 			Convey("Error is produced", func() {
 				So(err.Error(), ShouldEndWith, expectedError.Error())
@@ -422,20 +422,21 @@ func TestSetLabels(t *testing.T) {
 			vendorFeatureLabel := "vendor." + nfdv1alpha1.FeatureLabelNs + "/feature-4"
 			vendorProfileLabel := "vendor." + nfdv1alpha1.ProfileLabelNs + "/feature-5"
 			mockLabels := map[string]string{
-				"feature-1":                      "val-1",
-				"valid.ns/feature-2":             "val-2",
-				"random.denied.ns/feature-3":     "val-3",
-				"kubernetes.io/feature-4":        "val-4",
-				"sub.ns.kubernetes.io/feature-5": "val-5",
-				vendorFeatureLabel:               "val-6",
-				vendorProfileLabel:               "val-7",
-				"--invalid-name--":               "valid-val",
-				"valid-name":                     "--invalid-val--"}
+				"feature-1":                            "val-0",
+				"feature.node.kubernetes.io/feature-1": "val-1",
+				"valid.ns/feature-2":                   "val-2",
+				"random.denied.ns/feature-3":           "val-3",
+				"kubernetes.io/feature-4":              "val-4",
+				"sub.ns.kubernetes.io/feature-5":       "val-5",
+				vendorFeatureLabel:                     "val-6",
+				vendorProfileLabel:                     "val-7",
+				"--invalid-name--":                     "valid-val",
+				"valid-name":                           "--invalid-val--"}
 			expectedPatches := []apihelper.JsonPatch{
 				apihelper.NewJsonPatch("add", "/metadata/annotations",
 					instance+"."+nfdv1alpha1.FeatureLabelsAnnotation,
 					"feature-1,valid.ns/feature-2,"+vendorFeatureLabel+","+vendorProfileLabel),
-				apihelper.NewJsonPatch("add", "/metadata/labels", nfdv1alpha1.FeatureLabelNs+"/feature-1", mockLabels["feature-1"]),
+				apihelper.NewJsonPatch("add", "/metadata/labels", "feature.node.kubernetes.io/feature-1", mockLabels["feature.node.kubernetes.io/feature-1"]),
 				apihelper.NewJsonPatch("add", "/metadata/labels", "valid.ns/feature-2", mockLabels["valid.ns/feature-2"]),
 				apihelper.NewJsonPatch("add", "/metadata/labels", vendorFeatureLabel, mockLabels[vendorFeatureLabel]),
 				apihelper.NewJsonPatch("add", "/metadata/labels", vendorProfileLabel, mockLabels[vendorProfileLabel]),
@@ -468,7 +469,7 @@ func TestSetLabels(t *testing.T) {
 				apihelper.NewJsonPatch("add", "/status/capacity", nfdv1alpha1.FeatureLabelNs+"/feature-3", mockLabels["feature-3"]),
 			}
 
-			mockMaster.config.ResourceLabels = map[string]struct{}{"feature-3": {}, "feature-1": {}}
+			mockMaster.config.ResourceLabels = map[string]struct{}{"feature.node.kubernetes.io/feature-3": {}, "feature-1": {}}
 			mockHelper.On("GetClient").Return(mockClient, nil)
 			mockHelper.On("GetNode", mockClient, workerName).Return(mockNode, nil)
 			mockHelper.On("PatchNodeStatus", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedStatusPatches))).Return(nil)
@@ -503,7 +504,7 @@ func TestFilterLabels(t *testing.T) {
 	mockMaster := newMockMaster(mockHelper)
 
 	Convey("When using dynamic values", t, func() {
-		labelName := "testLabel"
+		labelName := "ns/testLabel"
 		labelValue := "@test.feature.LSM"
 		features := nfdv1alpha1.Features{
 			Attributes: map[string]nfdv1alpha1.AttributeFeatureSet{
diff --git a/test/e2e/data/nodefeature-1.yaml b/test/e2e/data/nodefeature-1.yaml
index 4b3b96523..c6ebe10fb 100644
--- a/test/e2e/data/nodefeature-1.yaml
+++ b/test/e2e/data/nodefeature-1.yaml
@@ -25,7 +25,9 @@ spec:
             attr_2: "9"
   # Labels to be created
   labels:
-    e2e-nodefeature-test-1: "obj-1"
+    e2e-nodefeature-test-1: "foo"
+    # The prefixed name should take precedence over the non-prefixed name above
+    feature.node.kubernetes.io/e2e-nodefeature-test-1: "obj-1"
     e2e-nodefeature-test-2: "obj-1"
     # Override feature from nfd-worker
     fake-fakefeature3: "overridden"