feat: cluster backend abstraction (colima + kind + dory)#70
Open
patrickleet wants to merge 7 commits into
Open
feat: cluster backend abstraction (colima + kind + dory)#70patrickleet wants to merge 7 commits into
patrickleet wants to merge 7 commits into
Conversation
📝 WalkthroughWalkthroughAdds a unified local-cluster backend abstraction supporting Colima, kind, and Dory. Lifecycle, registry wiring, installation, context handling, documentation, and backend-specific smoke validation are updated accordingly. ChangesLocal backend abstraction
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
This was referenced Jul 4, 2026
patrickleet
changed the title
Collaborator
Author
![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 (2)
src/commands/config/install.rs (1)
178-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract
prepare_local_registryto a shared module to eliminate duplication.This function is identical to the one in
src/commands/provider/install.rs:141-148. Both callensure_registry()then delegate tobackend::wire_local_registry_for_target. Extracting it topackage_install.rs(which both files already import from) orbackend/mod.rswould keep the registry-preparation contract in one place and prevent divergence.♻️ Proposed extraction to `package_install.rs`
In
src/commands/local/package_install.rs:use crate::commands::local::backend::{self, Backend}; pub fn prepare_local_registry( backend: Backend, backend_flag: Option<Backend>, context: Option<&str>, ) -> Result<(), Box<dyn Error>> { ensure_registry()?; backend::wire_local_registry_for_target(backend, backend_flag, context) }Then in both
config/install.rsandprovider/install.rs, replace the localprepare_local_registrywith an import:-fn prepare_local_registry( - backend: Backend, - backend_flag: Option<Backend>, - context: Option<&str>, -) -> Result<(), Box<dyn Error>> { - ensure_registry()?; - backend::wire_local_registry_for_target(backend, backend_flag, context) -} +use crate::commands::local::package_install::prepare_local_registry;🤖 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 `@src/commands/config/install.rs` around lines 178 - 185, Extract the duplicated prepare_local_registry function from config/install.rs and provider/install.rs into the shared local package_install module. Make it public there, preserve its ensure_registry call and backend::wire_local_registry_for_target delegation, then import and use the shared function from both install modules and remove their local copies.src/commands/local/mod.rs (1)
176-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate kubectl/helm blocks to reduce duplication.
The helm blocks in
run_cmdandrun_cmd_outputare near-identical to the kubectl blocks, differing only in the context-injection helper. Extracting a shared helper would eliminate the copy-paste and make it easier to add future backends or command wrappers.♻️ Proposed consolidation for run_cmd
pub fn run_cmd(program: &str, args: &[&str]) -> Result<(), Box<dyn Error>> { - if program == "kubectl" { - let full = with_kube_context(args); - let refs: Vec<&str> = full.iter().map(|s| s.as_str()).collect(); - return run_cmd_with_logged_args(program, &refs, &refs); - } - if program == "helm" { - let full = with_helm_kube_context(args); + if program == "kubectl" || program == "helm" { + let full = if program == "kubectl" { + with_kube_context(args) + } else { + with_helm_kube_context(args) + }; let refs: Vec<&str> = full.iter().map(|s| s.as_str()).collect(); return run_cmd_with_logged_args(program, &refs, &refs); } run_cmd_with_logged_args(program, args, args) }♻️ Proposed consolidation for run_cmd_output
pub fn run_cmd_output(program: &str, args: &[&str]) -> Result<String, Box<dyn Error>> { - if program == "kubectl" { - let full = with_kube_context(args); - log::debug!("Running: {} {}", program, full.join(" ")); - let output = Command::new(program).args(&full).output()?; - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - return Err(format!("{} exited with {}: {}", program, output.status, stderr).into()); - } - return Ok(String::from_utf8_lossy(&output.stdout).to_string()); - } - if program == "helm" { - let full = with_helm_kube_context(args); + if program == "kubectl" || program == "helm" { + let full = if program == "kubectl" { + with_kube_context(args) + } else { + with_helm_kube_context(args) + }; log::debug!("Running: {} {}", program, full.join(" ")); let output = Command::new(program).args(&full).output()?; if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr); return Err(format!("{} exited with {}: {}", program, output.status, stderr).into()); } return Ok(String::from_utf8_lossy(&output.stdout).to_string()); }Also applies to: 198-207
🤖 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 `@src/commands/local/mod.rs` around lines 176 - 186, Consolidate the duplicated kubectl/helm handling in run_cmd and run_cmd_output by extracting a shared helper for context injection and command execution. Have the helper select the appropriate context-injection function based on the program, while preserving unchanged behavior for other programs and both logged and output-capturing execution paths.
🤖 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 `@src/commands/local/mod.rs`:
- Around line 114-116: Update the doc comments for the Install and Reset
variants in the Backend enum to include Dory alongside colima and kind,
accurately documenting all supported local backends while preserving the
existing operation descriptions.
---
Nitpick comments:
In `@src/commands/config/install.rs`:
- Around line 178-185: Extract the duplicated prepare_local_registry function
from config/install.rs and provider/install.rs into the shared local
package_install module. Make it public there, preserve its ensure_registry call
and backend::wire_local_registry_for_target delegation, then import and use the
shared function from both install modules and remove their local copies.
In `@src/commands/local/mod.rs`:
- Around line 176-186: Consolidate the duplicated kubectl/helm handling in
run_cmd and run_cmd_output by extracting a shared helper for context injection
and command execution. Have the helper select the appropriate context-injection
function based on the program, while preserving unchanged behavior for other
programs and both logged and output-capturing execution paths.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e496c838-001d-41dd-ba0b-9e88e832a36b
📒 Files selected for processing (22)
.github/workflows/on-pr-kind-smoke.yamlREADME.mdsrc/commands/auth/bootstrap.rssrc/commands/config/install.rssrc/commands/local/backend/colima.rssrc/commands/local/backend/dory.rssrc/commands/local/backend/kind.rssrc/commands/local/backend/mod.rssrc/commands/local/destroy.rssrc/commands/local/doctor.rssrc/commands/local/install.rssrc/commands/local/mod.rssrc/commands/local/package_install.rssrc/commands/local/reset.rssrc/commands/local/resize.rssrc/commands/local/start.rssrc/commands/local/stop.rssrc/commands/local/uninstall.rssrc/commands/provider/install.rssrc/commands/secrets/list.rssrc/commands/vars/mod.rssrc/commands/vars/sync.rs
Comment on lines
+114
to
+116
| /// Install the local cluster backend (colima or kind) via Homebrew | ||
| Install, | ||
| /// Reset local Colima Kubernetes state | ||
| /// Reset local Kubernetes state (colima: k8s reset; kind: recreate cluster) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Doc comments omit Dory backend.
The Install and Reset variant doc comments mention only "colima or kind" but the Backend enum now includes Dory. Update to reflect all supported backends.
📝 Proposed doc updates
- /// Install the local cluster backend (colima or kind) via Homebrew
+ /// Install the local cluster backend (colima, kind, or dory)
Install,
- /// Reset local Kubernetes state (colima: k8s reset; kind: recreate cluster)
+ /// Reset local Kubernetes state (colima: k8s reset; kind: recreate cluster; dory: recreate cluster)
Reset,📝 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.
Suggested change
| /// Install the local cluster backend (colima or kind) via Homebrew | |
| Install, | |
| /// Reset local Colima Kubernetes state | |
| /// Reset local Kubernetes state (colima: k8s reset; kind: recreate cluster) | |
| /// Install the local cluster backend (colima, kind, or dory) | |
| Install, | |
| /// Reset local Kubernetes state (colima: k8s reset; kind: recreate cluster; dory: recreate cluster) | |
| Reset, |
🤖 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 `@src/commands/local/mod.rs` around lines 114 - 116, Update the doc comments
for the Install and Reset variants in the Backend enum to include Dory alongside
colima and kind, accurately documenting all supported local backends while
preserving the existing operation descriptions.
This was referenced Jul 11, 2026
Move all colima-specific operations behind a Backend enum in src/commands/local/backend/ and rename ColimaSizeArgs to SizeArgs. Registry wiring (hosts sync) now flows through backend::wire_local_registry so provider/config installs stay backend-agnostic. No behavior change: identical command invocations on the colima path. Implements [[tasks/cluster-backend-abstraction]] (phase 1)
Adds --backend <colima|kind> (global on hops local) with resolution order flag > persisted (~/.hops/local/backend) > detected cluster (colima wins for back-compat) > platform default (macOS colima, else kind). kind backend: create with a pinned 127.0.0.1:30500 port mapping for the in-cluster registry NodePort, docker start/stop of the node container for resume, destroy/reset via kind delete + recreate, and containerd certs.d hosts.toml aliases (both registry names -> registry ClusterIP over HTTP) written idempotently after the registry deploys. Sizing flags error on kind. HOPS_KUBE_CONTEXT is auto-set from the backend when --context is absent. Preflight enforces kind >= 0.27 (certs.d config_path default). Implements [[tasks/cluster-backend-abstraction]] (phase 2)
GH Actions workflow exercises the CI acceptance path on ubuntu-latest: hops local start --backend kind, doctor, a registry round-trip through both pull names (Service name and localhost:30500), the stop/start resume path via the persisted backend, and destroy. README documents backend selection, persistence/resolution order, and the dory-via-kind recipe. Implements [[tasks/cluster-backend-abstraction]] (phases 3-4)
* feat: native dory backend for hops local Backend::Dory drives dory's built-in k3s headlessly via the dory CLI (k8s enable/disable/status) and the engine docker socket: - start: writes ~/.dory/k8s/registries.yaml (k3s-native trust aliasing both registry pull names to the Service hostname over HTTP; read at boot via dory's bind mount), then 'dory k8s enable --publish 30500:30500'. The Dory app's port forwarder makes the published NodePort host-reachable. - wire_registry: syncs hostname -> ClusterIP in the node's /etc/hosts per start (in-place rewrite — /etc/hosts is a bind mount, sed -i fails). - stop/resume via docker stop/start of dory-k8s; destroy/reset via dory k8s disable (+ enable); sizing flags error (VM sized in the app). - kube_context 'dory' (dory names its kubeconfig context that); hops prepends ~/.kube/dory-config to KUBECONFIG for its kubectl/helm children. - detection order: colima > kind > dory. Also: 'helm repo update crossplane-stable' instead of bare update — a stale unrelated repo in the user's helm config no longer breaks start. Verified end-to-end locally: start (Crossplane 2.3.3 + providers + registry), doctor all green, registry round-trip through BOTH pull names to pod Ready, stop/start resume, doctor green again. Implements [[tasks/dory-native-backend]] * fix: harden dory integration contract (#74) * fix: harden dory integration contract [[tasks/rr-2-dory-contract]] * fix: skip KUBECONFIG plumbing when dory context is already merged Current dory merges the dory context into ~/.kube/config at enable time, so hops no longer needs to mutate KUBECONFIG for child processes; the side-file prepend remains as a fallback for pre-merge dory versions. Implements [[tasks/dory-kubeconfig-merge]] * fix: hold dory cluster through the app's engine provisioning window Launching Dory.app re-provisions its engine for ~90s and restarts dockerd in the VM at the end, SIGTERMing every container — a k3s node enabled during that window reports Ready and then dies mid-bootstrap. The engine socket's mtime marks the session start, so when it is younger than 180s, start/reset now watch the node until the window passes and re-enable it (up to 3x) if the engine restart takes it down. Steady-state starts pay one container inspect. Implements [[tasks/rr-2-dory-contract]] * fix: unify kube context/backend targeting (#75) [[tasks/rr-1-context-targeting]]
Match main's on-pr-quality trigger so stacked PRs into this branch also get kind-smoke and quality.
patrickleet
force-pushed
the
feat/cluster-backend-abstraction
branch
from
4297cd2 to
29b3db5
Compare
* ci: add macOS colima backend smoke workflow Mirror kind smoke on macos-15-intel with nested-virt pin, --backend colima, and kubectl --context colima. Implements [[tasks/hops-cli-colima-macos-smoke]] under [[tasks/hops-cli-macos-backend-smoke]]. * test: structural checks for colima macOS smoke workflow Assert the shipped on-pr-colima-smoke.yaml pins macos-15-intel and mirrors kind smoke without a full GHA run. * ci: run colima smoke for all PR base branches Drop pull_request.branches: [main] so this job runs on stacked PRs into feat/cluster-backend-abstraction. * ci: size colima smoke VM for macos-15-intel runners Hops defaults (8 CPU / 16 GiB / 60 GiB) exceed standard GHA intel runners (~4 / ~14), so VZ hostagent exits during start. Pass explicit smaller sizes and dump lima ha.stderr on failure. * ci: give colima smoke enough memory for Crossplane 2 CPU / 4 GiB booted Colima but helm --wait for Crossplane hit 5m with Available: 0/1. Use 3/8 on macos-15-intel (~14 GiB host) and raise Crossplane helm --wait timeout to 10m for nested virt. * fix: wait longer for Crossplane on nested-virt colima Helm --wait hit 10m with Available:0/1 on GHA macos-15-intel. Apply the chart without --wait, poll deployments for ~15m with periodic pod dumps, wait for nodes Ready after docker restart, and capture cluster state before destroy on CI failure. * fix: harden kubectl apply under nested-virt API load Crossplane came Ready on GHA colima but ProviderConfigs apply failed with openapi schema download timeouts. Apply with --validate=false, retry transient failures, and re-wait for the API after Crossplane becomes leader. Soft-wait rbac-manager when core is healthy. * fix: wait for Established CRDs and room for registry PVC ProviderConfig apply raced CRD discovery; wait for Established. Registry PVC requests 20Gi — give colima smoke a 40Gi VM disk so local-path can bind, and wait longer with diagnostics for registry. * ci: recover colima stop/start resume after VM container churn After colima stop/start, pods Error on missing docker containers. Wait longer for Available and rollout-restart crossplane/registry if the first wait stalls. * fix: survive apiserver overload when applying registry Nested-virt colima CI loses the apiserver (TLS timeouts) right after Crossplane/provider install. Wait longer for /readyz, retry kubectl apply with API re-probes, and pre-pull registry:2 before deploy. * fix(ci): give colima smoke more RAM and settle time for registry pulls At 3/8/40, dual-name registry smoke pods sat in ContainerCreating without IPs while CoreDNS and metrics-server thrashed. Bump VM memory to 10Gi, wait for node/CoreDNS before the push, extend Ready timeout, and dump smoke pod describe on failure.
* ci: add macOS dory backend smoke spike workflow Clone+build patrickleet/dory @ feat/hops-local-integration in CI, wait for engine.sock, then kind-parity hops smoke with --backend dory. workflow_dispatch / non-required until public GHA can boot the engine. Implements [[tasks/hops-cli-dory-macos-smoke]] under [[tasks/hops-cli-macos-backend-smoke]]. * test: structural checks for dory macOS smoke workflow Assert clone+build install contract and kind-parity steps in the shipped on-pr-dory-smoke.yaml. * fix(ci): port colima nested-virt lessons into dory smoke Bring shared start hardness (CRD Established waits, apply retries, longer Crossplane/registry waits) from the green colima path, and harden the dory workflow: settle CoreDNS before registry pods, 420s Ready, stop/start rollout restart, failure dump, and ECR pull retries. Structural tests lock the contract in. * ci: run dory smoke on pull_request so PR branches can execute it workflow_dispatch only works once the file exists on the default branch; enable pull_request (still non-required) so #77 can actually run dory-smoke. * fix(ci): colima-backed engine.sock when dory-hv cannot run on GHA Public GHA cannot run dory-hv (no nested virt for native engine; app never creates ~/.dory). Pin macos-15-intel, still clone+build dory for scripts/, try the app briefly, then expose Colima docker as ~/.dory/engine.sock so hops local --backend dory can complete kind-parity smoke. * fix(local): recreate dory k8s on create-time config drift dory enable exits 3 when ports/registry binds drift. hops just wrote the desired config; auto-retry once with --recreate so local start applies it. CI: skip native engine wait on Intel, set DORY_ENGINE_SOCK, disable k8s first. * fix(local): retry helm install when apiserver openapi is still soft After dory stop/start, nodes can be Ready while openapi/v2 still times out and helm upgrade fails. Retry helm with wait_for_kubernetes between attempts; CI pause briefly after stop before start. * fix(local): recreate dory k8s when enable fails after stop/start docker stop of dory-k8s often leaves k3s stuck past dory's Ready wait on resume. On enable exit 1/3, disable and re-enable with --recreate once.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
.github/workflows/on-pr-colima-smoke.yaml (1)
21-30: 🚀 Performance & Scalability | 🔵 TrivialConsider scoping/limiting this expensive job.
This 90-minute, nested-virt macOS job (billed at a high per-minute multiplier) runs unconditionally on every
pull_requestpush with nopathsfilter and noconcurrencygroup to cancel superseded runs. Same applies to the dory smoke workflow. Consider adding apaths/paths-ignorefilter scoped tosrc/commands/local/**and backend-relevant files, and/or aconcurrencygroup keyed on the PR ref to cancel stale runs.🤖 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 @.github/workflows/on-pr-colima-smoke.yaml around lines 21 - 30, Limit the colima-smoke workflow’s pull_request trigger to changes affecting src/commands/local/** and backend-relevant files, and add concurrency keyed by the pull request ref so superseded runs are cancelled. Apply the same path filtering and concurrency behavior to the dory smoke workflow, while preserving workflow_dispatch..github/workflows/on-pr-kind-smoke.yaml (1)
8-14: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider a
concurrencygroup to cancel superseded runs.Each push to the PR queues a full 45-minute job (build + cluster lifecycle). Adding a
concurrencygroup keyed on the ref would cancel stale in-flight runs on new pushes and save CI minutes.concurrency: group: kind-smoke-${{ github.ref }} cancel-in-progress: true🤖 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 @.github/workflows/on-pr-kind-smoke.yaml around lines 8 - 14, Add a concurrency configuration to the kind-smoke workflow, using a group keyed by github.ref and enabling cancel-in-progress so newer pushes cancel superseded runs. Place it at the workflow level alongside on and jobs, leaving the existing job configuration unchanged.
🤖 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 @.github/workflows/on-pr-colima-smoke.yaml:
- Line 32: Update the actions/checkout@v4 step in the workflow to set
persist-credentials to false, leaving the rest of the smoke-test job unchanged.
In @.github/workflows/on-pr-dory-smoke.yaml:
- Around line 44-52: Set persist-credentials to false in the with configuration
for both checkout steps, including “Checkout hops-cli” and “Checkout
patrickleet/dory (hops integration branch)”, while preserving the existing
repository, ref, and path settings.
In @.github/workflows/on-pr-kind-smoke.yaml:
- Line 16: Update the actions/checkout@v4 step to set persist-credentials to
false, preventing the GITHUB_TOKEN from being stored in git configuration before
cargo build runs. Add a least-privilege permissions block for the workflow if no
repository write or authentication permissions are required.
In `@src/commands/local/backend/kind.rs`:
- Around line 216-254: Update write_hosts_toml to stop interpolating dir or path
into the sh -c command; pass them as positional arguments and use a quoted-safe
shell operation such as mkdir with tee, while continuing to pipe desired as
stdin and preserve the existing status/error handling.
---
Nitpick comments:
In @.github/workflows/on-pr-colima-smoke.yaml:
- Around line 21-30: Limit the colima-smoke workflow’s pull_request trigger to
changes affecting src/commands/local/** and backend-relevant files, and add
concurrency keyed by the pull request ref so superseded runs are cancelled.
Apply the same path filtering and concurrency behavior to the dory smoke
workflow, while preserving workflow_dispatch.
In @.github/workflows/on-pr-kind-smoke.yaml:
- Around line 8-14: Add a concurrency configuration to the kind-smoke workflow,
using a group keyed by github.ref and enabling cancel-in-progress so newer
pushes cancel superseded runs. Place it at the workflow level alongside on and
jobs, leaving the existing job configuration unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd8244e8-175e-4d0e-ba4c-a2ae4c8c52a4
📒 Files selected for processing (26)
.github/workflows/on-pr-colima-smoke.yaml.github/workflows/on-pr-dory-smoke.yaml.github/workflows/on-pr-kind-smoke.yamlREADME.mdsrc/commands/auth/bootstrap.rssrc/commands/config/install.rssrc/commands/local/backend/colima.rssrc/commands/local/backend/dory.rssrc/commands/local/backend/kind.rssrc/commands/local/backend/mod.rssrc/commands/local/destroy.rssrc/commands/local/doctor.rssrc/commands/local/install.rssrc/commands/local/mod.rssrc/commands/local/package_install.rssrc/commands/local/reset.rssrc/commands/local/resize.rssrc/commands/local/start.rssrc/commands/local/stop.rssrc/commands/local/uninstall.rssrc/commands/provider/install.rssrc/commands/secrets/list.rssrc/commands/vars/mod.rssrc/commands/vars/sync.rstests/colima_smoke_workflow.rstests/dory_smoke_workflow.rs
🚧 Files skipped from review as they are similar to previous changes (17)
- src/commands/secrets/list.rs
- src/commands/local/destroy.rs
- src/commands/local/resize.rs
- src/commands/local/stop.rs
- src/commands/local/install.rs
- src/commands/auth/bootstrap.rs
- src/commands/local/uninstall.rs
- src/commands/vars/mod.rs
- src/commands/local/backend/colima.rs
- src/commands/local/backend/mod.rs
- README.md
- src/commands/local/reset.rs
- src/commands/vars/sync.rs
- src/commands/local/backend/dory.rs
- src/commands/local/start.rs
- src/commands/config/install.rs
- src/commands/provider/install.rs
| # Nested virt + cold Crossplane pulls can take well over 30m. | ||
| timeout-minutes: 90 | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Set persist-credentials: false on checkout.
zizmor flags this checkout for credential persistence (artipacked). Since this job doesn't push/commit, the persisted git credential is unnecessary attack surface for the third-party tools (brew, colima, docker) executed later in the job.
🔒 Proposed fix
- uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 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.
Suggested change
| - uses: actions/checkout@v4 | |
| - uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 32-32: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/on-pr-colima-smoke.yaml at line 32, Update the
actions/checkout@v4 step in the workflow to set persist-credentials to false,
leaving the rest of the smoke-test job unchanged.
Source: Linters/SAST tools
Comment on lines
+44
to
+52
| - name: Checkout hops-cli | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Checkout patrickleet/dory (hops integration branch) | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: patrickleet/dory | ||
| ref: feat/hops-local-integration | ||
| path: dory-src |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Set persist-credentials: false on both checkouts.
Same artipacked finding as the colima workflow, flagged twice here (hops-cli checkout and the patrickleet/dory checkout). This job additionally clones and builds third-party source (xcodebuild, CODE_SIGNING_ALLOWED=NO) from a mutable feature branch, which raises the value of not persisting the job's git credential on disk while that build runs.
🔒 Proposed fix
- name: Checkout hops-cli
uses: actions/checkout@v4
+ with:
+ persist-credentials: false
- name: Checkout patrickleet/dory (hops integration branch)
uses: actions/checkout@v4
with:
repository: patrickleet/dory
ref: feat/hops-local-integration
path: dory-src
+ persist-credentials: false📝 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.
Suggested change
| - name: Checkout hops-cli | |
| uses: actions/checkout@v4 | |
| - name: Checkout patrickleet/dory (hops integration branch) | |
| uses: actions/checkout@v4 | |
| with: | |
| repository: patrickleet/dory | |
| ref: feat/hops-local-integration | |
| path: dory-src | |
| - name: Checkout hops-cli | |
| uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false | |
| - name: Checkout patrickleet/dory (hops integration branch) | |
| uses: actions/checkout@v4 | |
| with: | |
| repository: patrickleet/dory | |
| ref: feat/hops-local-integration | |
| path: dory-src | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 44-45: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 47-52: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/on-pr-dory-smoke.yaml around lines 44 - 52, Set
persist-credentials to false in the with configuration for both checkout steps,
including “Checkout hops-cli” and “Checkout patrickleet/dory (hops integration
branch)”, while preserving the existing repository, ref, and path settings.
Source: Linters/SAST tools
| runs-on: ubuntu-latest | ||
| timeout-minutes: 45 | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Set persist-credentials: false on checkout.
Default actions/checkout@v4 persists the GITHUB_TOKEN in the runner's git config. Since the very next step runs cargo build (executing arbitrary third-party build scripts), a compromised dependency could exfiltrate that token. Add persist-credentials: false since the workflow doesn't need to push/authenticate as itself, and consider adding an explicit least-privilege permissions: block.
🔒️ Proposed fix
- uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 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.
Suggested change
| - uses: actions/checkout@v4 | |
| - uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 16-16: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/on-pr-kind-smoke.yaml at line 16, Update the
actions/checkout@v4 step to set persist-credentials to false, preventing the
GITHUB_TOKEN from being stored in git configuration before cargo build runs. Add
a least-privilege permissions block for the workflow if no repository write or
authentication permissions are required.
Source: Linters/SAST tools
Comment on lines
+216
to
+254
| fn write_hosts_toml(registry_name: &str, cluster_ip: &str) -> Result<(), Box<dyn Error>> { | ||
| let dir = format!("/etc/containerd/certs.d/{}", registry_name); | ||
| let path = format!("{}/hosts.toml", dir); | ||
| let desired = hosts_toml(cluster_ip); | ||
|
|
||
| let current = | ||
| run_cmd_output("docker", &["exec", NODE_CONTAINER, "cat", &path]).unwrap_or_default(); | ||
| if current == desired { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| log::info!( | ||
| "Wiring containerd registry alias: {} -> {}:5000", | ||
| registry_name, | ||
| cluster_ip | ||
| ); | ||
|
|
||
| let mut child = Command::new("docker") | ||
| .args([ | ||
| "exec", | ||
| "-i", | ||
| NODE_CONTAINER, | ||
| "sh", | ||
| "-c", | ||
| &format!("mkdir -p '{}' && cat > '{}'", dir, path), | ||
| ]) | ||
| .stdin(Stdio::piped()) | ||
| .stdout(Stdio::null()) | ||
| .stderr(Stdio::inherit()) | ||
| .spawn()?; | ||
| if let Some(ref mut stdin) = child.stdin { | ||
| stdin.write_all(desired.as_bytes())?; | ||
| } | ||
| let status = child.wait()?; | ||
| if !status.success() { | ||
| return Err(format!("failed to write {} on kind node", path).into()); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid interpolating values into a sh -c string.
dir/path are built from REGISTRY_PULL/REGISTRY_PUSH (fixed constants) so this isn't exploitable today, but interpolating them directly into a shell command string is a command-injection anti-pattern that will bite if either constant, or a future caller, ever derives from less-trusted input (e.g., a user-configurable registry name). Prefer passing the file content via tee/positional shell args instead of string-formatting into -c.
🔒️ Proposed fix using positional args instead of string interpolation
let mut child = Command::new("docker")
.args([
"exec",
"-i",
NODE_CONTAINER,
"sh",
"-c",
- &format!("mkdir -p '{}' && cat > '{}'", dir, path),
+ "mkdir -p \"$1\" && cat > \"$1/hosts.toml\"",
+ "sh",
+ &dir,
])📝 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.
Suggested change
| fn write_hosts_toml(registry_name: &str, cluster_ip: &str) -> Result<(), Box<dyn Error>> { | |
| let dir = format!("/etc/containerd/certs.d/{}", registry_name); | |
| let path = format!("{}/hosts.toml", dir); | |
| let desired = hosts_toml(cluster_ip); | |
| let current = | |
| run_cmd_output("docker", &["exec", NODE_CONTAINER, "cat", &path]).unwrap_or_default(); | |
| if current == desired { | |
| return Ok(()); | |
| } | |
| log::info!( | |
| "Wiring containerd registry alias: {} -> {}:5000", | |
| registry_name, | |
| cluster_ip | |
| ); | |
| let mut child = Command::new("docker") | |
| .args([ | |
| "exec", | |
| "-i", | |
| NODE_CONTAINER, | |
| "sh", | |
| "-c", | |
| &format!("mkdir -p '{}' && cat > '{}'", dir, path), | |
| ]) | |
| .stdin(Stdio::piped()) | |
| .stdout(Stdio::null()) | |
| .stderr(Stdio::inherit()) | |
| .spawn()?; | |
| if let Some(ref mut stdin) = child.stdin { | |
| stdin.write_all(desired.as_bytes())?; | |
| } | |
| let status = child.wait()?; | |
| if !status.success() { | |
| return Err(format!("failed to write {} on kind node", path).into()); | |
| } | |
| Ok(()) | |
| } | |
| fn write_hosts_toml(registry_name: &str, cluster_ip: &str) -> Result<(), Box<dyn Error>> { | |
| let dir = format!("/etc/containerd/certs.d/{}", registry_name); | |
| let path = format!("{}/hosts.toml", dir); | |
| let desired = hosts_toml(cluster_ip); | |
| let current = | |
| run_cmd_output("docker", &["exec", NODE_CONTAINER, "cat", &path]).unwrap_or_default(); | |
| if current == desired { | |
| return Ok(()); | |
| } | |
| log::info!( | |
| "Wiring containerd registry alias: {} -> {}:5000", | |
| registry_name, | |
| cluster_ip | |
| ); | |
| let mut child = Command::new("docker") | |
| .args([ | |
| "exec", | |
| "-i", | |
| NODE_CONTAINER, | |
| "sh", | |
| "-c", | |
| "mkdir -p \"$1\" && cat > \"$1/hosts.toml\"", | |
| "sh", | |
| &dir, | |
| ]) | |
| .stdin(Stdio::piped()) | |
| .stdout(Stdio::null()) | |
| .stderr(Stdio::inherit()) | |
| .spawn()?; | |
| if let Some(ref mut stdin) = child.stdin { | |
| stdin.write_all(desired.as_bytes())?; | |
| } | |
| let status = child.wait()?; | |
| if !status.success() { | |
| return Err(format!("failed to write {} on kind node", path).into()); | |
| } | |
| Ok(()) | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 232-240: Passing non-literal (user-controlled or interpolated) data to a shell invoked via std::process::Command::new("sh"|"bash"|...) with -c allows command injection. Avoid spawning a shell: pass the program and each argument separately to Command::new(program).arg(arg) so the OS never re-parses the string, or strictly allowlist/escape any value that must reach a shell.
Context: Command::new("docker")
.args([
"exec",
"-i",
NODE_CONTAINER,
"sh",
"-c",
&format!("mkdir -p '{}' && cat > '{}'", dir, path),
])
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-process-shell-rust)
🤖 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 `@src/commands/local/backend/kind.rs` around lines 216 - 254, Update
write_hosts_toml to stop interpolating dir or path into the sh -c command; pass
them as positional arguments and use a quoted-safe shell operation such as mkdir
with tee, while continuing to pipe desired as stdin and preserve the existing
status/error handling.
Source: Linters/SAST tools
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
1 participant
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.
Summary
hops localnow supports three backends behind the same commands:How it works
--backend <colima|kind>onhops local; persisted to~/.hops/local/backendon successful start. Resolution: flag > persisted > existing-cluster detection (colima wins for back-compat) > platform default (macOS colima, else kind).HOPS_KUBE_CONTEXTauto-set from the backend (colima/kind-hops) when--contextis absent.127.0.0.1:30500 -> 30500; stop/start resume viadocker stop/startof the node container; reset recreates the cluster.hosts.tomlfiles alias both pull names (registry.crossplane-system.svc.cluster.local:5000andlocalhost:30500, which provider runtime pods use) to the registry Service ClusterIP over HTTP. Written idempotently after the registry deploys; survives node restarts; no containerd restart needed (requires kind >= 0.27, enforced in preflight).resizefail with actionable errors on kind.provider install/config installregistry wiring flows through the same backend seam (was hardcodedcolima sshhosts sync).dory gets no native backend (only 6443 published, no CLI, trust wiped on recreate — see task doc); dory users run the kind backend against dory's docker socket. Documented in README.
Commits
refactor:extract backend seam, colima-only, no behavior changefeat:kind backend + flag/persistence/detectionci+docs:kind smoke workflow + READMETest output
cargo test: 116 passed (new: resolution order, kind config gen, hosts.toml content, kind version parse, sizing rejection)hops local doctoron a running colima cluster resolves backend to colima via detection and targets contextcolima;hops local resize --backend kindandstart --backend kind --cpus 4fail with the intended errors.kind backend smokeworkflow in this PR (doctor + dual-name registry round-trip + stop/start resume + destroy).Implements [[tasks/cluster-backend-abstraction]]
🤖 Generated with Claude Code
https://claude.ai/code/session_011Ei12zcYdwvUoZ3jCbwiWf
Summary by CodeRabbit
New Features
hops locallifecycle commands.Documentation
Bug Fixes