feat/nfd-master: configure CR restrictions #1592

AhmedThresh · 2024-02-16T14:44:53Z

Resolves #1380
Features implemented:

NodeFeatures Namespace selection by using selectors.
Max number of Taints, Labels, and ERs that can be generated by a single CR.
Enable/Disable overriding labels.
Enable/Disable NodeFeatures labels.

Result of e2e tests:

k8s-ci-robot · 2024-02-16T14:45:02Z

Welcome @AhmedThresh!

It looks like this is your first PR to kubernetes-sigs/node-feature-discovery 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/node-feature-discovery has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-02-16T14:45:03Z

Hi @AhmedThresh. 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 · 2024-02-16T14:45:08Z

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	`28b40c9`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/66e83b574e309400089f2011
😎 Deploy Preview	https://deploy-preview-1592--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.

AhmedThresh · 2024-02-16T14:46:26Z

/cc @marquiz

AhmedThresh · 2024-02-27T09:40:53Z

ping @marquiz

marquiz

Thanks for the update @AhmedThresh

/ok-to-test

pkg/nfd-master/nfd-master.go

linux-foundation-easycla · 2024-02-28T20:22:31Z

The committers listed above are authorized under a signed CLA.

✅ login: TessaIO (7bad0d5, 28b40c9, 925a071)

TessaIO · 2024-03-04T14:10:49Z

ping @marquiz. Any more thoughts on this?

ArangoGutierrez · 2024-03-11T10:05:44Z

/assign @marquiz

pkg/nfd-master/nfd-master.go

marquiz · 2024-09-12T07:34:13Z

pkg/nfd-master/nfd-master.go

-			features.Labels = addNsToMapKeys(features.Labels, nfdv1alpha1.FeatureLabelNs)
+		features := filteredObjs[0].Spec.DeepCopy()
+
+		if m.config.Restrictions.DenyNodeFeatureLabels && !m.isThirdPartyNodeFeature(*filteredObjs[0], nodeName, m.namespace) {


This still only affects the 'DisableAutoPrefix` feature gate, not whether the NF labels are skipped or not

marquiz · 2024-09-12T07:48:36Z

pkg/nfd-master/nfd-master.go

+				if m.config.Restrictions.DenyNodeFeatureLabels && !m.isThirdPartyNodeFeature(*filteredObjs[0], nodeName, m.namespace) {
+					klog.V(2).InfoS("node feature labels are disabled in configuration (restrictions.denyNodeFeatureLabels=true)")
+				} else {
+					s.Labels = addNsToMapKeys(s.Labels, nfdv1alpha1.FeatureLabelNs)
+				}


Looking at this, I think the whole function could be refactored. The outer loop could be rewritten (and we shouldn't need to if len(filteredObjs) > 0). For example, something like (haven't tested):

features := &nfdv1alpha1.NodeFeatureSpec{} for _, o := range filteredObjs { s := o.Spec.DeepCopy() if m.config.Restrictions.DenyNodeFeatureLabels && m.isThirdPartyNodeFeature(o, nodeName, m.namespace) { klog.V(2).InfoS("node feature labels are disabled in configuration (restrictions.denyNodeFeatureLabels=true)") s.Labels = nil } if !nfdfeatures.NFDFeatureGate.Enabled(nfdfeatures.DisableAutoPrefix) && m.config.AutoDefaultNs { s.Labels = addNsToMapKeys(s.Labels, nfdv1alpha1.FeatureLabelNs) } s.MergeInto(features) } return &nfdv1alpha1.NodeFeature{ ObjectMeta: metav1.ObjectMeta{ Name: nodeName, }, Spec: *features }, nil

EDIT: code fixed. We don't need this heavy refactoring but could be done (to keep it simpler). WDYT?

EDIT2: maybe we can keep the original optimization features := filteredObjs[0].Spec.DeepCopy()

marquiz · 2024-09-13T11:23:09Z

test/e2e/node_feature_discovery_test.go

+				By("Verifying node labels from NodeFeature object #6 are not created")
+				expectedLabels := map[string]k8sLabels{
+					"*": {
+						"e2e.feature.node.kubernetes.io/restricted-label-1": "true",


There has been so much devilish details in this feature that it would be good to verify that the non-3rd-party NF labels ARE still created. Either by deploying nfd-worker or faking a NF.

You could also just add a note and we could add it in a separate PR. WDYT?

marquiz · 2024-09-13T11:23:23Z

test/e2e/node_feature_discovery_test.go

+				targetNodeName := nodes[0].Name
+				Expect(targetNodeName).ToNot(BeEmpty(), "No suitable worker node found")


How about this?

marquiz · 2024-09-13T11:28:03Z

pkg/nfd-master/nfd-master.go

+		} else {
+			if !nfdfeatures.NFDFeatureGate.Enabled(nfdfeatures.DisableAutoPrefix) && m.config.AutoDefaultNs {
+				features.Labels = addNsToMapKeys(features.Labels, nfdv1alpha1.FeatureLabelNs)
+			}


Note: the else is not necessary but

Suggested change

} else {

if !nfdfeatures.NFDFeatureGate.Enabled(nfdfeatures.DisableAutoPrefix) && m.config.AutoDefaultNs {

features.Labels = addNsToMapKeys(features.Labels, nfdv1alpha1.FeatureLabelNs)

}

if !nfdfeatures.NFDFeatureGate.Enabled(nfdfeatures.DisableAutoPrefix) && m.config.AutoDefaultNs {

features.Labels = addNsToMapKeys(features.Labels, nfdv1alpha1.FeatureLabelNs)

}

marquiz · 2024-09-13T11:30:46Z

pkg/nfd-master/nfd-master.go

+			if m.config.Restrictions.DenyNodeFeatureLabels && m.isThirdPartyNodeFeature(*o, nodeName, m.namespace) {
+				klog.V(2).InfoS("node feature labels are disabled in configuration (restrictions.denyNodeFeatureLabels=true)")
+				s.Labels = nil
+				continue


We shouldn't skip the whole NF, just set labels to nil.

Suggested change

continue

Why didn't the e2e tests catch this? 🤔 We should have a NFR there to verify that the features are still present. You can also just add a note about this there for now.

TessaIO · 2024-09-16T10:09:25Z

ping @marquiz

marquiz · 2024-09-16T10:59:35Z

docs/reference/master-configuration-reference.md

+
+## restrictions
+
+The following options specify the restrictions that can be applied by the


One last comment. Should we call this EXPERIMENTAL in this release(?) There have been so many subtle details during the review process that we've certainly missed something 😊 WDYT?

Yes, please. I will create another issue to cover those unseen details. If you have some bullet points in mind you can leave them here.

Signed-off-by: AhmedGrati <ahmedgrati1999@gmail.com> Signed-off-by: AhmedThresh <ahmed.grati@insat.ucar.tn> Signed-off-by: AhmedGrati <ahmedgrati1999@gmail.com> Signed-off-by: AhmedThresh <ahmed.grati@insat.ucar.tn> Signed-off-by: AhmedGrati <ahmedgrati1999@gmail.com> Signed-off-by: AhmedThresh <ahmed.grati@insat.ucar.tn>

TessaIO · 2024-09-16T14:06:30Z

@marquiz PTAL

marquiz

Thanks @TessaIO for this monumental effort with a lot of twists, turns and surprises 💯 🚀 I'm ready to merge this now.

We have some TODO items to improve the e2e-tests and will address those later

I'll let @ArangoGutierrez to take a final look. E.g. check the docs if you understand what we're trying to do here 😜
/assign @ArangoGutierrez

TessaIO · 2024-09-17T08:37:34Z

Thanks @TessaIO for this monumental effort with a lot of twists, turns and surprises 💯 🚀 I'm ready to merge this now.

Thanks for your thorough and precise review! 🚀

TessaIO · 2024-09-20T15:29:24Z

ping @ArangoGutierrez

TessaIO · 2024-10-10T12:39:27Z

kind reminder @ArangoGutierrez @marquiz

ArangoGutierrez · 2024-10-24T11:04:31Z

/test all

ArangoGutierrez

/lgtm

k8s-ci-robot · 2024-10-24T11:19:50Z

LGTM label has been added.

Git tree hash: 491d4d21569336c2bdca3d723df02e1d1af61431

k8s-ci-robot · 2024-10-24T11:19:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AhmedThresh, 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

marquiz · 2024-10-24T17:04:33Z

Ach, finally 🚀 ☺️

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 16, 2024

k8s-ci-robot requested review from jjacobelli and zvonkok February 16, 2024 14:44

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 16, 2024

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 16, 2024

k8s-ci-robot requested a review from marquiz February 16, 2024 14:46

marquiz reviewed Feb 28, 2024

View reviewed changes

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

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

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 28, 2024

AhmedThresh force-pushed the feat-configure-cr-restrictions branch from 46daaca to 6f271b0 Compare February 28, 2024 20:22

AhmedThresh force-pushed the feat-configure-cr-restrictions branch 4 times, most recently from 8d7c72c to 9e4aa7e Compare February 29, 2024 10:44

TessaIO mentioned this pull request Mar 4, 2024

Feat integrate spiffe #1434

Draft

ArangoGutierrez requested a review from marquiz March 11, 2024 10:04

k8s-ci-robot assigned marquiz Mar 11, 2024

ArangoGutierrez requested changes Mar 11, 2024

View reviewed changes

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

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

marquiz reviewed Sep 12, 2024

View reviewed changes

AhmedThresh force-pushed the feat-configure-cr-restrictions branch 9 times, most recently from ff4b343 to 7a8ff18 Compare September 13, 2024 06:26

marquiz reviewed Sep 13, 2024

View reviewed changes

AhmedThresh force-pushed the feat-configure-cr-restrictions branch from 7a8ff18 to 75961e2 Compare September 13, 2024 18:12

marquiz reviewed Sep 16, 2024

View reviewed changes

AhmedThresh force-pushed the feat-configure-cr-restrictions branch from 75961e2 to 28b40c9 Compare September 16, 2024 14:06

marquiz approved these changes Sep 16, 2024

View reviewed changes

k8s-ci-robot assigned ArangoGutierrez Sep 16, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 16, 2024

ArangoGutierrez approved these changes Oct 24, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 24, 2024

k8s-ci-robot merged commit fd2893e into kubernetes-sigs:master Oct 24, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat/nfd-master: configure CR restrictions #1592

feat/nfd-master: configure CR restrictions #1592

AhmedThresh commented Feb 16, 2024 •

edited

Loading

k8s-ci-robot commented Feb 16, 2024

k8s-ci-robot commented Feb 16, 2024

netlify bot commented Feb 16, 2024 •

edited

Loading

AhmedThresh commented Feb 16, 2024

AhmedThresh commented Feb 27, 2024

marquiz left a comment

linux-foundation-easycla bot commented Feb 28, 2024 •

edited

Loading

TessaIO commented Mar 4, 2024

ArangoGutierrez commented Mar 11, 2024

marquiz Sep 12, 2024 •

edited

Loading

marquiz Sep 12, 2024 •

edited

Loading

marquiz Sep 13, 2024

marquiz Sep 13, 2024

marquiz Sep 13, 2024

marquiz Sep 13, 2024

TessaIO commented Sep 16, 2024

marquiz Sep 16, 2024

TessaIO Sep 16, 2024

TessaIO commented Sep 16, 2024

marquiz left a comment

TessaIO commented Sep 17, 2024

TessaIO commented Sep 20, 2024

TessaIO commented Oct 10, 2024

ArangoGutierrez commented Oct 24, 2024

ArangoGutierrez left a comment

k8s-ci-robot commented Oct 24, 2024

k8s-ci-robot commented Oct 24, 2024

marquiz commented Oct 24, 2024

		targetNodeName := nodes[0].Name
		Expect(targetNodeName).ToNot(BeEmpty(), "No suitable worker node found")


		## restrictions

		The following options specify the restrictions that can be applied by the

feat/nfd-master: configure CR restrictions #1592

feat/nfd-master: configure CR restrictions #1592

Conversation

AhmedThresh commented Feb 16, 2024 • edited Loading

k8s-ci-robot commented Feb 16, 2024

k8s-ci-robot commented Feb 16, 2024

netlify bot commented Feb 16, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-nfd ready!

AhmedThresh commented Feb 16, 2024

AhmedThresh commented Feb 27, 2024

marquiz left a comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Feb 28, 2024 • edited Loading

TessaIO commented Mar 4, 2024

ArangoGutierrez commented Mar 11, 2024

marquiz Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

marquiz Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

marquiz Sep 13, 2024

Choose a reason for hiding this comment

marquiz Sep 13, 2024

Choose a reason for hiding this comment

marquiz Sep 13, 2024

Choose a reason for hiding this comment

marquiz Sep 13, 2024

Choose a reason for hiding this comment

TessaIO commented Sep 16, 2024

marquiz Sep 16, 2024

Choose a reason for hiding this comment

TessaIO Sep 16, 2024

Choose a reason for hiding this comment

TessaIO commented Sep 16, 2024

marquiz left a comment

Choose a reason for hiding this comment

TessaIO commented Sep 17, 2024

TessaIO commented Sep 20, 2024

TessaIO commented Oct 10, 2024

ArangoGutierrez commented Oct 24, 2024

ArangoGutierrez left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 24, 2024

k8s-ci-robot commented Oct 24, 2024

marquiz commented Oct 24, 2024

AhmedThresh commented Feb 16, 2024 •

edited

Loading

netlify bot commented Feb 16, 2024 •

edited

Loading

linux-foundation-easycla bot commented Feb 28, 2024 •

edited

Loading

marquiz Sep 12, 2024 •

edited

Loading

marquiz Sep 12, 2024 •

edited

Loading