Repository: kubernetes-sigs/node-feature-discovery
Pull request #307 (2020-04-28 12:11:26)

nfd-worker: fix --label-whitelist
After commenting #306 I looked into the code and was not happy what I found (with my own work).

Unify handling of --label-whitelist in nfd-worker and nfd-master. That is,
in nfd-worker, apply the regexp filter on non-namespaced part of the
label name.

Brief history:
1. Originally the whitelist regexp was applied on the full namespaced
   label name (that would be e.g.
   'feature.node.kubernetes.io/cpu-cpuid.AVX' in the current nfd version)

2. Commit 81752b2d changed the behavior so that the regexp was applied
   on the non-namespaced part (that would be `cpu-cpuid.AVX`)

3. Commit 40918827 added support for custom label namespaces. With this
   change, the label whitelist handling diverged between nfd-worker and
   nfd-master. In nfd-master the whitelist regexp is always applied on
   the non-namespaced label name. However, in nfd-worker the whitelist
   handling is two-fold (and inconsistent): for labels in the standard
   nfd namespace regexp is applied on the non-namespaced part (e.g.
   `cpu-cpuid.AVX`, but, for labels in custom namespaces the regexp is
   applied on the full name (e.g. `example.com/my-feature`).

This patch changes nfd-worker to behave similarly to nfd-master. The
namespace part is now always omitted, which should be easier for the
users to comprehend.

Also, fixes a bug in the label name prefixing so that the name of the
feature source is not prefixed into labels with custom label namespace
(effectively mangling the intended namespace). For example, previously a
'example.com/feature' label from the 'custom' feature source would be
prefixed with the source name, mangling it to
'custom-example.com/feature'.

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/307#" 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":[]} -->
marquiz: Forgot to add that this is one possible way to "fix" this. Alternatives include:
1. Change nfd-master to behave similarly to nfd-worker, instead
2. Revert back to the original, i.e. always apply the regexp to the fully namespaced label name

/hold
Ethyling: I think we should apply the same policy for labels regardless of the namespace (default or not). 

However, I don't know if we should apply the filter on the namespace or not. I would say that applying the whitelisting on namespaces would add the possibility to filter labels as we want on each node. For example, currently we can't filter namespaces on nfd-worker, we can only do this on nfd-master, so it would allow us to limit namespaces per node. But it would add more "complexity" on this feature for the user.

What do you think?
marquiz: These were the same questions I was pondering.

The reason I ended up the solution implemented here (i.e. whitelisting NOT applied on namespace) is that 
1. nfd-master already has other means of controlling the namespaces (`--extra-label-ns`). It is not exactly the same thing, but, probably enough
2. Labels in non-default namespaces are always result of user-made customization (hooks, feature files or the `custom` feature source). Thus, it's less likely that the user would want to filter out labels that she has has specifically enabled herself.
Ethyling: > The reason I ended up the solution implemented here (i.e. whitelisting NOT applied on namespace) is that
> 
>     1. nfd-master already has other means of controlling the namespaces (`--extra-label-ns`). It is not exactly the same thing, but, probably enough
> 
>     2. Labels in non-default namespaces are always result of user-made customization (hooks, feature files or the `custom` feature source). Thus, it's less likely that the user would want to filter out labels that she has has specifically enabled herself.

Sounds reasonable to me. I will take a look at the code and test it.

mythi: > Unify handling of --label-whitelist in nfd-worker and nfd-master. 

Maybe slightly off-topic but what's the use-case for double filtering?
marquiz: > Maybe slightly off-topic but what's the use-case for double filtering?

In short: historical reasons 😄 It was "cloned" when NFD was split to master and worker.
marquiz: /unhold
Ethyling: LGTM but I think we should update the README to specify that the whitelist is not applied on the namespace part. WDYT?
k8s-ci-robot: @marquiz: PR needs rebase.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

Patch:
diff --git a/README.md b/README.md
index 713412838..1d1f80b21 100644
--- a/README.md
+++ b/README.md
@@ -74,7 +74,10 @@ nfd-master.
                                   has been enabled.
   --no-publish                    Do not publish feature labels
   --label-whitelist=<pattern>     Regular expression to filter label names to
-                                  publish to the Kubernetes API server. [Default: ]
+                                  publish to the Kubernetes API server.
+                                  NB: the label namespace is omitted i.e. the filter
+                                  is only applied to the name part after '/'.
+                                  [Default: ]
   --extra-label-ns=<list>         Comma separated list of allowed extra label namespaces
                                   [Default: ]
   --resource-labels=<list>        Comma separated list of labels to be exposed as extended resources.
@@ -129,11 +132,14 @@ nfd-worker.
                               in testing
                               [Default: ]
   --sources=<sources>         Comma separated list of feature sources.
-                              [Default: cpu,iommu,kernel,local,memory,network,pci,storage,system,usb]
+                              [Default: cpu,custom,iommu,kernel,local,memory,network,pci,storage,system,usb]
   --no-publish                Do not publish discovered features to the
                               cluster-local Kubernetes API server.
   --label-whitelist=<pattern> Regular expression to filter label names to
-                              publish to the Kubernetes API server. [Default: ]
+                              publish to the Kubernetes API server.
+                              NB: the label namespace is omitted i.e. the filter
+                              is only applied to the name part after '/'.
+                              [Default: ]
   --oneshot                   Label once and exit.
   --sleep-interval=<seconds>  Time to sleep between re-labeling. Non-positive
                               value implies no re-labeling (i.e. infinite
diff --git a/cmd/nfd-master/main.go b/cmd/nfd-master/main.go
index c40203ca0..894cc9778 100644
--- a/cmd/nfd-master/main.go
+++ b/cmd/nfd-master/main.go
@@ -85,7 +85,10 @@ func argsParse(argv []string) (master.Args, error) {
                                   has been enabled.
   --no-publish                    Do not publish feature labels
   --label-whitelist=<pattern>     Regular expression to filter label names to
-                                  publish to the Kubernetes API server. [Default: ]
+                                  publish to the Kubernetes API server.
+                                  NB: the label namespace is omitted i.e. the filter
+                                  is only applied to the name part after '/'.
+                                  [Default: ]
   --extra-label-ns=<list>         Comma separated list of allowed extra label namespaces
                                   [Default: ]
   --resource-labels=<list>        Comma separated list of labels to be exposed as extended resources.
diff --git a/cmd/nfd-worker/main.go b/cmd/nfd-worker/main.go
index b5e3d6faf..1b4606782 100644
--- a/cmd/nfd-worker/main.go
+++ b/cmd/nfd-worker/main.go
@@ -95,7 +95,10 @@ func argsParse(argv []string) (worker.Args, error) {
   --no-publish                Do not publish discovered features to the
                               cluster-local Kubernetes API server.
   --label-whitelist=<pattern> Regular expression to filter label names to
-                              publish to the Kubernetes API server. [Default: ]
+                              publish to the Kubernetes API server.
+                              NB: the label namespace is omitted i.e. the filter
+                              is only applied to the name part after '/'.
+                              [Default: ]
   --oneshot                   Label once and exit.
   --sleep-interval=<seconds>  Time to sleep between re-labeling. Non-positive
                               value implies no re-labeling (i.e. infinite
diff --git a/pkg/nfd-worker/nfd-worker.go b/pkg/nfd-worker/nfd-worker.go
index de43e331a..2468c12ac 100644
--- a/pkg/nfd-worker/nfd-worker.go
+++ b/pkg/nfd-worker/nfd-worker.go
@@ -315,7 +315,7 @@ func createFeatureLabels(sources []source.FeatureSource, labelWhiteList *regexp.
 
 	// Do feature discovery from all configured sources.
 	for _, source := range sources {
-		labelsFromSource, err := getFeatureLabels(source)
+		labelsFromSource, err := getFeatureLabels(source, labelWhiteList)
 		if err != nil {
 			stderrLogger.Printf("discovery failed for source [%s]: %s", source.Name(), err.Error())
 			stderrLogger.Printf("continuing ...")
@@ -325,11 +325,6 @@ func createFeatureLabels(sources []source.FeatureSource, labelWhiteList *regexp.
 		for name, value := range labelsFromSource {
 			// Log discovered feature.
 			stdoutLogger.Printf("%s = %s", name, value)
-			// Skip if label doesn't match labelWhiteList
-			if !labelWhiteList.Match([]byte(name)) {
-				stderrLogger.Printf("%s does not match the whitelist (%s) and will not be published.", name, labelWhiteList.String())
-				continue
-			}
 			labels[name] = value
 		}
 	}
@@ -338,7 +333,7 @@ func createFeatureLabels(sources []source.FeatureSource, labelWhiteList *regexp.
 
 // getFeatureLabels returns node labels for features discovered by the
 // supplied source.
-func getFeatureLabels(source source.FeatureSource) (labels Labels, err error) {
+func getFeatureLabels(source source.FeatureSource, labelWhiteList *regexp.Regexp) (labels Labels, err error) {
 	defer func() {
 		if r := recover(); r != nil {
 			stderrLogger.Printf("panic occurred during discovery of source [%s]: %v", source.Name(), r)
@@ -351,24 +346,33 @@ func getFeatureLabels(source source.FeatureSource) (labels Labels, err error) {
 	if err != nil {
 		return nil, err
 	}
+
+	// Prefix for labels in the default namespace
+	prefix := source.Name() + "-"
+	switch source.(type) {
+	case local.Source:
+		// Do not prefix labels from the hooks
+		prefix = ""
+	}
+
 	for k, v := range features {
-		// Validate label name
-		prefix := source.Name() + "-"
-		switch source.(type) {
-		case local.Source:
-			// Do not prefix labels from the hooks
-			prefix = ""
+		// Split label name into namespace and name compoents. Use dummy 'ns'
+		// default namespace because there is no function to validate just
+		// the name part
+		split := strings.SplitN(k, "/", 2)
+
+		label := prefix + split[0]
+		nameForValidation := "ns/" + label
+		nameForWhiteListing := label
+
+		if len(split) == 2 {
+			label = k
+			nameForValidation = label
+			nameForWhiteListing = split[1]
 		}
 
-		label := prefix + k
-		// Validate label name. Use dummy namespace 'ns' because there is no
-		// function to validate just the name part
-		labelName := "ns/" + label
-		// Do not use dummy namespace if there is already a namespace
-		if strings.Contains(label, "/") {
-			labelName = label
-		}
-		errs := validation.IsQualifiedName(labelName)
+		// Validate label name.
+		errs := validation.IsQualifiedName(nameForValidation)
 		if len(errs) > 0 {
 			stderrLogger.Printf("Ignoring invalid feature name '%s': %s", label, errs)
 			continue
@@ -382,6 +386,12 @@ func getFeatureLabels(source source.FeatureSource) (labels Labels, err error) {
 			continue
 		}
 
+		// Skip if label doesn't match labelWhiteList
+		if !labelWhiteList.MatchString(nameForWhiteListing) {
+			stderrLogger.Printf("%q does not match the whitelist (%s) and will not be published.", nameForWhiteListing, labelWhiteList.String())
+			continue
+		}
+
 		labels[label] = value
 	}
 	return labels, nil


Test patch:
diff --git a/pkg/nfd-worker/nfd-worker-internal_test.go b/pkg/nfd-worker/nfd-worker-internal_test.go
index 7716cdbb2..5483dbb26 100644
--- a/pkg/nfd-worker/nfd-worker-internal_test.go
+++ b/pkg/nfd-worker/nfd-worker-internal_test.go
@@ -21,6 +21,7 @@ import (
 	"io/ioutil"
 	"os"
 	"regexp"
+	"strings"
 	"testing"
 
 	. "github.com/smartystreets/goconvey/convey"
@@ -32,25 +33,26 @@ import (
 	"sigs.k8s.io/node-feature-discovery/source/panic_fake"
 )
 
+const fakeFeatureSourceName string = "testSource"
+
 func TestDiscoveryWithMockSources(t *testing.T) {
 	Convey("When I discover features from fake source and update the node using fake client", t, func() {
 		mockFeatureSource := new(source.MockFeatureSource)
-		fakeFeatureSourceName := string("testSource")
-		fakeFeatureNames := []string{"testfeature1", "testfeature2", "testfeature3"}
-		fakeFeatures := source.Features{}
-		fakeFeatureLabels := Labels{}
-		for _, f := range fakeFeatureNames {
-			fakeFeatures[f] = true
-			labelName := fakeFeatureSourceName + "-" + f
-			fakeFeatureLabels[labelName] = "true"
-		}
+		allFeatureNames := []string{"testfeature1", "testfeature2", "test.ns/test", "test.ns/foo", "/no-ns-label", "invalid/test/feature"}
+		whiteListFeatureNames := []string{"testfeature1", "testfeature2", "test.ns/test"}
+
+		fakeFeatures, _ := makeFakeFeatures(allFeatureNames)
+		_, fakeFeatureLabels := makeFakeFeatures(whiteListFeatureNames)
+
 		fakeFeatureSource := source.FeatureSource(mockFeatureSource)
 
+		labelWhiteList := regexp.MustCompile("^test")
+
 		Convey("When I successfully get the labels from the mock source", func() {
 			mockFeatureSource.On("Name").Return(fakeFeatureSourceName)
 			mockFeatureSource.On("Discover").Return(fakeFeatures, nil)
 
-			returnedLabels, err := getFeatureLabels(fakeFeatureSource)
+			returnedLabels, err := getFeatureLabels(fakeFeatureSource, labelWhiteList)
 			Convey("Proper label is returned", func() {
 				So(returnedLabels, ShouldResemble, fakeFeatureLabels)
 			})
@@ -63,7 +65,7 @@ func TestDiscoveryWithMockSources(t *testing.T) {
 			expectedError := errors.New("fake error")
 			mockFeatureSource.On("Discover").Return(nil, expectedError)
 
-			returnedLabels, err := getFeatureLabels(fakeFeatureSource)
+			returnedLabels, err := getFeatureLabels(fakeFeatureSource, labelWhiteList)
 			Convey("No label is returned", func() {
 				So(returnedLabels, ShouldBeNil)
 			})
@@ -74,6 +76,21 @@ func TestDiscoveryWithMockSources(t *testing.T) {
 	})
 }
 
+func makeFakeFeatures(names []string) (source.Features, Labels) {
+	features := source.Features{}
+	labels := Labels{}
+	for _, f := range names {
+		features[f] = true
+		labelName := fakeFeatureSourceName + "-" + f
+		if strings.IndexByte(f, '/') >= 0 {
+			labelName = f
+		}
+		labels[labelName] = "true"
+	}
+
+	return features, labels
+}
+
 func TestConfigParse(t *testing.T) {
 	Convey("When parsing configuration file", t, func() {
 		Convey("When non-accessible file is given", func() {
@@ -209,7 +226,7 @@ func TestGetFeatureLabels(t *testing.T) {
 	Convey("When I get feature labels and panic occurs during discovery of a feature source", t, func() {
 		fakePanicFeatureSource := source.FeatureSource(new(panicfake.Source))
 
-		returnedLabels, err := getFeatureLabels(fakePanicFeatureSource)
+		returnedLabels, err := getFeatureLabels(fakePanicFeatureSource, regexp.MustCompile(""))
 		Convey("No label is returned", func() {
 			So(len(returnedLabels), ShouldEqual, 0)
 		})