feat(local): per-provider DRCs, hops local doctor, and global --context#53
Merged
Merged
Conversation
…text Split the shared `local-dev` DeploymentRuntimeConfig into per-provider DRCs (local-dev-kubernetes, local-dev-helm), each with its own cluster-admin ServiceAccount + ClusterRoleBinding, and point each provider's runtimeConfigRef at its own DRC. A shared DRC let one provider's runtime image/SA silently clobber the other's pod. Add `hops local doctor`: verifies what `hops local start` set up — crossplane, both providers (installed / healthy / runtimeConfigRef pinned to its own DRC / DRC present / cluster-admin binding / ProviderConfig) and the registry — and reports drift with a non-zero exit + remediation. Catches a provider whose runtimeConfigRef reverted to `default`, dropping its cluster-admin SA (which breaks observing XRs through the in-cluster ProviderConfig). Add a global `--context` flag to `hops local` so every subcommand can target a context (e.g. `hops local aws --refresh --profile hops --context colima`), given before or after the subcommand. Plumbs through HOPS_KUBE_CONTEXT_ENV like config/provider install. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughRefactors Crossplane local development setup from a single shared ChangesPer-provider DRCs and doctor validation
Sequence Diagram(s)sequenceDiagram
participant User
participant DoctorCmd as hops local doctor
participant Kubectl
User->>DoctorCmd: Invoke doctor command
DoctorCmd->>Kubectl: Get current kube context
DoctorCmd->>Kubectl: Check Crossplane deployments Available
DoctorCmd->>Kubectl: Check local registry deployment Available
loop For each provider (helm, kubernetes)
DoctorCmd->>Kubectl: Get Provider status (Installed/Healthy)
DoctorCmd->>Kubectl: Get Provider runtimeConfigRef.name
DoctorCmd->>Kubectl: Verify DRC resource exists
DoctorCmd->>Kubectl: Check ClusterRoleBinding role ref is cluster-admin
DoctorCmd->>Kubectl: Verify ServiceAccount subject matches expected SA
DoctorCmd->>Kubectl: Verify ProviderConfig exists in namespace
end
DoctorCmd->>User: Print report with checkmarks/crosses
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/doctor.rs`:
- Around line 234-247: The exists() and jsonpath() helpers currently swallow all
kubectl errors and map them to false/None, which hides transport/auth failures;
change them to propagate errors instead (e.g., return Result<bool, Error> and
Result<String, Error> or Option<Result<...>>), updating call sites in doctor
aggregation to treat real kubectl failures as fatal, or implement a separate
kubectl connectivity preflight that runs a simple kubectl command and aborts on
any non-not-found/error to avoid reporting drift; update the functions named
exists and jsonpath to surface run_cmd_output errors rather than unwrapping to
defaults so upstream logic can distinguish "resource not found" from
transport/auth failures.
- Around line 170-181: The check only compares ServiceAccount names; update the
jsonpath query and the binding_ok logic to validate both subject.name and
subject.namespace against the expected values: change the jsonpath call that
builds subjects (currently referencing "clusterrolebinding" and e.binding) to
return subject name/namespace pairs (e.g. extract
"{.subjects[?(@.kind==\"ServiceAccount\")].name}{\"/\"}{.subjects[?(@.kind==\"ServiceAccount\")].namespace}"
or otherwise return both fields), then update the binding_ok computation (which
currently uses role, subjects, e.sa) to also compare the namespace (e.g. match
each subject tuple against (e.sa, e.namespace) or similar). Keep references to
jsonpath, subjects, binding_ok, role, e.binding and e.sa so the change is
localized.
🪄 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: cc6eb260-f12f-4321-ba83-529461c16c27
📒 Files selected for processing (8)
bootstrap/drc/helm.yamlbootstrap/drc/kubernetes.yamlbootstrap/drc/local-dev.yamlbootstrap/providers/provider-helm.yamlbootstrap/providers/provider-kubernetes.yamlsrc/commands/local/doctor.rssrc/commands/local/mod.rssrc/commands/local/start.rs
💤 Files with no reviewable changes (1)
- bootstrap/drc/local-dev.yaml
Comment on lines
+170
to
+181
| let subjects = jsonpath(&[ | ||
| "get", | ||
| "clusterrolebinding", | ||
| e.binding, | ||
| "-o", | ||
| "jsonpath={.subjects[?(@.kind==\"ServiceAccount\")].name}", | ||
| ]); | ||
| let binding_ok = role.as_deref() == Some("cluster-admin") | ||
| && subjects | ||
| .as_deref() | ||
| .map(|s| s.split_whitespace().any(|n| n == e.sa)) | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
Match the ServiceAccount namespace in the binding check.
This only validates the ServiceAccount name. A ClusterRoleBinding pointing at local-dev-kubernetes or local-dev-helm in the wrong namespace would still pass here, even though the provider pod would not get the intended privileges. Compare both subject name and namespace against the expected binding target.
🤖 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/doctor.rs` around lines 170 - 181, The check only compares
ServiceAccount names; update the jsonpath query and the binding_ok logic to
validate both subject.name and subject.namespace against the expected values:
change the jsonpath call that builds subjects (currently referencing
"clusterrolebinding" and e.binding) to return subject name/namespace pairs (e.g.
extract
"{.subjects[?(@.kind==\"ServiceAccount\")].name}{\"/\"}{.subjects[?(@.kind==\"ServiceAccount\")].namespace}"
or otherwise return both fields), then update the binding_ok computation (which
currently uses role, subjects, e.sa) to also compare the namespace (e.g. match
each subject tuple against (e.sa, e.namespace) or similar). Keep references to
jsonpath, subjects, binding_ok, role, e.binding and e.sa so the change is
localized.
Comment on lines
+234
to
+247
| fn exists(get_args: &[&str]) -> bool { | ||
| let mut args = get_args.to_vec(); | ||
| args.extend_from_slice(&["--ignore-not-found", "-o", "name"]); | ||
| run_cmd_output("kubectl", &args) | ||
| .map(|s| !s.trim().is_empty()) | ||
| .unwrap_or(false) | ||
| } | ||
|
|
||
| /// Run a kubectl query, returning the trimmed stdout or `None` if kubectl errors | ||
| /// (e.g. the resource does not exist). | ||
| fn jsonpath(args: &[&str]) -> Option<String> { | ||
| run_cmd_output("kubectl", args) | ||
| .ok() | ||
| .map(|s| s.trim().to_string()) |
There was a problem hiding this comment.
Don't collapse kubectl transport/auth failures into drift.
exists() and jsonpath() turn every kubectl error into false/None. If the selected context is unreachable, auth is broken, or kubectl itself fails, doctor will report missing providers/DRCs and recommend rerunning hops local start, which is the wrong remediation. Please propagate non-not-found errors, or add a single connectivity preflight and abort before aggregating checks.
🤖 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/doctor.rs` around lines 234 - 247, The exists() and
jsonpath() helpers currently swallow all kubectl errors and map them to
false/None, which hides transport/auth failures; change them to propagate errors
instead (e.g., return Result<bool, Error> and Result<String, Error> or
Option<Result<...>>), updating call sites in doctor aggregation to treat real
kubectl failures as fatal, or implement a separate kubectl connectivity
preflight that runs a simple kubectl command and aborts on any
non-not-found/error to avoid reporting drift; update the functions named exists
and jsonpath to surface run_cmd_output errors rather than unwrapping to defaults
so upstream logic can distinguish "resource not found" from transport/auth
failures.
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
Three related
hops localimprovements, all driven by an RBAC drift that broke the SMTPSender consumer-Observe pattern (a provider-kubernetes that observes a control-plane XR needs cluster-admin; it had drifted off its pinned cluster-admin SA).1. Per-provider DeploymentRuntimeConfigs (no more shared
local-dev)hops local startshared ONE DRC (local-dev) across provider-kubernetes AND provider-helm. A shared DRC lets one provider's runtime image/SA silently clobber the other's pod. Now each provider gets its own:bootstrap/drc/kubernetes.yaml→ DRClocal-dev-kubernetes+ SA +local-dev-kubernetes-cluster-adminbindingbootstrap/drc/helm.yaml→ DRClocal-dev-helm+ SA +local-dev-helm-cluster-adminbindingruntimeConfigRefpoints at its own DRC;start.rsapplies both2.
hops local doctorVerifies what
hops local startset up and surfaces drift: crossplane, both providers (installed / healthy / runtimeConfigRef pinned to its own DRC / DRC present / cluster-admin binding / ProviderConfig), and the registry. Prints a ✓/✗ report and exits non-zero with remediation. This catches the exact failure that motivated the change — a provider whoseruntimeConfigRefreverts todefault, dropping its cluster-admin SA so it can no longer observe XRs through the in-cluster ProviderConfig.3. Global
--contextflag onhops localhops local aws --refresh --profile hops --context colimapreviously errored (unexpected argument '--context'). Added aglobal = true--contexttohops localso every subcommand accepts it (before or after the subcommand); it plumbs throughHOPS_KUBE_CONTEXT_ENVlikeconfig/provider install.Verification
cargo buildclean ·cargo test86 passed · new files clippy-cleanhops local doctorrun against a live colima correctly flagged all 6 real drift items (both providers off their DRCs, missing DRCs/bindings) and exited 1hops local aws --helpnow lists--context;hops local doctor --context colimaparsesNote
The remote-cluster equivalents (
crossplane-{kubernetes,helm}-provider-stack) already compose per-provider cluster-admin DRCs and reconcile continuously — no change needed there. This PR brings local-dev (CLI-managed) to parity.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
hops local doctorcommand to validate and troubleshoot your local Crossplane deployment setup, provider health, and configurations--contextglobal option to local commands for specifying your target Kubernetes contextChores