SPLAT-2683: Implement opt-in RHCOS OVA resolution, download, and import - #57
Open
jcpowermac wants to merge 5 commits into
Open
SPLAT-2683: Implement opt-in RHCOS OVA resolution, download, and import #57jcpowermac wants to merge 5 commits into
jcpowermac wants to merge 5 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jcpowermac: This pull request references SPLAT-2683 which is a valid jira issue. 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. |
|
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 adds optional RHCOS OVA import support, a new migration condition, vSphere preflight and reconcile logic, cluster tag helpers, and related API, CRD, UI, manifest, and test updates. ChangesRHCOS OVA migration flow
Sequence Diagram(s)sequenceDiagram
participant Reconciler
participant ConfigMap
participant ImageLibrary
participant vSphere
participant MigrationStatus
Reconciler->>ConfigMap: ResolveRHCOSOVAFromConfigMap
Reconciler->>ImageLibrary: DownloadOVAToDir
Reconciler->>ImageLibrary: ImportOVA
ImageLibrary->>vSphere: Create import spec and upload VMDKs
ImageLibrary->>vSphere: Mark template and disable secure boot
Reconciler->>MigrationStatus: Record download and imported template status
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jcpowermac 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: 9
🧹 Nitpick comments (1)
internal/vsphere/tags_test.go (1)
372-389: ⚡ Quick winStrengthen
TestEnsureClusterTagwith associable-type compatibility checks.Please add assertions that the created category includes all required cluster associable types, and add a case for reusing an existing incompatible category (missing
VirtualMachine, etc.) to lock in expected failures.Also applies to: 392-410
🤖 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/vsphere/tags_test.go` around lines 372 - 389, Update TestEnsureClusterTag to assert the created category's associable types include the required cluster types (e.g., "VirtualMachine", plus any other cluster-specific types used by our code) by fetching the category via s.TagManager.GetCategory and checking Category.AssociableTypes contains those entries in addition to checking Category.Cardinality; also add a new subtest that simulates reusing an existing incompatible category (one missing "VirtualMachine" in its AssociableTypes) and assert EnsureClusterTag returns the expected error/failed outcome for that case so we lock in the incompatibility behavior. Ensure you reference the test function TestEnsureClusterTag, the TagManager.GetCategory/GetTag calls, the category variable (cat) and its Cardinality and AssociableTypes fields when adding these assertions and the new negative subtest.
🤖 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/vmwarecloudfoundationmigration_types.go`:
- Around line 64-69: The doc comment for Image and the embedded
VSpherePlatformFailureDomainSpec disagree about whether topology.template is
auto-calculated when spec.image is omitted; update the API comments to be
consistent: change either the Image field comment or the embedded
VSpherePlatformFailureDomainSpec's template comment so both state the actual
fallback behavior (e.g., if Image is omitted, topology.template must be provided
manually and will NOT be auto-calculated), and ensure you reference the symbols
Image (*ImageSpec), topology.template, and VSpherePlatformFailureDomainSpec so
readers can find and reconcile the contract.
- Around line 74-82: The OVAUrl and resolvedOVAUrl fields allow
credential-bearing URLs to be stored in plain CR spec/status; change the API to
avoid persisting secrets by replacing or augmenting OVAUrl with a Secret
reference (e.g., a new field like OVASecretRef or OVASecretName) and stop
echoing full URLs into status (remove or redact resolvedOVAUrl), or, if you must
keep a URL field, tighten validation on OVAUrl/validated input in the
VMWARECLOUDFOUNDATIONMIGRATION spec to reject query strings/credentials (no "?"
or token patterns) and update code that sets resolvedOVAUrl to persist only
non-credential metadata (e.g., hostname/path) rather than the full URL; update
related types and any controller logic that reads OVAUrl/resolvedOVAUrl (search
for OVAUrl and resolvedOVAUrl) accordingly.
In `@internal/controller/preflight.go`:
- Around line 235-237: The log is exposing sensitive OVA URLs; update logging
and error surfaces to redact or omit full spec.image.ovaUrl. Modify the
preflight reachability path (where checkOVAURLReachable is called) to pass a
sanitized value to log.V(1).Info (e.g., replace host/credentials/query with a
masked string) or call a new helper sanitizeOVAURL(ovaURL) before logging, and
change checkOVAURLReachable and the main import flow to return or log only the
sanitized URL or a generic identifier instead of the raw ovaURL so no
credentials/tokens/hostnames are written to logs or error messages. Ensure all
references to spec.image.ovaUrl in preflight.go and the import code use that
sanitizer.
- Around line 255-303: validateImageImportPrivileges currently always checks
cluster.ResourcePool(ctx); change its signature to accept an optional resource
pool reference (e.g. add param resourcePoolRef *types.ManagedObjectReference or
similar) and inside the function use
object.NewResourcePool(session.Client.Client, *resourcePoolRef) when
resourcePoolRef is non-nil (fall back to cluster.ResourcePool(ctx) otherwise),
then run the privilege checks against that chosen resource pool; update all call
sites (notably the preflight call that validates per-failure-domain) to pass
fd.Topology.ResourcePool when set so the privilege check uses the effective
resource pool used by ImportOVA.
In `@internal/vsphere/image.go`:
- Around line 137-139: The defer currently swallows the syscall.Flock unlock
error and the temp-file cleanup errors (e.g., lines using os.Remove or
tempFile.Remove) which violates the “Never ignore error returns” rule; update
the defer and cleanup code to capture the returned error from syscall.Flock(...,
syscall.LOCK_UN) (where lockFile is used) and from temp-file removal, and handle
them (log via the package logger or return a wrapped error) instead of assigning
to _; ensure you reference the same symbols (lockFile, syscall.Flock,
syscall.LOCK_UN and the temp file variable/name used in the cleanup) and
propagate or log failures so lock/unlink issues are visible in production.
- Line 153: The log currently emits the full ovaURL (in the log.Info call),
which can leak signed tokens in query strings; before logging, parse and
sanitize ovaURL (e.g., use net/url.Parse to build a sanitized string by dropping
u.RawQuery or by logging only u.Scheme + "://" + u.Host + u.Path and/or a flag
indicating a query was removed) and replace ovaURL in the log.Info call with the
sanitized value (keep localPath as-is). Locate the log.Info("downloading OVA",
"url", ovaURL, "dest", localPath) usage and change it to log the sanitized URL
or host/path plus an indicator that query parameters were omitted.
- Around line 366-385: After obtaining the NFC lease from
resourcePool.ImportVApp, ensure the lease is aborted on all error paths by
adding a defer that calls lease.Abort(ctx, errOrReason) unless the lease was
successfully completed; locate the block using resourcePool.ImportVApp,
lease.Wait, lease.StartUpdater, upload, and lease.Complete and replace the
current early returns so the defer aborts the lease on failures and is
canceled/disabled when lease.Complete(ctx) succeeds (or set a flag to skip abort
in the defer).
In `@internal/vsphere/tags.go`:
- Around line 347-353: ensureTagCategoryWithTypes currently reuses an existing
tag category ID without checking that existing.AssociableTypes contains the
required associable types (and it also skips these checks in the
"already_exists" retry path); update ensureTagCategoryWithTypes to validate that
existing.AssociableTypes includes all required types and that
existing.Cardinality equals the requested cardinality before returning the ID
(use the same validation in both the initial reuse branch where existing != nil
and in the "already_exists" retry branch), and return a clear error (including
name, existing.AssociableTypes and cardinality) if the checks fail instead of
silently reusing the category.
---
Nitpick comments:
In `@internal/vsphere/tags_test.go`:
- Around line 372-389: Update TestEnsureClusterTag to assert the created
category's associable types include the required cluster types (e.g.,
"VirtualMachine", plus any other cluster-specific types used by our code) by
fetching the category via s.TagManager.GetCategory and checking
Category.AssociableTypes contains those entries in addition to checking
Category.Cardinality; also add a new subtest that simulates reusing an existing
incompatible category (one missing "VirtualMachine" in its AssociableTypes) and
assert EnsureClusterTag returns the expected error/failed outcome for that case
so we lock in the incompatibility behavior. Ensure you reference the test
function TestEnsureClusterTag, the TagManager.GetCategory/GetTag calls, the
category variable (cat) and its Cardinality and AssociableTypes fields when
adding these assertions and the new negative subtest.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8686d85a-f34a-44a8-88d2-fc56c920be79
⛔ Files ignored due to path filters (8)
api/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*go.sumis excluded by!**/*.sumvendor/github.com/coreos/stream-metadata-go/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/stream-metadata-go/stream/artifact_utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/stream-metadata-go/stream/rhcos/rhcos.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/stream-metadata-go/stream/stream.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/stream-metadata-go/stream/stream_utils.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (15)
api/v1alpha1/vmwarecloudfoundationmigration_types.goconfig/crd/bases/migration.openshift.io_vmwarecloudfoundationmigrations.yamlconfig/manager/manager.yamlconsole-plugin/web/src/app/pages/MigrationDetailPage.tsxgo.modinternal/controller/image_import_test.gointernal/controller/preflight.gointernal/controller/vmwarecloudfoundationmigration_controller.gointernal/openshift/infrastructure.gointernal/openshift/infrastructure_test.gointernal/openshift/version.gointernal/vsphere/image.gointernal/vsphere/image_test.gointernal/vsphere/tags.gointernal/vsphere/tags_test.go
💤 Files with no reviewable changes (1)
- internal/openshift/infrastructure.go
Comment on lines
+74
to
+82
| // OVAUrl is a direct URL to the RHCOS OVA file. When set, the operator | ||
| // downloads from this URL instead of resolving via the coreos-bootimages | ||
| // ConfigMap delivered by CVO. | ||
| // Supports both direct .ova URLs and URLs with query parameters (e.g. | ||
| // integrity digests appended by stream metadata tooling). | ||
| // Required for air-gapped environments: point to an internal HTTP(S) mirror. | ||
| // +optional | ||
| // +kubebuilder:validation:Pattern=`^https://.*\.ova(\?.*)?$` | ||
| OVAUrl string `json:"ovaUrl,omitempty"` |
There was a problem hiding this comment.
Avoid credential-bearing OVA URLs in plain CR fields.
ovaUrl is a plain-text spec field, resolvedOVAUrl echoes it into status, and the docs explicitly allow query parameters. That makes presigned URLs or tokenized mirrors durable CR metadata visible to any reader of the object. Prefer a Secret/reference for authenticated downloads, or at minimum reject credential-bearing query strings and avoid persisting the full URL in status.
Also applies to: 116-118
🤖 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/vmwarecloudfoundationmigration_types.go` around lines 74 - 82,
The OVAUrl and resolvedOVAUrl fields allow credential-bearing URLs to be stored
in plain CR spec/status; change the API to avoid persisting secrets by replacing
or augmenting OVAUrl with a Secret reference (e.g., a new field like
OVASecretRef or OVASecretName) and stop echoing full URLs into status (remove or
redact resolvedOVAUrl), or, if you must keep a URL field, tighten validation on
OVAUrl/validated input in the VMWARECLOUDFOUNDATIONMIGRATION spec to reject
query strings/credentials (no "?" or token patterns) and update code that sets
resolvedOVAUrl to persist only non-credential metadata (e.g., hostname/path)
rather than the full URL; update related types and any controller logic that
reads OVAUrl/resolvedOVAUrl (search for OVAUrl and resolvedOVAUrl) accordingly.
Comment on lines
+331
to
+335
| // Phase 1: Skip if not opted in. | ||
| if migration.Spec.Image == nil { | ||
| r.setCondition(migration, condType, metav1.ConditionTrue, migrationv1alpha1.ReasonCompleted, | ||
| "Image import not requested, using pre-configured templates") | ||
| return ctrl.Result{}, nil |
There was a problem hiding this comment.
Don’t complete DestinationImageImported while templates are still missing.
When spec.image is nil, this stage goes straight to True without checking failureDomains[*].topology.template. ensureWorkloadMigrated later hard-fails on that prerequisite, so the controller can already run destination initialization and multi-site reconfiguration before discovering the migration cannot continue. Gate this stage on every failure domain having a template when image import is not requested.
Suggested fix
// Phase 1: Skip if not opted in.
if migration.Spec.Image == nil {
+ for i := range migration.Spec.FailureDomains {
+ if migration.Spec.FailureDomains[i].Topology.Template == "" {
+ return ctrl.Result{}, fmt.Errorf(
+ "spec.failureDomains[%d].topology.template is required when spec.image is omitted",
+ i,
+ )
+ }
+ }
r.setCondition(migration, condType, metav1.ConditionTrue, migrationv1alpha1.ReasonCompleted,
"Image import not requested, using pre-configured templates")
return ctrl.Result{}, nil
}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/vsphere/tags_test.go (1)
427-451: 💤 Low valueConsider verifying all missing types in the error message.
The test creates a category missing three required associable types (
VirtualMachine,ResourcePool,StoragePod) but only asserts thatVirtualMachineappears in the error. Checking for all three missing types would provide more complete coverage of the error-formatting logic invalidateCategoryCompatibility.♻️ Optional enhancement
if !strings.Contains(err.Error(), virtualMachineType) { t.Fatalf("EnsureClusterTag error = %q, want %q in missing types", err.Error(), virtualMachineType) } + if !strings.Contains(err.Error(), resourcePoolType) { + t.Fatalf("EnsureClusterTag error = %q, want %q in missing types", err.Error(), resourcePoolType) + } + if !strings.Contains(err.Error(), storagePodType) { + t.Fatalf("EnsureClusterTag error = %q, want %q in missing types", err.Error(), storagePodType) + } }) }🤖 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/vsphere/tags_test.go` around lines 427 - 451, Update the test case in tags_test.go so it asserts that the error from EnsureClusterTag (which calls validateCategoryCompatibility) contains all three missing associable types (virtualMachineType, resourcePoolType, storagePodType) instead of only VirtualMachine; locate the failing test block using EnsureClusterTag, createTestCategory and validateCategoryCompatibility and add checks that the error string contains each of those three symbols to fully verify the error-formatting logic.
🤖 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 `@internal/vsphere/tags_test.go`:
- Around line 427-451: Update the test case in tags_test.go so it asserts that
the error from EnsureClusterTag (which calls validateCategoryCompatibility)
contains all three missing associable types (virtualMachineType,
resourcePoolType, storagePodType) instead of only VirtualMachine; locate the
failing test block using EnsureClusterTag, createTestCategory and
validateCategoryCompatibility and add checks that the error string contains each
of those three symbols to fully verify the error-formatting logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d6f63204-c74d-48b2-8fad-7921dc9a14b2
📒 Files selected for processing (4)
internal/controller/preflight.gointernal/vsphere/image.gointernal/vsphere/tags.gointernal/vsphere/tags_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/vsphere/tags.go
- internal/controller/preflight.go
- internal/vsphere/image.go
jcpowermac
force-pushed
the
rhcos-ova-import
branch
from
June 15, 2026 18:02
1b69e7c to
682644b
Compare
jcpowermac
force-pushed
the
rhcos-ova-import
branch
from
June 24, 2026 20:24
682644b to
c4e84d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
internal/controller/preflight.go (1)
346-359: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRedact OVA host/path, not only query parameters.
sanitizeOVAURLstill returnsscheme://host/path, so the reachability log can expose internal mirror hostnames and path components. Prefer a generic value such ashttps://<redacted>plus a query-redacted marker, or a stable non-reversible identifier.As per coding guidelines, “Flag logging that may expose passwords, tokens, API keys, PII (email, SSN, credit card), session IDs, internal hostnames, or customer data.”
Also applies to: 242-245
🤖 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/preflight.go` around lines 346 - 359, The sanitizeOVAURL helper currently redacts only query parameters but still leaks the URL host and path in reachability logs. Update sanitizeOVAURL to return a generic redacted value instead of scheme://host/path, while still preserving a query-redacted marker when RawQuery is present; use the sanitizeOVAURL symbol to adjust any callers in preflight reachability logging that rely on this output.Source: Coding guidelines
internal/vsphere/image.go (3)
153-168: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRedact download error URLs and strengthen the sanitizer.
Lines 163 and 168 still return
ovaURLdirectly, and the sanitizer keeps host/path. These errors can be surfaced outside logs, so use a sanitizer that omits internal hostnames and apply it to all URL-bearing errors.As per coding guidelines, “Flag logging that may expose passwords, tokens, API keys, PII (email, SSN, credit card), session IDs, internal hostnames, or customer data.”
Suggested fix
- return "", fmt.Errorf("downloading OVA from %s: %w", ovaURL, err) + return "", fmt.Errorf("downloading OVA from %s: %w", sanitizeOVAURL(ovaURL), err) @@ - return "", fmt.Errorf("downloading OVA from %s: HTTP %d %s", ovaURL, resp.StatusCode, resp.Status) + return "", fmt.Errorf("downloading OVA from %s: HTTP %d %s", sanitizeOVAURL(ovaURL), resp.StatusCode, resp.Status) @@ func sanitizeOVAURL(rawURL string) string { u, err := url.Parse(rawURL) - if err != nil { - return "<unparseable-url>" + if err != nil || u.Scheme == "" { + return "<redacted-ova-url>" } - sanitized := u.Scheme + "://" + u.Host + u.Path + sanitized := u.Scheme + "://<redacted>" if u.RawQuery != "" { sanitized += "?<redacted>" }Also applies to: 645-658
🤖 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/vsphere/image.go` around lines 153 - 168, The OVA download path still leaks the raw URL in returned errors, and sanitizeOVAURL is not redacting internal host/path details enough. Update the error handling in internal/vsphere/image.go around the download request and HTTP response checks to use a stricter sanitizer that removes internal hostnames and any sensitive URL components, then apply that sanitizer consistently anywhere ovaURL is included in returned errors in the download flow, including the related code referenced by the same issue.Source: Coding guidelines
367-384: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winInstall the NFC lease abort defer before
lease.Wait.If
lease.Waitfails, the function returns before the abort defer is registered, leaking the import lease. MoveleaseCompletedand the abort defer immediately afterImportVAppsucceeds.🤖 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/vsphere/image.go` around lines 367 - 384, Move the NFC lease cleanup setup in the vSphere import flow so the abort defer is registered immediately after ImportVApp succeeds, before calling lease.Wait. Update the image import logic in image.go around leaseCompleted and the deferred lease.Abort handling so any failure from lease.Wait still triggers cleanup, and keep the existing ImportVApp, lease.Wait, and lease.Abort symbols as the anchors for the change.
133-140: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle cleanup errors instead of discarding them.
The lock close/unlock and temp-file close/remove paths still discard errors. This also hides the expected double-close after Line 194. Track close/rename state and log cleanup failures.
As per path instructions, “Never ignore error returns.”
Also applies to: 177-180, 194-209
🤖 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/vsphere/image.go` around lines 133 - 140, The lock and temp-file cleanup in image handling is still ignoring errors, including the expected double-close after the rename path. Update the cleanup logic around the lock acquisition block, the temp-file close/remove flow, and the rename path in the image creation code to track whether the file has already been closed/renamed, and surface or log any close, unlock, or remove failures instead of discarding them. Use the relevant helpers and flow in internal/vsphere/image.go to ensure every cleanup error is handled explicitly and “Never ignore error returns.”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 `@internal/vsphere/image_test.go`:
- Around line 157-158: The test helpers in image_test.go are discarding I/O
errors when writing HTTP responses and when closing tar/file writers, which can
hide broken OVA fixtures. Update the affected helpers and test setup code to
check and handle the return values from response writes, tar.Writer.Close, and
file closes instead of ignoring them, especially in the blocks around the server
setup and the other listed call sites. Use the relevant helpers such as the
httptest.NewServer handler, tar.Writer, and any fixture-writing/closing logic so
the fix is applied consistently everywhere “never ignore error returns”
currently applies.
In `@internal/vsphere/image.go`:
- Around line 148-150: The cache reuse branch in the image download flow should
not return an existing OVA based only on the local filename when sha256Expected
is empty. Update the logic around the localPath/os.Stat check in the
download/cache path so unverified entries are not reused across different source
URLs, either by disabling cache reuse without a digest or by incorporating a URL
fingerprint into the cache key used by the OVA retrieval path.
- Around line 405-407: The secure boot handling in disableSecureBootIfNeeded is
only logging failures, which lets image import continue and produce an unusable
template. Update the import flow in image.go around disableSecureBootIfNeeded
and the surrounding VM/template setup to return a wrapped error when secure boot
cannot be disabled, and propagate that error so the import stops instead of
marking the VM as a template.
In `@internal/vsphere/tags_test.go`:
- Around line 349-372: Resolve the merge conflict in
internal/vsphere/tags_test.go by removing all conflict markers and keeping the
intended test cases intact, since the current HEAD/======= sections leave
invalid Go. Review the affected test functions around ObjectHasTagInCategory and
the neighboring test blocks to ensure the braces and t.Run structure remain
balanced after the conflict is resolved. Make sure the final file compiles
cleanly with no leftover marker text.
---
Duplicate comments:
In `@internal/controller/preflight.go`:
- Around line 346-359: The sanitizeOVAURL helper currently redacts only query
parameters but still leaks the URL host and path in reachability logs. Update
sanitizeOVAURL to return a generic redacted value instead of scheme://host/path,
while still preserving a query-redacted marker when RawQuery is present; use the
sanitizeOVAURL symbol to adjust any callers in preflight reachability logging
that rely on this output.
In `@internal/vsphere/image.go`:
- Around line 153-168: The OVA download path still leaks the raw URL in returned
errors, and sanitizeOVAURL is not redacting internal host/path details enough.
Update the error handling in internal/vsphere/image.go around the download
request and HTTP response checks to use a stricter sanitizer that removes
internal hostnames and any sensitive URL components, then apply that sanitizer
consistently anywhere ovaURL is included in returned errors in the download
flow, including the related code referenced by the same issue.
- Around line 367-384: Move the NFC lease cleanup setup in the vSphere import
flow so the abort defer is registered immediately after ImportVApp succeeds,
before calling lease.Wait. Update the image import logic in image.go around
leaseCompleted and the deferred lease.Abort handling so any failure from
lease.Wait still triggers cleanup, and keep the existing ImportVApp, lease.Wait,
and lease.Abort symbols as the anchors for the change.
- Around line 133-140: The lock and temp-file cleanup in image handling is still
ignoring errors, including the expected double-close after the rename path.
Update the cleanup logic around the lock acquisition block, the temp-file
close/remove flow, and the rename path in the image creation code to track
whether the file has already been closed/renamed, and surface or log any close,
unlock, or remove failures instead of discarding them. Use the relevant helpers
and flow in internal/vsphere/image.go to ensure every cleanup error is handled
explicitly and “Never ignore error returns.”
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2a70003a-bfb8-4fa6-8243-bd92cd85ccab
⛔ Files ignored due to path filters (8)
api/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*go.sumis excluded by!**/*.sumvendor/github.com/coreos/stream-metadata-go/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/stream-metadata-go/stream/artifact_utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/stream-metadata-go/stream/rhcos/rhcos.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/stream-metadata-go/stream/stream.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/stream-metadata-go/stream/stream_utils.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (15)
api/v1alpha1/vmwarecloudfoundationmigration_types.goconfig/crd/bases/migration.openshift.io_vmwarecloudfoundationmigrations.yamlconfig/manager/manager.yamlconsole-plugin/web/src/app/pages/MigrationDetailPage.tsxgo.modinternal/controller/image_import_test.gointernal/controller/preflight.gointernal/controller/vmwarecloudfoundationmigration_controller.gointernal/openshift/infrastructure.gointernal/openshift/infrastructure_test.gointernal/openshift/version.gointernal/vsphere/image.gointernal/vsphere/image_test.gointernal/vsphere/tags.gointernal/vsphere/tags_test.go
💤 Files with no reviewable changes (1)
- internal/openshift/infrastructure.go
✅ Files skipped from review due to trivial changes (2)
- internal/openshift/version.go
- internal/openshift/infrastructure_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- go.mod
- console-plugin/web/src/app/pages/MigrationDetailPage.tsx
- config/manager/manager.yaml
- config/crd/bases/migration.openshift.io_vmwarecloudfoundationmigrations.yaml
- internal/controller/image_import_test.go
- api/v1alpha1/vmwarecloudfoundationmigration_types.go
- internal/vsphere/tags.go
- internal/controller/vmwarecloudfoundationmigration_controller.go
Contributor
|
/retest |
openshift-ci
Bot
added
the
needs-rebase
Contributor
|
/retest |
Implement opt-in RHCOS OVA resolution, download, and import into destination vCenter as VM templates. When spec.image is set, the operator reads the coreos-bootimages ConfigMap (CVO-delivered), downloads the OVA with SHA256 verification, imports it via NFC lease, and populates topology.template automatically per failure domain. When spec.image is omitted, behavior is unchanged. Key changes: - Add ImageSpec/ImageStatus types and ConditionDestinationImageImported - Add ensureDestinationImageImported handler with 5-phase workflow (resolve, download, import per-FD, populate topology.template) - Add EnsureClusterTag matching installer's openshift-<infraID> pattern - Add OVA import adapted from MCO's vsphere_helpers.go using govmomi - Update preflight to skip template checks for auto-import FDs and validate VApp.Import privileges - Add scratch volume (2Gi emptyDir) and raise memory limit to 256Mi - Update console plugin with new condition - Add stream-metadata-go dependency for RHCOS stream metadata parsing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Guard against nil ConfigClient in ensureDestinationImageImported to prevent panics when the reconciler is constructed without it (e.g. in unit tests). Return a descriptive error instead. - Fix Phase 5 completion message counting: track pre-existing templates before populating topology.template so "imported vs pre-existing" counts are accurate. - Add TestImportOVA with a synthetic OVA (valid OVF + dummy VMDK) using the govmomi simulator to verify the full import-and-mark-as-template flow including host selection. - Add TestFindAvailableHost for host selection logic. - Extend preflight to auto-resolve and HEAD-check OVA URL from the coreos-bootimages ConfigMap when spec.image is set without ovaUrl. - Add 15-minute context timeout for OVA downloads to prevent indefinite reconcile blocking on slow networks. - Fix controller tests to expect ConfigClient errors instead of infrastructure errors after the nil guard. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cleanup, and tag category validation Sanitize OVA URLs before logging to prevent signed tokens in query strings from being written to operator logs. Add validateImageImportPrivileges support for per-failure-domain resource pools so privilege checks match the actual resource pool used during import. Add NFC lease abort on error paths after ImportVApp to prevent dangling leases. Validate AssociableTypes in ensureTagCategoryWithTypes when reusing existing categories, and add test coverage for cluster tag category compatibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ering, and test helpers Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jcpowermac
force-pushed
the
rhcos-ova-import
branch
from
July 28, 2026 19:29
03c3964 to
7df3e9e
Compare
openshift-ci
Bot
removed
the
needs-rebase
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jcpowermac
force-pushed
the
rhcos-ova-import
branch
from
July 29, 2026 11:48
7df3e9e to
b2e7ee8
Compare
|
@jcpowermac: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implement opt-in RHCOS OVA resolution, download, and import into
destination vCenter as VM templates. When spec.image is set, the
operator reads the coreos-bootimages ConfigMap (CVO-delivered),
downloads the OVA with SHA256 verification, imports it via NFC lease,
and populates topology.template automatically per failure domain.
When spec.image is omitted, behavior is unchanged.
Key changes:
(resolve, download, import per-FD, populate topology.template)
validate VApp.Import privileges
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Summary by CodeRabbit
spec.image/status.image), including disk provisioning, resolved URL/digest, download completion, and per-domain imported templates.DestinationImageImported), surfaced in the migration details UI.