OCPBUGS-87991: validate additionalNetworks name format in KubeVirt NodePools#8710
Conversation
…dePools The additionalNetworks[].name field in KubeVirt NodePools requires Multus NAD references in <namespace>/<name> format, but no validation enforces this. Invalid names are silently accepted — NodePool reports ValidPlatformConfig=True "All is well" — while VMs fail to start with errors buried 4 layers deep in the hosted control plane namespace (VM conditions set by virt-controller). Add ValidateAdditionalNetworks() called from both the admission webhook (instant user feedback at oc apply time) and PlatformValidation() (reconcile safety net): - Format: exactly one slash with non-empty namespace and name segments - Uniqueness: reject duplicate network names - Length: generated interface name must not exceed 63 characters Fixes: https://redhat.atlassian.net/browse/OCPBUGS-87991 Co-authored-by: Cursor <cursoragent@cursor.com>
|
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-87991, 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. |
📝 WalkthroughWalkthroughThis PR adds validation for KubeVirt NodePool additional networks. A new 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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/hypershift-operator
|
[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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8710 +/- ##
==========================================
+ Coverage 41.50% 41.51% +0.01%
==========================================
Files 758 758
Lines 93689 93719 +30
==========================================
+ Hits 38882 38906 +24
- Misses 52070 52074 +4
- Partials 2737 2739 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hypershift-operator/controllers/nodepool/kubevirt/kubevirt.go (1)
142-162: ⚡ Quick winConsider validating namespace and name parts against Kubernetes DNS subdomain rules.
The current validation checks that parts are non-empty after trimming, but doesn't enforce Kubernetes DNS subdomain naming conventions (RFC 1123). This means names like
"my ns/nad"(with space) or"my_ns/nad"(with underscore) pass validation but will cause VM startup failures when KubeVirt attempts to reference a non-existent NetworkAttachmentDefinition.Kubernetes resource names must be lowercase alphanumeric with
-and., starting and ending with alphanumeric. Adding a regex check like^[a-z0-9]([-a-z0-9]*[a-z0-9])?$for each part would catch these at admission time with clearer error messages.♻️ Example validation with DNS subdomain rules
+import ( + "regexp" +) + +var dnsSubdomainRegex = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$`) + func ValidateAdditionalNetworks(networks []hyperv1.KubevirtNetwork) error { if len(networks) == 0 { return nil } seen := make(map[string]bool, len(networks)) for idx, network := range networks { parts := strings.Split(network.Name, "/") if len(parts) != 2 || strings.TrimSpace(parts[0]) == "" || strings.TrimSpace(parts[1]) == "" { return fmt.Errorf("additionalNetworks[%d].name %q must be in the format <namespace>/<name>", idx, network.Name) } + namespace := strings.TrimSpace(parts[0]) + name := strings.TrimSpace(parts[1]) + if !dnsSubdomainRegex.MatchString(namespace) { + return fmt.Errorf("additionalNetworks[%d].name %q has invalid namespace part %q (must be a valid DNS subdomain)", idx, network.Name, namespace) + } + if !dnsSubdomainRegex.MatchString(name) { + return fmt.Errorf("additionalNetworks[%d].name %q has invalid name part %q (must be a valid DNS subdomain)", idx, network.Name, name) + } if seen[network.Name] { return fmt.Errorf("additionalNetworks[%d].name %q is duplicated", idx, network.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 `@hypershift-operator/controllers/nodepool/kubevirt/kubevirt.go` around lines 142 - 162, The ValidateAdditionalNetworks function currently only checks for non-empty namespace/name parts; update it to enforce Kubernetes DNS subdomain/name rules by validating each part (the namespace and the name from strings.Split(network.Name, "/")) against the RFC1123 regex (e.g. ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$), and return clear formatted errors like "additionalNetworks[%d].name %q: namespace %q is invalid" or "…: name %q is invalid" when a part fails; keep the existing duplicate check and virtualMachineInterfaceName length check intact, and reference the same symbols (ValidateAdditionalNetworks, virtualMachineInterfaceName) so the change is localized to this function.
🤖 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.
Nitpick comments:
In `@hypershift-operator/controllers/nodepool/kubevirt/kubevirt.go`:
- Around line 142-162: The ValidateAdditionalNetworks function currently only
checks for non-empty namespace/name parts; update it to enforce Kubernetes DNS
subdomain/name rules by validating each part (the namespace and the name from
strings.Split(network.Name, "/")) against the RFC1123 regex (e.g.
^[a-z0-9]([-a-z0-9]*[a-z0-9])?$), and return clear formatted errors like
"additionalNetworks[%d].name %q: namespace %q is invalid" or "…: name %q is
invalid" when a part fails; keep the existing duplicate check and
virtualMachineInterfaceName length check intact, and reference the same symbols
(ValidateAdditionalNetworks, virtualMachineInterfaceName) so the change is
localized to this function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 098416f3-4a27-47b2-8089-b95cf2b80aec
📒 Files selected for processing (3)
hypershift-operator/controllers/hostedcluster/hostedcluster_webhook.gohypershift-operator/controllers/nodepool/kubevirt/kubevirt.gohypershift-operator/controllers/nodepool/kubevirt/kubevirt_test.go
|
I now have all the information needed to produce the final report. The root cause is clear. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe gitlint check failed because the commit message title Root CauseThe commit message title uses a The openshift/hypershift repository enforces Conventional Commits via gitlint's built-in [general]
contrib=contrib-title-conventional-commits
[contrib-title-conventional-commits]
types = fix,feat,chore,docs,style,refactor,perf,test,revert,ci,buildThe CT1 rule requires the title to match the pattern This is purely a commit message formatting issue — it is not a code or test problem. RecommendationsAmend the commit message to use the Conventional Commits format. Since this PR adds validation logic (a new feature), the title should use the Alternatively, if this is considered a bug fix (enforcing a contract that should have existed), use To amend: Evidence
|
|
@chdeshpa-hue: all tests passed! 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
KubeVirt NodePools accept invalid
additionalNetworks[].namevalues without any error at admission or reconcile time. The field requires Multus NAD references in<namespace>/<name>format, but no validation enforces this. Users who omit the namespace prefix get a HostedCluster that appears healthy (ValidPlatformConfig: True "All is well") while VMs silently fail to start with errors buried in the hosted control plane namespace.This PR adds
ValidateAdditionalNetworks()called from both:oc applytimePlatformValidation()→ reconcile-time safety netChecks added:
/with non-empty namespace and name segmentsProblem
Without this fix, the debugging path requires navigating 6 layers before finding the root cause (VM conditions in the HC namespace set by virt-controller). NodePool status actively misleads with "All is well". Time to root cause ranges from 30 minutes (expert) to impossible (app team without HC namespace RBAC).
User Experience After Fix
Testing
gofmtandgo vetcleanBackward Compatibility
additionalNetworksconfigured → validation skipped entirelyns/nameentries → pass all checks, zero behavior changeFixes: https://redhat.atlassian.net/browse/OCPBUGS-87991
Made with Cursor
Summary by CodeRabbit