Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] feat: configure CR restrictions #1540

Conversation

AhmedGrati
Copy link

@AhmedGrati AhmedGrati commented Jan 15, 2024

POC of #1380
Things TBD:

  • unit and e2e tests
  • Docs

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 15, 2024
Copy link

netlify bot commented Jan 15, 2024

Deploy Preview for kubernetes-sigs-nfd ready!

Name Link
🔨 Latest commit 5f7569f
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/65b8280b96084400082a4e22
😎 Deploy Preview https://deploy-preview-1540--kubernetes-sigs-nfd.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 15, 2024
@AhmedGrati AhmedGrati force-pushed the feat-configure-cr-restrictions branch from 8c3713e to b641da1 Compare January 16, 2024 20:53
@marquiz
Copy link
Contributor

marquiz commented Jan 16, 2024

Thanks @AhmedGrati for working on this. Will look at it later
/assign

@AhmedGrati AhmedGrati marked this pull request as ready for review January 16, 2024 23:09
@AhmedGrati AhmedGrati force-pushed the feat-configure-cr-restrictions branch from b641da1 to 4c979a4 Compare January 16, 2024 23:14
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AhmedGrati
Once this PR has been reviewed and has the lgtm label, please ask for approval from marquiz. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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

@AhmedGrati AhmedGrati force-pushed the feat-configure-cr-restrictions branch 2 times, most recently from 10614db to 106c04a Compare January 24, 2024 10:56
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 27, 2024
@AhmedGrati AhmedGrati force-pushed the feat-configure-cr-restrictions branch from 106c04a to 284a56e Compare January 29, 2024 22:14
Signed-off-by: AhmedGrati <ahmedgrati1999@gmail.com>
@AhmedGrati AhmedGrati force-pushed the feat-configure-cr-restrictions branch from 284a56e to b4ef825 Compare January 29, 2024 22:21
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 29, 2024
Signed-off-by: AhmedGrati <ahmedgrati1999@gmail.com>
@AhmedGrati AhmedGrati force-pushed the feat-configure-cr-restrictions branch from b4ef825 to 5f7569f Compare January 29, 2024 22:34
Copy link

codecov bot commented Jan 29, 2024

Codecov Report

Attention: 26 lines in your changes are missing coverage. Please review.

Comparison is base (5c99ae8) 31.15% compared to head (5f7569f) 31.21%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1540      +/-   ##
==========================================
+ Coverage   31.15%   31.21%   +0.05%     
==========================================
  Files          62       62              
  Lines        7774     7798      +24     
==========================================
+ Hits         2422     2434      +12     
- Misses       5108     5114       +6     
- Partials      244      250       +6     
Files Coverage Δ
pkg/nfd-master/nfd-api-controller.go 12.50% <18.75%> (+2.03%) ⬆️
pkg/nfd-master/nfd-master.go 45.89% <45.83%> (+0.26%) ⬆️

Copy link
Contributor

@marquiz marquiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks you @AhmedGrati for working on this feature. Some comments below

case "deny-node-feature-labels":
args.Overrides.DenyNodeFeatureLabels = overrides.DenyNodeFeatureLabels
case "overwrite-labels":
args.Overrides.OverwriteLabels = overrides.OverwriteLabels
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could have these as config file options only, if you want to keep things simple. E.g. the leader election parameters are config-file-only so this is asymmetric, already. WDYT?

@@ -47,13 +48,15 @@ type nfdController struct {
type nfdApiControllerOptions struct {
DisableNodeFeature bool
ResyncPeriod time.Duration
AllowedNamespaces []string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, looking at this now I think we could do better (more k8s-like). Could we use metav1.LabelSelector directly. It would make it very flexible to pick up the namespaces to watch. The name of the config option would be something like nodeFeatureNamespaceSelector. WDYT?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I got this correctly, users would need to pass a map (instead of an array) to specify selectors, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or even use the metav1.LabelSelector type directly?

@AhmedThresh
Copy link

@marquiz I think we can close this.

@TessaIO
Copy link
Contributor

TessaIO commented Mar 14, 2024

ping @marquiz, can we close this?

@marquiz
Copy link
Contributor

marquiz commented Mar 15, 2024

Superseded by #1592
/close

@k8s-ci-robot
Copy link
Contributor

@marquiz: Closed this PR.

In response to this:

Superseded by #1592
/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants