OCPBUGS-98700: Azure: delete bootstrap ignition storage during bootstrap destroy#10701
OCPBUGS-98700: Azure: delete bootstrap ignition storage during bootstrap destroy#10701chdeshpa-hue wants to merge 1 commit into
Conversation
The installer creates a storage account to host bootstrap.ign during Azure IPI installs but never deletes it during bootstrap destroy. This leaves sensitive bootstrap data (CA certs, auth tokens) in a network-accessible storage account for the lifetime of the cluster. Extend PostDestroy to delete the ignition container and, if no other containers remain (e.g. vhd for OKD/confidential VM image gallery), delete the entire storage account. Azure Stack is excluded (uses a different storage SDK). Cleanup is best-effort: failures log a warning but do not block install completion. 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/severity-important
|
@chdeshpa-hue: This pull request references Jira Issue OCPBUGS-98700, 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. |
📝 WalkthroughWalkthroughAzure ChangesAzure bootstrap cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ 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.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 refresh |
openshift-ci-robot
added
jira/valid-bug
|
@chdeshpa-hue: This pull request references Jira Issue OCPBUGS-98700, 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. |
![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
🤖 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/infrastructure/azure/azure.go`:
- Around line 931-932: Update the container-listing error branch in the helper
used by PostDestroy to return the encountered error instead of nil, preserving
the existing successful return path. Ensure PostDestroy continues to handle the
propagated error for best-effort cleanup.
🪄 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: CHILL
Plan: Enterprise
Run ID: 535bc5a7-4392-41a9-bdb9-5068013338a7
📒 Files selected for processing (1)
pkg/infrastructure/azure/azure.go
| return nil | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Propagate the error when listing containers fails.
Returning nil here swallows the error and falsely reports success to the caller. Since PostDestroy safely traps errors from this helper to perform a best-effort cleanup without failing the overall process, returning the error directly ensures it is properly propagated and logged by the caller. As per path instructions, never ignore error returns.
♻️ Proposed fix to propagate the error
- logrus.Warnf("Failed to list containers in storage account %s, skipping account deletion: %v", storageAccountName, err)
- return nil
+ return fmt.Errorf("failed to list containers in storage account %s: %w", storageAccountName, err)📝 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.
| return nil | |
| } | |
| return fmt.Errorf("failed to list containers in storage account %s: %w", storageAccountName, err) | |
| } |
🤖 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/infrastructure/azure/azure.go` around lines 931 - 932, Update the
container-listing error branch in the helper used by PostDestroy to return the
encountered error instead of nil, preserving the existing successful return
path. Ensure PostDestroy continues to handle the propagated error for
best-effort cleanup.
Source: Path instructions
|
@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
{infraID-no-hyphens}sa) to hostbootstrap.ign, but never deletes it during bootstrap destroy. The account can persist for the life of the cluster with sensitive bootstrap material.PostDestroyto delete the ignition container and, if no other containers remain (e.g. OKD/CVMvhdimage gallery), delete the storage account. Azure Stack excluded. Best-effort (warn, do not fail install). Idempotent on 404.Fix details
ignitioncontainer (always safe)ResourceNotFound/404 as successPattern: AWS
PostDestroyS3 ignition cleanup; GCP equivalent in #8489 (OCPBUGS-33681).What this does NOT change
PostDestroydestroy cluster(RG deletion still cleans up)Test plan
gofmt/go vetclean onpkg/infrastructure/azure/...openshift-install{infraID}saremains after install completevhdcontainer remainsBuild evidence
Fixes: https://issues.redhat.com/browse/OCPBUGS-98700
Made with Cursor
Summary by CodeRabbit