Add UPI (User Provided Infrastructure) provider#218
Merged
Conversation
|
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:
WalkthroughAdds a UPI (User Provided Infrastructure) provider and wires it end-to-end: provider implementation and instance creation, manager registration, CLI command, Pulumi workspace/secrets adjustments, SSH/cluster setup updates, and README documentation. ChangesUPI Provider Addition
Sequence DiagramsequenceDiagram
participant CLI
participant Manager
participant Pulumi
participant VM
CLI->>Manager: crc-cloud create --provider upi (host-ip, ssh-key-path, secrets)
Manager->>Pulumi: start UPI CreateRunFunc with args
Pulumi->>VM: SSH (swap keys), upload & run cluster_setup.sh
VM-->>Pulumi: kubeconfig and credentials
Pulumi-->>Manager: exported outputs (kubeconfig, keys, host, user, password)
Manager-->>CLI: return outputs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/bundle/setup/clustersetup.go (1)
90-156:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHardcoded
/var/home/core/paths defeat configurable username inSetup.
Setupnow derivesuserfromdata.Usernamefor the SSH connection, but the script upload target (Line 113),chmodcommand (Line 119), and exec path (Line 156) remain pinned to/var/home/core/cluster_setup.sh. For any UPI VM whoseUsernameis notcore, the copy will land in another user's home (or fail on permissions) and the subsequent commands will run against the wrong path. Either keep the path under/tmp/(writable for any user) or template it usinguser.🔧 Suggested fix (excerpt)
- clusterSetupRemoteCopyResource, err := + remoteScriptPath := fmt.Sprintf("/var/home/%s/cluster_setup.sh", user) + clusterSetupRemoteCopyResource, err := remote.NewCopyFile(ctx, "uploadClusterSetupScript", &remote.CopyFileArgs{ Connection: c, LocalPath: pulumi.String(clusterSetupfileName), - RemotePath: pulumi.String("/var/home/core/cluster_setup.sh"), + RemotePath: pulumi.String(remoteScriptPath), }, pulumi.IgnoreChanges([]string{"localPath"})) ... - scriptXRightsCommand := "chmod +x /var/home/core/cluster_setup.sh" + scriptXRightsCommand := fmt.Sprintf("chmod +x %s", remoteScriptPath) ... - return fmt.Sprintf("%s %s", - execClusterSetupCommand, - "/var/home/core/cluster_setup.sh") + return fmt.Sprintf("%s %s", execClusterSetupCommand, remoteScriptPath)🤖 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/bundle/setup/clustersetup.go` around lines 90 - 156, The code hardcodes "/var/home/core/cluster_setup.sh" for uploading, chmod and execution which breaks when SSH user (variable user) from Setup (derived from data.Username) is not "core"; update the remote.NewCopyFile LocalPath/RemotePath, the chmod command passed to remote.NewCommand (scriptXRightsCommand / scriptXRightsCommandResource), and the exec string built in execClusterSetupCommand to construct the remote path dynamically (or use a universal writable path like /tmp/cluster_setup.sh) using the computed user variable (and/or use the same computed remotePath variable in all three places) so upload, permission change and execution all reference the same templated path; touch symbols to change: user, remote.NewCopyFile("uploadClusterSetupScript"), remote.NewCommand("setXRightsForClusterSetupScript"), and the execClusterSetupCommand builder.pkg/bundle/setup/clustersetup.sh (1)
93-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
replace_default_pubkeystill reads from/home/core/id_rsa.pub, conflicting with the new configurable username.
SwapKeysnow uploads the new public key to/home/<user>/id_rsa.pub(clustersetup.go Line 66), but this function (untouched in the diff) still hardcodes/home/core/id_rsa.pub. For any UPI deployment where--ssh-usernameis notcore,replace_default_pubkeywill fail because the file does not exist at that path. Either parameterize this path (e.g., via an env var injected bySetup) or document that the SSH user must remaincore.🤖 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/bundle/setup/clustersetup.sh` around lines 93 - 99, The replace_default_pubkey function still hardcodes /home/core/id_rsa.pub which breaks deployments when --ssh-username is not core; update replace_default_pubkey to read the SSH username from the same source SwapKeys uses (the configurable SSH user set by Setup / clustersetup.go) — e.g., use an env var like SSH_USERNAME (default to "core" if unset) and build the path as /home/${SSH_USERNAME}/id_rsa.pub before reading it, then continue with wait_for_resource machineconfig and the oc patch call; ensure the variable name matches what's injected by Setup/SwapKeys so non-core users work.
🧹 Nitpick comments (3)
pkg/provider/upi/upi.go (1)
19-22: ⚡ Quick winReturn an explicit error instead of
(nil, nil)for unsupported image import.Returning
nil, nilsilently means a caller ofmanager.Importwith the UPI provider gets anilDeployFuncplugged intoproviderAPI.Stack, which will fail deep inside Pulumi with a confusing error (or panic) rather than at the API boundary. Since the README documents that import is unsupported for UPI, surface that contract here.♻️ Suggested change
-func (a *Provider) ImportImageRunFunc(_, _, _ string) (pulumi.RunFunc, error) { - // UPI provider doesn't import images - return nil, nil -} +func (a *Provider) ImportImageRunFunc(_, _, _ string) (pulumi.RunFunc, error) { + // UPI provider operates on an existing VM and does not import images. + return nil, fmt.Errorf("image import is not supported by the UPI provider") +}🤖 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/provider/upi/upi.go` around lines 19 - 22, The ImportImageRunFunc currently returns (nil, nil) which hides the unsupported-import contract; update Provider.ImportImageRunFunc to return a non-nil error indicating import is unsupported for the UPI provider (e.g., "image import not supported for UPI") instead of nil, so callers of manager.Import get a clear error at the API boundary; modify the function handling in Provider.ImportImageRunFunc to construct and return that explicit error while keeping the DeployFunc return nil.pkg/manager/manager.go (1)
35-39: 💤 Low valueExtract duplicated nil-safe plugin extraction into a helper.
The same 5-line
plugin := p.GetPlugin(); var pluginInfo providerAPI.PluginInfo; if plugin != nil { pluginInfo = *plugin }block is repeated verbatim inImport,Create, andDestroy. Consider a small helper to keep these call sites DRY and ensure future changes stay in sync.♻️ Suggested helper
func pluginInfoOf(p providerAPI.Provider) providerAPI.PluginInfo { if plugin := p.GetPlugin(); plugin != nil { return *plugin } return providerAPI.PluginInfo{} }Then each call site becomes
Plugin: pluginInfoOf(p).Also applies to: 98-102, 122-126
🤖 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/manager/manager.go` around lines 35 - 39, Extract the repeated nil-safe plugin extraction into a small helper function (e.g., pluginInfoOf) that takes a providerAPI.Provider, calls GetPlugin(), returns the dereferenced providerAPI.PluginInfo when non-nil and the zero value otherwise; replace the five-line pattern in Import, Create, and Destroy with Plugin: pluginInfoOf(p) (or equivalent) to make the call sites DRY and to centralize future changes to plugin extraction.pkg/provider/upi/create-instance.go (1)
79-94: 💤 Low valueDuplicate username defaulting.
The username default is already applied inside
setup.SwapKeysandsetup.Setup(both fall back tobundle.ImageUsernamewhen the field/arg is empty). Computingusernamehere just to populateOutputUsernameis fine, but consider passing the resolvedusernametoSetup(instead of the possibly-emptyr.sshUsername) so the exported username and the SSH user always agree even if defaulting logic changes in one place.🔧 Suggested change
- // Determine the username to use (default to "core" if not specified) - username := r.sshUsername - if username == "" { - username = bundle.ImageUsername - } - - // Run the cluster setup with the new key + // Determine the username to use (default to "core" if not specified) + username := r.sshUsername + if username == "" { + username = bundle.ImageUsername + } + + // Run the cluster setup with the new key kubeconfig, _, err := setup.Setup(ctx, &publicIP, &privateKey.PrivateKeyOpenssh, setup.Data{ PrivateIP: &internalIP, PublicIP: &publicIP, Password: &password.Result, OCPPullSecretFilePath: r.ocpPullSecretFilePath, - Username: r.sshUsername, + Username: username, })🤖 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/provider/upi/create-instance.go` around lines 79 - 94, You compute a local resolved username but still pass r.sshUsername into setup.Setup, causing potential mismatch if defaults change; update the call to setup.Setup to pass the resolved username variable (the local username that falls back to bundle.ImageUsername) into the setup.Data.Username field so setup.Setup and your exported OutputUsername use the same resolved value (reference: the local username variable, r.sshUsername, bundle.ImageUsername, and setup.Setup/setup.Data.Username).
🤖 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 `@cmd/cmd/create/upi.go`:
- Line 34: Remove the leftover debug print statement fmt.Printf("providerParams:
%v", providerParams); either delete it entirely or replace it with the project's
logger at debug level (e.g., logger.Debugf("providerParams: %v",
providerParams)) and ensure sensitive fields (like --ssh-key-path) are redacted
before logging; locate the print by searching for fmt.Printf and the
providerParams variable in the upi creation code and apply the fix there.
In `@pkg/bundle/setup/clustersetup.sh`:
- Around line 231-236: The podman run commands pass unquoted password variables
($PASS_DEVELOPER, $PASS_KUBEADMIN, $PASS_REDHAT) which can break on special
characters; update the three podman run invocations in this script to quote
those expansions (e.g., "$PASS_DEVELOPER", "$PASS_KUBEADMIN", "$PASS_REDHAT") so
the shell treats each password as a single argument and avoids word-splitting or
globbing, leaving the stop_if_failed calls unchanged.
In `@pkg/manager/util.go`:
- Around line 78-82: The current code sets PULUMI_CONFIG_PASSPHRASE to a fixed
string in the auto.EnvVars block alongside auto.SecretsProvider("passphrase"),
which embeds a global secret into every build; replace this by sourcing the
passphrase dynamically: read it from an environment variable or a CLI flag
(preferably both), and only fall back to prompting the user or generating a
per-stack random passphrase that is persisted out-of-band; update the code that
constructs the auto.EnvVars map to use the runtime value for
"PULUMI_CONFIG_PASSPHRASE" (or omit it entirely when absent), add a CLI/flag
parsing hook or env lookup before creating auto.EnvVars, and ensure any
generated passphrase is documented and stored per-stack rather than hardcoded.
In `@pkg/provider/upi/create-instance.go`:
- Around line 25-37: Tighten validation for args by rejecting empty values and
surfacing all os.Stat errors: after extracting hostIPValue and sshKeyPathValue
from args, check they are non-empty and return an error if empty (handle
explicit "--host-ip=\"\""/"--ssh-key-path=\"\""); when calling
os.Stat(sshKeyPathValue) do not only check os.IsNotExist(err) — if err != nil
return fmt.Errorf("stat ssh key %s: %w", sshKeyPathValue, err) so permission or
I/O errors are returned; keep the current not-found message only when
os.IsNotExist(err) if you want a custom message but still return other errors.
- Around line 71-83: Document that setup.SwapKeys mutates the VM's
/home/<username>/.ssh/authorized_keys (it connects as the provided
--ssh-username and appends a new key) and then either (A) update the README for
UPI to explicitly require --ssh-username be "core" until clustersetup.sh is
changed, or (B) parameterize pkg/bundle/setup/clustersetup.sh so it uses the
actual SSH username passed to create-instance (make clustersetup.sh read a
USERNAME variable or substitute the username at render time) and adjust
create-instance.go comments near the setup.SwapKeys call to mention this
behavior.
---
Outside diff comments:
In `@pkg/bundle/setup/clustersetup.go`:
- Around line 90-156: The code hardcodes "/var/home/core/cluster_setup.sh" for
uploading, chmod and execution which breaks when SSH user (variable user) from
Setup (derived from data.Username) is not "core"; update the remote.NewCopyFile
LocalPath/RemotePath, the chmod command passed to remote.NewCommand
(scriptXRightsCommand / scriptXRightsCommandResource), and the exec string built
in execClusterSetupCommand to construct the remote path dynamically (or use a
universal writable path like /tmp/cluster_setup.sh) using the computed user
variable (and/or use the same computed remotePath variable in all three places)
so upload, permission change and execution all reference the same templated
path; touch symbols to change: user,
remote.NewCopyFile("uploadClusterSetupScript"),
remote.NewCommand("setXRightsForClusterSetupScript"), and the
execClusterSetupCommand builder.
In `@pkg/bundle/setup/clustersetup.sh`:
- Around line 93-99: The replace_default_pubkey function still hardcodes
/home/core/id_rsa.pub which breaks deployments when --ssh-username is not core;
update replace_default_pubkey to read the SSH username from the same source
SwapKeys uses (the configurable SSH user set by Setup / clustersetup.go) — e.g.,
use an env var like SSH_USERNAME (default to "core" if unset) and build the path
as /home/${SSH_USERNAME}/id_rsa.pub before reading it, then continue with
wait_for_resource machineconfig and the oc patch call; ensure the variable name
matches what's injected by Setup/SwapKeys so non-core users work.
---
Nitpick comments:
In `@pkg/manager/manager.go`:
- Around line 35-39: Extract the repeated nil-safe plugin extraction into a
small helper function (e.g., pluginInfoOf) that takes a providerAPI.Provider,
calls GetPlugin(), returns the dereferenced providerAPI.PluginInfo when non-nil
and the zero value otherwise; replace the five-line pattern in Import, Create,
and Destroy with Plugin: pluginInfoOf(p) (or equivalent) to make the call sites
DRY and to centralize future changes to plugin extraction.
In `@pkg/provider/upi/create-instance.go`:
- Around line 79-94: You compute a local resolved username but still pass
r.sshUsername into setup.Setup, causing potential mismatch if defaults change;
update the call to setup.Setup to pass the resolved username variable (the local
username that falls back to bundle.ImageUsername) into the setup.Data.Username
field so setup.Setup and your exported OutputUsername use the same resolved
value (reference: the local username variable, r.sshUsername,
bundle.ImageUsername, and setup.Setup/setup.Data.Username).
In `@pkg/provider/upi/upi.go`:
- Around line 19-22: The ImportImageRunFunc currently returns (nil, nil) which
hides the unsupported-import contract; update Provider.ImportImageRunFunc to
return a non-nil error indicating import is unsupported for the UPI provider
(e.g., "image import not supported for UPI") instead of nil, so callers of
manager.Import get a clear error at the API boundary; modify the function
handling in Provider.ImportImageRunFunc to construct and return that explicit
error while keeping the DeployFunc return nil.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ba39ef1-2fc9-446b-be3e-90b1e0c89222
📒 Files selected for processing (14)
README.mdcmd/cmd/create/create.gocmd/cmd/create/upi.gopkg/bundle/setup/clustersetup.gopkg/bundle/setup/clustersetup.shpkg/manager/manager.gopkg/manager/providers.gopkg/manager/util.gopkg/provider/aws/create-instance.gopkg/provider/gcp/create-instance.gopkg/provider/openstack/create-instance.gopkg/provider/upi/constants.gopkg/provider/upi/create-instance.gopkg/provider/upi/upi.go
morenod
force-pushed
the
upi_provider
branch
2 times, most recently
from
c1840f3 to
3d8c99c
Compare
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/bundle/setup/clustersetup.go`:
- Line 59: The command hardcodes /home/<user>; replace that with the actual home
directory for bundle.ImageUsername by performing a user lookup
(os/user.Lookup(bundle.ImageUsername)) and using the returned User.HomeDir to
build the authorized_keys path (path/filepath.Join(u.HomeDir, ".ssh",
"authorized_keys")), then format overrideKeyCommand with that path; also handle
the lookup error (and fall back or return it) before creating overrideKeyCommand
so it won’t break for users with non-/home homes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7aa98543-eabf-4bbd-9c17-5a00cd7587d5
📒 Files selected for processing (11)
README.mdcmd/cmd/create/create.gocmd/cmd/create/upi.gopkg/bundle/setup/clustersetup.gopkg/bundle/setup/clustersetup.shpkg/manager/manager.gopkg/manager/providers.gopkg/manager/util.gopkg/provider/upi/constants.gopkg/provider/upi/create-instance.gopkg/provider/upi/upi.go
✅ Files skipped from review due to trivial changes (2)
- pkg/provider/upi/constants.go
- README.md
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
pkg/bundle/setup/clustersetup.go (1)
59-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid hardcoding
/home/<user>for key placement.Line 59 still assumes the SSH user home is
/home/<username>. This breaks for accounts with different home directory structures (e.g.,/rootfor root, custom home paths in non-standard configurations) and can block UPI setup.🔧 Suggested fix
-overrideKeyCommand := fmt.Sprintf("cat /home/%[1]s/id_rsa.pub >> /home/%[1]s/.ssh/authorized_keys", bundle.ImageUsername) +overrideKeyCommand := "cat ~/id_rsa.pub >> ~/.ssh/authorized_keys"🤖 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/bundle/setup/clustersetup.go` at line 59, The code hardcodes "/home/<user>" when building overrideKeyCommand (overrideKeyCommand := fmt.Sprintf(... bundle.ImageUsername)), which fails for users whose home isn't under /home; change it to resolve the user's actual home directory (use os/user.Lookup(bundle.ImageUsername) to get usr.HomeDir, with a safe fallback if lookup fails), then build the path with filepath.Join(homeDir, ".ssh", "authorized_keys") and use that path in the command instead of "/home/%[1]s/...". Ensure you reference overrideKeyCommand and bundle.ImageUsername when making the change and handle lookup errors gracefully.
🤖 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 `@cmd/cmd/create/upi.go`:
- Around line 43-45: The handler inside the Cobra command's RunE (in upi.go)
calls os.Exit(1) after printing an error for manager.UPI, which prevents normal
error propagation; instead remove os.Exit(1) and return an error from RunE
(e.g., wrap the existing message and underlying err with fmt.Errorf or
errors.Wrap) so Cobra/test callers can handle it; update the RunE return path in
the upi.go command (and mirror this pattern in other provider handlers) to
return the constructed error rather than exiting the process.
In `@pkg/bundle/setup/clustersetup.go`:
- Line 95: The remoteScriptPath in clustersetup.go is using
"/var/home/%s/cluster_setup.sh" which mismatches other SSH-related paths that
use "/home/<user>/..." (see usage of id_rsa.pub and .ssh/authorized_keys and the
clustersetup.sh script expecting /home/core/id_rsa.pub); change the
remoteScriptPath construction (remoteScriptPath := fmt.Sprintf(...)) to use
"/home/%s/cluster_setup.sh" with bundle.ImageUsername so the upload target
matches the rest of the code, and update any related comments or tests to
reflect the /home path (or alternatively add a clear comment explaining why
/var/home is intentionally required if that was the original intent).
In `@pkg/provider/upi/create-instance.go`:
- Around line 22-46: fillCreateRequest currently only reads host-ip and
ssh-key-path; to forward --ssh-username end-to-end add a new sshUsername field
to the createRequest type and populate it from args (key "ssh-username") inside
fillCreateRequest (alongside hostIP and sshKeyPath). Validate that the value
exists (similar pattern used for hostIP/sshKeyPath) and set
createRequest.sshUsername = that value before returning. Also ensure downstream
code that constructs providerAPI.OutputUsername / bundle.ImageUsername reads
from the createRequest.sshUsername field so the CLI --ssh-username is honored
end-to-end.
- Around line 24-27: The current check only ensures hostIPValue is non-empty;
instead validate it is a syntactically valid IP before proceeding: after
retrieving hostIPValue from args (hostIP, hostIPValue), call
net.ParseIP(hostIPValue) and if it returns nil return a clear error (e.g.,
fmt.Errorf("invalid host-ip: %q", hostIPValue)); update the error message path
in the same block in create-instance.go so downstream remote operations fail
fast with a helpful message.
---
Duplicate comments:
In `@pkg/bundle/setup/clustersetup.go`:
- Line 59: The code hardcodes "/home/<user>" when building overrideKeyCommand
(overrideKeyCommand := fmt.Sprintf(... bundle.ImageUsername)), which fails for
users whose home isn't under /home; change it to resolve the user's actual home
directory (use os/user.Lookup(bundle.ImageUsername) to get usr.HomeDir, with a
safe fallback if lookup fails), then build the path with filepath.Join(homeDir,
".ssh", "authorized_keys") and use that path in the command instead of
"/home/%[1]s/...". Ensure you reference overrideKeyCommand and
bundle.ImageUsername when making the change and handle lookup errors gracefully.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31dbe9c9-643a-4f61-91ae-debcdd0e8a07
📒 Files selected for processing (11)
README.mdcmd/cmd/create/create.gocmd/cmd/create/upi.gopkg/bundle/setup/clustersetup.gopkg/bundle/setup/clustersetup.shpkg/manager/manager.gopkg/manager/providers.gopkg/manager/util.gopkg/provider/upi/constants.gopkg/provider/upi/create-instance.gopkg/provider/upi/upi.go
✅ Files skipped from review due to trivial changes (1)
- README.md
Comment on lines
+22
to
+46
| func fillCreateRequest(projectName, bootingPrivateKeyFilePath, ocpPullSecretFilePath string, | ||
| args map[string]string) (*createRequest, error) { | ||
| hostIPValue, ok := args[hostIP] | ||
| if !ok || hostIPValue == "" { | ||
| return nil, fmt.Errorf("host-ip not found") | ||
| } | ||
| sshKeyPathValue, ok := args[sshKeyPath] | ||
| if !ok || sshKeyPathValue == "" { | ||
| return nil, fmt.Errorf("ssh-key-path not found") | ||
| } | ||
|
|
||
| // Verify SSH key file exists and is accessible | ||
| if _, err := os.Stat(sshKeyPathValue); err != nil { | ||
| if os.IsNotExist(err) { | ||
| return nil, fmt.Errorf("SSH key file not found at path: %s", sshKeyPathValue) | ||
| } | ||
| return nil, fmt.Errorf("unable to access SSH key at %s: %w", sshKeyPathValue, err) | ||
| } | ||
|
|
||
| return &createRequest{ | ||
| projectName: projectName, | ||
| hostIP: hostIPValue, | ||
| sshKeyPath: sshKeyPathValue, | ||
| ocpPullSecretFilePath: ocpPullSecretFilePath, | ||
| }, nil |
There was a problem hiding this comment.
❓ Verification inconclusive
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify UPI params and whether ssh username is consumed in create-instance flow
rg -n "ssh-username|sshUsername|host-ip|ssh-key-path" pkg/provider/upi -S
# Verify current setup function signatures and expected args
rg -n "func SwapKeys\\(|func Setup\\(" pkg/bundle/setup/clustersetup.go -S
# Verify call sites from UPI create path
rg -n "SwapKeys\\(|setup\\.Setup\\(|OutputUsername" pkg/provider/upi/create-instance.go -SRepository: crc-org/crc-cloud
Length of output: 646
Forward --ssh-username through the UPI create flow.
Current UPI create code only consumes host-ip and ssh-key-path, and it exports providerAPI.OutputUsername as bundle.ImageUsername, so the --ssh-username input won’t be honored end-to-end.
🤖 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/provider/upi/create-instance.go` around lines 22 - 46, fillCreateRequest
currently only reads host-ip and ssh-key-path; to forward --ssh-username
end-to-end add a new sshUsername field to the createRequest type and populate it
from args (key "ssh-username") inside fillCreateRequest (alongside hostIP and
sshKeyPath). Validate that the value exists (similar pattern used for
hostIP/sshKeyPath) and set createRequest.sshUsername = that value before
returning. Also ensure downstream code that constructs
providerAPI.OutputUsername / bundle.ImageUsername reads from the
createRequest.sshUsername field so the CLI --ssh-username is honored end-to-end.
28c62f2 to
503632d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/manager/util.go (1)
69-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the shared fallback passphrase.
Using a fixed default (
"crc-cloud-default-passphrase") means stacks can still be decrypted with a known key whenPULUMI_CONFIG_PASSPHRASEis unset. Fail fast instead of silently defaulting.🔒 Minimal fix
passphrase := os.Getenv("PULUMI_CONFIG_PASSPHRASE") if passphrase == "" { - passphrase = "crc-cloud-default-passphrase" + logging.Errorf("PULUMI_CONFIG_PASSPHRASE must be set") + os.Exit(1) }Also applies to: 86-89
🤖 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/manager/util.go` around lines 69 - 75, Remove the hardcoded fallback passphrase by failing fast when PULUMI_CONFIG_PASSPHRASE is unset: replace the current logic that sets passphrase := os.Getenv("PULUMI_CONFIG_PASSPHRASE") and then defaults to "crc-cloud-default-passphrase" with code that reads the env var into passphrase and returns/propagates an error (or panics) if passphrase == "" so decryption cannot proceed without an explicit secret; apply the same change to the other occurrence around the passphrase usage at the later block (lines referenced in the review).pkg/bundle/setup/clustersetup.go (1)
95-122:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse
remoteScriptPathconsistently for chmod and execution.Line 107 and Line 145 still point to
/var/home/core/cluster_setup.shwhile upload uses/home/<user>/cluster_setup.sh. This can cause “file not found” and fail setup.🛠️ Suggested fix
- scriptXRightsCommand := "chmod +x /var/home/core/cluster_setup.sh" + scriptXRightsCommand := fmt.Sprintf("chmod +x %s", remoteScriptPath) ... return fmt.Sprintf("%s %s", execClusterSetupCommand, - "/var/home/core/cluster_setup.sh") + args[3].(string))Also applies to: 107-107, 143-145
🤖 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/bundle/setup/clustersetup.go` around lines 95 - 122, The chmod and execution commands are using a hardcoded /var/home/core/cluster_setup.sh while the upload uses remoteScriptPath ("/home/<user>/cluster_setup.sh"); update the remote.NewCommand that creates scriptXRightsCommand (resource "setXRightsForClusterSetupScript") to use remoteScriptPath instead of the hardcoded path, and ensure the execClusterSetupCommand.ApplyT uses the same remoteScriptPath variable for the execution command so upload, chmod (scriptXRightsCommand) and execution all reference the same remoteScriptPath; verify changes around scriptXRightsCommand, setXRightsForClusterSetupScript, remote.NewCommand and execClusterSetupCommand.ApplyT to keep the pulumi.DependsOn on clusterSetupRemoteCopyResource.
🤖 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/bundle/setup/clustersetup.sh`:
- Around line 231-235: The podman runs inside set_credentials() are using -ti
which allocates a TTY and can fail in non-interactive CI/automation; modify the
three podman run invocations that generate htpasswd (the lines invoking
ghcr.io/crc-org/crc-cloud/httpd-tools:latest for developer, kubeadmin and
redhat) to remove the -ti (use non-interactive flags only, e.g., --rm) so output
redirection to htpasswd.developer, htpasswd.kubeadmin and htpasswd.redhat works
reliably in non-interactive flows.
In `@pkg/provider/upi/upi.go`:
- Around line 26-35: CreateParams and CreateParamsMandatory currently only
expose hostIP and sshKeyPath so ssh-username is never surfaced; update
Provider.CreateParams to include the ssh-username key and description (alongside
hostIP and sshKeyPath) and update Provider.CreateParamsMandatory if you want it
mandatory (or leave out to make optional). In
pkg/provider/upi/create-instance.go, read the ssh-username param (use "core" as
the default when empty), use that value in the SSH/setup calls, and ensure the
value is included in any setup/output export so downstream code receives the
configured username.
---
Duplicate comments:
In `@pkg/bundle/setup/clustersetup.go`:
- Around line 95-122: The chmod and execution commands are using a hardcoded
/var/home/core/cluster_setup.sh while the upload uses remoteScriptPath
("/home/<user>/cluster_setup.sh"); update the remote.NewCommand that creates
scriptXRightsCommand (resource "setXRightsForClusterSetupScript") to use
remoteScriptPath instead of the hardcoded path, and ensure the
execClusterSetupCommand.ApplyT uses the same remoteScriptPath variable for the
execution command so upload, chmod (scriptXRightsCommand) and execution all
reference the same remoteScriptPath; verify changes around scriptXRightsCommand,
setXRightsForClusterSetupScript, remote.NewCommand and
execClusterSetupCommand.ApplyT to keep the pulumi.DependsOn on
clusterSetupRemoteCopyResource.
In `@pkg/manager/util.go`:
- Around line 69-75: Remove the hardcoded fallback passphrase by failing fast
when PULUMI_CONFIG_PASSPHRASE is unset: replace the current logic that sets
passphrase := os.Getenv("PULUMI_CONFIG_PASSPHRASE") and then defaults to
"crc-cloud-default-passphrase" with code that reads the env var into passphrase
and returns/propagates an error (or panics) if passphrase == "" so decryption
cannot proceed without an explicit secret; apply the same change to the other
occurrence around the passphrase usage at the later block (lines referenced in
the review).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cbf6eea6-22be-4c0e-8d9d-111043e5dc33
📒 Files selected for processing (11)
README.mdcmd/cmd/create/create.gocmd/cmd/create/upi.gopkg/bundle/setup/clustersetup.gopkg/bundle/setup/clustersetup.shpkg/manager/manager.gopkg/manager/providers.gopkg/manager/util.gopkg/provider/upi/constants.gopkg/provider/upi/create-instance.gopkg/provider/upi/upi.go
✅ Files skipped from review due to trivial changes (2)
- pkg/provider/upi/constants.go
- README.md
Comment on lines
+26
to
+35
| func (a *Provider) CreateParams() map[string]string { | ||
| return map[string]string{ | ||
| hostIP: hostIPDesc, | ||
| sshKeyPath: sshKeyPathDesc, | ||
| } | ||
| } | ||
|
|
||
| func (a *Provider) CreateParamsMandatory() []string { | ||
| return []string{hostIP, sshKeyPath} | ||
| } |
There was a problem hiding this comment.
ssh-username is not exposed in provider params, so the feature cannot be configured end-to-end.
CreateParams()/CreateParamsMandatory() currently only surface host-ip and ssh-key-path (Line 26–35), which prevents the CLI from discovering and passing ssh-username as intended.
🔧 Suggested fix
func (a *Provider) CreateParams() map[string]string {
return map[string]string{
hostIP: hostIPDesc,
sshKeyPath: sshKeyPathDesc,
+ sshUsername: sshUsernameDesc,
}
}
func (a *Provider) CreateParamsMandatory() []string {
return []string{hostIP, sshKeyPath}
}And in pkg/provider/upi/create-instance.go, consume ssh-username with default "core" when empty, then pass/export that value through setup/output.
Also applies to: 37-44
🤖 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/provider/upi/upi.go` around lines 26 - 35, CreateParams and
CreateParamsMandatory currently only expose hostIP and sshKeyPath so
ssh-username is never surfaced; update Provider.CreateParams to include the
ssh-username key and description (alongside hostIP and sshKeyPath) and update
Provider.CreateParamsMandatory if you want it mandatory (or leave out to make
optional). In pkg/provider/upi/create-instance.go, read the ssh-username param
(use "core" as the default when empty), use that value in the SSH/setup calls,
and ensure the value is included in any setup/output export so downstream code
receives the configured username.
Contributor
Author
There was a problem hiding this comment.
ssh-username is hardcoded to "core" everywhere, setting as parameter could be confused, as it cannot be changed without updating every reference to core in the rest of files
There was a problem hiding this comment.
@morenod, understood — since "core" is embedded in multiple other files throughout the setup flow, surfacing ssh-username as a configurable parameter without a broader refactor would be misleading. The current hardcoded approach is the correct call for this PR's scope.
✏️ Learnings added
Learnt from: morenod
Repo: crc-org/crc-cloud PR: 218
File: pkg/provider/upi/upi.go:26-35
Timestamp: 2026-05-26T10:25:37.766Z
Learning: In `pkg/provider/upi/upi.go` (crc-org/crc-cloud), the SSH username `"core"` is intentionally hardcoded and not exposed as a provider parameter (`ssh-username` is absent from `CreateParams()`/`CreateParamsMandatory()`). This is by design: the username is referenced in many other files across the codebase, so making it configurable here without a broader refactor would be misleading and incomplete.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
morenod
commented
May 26, 2026
| return nil, err | ||
| } | ||
| overrideKeyCommand := "cat /home/core/id_rsa.pub >> /home/core/.ssh/authorized_keys" | ||
| overrideKeyCommand := "cat /var/home/core/id_rsa.pub >> /var/home/core/.ssh/authorized_keys" |
Contributor
Author
There was a problem hiding this comment.
changed because the real home of the user core is /var/home/core. It was working because there is a symlink on coreos from /home/ to /var/home
danpawlik
approved these changes
Jun 12, 2026
Member
|
@morenod Does this UPI means running cluster in the VM (nested virt) ? if yes then how it is different than just ssh to that VM and get the crc binary and run it? |
Contributor
Author
Im not an expert on crc-cloud, but when you execute the create command, it is not just about launching the binary, it modifies the SSH keys, it creates certificates, add the nip.io route... As I said, Im not an expert on crc-cloud, I have just tried the current behaviour for a provider, but without creating the CRC VM, and just considering that VM already up and running so we can reuse all the scripts (kubeconfig created after the execution, id_rsa copied to local folder, etc) |
Contributor
Author
Checking your comment again, this is not nested virt, the VM that we connect will be the CRC, no new VMs are created inside it |
This commit adds a new UPI provider to crc-cloud that allows users to
deploy OpenShift clusters on existing VMs they already manage, instead
of creating new cloud infrastructure.
Changes:
- Add new UPI provider package (pkg/provider/upi/)
- upi.go: Provider implementation following the provider interface
- constants.go: Provider parameters (host-ip, ssh-key-path)
- create-instance.go: Core logic for UPI deployments with enhanced
argument validation (reject empty values, handle all os.Stat errors,
validate IP address format)
- Register UPI provider in pkg/manager/providers.go
- Update pkg/manager/manager.go:
- Handle providers without Pulumi plugins (nil-safe plugin extraction)
- Add pluginInfoOf() helper to avoid code duplication
- Update pkg/manager/util.go:
- Skip plugin installation when not specified
- Read PULUMI_CONFIG_PASSPHRASE from environment instead of hardcoding
(maintains backward compatibility with default fallback)
- Add CLI command (cmd/cmd/create/upi.go) for UPI provider
- Proper error propagation (return error instead of os.Exit)
- Register UPI subcommand in cmd/cmd/create/create.go
- Document UPI provider in README.md
- Add security documentation for PULUMI_CONFIG_PASSPHRASE environment variable
The UPI provider accepts an existing VM's IP address and SSH private key,
then proceeds directly to cluster setup without creating any infrastructure.
This is useful for on-premise deployments or when infrastructure is managed
externally.
Security Note:
The Pulumi state encryption passphrase can now be configured via the
PULUMI_CONFIG_PASSPHRASE environment variable. While a default is provided
for convenience, users should set a custom passphrase in production
environments to ensure secrets in the Pulumi state remain secure.
Usage:
crc-cloud create upi \
--host-ip 192.168.1.100 \
--ssh-key-path /path/to/ssh_private_key \
--project-name crc-upi-cluster \
--pullsecret-filepath /path/to/pullsecret.json \
--backed-url file:///tmp/state \
--output /tmp/output
Assisted-By: Claude Sonnet 4.5 <noreply@anthropic.com>
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.
This commit adds a new provider that allows using crc-cloud with user-managed VMs instead of creating cloud infrastructure.
Features:
Changes:
pkg/provider/upi/: New provider implementation
pkg/bundle/setup/clustersetup.go: Parametrized SSH username
pkg/manager/: Updated for plugin-less providers
pkg/provider/{aws,gcp,openstack}/create-instance.go:
cmd/cmd/create/: CLI support for UPI
README.md: Documented UPI provider
Usage:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation