diff --git a/pkg/nfd-master/nfd-master-internal_test.go b/pkg/nfd-master/nfd-master-internal_test.go index 0f33ea5c3..e715cd666 100644 --- a/pkg/nfd-master/nfd-master-internal_test.go +++ b/pkg/nfd-master/nfd-master-internal_test.go @@ -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/pkg/nfd-master/nfd-master.go b/pkg/nfd-master/nfd-master.go index 70ed6d1ad..02dfdc604 100644 --- a/pkg/nfd-master/nfd-master.go +++ b/pkg/nfd-master/nfd-master.go @@ -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 @@ -764,8 +763,11 @@ func (m *nfdMaster) nfdAPIUpdateOneNode(nodeName string) error { // 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)) @@ -790,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) @@ -807,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) @@ -834,9 +836,7 @@ func filterExtendedResource(name, value string, features *nfdv1alpha1.Features) } func (m *nfdMaster) refreshNodeFeatures(cli *kubernetes.Clientset, nodeName string, labels map[string]string, features *nfdv1alpha1.Features) error { - if labels == nil { - labels = make(map[string]string) - } + labels = addNsToMapKeys(labels, nfdv1alpha1.FeatureLabelNs) crLabels, crAnnotations, crExtendedResources, crTaints := m.processNodeFeatureRule(nodeName, features) @@ -1011,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 } @@ -1285,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, "/") { @@ -1404,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 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"