Option to stop implicitly adding default prefix to names

[marquiz]

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.

[netlify]

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	1d012a2
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/65607f8004e0a70008373e36
😎 Deploy Preview	https://deploy-preview-1461--kubernetes-sigs-nfd.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[marquiz]

Request for comments/thoughts

/hold

[ArangoGutierrez]

We discussed this online, I am in (lgtm)
just one nit

[marquiz]

/retest

[ArangoGutierrez]

@zvonkok @yevgeny-shnaidman @PiotrProkop Any comments?

[PiotrProkop]

LGTM will leave final approval to @ArangoGutierrez

[yevgeny-shnaidman]

@marquiz does autoDefaultNs option means that it also supports the previous Rules/hooks configuration? If not that this commit is very problematic, since all of our customers will certainly complain regarding backwards compatibility

[marquiz]

@marquiz does autoDefaultNs option means that it also supports the previous Rules/hooks configuration? If not that this commit is very problematic, since all of our customers will certainly complain regarding backwards compatibility

Thank you for the feedback @yevgeny-shnaidman. Yes, this config option is effectively a "feature gate", reverting to the old behavior.

One possibility would be to actually have this change as "alpha" default-disabled (i.e. autoDefaultNs=true) in v0.15 and turn it on by default in a future release. And now looking at this it might be the right thing to do.

What I was/am unsure if almost anybody really rely on the default namespacing of features i.e. have non-namespaced labels in their hook/feature files or rule configuration. Anything I've seen out there is always something like vendor.com/feature-x=foo (and not feature-x=foo which will turn to feature.node.kubernetes.io/feature-x=foo). One extra motivation of this change would be to eventually allow creating non-namespaced labels, too.

[ArangoGutierrez]

@marquiz does autoDefaultNs option means that it also supports the previous Rules/hooks configuration? If not that this commit is very problematic, since all of our customers will certainly complain regarding backwards compatibility

Thank you for the feedback @yevgeny-shnaidman. Yes, this config option is effectively a "feature gate", reverting to the old behavior.

One possibility would be to actually have this change as "alpha" default-disabled (i.e. autoDefaultNs=true) in v0.15 and turn it on by default in a future release. And now looking at this it might be the right thing to do.

What I was/am unsure if almost anybody really rely on the default namespacing of features i.e. have non-namespaced labels in their hook/feature files or rule configuration. Anything I've seen out there is always something like vendor.com/feature-x=foo (and not feature-x=foo which will turn to feature.node.kubernetes.io/feature-x=foo). One extra motivation of this change would be to eventually allow creating non-namespaced labels, too.

unhold?

[marquiz]

unhold?

I'll probably change this feature to default-off so it'll be a non-breaking change in v0.15. Then warn about the users about the upcoming default-on change in a future release.

[marquiz]

/retitle Option to stop implicitly adding default prefix to names

[marquiz]

Changed the autoDefaultNs default to true, i.e. not changing the existing behavior. The idea is that the default will be changed to false in a future release. PR title and description updated accordingly.

[ArangoGutierrez]

Changed the autoDefaultNs default to true, i.e. not changing the existing behavior. The idea is that the default will be changed to false in a future release. PR title and description updated accordingly.

unhold?

[marquiz]

unhold?

Not yet. As I kinda feared there are hidden subtle bugs in NFD that I found out while working on this 😬 I'll submit fixes soon. Meanwhile check #1470 as that's pre-cleanup before landing those fixes....

[marquiz]

Updated. Depends on #1470 and #1471

[marquiz]

Unoholding this as it's now effectively an "alpha" feature, turned off by default

[marquiz]

/unhold

[ArangoGutierrez]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: b2577da6dc897dff4dfd702058cf9e3a9dbec3b0

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ArangoGutierrez, marquiz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [ArangoGutierrez,marquiz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[codecov]

Codecov Report

Merging #1461 (1d012a2) into master (4e7f8b1) will increase coverage by 0.39%.
Report is 2 commits behind head on master.
The diff coverage is 33.33%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1461      +/-   ##
==========================================
+ Coverage   30.51%   30.91%   +0.39%     
==========================================
  Files          58       58              
  Lines        7530     7653     +123     
==========================================
+ Hits         2298     2366      +68     
- Misses       4982     5028      +46     
- Partials      250      259       +9     

Files	Coverage Δ
pkg/nfd-worker/nfd-worker.go	57.78% <85.71%> (-0.03%)	⬇️
pkg/nfd-master/nfd-master.go	43.76% <9.67%> (-2.04%)	⬇️

... and 3 files with indirect coverage changes