diff --git a/cmd/nfd-master/main.go b/cmd/nfd-master/main.go index caf1e03f0..5247477d0 100644 --- a/cmd/nfd-master/main.go +++ b/cmd/nfd-master/main.go @@ -122,7 +122,10 @@ func argsParse(argv []string) (master.Args, error) { return args, fmt.Errorf("error parsing whitelist regex (%s): %s", arguments["--label-whitelist"], err) } args.VerifyNodeName = arguments["--verify-node-name"].(bool) - args.ExtraLabelNs = strings.Split(arguments["--extra-label-ns"].(string), ",") + args.ExtraLabelNs = map[string]struct{}{} + for _, n := range strings.Split(arguments["--extra-label-ns"].(string), ",") { + args.ExtraLabelNs[n] = struct{}{} + } args.ResourceLabels = strings.Split(arguments["--resource-labels"].(string), ",") args.Prune = arguments["--prune"].(bool) args.Kubeconfig = arguments["--kubeconfig"].(string) diff --git a/pkg/nfd-master/nfd-master-internal_test.go b/pkg/nfd-master/nfd-master-internal_test.go index 7810c08bc..8476d098e 100644 --- a/pkg/nfd-master/nfd-master-internal_test.go +++ b/pkg/nfd-master/nfd-master-internal_test.go @@ -54,22 +54,22 @@ func newMockNode() *api.Node { func TestUpdateNodeFeatures(t *testing.T) { Convey("When I update the node using fake client", t, func() { - fakeFeatureLabels := map[string]string{"source-feature.1": "1", "source-feature.2": "2", "source-feature.3": "val3"} - fakeAnnotations := map[string]string{"version": version.Get()} - fakeExtResources := ExtendedResources{"source-feature.1": "", "source-feature.2": ""} + fakeFeatureLabels := map[string]string{LabelNs + "/source-feature.1": "1", LabelNs + "/source-feature.2": "2", LabelNs + "/source-feature.3": "val3"} + fakeAnnotations := map[string]string{AnnotationNs + "/version": version.Get()} + fakeExtResources := ExtendedResources{LabelNs + "/source-feature.1": "", LabelNs + "/source-feature.2": ""} fakeFeatureLabelNames := make([]string, 0, len(fakeFeatureLabels)) for k := range fakeFeatureLabels { fakeFeatureLabelNames = append(fakeFeatureLabelNames, k) } sort.Strings(fakeFeatureLabelNames) - fakeAnnotations["feature-labels"] = strings.Join(fakeFeatureLabelNames, ",") + fakeAnnotations[AnnotationNs+"/feature-labels"] = strings.Join(fakeFeatureLabelNames, ",") mockAPIHelper := new(apihelper.MockAPIHelpers) mockClient := &k8sclient.Clientset{} // Mock node with old features mockNode := newMockNode() - mockNode.Labels[LabelNs+"old-feature"] = "old-value" - mockNode.Annotations[AnnotationNs+"feature-labels"] = "old-feature" + mockNode.Labels[LabelNs+"/old-feature"] = "old-value" + mockNode.Annotations[AnnotationNs+"/feature-labels"] = "old-feature" Convey("When I successfully update the node with feature labels", func() { mockAPIHelper.On("GetClient").Return(mockClient, nil) @@ -84,11 +84,11 @@ func TestUpdateNodeFeatures(t *testing.T) { Convey("Node object should have updated with labels and annotations", func() { So(len(mockNode.Labels), ShouldEqual, len(fakeFeatureLabels)) for k, v := range fakeFeatureLabels { - So(mockNode.Labels[LabelNs+k], ShouldEqual, v) + So(mockNode.Labels[k], ShouldEqual, v) } So(len(mockNode.Annotations), ShouldEqual, len(fakeAnnotations)) for k, v := range fakeAnnotations { - So(mockNode.Annotations[AnnotationNs+k], ShouldEqual, v) + So(mockNode.Annotations[k], ShouldEqual, v) } }) }) @@ -195,23 +195,23 @@ func TestAddingExtResources(t *testing.T) { Convey("When there are matching labels", func() { mockNode := newMockNode() - mockResourceLabels := ExtendedResources{"feature-1": "1", "feature-2": "2"} + mockResourceLabels := ExtendedResources{LabelNs + "/feature-1": "1", LabelNs + "feature-2": "2"} resourceOps := getExtendedResourceOps(mockNode, mockResourceLabels) So(len(resourceOps), ShouldBeGreaterThan, 0) }) Convey("When the resource already exists", func() { mockNode := newMockNode() - mockNode.Status.Capacity[api.ResourceName(LabelNs+"feature-1")] = *resource.NewQuantity(1, resource.BinarySI) - mockResourceLabels := ExtendedResources{"feature-1": "1"} + mockNode.Status.Capacity[api.ResourceName(LabelNs+"/feature-1")] = *resource.NewQuantity(1, resource.BinarySI) + mockResourceLabels := ExtendedResources{LabelNs + "/feature-1": "1"} resourceOps := getExtendedResourceOps(mockNode, mockResourceLabels) So(len(resourceOps), ShouldEqual, 0) }) Convey("When the resource already exists but its capacity has changed", func() { mockNode := newMockNode() - mockNode.Status.Capacity[api.ResourceName(LabelNs+"feature-1")] = *resource.NewQuantity(2, resource.BinarySI) - mockResourceLabels := ExtendedResources{"feature-1": "1"} + mockNode.Status.Capacity[api.ResourceName(LabelNs+"/feature-1")] = *resource.NewQuantity(2, resource.BinarySI) + mockResourceLabels := ExtendedResources{LabelNs + "/feature-1": "1"} resourceOps := getExtendedResourceOps(mockNode, mockResourceLabels) So(len(resourceOps), ShouldBeGreaterThan, 0) }) @@ -222,28 +222,28 @@ func TestRemovingExtResources(t *testing.T) { Convey("When removing extended resources", t, func() { Convey("When none are removed", func() { mockNode := newMockNode() - mockResourceLabels := ExtendedResources{"feature-1": "1", "feature-2": "2"} - mockNode.Annotations[AnnotationNs+"extended-resources"] = "feature-1,feature-2" - mockNode.Status.Capacity[api.ResourceName(LabelNs+"feature-1")] = *resource.NewQuantity(1, resource.BinarySI) - mockNode.Status.Capacity[api.ResourceName(LabelNs+"feature-2")] = *resource.NewQuantity(2, resource.BinarySI) + mockResourceLabels := ExtendedResources{LabelNs + "/feature-1": "1", LabelNs + "/feature-2": "2"} + mockNode.Annotations[AnnotationNs+"/extended-resources"] = "feature-1,feature-2" + mockNode.Status.Capacity[api.ResourceName(LabelNs+"/feature-1")] = *resource.NewQuantity(1, resource.BinarySI) + mockNode.Status.Capacity[api.ResourceName(LabelNs+"/feature-2")] = *resource.NewQuantity(2, resource.BinarySI) resourceOps := getExtendedResourceOps(mockNode, mockResourceLabels) So(len(resourceOps), ShouldEqual, 0) }) Convey("When the related label is gone", func() { mockNode := newMockNode() - mockResourceLabels := ExtendedResources{"feature-4": "", "feature-2": "2"} - mockNode.Annotations[AnnotationNs+"extended-resources"] = "feature-4,feature-2" - mockNode.Status.Capacity[api.ResourceName(LabelNs+"feature-4")] = *resource.NewQuantity(4, resource.BinarySI) - mockNode.Status.Capacity[api.ResourceName(LabelNs+"feature-2")] = *resource.NewQuantity(2, resource.BinarySI) + mockResourceLabels := ExtendedResources{LabelNs + "/feature-4": "", LabelNs + "/feature-2": "2"} + mockNode.Annotations[AnnotationNs+"/extended-resources"] = "feature-4,feature-2" + mockNode.Status.Capacity[api.ResourceName(LabelNs+"/feature-4")] = *resource.NewQuantity(4, resource.BinarySI) + mockNode.Status.Capacity[api.ResourceName(LabelNs+"/feature-2")] = *resource.NewQuantity(2, resource.BinarySI) resourceOps := getExtendedResourceOps(mockNode, mockResourceLabels) So(len(resourceOps), ShouldBeGreaterThan, 0) }) Convey("When the extended resource is no longer wanted", func() { mockNode := newMockNode() - mockNode.Status.Capacity[api.ResourceName(LabelNs+"feature-1")] = *resource.NewQuantity(1, resource.BinarySI) - mockNode.Status.Capacity[api.ResourceName(LabelNs+"feature-2")] = *resource.NewQuantity(2, resource.BinarySI) - mockResourceLabels := ExtendedResources{"feature-2": "2"} - mockNode.Annotations[AnnotationNs+"extended-resources"] = "feature-1,feature-2" + mockNode.Status.Capacity[api.ResourceName(LabelNs+"/feature-1")] = *resource.NewQuantity(1, resource.BinarySI) + mockNode.Status.Capacity[api.ResourceName(LabelNs+"/feature-2")] = *resource.NewQuantity(2, resource.BinarySI) + mockResourceLabels := ExtendedResources{LabelNs + "/feature-2": "2"} + mockNode.Annotations[AnnotationNs+"/extended-resources"] = "feature-1,feature-2" resourceOps := getExtendedResourceOps(mockNode, mockResourceLabels) So(len(resourceOps), ShouldBeGreaterThan, 0) }) @@ -259,6 +259,7 @@ func TestSetLabels(t *testing.T) { mockNode := newMockNode() mockServer := labelerServer{args: Args{LabelWhiteList: regexp.MustCompile("")}, apiHelper: mockHelper} mockCtx := context.Background() + // In the gRPC request the label names may omit the default ns mockLabels := map[string]string{"feature-1": "val-1", "feature-2": "val-2", "feature-3": "val-3"} mockReq := &labeler.SetLabelsRequest{NodeName: workerName, NfdVersion: workerVer, Labels: mockLabels} @@ -282,11 +283,11 @@ func TestSetLabels(t *testing.T) { Convey("Node object should have updated with labels and annotations", func() { So(len(mockNode.Labels), ShouldEqual, len(mockLabels)) for k, v := range mockLabels { - So(mockNode.Labels[LabelNs+k], ShouldEqual, v) + So(mockNode.Labels[LabelNs+"/"+k], ShouldEqual, v) } So(len(mockNode.Annotations), ShouldEqual, len(expectedAnnotations)) for k, v := range expectedAnnotations { - So(mockNode.Annotations[AnnotationNs+k], ShouldEqual, v) + So(mockNode.Annotations[AnnotationNs+"/"+k], ShouldEqual, v) } }) }) @@ -302,19 +303,22 @@ func TestSetLabels(t *testing.T) { }) Convey("Node object should only have whitelisted labels", func() { So(len(mockNode.Labels), ShouldEqual, 1) - So(mockNode.Labels, ShouldResemble, map[string]string{LabelNs + "feature-2": "val-2"}) + So(mockNode.Labels, ShouldResemble, map[string]string{LabelNs + "/feature-2": "val-2"}) - a := map[string]string{AnnotationNs + "worker.version": workerVer, AnnotationNs + "feature-labels": "feature-2", AnnotationNs + "extended-resources": ""} + a := map[string]string{AnnotationNs + "/worker.version": workerVer, + AnnotationNs + "/feature-labels": "feature-2", + AnnotationNs + "/extended-resources": ""} So(len(mockNode.Annotations), ShouldEqual, len(a)) So(mockNode.Annotations, ShouldResemble, a) }) }) Convey("When --extra-label-ns is specified", func() { - mockServer.args.ExtraLabelNs = []string{"valid.ns"} + mockServer.args.ExtraLabelNs = map[string]struct{}{"valid.ns": struct{}{}} mockHelper.On("GetClient").Return(mockClient, nil) mockHelper.On("GetNode", mockClient, workerName).Return(mockNode, nil) mockHelper.On("UpdateNode", mockClient, mockNode).Return(nil) + // In the gRPC request the label names may omit the default ns mockLabels := map[string]string{"feature-1": "val-1", "valid.ns/feature-2": "val-2", "invalid.ns/feature-3": "val-3"} @@ -325,9 +329,11 @@ func TestSetLabels(t *testing.T) { }) Convey("Node object should only have allowed label namespaces", func() { So(len(mockNode.Labels), ShouldEqual, 2) - So(mockNode.Labels, ShouldResemble, map[string]string{LabelNs + "feature-1": "val-1", "valid.ns/feature-2": "val-2"}) + So(mockNode.Labels, ShouldResemble, map[string]string{LabelNs + "/feature-1": "val-1", "valid.ns/feature-2": "val-2"}) - a := map[string]string{AnnotationNs + "worker.version": workerVer, AnnotationNs + "feature-labels": "feature-1,valid.ns/feature-2", AnnotationNs + "extended-resources": ""} + a := map[string]string{AnnotationNs + "/worker.version": workerVer, + AnnotationNs + "/feature-labels": "feature-1,valid.ns/feature-2", + AnnotationNs + "/extended-resources": ""} So(len(mockNode.Annotations), ShouldEqual, len(a)) So(mockNode.Annotations, ShouldResemble, a) }) @@ -371,9 +377,9 @@ func TestAddLabels(t *testing.T) { Convey("They should be added to the node.Labels", func() { test1 := "test1" - labels[test1] = "true" + labels[LabelNs+"/"+test1] = "true" addLabels(n, labels) - So(n.Labels, ShouldContainKey, LabelNs+test1) + So(n.Labels, ShouldContainKey, LabelNs+"/"+test1) }) }) } diff --git a/pkg/nfd-master/nfd-master.go b/pkg/nfd-master/nfd-master.go index 66ae9799b..07fcdd4f8 100644 --- a/pkg/nfd-master/nfd-master.go +++ b/pkg/nfd-master/nfd-master.go @@ -24,6 +24,7 @@ import ( "log" "net" "os" + "path/filepath" "regexp" "sort" "strconv" @@ -42,10 +43,10 @@ import ( const ( // Namespace for feature labels - LabelNs = "feature.node.kubernetes.io/" + LabelNs = "feature.node.kubernetes.io" // Namespace for all NFD-related annotations - AnnotationNs = "nfd.node.kubernetes.io/" + AnnotationNs = "nfd.node.kubernetes.io" ) // package loggers @@ -68,7 +69,7 @@ type Annotations map[string]string type Args struct { CaFile string CertFile string - ExtraLabelNs []string + ExtraLabelNs map[string]struct{} KeyFile string Kubeconfig string LabelWhiteList *regexp.Regexp @@ -100,9 +101,6 @@ type statusOp struct { } func createStatusOp(verb string, resource string, path string, value string) statusOp { - if !strings.Contains(resource, "/") { - resource = LabelNs + resource - } res := strings.ReplaceAll(resource, "/", "~1") return statusOp{verb, "/status/" + path + "/" + res, value} } @@ -260,7 +258,7 @@ func updateMasterNode(helper apihelper.APIHelpers) error { } // Advertise NFD version as an annotation - addAnnotations(node, Annotations{"master.version": version.Get()}) + addAnnotations(node, Annotations{AnnotationNs + "/master.version": version.Get()}) err = helper.UpdateNode(cli, node) if err != nil { stderrLogger.Printf("can't update node: %s", err.Error()) @@ -270,50 +268,52 @@ func updateMasterNode(helper apihelper.APIHelpers) error { return nil } -// Filter labels by namespace and name whitelist -func filterFeatureLabels(labels Labels, extraLabelNs []string, labelWhiteList *regexp.Regexp, extendedResourceNames []string) (Labels, ExtendedResources) { - for label := range labels { - split := strings.SplitN(label, "/", 2) - name := split[0] +// Filter labels by namespace and name whitelist, and, turn selected labels +// into extended resources. This function also handles proper namespacing of +// labels and ERs, i.e. adds the possibly missing default namespace for labels +// arriving through the gRPC API. +func filterFeatureLabels(labels Labels, extraLabelNs map[string]struct{}, labelWhiteList *regexp.Regexp, extendedResourceNames []string) (Labels, ExtendedResources) { + outLabels := Labels{} - // Check namespaced labels, filter out if ns is not whitelisted - if len(split) == 2 { - ns := split[0] - name = split[1] - for i, extraNs := range extraLabelNs { - if ns == extraNs { - break - } else if i == len(extraLabelNs)-1 { - stderrLogger.Printf("Namespace '%s' is not allowed. Ignoring label '%s'\n", ns, label) - delete(labels, label) - } + for label, value := range labels { + // Add possibly missing default ns + label := addNs(label, LabelNs) + + ns, name := splitNs(label) + + // Check label namespace, filter out if ns is not whitelisted + if ns != LabelNs { + if _, ok := extraLabelNs[ns]; !ok { + stderrLogger.Printf("Namespace '%s' is not allowed. Ignoring label '%s'\n", ns, label) + continue } } // Skip if label doesn't match labelWhiteList if !labelWhiteList.MatchString(name) { - stderrLogger.Printf("%s does not match the whitelist (%s) and will not be published.", name, labelWhiteList.String()) - delete(labels, label) + stderrLogger.Printf("%s (%s) does not match the whitelist (%s) and will not be published.", name, label, labelWhiteList.String()) + continue } + outLabels[label] = value } // Remove labels which are intended to be extended resources extendedResources := ExtendedResources{} for _, extendedResourceName := range extendedResourceNames { - // remove possibly given default LabelNs to keep annotations shorter - extendedResourceName = strings.TrimPrefix(extendedResourceName, LabelNs) - if _, ok := labels[extendedResourceName]; ok { - if _, err := strconv.Atoi(labels[extendedResourceName]); err != nil { - stderrLogger.Printf("bad label value encountered for extended resource: %s", err.Error()) + // Add possibly missing default ns + extendedResourceName = addNs(extendedResourceName, LabelNs) + if value, ok := outLabels[extendedResourceName]; ok { + if _, err := strconv.Atoi(value); err != nil { + stderrLogger.Printf("bad label value (%s: %s) encountered for extended resource: %s", extendedResourceName, value, err.Error()) continue // non-numeric label can't be used } - extendedResources[extendedResourceName] = labels[extendedResourceName] - delete(labels, extendedResourceName) + extendedResources[extendedResourceName] = value + delete(outLabels, extendedResourceName) } } - return labels, extendedResources + return outLabels, extendedResources } // Implement LabelerServer @@ -355,19 +355,21 @@ func (s *labelerServer) SetLabels(c context.Context, r *pb.SetLabelsRequest) (*p // Advertise NFD worker version, label names and extended resources as annotations labelKeys := make([]string, 0, len(labels)) for k := range labels { - labelKeys = append(labelKeys, k) + // Drop the ns part for labels in the default ns + labelKeys = append(labelKeys, strings.TrimPrefix(k, LabelNs+"/")) } sort.Strings(labelKeys) extendedResourceKeys := make([]string, 0, len(extendedResources)) for key := range extendedResources { - extendedResourceKeys = append(extendedResourceKeys, key) + // Drop the ns part if in the default ns + extendedResourceKeys = append(extendedResourceKeys, strings.TrimPrefix(key, LabelNs+"/")) } sort.Strings(extendedResourceKeys) - annotations := Annotations{"worker.version": r.NfdVersion, - "feature-labels": strings.Join(labelKeys, ","), - "extended-resources": strings.Join(extendedResourceKeys, ","), + annotations := Annotations{AnnotationNs + "/worker.version": r.NfdVersion, + AnnotationNs + "/feature-labels": strings.Join(labelKeys, ","), + AnnotationNs + "/extended-resources": strings.Join(extendedResourceKeys, ","), } err := updateNodeFeatures(s.apiHelper, r.NodeName, labels, annotations, extendedResources) @@ -398,8 +400,12 @@ func updateNodeFeatures(helper apihelper.APIHelpers, nodeName string, labels Lab statusOps := getExtendedResourceOps(node, extendedResources) // Remove old labels - if l, ok := node.Annotations[AnnotationNs+"feature-labels"]; ok { + if l, ok := node.Annotations[AnnotationNs+"/feature-labels"]; ok { oldLabels := strings.Split(l, ",") + for i, name := range oldLabels { + // Names in the annotation may omit the default ns + oldLabels[i] = addNs(name, LabelNs) + } removeLabels(node, oldLabels) } @@ -444,11 +450,7 @@ func removeLabelsWithPrefix(n *api.Node, search string) { // Removes NFD labels from a Node object func removeLabels(n *api.Node, labelNames []string) { for _, l := range labelNames { - if strings.Contains(l, "/") { - delete(n.Labels, l) - } else { - delete(n.Labels, LabelNs+l) - } + delete(n.Labels, l) } } @@ -456,16 +458,22 @@ func removeLabels(n *api.Node, labelNames []string) { func getExtendedResourceOps(n *api.Node, extendedResources ExtendedResources) []statusOp { var statusOps []statusOp - oldResources := strings.Split(n.Annotations[AnnotationNs+"extended-resources"], ",") + // Form a list of namespaced resource names managed by us + if old, ok := n.Annotations[AnnotationNs+"/extended-resources"]; ok { + oldResources := strings.Split(old, ",") + for i, name := range oldResources { + // Names in the annotation may omit the default ns + oldResources[i] = addNs(name, LabelNs) + } - // figure out which resources to remove - for _, resource := range oldResources { - if _, ok := n.Status.Capacity[api.ResourceName(addNs(resource, LabelNs))]; ok { - // check if the ext resource is still needed - _, extResNeeded := extendedResources[resource] - if !extResNeeded { - statusOps = append(statusOps, createStatusOp("remove", resource, "capacity", "")) - statusOps = append(statusOps, createStatusOp("remove", resource, "allocatable", "")) + // figure out which resources to remove + for _, resource := range oldResources { + if _, ok := n.Status.Capacity[api.ResourceName(resource)]; ok { + // check if the ext resource is still needed + if _, extResNeeded := extendedResources[resource]; !extResNeeded { + statusOps = append(statusOps, createStatusOp("remove", resource, "capacity", "")) + statusOps = append(statusOps, createStatusOp("remove", resource, "allocatable", "")) + } } } } @@ -473,7 +481,7 @@ func getExtendedResourceOps(n *api.Node, extendedResources ExtendedResources) [] // figure out which resources to replace and which to add for resource, value := range extendedResources { // check if the extended resource already exists with the same capacity in the node - if quantity, ok := n.Status.Capacity[api.ResourceName(addNs(resource, LabelNs))]; ok { + if quantity, ok := n.Status.Capacity[api.ResourceName(resource)]; ok { val, _ := quantity.AsInt64() if strconv.FormatInt(val, 10) != value { statusOps = append(statusOps, createStatusOp("replace", resource, "capacity", value)) @@ -489,20 +497,16 @@ func getExtendedResourceOps(n *api.Node, extendedResources ExtendedResources) [] } // Add NFD labels to a Node object. -func addLabels(n *api.Node, labels map[string]string) { +func addLabels(n *api.Node, labels Labels) { for k, v := range labels { - if strings.Contains(k, "/") { - n.Labels[k] = v - } else { - n.Labels[LabelNs+k] = v - } + n.Labels[k] = v } } // Add Annotations to a Node object -func addAnnotations(n *api.Node, annotations map[string]string) { +func addAnnotations(n *api.Node, annotations Annotations) { for k, v := range annotations { - n.Annotations[AnnotationNs+k] = v + n.Annotations[k] = v } } @@ -511,5 +515,14 @@ func addNs(src string, nsToAdd string) string { if strings.Contains(src, "/") { return src } - return nsToAdd + src + return filepath.Join(nsToAdd, src) +} + +// splitNs splits a name into its namespace and name parts +func splitNs(fullname string) (string, string) { + split := strings.SplitN(fullname, "/", 2) + if len(split) == 2 { + return split[0], split[1] + } + return "", fullname } diff --git a/test/e2e/node_feature_discovery.go b/test/e2e/node_feature_discovery.go index e2cf763f2..e80b1f76c 100644 --- a/test/e2e/node_feature_discovery.go +++ b/test/e2e/node_feature_discovery.go @@ -458,9 +458,9 @@ var _ = framework.KubeDescribe("[NFD] Node Feature Discovery", func() { ginkgo.It("it should decorate the node with the fake feature labels", func() { fakeFeatureLabels := map[string]string{ - master.LabelNs + "fake-fakefeature1": "true", - master.LabelNs + "fake-fakefeature2": "true", - master.LabelNs + "fake-fakefeature3": "true", + master.LabelNs + "/fake-fakefeature1": "true", + master.LabelNs + "/fake-fakefeature2": "true", + master.LabelNs + "/fake-fakefeature3": "true", } // Remove pre-existing stale annotations and labels