-
Notifications
You must be signed in to change notification settings - Fork 277
nfd-worker: Add FeatureAllowList, FeatureDenyList #2255
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
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for kubernetes-sigs-nfd ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Hi @ozhuraki. 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 Once the patch is verified, the new status will be reflected by the 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-sigs/prow repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ozhuraki 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ozhuraki for starting to work on this.
I have two main comments:
- The main motivation for this feature is decreasing the size of the NodeFeature object(s). We need filtering for the "raw" features that get into those CRs. For the labels we have adequate controls (I think).
- We need some more expressive mechanism than one-regexp-to-rule-them-all. Like Only show cpu.cpuid.AVX512* and cpu.cpuid.AMX but nothing else or Filter out all PCI devices with class 08
Signed-off-by: Oleg Zhurakivskyy <[email protected]>
ed79a20
to
e41822c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds feature allowlist and denylist functionality to the nfd-worker component, providing granular control over which features are published to the Kubernetes API server. This addresses issue #2026 by implementing regex-based filtering for feature names.
- Adds
FeatureAllowList
andFeatureDenyList
configuration options to the core config - Implements filtering logic to check features against both allowlist and denylist patterns
- Adds command-line flags for configuring the new filter options
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
pkg/nfd-worker/nfd-worker.go | Adds core filtering functionality with config fields and filter logic |
cmd/nfd-master/main.go | Adds command-line flag support for the new filtering options |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
ExtraLabelNs: &utils.StringSetVal{}, | ||
ResyncPeriod: &utils.DurationVal{Duration: time.Duration(1) * time.Hour}, | ||
LabelWhiteList: &utils.RegexpVal{}, | ||
FeatureAlowList: &utils.RegexpVal{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in field name: 'FeatureAlowList' should be 'FeatureAllowList' (missing 'l').
FeatureAlowList: &utils.RegexpVal{}, | |
FeatureAllowList: &utils.RegexpVal{}, |
Copilot uses AI. Check for mistakes.
klog.InfoS("feature matches the denylist", "feature", name, "regexp", featureDenyList.String()) | ||
} | ||
|
||
if !(!denied && allowed) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic condition !(!denied && allowed)
is unnecessarily complex and hard to read. It should be simplified to denied || !allowed
using De Morgan's law for better clarity.
if !(!denied && allowed) { | |
if denied || !allowed { |
Copilot uses AI. Check for mistakes.
|
||
allowed := featureAllowList.MatchString(name) | ||
if !allowed { | ||
klog.InfoS("feature does not match the allowlist", "feature", name, "regexp", featureAllowList.String()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logging when a feature doesn't match the allowlist will generate excessive log entries for every filtered feature. Consider only logging at debug level or when verbose logging is enabled to avoid log spam.
klog.InfoS("feature does not match the allowlist", "feature", name, "regexp", featureAllowList.String()) | |
klog.V(1).InfoS("feature does not match the allowlist", "feature", name, "regexp", featureAllowList.String()) |
Copilot uses AI. Check for mistakes.
flagset.Var(overrides.FeatureDenyList, "feature-denylist", | ||
"Regular expression to filter out feature names") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need something more structured than plain regexp. I'd suggest to drop the command line flags and make this a config-file only option.
Klog klogutils.KlogConfigOpts | ||
LabelWhiteList utils.RegexpVal | ||
FeatureAllowList utils.RegexpVal | ||
FeatureDenyList utils.RegexpVal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need something more structured. One idea could be to follow something we have in NodeFeatureRules. We have stuff organized in hierarchy like <source>.<feature>.<element>
. So following the NodeFeatureRule model we could have
featureDenyList:
- feature: "kernel.config"
and then more refined configuration
featureDenyList:
- feature: "pci.device"
class: {op: In, value: ["0880"]}
Closes: #2026