NO-JIRA: cleanup unused code and refactor for allowlist controller #2846
Conversation
karampok
commented
Nov 27, 2025
karampok
commented
- Removes unused code (scheme field, configmap template)
- Refactors/simplifies (inlines single-use helpers, better naming)
- No functional changes, just QOL improvements
WalkthroughRemoves a bundled allowlist ConfigMap manifest, renames two exported constants, and refactors the allowlist controller: replaces wiring/signatures to use a cnoclient.Client, renames constants/helpers, changes manifest rendering to accept a manifest path, and updates lifecycle handling for the ConfigMap and DaemonSet. (50 words) Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/controller/allowlist/allowlist_controller.go (2)
108-116: TODO comment needs clarification or resolution.The comment on line 113 (
TODO should reconcile before on create?) is unclear. Consider addressing it in this PR or adding more context about what needs to be decided.
242-252: Redundant nil check on successful Get.When
Getreturns without error,cmwill always be non-nil. Thecm != nilcheck on line 251 is unnecessary.if err != nil { if apierrors.IsNotFound(err) { return false, nil } return false, err } - return cm != nil, nil + return true, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
bindata/allowlist/config/configmap.yaml(0 hunks)pkg/controller/allowlist/allowlist_controller.go(11 hunks)pkg/names/names.go(1 hunks)
💤 Files with no reviewable changes (1)
- bindata/allowlist/config/configmap.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/names/names.gopkg/controller/allowlist/allowlist_controller.go
🧬 Code graph analysis (1)
pkg/controller/allowlist/allowlist_controller.go (2)
pkg/names/names.go (2)
MultusNamespace(33-33)AllowlistConfigName(36-36)pkg/render/render.go (2)
MakeRenderData(25-30)RenderDir(34-36)
🔇 Additional comments (9)
pkg/names/names.go (1)
35-36: LGTM!The constant rename to
AllowlistConfigNamefollows Go naming conventions, and the comment accurately describes its purpose.pkg/controller/allowlist/allowlist_controller.go (8)
46-51: LGTM!The simplified initialization directly constructing
ReconcileAllowlistremoves unnecessary indirection and improves readability.
79-82: LGTM!Removing the unused
schemefield simplifies the struct.
118-150: LGTM!The reconciliation flow is clear, and the use of
defer cleanupDaemonSetensures cleanup happens regardless of the path taken through the function.
152-167: LGTM!The refactored
createObjectsFromfunction is more flexible by accepting a manifest path parameter, and provides clear error context.
169-179: LGTM!The renamed
getConfigMapfunction has clearer naming and appropriate error handling.
181-213: LGTM!The pod readiness check correctly uses the new constants and handles ownership validation.
215-240: LGTM!The renamed
cleanupDaemonSetfunction is more descriptive, and both functions correctly use the centralized constants.
38-44: Let me search for information about theRenderDirfunction online to understand its expected behavior.
[search query: RenderDir function go manifest rendering bindata]
| // MultusNamespace is the namespace where applied configuration | ||
| // configmaps are stored. | ||
| // Should match 00_namespace.yaml | ||
| const MULTUS_NAMESPACE = "openshift-multus" | ||
| const MultusNamespace = "openshift-multus" |
There was a problem hiding this comment.
Misleading comment copied from APPLIED_NAMESPACE.
The comment states this is "the namespace where applied configuration configmaps are stored" but that describes APPLIED_NAMESPACE above. MultusNamespace is the namespace for Multus network components.
-// MultusNamespace is the namespace where applied configuration
-// configmaps are stored.
-// Should match 00_namespace.yaml
+// MultusNamespace is the namespace where Multus components are deployed.
const MultusNamespace = "openshift-multus"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // MultusNamespace is the namespace where applied configuration | |
| // configmaps are stored. | |
| // Should match 00_namespace.yaml | |
| const MULTUS_NAMESPACE = "openshift-multus" | |
| const MultusNamespace = "openshift-multus" | |
| // MultusNamespace is the namespace where Multus components are deployed. | |
| const MultusNamespace = "openshift-multus" |
🤖 Prompt for AI Agents
In pkg/names/names.go around lines 30 to 33, the comment for MultusNamespace
incorrectly repeats the description for APPLIED_NAMESPACE; update the comment to
accurately state that MultusNamespace is the namespace for Multus network
components (e.g., used for deploying Multus-related resources) and remove the
reference to "applied configuration configmaps". Keep the constant name and
value unchanged.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: karampok, zeeke The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/retest |
karampok
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@karampok: This pull request explicitly references no jira issue. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-aws-ovn-windows PR merged |
| allowlistManifestDir = "../../bindata/network/multus/004-sysctl-configmap.yaml" | ||
| dsName = "cni-sysctl-allowlist-ds" | ||
| dsAnnotation = "app=cni-sysctl-allowlist-ds" | ||
| dsManifest = "../../bindata/allowlist/daemonset/daemonset.yaml" |
There was a problem hiding this comment.
Can you explain the motivation of the dsManifest value change? I am missing it from the commit message. This is being passed to a renderDir function so it wouldn't naturally make sense.
There was a problem hiding this comment.
Since that dir has only one manifest
tree bindata/allowlist/daemonset/
bindata/allowlist/daemonset/
└── daemonset.yaml
having a Dir is more confusing, and there is no path (AFAIK) that will be any other manifest there.
This PR is minimal refactoring, in a next PR I will also refactor the renderDir as this is what does not make sense
There was a problem hiding this comment.
I am not sure I agree. The daemonset is represented by the contents of bindata/allowlist/daemonset/; the fact that this is represented by a single or multiple manifests is a detail that is best left out from the controller code. Otherwise, if in the future we want to add more manifests (let's say a config map) then we would need to change the code. Rendering directories is the common pattern we follow in CNO to abstract the code from this fact.
There was a problem hiding this comment.
no strong opinion, i changed to dsManifestDir = "../../bindata/allowlist/daemonset"
I will fix the typo for the first comment. The second comment is actually code/comment which I do not want to touch since many comments are in that way.
Unfortunately there is no unit test, there are some high level test in origin |
karampok
force-pushed
the
cni-systl-allowlist
branch
from
37a59a6 to
1047c0a
Compare
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/controller/allowlist/allowlist_controller.go (1)
70-70: Typo already flagged in previous review.The comment contains "there cni-sysctl-allowlist" which should be "the cni-sysctl-allowlist". This was already identified in a previous review and you acknowledged you would fix it.
🧹 Nitpick comments (1)
pkg/controller/allowlist/allowlist_controller.go (1)
108-113: Consider clarifying or removing the TODO comment.The TODO on line 113 ("should reconcile before on create?") is unclear. If this is a known enhancement, consider creating an issue and referencing it in the comment, or remove it if it's no longer relevant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
bindata/allowlist/config/configmap.yaml(0 hunks)pkg/controller/allowlist/allowlist_controller.go(11 hunks)pkg/names/names.go(1 hunks)
💤 Files with no reviewable changes (1)
- bindata/allowlist/config/configmap.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/names/names.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/controller/allowlist/allowlist_controller.go
🔇 Additional comments (10)
pkg/controller/allowlist/allowlist_controller.go (10)
1-2: LGTM: Clear package documentation.The updated package comment clearly describes the controller's purpose.
46-47: LGTM: Simplified initialization.Direct instantiation of the reconciler is cleaner than the previous pattern.
79-82: LGTM: Struct simplified.Removal of the unused scheme field aligns with the PR objective to clean up unused code.
86-95: LGTM: Cleaner bootstrap logic.The refactored ConfigMap existence check and creation flow is more straightforward.
152-167: LGTM: Simplified object creation.Removing the intermediate
createObjecthelper and using direct client creation is cleaner. The enhanced error wrapping provides better debugging context.
169-179: LGTM: Clear function naming.Renaming to
getConfigMapis more descriptive than the previousgetConfig.
191-192: LGTM: Constants usage updated.Correct usage of the new public constants
names.MultusNamespaceanddsAnnotation.
215-228: LGTM: Clean DaemonSet cleanup.The function correctly uses updated constants and has appropriate error handling.
230-240: LGTM: Standard DaemonSet retrieval.Correct usage of updated constants with proper error handling for NotFound cases.
242-252: LGTM: Clear existence check.The new
allowlistConfigMapExistsfunction has clear semantics and correctly handles both the NotFound case and other errors.
karampok
force-pushed
the
cni-systl-allowlist
branch
from
1047c0a to
65a9f7c
Compare
Rename constants and functions to make it easier for new users to read the code. Variables within the package do not need to be prefixed with "allowlist" since the package context already provides that scope. Also name in GO should not use ALL_CAPS but use CamelCase instead. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Konstantinos Karampogias <karampok@gmail.com>
The bindata/allowlist/config/configmap.yaml file is not used by any code. The controller uses bindata/network/multus/004-sysctl-configmap.yaml as the template for creating the cni-sysctl-allowlist ConfigMap when it doesn't exist. This template is shared with multus which uses it to create the default-cni-sysctl-allowlist ConfigMap. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Konstantinos Karampogias <karampok@gmail.com>
The runtime.Scheme field was never used - controller creates objects directly via client.Create() without needing type conversion or owner references. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Konstantinos Karampogias <karampok@gmail.com>
Remove createObject() and deleteDaemonSet() helper functions that were each called only once. Inlining them at their call sites improves readability by eliminating unnecessary indirection. This also removes the unused unstructured import. Signed-off-by: Konstantinos Karampogias <karampok@gmail.com>
karampok
force-pushed
the
cni-systl-allowlist
branch
from
65a9f7c to
dbc7837
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/allowlist/allowlist_controller.go (1)
86-95: Fix error handling around allowlistConfigMapExists in ReconcileThe current pattern:
if exists, err := allowlistConfigMapExists(ctx, r.client); !exists { err = createObjectsFrom(ctx, r.client, defaultCMManifest) ... } else if err != nil { ... }is unsafe: if
allowlistConfigMapExistsreturns(false, err)for a non-NotFound error, the!existsbranch runs and attempts to create the ConfigMap while silently discarding the lookup failure. This can mask real API errors and cause confusing behavior.Re-structure to handle
errfirst, thenexists, e.g.:- if exists, err := allowlistConfigMapExists(ctx, r.client); !exists { - err = createObjectsFrom(ctx, r.client, defaultCMManifest) - if err != nil { - klog.Errorf("Failed to create allowlist config map: %v", err) - return reconcile.Result{}, err - } - } else if err != nil { - klog.Errorf("Failed to look up allowlist config map: %v", err) - return reconcile.Result{}, err - } + exists, err := allowlistConfigMapExists(ctx, r.client) + if err != nil { + klog.Errorf("Failed to look up allowlist config map: %v", err) + return reconcile.Result{}, err + } + if !exists { + if err := createObjectsFrom(ctx, r.client, defaultCMManifest); err != nil { + klog.Errorf("Failed to create allowlist config map: %v", err) + return reconcile.Result{}, err + } + }This preserves behavior in the success/NotFound cases while ensuring genuine lookup errors are handled correctly.
Also applies to: 241-250
♻️ Duplicate comments (1)
pkg/names/names.go (1)
30-33: MultusNamespace comment still describes the applied-configmaps namespaceThe comment here still matches APPLIED_NAMESPACE above rather than the Multus namespace purpose, which can confuse readers scanning these constants together.
Consider updating it along these lines to better reflect usage:
-// MultusNamespace is the namespace where applied configuration -// configmaps are stored. -// Should match 00_namespace.yaml +// MultusNamespace is the namespace where Multus components are deployed. +// Should match the Multus namespace in manifests (e.g. 00_namespace.yaml).
🧹 Nitpick comments (1)
pkg/controller/allowlist/allowlist_controller.go (1)
117-117: Consider making createObjectsFrom idempotent for AlreadyExists errorsThe DS lifecycle is:
defer cleanupDaemonSet(...)to ensure cleanup.getDaemonSet+ early retry if DS already exists.createObjectsFromto render and create DS objects.checkDsPodsReadyto wait for readiness.
createObjectsFromcurrently does a plainCreatefor each rendered object:for _, obj := range manifests { if err := client.Default().CRClient().Create(ctx, obj); err != nil { return errors.Wrapf(err, "error creating %s %s/%s", ...) } }If a race or prior partial run leaves a resource already present, this will fail with an AlreadyExists error and abort the reconcile, even though the state might be acceptable.
You could make this more robust by treating AlreadyExists as non-fatal for these one-shot resources, e.g.:
- if err := client.Default().CRClient().Create(ctx, obj); err != nil { - return errors.Wrapf(err, "error creating %s %s/%s", obj.GroupVersionKind(), obj.GetNamespace(), obj.GetName()) - } + if err := client.Default().CRClient().Create(ctx, obj); err != nil { + if apierrors.IsAlreadyExists(err) { + continue + } + return errors.Wrapf(err, "error creating %s %s/%s", obj.GroupVersionKind(), obj.GetNamespace(), obj.GetName()) + }That would keep behavior the same in normal cases while avoiding unnecessary failures on harmless AlreadyExists races.
Also applies to: 130-130, 151-166, 214-223, 230-231
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
bindata/allowlist/config/configmap.yaml(0 hunks)pkg/controller/allowlist/allowlist_controller.go(11 hunks)pkg/names/names.go(1 hunks)
💤 Files with no reviewable changes (1)
- bindata/allowlist/config/configmap.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/names/names.gopkg/controller/allowlist/allowlist_controller.go
🧬 Code graph analysis (1)
pkg/controller/allowlist/allowlist_controller.go (2)
pkg/names/names.go (2)
MultusNamespace(33-33)AllowlistConfigName(36-36)pkg/render/render.go (2)
MakeRenderData(25-30)RenderDir(34-36)
🔇 Additional comments (6)
pkg/names/names.go (1)
35-36: AllowlistConfigName constant is clear and matches controller usageThe new
AllowlistConfigNameconstant and comment are clear and align with its use in the allowlist controller (watch predicate, render data, and existence checks).pkg/controller/allowlist/allowlist_controller.go (5)
1-2: Package-level doc for allowlist controller is concise and helpfulThe new package comment clearly states the controller’s purpose (distributing CNI sysctl allowlist config to nodes) and improves readability without changing behavior.
39-44: DaemonSet/manifest constants are well factoredThe
dsName,dsAnnotation,dsManifestDir, anddefaultCMManifestconstants make the DS and manifest wiring explicit and keep magic strings out of the logic, which helps maintainability.
46-47: Add() wiring and ConfigMap watch predicate look sound
- Using
names.MultusNamespacefor the ConfigMap informer scope keeps the controller focused on the expected namespace.- The predicate with
strings.Contains(object.GetName(), names.AllowlistConfigName)matches bothcni-sysctl-allowlistanddefault-cni-sysctl-allowlist, consistent with the comment about using the default as a trigger.This matches the intended behavior without introducing extra complexity.
Also applies to: 56-56, 70-72
97-115: ConfigMap get + delete semantics are consistent with the documented behavior
if request.Name != names.AllowlistConfigName { return reconcile.Result{}, nil }ensures we only run the DS flow for the main allowlist ConfigMap, while still using changes to the default map as a trigger for creation.getConfigMapreturns(nil, nil)on NotFound, and the Reconcile logic treatsconfigMap == nilas a no-op, matching the comment about deletion not breaking pods and relying on the auto-create check above.This aligns the code with the documented behavior and avoids unnecessary churn on accidental deletes.
Also applies to: 168-178
190-191: Pod readiness check correctly scopes to Multus namespace and DS labelListing pods in
names.MultusNamespacewithLabelSelector: dsAnnotationties the readiness check tightly to the DS you just created and avoids interference from pods in other namespaces.
|
@karampok: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
5 participants
