From 1d012a28cd1b67580e53b9f97d469d0ae560c7b3 Mon Sep 17 00:00:00 2001 From: Markus Lehtonen Date: Wed, 8 Nov 2023 09:51:19 +0200 Subject: [PATCH] Option to stop implicitly adding default prefix to names Add new autoDefaultNs (default is "true") config option to nfd-master. Setting the config option to false stops NFD from automatically adding the "feature.node.kubernetes.io/" prefix to labels, annotations and extended resources. Taints are not affected as for them no prefix is automatically added. The user-visible part of enabling the option change is that NodeFeatureRules, local feature files, hooks and configuration of the "custom" may need to be altereda (if the auto-prefixing is relied on). For now, the config option defaults to "true", meaning no change in default behavior. However, the intent is to change the default to "false" in a future release, deprecating the option and eventually removing it (forcing it to "false"). The goal of stopping doing "auto-prefixing" is to simplify the operation (of nfd and users). Make the naming more straightforward and easier to understand and debug (kind of WYSIWYG), eliminating peculiar corner cases: 1. Make validation simpler and unambiguous 2. Remove "overloading" of names, i.e. the mapping two values to the same actual name. E.g. previously something like labels: feature.node.kubernetes.io/foo: bar foo: baz Could actually result in node label: feature.node.kubernetes.io/foo: baz 3. Make the processing/usagee of the "rule.matched" and "local.labels" feature in NodeFeatureRules unambiguous and more understadable. E.g. previously you could have node label "feature.node.kubernetes.io/local-foo: bar" but in the NodeFeatureRule you'd need to use the unprefixed name "local-foo" or the fully prefixed name, depending on what was specified in the feature file (or hook) on the node(s). NOTE: setting autoDefaultNs to false is a breaking change for users who rely on automatic prefixing with the default feature.node.kubernetes.io/ namespace. NodeFeatureRules, feature files, hooks and custom rules (configuration of the "custom" source of nfd-worker) will need to be altered. Unprefixed labels, annoations and extended resources will be denied by nfd-master. --- .../master-config/nfd-master.conf.example | 1 + .../worker-config/nfd-worker.conf.example | 14 +-- .../helm/node-feature-discovery/values.yaml | 15 +-- .../master-configuration-reference.md | 32 +++++ docs/usage/customization-guide.md | 92 +++++++------- examples/nodefeaturerule.yaml | 4 +- pkg/nfd-master/nfd-master-internal_test.go | 113 ++++++++++++++---- pkg/nfd-master/nfd-master.go | 44 +++++-- pkg/nfd-worker/nfd-worker-internal_test.go | 9 +- pkg/nfd-worker/nfd-worker.go | 39 +++--- 10 files changed, 245 insertions(+), 118 deletions(-) diff --git a/deployment/components/master-config/nfd-master.conf.example b/deployment/components/master-config/nfd-master.conf.example index 11d320272..8aa0cb29c 100644 --- a/deployment/components/master-config/nfd-master.conf.example +++ b/deployment/components/master-config/nfd-master.conf.example @@ -1,4 +1,5 @@ # noPublish: false +# autoDefaultNs: true # extraLabelNs: ["added.ns.io","added.kubernets.io"] # denyLabelNs: ["denied.ns.io","denied.kubernetes.io"] # resourceLabels: ["vendor-1.com/feature-1","vendor-2.io/feature-2"] diff --git a/deployment/components/worker-config/nfd-worker.conf.example b/deployment/components/worker-config/nfd-worker.conf.example index b37878c76..f1450f168 100644 --- a/deployment/components/worker-config/nfd-worker.conf.example +++ b/deployment/components/worker-config/nfd-worker.conf.example @@ -82,7 +82,7 @@ # # The following feature demonstrates the capabilities of the matchFeatures # - name: "my custom rule" # labels: -# my-ng-feature: "true" +# "vendor.io/my-ng-feature": "true" # # matchFeatures implements a logical AND over all matcher terms in the # # list (i.e. all of the terms, or per-feature matchers, must match) # matchFeatures: @@ -153,7 +153,7 @@ # # The following feature demonstrates the capabilities of the matchAny # - name: "my matchAny rule" # labels: -# my-ng-feature-2: "my-value" +# "vendor.io/my-ng-feature-2": "my-value" # # matchAny implements a logical IF over all elements (sub-matchers) in # # the list (i.e. at least one feature matcher must match) # matchAny: @@ -177,7 +177,7 @@ # # The following features demonstreate label templating capabilities # - name: "my template rule" # labelsTemplate: | -# {{ range .system.osrelease }}my-system-feature.{{ .Name }}={{ .Value }} +# {{ range .system.osrelease }}vendor.io/my-system-feature.{{ .Name }}={{ .Value }} # {{ end }} # matchFeatures: # - feature: system.osrelease @@ -187,7 +187,7 @@ # # - name: "my template rule 2" # labelsTemplate: | -# {{ range .pci.device }}my-pci-device.{{ .class }}-{{ .device }}=with-cpuid +# {{ range .pci.device }}vendor.io/my-pci-device.{{ .class }}-{{ .device }}=with-cpuid # {{ end }} # matchFeatures: # - feature: pci.device @@ -202,7 +202,7 @@ # # previous labels and vars # - name: "my dummy kernel rule" # labels: -# "my.kernel.feature": "true" +# "vendor.io/my.kernel.feature": "true" # matchFeatures: # - feature: kernel.version # matchExpressions: @@ -217,10 +217,10 @@ # # - name: "my rule using backrefs" # labels: -# "my.backref.feature": "true" +# "vendor.io/my.backref.feature": "true" # matchFeatures: # - feature: rule.matched # matchExpressions: -# my.kernel.feature: {op: IsTrue} +# vendor.io/my.kernel.feature: {op: IsTrue} # my.dummy.var: {op: Gt, value: ["0"]} # diff --git a/deployment/helm/node-feature-discovery/values.yaml b/deployment/helm/node-feature-discovery/values.yaml index 74623fadf..66bd39787 100644 --- a/deployment/helm/node-feature-discovery/values.yaml +++ b/deployment/helm/node-feature-discovery/values.yaml @@ -16,6 +16,7 @@ master: enable: true config: ### # noPublish: false + # autoDefaultNs: true # extraLabelNs: ["added.ns.io","added.kubernets.io"] # denyLabelNs: ["denied.ns.io","denied.kubernetes.io"] # resourceLabels: ["vendor-1.com/feature-1","vendor-2.io/feature-2"] @@ -219,7 +220,7 @@ worker: # # The following feature demonstrates the capabilities of the matchFeatures # - name: "my custom rule" # labels: - # my-ng-feature: "true" + # "vendor.io/my-ng-feature": "true" # # matchFeatures implements a logical AND over all matcher terms in the # # list (i.e. all of the terms, or per-feature matchers, must match) # matchFeatures: @@ -290,7 +291,7 @@ worker: # # The following feature demonstrates the capabilities of the matchAny # - name: "my matchAny rule" # labels: - # my-ng-feature-2: "my-value" + # "vendor.io/my-ng-feature-2": "my-value" # # matchAny implements a logical IF over all elements (sub-matchers) in # # the list (i.e. at least one feature matcher must match) # matchAny: @@ -314,7 +315,7 @@ worker: # # The following features demonstreate label templating capabilities # - name: "my template rule" # labelsTemplate: | - # {{ range .system.osrelease }}my-system-feature.{{ .Name }}={{ .Value }} + # {{ range .system.osrelease }}vendor.io/my-system-feature.{{ .Name }}={{ .Value }} # {{ end }} # matchFeatures: # - feature: system.osrelease @@ -324,7 +325,7 @@ worker: # # - name: "my template rule 2" # labelsTemplate: | - # {{ range .pci.device }}my-pci-device.{{ .class }}-{{ .device }}=with-cpuid + # {{ range .pci.device }}vendor.io/my-pci-device.{{ .class }}-{{ .device }}=with-cpuid # {{ end }} # matchFeatures: # - feature: pci.device @@ -339,7 +340,7 @@ worker: # # previous labels and vars # - name: "my dummy kernel rule" # labels: - # "my.kernel.feature": "true" + # "vendor.io/my.kernel.feature": "true" # matchFeatures: # - feature: kernel.version # matchExpressions: @@ -354,11 +355,11 @@ worker: # # - name: "my rule using backrefs" # labels: - # "my.backref.feature": "true" + # "vendor.io/my.backref.feature": "true" # matchFeatures: # - feature: rule.matched # matchExpressions: - # my.kernel.feature: {op: IsTrue} + # vendor.io/my.kernel.feature: {op: IsTrue} # my.dummy.var: {op: Gt, value: ["0"]} # ### diff --git a/docs/reference/master-configuration-reference.md b/docs/reference/master-configuration-reference.md index 98a44c3b0..ebbfc72f8 100644 --- a/docs/reference/master-configuration-reference.md +++ b/docs/reference/master-configuration-reference.md @@ -70,6 +70,38 @@ Example: denyLabelNs: ["denied.ns.io","denied.kubernetes.io"] ``` +## autoDefaultNs + +The `autoDefaultNs` option controls the automatic prefixing of names. When set +to true (the default in {{ site.version }}) nfd-master automatically adds the +default `feature.node.kubernetes.io/` prefix to unprefixed labels, annotations +and extended resources - this is also the default behavior in NFD v0.13 and +earlier. When the option is set to `false`, no prefix will be prepended to +unprefixed names, effectively causing them to be filtered out (as NFD does not +currently allow unprefixed names of labels, annotations or extended resources). +The default will be changed to `false` in a future release. + +For example, with the `autoDefaultNs` set to `true`, a NodeFeatureRule with + +```yaml + labels: + foo: bar +``` + +Will turn into `feature.node.kubernetes.io/foo=bar` node label. With +`autoDefaultNs` set to `false`, no prefix is added and the label will be +filtered out. + +Note that taint keys are not affected by this option. + +Default: `true` + +Example: + +```yaml +autoDefaultNs: false +``` + ## resourceLabels **DEPRECATED**: [NodeFeatureRule](../usage/custom-resources.md#nodefeaturerule) diff --git a/docs/usage/customization-guide.md b/docs/usage/customization-guide.md index 40e33406b..a42a4769f 100644 --- a/docs/usage/customization-guide.md +++ b/docs/usage/customization-guide.md @@ -81,7 +81,7 @@ spec: vendor: "acme" # Labels to be created labels: - vendor-feature.enabled: "true" + vendor.io/feature.enabled: "true" ``` The object targets node named `node-1`. It lists two "flag type" features under @@ -92,8 +92,7 @@ labels but they will be used as input when the [`NodeFeatureRule`](#nodefeaturerule-custom-resource) objects are evaluated. In addition, the example requests directly the -`feature.node.kubenernetes.io/vendor-feature.enabled=true` node label to be -created. +`vendor.io/feature.enabled=true` node label to be created. The `nfd.node.kubernetes.io/node-name=` must be in place for each NodeFeature object as NFD uses it to determine the node which it is targeting. @@ -130,7 +129,7 @@ spec: rules: - name: "my sample rule" labels: - "my-sample-feature": "true" + "feature.node.kubernetes.io/my-sample-feature": "true" matchFeatures: - feature: kernel.loadedmodule matchExpressions: @@ -141,7 +140,7 @@ spec: ``` It specifies one rule which creates node label -`feature.node.kubenernetes.io/my-sample-feature=true` if both of the following +`feature.node.kubernetes.io/my-sample-feature=true` if both of the following conditions are true (`matchFeatures` implements a logical AND over the matchers): @@ -216,9 +215,9 @@ having the following contents (or alternatively a shell script following stdout output): ```plaintext -my-feature.1 -my-feature.2=myvalue -my.namespace/my-feature.3=456 +feature.node.kubernetes.io/my-feature.1 +feature.node.kubernetes.io/my-feature.2=myvalue +vendor.io/my-feature.3=456 ``` This will translate into the following node labels: @@ -226,7 +225,7 @@ This will translate into the following node labels: ```yaml feature.node.kubernetes.io/my-feature.1: "true" feature.node.kubernetes.io/my-feature.2: "myvalue" -my.namespace/my-feature.3: "456" +vendor.io/my-feature.3: "456" ``` ### Feature files @@ -277,12 +276,12 @@ key-value pairs, separated by newlines: ```plaintext # This is a comment -[=] +[=] ``` The label value defaults to `true`, if not specified. -Label namespace may be specified with `/[=]`. +Label namespace must be specified with `/[=]`. > **NOTE:** The feature file size limit it 64kB. The feature file will be > ignored if the size limit is exceeded. @@ -302,20 +301,20 @@ Considering the following file: ```plaintext # +expiry-time=2012-07-28T11:22:33Z -featureKey=featureValue +vendor.io/feature1=featureValue # +expiry-time=2080-07-28T11:22:33Z -featureKey2=featureValue2 +vendor.io/feature2=featureValue2 # +expiry-time=2070-07-28T11:22:33Z -featureKey3=featureValue3 +vendor.io/feature3=featureValue3 # +expiry-time=2002-07-28T11:22:33Z -featureKey4=featureValue4 +vendor.io/feature4=featureValue4 ``` -After processing the above file, only `featureKey2` and `featureKey3` would be -included in the list of accepted features. +After processing the above file, only `vendor.io/feature2` and +`vendor.io/feature3` would be included in the list of accepted features. > **NOTE:** The time format that we are supporting is RFC3339. Also, the `expiry-time` > tag is only evaluated in each re-discovery period, and the expiration of @@ -329,11 +328,12 @@ Considering the following file: ```plaintext # +no-feature -label-only=value +vendor.io/label-only=value -my-feature=value +vendor.io/my-feature=value + +vendor.io/foo=bar -foo=bar # +no-label foo=baz ``` @@ -343,19 +343,27 @@ Processing the above file would result in the following Features: ```yaml local.features: foo: baz - my-feature: value + vendor.io/my-feature: value local.labels: - label-only: value - my-feature: value + vendor.io/label-only: value + vendor.io/my-feature: value ``` and the following labels added to the Node: ```plaintext -feature.node.kubernetes.io/label-only=value -feature.node.kubernetes.io/my-feature=value +vendor.io/label-only=value +vendor.io/my-feature=value ``` +> **NOTE:** use of unprefixed label names (like `foo=bar`) should not be used. +> In NFD {{ site.version }} unprefixed names will be automatically prefixed +> with `feature.node.kubernetes.io/` but this will change in a future version +> (see +> [autoDefaultNs config option](../reference/master-configuration-reference.md#autoDefaultNs). +> Unprefixed names for plain Features (tagged with `# +no-label`) can be used +> without restrictions, however. + ### Mounts The standard NFD deployments contain `hostPath` mounts for @@ -395,7 +403,7 @@ sources: custom: - name: "my sample rule" labels: - "my-sample-feature": "true" + "feature.node.kubenernetes.io/my-sample-feature": "true" matchFeatures: - feature: kernel.loadedmodule matchExpressions: @@ -434,7 +442,7 @@ following content: ```yaml - name: "my e1000 rule" labels: - "e1000.present": "true" + "feature.node.kubenernetes.io/e1000.present": "true" matchFeatures: - feature: kernel.loadedmodule matchExpressions: @@ -459,8 +467,7 @@ Feature labels have the following format: The namespace part (i.e. prefix) of the labels is controlled by nfd: -- All built-in labels use `feature.node.kubernetes.io`. This is also - the default for user defined features that don't specify any namespace. +- All built-in labels use `feature.node.kubernetes.io`. - Namespaces may be excluded with the [`-deny-label-ns`](../reference/master-commandline-reference.md#-deny-label-ns) command line flag of nfd-master @@ -468,6 +475,9 @@ The namespace part (i.e. prefix) of the labels is controlled by nfd: [`-extra-label-ns`](../reference/master-commandline-reference.md#-extra-label-ns) command line flag of nfd-master. e.g: `nfd-master -deny-label-ns="*" -extra-label-ns=example.com` + - Built-in default namespaces `feature.node.kubernetes.io` and + `profile.node.kubernetes.io` (and their sub-namespaces) are always allowed + and cannot be denied. ## Feature rule format @@ -485,7 +495,7 @@ Take this rule as a referential example: ```yaml - name: "my feature rule" labels: - "my-special-feature": "my-value" + "feature.node.kubernetes.io/my-special-feature": "my-value" matchFeatures: - feature: cpu.cpuid matchExpressions: @@ -500,7 +510,7 @@ Take this rule as a referential example: class: {op: In, value: ["0200"]} ``` -This will yield `feature.node.kubenernetes.io/my-special-feature=my-value` node +This will yield `feature.node.kubernetes.io/my-special-feature=my-value` node label if all of these are true (`matchFeatures` implements a logical AND over the matchers): @@ -529,8 +539,8 @@ spec: rules: - name: "my dynamic label value rule" labels: - linux-lsm-enabled: "@kernel.config.LSM" - custom-label: "customlabel" + feature.node.kubernetes.io/linux-lsm-enabled: "@kernel.config.LSM" + feature.node.kubernetes.io/custom-label: "customlabel" ``` Label `linux-lsm-enabled` uses the `@` notation for dynamic values. @@ -576,8 +586,7 @@ spec: rules: - name: "annotation-example" annotations: - defaul-ns-annotation: "foo" - feature.node.kubernetes.io/defaul-ns-annotation-2: "bar" + feature.node.kubernetes.io/defaul-ns-annotation: "foo" custom.vendor.io/feature: "baz" matchFeatures: - feature: kernel.version @@ -591,7 +600,6 @@ This will yield into the following node annotations: annotations: ... feature.node.kubernetes.io/defaul-ns-annotation: "foo" - feature.node.kubernetes.io/defaul-ns-annotation-2: "bar" custom.vendor.io/feature: "baz" ... ``` @@ -602,8 +610,10 @@ NFD enforces some limitations to the namespace (or prefix)/ of the annotations: generally be used - the only exception is `feature.node.kubernetes.io/` and its sub-namespaces (like `sub.ns.feature.node.kubernetes.io`) -- unprefixed names will get prefixed with `feature.node.kubernetes.io/` - automatically (e.g. `foo` becomes `feature.node.kubernetes.io/foo`) +- unprefixed names (like `my-annotation`) should not be used. In NFD {{ + site.version }} unprefixed names will be automatically prefixed with + `feature.node.kubernetes.io/` but this will change in a future version (see + [autoDefaultNs config option](../reference/master-configuration-reference.md#autoDefaultNs). > **NOTE:** The `annotations` field has will only advertise features via node > annotations the features won't be advertised as node labels unless they are @@ -719,8 +729,10 @@ Resources names: generally be used - the only exception is `feature.node.kubernetes.io/` and its sub-namespaces (like `sub.ns.feature.node.kubernetes.io`) -- unprefixed names will get prefixed with `feature.node.kubernetes.io/` - automatically (e.g. `foo` becomes `feature.node.kubernetes.io/foo`) +- unprefixed names (like `my-er`) site.version }} unprefixed names will be + automatically prefixed with `feature.node.kubernetes.io/` but this will + change in a future version (see + [autoDefaultNs config option](../reference/master-configuration-reference.md#autoDefaultNs). > **NOTE:** `.extendedResources` is not supported by the > [custom feature source](#custom-feature-source) -- it can only be used in diff --git a/examples/nodefeaturerule.yaml b/examples/nodefeaturerule.yaml index 39bb699b4..06fd662e6 100644 --- a/examples/nodefeaturerule.yaml +++ b/examples/nodefeaturerule.yaml @@ -6,7 +6,7 @@ spec: rules: - name: "my sample rule" labels: - "my-sample-feature": "true" + "vendor.io/my-sample-feature": "true" matchFeatures: - feature: kernel.loadedmodule matchExpressions: @@ -28,4 +28,4 @@ spec: matchFeatures: - feature: kernel.version matchExpressions: - major: {op: Exists} \ No newline at end of file + major: {op: Exists} diff --git a/pkg/nfd-master/nfd-master-internal_test.go b/pkg/nfd-master/nfd-master-internal_test.go index e715cd666..4268ce387 100644 --- a/pkg/nfd-master/nfd-master-internal_test.go +++ b/pkg/nfd-master/nfd-master-internal_test.go @@ -370,12 +370,12 @@ func TestSetLabels(t *testing.T) { mockNode := newMockNode() mockCtx := context.Background() // In the gRPC request the label names may omit the default ns - mockLabels := map[string]string{"feature-1": "1", "feature-2": "val-2", "feature-3": "3"} + mockLabels := map[string]string{"feature.node.kubernetes.io/feature-1": "1", "example.io/feature-2": "val-2", "feature.node.kubernetes.io/feature-3": "3"} mockReq := &labeler.SetLabelsRequest{NodeName: workerName, NfdVersion: workerVer, Labels: mockLabels} mockLabelNames := make([]string, 0, len(mockLabels)) for k := range mockLabels { - mockLabelNames = append(mockLabelNames, k) + mockLabelNames = append(mockLabelNames, strings.TrimPrefix(k, nfdv1alpha1.FeatureLabelNs+"/")) } sort.Strings(mockLabelNames) @@ -386,7 +386,7 @@ func TestSetLabels(t *testing.T) { apihelper.NewJsonPatch("add", "/metadata/annotations", nfdv1alpha1.FeatureLabelsAnnotation, strings.Join(mockLabelNames, ",")), } for k, v := range mockLabels { - expectedPatches = append(expectedPatches, apihelper.NewJsonPatch("add", "/metadata/labels", nfdv1alpha1.FeatureLabelNs+"/"+k, v)) + expectedPatches = append(expectedPatches, apihelper.NewJsonPatch("add", "/metadata/labels", k, v)) } mockHelper.On("GetClient").Return(mockClient, nil) @@ -401,8 +401,8 @@ func TestSetLabels(t *testing.T) { Convey("When -label-whitelist is specified", func() { expectedPatches := []apihelper.JsonPatch{ - apihelper.NewJsonPatch("add", "/metadata/annotations", nfdv1alpha1.FeatureLabelsAnnotation, "feature-2"), - apihelper.NewJsonPatch("add", "/metadata/labels", nfdv1alpha1.FeatureLabelNs+"/feature-2", mockLabels["feature-2"]), + apihelper.NewJsonPatch("add", "/metadata/annotations", nfdv1alpha1.FeatureLabelsAnnotation, "example.io/feature-2"), + apihelper.NewJsonPatch("add", "/metadata/labels", "example.io/feature-2", mockLabels["example.io/feature-2"]), } mockMaster.config.LabelWhiteList.Regexp = *regexp.MustCompile("^f.*2$") @@ -460,13 +460,13 @@ func TestSetLabels(t *testing.T) { Convey("When -resource-labels is specified", func() { expectedPatches := []apihelper.JsonPatch{ - apihelper.NewJsonPatch("add", "/metadata/annotations", nfdv1alpha1.FeatureLabelsAnnotation, "feature-2"), + apihelper.NewJsonPatch("add", "/metadata/annotations", nfdv1alpha1.FeatureLabelsAnnotation, "example.io/feature-2"), apihelper.NewJsonPatch("add", "/metadata/annotations", nfdv1alpha1.ExtendedResourceAnnotation, "feature-1,feature-3"), - apihelper.NewJsonPatch("add", "/metadata/labels", nfdv1alpha1.FeatureLabelNs+"/feature-2", mockLabels["feature-2"]), + apihelper.NewJsonPatch("add", "/metadata/labels", "example.io/feature-2", mockLabels["example.io/feature-2"]), } expectedStatusPatches := []apihelper.JsonPatch{ - apihelper.NewJsonPatch("add", "/status/capacity", nfdv1alpha1.FeatureLabelNs+"/feature-1", mockLabels["feature-1"]), - apihelper.NewJsonPatch("add", "/status/capacity", nfdv1alpha1.FeatureLabelNs+"/feature-3", mockLabels["feature-3"]), + apihelper.NewJsonPatch("add", "/status/capacity", "feature.node.kubernetes.io/feature-1", mockLabels["feature.node.kubernetes.io/feature-1"]), + apihelper.NewJsonPatch("add", "/status/capacity", "feature.node.kubernetes.io/feature-3", mockLabels["feature.node.kubernetes.io/feature-3"]), } mockMaster.config.ResourceLabels = map[string]struct{}{"feature.node.kubernetes.io/feature-3": {}, "feature-1": {}} @@ -502,29 +502,92 @@ func TestSetLabels(t *testing.T) { func TestFilterLabels(t *testing.T) { mockHelper := &apihelper.MockAPIHelpers{} mockMaster := newMockMaster(mockHelper) + mockMaster.deniedNs = deniedNs{ + normal: map[string]struct{}{"": struct{}{}, "kubernetes.io": struct{}{}, "denied.ns": struct{}{}}, + wildcard: map[string]struct{}{".kubernetes.io": struct{}{}, ".denied.subns": struct{}{}}, + } - Convey("When using dynamic values", t, func() { - labelName := "ns/testLabel" - labelValue := "@test.feature.LSM" - features := nfdv1alpha1.Features{ - Attributes: map[string]nfdv1alpha1.AttributeFeatureSet{ - "test.feature": nfdv1alpha1.AttributeFeatureSet{ - Elements: map[string]string{ - "LSM": "123", + type TC struct { + description string + labelName string + labelValue string + features nfdv1alpha1.Features + expectErr bool + expectedValue string + } + + tcs := []TC{ + TC{ + description: "Static value", + labelName: "example.io/test", + labelValue: "test-val", + expectedValue: "test-val", + }, + TC{ + description: "Dynamic value", + labelName: "example.io/testLabel", + labelValue: "@test.feature.LSM", + features: nfdv1alpha1.Features{ + Attributes: map[string]nfdv1alpha1.AttributeFeatureSet{ + "test.feature": nfdv1alpha1.AttributeFeatureSet{ + Elements: map[string]string{ + "LSM": "123", + }, }, }, }, - } - labelValue, err := mockMaster.filterFeatureLabel(labelName, labelValue, &features) + expectedValue: "123", + }, + TC{ + description: "Unprefixed should be denied", + labelName: "test-label", + labelValue: "test-value", + expectErr: true, + }, + TC{ + description: "kubernetes.io ns should be denied", + labelName: "kubernetes.io/test-label", + labelValue: "test-value", + expectErr: true, + }, + TC{ + description: "*.kubernetes.io ns should be denied", + labelName: "sub.ns.kubernetes.io/test-label", + labelValue: "test-value", + expectErr: true, + }, + TC{ + description: "denied.ns ns should be denied", + labelName: "denied.ns/test-label", + labelValue: "test-value", + expectErr: true, + }, + TC{ + description: "*.denied.subns ns should be denied", + labelName: "my.denied.subns/test-label", + labelValue: "test-value", + expectErr: true, + }, + } - Convey("Operation should succeed", func() { - So(err, ShouldBeNil) - }) + for _, tc := range tcs { + t.Run(tc.description, func(t *testing.T) { + labelValue, err := mockMaster.filterFeatureLabel(tc.labelName, tc.labelValue, &tc.features) - Convey("Label value should change", func() { - So(labelValue, ShouldEqual, "123") + if tc.expectErr { + Convey("Label should be filtered out", t, func() { + So(err, ShouldBeError) + }) + } else { + Convey("Label should not be filtered out", t, func() { + So(err, ShouldBeNil) + }) + Convey("Label value should be correct", t, func() { + So(labelValue, ShouldEqual, tc.expectedValue) + }) + } }) - }) + } } func TestCreatePatches(t *testing.T) { diff --git a/pkg/nfd-master/nfd-master.go b/pkg/nfd-master/nfd-master.go index 02dfdc604..caad5fb2c 100644 --- a/pkg/nfd-master/nfd-master.go +++ b/pkg/nfd-master/nfd-master.go @@ -71,6 +71,7 @@ type Annotations map[string]string // NFDConfig contains the configuration settings of NfdMaster. type NFDConfig struct { + AutoDefaultNs bool DenyLabelNs utils.StringSetVal ExtraLabelNs utils.StringSetVal LabelWhiteList utils.RegexpVal @@ -196,6 +197,7 @@ func newDefaultConfig() *NFDConfig { DenyLabelNs: utils.StringSetVal{}, ExtraLabelNs: utils.StringSetVal{}, NoPublish: false, + AutoDefaultNs: true, NfdApiParallelism: 10, ResourceLabels: utils.StringSetVal{}, EnableTaints: false, @@ -551,7 +553,6 @@ func (m *nfdMaster) filterFeatureLabels(labels Labels, features *nfdv1alpha1.Fea } func (m *nfdMaster) filterFeatureLabel(name, value string, features *nfdv1alpha1.Features) (string, error) { - //Validate label name if errs := k8svalidation.IsQualifiedName(name); len(errs) > 0 { return "", fmt.Errorf("invalid name %q: %s", name, strings.Join(errs, "; ")) @@ -763,10 +764,14 @@ 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) + if m.config.AutoDefaultNs { + features.Labels = addNsToMapKeys(features.Labels, nfdv1alpha1.FeatureLabelNs) + } for _, o := range objs[1:] { s := o.Spec.DeepCopy() - s.Labels = addNsToMapKeys(s.Labels, nfdv1alpha1.FeatureLabelNs) + if m.config.AutoDefaultNs { + s.Labels = addNsToMapKeys(s.Labels, nfdv1alpha1.FeatureLabelNs) + } s.MergeInto(features) } @@ -789,7 +794,7 @@ func (m *nfdMaster) nfdAPIUpdateOneNode(nodeName string) error { // filterExtendedResources filters extended resources and returns a map // of valid extended resources. -func filterExtendedResources(features *nfdv1alpha1.Features, extendedResources ExtendedResources) ExtendedResources { +func (m *nfdMaster) filterExtendedResources(features *nfdv1alpha1.Features, extendedResources ExtendedResources) ExtendedResources { outExtendedResources := ExtendedResources{} for name, value := range extendedResources { capacity, err := filterExtendedResource(name, value, features) @@ -836,7 +841,11 @@ 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 { - labels = addNsToMapKeys(labels, nfdv1alpha1.FeatureLabelNs) + if m.config.AutoDefaultNs { + labels = addNsToMapKeys(labels, nfdv1alpha1.FeatureLabelNs) + } else if labels == nil { + labels = make(map[string]string) + } crLabels, crAnnotations, crExtendedResources, crTaints := m.processNodeFeatureRule(nodeName, features) @@ -853,7 +862,7 @@ func (m *nfdMaster) refreshNodeFeatures(cli *kubernetes.Clientset, nodeName stri for k, v := range crExtendedResources { extendedResources[k] = v } - extendedResources = filterExtendedResources(features, extendedResources) + extendedResources = m.filterExtendedResources(features, extendedResources) // Annotations annotations := m.filterFeatureAnnotations(crAnnotations) @@ -1011,13 +1020,22 @@ func (m *nfdMaster) processNodeFeatureRule(nodeName string, features *nfdv1alpha continue } taints = append(taints, ruleOut.Taints...) - for k, v := range addNsToMapKeys(ruleOut.Labels, nfdv1alpha1.FeatureLabelNs) { + + l := ruleOut.Labels + e := ruleOut.ExtendedResources + a := ruleOut.Annotations + if m.config.AutoDefaultNs { + l = addNsToMapKeys(ruleOut.Labels, nfdv1alpha1.FeatureLabelNs) + e = addNsToMapKeys(ruleOut.ExtendedResources, nfdv1alpha1.ExtendedResourceNs) + a = addNsToMapKeys(ruleOut.Annotations, nfdv1alpha1.FeatureAnnotationNs) + } + for k, v := range l { labels[k] = v } - for k, v := range addNsToMapKeys(ruleOut.ExtendedResources, nfdv1alpha1.ExtendedResourceNs) { + for k, v := range e { extendedResources[k] = v } - for k, v := range addNsToMapKeys(ruleOut.Annotations, nfdv1alpha1.FeatureAnnotationNs) { + for k, v := range a { annotations[k] = v } @@ -1430,9 +1448,13 @@ func (m *nfdMaster) filterFeatureAnnotations(annotations map[string]string) map[ } // 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 + // If the namespace is denied the annotation will be ignored + if ns == "" { + klog.ErrorS(fmt.Errorf("labels without namespace (prefix/) not allowed"), fmt.Sprintf("Ignoring annotation %s", annotation)) + continue + } if ns == "kubernetes.io" || strings.HasSuffix(ns, ".kubernetes.io") || ns == nfdv1alpha1.AnnotationNs { - klog.ErrorS(fmt.Errorf("namespace %v is not allowed", ns), fmt.Sprintf("Ignoring annotation %v\n", annotation)) + klog.ErrorS(fmt.Errorf("namespace %q is not allowed", ns), fmt.Sprintf("Ignoring annotation %s", annotation)) continue } } diff --git a/pkg/nfd-worker/nfd-worker-internal_test.go b/pkg/nfd-worker/nfd-worker-internal_test.go index 94852ee3a..7747c1fc5 100644 --- a/pkg/nfd-worker/nfd-worker-internal_test.go +++ b/pkg/nfd-worker/nfd-worker-internal_test.go @@ -28,6 +28,7 @@ import ( "github.com/stretchr/testify/mock" "github.com/vektra/errors" + nfdv1alpha1 "sigs.k8s.io/node-feature-discovery/pkg/apis/nfd/v1alpha1" "sigs.k8s.io/node-feature-discovery/pkg/labeler" "sigs.k8s.io/node-feature-discovery/pkg/utils" "sigs.k8s.io/node-feature-discovery/source" @@ -84,7 +85,7 @@ func makeFakeFeatures(names []string) (source.FeatureLabels, Labels) { labels := Labels{} for _, f := range names { features[f] = true - labelName := fakeLabelSourceName + "-" + f + labelName := nfdv1alpha1.FeatureLabelNs + "/" + fakeLabelSourceName + "-" + f if strings.IndexByte(f, '/') >= 0 { labelName = f } @@ -352,9 +353,9 @@ func TestCreateFeatureLabels(t *testing.T) { Convey("Proper fake labels are returned", func() { So(len(labels), ShouldEqual, 3) - So(labels, ShouldContainKey, "fake-fakefeature1") - So(labels, ShouldContainKey, "fake-fakefeature2") - So(labels, ShouldContainKey, "fake-fakefeature3") + So(labels, ShouldContainKey, nfdv1alpha1.FeatureLabelNs+"/"+"fake-fakefeature1") + So(labels, ShouldContainKey, nfdv1alpha1.FeatureLabelNs+"/"+"fake-fakefeature2") + So(labels, ShouldContainKey, nfdv1alpha1.FeatureLabelNs+"/"+"fake-fakefeature3") }) }) Convey("When fake feature source is configured with a whitelist that doesn't match", func() { diff --git a/pkg/nfd-worker/nfd-worker.go b/pkg/nfd-worker/nfd-worker.go index 5cf47167c..9f7e4bca5 100644 --- a/pkg/nfd-worker/nfd-worker.go +++ b/pkg/nfd-worker/nfd-worker.go @@ -580,34 +580,29 @@ func getFeatureLabels(source source.LabelSource, labelWhiteList regexp.Regexp) ( return nil, err } - // Prefix for labels in the default namespace - prefix := source.Name() + "-" - switch source.Name() { - case "local", "custom": - // Do not prefix labels from the custom rules, hooks or feature files - prefix = "" - } - for k, v := range features { - // 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 + name := k + switch sourceName := source.Name(); sourceName { + case "local", "custom": + // No mangling of labels from the custom rules, hooks or feature files + default: + // Prefix for labels from other sources + if !strings.Contains(name, "/") { + name = nfdv1alpha1.FeatureLabelNs + "/" + sourceName + "-" + name + } + } + // Split label name into namespace and name compoents + split := strings.SplitN(name, "/", 2) + nameForWhiteListing := name if len(split) == 2 { - label = k - nameForValidation = label nameForWhiteListing = split[1] } // Validate label name. - errs := validation.IsQualifiedName(nameForValidation) + errs := validation.IsQualifiedName(name) if len(errs) > 0 { - klog.InfoS("ignoring label with invalid name", "lableKey", label, "errors", errs) + klog.InfoS("ignoring label with invalid name", "lableKey", name, "errors", errs) continue } @@ -615,7 +610,7 @@ func getFeatureLabels(source source.LabelSource, labelWhiteList regexp.Regexp) ( // Validate label value errs = validation.IsValidLabelValue(value) if len(errs) > 0 { - klog.InfoS("ignoring label with invalide value", "labelKey", label, "labelValue", value, "errors", errs) + klog.InfoS("ignoring label with invalide value", "labelKey", name, "labelValue", value, "errors", errs) continue } @@ -625,7 +620,7 @@ func getFeatureLabels(source source.LabelSource, labelWhiteList regexp.Regexp) ( continue } - labels[label] = value + labels[name] = value } return labels, nil }