OCPBUGS-105315: Check secret before mutating vSphere templates - #6382
OCPBUGS-105315: Check secret before mutating vSphere templates#6382djoshy wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe vSphere controller validates and upgrades the referenced ignition stub before creating a template, restoring a rollback template, or replacing an outdated template. Errors stop processing before OVA downloads or vSphere mutations. ChangesvSphere ignition stub preflight
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant TemplateReconciliation
participant IgnitionStub
participant VSphereTemplateOperations
TemplateReconciliation->>IgnitionStub: Validate and upgrade referenced stub
alt validation succeeds
IgnitionStub-->>TemplateReconciliation: Return success
TemplateReconciliation->>VSphereTemplateOperations: Create, restore, or replace template
else validation fails
IgnitionStub-->>TemplateReconciliation: Return error
TemplateReconciliation-->>TemplateReconciliation: Stop before vSphere operations
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: djoshy 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
djoshy
changed the title
openshift-ci-robot
added
jira/severity-moderate
|
@djoshy: This pull request references Jira Issue OCPBUGS-105315, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
@djoshy: This pull request references Jira Issue OCPBUGS-105315, which is valid. 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. |
![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
🧹 Nitpick comments (1)
pkg/controller/bootimage/vsphere_helpers.go (1)
365-370: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftThread
context.Contextthrough boot-image reconciliation.
upgradeStubIgnitionIfRequiredusescontext.TODO()for both Secret requests. Introduce a reconciliation context and pass it throughsyncMAPIMachineSet,checkMachineSet, the provider helpers, andcreateNewVMTemplate. Use it for bothGetandUpdate, and update all six callers.🤖 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 `@pkg/controller/bootimage/vsphere_helpers.go` around lines 365 - 370, Thread the reconciliation context through syncMAPIMachineSet, checkMachineSet, the provider helper methods, and createNewVMTemplate, updating all six callers and their signatures. Change upgradeStubIgnitionIfRequired to accept that context and use it for both Secret Get and Update operations instead of context.TODO().Source: Path instructions
🤖 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 `@pkg/controller/bootimage/vsphere_helpers.go`:
- Around line 751-758: The validation in createNewVMTemplate must occur before
resolveExistingTemplateVM renames the existing oldVM; move
upgradeStubIgnitionIfRequired ahead of the rollback rename and ensure every path
validates the Secret, including when the restored template already matches
release. Add a regression test covering an invalid Secret with an existing
mco-old-* VM, asserting no vCenter template mutation and preservation of
BootImageUpdateDegraded.
---
Nitpick comments:
In `@pkg/controller/bootimage/vsphere_helpers.go`:
- Around line 365-370: Thread the reconciliation context through
syncMAPIMachineSet, checkMachineSet, the provider helper methods, and
createNewVMTemplate, updating all six callers and their signatures. Change
upgradeStubIgnitionIfRequired to accept that context and use it for both Secret
Get and Update operations instead of context.TODO().
🪄 Autofix
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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d7d7d86b-bace-4e34-b0a1-57693f4b8d18
📒 Files selected for processing (1)
pkg/controller/bootimage/vsphere_helpers.go
9a69788 to
488c2a3
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/test e2e-vsphere-mco-bootimages e2e-vsphere-multi-vcenter-mco-bootimages |
|
/test e2e-vsphere-mco-bootimages e2e-vsphere-multi-vcenter-mco-bootimages |
|
@djoshy: 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. |
|
closing in favor of #6387 (comment) |
|
@djoshy: This pull request references Jira Issue OCPBUGS-105315. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW state. 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. |
2 participants
- What I did
vSphere's reconciler swapped the VM template in vCenter before validating the user-data secret via
upgradeStubIgnitionIfRequired. If that secret was broken, the error only surfaced after vCenter was already mutated, so the next reconcile saw a matching version and clearedBootImageUpdateDegraded, even though the secret was still broken. Other platforms don't hit this since none of them involve mutating objects native to their environment for performing updates.To fix this, additionals call for
upgradeStubIgnitionIfRequiredwere added before the mutating create/swap in bothresolveExistingTemplateVMandcreateNewVMTemplate, so a bad secret fails fast and the error persists as expected. Kept the existing call inplatform_helpers.gofor the name-divergence-only patch path.- How to verify it
The targeted test for this failure mode,
[sig-mco][Suite:openshift/machine-config-operator/longduration][Serial][Disruptive] MCO Bootimages [PolarionID:80436][OTP] Bootimage secret doesn't exist error upgrading stub ignition to spec 3should now pass.Existing boot image tests should also continue to pass.
Summary by CodeRabbit