feat: support AllNamespaces install mode for OLMv1#73
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR enforces that PFLACPMonitor resources must be created in the operator's namespace. It wires an OPERATOR_NAMESPACE env var into the operator bootstrap and manifests, passes that namespace into the PFLACPMonitor webhook, and rejects create/update requests for monitors in other namespaces; unit and e2e tests cover the behavior. ChangesNamespace Enforcement for PFLACPMonitor
🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 15 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: karampok The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/v1alpha1/pflacpmonitor_webhook_test.go (1)
73-96:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffAdd cross-namespace conflict test to validate the primary feature.
All existing tests create monitors in the same namespace ("default"), but the primary purpose of this PR change is to enable cluster-wide interface uniqueness validation across different namespaces. The test suite should include at least one test that:
- Creates a monitor with interfaces ["eth0"] in namespace "ns1"
- Attempts to create a monitor with interfaces ["eth0"] in namespace "ns2" (different namespace, overlapping interfaces)
- Verifies the validation correctly detects the conflict and includes both namespace and name in the error message
Without cross-namespace test coverage, the core behavior change (line 11 in validate.go requiring both Name and Namespace match) is not verified.
🧪 Suggested cross-namespace test
Context("with cross-namespace conflicts", func() { BeforeEach(func() { // Create a monitor in namespace "namespace-a" monitorInNsA := &PFLACPMonitor{ ObjectMeta: metav1.ObjectMeta{Name: "monitor-a", Namespace: "namespace-a"}, Spec: PFLACPMonitorSpec{ Interfaces: []string{"eth0"}, }, } err := validator.Client.Create(ctx, monitorInNsA) Expect(err).NotTo(HaveOccurred()) }) It("should reject a monitor in a different namespace with conflicting interfaces", func() { monitorInNsB := &PFLACPMonitor{ ObjectMeta: metav1.ObjectMeta{Name: "monitor-b", Namespace: "namespace-b"}, Spec: PFLACPMonitorSpec{ Interfaces: []string{"eth0"}, // Conflicts with monitor-a }, } _, err := validator.ValidateCreate(ctx, monitorInNsB) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("interfaces [eth0] conflict with the ones from PFLACPMonitor namespace-a/monitor-a")) }) })As per coding guidelines, Ginkgo test code should be reviewed for quality including single responsibility and consistency with codebase patterns.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/v1alpha1/pflacpmonitor_webhook_test.go` around lines 73 - 96, The test suite lacks coverage for cross-namespace interface conflicts; add a Ginkgo Context that uses the validator.Client to create a PFLACPMonitor in one namespace (e.g., PFLACPMonitor with Name "monitor-a", Namespace "namespace-a", Spec.Interfaces ["eth0"]), then attempt to ValidateCreate a second PFLACPMonitor in a different namespace (e.g., Name "monitor-b", Namespace "namespace-b", Spec.Interfaces ["eth0"]) and assert ValidateCreate returns an error whose message contains the conflicting monitor's namespace/name (e.g., "namespace-a/monitor-a"); target the ValidateCreate flow and PFLACPMonitor objects so the test verifies cluster-wide uniqueness behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/v1alpha1/pflacpmonitor_webhook.go`:
- Line 84: The change that removed the namespace filter on
v.Client.List(ctxTimeout, monitorList) is intentional to enable cluster-wide
uniqueness checks across namespaces; ensure the List call remains
namespace-agnostic (no Namespace option passed), verify that monitorList (the
PFLACPMonitor list object) and ctxTimeout (the context with timeout) are
properly constructed and used, and add a short code comment near v.Client.List
mentioning this is deliberate for cross-namespace validation and that webhook
latency should be monitored (and the 60s timeout reviewed) in large clusters.
---
Outside diff comments:
In `@api/v1alpha1/pflacpmonitor_webhook_test.go`:
- Around line 73-96: The test suite lacks coverage for cross-namespace interface
conflicts; add a Ginkgo Context that uses the validator.Client to create a
PFLACPMonitor in one namespace (e.g., PFLACPMonitor with Name "monitor-a",
Namespace "namespace-a", Spec.Interfaces ["eth0"]), then attempt to
ValidateCreate a second PFLACPMonitor in a different namespace (e.g., Name
"monitor-b", Namespace "namespace-b", Spec.Interfaces ["eth0"]) and assert
ValidateCreate returns an error whose message contains the conflicting monitor's
namespace/name (e.g., "namespace-a/monitor-a"); target the ValidateCreate flow
and PFLACPMonitor objects so the test verifies cluster-wide uniqueness behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: a5791f83-f230-4cb5-8fd0-b16939523a18
📒 Files selected for processing (17)
api/v1alpha1/pflacpmonitor_webhook.goapi/v1alpha1/pflacpmonitor_webhook_test.goapi/v1alpha1/validate.gobundle/manifests/pf-status-relay-operator-pf-status-relay_rbac.authorization.k8s.io_v1_clusterrole.yamlbundle/manifests/pf-status-relay-operator-pf-status-relay_rbac.authorization.k8s.io_v1_clusterrolebinding.yamlbundle/manifests/pf-status-relay-operator.clusterserviceversion.yamlcmd/main.goconfig/manager/manager.yamlconfig/manifests/bases/pf-status-relay-operator.clusterserviceversion.yamlconfig/rbac/operand/role.yamlconfig/rbac/operand/role_binding.yamlinternal/controller/pflacpmonitor_controller.gointernal/controller/pflacpmonitor_controller_test.gointernal/controller/suite_test.gomanifests/stable/pf-status-relay-operator-pf-status-relay_rbac.authorization.k8s.io_v1_clusterrole.yamlmanifests/stable/pf-status-relay-operator-pf-status-relay_rbac.authorization.k8s.io_v1_clusterrolebinding.yamlmanifests/stable/pf-status-relay-operator.clusterserviceversion.yaml
|
|
||
| monitorList := &PFLACPMonitorList{} | ||
| err := v.Client.List(ctxTimeout, monitorList, client.InNamespace(monitor.Namespace)) | ||
| err := v.Client.List(ctxTimeout, monitorList) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Cluster-wide list enables cross-namespace validation.
The removal of the namespace filter correctly enables cluster-wide interface uniqueness checking, which is the intended behavior for AllNamespaces install mode.
Operational note: In clusters with a large number of PFLACPMonitor resources across many namespaces, this cluster-wide list during webhook admission may introduce latency. Consider monitoring webhook response times after deployment and verifying that the 60-second timeout (line 33) is sufficient for your largest clusters.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/v1alpha1/pflacpmonitor_webhook.go` at line 84, The change that removed
the namespace filter on v.Client.List(ctxTimeout, monitorList) is intentional to
enable cluster-wide uniqueness checks across namespaces; ensure the List call
remains namespace-agnostic (no Namespace option passed), verify that monitorList
(the PFLACPMonitor list object) and ctxTimeout (the context with timeout) are
properly constructed and used, and add a short code comment near v.Client.List
mentioning this is deliberate for cross-namespace validation and that webhook
latency should be monitored (and the 60s timeout reviewed) in large clusters.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/pflacpmonitor_controller.go`:
- Around line 158-160: The daemonSetName function currently returns a name based
only on namePrefix and pfMonitor.Name which causes DaemonSet name collisions
when two PFLACPMonitor resources share the same name in different namespaces;
update daemonSetName (used by the reconciler) to incorporate the monitor's
namespace (e.g., include pfMonitor.Namespace) into the generated DaemonSet name
so each (namespace,name) pair maps to a unique DaemonSet, and/or add a
validation path in the reconciler to reject duplicate monitor names across
namespaces until the CRD is cluster-scoped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 884d351e-908b-4e89-a99c-4d60dbe3126a
📒 Files selected for processing (3)
internal/controller/pflacpmonitor_controller.gointernal/controller/pflacpmonitor_controller_test.gointernal/controller/suite_test.go
| func (r *PFLACPMonitorReconciler) daemonSetName(pfMonitor *pfstatusrelayv1alpha1.PFLACPMonitor) string { | ||
| return fmt.Sprintf("%s-ds-%s", namePrefix, pfMonitor.Name) | ||
| } |
There was a problem hiding this comment.
Name collision when monitors share names across namespaces.
In AllNamespaces mode, two PFLACPMonitor resources with the same name in different namespaces will generate identical DaemonSet names (e.g., "pf-status-relay-ds-foo") in r.OperatorNamespace. The second monitor's reconciliation will overwrite the first's DaemonSet spec, causing configuration thrashing and operand misconfiguration (wrong interfaces, polling intervals, node selectors).
The commit message mentions removing the namespace prefix "to avoid upgrade issues and prepare for cluster-scoped CRD migration," but until the CRD becomes cluster-scoped, this creates a critical collision window.
🔒 Recommended fix: include namespace in DaemonSet name
func (r *PFLACPMonitorReconciler) daemonSetName(pfMonitor *pfstatusrelayv1alpha1.PFLACPMonitor) string {
- return fmt.Sprintf("%s-ds-%s", namePrefix, pfMonitor.Name)
+ return fmt.Sprintf("%s-ds-%s-%s", namePrefix, pfMonitor.Namespace, pfMonitor.Name)
}Alternatively, if maintaining the original name is required for upgrade compatibility, add cluster-wide validation to reject monitors with duplicate names across namespaces.
📝 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.
| func (r *PFLACPMonitorReconciler) daemonSetName(pfMonitor *pfstatusrelayv1alpha1.PFLACPMonitor) string { | |
| return fmt.Sprintf("%s-ds-%s", namePrefix, pfMonitor.Name) | |
| } | |
| func (r *PFLACPMonitorReconciler) daemonSetName(pfMonitor *pfstatusrelayv1alpha1.PFLACPMonitor) string { | |
| return fmt.Sprintf("%s-ds-%s-%s", namePrefix, pfMonitor.Namespace, pfMonitor.Name) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/pflacpmonitor_controller.go` around lines 158 - 160, The
daemonSetName function currently returns a name based only on namePrefix and
pfMonitor.Name which causes DaemonSet name collisions when two PFLACPMonitor
resources share the same name in different namespaces; update daemonSetName
(used by the reconciler) to incorporate the monitor's namespace (e.g., include
pfMonitor.Namespace) into the generated DaemonSet name so each (namespace,name)
pair maps to a unique DaemonSet, and/or add a validation path in the reconciler
to reject duplicate monitor names across namespaces until the CRD is
cluster-scoped.
| --- | ||
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: RoleBinding | ||
| kind: ClusterRoleBinding |
There was a problem hiding this comment.
Why this change? The pf-status-relay svcacc only use securitycontextconstraints, and Role should be enough. right?
karampok
force-pushed
the
allns-install-mode
branch
2 times, most recently
from
9cc002b to
6784b6a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/main.go (1)
164-169:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake namespace enforcement non-optional at runtime.
Line 164 keeps webhook registration optional. In this PR, namespace policy is enforced in the webhook validator; if
ENABLE_WEBHOOKS=false, out-of-namespacePFLACPMonitorobjects can be admitted but never reconciled (cache is still scoped tooperatorNamespace).Proposed fix
- if os.Getenv("ENABLE_WEBHOOKS") != "false" { - if err = (&pfstatusrelayv1alpha1.PFLACPMonitor{}).SetupWebhookWithManager(mgr, operatorNamespace); err != nil { - setupLog.Error(err, "unable to create webhook", "webhook", "PFLACPMonitor") - os.Exit(1) - } - } + if os.Getenv("ENABLE_WEBHOOKS") == "false" { + setupLog.Error(fmt.Errorf("ENABLE_WEBHOOKS=false"), "namespace enforcement requires webhooks enabled") + os.Exit(1) + } + if err = (&pfstatusrelayv1alpha1.PFLACPMonitor{}).SetupWebhookWithManager(mgr, operatorNamespace); err != nil { + setupLog.Error(err, "unable to create webhook", "webhook", "PFLACPMonitor") + os.Exit(1) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/main.go` around lines 164 - 169, The webhook registration is currently gated by the ENABLE_WEBHOOKS env which allows namespace enforcement to be bypassed; remove that conditional so (&pfstatusrelayv1alpha1.PFLACPMonitor{}).SetupWebhookWithManager(mgr, operatorNamespace) is always invoked (and keep the existing error handling), ensuring the webhook validator enforcing operatorNamespace is always registered regardless of ENABLE_WEBHOOKS.
♻️ Duplicate comments (1)
api/v1alpha1/pflacpmonitor_webhook.go (1)
97-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInterface uniqueness check is still namespace-scoped.
Line 97 uses
client.InNamespace(monitor.Namespace), so duplicate interfaces across namespaces are not detected. That contradicts the cluster-wide uniqueness behavior described for AllNamespaces mode.Suggested fix
- err := v.Client.List(ctxTimeout, monitorList, client.InNamespace(monitor.Namespace)) + err := v.Client.List(ctxTimeout, monitorList)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/v1alpha1/pflacpmonitor_webhook.go` at line 97, The list call that checks for duplicate Interfaces is incorrectly limited to a single namespace: change the v.Client.List invocation that currently passes client.InNamespace(monitor.Namespace) so it performs a cluster-wide list when running in AllNamespaces mode (i.e., remove the client.InNamespace(...) option and call v.Client.List(ctxTimeout, monitorList) or otherwise omit the namespace option). Update the code around v.Client.List, the client.InNamespace usage, and any conditional that detects AllNamespaces so duplicate-interface checks use a cluster-scoped list instead of monitor.Namespace.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/v1alpha1/pflacpmonitor_webhook.go`:
- Around line 72-78: The namespace check currently rejects all namespaced
objects when v.OperatorNamespace is empty; change the conditional to only
enforce the namespace when v.OperatorNamespace is non-empty (e.g., if
v.OperatorNamespace != "" && monitor.Namespace != v.OperatorNamespace) so the
validator becomes a no-op when OperatorNamespace is unset; update the block that
returns apierrors.NewForbidden (using monitor.Namespace, v.OperatorNamespace,
GroupVersion.Group and resource "pflacpmonitors") accordingly.
---
Outside diff comments:
In `@cmd/main.go`:
- Around line 164-169: The webhook registration is currently gated by the
ENABLE_WEBHOOKS env which allows namespace enforcement to be bypassed; remove
that conditional so
(&pfstatusrelayv1alpha1.PFLACPMonitor{}).SetupWebhookWithManager(mgr,
operatorNamespace) is always invoked (and keep the existing error handling),
ensuring the webhook validator enforcing operatorNamespace is always registered
regardless of ENABLE_WEBHOOKS.
---
Duplicate comments:
In `@api/v1alpha1/pflacpmonitor_webhook.go`:
- Line 97: The list call that checks for duplicate Interfaces is incorrectly
limited to a single namespace: change the v.Client.List invocation that
currently passes client.InNamespace(monitor.Namespace) so it performs a
cluster-wide list when running in AllNamespaces mode (i.e., remove the
client.InNamespace(...) option and call v.Client.List(ctxTimeout, monitorList)
or otherwise omit the namespace option). Update the code around v.Client.List,
the client.InNamespace usage, and any conditional that detects AllNamespaces so
duplicate-interface checks use a cluster-scoped list instead of
monitor.Namespace.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 4608a187-7b29-47a7-b951-e517cf085119
📒 Files selected for processing (8)
api/v1alpha1/pflacpmonitor_webhook.goapi/v1alpha1/pflacpmonitor_webhook_test.gobundle/manifests/pf-status-relay-operator.clusterserviceversion.yamlcmd/main.goconfig/manager/manager.yamlconfig/manifests/bases/pf-status-relay-operator.clusterserviceversion.yamle2e/webhook_test.gomanifests/stable/pf-status-relay-operator.clusterserviceversion.yaml
| if monitor.Namespace != v.OperatorNamespace { | ||
| return nil, apierrors.NewForbidden( | ||
| schema.GroupResource{Group: GroupVersion.Group, Resource: "pflacpmonitors"}, | ||
| monitor.Name, | ||
| fmt.Errorf("PFLACPMonitor resources must be created in the operator namespace %q, got %q", v.OperatorNamespace, monitor.Namespace), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Guard namespace enforcement when OperatorNamespace is unset.
Line 72 currently blocks all namespaced objects when OperatorNamespace == "". That breaks the no-restriction validator path exercised by the new tests and makes behavior brittle for any direct validator wiring.
Suggested fix
- if monitor.Namespace != v.OperatorNamespace {
+ if v.OperatorNamespace != "" && monitor.Namespace != v.OperatorNamespace {
return nil, apierrors.NewForbidden(
schema.GroupResource{Group: GroupVersion.Group, Resource: "pflacpmonitors"},
monitor.Name,
fmt.Errorf("PFLACPMonitor resources must be created in the operator namespace %q, got %q", v.OperatorNamespace, monitor.Namespace),
)
}📝 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.
| if monitor.Namespace != v.OperatorNamespace { | |
| return nil, apierrors.NewForbidden( | |
| schema.GroupResource{Group: GroupVersion.Group, Resource: "pflacpmonitors"}, | |
| monitor.Name, | |
| fmt.Errorf("PFLACPMonitor resources must be created in the operator namespace %q, got %q", v.OperatorNamespace, monitor.Namespace), | |
| ) | |
| } | |
| if v.OperatorNamespace != "" && monitor.Namespace != v.OperatorNamespace { | |
| return nil, apierrors.NewForbidden( | |
| schema.GroupResource{Group: GroupVersion.Group, Resource: "pflacpmonitors"}, | |
| monitor.Name, | |
| fmt.Errorf("PFLACPMonitor resources must be created in the operator namespace %q, got %q", v.OperatorNamespace, monitor.Namespace), | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/v1alpha1/pflacpmonitor_webhook.go` around lines 72 - 78, The namespace
check currently rejects all namespaced objects when v.OperatorNamespace is
empty; change the conditional to only enforce the namespace when
v.OperatorNamespace is non-empty (e.g., if v.OperatorNamespace != "" &&
monitor.Namespace != v.OperatorNamespace) so the validator becomes a no-op when
OperatorNamespace is unset; update the block that returns apierrors.NewForbidden
(using monitor.Namespace, v.OperatorNamespace, GroupVersion.Group and resource
"pflacpmonitors") accordingly.
6784b6a to
3c1bd08
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/v1alpha1/pflacpmonitor_webhook.go (1)
97-97: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider clarifying namespace-scoped list behavior.
Since line 72 enforces that all
PFLACPMonitorresources must be in the operator namespace, theclient.InNamespace(monitor.Namespace)filter at line 97 effectively performs a cluster-wide uniqueness check (all monitors are in the same namespace). Consider adding a comment explaining this, or removing the filter entirely for clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/v1alpha1/pflacpmonitor_webhook.go` at line 97, The List call using v.Client.List(ctxTimeout, monitorList, client.InNamespace(monitor.Namespace)) gives the impression of a namespace-scoped uniqueness check but earlier validation requires all PFLACPMonitor resources to live in the operator namespace, so this filter is redundant and confusing; remove the client.InNamespace(monitor.Namespace) argument from the v.Client.List call (or if you prefer to keep it, add a single-line comment next to the v.Client.List invocation explaining that all PFLACPMonitor objects are validated to exist in the operator namespace so the namespace filter is effectively global/cluster-wide) to make the intent clear (referencing v.Client.List, monitor.Namespace and the PFLACPMonitor validation logic).
♻️ Duplicate comments (1)
api/v1alpha1/pflacpmonitor_webhook.go (1)
72-78:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard namespace enforcement when OperatorNamespace is unset.
While
cmd/main.goensuresOPERATOR_NAMESPACEis set in production, adding a guard improves defensive programming and prevents issues if the validator is ever constructed directly in tests or other contexts.🛡️ Suggested defensive guard
func (v *pflacpmonitorValidator) validate(ctx context.Context, monitor *PFLACPMonitor) (admission.Warnings, error) { - if monitor.Namespace != v.OperatorNamespace { + if v.OperatorNamespace != "" && monitor.Namespace != v.OperatorNamespace { return nil, apierrors.NewForbidden( schema.GroupResource{Group: GroupVersion.Group, Resource: "pflacpmonitors"}, monitor.Name, fmt.Errorf("PFLACPMonitor resources must be created in the operator namespace %q, got %q", v.OperatorNamespace, monitor.Namespace), ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/v1alpha1/pflacpmonitor_webhook.go` around lines 72 - 78, The namespace check should guard for an unset operator namespace to avoid silent misvalidation when the validator is constructed outside main; before comparing monitor.Namespace to v.OperatorNamespace in the pflacpmonitor webhook validation (the block referencing v.OperatorNamespace), add an early check for an empty v.OperatorNamespace and return a clear API error (e.g., apierrors.NewInternalError or apierrors.NewBadRequest with a message like "operator namespace not configured on validator") so callers get a deterministic error instead of an incorrect allow/deny; ensure you reference v.OperatorNamespace and use the same apierrors package used elsewhere in this file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@api/v1alpha1/pflacpmonitor_webhook.go`:
- Line 97: The List call using v.Client.List(ctxTimeout, monitorList,
client.InNamespace(monitor.Namespace)) gives the impression of a
namespace-scoped uniqueness check but earlier validation requires all
PFLACPMonitor resources to live in the operator namespace, so this filter is
redundant and confusing; remove the client.InNamespace(monitor.Namespace)
argument from the v.Client.List call (or if you prefer to keep it, add a
single-line comment next to the v.Client.List invocation explaining that all
PFLACPMonitor objects are validated to exist in the operator namespace so the
namespace filter is effectively global/cluster-wide) to make the intent clear
(referencing v.Client.List, monitor.Namespace and the PFLACPMonitor validation
logic).
---
Duplicate comments:
In `@api/v1alpha1/pflacpmonitor_webhook.go`:
- Around line 72-78: The namespace check should guard for an unset operator
namespace to avoid silent misvalidation when the validator is constructed
outside main; before comparing monitor.Namespace to v.OperatorNamespace in the
pflacpmonitor webhook validation (the block referencing v.OperatorNamespace),
add an early check for an empty v.OperatorNamespace and return a clear API error
(e.g., apierrors.NewInternalError or apierrors.NewBadRequest with a message like
"operator namespace not configured on validator") so callers get a deterministic
error instead of an incorrect allow/deny; ensure you reference
v.OperatorNamespace and use the same apierrors package used elsewhere in this
file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 14171852-7694-4b6a-ad79-0e5ff76b29ca
📒 Files selected for processing (9)
api/v1alpha1/pflacpmonitor_webhook.goapi/v1alpha1/pflacpmonitor_webhook_test.gobundle/manifests/pf-status-relay-operator.clusterserviceversion.yamlcmd/main.goconfig/manager/manager.yamlconfig/manifests/bases/pf-status-relay-operator.clusterserviceversion.yamlconfig/openshift/kustomization.yamle2e/webhook_test.gomanifests/stable/pf-status-relay-operator.clusterserviceversion.yaml
💤 Files with no reviewable changes (1)
- config/openshift/kustomization.yaml
karampok
force-pushed
the
allns-install-mode
branch
2 times, most recently
from
dc5e7f6 to
3ee20af
Compare
openshift-ci
Bot
added
the
needs-rebase
Switch install mode from OwnNamespace to AllNamespaces. Replace WATCH_NAMESPACE with OPERATOR_NAMESPACE to always scope cache to operator namespace regardless of OLM install mode. Restrict CR creation to operator namespace via validating webhook. Includes unit and e2e tests. Assisted-By: Claude <noreply@anthropic.com>
karampok
force-pushed
the
allns-install-mode
branch
from
3ee20af to
7d4fa82
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
@karampok: The following test 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. |
2 participants
Summary
AllNamespacesinstall mode for OLMv1 readiness (CNF-23024, parent CNF-22688)Details
Architecture
WATCH_NAMESPACE=""(set by OLM viaolm.targetNamespacesannotation) enables all-namespace watchOPERATOR_NAMESPACEenv (downward API) identifies where DaemonSets are createdpf-status-relay-ds-{cr-namespace}-{cr-name}prevents collisionspfstatusrelay.openshift.io/monitor-nameandmonitor-namespacetrack ownershipOwns()replaced withWatches()+ label mapper (owner refs don't work cross-namespace)Backward compatibility
OwnNamespacemode still supported (non-emptyWATCH_NAMESPACEscopes cache as before)OPERATOR_NAMESPACEalways set via downward API in both deployment methodsTest plan
make generate && make manifests-- no unexpected changesmake test-- all unit tests pass (controller + webhook)operator-sdk bundle validateoperator-sdk run bundlein AllNamespaces modeRef: CNF-23024
Assisted-By: Claude noreply@anthropic.com
Summary by CodeRabbit
New Features
Configuration
Tests