feat: add deny-label-ns flag which supports wildcard #1051

AhmedGrati · 2023-02-06T18:49:58Z

Resolves #1022.

Signed-off-by: AhmedGrati [email protected]

k8s-ci-robot · 2023-02-06T18:50:07Z

Hi @AhmedGrati. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

netlify · 2023-02-06T18:50:09Z

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	`b499799`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/63ec9c0d3d0ff0000869fe07
😎 Deploy Preview	https://deploy-preview-1051--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 settings.

marquiz

Thanks @AhmedGrati for working on this!

I don't think we want to replace -extra-label-ns, just add -deny-label-ns on top of that. This will allow strict namespace control (if a user wants that) e.g. with

-deny-label-ns="*" -extra-label-ns=example.com

you could deny everything but example.com, kind of mimicking what we currently do

/ok-to-test

marquiz · 2023-02-08T19:11:35Z

pkg/nfd-master/nfd-master.go

@@ -449,6 +451,17 @@ func verifyNodeName(cert *x509.Certificate, nodeName string) error {
 	return nil
 }

+func isNamespaceDenied(labelNs string, deniedNs map[string]struct{}) bool {
+	for ns := range deniedNs {
+		deniedNsWildcard := fmt.Sprintf("*.%v", ns)


I don't think we want to forcibly make all items a glob. Otherwise you couldn't do something like "deny kubernetes.io but allow all *.kubernetes.io"

Yes true, but I just followed what was said here. Which means basically denying all namespaces. In fact, the user will input a namespace (without wildcard) and we will deny all sub-namespaces by default.
However, If you see that we need to give users the flexibility to deny all sub-namespaces using wildcards, I think we should change this implementation above.

It can be that what I suggested above is not the "right thing to do". NOT forcing a glob (i.e. not automatically denying sub-namespaces) would give us the most flexibility. I now feel that if we forcibly deny all sub-namespaces it will bit us later (could be countered by supporting wildcards in -extra-label-ns though)

Let's see what others think. Any thoughts on this @cedricmckinnie @ArangoGutierrez @zvonkok @fmuyassarov ?

Yes, I agree with you!

marquiz · 2023-02-08T19:23:08Z

pkg/nfd-master/nfd-master.go

+		g := glob.MustCompile(deniedNsWildcard)
+		if g.Match(labelNs) {


I'm not entirely sure we need to support full-fledged globs (performance, extra external dependency), we could probably do away with something like

if strings.HasPrefix(ns, "*") { deniedSuffix := strings.TrimLeft(ns, "*") if strings.HasSuffix(labelNs, deniedSuffix) { return true } } else if labelNs == ns { return true } return false

And just document that we support asterisk ("*") in front of the name to deny all sub-namespaces. WDYT?

Also, we probably want to pre-process the user input at startup to repeat same string (or glob) operations over and over again.

Yes got it, and it's a valid point I think. I just didn't understand the last point. Can you explain it to me further, please?

Can you explain it to me further, please?

My point is that in the current implementation for each label we create, we run through the deny list and do glob.MustCompile for each deny item. That is for each time a label request arrives, for each label, for each item in deny list do compile. Even though that information is basically static, doing compile once at the startup would be enough and save a lot of cpu cycles

Same could basically be applied to the simplified poor-man's asterisk-based I proposed. I.e. pre-process the deny list to two separate lists, one to do exact matches (==) and one to do suffixes (HasSuffix())

@marquiz what about having working with such wildcards: prefix.*.suffix. The aesterik-based implementation won't work in the expected way. isn't it?

It's just me, but I wouldn't be concerned about complex regexs or globs at the moment. Think about how domain names and IP addresses generally work, for example, you don't generally specify IP ranges like 192.168.*.3. For sub-namespaces I see a clear use case, though. More advanced globbing can be added later if it is clearly needed

ArangoGutierrez · 2023-02-10T16:44:48Z

cmd/nfd-master/main.go

 	}

 	flagset.StringVar(&args.CaFile, "ca-file", "",
 		"Root certificate for verifying connections")
 	flagset.StringVar(&args.CertFile, "cert-file", "",
 		"Certificate used for authenticating connections")
-	flagset.Var(&args.ExtraLabelNs, "extra-label-ns",


@marquiz should we have both flags at least for a release, having ExtraLabelNS to print a deprecation warning? I feel that removing the flag all together could break some users deployments if they try a roll-update and the pod get's called with old flags.
WDYT?

@marquiz suggested that we keep the extra-label-ns flag because that would give flexibility to our users.

@ArangoGutierrez yes, I don't see any reason removing the existing flag. It will also give flexibility in enabling scenarios like "deny all except for example.com/ and acme.io/"

ArangoGutierrez

PR looks good now
Leaving the lgtm to @marquiz

marquiz

Thanks for the update @AhmedGrati

I think we need to adjust the docs a bit more, probably removing all references to -extra-label-ns in docs/usage/customization-guide.md and also adjust description of -extra-label-ns in docs/reference/master-commandline-reference.md to match the new behavior.

cmd/nfd-master/main.go

pkg/nfd-master/nfd-master.go

AhmedGrati · 2023-02-13T12:39:54Z

@marquiz why should we remove all references to -extra-label-ns in docs/usage/customization-guide.md?

marquiz · 2023-02-13T14:10:31Z

@marquiz why should we remove all references to -extra-label-ns in docs/usage/customization-guide.md?

Heh, my comment wasn't very clear 😇 I should've written that review all references to -extra-label-ns in the doc. IIRC it's mentioned in few places, saying that by default all namespaces except for '*.feature.node.kubernetes.io` are denied... which obviously isn't true anymore after this PR so we could just remove those mentions

AhmedGrati · 2023-02-13T14:23:20Z

@marquiz why should we remove all references to -extra-label-ns in docs/usage/customization-guide.md?

Heh, my comment wasn't very clear 😇 I should've written that review all references to -extra-label-ns in the doc. IIRC it's mentioned in few places, saying that by default all namespaces except for '*.feature.node.kubernetes.io` are denied... which obviously isn't true anymore after this PR so we could just remove those mentions

Oh yes got it now, thank you 😊

AhmedGrati · 2023-02-13T16:49:59Z

@marquiz I added the fact that we'll forcibly deny the *.kubernetes.io namespace.

marquiz · 2023-02-14T12:19:03Z

but setting directly the mockMaster.deniedNs, won't cover the test of the preProcessing part of the DeniedLabelNs. For the moment, I think it should be done that way.

That is very much true. D'ya have plans for adding e2e-tests? 😉

AhmedGrati · 2023-02-14T13:05:33Z

@marquiz I'll work on adding the e2e tests, but it'll take me some time.

marquiz · 2023-02-14T13:14:00Z

@marquiz I'll work on adding the e2e tests, but it'll take me some time.

No worries, that would be really cool. I think we can take that as a separate PR

AhmedGrati · 2023-02-14T13:16:17Z

Okay then, I'll finish the requested changes and I'll create a separate issue and PR for the e2e tests.

pkg/nfd-master/nfd-master-internal_test.go

marquiz

Thanks @AhmedGrati for the update. Looks like we're almost there, just a few small comments/suggestions

EDIT: also I think you want to drop this from the customization guide:

Note that in the example above `-extra-label-ns=my.namespace` must be specified
on the nfd-master command line.

marquiz · 2023-02-15T07:28:15Z

pkg/nfd-master/nfd-master.go

+	nfd.deniedNs.wildcard = wildcardDeniedNs
+	// We forcibly deny kubernetes.io
+	nfd.deniedNs.normal["kubernetes.io"] = struct{}{}
+	nfd.deniedNs.wildcard["kubernetes.io"] = struct{}{}


To be correct i think we want to deny *.kubernetes.io

Suggested change

nfd.deniedNs.wildcard["kubernetes.io"] = struct{}{}

nfd.deniedNs.wildcard[".kubernetes.io"] = struct{}{}

docs/reference/master-commandline-reference.md

pkg/nfd-master/nfd-master.go

Signed-off-by: AhmedGrati <[email protected]>

marquiz

Thanks @AhmedGrati for your efforts on this! From my perspective this looks good to go 👍

marquiz · 2023-02-15T09:51:30Z

pkg/nfd-master/nfd-master.go

@@ -392,7 +409,7 @@ func (m *nfdMaster) updateMasterNode() error {
 // into extended resources. This function also handles proper namespacing of
 // labels and ERs, i.e. adds the possibly missing default namespace for labels
 // arriving through the gRPC API.
-func filterFeatureLabels(labels Labels, extraLabelNs map[string]struct{}, labelWhiteList regexp.Regexp, extendedResourceNames map[string]struct{}) (Labels, ExtendedResources) {
+func (m *nfdMaster) filterFeatureLabels(labels Labels) (Labels, ExtendedResources) {


Thanks, this looks so much cleaner 👍

marquiz · 2023-02-15T09:55:13Z

/assign @ArangoGutierrez @fmuyassarov

AhmedGrati · 2023-02-15T10:05:36Z

Many thanks for your review @marquiz 👌.

ArangoGutierrez

/lgtm

k8s-ci-robot · 2023-02-15T11:25:27Z

LGTM label has been added.

Git tree hash: 1fea083de80ed12d154588ced604dfdc9a91d7be

k8s-ci-robot · 2023-02-15T11:25:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AhmedGrati, 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~~ [marquiz]

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

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 6, 2023

k8s-ci-robot requested review from adrianchiris and fmuyassarov February 6, 2023 18:50

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 6, 2023

marquiz reviewed Feb 8, 2023

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 8, 2023

ArangoGutierrez requested changes Feb 10, 2023

View reviewed changes

AhmedGrati force-pushed the feat-add-deny-label-ns-with-wildcard branch from c9695a1 to 2b927c8 Compare February 11, 2023 11:04

AhmedGrati changed the title ~~feat: replace extra-label-ns with deny-label-ns flag which supports wildcard~~ feat: add deny-label-ns flag which supports wildcard Feb 11, 2023

AhmedGrati force-pushed the feat-add-deny-label-ns-with-wildcard branch 2 times, most recently from e1eff45 to 5182050 Compare February 11, 2023 11:11

AhmedGrati requested review from marquiz and removed request for adrianchiris and fmuyassarov February 12, 2023 10:11

ArangoGutierrez approved these changes Feb 13, 2023

View reviewed changes

marquiz reviewed Feb 13, 2023

View reviewed changes

cmd/nfd-master/main.go Outdated Show resolved Hide resolved

pkg/nfd-master/nfd-master.go Outdated Show resolved Hide resolved

AhmedGrati force-pushed the feat-add-deny-label-ns-with-wildcard branch from 5182050 to 1b46830 Compare February 13, 2023 16:49

AhmedGrati requested review from marquiz and ArangoGutierrez and removed request for marquiz February 13, 2023 16:49

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 13, 2023

marquiz reviewed Feb 14, 2023

View reviewed changes

pkg/nfd-master/nfd-master-internal_test.go Outdated Show resolved Hide resolved

AhmedGrati force-pushed the feat-add-deny-label-ns-with-wildcard branch 3 times, most recently from 9f9badd to 1d5fead Compare February 14, 2023 16:12

AhmedGrati requested review from marquiz and ArangoGutierrez and removed request for marquiz and ArangoGutierrez February 14, 2023 16:12

marquiz reviewed Feb 15, 2023

View reviewed changes

feat: add deny-label-ns flag which supports wildcard

b499799

Signed-off-by: AhmedGrati <[email protected]>

AhmedGrati force-pushed the feat-add-deny-label-ns-with-wildcard branch from 1d5fead to b499799 Compare February 15, 2023 08:47

AhmedGrati requested a review from marquiz February 15, 2023 09:21

marquiz approved these changes Feb 15, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 15, 2023

k8s-ci-robot assigned ArangoGutierrez and fmuyassarov Feb 15, 2023

ArangoGutierrez approved these changes Feb 15, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 15, 2023

k8s-ci-robot merged commit a92614c into kubernetes-sigs:master Feb 15, 2023

AhmedGrati mentioned this pull request Apr 11, 2023

REQUEST: New membership for AhmedGrati kubernetes/org#4157

Closed

9 tasks

marquiz mentioned this pull request Apr 12, 2023

Release v0.13.0 #1068

Closed

24 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add deny-label-ns flag which supports wildcard #1051

feat: add deny-label-ns flag which supports wildcard #1051

AhmedGrati commented Feb 6, 2023

k8s-ci-robot commented Feb 6, 2023

netlify bot commented Feb 6, 2023 •

edited

Loading

marquiz left a comment

marquiz Feb 8, 2023

AhmedGrati Feb 8, 2023 •

edited

Loading

marquiz Feb 9, 2023

AhmedGrati Feb 9, 2023

marquiz Feb 8, 2023

AhmedGrati Feb 8, 2023

marquiz Feb 9, 2023 •

edited

Loading

AhmedGrati Feb 10, 2023

marquiz Feb 10, 2023

ArangoGutierrez Feb 10, 2023

AhmedGrati Feb 10, 2023

marquiz Feb 10, 2023

ArangoGutierrez left a comment

marquiz left a comment

AhmedGrati commented Feb 13, 2023

marquiz commented Feb 13, 2023

AhmedGrati commented Feb 13, 2023

AhmedGrati commented Feb 13, 2023

marquiz commented Feb 14, 2023

AhmedGrati commented Feb 14, 2023

marquiz commented Feb 14, 2023

AhmedGrati commented Feb 14, 2023

marquiz left a comment •

edited

Loading

marquiz Feb 15, 2023

marquiz left a comment

marquiz Feb 15, 2023

marquiz commented Feb 15, 2023

AhmedGrati commented Feb 15, 2023

ArangoGutierrez left a comment

k8s-ci-robot commented Feb 15, 2023

k8s-ci-robot commented Feb 15, 2023

		g := glob.MustCompile(deniedNsWildcard)
		if g.Match(labelNs) {

	nfd.deniedNs.wildcard["kubernetes.io"] = struct{}{}
	nfd.deniedNs.wildcard[".kubernetes.io"] = struct{}{}

feat: add deny-label-ns flag which supports wildcard #1051

feat: add deny-label-ns flag which supports wildcard #1051

Conversation

AhmedGrati commented Feb 6, 2023

k8s-ci-robot commented Feb 6, 2023

netlify bot commented Feb 6, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-nfd ready!

marquiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AhmedGrati Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marquiz Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArangoGutierrez left a comment

Choose a reason for hiding this comment

marquiz left a comment

Choose a reason for hiding this comment

AhmedGrati commented Feb 13, 2023

marquiz commented Feb 13, 2023

AhmedGrati commented Feb 13, 2023

AhmedGrati commented Feb 13, 2023

marquiz commented Feb 14, 2023

AhmedGrati commented Feb 14, 2023

marquiz commented Feb 14, 2023

AhmedGrati commented Feb 14, 2023

marquiz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marquiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marquiz commented Feb 15, 2023

AhmedGrati commented Feb 15, 2023

ArangoGutierrez left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 15, 2023

k8s-ci-robot commented Feb 15, 2023

netlify bot commented Feb 6, 2023 •

edited

Loading

AhmedGrati Feb 8, 2023 •

edited

Loading

marquiz Feb 9, 2023 •

edited

Loading

marquiz left a comment •

edited

Loading