Allow wildcard or regrx in "extra label ns"

[cedricmckinnie]

What would you like to be added:
The ability to pass a wildcard or regex etc. to the label namespace whitelist i.e.

# Command line
--extra-label-ns='*'

# and

# In helm chart
extraLabelNs: ["*"]

Another possibility could be to disable whitelist altogether because my use case doesn't need to prevent any label namespaces.

Why is this needed:

• I'm looking to retrofit a huge list of custom labels for a lot of different nodes to be managed by NFD.
• The whitelist makes it rather tedious/error-prone to add every single namespace to the whitelist on the operator when I don't need to restrict any to begin with.
• When creating a new label to the NodeFeatureRule that have a new label namespace, I don't want to have to remember to go and add it to the whitelist in the helm chart as well.
• Having this will also allow me to avoid having to update all of the workloads across a ton of systems to use the NFD default label namespaces which would be a large, error-prone undertaking.

Thanks so much in advance for your consideration! Happy to help if needed!

[cedricmckinnie]

node-feature-discovery/pkg/nfd-master/nfd-master.go

Lines 403 to 405 in d97297e

	if _, ok := extraLabelNs[ns]; !ok {
	klog.Errorf("Namespace %q is not allowed. Ignoring label %q\n", ns, label)
	continue

Seems like it all comes down to updating the condition above. If you're worried about maintaining high performance of this lookup, I think a TreeMap could work since it can use a StringComparator in its key lookup.

https://pkg.go.dev/github.com/emirpasic/gods/maps/treemap#NewWithStringComparator

[marquiz]

I think this makes sense. Especially in the context you mentioned i.e. replacing some existing labeling scenarios with NFD. We've prolly been too restrictive on the labels namespaces – originally we wanted to prevent NFD to replace or "steal" existing labels, especially in kubernetes.io/*. As you say, there are multiple ways to solve this, at least:

1. support regexps in -extra-label-ns
2. special value e.g. * to allow all (i.e. disable whitelisting alltogether)
3. replace -extra-label-ns with a "deny list" (-deny-label-ns defaulting to kubernetes.io/or smth) instead

Any thoughts/comments @zvonkok @ArangoGutierrez @fmuyassarov ?

[cedricmckinnie]

@marquiz Thanks for the prompt response. This is great new. Looking forward to seeing how you all plan to implement it! Happy to help if needed

[fmuyassarov]

I think this makes sense. Especially in the context you mentioned i.e. replacing some existing labeling scenarios with NFD. We've prolly been too restrictive on the labels namespaces – originally we wanted to prevent NFD to replace or "steal" existing labels, especially in kubernetes.io/*. As you say, there are multiple ways to solve this, at least:

1. support regexps in -extra-label-ns
2. special value e.g. * to allow all (i.e. disable whitelisting alltogether)
3. replace -extra-label-ns with a "deny list" (-deny-label-ns defaulting to kubernetes.io/or smth) instead

Any thoughts/comments @zvonkok @ArangoGutierrez @fmuyassarov ?

I think it makes sense to have this feature improvement. However, I don't have a strong opinion which of those options would be the best to use.

[ArangoGutierrez]

I like @marquiz option 3. Maybe something to add to the next release milestones

[marquiz]

I like @marquiz option 3.

Now after sleeping over it, I'm also inclined to prefer this one.

Maybe something to add to the next release milestones

Yes. Added to v0.13 milestone.

[AhmedGrati]

Hey @marquiz, should the -deny-label-ns support a regex or not?

[marquiz]

Hey @marquiz, should the -deny-label-ns support a regex or not?

I'm open to discussion on the details. At a quick thought I'd say that probably not. But probably deny all "sub-namespaces" of the given names. I.e. specifying kubernetes.io would deny all *.kubernetes.io/... labels

[AhmedGrati]

IMHO, passing directly the regex would give more flexibility to users. In some cases, users would need to deny some "sub-namespaces", but not all of them. wdyt?

[marquiz]

IMHO, passing directly the regex would give more flexibility to users. In some cases, users would need to deny some "sub-namespaces", but not all of them. wdyt?

Hmm, need to think about this 🧐 The reason I sort of don't like regexp is that the format is a bit involved for the usual use cases (i.e. remember to specify $ and/or to ^ like "\.kubernetes.io$") and can have unexpected results for the uneducated user if care is not taken. Using a simple filepath style glob (e.g. supporting * as a wildcard) would be easier to master, I think. But you've got a point that sub-namespaces can cause a problem, irrespective whether we'd automatically deny all sub-namespaces or allow them.

If we'd automatically deny all sub-namespaces then there's no way to allow all of them (or only deny SOME sub-namespaces as you put it). If we automatically allow all sub-namespaces then it becomes impossible to deny ALL subnamespaces. Regexps or simple glob would resolve those use cases. Don't remember if golang has a library glob function - one possibility would to hack in a simple glob support like "if item StartsWith("*") then use HasSuffix() else use ==)

Any thoughts @cedricmckinnie @fmuyassarov @ArangoGutierrez @zvonkok on this?

[ArangoGutierrez]

Agree with you Markus, enabling the input of regex can come back and bite us.
a -deny-label-ns with kubernetes.io defaulted sounds good to me, we should put even extra blocks for people to use the kubernetes.io ns, maybe a separate flag -allow-k8s-ns or something.
But overall, agree with your comment Markus

[cedricmckinnie]

@ArangoGutierrez hit the mark with a deny flag although, I can't really think of any reason why someone would want to control any kubernetes.io labels since they're generally just privately managed by the controllers. Seems like even for advanced users, it might be rather tricky to detect an insidious issue that arises from a "label-control overlap conflict" and be able to trace that problem back to NFD operator having mutated the label as the root cause. Perhaps unconditionally ignoring those until we have a use case or value prop would be safer. Any thoughts?

[marquiz]

Good considerations @cedricmckinnie and @ArangoGutierrez, I think you got a good point there. It probably is a good idea and and safe measure to always forcibly deny *.kubernetes.io, even with -deny-label-ns="". @AhmedGrati could you adjust the PR accordingly?

We can probably keep the current behavior where -extra-label-ns does not have any constraints i.e. allows also enabling k8s.io namespaces if somebody really wants that.

[AhmedGrati]

Yes sure 👍.