Repository: kubernetes-sigs/node-feature-discovery
Pull request #1228 (2023-05-26 13:31:04)

nfd-master: add validation of label names and values
Validate labels before trying to update the node. Makes us fail early nad prevent useless retries in case invalid labels are tried.

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/1228#" 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: /assign @PiotrProkop 
/cherrypick release-0.13
k8s-infra-cherrypick-robot: @marquiz: once the present PR merges, I will cherry-pick it on top of release-0.13 in a new PR and assign it to you.

<details>

In response to [this](https://github.com/kubernetes-sigs/node-feature-discovery/pull/1228#issuecomment-1564404887):

>/assign @PiotrProkop 
>/cherrypick release-0.13


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>
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1228?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) Report
> Merging [#1228](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1228?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) (291ba82) into [master](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/commit/7d715109e314099a3abb9f9edef95c8dc5949a29?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) (7d71510) will **increase** coverage by `0.05%`.
> The diff coverage is `100.00%`.

<details><summary>Additional details and impacted files</summary>


[![Impacted file tree graph](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1228/graphs/tree.svg?width=650&height=150&src=pr&token=mEZgtJQ1vg&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs)](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1228?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs)

```diff
@@            Coverage Diff             @@
##           master    #1228      +/-   ##
==========================================
+ Coverage   28.66%   28.72%   +0.05%     
==========================================
  Files          51       51              
  Lines        7165     7171       +6     
==========================================
+ Hits         2054     2060       +6     
  Misses       4898     4898              
  Partials      213      213              
```


| [Impacted Files](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1228?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) | Coverage Δ | |
|---|---|---|
| [pkg/nfd-master/nfd-master.go](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1228?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs#diff-cGtnL25mZC1tYXN0ZXIvbmZkLW1hc3Rlci5nbw==) | `42.02% <100.00%> (+0.39%)` | :arrow_up: |

</details>

Patch:
diff --git a/pkg/nfd-master/nfd-master.go b/pkg/nfd-master/nfd-master.go
index 98ef6689d..b5a31cbd5 100644
--- a/pkg/nfd-master/nfd-master.go
+++ b/pkg/nfd-master/nfd-master.go
@@ -41,6 +41,7 @@ import (
 	k8sQuantity "k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	k8sLabels "k8s.io/apimachinery/pkg/labels"
+	k8svalidation "k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/client-go/kubernetes"
 	restclient "k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/leaderelection"
@@ -490,7 +491,7 @@ func (m *nfdMaster) filterFeatureLabels(labels Labels) (Labels, ExtendedResource
 		// Add possibly missing default ns
 		name := addNs(name, nfdv1alpha1.FeatureLabelNs)
 
-		if err := m.filterFeatureLabel(name); err != nil {
+		if err := m.filterFeatureLabel(name, value); err != nil {
 			klog.Errorf("ignoring label %s=%v: %v", name, value, err)
 		} else {
 			outLabels[name] = value
@@ -516,7 +517,12 @@ func (m *nfdMaster) filterFeatureLabels(labels Labels) (Labels, ExtendedResource
 	return outLabels, extendedResources
 }
 
-func (m *nfdMaster) filterFeatureLabel(name string) error {
+func (m *nfdMaster) filterFeatureLabel(name, value string) error {
+	//Validate label name
+	if errs := k8svalidation.IsQualifiedName(name); len(errs) > 0 {
+		return fmt.Errorf("invalid name %q: %s", name, strings.Join(errs, "; "))
+	}
+
 	// Check label namespace, filter out if ns is not whitelisted
 	ns, base := splitNs(name)
 	if ns != nfdv1alpha1.FeatureLabelNs && ns != nfdv1alpha1.ProfileLabelNs &&
@@ -533,6 +539,12 @@ func (m *nfdMaster) filterFeatureLabel(name string) error {
 	if !m.config.LabelWhiteList.Regexp.MatchString(base) {
 		return fmt.Errorf("%s (%s) does not match the whitelist (%s)", base, name, m.config.LabelWhiteList.Regexp.String())
 	}
+
+	// Validate the label value
+	if errs := k8svalidation.IsValidLabelValue(value); len(errs) > 0 {
+		return fmt.Errorf("invalid value %q: %s", value, strings.Join(errs, "; "))
+	}
+
 	return nil
 }
 


Test patch:
diff --git a/pkg/nfd-master/nfd-master-internal_test.go b/pkg/nfd-master/nfd-master-internal_test.go
index ffcac269c..9e828b302 100644
--- a/pkg/nfd-master/nfd-master-internal_test.go
+++ b/pkg/nfd-master/nfd-master-internal_test.go
@@ -365,8 +365,10 @@ func TestSetLabels(t *testing.T) {
 				"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"}
+				vendorFeatureLabel:               "val-6",
+				vendorProfileLabel:               "val-7",
+				"--invalid-name--":               "valid-val",
+				"valid-name":                     "--invalid-val--"}
 			expectedPatches := []apihelper.JsonPatch{
 				apihelper.NewJsonPatch("add", "/metadata/annotations", instance+"."+nfdv1alpha1.WorkerVersionAnnotation, workerVer),
 				apihelper.NewJsonPatch("add", "/metadata/annotations",