OCPBUGS-109196: fix(nodepool): validate KubeVirt additionalNetworks NAD namespace - #9299
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@chdeshpa-hue: This pull request references Jira Issue OCPBUGS-109196, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 4 reviews per rolling hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe documentation defines the required namespace for namespaced NetworkAttachmentDefinitions. KubeVirt validation accepts the virt-launcher infrastructure namespace and Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
area/documentation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chdeshpa-hue 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 |
|
/jira refreshed |
Codecov Report❌ Patch coverage is Please upload reports for the commit ffc16da to get more accurate results.
Additional details and impacted files@@ Coverage Diff @@
## main #9299 +/- ##
==========================================
+ Coverage 45.73% 45.74% +0.01%
==========================================
Files 781 781
Lines 97828 97861 +33
==========================================
+ Hits 44738 44765 +27
- Misses 50019 50025 +6
Partials 3071 3071
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
With Multus namespace isolation enabled (OpenShift default), virt-launcher pods can only reference NetworkAttachmentDefinitions in their own namespace or "default". Users naturally place NADs in the HostedCluster namespace, but virt-launcher runs in the HCP namespace. Add validation in validPlatformConfigCondition that checks the NAD namespace in each additionalNetworks entry against the resolved virt-launcher namespace. On mismatch, set ValidPlatformConfig=False with an actionable message telling the user the correct namespace. Also documents the namespace requirement in the KubeVirt networking how-to guide. Co-authored-by: Cursor <cursoragent@cursor.com>
bbcfd7b to
e6cefe8
Compare
chdeshpa-hue
changed the title
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
Two-axis review (Standards + Spec) of e6cefe830a against main.
Solid, well-tested fix that maps cleanly onto the OCPBUGS-109196 proposed fix — the namespace resolution (HCP ns with dot→hyphen, Credentials.InfraNamespace override for external infra) and the actionable error message are correct, and the 9 table-test cases match the described scenarios. Two things worth a look before merge:
Standards
- (discussion, not blocking) This adds controller-time validation where
api/AGENTS.mdsteers new validation to admission-time CEL. Namespace-string comparison is CEL-expressible, unlike the AWScasethis mirrors (AWS needs live subnet lookups). The mitigation is real — the "correct" namespace derives from cross-object data (HostedControlPlaneNamespace/Credentials.InfraNamespace) a NodePool CEL rule can't see — but the PR should state why CEL was rejected. If it stays controller-time,api/AGENTS.md's envtest requirement is moot. - Minor: the comment enumerates
openshift-multus/openshift-sriov-network-operatoras exempt, but the code only exemptsdefault. Comment describes behavior the code doesn't implement. - Minor (Primitive Obsession):
network.Namecarries anamespace/namedomain concept parsed inline; consider a small accessor onKubevirtNetwork.
Spec
- Faithful to the Proposed Fix. One nuance: the issue's Expected Result says validation should reject "immediately" (admission), but this rejects asynchronously via
ValidPlatformConfig=False. That's explicitly sanctioned by the Proposed Fix line, so acceptable — flagging only so it's a conscious choice. - No scope creep. The
defaultexemption is explicitly required by the issue;aggregated-docs.mdis generated.
Nothing here is a hard blocker on its own; the CEL-vs-controller question is the one worth a reviewer decision.
| condition.Reason = hyperv1.AWSErrorReason | ||
| condition.Message = err.Error() | ||
| } | ||
| case hyperv1.KubevirtPlatform: |
There was a problem hiding this comment.
Per api/AGENTS.md ("prefer admission-time via CEL over controller-time validation"), consider whether this belongs as a CEL rule on the API type rather than a reconcile-time condition. Namespace-string comparison is CEL-expressible (unlike the AWS case above, which needs live subnet lookups). If controller-time is intentional because the target namespace derives from cross-object data (HostedControlPlaneNamespace / Credentials.InfraNamespace) that a NodePool CEL rule can't reach, please note that rationale in the PR description.
There was a problem hiding this comment.
Controller-time is intentional here.
The allowed NAD namespace is not a NodePool field — it is the virt-launcher namespace, derived from the HostedCluster (HostedControlPlaneNamespace(hc.Namespace, hc.Name), or Credentials.InfraNamespace for external infra). A NodePool CEL rule can only see NodePool spec, so it cannot express this constraint.
I've added that rationale to the PR description, including why this is ValidPlatformConfig=False rather than an immediate admission reject. api/AGENTS.md's envtest/CEL requirement does not apply while this stays reconcile-time.
| } | ||
|
|
||
| for _, network := range kvPlatform.AdditionalNetworks { | ||
| parts := strings.SplitN(network.Name, "/", 2) |
There was a problem hiding this comment.
(Optional) network.Name encodes a namespace/name domain concept that's documented on KubevirtNetwork but parsed ad-hoc here. A small namespace() accessor on the type would keep this split in one place and avoid re-implementing it if another caller needs it.
There was a problem hiding this comment.
Leaving the split inline for this PR. This is the only call site, and KubevirtNetwork.Name is already documented as namespace/name. I'd rather not add an API helper until a second caller needs it.
|
|
||
| for _, network := range kvPlatform.AdditionalNetworks { | ||
| parts := strings.SplitN(network.Name, "/", 2) | ||
| if len(parts) != 2 { |
There was a problem hiding this comment.
Worth a one-line comment that bare names (no /) are intentionally skipped because Multus resolves them in the pod's own namespace, which is already safe. Makes the "why" explicit for the next reader.
There was a problem hiding this comment.
Done — added a comment above the len(parts) != 2 continue: bare names (no /) are skipped because Multus resolves them in the pod's own namespace, which is already the virt-launcher namespace.
| // "default" is always in Multus globalNamespaces on OCP. Other global namespaces | ||
| // (openshift-multus, openshift-sriov-network-operator) are also exempt from isolation | ||
| // but are operator-internal — user NADs should not be placed there. |
There was a problem hiding this comment.
The comment says openshift-multus/openshift-sriov-network-operator "are also exempt from isolation," but the code only exempts "default". Reads as behavior the code doesn't implement — suggest trimming the aside to just explain the default exemption to avoid misleading future readers.
There was a problem hiding this comment.
Done — trimmed the comment to only explain the default exemption. The code does not special-case openshift-multus / openshift-sriov-network-operator, so listing them as exempt was misleading.
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Good, focused fix — clear problem, minimal scope, solid tests. A few suggestions inline.
| // references a NAD in the namespace where virt-launcher pods will run, or in "default". | ||
| // With Multus namespace isolation enabled (OpenShift default), pods cannot reference NADs | ||
| // in other namespaces. | ||
| func (r *NodePoolReconciler) validateKubevirtAdditionalNetworkNamespaces(nodePool *hyperv1.NodePool, hc *hyperv1.HostedCluster) error { |
There was a problem hiding this comment.
The existing validateAWSPlatformConfig and the caller validPlatformConfigCondition both use a value receiver NodePoolReconciler. This function doesn't use r at all — could you switch it to a value receiver for consistency? Or even make it a standalone function since it's a pure validation.
There was a problem hiding this comment.
Done — switched to a value receiver to match validateAWSPlatformConfig and validPlatformConfigCondition. Left it as a method rather than a standalone function so the call site stays r.validate…, same as the AWS case.
| condition.Reason = hyperv1.AWSErrorReason | ||
| condition.Message = err.Error() | ||
| } | ||
| case hyperv1.KubevirtPlatform: |
There was a problem hiding this comment.
Have you considered what happens to existing clusters after the operator upgrades? If someone has a working setup where NADs are in a non-HCP namespace (e.g., Multus isolation is disabled, or they're on a non-OCP management cluster), this will start surfacing ValidPlatformConfig=False after the upgrade. The condition is informational so it won't break anything, but it could trigger monitoring alerts. Worth mentioning in the PR description or adding a release note?
There was a problem hiding this comment.
Yes — this is condition-only. validPlatformConfigCondition still returns nil, so reconcile and VM create are not blocked.
Two cases after upgrade:
- OpenShift with default Multus isolation:
ValidPlatformConfig=Falseis the real failure becoming visible (today it only shows up as an opaque virt-launcher/Multus error). - Isolation disabled, or a non-OCP management cluster: NADs outside the HCP/infra namespace can still work, so the False condition can be a false positive for anyone alerting on it.
I've added an upgrade note to the PR description covering both.
| // (openshift-multus, openshift-sriov-network-operator) are also exempt from isolation | ||
| // but are operator-internal — user NADs should not be placed there. | ||
| if nadNamespace != infraNS && nadNamespace != "default" { | ||
| return fmt.Errorf( |
There was a problem hiding this comment.
The error message says "this is likely to be rejected" (hedging) but the condition status is ConditionFalse (definitive). These two signals disagree — either make the message definitive ("this will be rejected") or consider whether a warning-level response would be more appropriate for cases where isolation might be disabled.
There was a problem hiding this comment.
Keeping ConditionFalse and the hedge.
False means the NAD namespace does not match the virt-launcher namespace under default Multus isolation — not that VM create is blocked. The message says "likely" because isolation can be disabled (or the management cluster may not be OpenShift); in those cases the VM can still start.
A warning-level / ConditionUnknown would change semantics for ValidPlatformConfig and is out of scope for this fix. The condition is the signal; reconcile does not short-circuit on it.
| infraNS = hc.Spec.Platform.Kubevirt.Credentials.InfraNamespace | ||
| } | ||
|
|
||
| for _, network := range kvPlatform.AdditionalNetworks { |
There was a problem hiding this comment.
Minor: with multiple additionalNetworks, this returns on the first invalid one. If a user has 3 NADs in the wrong namespace, they fix one, re-reconcile, see the next error, fix, repeat. Would collecting all invalid entries into a single error message be better UX? Not blocking — just a thought.
There was a problem hiding this comment.
Leaving fail-on-first to match validateAWSPlatformConfig. The user still gets an actionable namespace in the message; remaining entries show up on the next reconcile after they fix the current one. Happy to collect them in a follow-up if you want it in this PR.
Use a value receiver to match validateAWSPlatformConfig, document why bare NAD names are skipped, and trim the default-exemption comment to behavior the code actually implements. Co-authored-by: Cursor <cursoragent@cursor.com>
openshift-ci-robot
added
jira/valid-bug
|
@chdeshpa-hue: This pull request references Jira Issue OCPBUGS-109196, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
@chdeshpa-hue: 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. |
4 participants
Summary
validPlatformConfigConditionthat checksadditionalNetworksNAD namespace against the resolved virt-launcher pod namespaceValidPlatformConfig=Falsewith an actionable message telling users where to create the NADProblem
NodePool.spec.platform.kubevirt.additionalNetworksaccepts NAD references passed verbatim to Multus viaMultusNetwork.NetworkName. Users naturally place NADs in the HostedCluster namespace, but virt-launcher pods run in the HCP namespace ({hc-ns}-{hc-name}). With Multus namespace isolation enabled (OCP default), the cross-namespace reference is rejected — but the only signal is an opaque pod sandbox error buried in virt-launcher events.Fix
Mirrors the existing AWS platform validation pattern:
The validation resolves the target namespace (HCP ns or
Credentials.InfraNamespace) and checks eachadditionalNetworksentry's namespace portion.Why not CEL / admission-time validation
This check stays controller-time on purpose. The allowed NAD namespace is the virt-launcher namespace, which comes from the HostedCluster (
HostedControlPlaneNamespace/Credentials.InfraNamespace). A NodePool CEL rule cannot see that cross-object data.The issue's Expected Result asked for an immediate (admission) reject. This PR uses
ValidPlatformConfig=Falseinstead — asynchronous, informational, and aligned with the Proposed Fix and the existing AWS platform-validation pattern. There is no admission webhook in this change.Upgrade note
After this lands, KubeVirt NodePools whose
additionalNetworksNAD is not in the virt-launcher namespace (HCP/infra) ordefaultwill reportValidPlatformConfig=False. Reconcile is not blocked and VMs are still created.On OpenShift with default Multus isolation this matches real Multus rejection. If namespace isolation is disabled (or the management cluster is not OpenShift), the condition can be a false positive for monitoring.
Test plan
ci/prow/unit,ci/prow/verify,ci/prow/lintVerification output
/jira OCPBUGS-109196
Made with Cursor
Summary by CodeRabbit
Bug Fixes
defaultnamespace.Documentation