From dace0012f20259f22bdd0c96068d5b6a0be211b2 Mon Sep 17 00:00:00 2001 From: Erick Bourgeois Date: Sat, 30 May 2026 09:38:31 -0300 Subject: [PATCH] 1. Child-cluster Node API support (the bulk of the branch) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Routes Node/Pod operations to a CAPI/k0smotron workload (child) cluster while the ScheduledMachine lives on the management cluster. - New CRD field spec.kubeconfigSecretRef (src/crd.rs) — optional reference to a same-namespace -kubeconfig Secret; cross-namespace refs forbidden by design. - src/reconcilers/child_client.rs (+tests, ~1,400 lines) — resolves the right kube::Client (management vs child), caches by Secret with LRU + resourceVersion-based rotation. - src/reconcilers/child_watch.rs (+tests, untracked, ~520 lines) — one Node watcher per child cluster; maps child Node events back to ScheduledMachine reconciles via Controller::reconcile_on. - tests/integration_child_kubeconfig.rs (untracked, 416 lines) — end-to-end kubeconfig resolution tests. - Wiring/support: src/main.rs (controller plumbing), src/metrics.rs (Phase-2 child-cluster metrics), new error variants in the ScheduledMachine lives on the management cluster. - New CRD field spec.kubeconfigSecretRef (src/crd.rs) — optional reference to a same-namespace -kubeconfig Secret; cross-namespace refs resourceVersion-based rotation. - src/reconcilers/child_watch.rs (+tests, untracked, ~520 lines) — one Node watcher per child cluster; maps child Node events back to ScheduledMachine reconciles via Controller::reconcile_on. - tests/integration_child_kubeconfig.rs (untracked, 416 lines) — end-to-end kubeconfig resolution tests. - Wiring/support: src/main.rs (controller plumbing), src/metrics.rs (Phase-2 child-cluster metrics), new error variants in scheduled_machine.rs, Cargo.toml deps (kube unstable-runtime, tokio-stream, base64), and docs docs/src/concepts/child-cluster-kubeconfig.md + examples/scheduledmachine-child-cluster.yaml. 2. Security hardening - RBAC validation of bootstrapSpec/infrastructureSpec — user anti-escalation via VAP authorizer rules (13a/13b) + controller-SA SelfSubjectAccessReview pre-flight (ensure_can_create()); new PermissionDenied error + metric. - Embedded metadata enforcement — loudly reject metadata.name/metadata.namespace (VAP rules 13c–13f + runtime validate_embedded_metadata()), while newly supporting metadata.labels/annotations (reserved-prefix-checked, controller labels win). Required making the embedded metadata preserve-unknown so the field isn't pruned before rejection. - Updated deploy/admission/validatingadmissionpolicy.yaml, regenerated CRD + api.md, threat-model (T1 hardened), admission-validation doc, and SCHEDULED_MACHINE_LABEL constant. Signed-off-by: Erick Bourgeois --- .claude/CHANGELOG.md | 422 +++++++++ .gitignore | 2 + Cargo.lock | 2 + Cargo.toml | 15 +- .../admission/validatingadmissionpolicy.yaml | 91 ++ deploy/crds/scheduledmachine.yaml | 88 +- docs/src/concepts/child-cluster-kubeconfig.md | 175 ++++ docs/src/reference/api.md | 20 + docs/src/security/admission-validation.md | 89 +- docs/src/security/threat-model.md | 3 +- examples/scheduledmachine-child-cluster.yaml | 70 ++ src/bin/crddoc.rs | 20 + src/constants.rs | 47 + src/crd.rs | 187 ++++ src/crd_tests.rs | 298 +++++++ src/main.rs | 32 +- src/metrics.rs | 73 ++ src/reconcilers/child_client.rs | 569 ++++++++++++ src/reconcilers/child_client_tests.rs | 834 ++++++++++++++++++ src/reconcilers/child_watch.rs | 239 +++++ src/reconcilers/child_watch_tests.rs | 282 ++++++ src/reconcilers/helpers.rs | 274 +++++- src/reconcilers/helpers_tests.rs | 353 ++++++++ src/reconcilers/mod.rs | 6 + src/reconcilers/scheduled_machine.rs | 130 ++- src/reconcilers/scheduled_machine_tests.rs | 1 + tests/integration_child_kubeconfig.rs | 416 +++++++++ 27 files changed, 4700 insertions(+), 38 deletions(-) create mode 100644 docs/src/concepts/child-cluster-kubeconfig.md create mode 100644 examples/scheduledmachine-child-cluster.yaml create mode 100644 src/reconcilers/child_client.rs create mode 100644 src/reconcilers/child_client_tests.rs create mode 100644 src/reconcilers/child_watch.rs create mode 100644 src/reconcilers/child_watch_tests.rs create mode 100644 tests/integration_child_kubeconfig.rs diff --git a/.claude/CHANGELOG.md b/.claude/CHANGELOG.md index 76490b9..3624b20 100644 --- a/.claude/CHANGELOG.md +++ b/.claude/CHANGELOG.md @@ -9,6 +9,428 @@ The format is based on the regulated environment requirements: --- +## [2026-05-30 12:00] - Revert docs deploy trigger to release:published (CodeQL: pwn-request + cache poisoning) + +**Author:** Erick Bourgeois + +### Changed +- `.github/workflows/docs.yaml`: reverted the docs **deploy trigger** from + `workflow_run` (Build completion) back to the trusted `release: published` + event, removing the `ref: github.event.workflow_run.head_sha` checkout. The + file now matches `main`'s known-clean version (trusted trigger, no + event-derived checkout ref, plain `actions/cache`). + +### Why +PR #71 had switched the docs deploy trigger to `workflow_run` to gate deploy on +Build success. That introduced 7 CodeQL alerts — 5 critical "Checkout of +untrusted code in a privileged context" and 2 high "Cache Poisoning": +`workflow_run` is a privileged trigger (default-branch context, secrets, write +perms) that fired for every Build completion (incl. fork PRs), then checked out +`head_sha` and ran build steps. A first attempt to *harden* the workflow_run +path (job-level release-only `if` gate + cache restore/save split + least- +privilege id-token) did **not** clear the alerts — CodeQL's queries key +syntactically on the untrusted `head_sha` checkout under a privileged trigger, +regardless of guards. The only root-cause fix is to stop checking out the +untrusted ref, so the trigger was reverted to `release: published`. For a +release, `github.ref` is the tag, so docs still build from the exact released +commit. Trade-off accepted: docs deploy on release publication even if the +binary/image Build job failed (the prior, vulnerable, behaviour gated on it). + +### Impact +- [ ] Breaking change +- [ ] Requires cluster rollout +- [ ] Config change only +- [x] CI / security (workflow only) + +## [2026-05-30 00:00] - Embedded metadata: reject name/namespace, support labels/annotations + +**Author:** Erick Bourgeois + +### Changed +- `src/crd.rs`: `embedded_resource_schema` now declares an optional `metadata` + block with typed `labels`/`annotations` (string maps) and + `x-kubernetes-preserve-unknown-fields: true` (so `metadata.namespace`/`name` + are retained for rejection rather than pruned). Added `EmbeddedResource` + accessors `metadata_namespace()`, `metadata_name()`, `metadata_labels()`, + `metadata_annotations()` + `embedded_string_map()` helper. +- `src/reconcilers/helpers.rs`: New `validate_embedded_metadata()` — rejects + controller-owned `metadata.name`/`metadata.namespace` and reserved-prefix + labels/annotations; called for both specs in `add_machine_to_cluster()`. New + `merge_owned_labels()` merges user labels with controller-owned tracking + labels (controller wins). Bootstrap/infra resources now carry user-supplied + `metadata.labels`/`annotations`. +- `src/constants.rs`: Added `SCHEDULED_MACHINE_LABEL` (promotes a 3×-repeated + literal to a constant). +- `deploy/admission/validatingadmissionpolicy.yaml`: Added rules 13c–13f + rejecting `metadata.namespace`/`metadata.name` on both embedded specs. +- `src/bin/crddoc.rs` + `docs/src/reference/api.md`: Documented embedded + `metadata.labels`/`annotations` and the name/namespace rejection. +- `deploy/crds/scheduledmachine.yaml`: Regenerated (crdgen). +- `docs/src/security/admission-validation.md`, `docs/src/security/threat-model.md`: + Documented rules 13c–13f and hardened threat T1. +- `src/crd_tests.rs`, `src/reconcilers/helpers_tests.rs`: Added 14 tests + (metadata accessors, schema shape, validate_embedded_metadata paths, + merge_owned_labels precedence). + +### Why +A user could set `bootstrapSpec.metadata.namespace`/`name`; the API server +silently *pruned* it (no error), and the controller ignored it. The user +requested a loud rejection. Pruning happens before admission policies run and +`additionalProperties: false` is a no-op for CRDs, so the embedded `metadata` +subtree is now `x-kubernetes-preserve-unknown-fields` to let both the VAP and a +runtime check reject the field explicitly. Per follow-up, `metadata.labels`/ +`annotations` are now supported (safe: run through the reserved-prefix allowlist +so `cluster.x-k8s.io/*`/`5spot.finos.org/*` cannot be forged — threat T2). + +### Impact +- [ ] Breaking change +- [x] Requires cluster rollout +- [ ] Config change only +- [ ] Documentation only + +> **Note:** Re-apply `deploy/crds/scheduledmachine.yaml` and +> `deploy/admission/validatingadmissionpolicy.yaml`. Existing specs without an +> embedded `metadata` block are unaffected. + +## [2026-05-29 00:00] - RBAC validation for bootstrapSpec & infrastructureSpec + +**Author:** Erick Bourgeois + +### Changed +- `src/reconcilers/helpers.rs`: Added `ensure_can_create()` — a pre-flight + `SelfSubjectAccessReview` confirming the controller's service account may + `create` the embedded bootstrap, infrastructure, and CAPI `Machine` resource + types before any are created in `add_machine_to_cluster()`. Extracted + `resource_plural()` helper (dedupes the two plural-derivation sites) so the + access review and the create/delete calls always target the same RBAC + resource. +- `src/reconcilers/scheduled_machine.rs`: New `ReconcilerError::PermissionDenied` + variant + `record_error("permission_denied")` metric mapping. +- `src/constants.rs`: Added `RBAC_VERB_CREATE`. +- `src/reconcilers/helpers_tests.rs`: Added 8 tests — `resource_plural` cases + and `ensure_can_create` allowed / denied / denied-no-reason / API-error paths + (tower mock). +- `deploy/admission/validatingadmissionpolicy.yaml`: Added `spec.variables` + (group + plural derivation) and two `authorizer.check('create')` validations + (rules 13a/13b) requiring the *creating user* to hold `create` on the embedded + bootstrap/infrastructure GVKs — privilege-escalation guard at admission. +- `docs/src/security/admission-validation.md`: Documented rules 13a/13b and the + two-layer (user @ admission, controller SA @ reconcile) design. +- `docs/src/security/threat-model.md`: Added mitigated threat E1a (escalation + through the controller). + +### Why +A user able to create a `ScheduledMachine` but not the embedded bootstrap/ +infrastructure resource could otherwise have the broadly-permissioned controller +create it on their behalf — a privilege escalation through the controller. The +admission `authorizer` checks gate the requesting user; the controller's own SA +is independently gated at reconcile so an RBAC gap surfaces as a clear +`PermissionDenied` instead of an opaque 403 mid-creation. + +### Impact +- [ ] Breaking change +- [x] Requires cluster rollout +- [ ] Config change only +- [ ] Documentation only + +> **Note:** Re-apply `deploy/admission/validatingadmissionpolicy.yaml`. The +> controller's RBAC is unchanged, but users creating `ScheduledMachine` +> resources now additionally need `create` RBAC on the embedded bootstrap/ +> infrastructure resource types in their namespace. + +## [2026-05-11 10:00] - Child-cluster Node-watch multiplexer + Phase 2 metrics + +**Author:** Erick Bourgeois + +### Added +- `src/reconcilers/child_client.rs`: new `ChildWatchHook` trait + (`on_child_resolved` / `on_child_evicted`) that the cache fires when a + child `kube::Client` is first built, when a token rotation rebuilds + one, or when an entry is evicted (manual or LRU). Defaults to a no-op, + so unit tests + degenerate deployments are unaffected. The cache fires + the eviction-before-resolved sequence on every RV-driven rebuild so + the manager never has two watchers running on the same `CacheKey` with + different credentials. +- `src/reconcilers/child_watch.rs` (~220 LOC) + `_tests.rs` (~270 LOC): + new `ChildNodeWatchManager` implementing `ChildWatchHook`. Owns a + `HashMap>` and spawns one `kube::runtime:: + watcher::watcher` task per active child cluster. Each task maps + `Node` events through the existing canonical + `node_to_scheduled_machines_via_machine` (using a snapshot of the + management cluster's CAPI Machine reflector) and pushes the resulting + `ObjectRef`s on a shared `mpsc::Sender`. 10 + lifecycle tests cover spawn-on-resolve, abort-on-evict, idempotent + back-to-back resolve, double-key independence, and the + receiver-dropped graceful path. +- `src/main.rs`: builds the mpsc channel, constructs the manager from + `(tx, machine_store)`, installs it on the cache via + `context.child_clients.set_hook(...)`, and feeds the receiver into + `Controller::reconcile_on(ReceiverStream::new(rx))`. Closes the + event-driven gap left by the Phase 1 resolver work: a child-cluster + Node going `NotReady` / being cordoned externally / changing labels + now triggers a reconcile of the owning `ScheduledMachine` within + watcher latency rather than waiting for the periodic requeue. +- `src/metrics.rs`: two new Prometheus counters — + `fivespot_child_kubeconfig_resolutions_total{result}` (labels: + `management`, `child_explicit`, `child_auto`, `cache_hit`, `rebuild`, + `error`) and `fivespot_child_kubeconfig_errors_total{reason}` (labels: + `secret_missing_key`, `invalid_yaml`, `unreachable`, + `non_404_kube_error`). Helper fns `record_child_kubeconfig_resolution` + + `record_child_kubeconfig_error` wired into + `ChildClientCache::resolve`. +- `src/constants.rs`: `CHILD_NODE_EVENT_CHANNEL_CAP = 1024` for the + shared mpsc buffer; `CONDITION_TYPE_CHILD_CLUSTER_REACHABLE = + "ChildClusterReachable"` (constant added now; the writer wiring is + deferred — see "Deferred" below). +- `tests/integration_child_kubeconfig.rs` (~330 LOC): hermetic + end-to-end test exercising the full Context → cache → manager wiring + via `tower_test::mock`. 5 cases cover: + explicit-ref resolve spawns one watcher; auto-discovery 404 falls + back to management with no watcher; RV rotation cancels + restarts + exactly one watcher; explicit `evict()` cancels; explicit-ref 404 + fails closed (errors AND spawns no watcher). +- 5 new resolver tests in `src/reconcilers/child_client_tests.rs` for + the hook plumbing: fires-once-on-initial-build, does-not-fire-on- + cache-hit-same-RV, fires-evicted-then-resolved-on-RV-change, + explicit-evict-fires-once-per-removal, does-not-fire-for-management- + fallback. + +### Changed +- `Cargo.toml`: + - Added `tokio-stream = "0.1"` (direct dep) for `ReceiverStream` — + adapts `mpsc::Receiver` into the `Stream` that + `Controller::reconcile_on` expects. + - Enabled the `unstable-runtime` feature on the `kube` dep so + `Controller::reconcile_on` is callable. The feature is gated as + "unstable" by upstream (the API may shift between kube-runtime + minor versions), but it is the canonical way to push external + triggers into the controller loop and has no replacement in the + stable surface area. + +### Why +The Phase 1 work (2026-05-10) routed Node/Pod operations to the child +cluster correctly but left Node *events* uncovered: a child-cluster +Node going `NotReady`, getting cordoned by an out-of-band operator, or +being deleted upstream would not trigger a reconcile until the +periodic `TIMER_REQUEUE_SECS` requeue (worst case ~60s of latency for +drain progress / status enrichment). This change closes that gap with +one watcher per unique kubeconfig Secret, sharing watcher work across +all SMs that target the same child cluster. The Phase 2 metrics give +operators visibility into resolution outcomes (hit / rebuild / error) +without log-grepping. + +### Impact +- [ ] Breaking change +- [x] Requires cluster rollout (controller binary changed; behaviour + additive — co-located deployments without `kubeconfigSecretRef` + and without an auto-discoverable `-kubeconfig` + Secret see no change) +- [ ] Config change only +- [ ] Documentation only + +Operational notes: +- Each unique `(namespace, secret_name)` pair backs exactly one + watcher task. A deployment with 100 SMs spread across 5 child + clusters runs 5 child-cluster Node watchers, not 100. +- Token / cert rotations driven by CAPI's control-plane provider bump + the kubeconfig Secret's `resourceVersion`. The cache detects this on + the next `resolve()`, aborts the stale watcher, and starts a fresh + one with the rebuilt `Client` — no controller restart required. +- The mpsc buffer (`CHILD_NODE_EVENT_CHANNEL_CAP = 1024`) is sized for + initial-list bursts across all child clusters; a back-pressure + scenario (channel full) is logged at debug level and means the + controller is shutting down. + +### Deferred +- **`ChildClusterReachable` condition writer.** The condition type + constant is in place (`CONDITION_TYPE_CHILD_CLUSTER_REACHABLE`) but + the writer wiring (translate each `resolve()` outcome into a status + patch on the owning SM) is left for a follow-up — it requires + threading the SM ref + the recorder through each call site, which is + more invasive than the metrics work that closes the same + observability need today. +- **Real-cluster dual-cluster integration test.** The hermetic + integration test under `tests/` exercises the wiring deterministically; + a `kind`-based two-cluster test would add HTTP-level coverage and + fits the pattern of the existing `integration_node_taints.rs` / + `integration_emergency_reclaim.rs` real-cluster tests. Left for the + next development-environment standardisation pass. + +--- + +## [2026-05-10 14:00] - Child-cluster kubeconfig support (CRD + resolver + routing) + +**Author:** Erick Bourgeois + +### Added +- `src/crd.rs`: New optional `spec.kubeconfigSecretRef` field on + `ScheduledMachineSpec` plus a new `KubeconfigSecretRef { name, key }` + struct (with `deny_unknown_fields` to forbid cross-namespace `namespace` + fields by design). Two new schema helpers + (`kubeconfig_secret_name_schema`, `kubeconfig_secret_key_schema`) bound + the name (RFC-1123 DNS subdomain, 253 chars) and the data key. Default + for `key` is `"value"` — CAPI's convention. +- `src/reconcilers/child_client.rs` (~400 LOC) + `_tests.rs` (~470 LOC): + new `ChildClientCache` resolver. Public surface: `CacheKey`, + `ResolvedClient::{Management, Child}`, `ChildClientCache::resolve` + (and `peek` / `evict` helpers for future per-child watch integration). + Resolution order: explicit `spec.kubeconfigSecretRef` → auto-discover + `-kubeconfig` Secret in the SM namespace → + management-client fallback. Cache keyed by `(namespace, secret_name)` + and invalidated by Secret `resourceVersion`; bounded-LRU at + `CHILD_CLIENT_CACHE_CAP = 256`. Built child `kube::Client` inherits + the same `K8S_API_TIMEOUT_SECS` wire timeouts as the management + client. 12 new tests cover happy paths, cache hit/miss, RV-rebuild, + fail-closed for explicit-ref 404, 403 propagation, missing data key, + and invalid YAML. +- `src/reconcilers/scheduled_machine.rs`: three new `ReconcilerError` + variants — `KubeconfigSecretMissingKey`, `KubeconfigInvalid`, + `ChildClusterUnreachable` — each mapped to a distinct Prometheus + error label via the existing `record_error` switch. +- `src/constants.rs`: `CHILD_CLIENT_CACHE_CAP = 256`. +- `Context.child_clients: Arc` field, constructed in + `Context::new`. Cheap (`Arc>`) so existing + single-instance deployments pay nothing. +- `docs/src/concepts/child-cluster-kubeconfig.md`: full RBAC + requirements, resolution order, cache invalidation semantics, and + threat model. +- `examples/scheduledmachine-child-cluster.yaml`: end-to-end example + with an explicit `kubeconfigSecretRef`. +- 9 new CRD round-trip tests in `src/crd_tests.rs` covering: optional + field round-trip, default-key, `deny_unknown_fields`, required + `name`, schema-bounded name + key, and the new field being absent + from the `required` list (backward compat). + +### Changed +- `src/reconcilers/scheduled_machine.rs`: + - `provision_reclaim_agent_best_effort` resolves the child client at + its top and threads it into `reconcile_reclaim_agent_provision` + (the ConfigMap consumed by the reclaim-agent DaemonSet lives on + workload nodes; previously this projected to the wrong cluster). + - `reconcile_node_taints_best_effort` resolves and threads to + `reconcile_node_taints` so user-declared taints land on + workload-cluster Nodes. + - `handle_shutting_down_phase` resolves and threads to + `drain_node_with_timeout`. The CAPI Machine lookup + (`get_node_from_machine`) stays on the management client where + Machine objects live. +- `src/reconcilers/helpers.rs::handle_emergency_remove`: resolves the + child client once at the top of the flow and threads it to + `drain_node_with_timeout` + `clear_reclaim_annotations_best_effort`. + Schedule-disable patch + Machine deletion stay on the management + client. +- `deploy/crds/scheduledmachine.yaml`: regenerated via + `cargo run --bin crdgen` to surface the new field in the OpenAPI + schema (additive — no breaking change to existing CRs). +- `Cargo.toml`: added `base64 = "0.22"` as a dev-dependency (used by + `child_client_tests.rs` to build base64-encoded Secret payloads for + the `tower-test` mock server). + +### Why +In a CAPI + k0smotron / k0rdent topology, the `Node` and `Pod` objects +5-Spot manipulates (cordon, drain, taint, reclaim-agent labels / +ConfigMaps) live inside the workload (child) cluster, not the +management cluster. The previous single-client model worked correctly +only in the degenerate co-located case (management ≡ workload), +which is the dev/test posture rather than production. This change +makes 5-Spot multi-cluster-correct: a `ScheduledMachine` carrying a +`kubeconfigSecretRef` (or sitting in a namespace with a CAPI-convention +`-kubeconfig` Secret) has its Node/Pod traffic routed +through the child kubeconfig; everything else (the SM CR, the CAPI +Machine, bootstrap, infrastructure) stays on the management client. + +Fail-closed by design: a misconfigured or unparseable kubeconfig +surfaces as `ReconcilerError::ChildClusterUnreachable` and back-offs, +rather than silently falling through to the management client and +routing operations to the wrong cluster. + +### Impact +- [ ] Breaking change (additive optional field; existing SMs unchanged) +- [x] Requires cluster rollout (controller binary changed; CRD schema + additive but should be re-applied) +- [ ] Config change only +- [ ] Documentation only + +Operational notes: +- Existing single-cluster deployments without a + `-kubeconfig` Secret in the SM namespace continue to use + the management client for Node/Pod operations — zero behavioural + change. +- Operators converting an existing single-cluster SM to a child-cluster + SM should create the CAPI-format Secret in the SM's namespace + *before* adding the ref, so the first reconcile sees a valid Secret. +- Token / certificate rotations driven by CAPI's control-plane provider + bump the Secret's `resourceVersion`; the next reconcile rebuilds the + child client automatically. + +### Deferred to follow-up +- **Per-child-cluster Node watch (Phase 1.9 in + `~/.claude/plans/i-have-a-very-swift-treehouse.md`).** The management + Node watch in `main.rs` is unchanged, so co-located deployments keep + their Node-event-driven responsiveness. Child-cluster Node state + changes are picked up via the periodic `TIMER_REQUEUE_SECS` requeue + and via CAPI `Machine` status changes. A subsequent change will add + lazy per-`(namespace, secret_name)` Node watchers multiplexed into + the `Controller::reconcile_on` channel via a `mpsc` receiver, closing + the event-driven gap. Tracked separately. +- **`ChildClusterReachable` status condition + Prometheus counters + (Phase 2 in the plan).** +- **Integration test against two fake clusters (Phase 1.10 in the + plan).** Resolver is unit-tested end-to-end against + `tower_test::mock`; the full reconcile-with-dual-clusters scenario + is the next test to write. + +--- + +## [2026-05-10 12:00] - Gate docs Pages deploy on successful Build workflow completion + +**Author:** Erick Bourgeois + +### Changed +- `.github/workflows/docs.yaml`: Replaced the `release: published` deploy + trigger with `workflow_run` chained to the `Build` workflow's + completion. Setup Pages, Upload Pages artifact, and the Deploy job + are now gated on + `github.event.workflow_run.event == 'release' && ...conclusion == 'success'`. The build job pins + `actions/checkout` to `github.event.workflow_run.head_sha` on + workflow_run runs (workflow_run executes in the default-branch + context, so released docs would otherwise build from `main` rather + than the tag commit). Concurrency group now keys on + `workflow_run.head_branch`; `cancel-in-progress` is disabled only + for release-triggered workflow_run events. + +### Why +A v0.2.1 docs deploy was rejected by the `github-pages` environment +protection rules and sat unpublished for ~10 hours until re-run. +Beyond the protection-rule fix, the prior trigger model published +docs whenever a release was created — independent of whether the +release's binaries, container images, and Cosign signatures actually +built cleanly. Chaining the Pages deploy to a successful `Build` run +means: a release whose artifacts fail to build no longer ships docs +claiming those artifacts exist, and end users on the published site +can trust that every visible release tag corresponds to a complete, +signed artifact set. + +### Impact +- [ ] Breaking change +- [ ] Requires cluster rollout +- [x] Config change only +- [ ] Documentation only + +Operational notes: +- Docs deploy now waits for `Build` to finish on each release + (~10–20 min added latency between release publication and Pages + going live). +- `Build` PR / push-to-main completions trigger a docs validation + run via `workflow_run` in addition to the existing + `pull_request` / `push` triggers — redundant by design (a + Build-completion sanity check); short-circuits before deploy. +- `workflow_run` triggers ignore the `paths:` filter, so every + `Build` completion shows up in the Actions log even when no docs + sources changed. + +--- + ## [2026-05-02 23:30] - Emergency-reclaim roadmap closeout: drain stopwatch + loop protection + integration tests **Author:** Erick Bourgeois diff --git a/.gitignore b/.gitignore index 74b220f..cf23c85 100644 --- a/.gitignore +++ b/.gitignore @@ -11,3 +11,5 @@ __pycache__/ # Generated CALM diagrams (rendered by `make calm-diagrams` / CI). # Source: docs/architecture/calm/templates/mermaid/*.md.hbs /docs/src/architecture/ + +.DS_Store \ No newline at end of file diff --git a/Cargo.lock b/Cargo.lock index 36bd97f..d85e5c6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -545,6 +545,7 @@ version = "0.1.1" dependencies = [ "anyhow", "async-trait", + "base64", "chrono", "chrono-tz", "clap", @@ -569,6 +570,7 @@ dependencies = [ "test-log", "thiserror 2.0.18", "tokio", + "tokio-stream", "tokio-test", "toml", "tower", diff --git a/Cargo.toml b/Cargo.toml index a860f5f..6cc0c1e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,10 +29,19 @@ name = "auto-vex-reachability" path = "src/bin/auto_vex_reachability.rs" [dependencies] -kube = { version = "3.1", features = ["runtime", "derive", "client", "ws"] } +# `unstable-runtime` is opt-in for `Controller::reconcile_on`, which we +# use in main.rs to feed per-child-cluster Node-watch events into the +# Controller. The feature is "unstable" in the SemVer sense (the API +# may change between minor kube-runtime versions) but is the canonical +# way to push external triggers into the controller loop. +kube = { version = "3.1", features = ["runtime", "derive", "client", "ws", "unstable-runtime"] } kube-lease-manager = "0.11" k8s-openapi = { version = "0.27", features = ["latest", "schemars"] } tokio = { version = "1", features = ["full"] } +# `ReceiverStream` adapter so an `mpsc::Receiver>` can be +# fed into `Controller::reconcile_on`. Used by main.rs to plumb +# child-cluster Node-watch events back into the controller. +tokio-stream = "0.1" serde = { version = "1", features = ["derive"] } serde_json = "1" serde_yaml = "0.9" @@ -79,6 +88,10 @@ test-log = "0.2" tempfile = "3" tower-test = "0.4" http = "1" +# Used by src/reconcilers/child_client_tests.rs to build base64-encoded +# Secret payloads for the tower-test mock server. base64 0.22 is already +# in the dependency graph transitively (kube / k8s-openapi pull it). +base64 = "0.22" [profile.release] opt-level = 3 diff --git a/deploy/admission/validatingadmissionpolicy.yaml b/deploy/admission/validatingadmissionpolicy.yaml index b0298db..8313903 100644 --- a/deploy/admission/validatingadmissionpolicy.yaml +++ b/deploy/admission/validatingadmissionpolicy.yaml @@ -32,6 +32,21 @@ spec: resources: ["scheduledmachines"] operations: ["CREATE", "UPDATE"] + # Derived values reused by the RBAC authorizer checks below. The group is the + # part of apiVersion before '/'; the resource is the naive lowercase plural of + # kind (lowerAscii + 's'). This mirrors resource_plural() in + # src/reconcilers/helpers.rs so the permission checked here is exactly the one + # the controller will exercise when it creates the resource. + variables: + - name: bootstrapGroup + expression: "object.spec.bootstrapSpec.apiVersion.split('/')[0]" + - name: bootstrapResource + expression: "object.spec.bootstrapSpec.kind.lowerAscii() + 's'" + - name: infraGroup + expression: "object.spec.infrastructureSpec.apiVersion.split('/')[0]" + - name: infraResource + expression: "object.spec.infrastructureSpec.kind.lowerAscii() + 's'" + validations: # ── 1. clusterName: non-empty ──────────────────────────────────────────── @@ -196,6 +211,82 @@ spec: message: "spec.infrastructureSpec.kind must not be empty" reason: Invalid + # ── 13c–13f. Embedded metadata: name/namespace are controller-owned ─────── + # The controller owns the created resource's identity: it always names the + # bootstrap/infrastructure resource after the ScheduledMachine and creates + # it in the SM's own namespace (cross-namespace creation is forbidden — + # threat T1). A user-supplied metadata.name/metadata.namespace is therefore + # rejected, not silently ignored. + # + # NOTE: this only works because EmbeddedResource's `metadata` is marked + # `x-kubernetes-preserve-unknown-fields: true` in src/crd.rs. Without it the + # API server PRUNES unknown fields before admission policies run, so the + # field would be gone before this rule could see it. The runtime + # `validate_embedded_metadata()` in src/reconcilers/helpers.rs enforces the + # same rule as defence-in-depth. Only metadata.labels / metadata.annotations + # are user-settable (and are reserved-prefix-checked at reconcile time). + - expression: "!has(object.spec.bootstrapSpec.metadata) || !has(object.spec.bootstrapSpec.metadata.namespace)" + message: >- + spec.bootstrapSpec.metadata.namespace is not permitted — the 5Spot + controller always creates the bootstrap resource in the + ScheduledMachine's own namespace + reason: Forbidden + - expression: "!has(object.spec.bootstrapSpec.metadata) || !has(object.spec.bootstrapSpec.metadata.name)" + message: >- + spec.bootstrapSpec.metadata.name is not permitted — the 5Spot controller + names the bootstrap resource after the ScheduledMachine + reason: Forbidden + - expression: "!has(object.spec.infrastructureSpec.metadata) || !has(object.spec.infrastructureSpec.metadata.namespace)" + message: >- + spec.infrastructureSpec.metadata.namespace is not permitted — the 5Spot + controller always creates the infrastructure resource in the + ScheduledMachine's own namespace + reason: Forbidden + - expression: "!has(object.spec.infrastructureSpec.metadata) || !has(object.spec.infrastructureSpec.metadata.name)" + message: >- + spec.infrastructureSpec.metadata.name is not permitted — the 5Spot + controller names the infrastructure resource after the ScheduledMachine + reason: Forbidden + + # ── 13a. RBAC: requesting user may create the bootstrapSpec resource ────── + # Privilege-escalation guard. The 5Spot controller runs with broad RBAC so + # it can create bootstrap/infrastructure/Machine objects on the user's + # behalf. Without this check, a user able to create a ScheduledMachine but + # NOT a K0sWorkerConfig could have the controller create one for them — + # escalating through the controller. We require the *requesting user* to + # independently hold `create` on the embedded bootstrap GVK in the target + # namespace, mirroring how CAPI's own webhooks gate templated resources. + # The controller-side equivalent (its own service account) is enforced at + # reconcile time by ensure_can_create() in src/reconcilers/helpers.rs. + - expression: >- + authorizer.group(variables.bootstrapGroup) + .resource(variables.bootstrapResource) + .namespace(object.metadata.namespace) + .check('create') + .allowed() + message: >- + user is not permitted to create the spec.bootstrapSpec resource type — + creating a ScheduledMachine requires 'create' permission on the + embedded bootstrap resource (RBAC) to prevent privilege escalation + through the 5Spot controller + reason: Forbidden + + # ── 13b. RBAC: requesting user may create the infrastructureSpec resource ── + # Same privilege-escalation guard as 13a, applied to the embedded + # infrastructure GVK (e.g. RemoteMachine). + - expression: >- + authorizer.group(variables.infraGroup) + .resource(variables.infraResource) + .namespace(object.metadata.namespace) + .check('create') + .allowed() + message: >- + user is not permitted to create the spec.infrastructureSpec resource + type — creating a ScheduledMachine requires 'create' permission on the + embedded infrastructure resource (RBAC) to prevent privilege escalation + through the 5Spot controller + reason: Forbidden + # ── 14. nodeTaints: key format (RFC-1123 qualified name) ────────────────── # Mirrors validate_taint_key() in src/crd.rs. The optional "/" part # is a DNS subdomain; the name part is a qualified name. Both capped at 63 diff --git a/deploy/crds/scheduledmachine.yaml b/deploy/crds/scheduledmachine.yaml index 0d2b95c..9698d5e 100644 --- a/deploy/crds/scheduledmachine.yaml +++ b/deploy/crds/scheduledmachine.yaml @@ -1,5 +1,3 @@ - Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.12s - Running `target/debug/crdgen` apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: @@ -58,6 +56,21 @@ spec: kind: description: Kind of the resource (e.g., 'K0sWorkerConfig', 'RemoteMachine') type: string + metadata: + description: Optional labels/annotations to stamp on the created resource. Only 'labels' and 'annotations' are honoured; 'name' and 'namespace' are controller-owned and rejected at admission. + properties: + annotations: + additionalProperties: + type: string + description: Annotations merged onto the created resource (reserved prefixes are rejected) + type: object + labels: + additionalProperties: + type: string + description: Labels merged onto the created resource (reserved prefixes are rejected) + type: object + type: object + x-kubernetes-preserve-unknown-fields: true spec: description: Provider-specific configuration type: object @@ -97,6 +110,21 @@ spec: kind: description: Kind of the resource (e.g., 'K0sWorkerConfig', 'RemoteMachine') type: string + metadata: + description: Optional labels/annotations to stamp on the created resource. Only 'labels' and 'annotations' are honoured; 'name' and 'namespace' are controller-owned and rejected at admission. + properties: + annotations: + additionalProperties: + type: string + description: Annotations merged onto the created resource (reserved prefixes are rejected) + type: object + labels: + additionalProperties: + type: string + description: Labels merged onto the created resource (reserved prefixes are rejected) + type: object + type: object + x-kubernetes-preserve-unknown-fields: true spec: description: Provider-specific configuration type: object @@ -137,6 +165,62 @@ spec: default: false description: When true, immediately removes the machine from cluster type: boolean + kubeconfigSecretRef: + description: |- + Reference to a Secret in this ScheduledMachine's namespace containing + a kubeconfig for the workload (child) cluster whose Node(s) this + resource manages. + + When set, every Node and Pod API call the controller makes on behalf + of this resource — cordon, taint, drain (pod list + delete), + reclaim-agent annotations / labels / ConfigMaps, status enrichment — + is routed through that kubeconfig. CAPI / bootstrap / infrastructure + / Machine objects continue to use the management cluster's in-cluster + client. + + When unset (default), the controller first tries to auto-discover a + Secret named `-kubeconfig` in this same namespace + (CAPI convention). If that Secret does not exist, the management + client is used for Node/Pod operations as well — the degenerate + single-cluster dev/test posture where management ≡ workload cluster. + + Cross-namespace Secret references are NOT supported: the Secret MUST + live in this resource's own namespace. This is a security boundary + (cross-namespace would let a tenant in one namespace read a kubeconfig + in another). + + The supplied kubeconfig MUST grant: `nodes` get/list/watch/patch and + `pods` get/list/delete in all namespaces of the child cluster, plus + `configmaps` get/create/patch/delete in the reclaim-agent namespace + if `killIfCommands` is also set. See + `docs/src/concepts/child-cluster-kubeconfig.md` for the full RBAC + requirements and threat model. + nullable: true + properties: + key: + default: value + description: |- + Key within the Secret's `data` map whose value is the kubeconfig YAML + document. Defaults to `value` — CAPI's convention for + `-kubeconfig` Secrets generated by the control-plane + provider. Common overrides: `kubeconfig` (some k0smotron flows), + `admin.conf` (kubeadm-style). + maxLength: 253 + minLength: 1 + pattern: ^[A-Za-z0-9._-]+$ + type: string + name: + description: |- + Name of the Secret. RFC-1123 DNS subdomain (max 253 chars). + The Secret MUST live in the same namespace as the ScheduledMachine — + there is no `namespace` field by design. + maxLength: 253 + minLength: 1 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ + type: string + required: + - name + type: object machineTemplate: description: |- Optional configuration for the created CAPI Machine diff --git a/docs/src/concepts/child-cluster-kubeconfig.md b/docs/src/concepts/child-cluster-kubeconfig.md new file mode 100644 index 0000000..e8a2ff6 --- /dev/null +++ b/docs/src/concepts/child-cluster-kubeconfig.md @@ -0,0 +1,175 @@ +# Child-cluster kubeconfig support + +5-Spot manages physical machines via `ScheduledMachine` custom resources. +The `ScheduledMachine`, the CAPI `Machine`, the bootstrap config, and the +infrastructure resource all live on the **management cluster**. In a +production CAPI + k0smotron / k0rdent topology, however, the actual +`Node` (and the `Pod`s scheduled onto it) live inside a **workload +(child) cluster** whose API server is reachable only via that cluster's +kubeconfig. + +This page documents how 5-Spot bridges that split: when a +`ScheduledMachine` carries a kubeconfig reference, the controller uses +the management client for `Machine` / bootstrap / infrastructure +operations, and the **child-cluster client** for every `Node` and `Pod` +operation it performs on behalf of that resource. + +## The `kubeconfigSecretRef` field + +`spec.kubeconfigSecretRef` is an optional pointer to a Secret in the +`ScheduledMachine`'s own namespace whose data contains a kubeconfig YAML +document. + +```yaml +apiVersion: 5spot.finos.org/v1alpha1 +kind: ScheduledMachine +metadata: + name: gpu-worker + namespace: hosted-cluster-alpha +spec: + clusterName: alpha + kubeconfigSecretRef: + name: alpha-kubeconfig # CAPI convention + key: value # default — CAPI writes kubeconfig under data.value + schedule: + daysOfWeek: ["mon-fri"] + hoursOfDay: ["9-17"] + timezone: America/Toronto + enabled: true + bootstrapSpec: + apiVersion: bootstrap.cluster.x-k8s.io/v1beta1 + kind: K0sWorkerConfig + spec: {} + infrastructureSpec: + apiVersion: infrastructure.cluster.x-k8s.io/v1beta1 + kind: RemoteMachine + spec: + address: 10.0.0.1 + port: 22 + user: admin +``` + +### Resolution order + +When 5-Spot needs a `Node` or `Pod` client for a `ScheduledMachine`, it +resolves the right client in this order: + +1. **Explicit `spec.kubeconfigSecretRef`.** The Secret named in the field + is read from the SM's namespace. A missing Secret or unparseable + kubeconfig **fails closed**: the reconciler does not silently fall + back to the management client. The SM goes into an error state and + backs off. +2. **Auto-discovery — `-kubeconfig`.** With no explicit ref, + the controller looks for a Secret named `-kubeconfig` + in the same namespace (CAPI's convention). If found, it's used. If + absent (404), the controller falls through silently. +3. **Management client.** When neither an explicit ref nor an + auto-discovered Secret is available, the management client is used + for `Node` / `Pod` operations as well. This is the **degenerate + single-cluster posture** — useful for dev/test where management ≡ + workload. + +### Why no cross-namespace `namespace` field + +`KubeconfigSecretRef` intentionally has only `name` and `key`. The +Secret MUST live in the `ScheduledMachine`'s own namespace. Allowing a +`namespace` field would let a tenant in one namespace point at a +privileged kubeconfig in another — a privilege-escalation surface. The +CRD enforces this via `deny_unknown_fields`: any `namespace` key in the +ref is a hard schema error, not a silent miss. + +## Required child-cluster RBAC + +The supplied kubeconfig must authenticate as a service account or user +whose ClusterRole grants: + +```yaml +- apiGroups: [""] + resources: ["nodes"] + verbs: ["get", "list", "watch", "patch"] +- apiGroups: [""] + resources: ["pods"] + verbs: ["get", "list", "delete"] +``` + +If `spec.killIfCommands` is also set, the kubeconfig additionally needs: + +```yaml +- apiGroups: [""] + resources: ["configmaps"] + resourceNames: ["reclaim-agent-"] + verbs: ["get", "create", "patch", "delete"] + # in the reclaim-agent namespace (default: `5spot-system`) +``` + +A future preflight tool (`5spot validate-kubeconfig --secret /`, +tracked in Phase 3) will run `SelfSubjectAccessReview` checks against +each of these verbs and report any gaps. + +## Cache invalidation + +5-Spot caches the built child-cluster `kube::Client` per `(namespace, +secret_name)` keyed by the Secret's `metadata.resourceVersion`. Every +reconcile GETs the Secret to compare `resourceVersion` against the +cached entry; on mismatch the client is rebuilt. This means **token / +certificate rotations driven by CAPI's control-plane provider take +effect on the next reconcile** with no additional refresh logic on +either side. The cache holds up to 256 entries and evicts the +least-recently-used entry when full +(`CHILD_CLIENT_CACHE_CAP` in `src/constants.rs`). + +## What stays on the management client + +For the avoidance of doubt, the following always use the management +cluster's client: + +- `ScheduledMachine` status patches +- CAPI `Machine` create / get / delete +- Bootstrap and infrastructure resources (`K0sWorkerConfig`, + `RemoteMachine`, etc.) +- Reads of the kubeconfig Secret itself +- Kubernetes Events +- The `kube-runtime` `Controller` driver (the watch on + `ScheduledMachine` and on CAPI `Machine`) + +What uses the child client: + +- `Node` cordon, taint apply, status enrichment, reclaim annotation + cleanup, reclaim-agent label patch +- `Pod` list and delete (drain) +- The reclaim-agent `ConfigMap` apply (the consumer `DaemonSet` runs on + workload-cluster Nodes; the ConfigMap must land there too) + +## Threat model + +- **Compromised tenant.** A tenant who can create a `ScheduledMachine` + in their own namespace can only point it at a kubeconfig Secret in + that same namespace. They cannot reach into another namespace's + Secrets via this CRD. +- **Compromised child cluster.** A compromised child kubeconfig grants + attacker access to the workload cluster's `nodes` + `pods` + (if + `killIfCommands` is set) `configmaps`. It does NOT grant management + cluster access. The blast radius is the workload cluster only. +- **Stale credentials.** A kubeconfig Secret whose token has been + rotated externally (without a CAPI Secret update) will fail at the + *child* cluster's API server, not silently succeed. 5-Spot surfaces + the failure as `ReconcilerError::ChildClusterUnreachable` and backs + off. A future Phase 2 `ChildClusterReachable=False` condition will + make this visible without log-grepping. + +## Limitations of this release + +This release ships the **client resolution + routing** layer of +multi-cluster support. The full event-driven story still has one gap: + +- **No per-child-cluster Node watch yet.** The management-cluster Node + watch (in `main.rs`) is unchanged, so co-located deployments + (management ≡ workload) keep their existing Node-event-driven + responsiveness. For child-cluster SMs, Node state changes are picked + up via the periodic `TIMER_REQUEUE_SECS` requeue and via CAPI + `Machine` status changes (which are watched on the management + cluster). Drain progress and Node-Ready transitions may therefore lag + by up to the requeue interval. A follow-up adds per-`(namespace, + secret_name)` Node watchers, lazily started on the first + reconcile that observes a kubeconfig reference, and multiplexed into + the `Controller` via `reconcile_on`. diff --git a/docs/src/reference/api.md b/docs/src/reference/api.md index 6668894..cf476e6 100644 --- a/docs/src/reference/api.md +++ b/docs/src/reference/api.md @@ -93,6 +93,16 @@ This is a fully unstructured object that must contain: The controller validates that the apiVersion belongs to an allowed bootstrap API group. +It may also include an optional `metadata` block: + +- **metadata.labels** (optional, map of string to string): merged onto the created bootstrap resource +- **metadata.annotations** (optional, map of string to string): merged onto the created bootstrap resource + +`metadata.name` and `metadata.namespace` are **not** permitted — the controller +names the resource after the ScheduledMachine and creates it in the SM's own +namespace. Labels/annotations using reserved prefixes (`5spot.finos.org/`, +`cluster.x-k8s.io/`, `kubernetes.io/`, `k8s.io/`) are rejected. + #### infrastructureSpec (required, object) Inline infrastructure configuration that will be created when the schedule is active. @@ -104,6 +114,16 @@ This is a fully unstructured object that must contain: The controller validates that the apiVersion belongs to an allowed infrastructure API group. +It may also include an optional `metadata` block: + +- **metadata.labels** (optional, map of string to string): merged onto the created infrastructure resource +- **metadata.annotations** (optional, map of string to string): merged onto the created infrastructure resource + +`metadata.name` and `metadata.namespace` are **not** permitted — the controller +names the resource after the ScheduledMachine and creates it in the SM's own +namespace. Labels/annotations using reserved prefixes (`5spot.finos.org/`, +`cluster.x-k8s.io/`, `kubernetes.io/`, `k8s.io/`) are rejected. + #### machineTemplate (optional, object) Configuration for the created CAPI Machine resource. diff --git a/docs/src/security/admission-validation.md b/docs/src/security/admission-validation.md index 12ba3cd..0cbb21f 100644 --- a/docs/src/security/admission-validation.md +++ b/docs/src/security/admission-validation.md @@ -55,7 +55,7 @@ sequenceDiagram participant etcd U ->> API : CREATE / UPDATE ScheduledMachine - API ->> VAP : Evaluate 13 CEL rules against object.spec + API ->> VAP : Evaluate structural CEL rules + RBAC authorizer checks alt All rules pass VAP -->> API : Allowed API ->> etcd : Persist resource @@ -94,6 +94,17 @@ resource) in the order listed. | 10 | `spec.infrastructureSpec.apiVersion` | Must contain `/` | `must use a namespaced API group` | | 11 | `spec.infrastructureSpec.apiVersion` | Group must be `infrastructure.cluster.x-k8s.io` or `k0smotron.io` | `must be from an allowed group` | | 12 | `spec.infrastructureSpec.kind` | Must not be empty | `spec.infrastructureSpec.kind must not be empty` | +| 13a | `spec.bootstrapSpec` (RBAC) | Requesting **user** must hold `create` on the embedded bootstrap GVK in the target namespace | `user is not permitted to create the spec.bootstrapSpec resource type …` | +| 13b | `spec.infrastructureSpec` (RBAC) | Requesting **user** must hold `create` on the embedded infrastructure GVK in the target namespace | `user is not permitted to create the spec.infrastructureSpec resource type …` | +| 13c | `spec.bootstrapSpec.metadata.namespace` | Must not be set — controller-owned | `spec.bootstrapSpec.metadata.namespace is not permitted …` | +| 13d | `spec.bootstrapSpec.metadata.name` | Must not be set — controller-owned | `spec.bootstrapSpec.metadata.name is not permitted …` | +| 13e | `spec.infrastructureSpec.metadata.namespace` | Must not be set — controller-owned | `spec.infrastructureSpec.metadata.namespace is not permitted …` | +| 13f | `spec.infrastructureSpec.metadata.name` | Must not be set — controller-owned | `spec.infrastructureSpec.metadata.name is not permitted …` | + +!!! note "nodeTaints rules" + The policy also carries structural `spec.nodeTaints` rules (key format, + length, reserved-prefix, and duplicate checks). They are omitted from the + table above for brevity — see the policy YAML for the authoritative list. ### Rule details @@ -151,6 +162,82 @@ To add a new provider, update both the `ValidatingAdmissionPolicy` (rules 8 and 11) and the constants in `src/constants.rs` (`ALLOWED_BOOTSTRAP_API_GROUPS`, `ALLOWED_INFRASTRUCTURE_API_GROUPS`). +#### Rules 13a–13b — RBAC privilege-escalation guard + +The 5Spot controller runs with broad RBAC so it can create the embedded +bootstrap, infrastructure, and CAPI `Machine` objects on the user's behalf. +Without a guard, a user who can create a `ScheduledMachine` but **not** the +embedded resource directly (e.g. a `K0sWorkerConfig`) could have the +controller create it for them — escalating privileges *through* the +controller. This is the same class of risk that Cluster API's own webhooks +address for templated resources. + +Rules 13a and 13b close this by requiring the **requesting user** to +independently hold `create` permission on the embedded bootstrap and +infrastructure GVKs in the target namespace. The policy derives the RBAC +resource from the spec using CEL `variables`: + +```yaml +variables: + - name: bootstrapGroup + expression: "object.spec.bootstrapSpec.apiVersion.split('/')[0]" + - name: bootstrapResource + expression: "object.spec.bootstrapSpec.kind.lowerAscii() + 's'" + # … infraGroup / infraResource analogous … +``` + +and then evaluates the request user's permission via the CEL `authorizer`: + +```yaml +- expression: >- + authorizer.group(variables.bootstrapGroup) + .resource(variables.bootstrapResource) + .namespace(object.metadata.namespace) + .check('create') + .allowed() + reason: Forbidden +``` + +The `lowerAscii() + 's'` pluralization mirrors `resource_plural()` in +`src/reconcilers/helpers.rs`, so the permission checked at admission is +exactly the one the controller exercises when it creates the resource. + +!!! info "Two-layer defense" + Rules 13a/13b check the **requesting user** at admission. The + controller's **own** service account is independently checked at + reconcile time by `ensure_can_create()` (a `SelfSubjectAccessReview`) + in `src/reconcilers/helpers.rs`, which fails fast with a clear + `PermissionDenied` error — naming the denied resource — instead of an + opaque `403` surfacing partway through resource creation. The two layers + together cover both *who asked* and *who acts*. + +#### Rules 13c–13f — Embedded metadata is controller-owned + +The controller owns the **identity** of every resource it creates: each +bootstrap/infrastructure resource is named after the `ScheduledMachine` and +created in the SM's **own namespace** (cross-namespace creation is forbidden — +threat T1; deletion in `remove_machine_from_cluster()` relies on the name +match). Accordingly, a user-supplied `metadata.name` or `metadata.namespace` +in `bootstrapSpec`/`infrastructureSpec` is **rejected**, not silently ignored. + +Only `metadata.labels` and `metadata.annotations` are user-settable; the +controller merges them onto the created resource after running them through +the reserved-prefix allowlist (`validate_embedded_metadata()`), so a user +cannot forge `cluster.x-k8s.io/cluster-name` (threat T2) or `5spot.finos.org/*` +keys. + +!!! warning "Why `metadata` preserves unknown fields" + For CRDs, the API server **prunes** unknown fields *before* admission + policies run — so a naive schema would silently drop + `metadata.namespace` and rules 13c–13f could never see it. To make a + *loud* rejection possible, `EmbeddedResource.metadata` is declared + `x-kubernetes-preserve-unknown-fields: true` in `src/crd.rs`, which keeps + the field around long enough for the policy (and the runtime + `validate_embedded_metadata()` backstop) to reject it. A side effect is + that other unknown `metadata.*` keys are preserved too — they are simply + ignored, since the controller constructs the resource's `metadata` from + scratch. + --- ## Deployment diff --git a/docs/src/security/threat-model.md b/docs/src/security/threat-model.md index c74118f..1a1e584 100644 --- a/docs/src/security/threat-model.md +++ b/docs/src/security/threat-model.md @@ -164,7 +164,7 @@ flowchart LR #### Tampering | ID | Threat | Likelihood | Impact | Status | |---|---|---|---|---| -| T1 | **Cross-namespace resource creation** — user sets `bootstrapSpec.namespace` to `kube-system` | High | Critical | **Mitigated (2026-04-08)** — `namespace` field removed from `EmbeddedResource`; controller always uses SM's own namespace | +| T1 | **Cross-namespace resource creation** — user sets `bootstrapSpec.metadata.namespace` to `kube-system` | High | Critical | **Mitigated (2026-04-08, hardened 2026-05-30)** — controller always uses the SM's own namespace; `metadata.namespace`/`metadata.name` are now **loudly rejected** at admission (VAP rules 13c–13f) and at reconcile (`validate_embedded_metadata()`). Only `metadata.labels`/`metadata.annotations` are accepted, reserved-prefix-checked | | T2 | **Label injection** — user sets `machineTemplate.labels["cluster.x-k8s.io/cluster-name"]` to redirect machine to attacker-controlled cluster | High | Critical | **Mitigated (2026-04-08)** — `validate_labels()` blocks reserved prefixes | | T3 | **Annotation injection** — user injects `kubectl.kubernetes.io/restartedAt` to trigger rolling restarts | Medium | Medium | **Mitigated (2026-04-08)** — same prefix allowlist | | T4 | **apiVersion/kind injection** — user sets `bootstrapSpec.kind: ClusterRole` to create RBAC resources | High | High | **Mitigated (2026-04-08)** — `validate_api_group()` enforces allowlist | @@ -198,6 +198,7 @@ flowchart LR | ID | Threat | Likelihood | Impact | Status | |---|---|---|---|---| | E1 | Attacker uses `apiVersion: rbac.authorization.k8s.io/v1, kind: ClusterRole` to create RBAC resources via controller | High | Critical | **Mitigated (2026-04-08)** — API group allowlist | +| E1a | **Escalation through the controller** — user who can create a `ScheduledMachine` but **not** the embedded bootstrap/infrastructure resource has the broadly-permissioned controller create it on their behalf | High | High | **Mitigated (2026-05-29)** — VAP `authorizer` rules (13a/13b) require the *requesting user* to hold `create` on the embedded GVKs; controller's own SA independently gated at reconcile by `ensure_can_create()` (`SelfSubjectAccessReview`) | | E2 | **Compromised controller pod** gains cluster-wide node cordon/drain, CAPI write access | Medium | Critical | **Partially mitigated** — k0smotron.io RBAC narrowed; bootstrap/infra still use wildcards (provider-agnostic requirement) | | E3 | Controller service account token stolen from pod filesystem | Low | Critical | **Mitigated** — read-only root filesystem; token mounted at standard path (Kubernetes default); no projected service account with long lifetime | diff --git a/examples/scheduledmachine-child-cluster.yaml b/examples/scheduledmachine-child-cluster.yaml new file mode 100644 index 0000000..8d1082e --- /dev/null +++ b/examples/scheduledmachine-child-cluster.yaml @@ -0,0 +1,70 @@ +# Example ScheduledMachine targeting a CAPI / k0smotron child cluster. +# +# The ScheduledMachine, the CAPI Machine, the K0sWorkerConfig, and the +# RemoteMachine all live on the *management* cluster. The Node and Pod +# objects the controller manages on behalf of this resource live inside +# the *workload* (child) cluster. +# +# spec.kubeconfigSecretRef points 5-Spot at the workload cluster's +# kubeconfig — stored as a Secret in this same namespace, in the format +# CAPI's control-plane provider writes by default (`-kubeconfig`, +# with the kubeconfig YAML under `data.value`). +# +# Required child-cluster RBAC (must be granted to whoever the kubeconfig +# authenticates as): +# nodes: get, list, watch, patch +# pods: get, list, delete +# configmaps (in `5spot-system`): get, create, patch, delete +# — only when spec.killIfCommands is also set +# +# See docs/src/concepts/child-cluster-kubeconfig.md for the full +# resolution order, threat model, and limitations. + +apiVersion: 5spot.finos.org/v1alpha1 +kind: ScheduledMachine +metadata: + name: gpu-worker + namespace: hosted-cluster-alpha +spec: + clusterName: alpha + + # Explicit ref. Could be omitted: 5-Spot would then auto-discover + # the Secret `-kubeconfig` (`alpha-kubeconfig`) in this + # namespace. The explicit form is documented here for clarity. + kubeconfigSecretRef: + name: alpha-kubeconfig + key: value # default; CAPI convention + + schedule: + daysOfWeek: + - mon-fri + hoursOfDay: + - "9-17" + timezone: America/Toronto + enabled: true + + bootstrapSpec: + apiVersion: bootstrap.cluster.x-k8s.io/v1beta1 + kind: K0sWorkerConfig + spec: + version: v1.30.0+k0s.0 + + infrastructureSpec: + apiVersion: infrastructure.cluster.x-k8s.io/v1beta1 + kind: RemoteMachine + spec: + address: 10.0.0.1 + port: 22 + user: admin + + machineTemplate: + labels: + node-role.kubernetes.io/worker: "" + 5spot.finos.org/target-cluster: alpha + annotations: + 5spot.finos.org/scheduled-by: gpu-worker + + priority: 50 + gracefulShutdownTimeout: 5m + nodeDrainTimeout: 5m + killSwitch: false diff --git a/src/bin/crddoc.rs b/src/bin/crddoc.rs index f72ec13..5141f1f 100644 --- a/src/bin/crddoc.rs +++ b/src/bin/crddoc.rs @@ -133,6 +133,16 @@ fn main() { "The controller validates that the apiVersion belongs to an allowed bootstrap API group." ); println!(); + println!("It may also include an optional `metadata` block:"); + println!(); + println!("- **metadata.labels** (optional, map of string to string): merged onto the created bootstrap resource"); + println!("- **metadata.annotations** (optional, map of string to string): merged onto the created bootstrap resource"); + println!(); + println!("`metadata.name` and `metadata.namespace` are **not** permitted — the controller"); + println!("names the resource after the ScheduledMachine and creates it in the SM's own"); + println!("namespace. Labels/annotations using reserved prefixes (`5spot.finos.org/`,"); + println!("`cluster.x-k8s.io/`, `kubernetes.io/`, `k8s.io/`) are rejected."); + println!(); println!("#### infrastructureSpec"); println!(); println!("(required, object) Inline infrastructure configuration that will be created when the schedule is active."); @@ -144,6 +154,16 @@ fn main() { println!(); println!("The controller validates that the apiVersion belongs to an allowed infrastructure API group."); println!(); + println!("It may also include an optional `metadata` block:"); + println!(); + println!("- **metadata.labels** (optional, map of string to string): merged onto the created infrastructure resource"); + println!("- **metadata.annotations** (optional, map of string to string): merged onto the created infrastructure resource"); + println!(); + println!("`metadata.name` and `metadata.namespace` are **not** permitted — the controller"); + println!("names the resource after the ScheduledMachine and creates it in the SM's own"); + println!("namespace. Labels/annotations using reserved prefixes (`5spot.finos.org/`,"); + println!("`cluster.x-k8s.io/`, `kubernetes.io/`, `k8s.io/`) are rejected."); + println!(); println!("#### machineTemplate"); println!(); println!("(optional, object) Configuration for the created CAPI Machine resource."); diff --git a/src/constants.rs b/src/constants.rs index 3858d73..87e7a17 100644 --- a/src/constants.rs +++ b/src/constants.rs @@ -206,6 +206,28 @@ pub const MAX_BACKOFF_SECS: u64 = 300; /// Maximum number of per-resource reconciliation retries before capping at [`MAX_BACKOFF_SECS`] pub const MAX_RECONCILE_RETRIES: u32 = 10; +/// Buffer size for the mpsc channel that carries +/// `ObjectRef` from per-child-cluster Node watchers +/// into the management cluster's `Controller::reconcile_on` stream. +/// +/// One channel is shared across every child watcher. Bursts at watcher +/// init time (full Node list) plus sporadic per-event sends afterwards +/// fit comfortably under 1024 even with dozens of child clusters and +/// large Node counts; the bound exists to apply back-pressure if a +/// pathological burst (or a stuck controller) ever surfaces, not as a +/// steady-state ceiling. +pub const CHILD_NODE_EVENT_CHANNEL_CAP: usize = 1024; + +/// Maximum number of cached child-cluster `kube::Client` instances kept in +/// memory by [`crate::reconcilers::child_client::ChildClientCache`]. +/// +/// One entry per unique `(namespace, secret_name)`. 256 is well above any +/// realistic per-controller workload — a single management cluster hosting +/// hundreds of CAPI-managed workload clusters still fits. Above the cap, +/// the least-recently-used entry is evicted; the next reconcile referencing +/// the evicted Secret simply rebuilds, paying one kubeconfig-parse round. +pub const CHILD_CLIENT_CACHE_CAP: usize = 256; + // ============================================================================ // Leader Election Constants // ============================================================================ @@ -249,6 +271,17 @@ pub const CONDITION_TYPE_REFERENCES_VALID: &str = "ReferencesValid"; /// See `~/dev/roadmaps/completed-5spot-user-defined-node-taints.md` Phase 2. pub const CONDITION_TYPE_NODE_TAINTED: &str = "NodeTainted"; +/// `ChildClusterReachable` condition type — tracks whether the +/// child-cluster kubeconfig (`spec.kubeconfigSecretRef` or +/// auto-discovered `-kubeconfig`) resolved into a usable +/// `kube::Client`. `True` after a successful resolve; `False` after +/// any [`crate::reconcilers::ReconcilerError::KubeconfigSecretMissingKey`], +/// [`crate::reconcilers::ReconcilerError::KubeconfigInvalid`], or +/// [`crate::reconcilers::ReconcilerError::ChildClusterUnreachable`]. +/// Single-cluster (management-fallback) SMs may omit the condition or +/// set it to `Unknown`. +pub const CONDITION_TYPE_CHILD_CLUSTER_REACHABLE: &str = "ChildClusterReachable"; + // ============================================================================ // Condition Statuses // ============================================================================ @@ -442,6 +475,13 @@ pub const CAPI_MACHINE_API_VERSION_FULL: &str = "cluster.x-k8s.io/v1beta1"; /// CAPI cluster name label pub const CAPI_CLUSTER_NAME_LABEL: &str = "cluster.x-k8s.io/cluster-name"; +/// Label stamped by the controller on every derived resource (bootstrap, +/// infrastructure, CAPI `Machine`) linking it back to its owning +/// `ScheduledMachine`. Uses the reserved `5spot.finos.org/` prefix so users +/// cannot set or override it via embedded `metadata.labels` (blocked by +/// [`RESERVED_LABEL_PREFIXES`]). +pub const SCHEDULED_MACHINE_LABEL: &str = "5spot.finos.org/scheduled-machine"; + /// CAPI Machine resource plural name pub const CAPI_RESOURCE_MACHINES: &str = "machines"; @@ -561,3 +601,10 @@ pub const ALLOWED_BOOTSTRAP_API_GROUPS: &[&str] = &["bootstrap.cluster.x-k8s.io" /// Allowed API groups for infrastructure embedded resources pub const ALLOWED_INFRASTRUCTURE_API_GROUPS: &[&str] = &["infrastructure.cluster.x-k8s.io", "k0smotron.io"]; + +/// RBAC verb used for pre-flight `SelfSubjectAccessReview` checks issued before +/// the controller creates an embedded bootstrap/infrastructure resource or the +/// CAPI `Machine`. The controller verifies its own service account is permitted +/// to `create` each resource type so an RBAC gap surfaces as a clear +/// `PermissionDenied` error instead of an opaque 403 mid-creation. +pub const RBAC_VERB_CREATE: &str = "create"; diff --git a/src/crd.rs b/src/crd.rs index 43831e4..8b7d281 100644 --- a/src/crd.rs +++ b/src/crd.rs @@ -120,6 +120,37 @@ pub struct ScheduledMachineSpec { #[serde(skip_serializing_if = "Option::is_none", default)] #[schemars(schema_with = "kill_if_commands_schema")] pub kill_if_commands: Option>, + + /// Reference to a Secret in this ScheduledMachine's namespace containing + /// a kubeconfig for the workload (child) cluster whose Node(s) this + /// resource manages. + /// + /// When set, every Node and Pod API call the controller makes on behalf + /// of this resource — cordon, taint, drain (pod list + delete), + /// reclaim-agent annotations / labels / ConfigMaps, status enrichment — + /// is routed through that kubeconfig. CAPI / bootstrap / infrastructure + /// / Machine objects continue to use the management cluster's in-cluster + /// client. + /// + /// When unset (default), the controller first tries to auto-discover a + /// Secret named `-kubeconfig` in this same namespace + /// (CAPI convention). If that Secret does not exist, the management + /// client is used for Node/Pod operations as well — the degenerate + /// single-cluster dev/test posture where management ≡ workload cluster. + /// + /// Cross-namespace Secret references are NOT supported: the Secret MUST + /// live in this resource's own namespace. This is a security boundary + /// (cross-namespace would let a tenant in one namespace read a kubeconfig + /// in another). + /// + /// The supplied kubeconfig MUST grant: `nodes` get/list/watch/patch and + /// `pods` get/list/delete in all namespaces of the child cluster, plus + /// `configmaps` get/create/patch/delete in the reclaim-agent namespace + /// if `killIfCommands` is also set. See + /// `docs/src/concepts/child-cluster-kubeconfig.md` for the full RBAC + /// requirements and threat model. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub kubeconfig_secret_ref: Option, } fn default_priority() -> u8 { @@ -250,6 +281,47 @@ impl EmbeddedResource { self.0.get("spec") } + /// Get `metadata.namespace` if the user set one. + /// + /// This is **controller-owned** and must never be honoured — the controller + /// always creates the resource in the `ScheduledMachine`'s own namespace. + /// Used by `validate_embedded_metadata` to reject the field loudly. + #[must_use] + pub fn metadata_namespace(&self) -> Option<&str> { + self.0 + .get("metadata") + .and_then(|m| m.get("namespace")) + .and_then(Value::as_str) + } + + /// Get `metadata.name` if the user set one. + /// + /// Controller-owned: the created resource is always named after the + /// `ScheduledMachine` (deletion relies on this), so a user-supplied name is + /// rejected rather than silently overridden. + #[must_use] + pub fn metadata_name(&self) -> Option<&str> { + self.0 + .get("metadata") + .and_then(|m| m.get("name")) + .and_then(Value::as_str) + } + + /// Get `metadata.labels` as a string map (empty if absent or malformed). + /// + /// Non-string label values are skipped — the CRD schema already constrains + /// values to strings, so this is a defensive narrowing for raw input. + #[must_use] + pub fn metadata_labels(&self) -> BTreeMap { + embedded_string_map(self.0.get("metadata").and_then(|m| m.get("labels"))) + } + + /// Get `metadata.annotations` as a string map (empty if absent or malformed). + #[must_use] + pub fn metadata_annotations(&self) -> BTreeMap { + embedded_string_map(self.0.get("metadata").and_then(|m| m.get("annotations"))) + } + /// Get the inner JSON value #[must_use] pub fn inner(&self) -> &Value { @@ -257,6 +329,19 @@ impl EmbeddedResource { } } +/// Narrow an optional JSON value to a `BTreeMap`, keeping only +/// string-valued entries. Returns an empty map for `None` or non-object values. +fn embedded_string_map(value: Option<&Value>) -> BTreeMap { + value + .and_then(Value::as_object) + .map(|obj| { + obj.iter() + .filter_map(|(k, v)| v.as_str().map(|s| (k.clone(), s.to_string()))) + .collect() + }) + .unwrap_or_default() +} + /// Schema for the timezone field — bounded string to prevent log injection fn timezone_schema(_: &mut schemars::SchemaGenerator) -> schemars::Schema { schemars::json_schema!({ @@ -295,9 +380,61 @@ fn kill_if_commands_schema(_: &mut schemars::SchemaGenerator) -> schemars::Schem }) } +/// Schema for `KubeconfigSecretRef.name` — bounded to RFC-1123 DNS subdomain +/// length (253 chars) with the Kubernetes-standard charset. Matches the bound +/// the Kubernetes API server itself enforces on Secret names, so a value that +/// fits this schema is guaranteed to be a syntactically valid Secret name. +fn kubeconfig_secret_name_schema(_: &mut schemars::SchemaGenerator) -> schemars::Schema { + schemars::json_schema!({ + "type": "string", + "minLength": 1, + "maxLength": 253, + "pattern": "^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$" + }) +} + +/// Schema for `KubeconfigSecretRef.key` — bounded to the same 253-char cap as +/// the name, with the Secret-data key charset (alphanumerics, `.`, `-`, `_`). +/// The bound prevents pathologically long keys from inflating the Secret GET +/// path; the charset matches what the Kubernetes API server enforces on Secret +/// `data` keys. +fn kubeconfig_secret_key_schema(_: &mut schemars::SchemaGenerator) -> schemars::Schema { + schemars::json_schema!({ + "type": "string", + "minLength": 1, + "maxLength": 253, + "pattern": "^[A-Za-z0-9._-]+$" + }) +} + +/// Default key for `KubeconfigSecretRef.key` — CAPI convention is to store the +/// kubeconfig YAML under `data.value` in a Secret named `-kubeconfig`. +fn default_kubeconfig_secret_key() -> String { + "value".to_string() +} + /// Schema for `EmbeddedResource` — requires apiVersion, kind, and spec fields. /// The `spec` field uses `x-kubernetes-preserve-unknown-fields` to allow any /// provider-specific fields (`K0sWorkerConfig`, `RemoteMachine`, `AWSMachine`, etc.). +/// +/// # `metadata` — labels/annotations only; name/namespace are controller-owned +/// +/// `metadata` accepts **only** `labels` and `annotations` (string maps), which +/// the controller merges onto the created resource (after applying the +/// reserved-prefix allowlist, so users cannot forge `cluster.x-k8s.io/*` or +/// `5spot.finos.org/*` keys). `metadata.name` and `metadata.namespace` are +/// **not** valid — the controller owns the resource identity, always naming the +/// resource after the `ScheduledMachine` and creating it in the SM's own +/// namespace. +/// +/// `metadata` is marked `x-kubernetes-preserve-unknown-fields: true` **on +/// purpose**: without it the API server would silently *prune* an unknown +/// `metadata.namespace`/`metadata.name` before any admission policy runs, +/// making a loud rejection impossible. Preserving the subtree lets the +/// `ValidatingAdmissionPolicy` (and the runtime `validate_embedded_metadata` +/// check) see and explicitly reject those fields. The pruned-vs-rejected +/// distinction is a documented CRD behaviour — see +/// . fn embedded_resource_schema(_: &mut schemars::SchemaGenerator) -> schemars::Schema { schemars::json_schema!({ "type": "object", @@ -311,6 +448,23 @@ fn embedded_resource_schema(_: &mut schemars::SchemaGenerator) -> schemars::Sche "type": "string", "description": "Kind of the resource (e.g., 'K0sWorkerConfig', 'RemoteMachine')" }, + "metadata": { + "type": "object", + "x-kubernetes-preserve-unknown-fields": true, + "description": "Optional labels/annotations to stamp on the created resource. Only 'labels' and 'annotations' are honoured; 'name' and 'namespace' are controller-owned and rejected at admission.", + "properties": { + "labels": { + "type": "object", + "additionalProperties": { "type": "string" }, + "description": "Labels merged onto the created resource (reserved prefixes are rejected)" + }, + "annotations": { + "type": "object", + "additionalProperties": { "type": "string" }, + "description": "Annotations merged onto the created resource (reserved prefixes are rejected)" + } + } + }, "spec": { "type": "object", "x-kubernetes-preserve-unknown-fields": true, @@ -350,6 +504,39 @@ pub struct MachineTemplateSpec { pub annotations: BTreeMap, } +// ============================================================================ +// KubeconfigSecretRef - Reference to a child-cluster kubeconfig Secret +// ============================================================================ + +/// Pointer to a Secret in the ScheduledMachine's own namespace whose data +/// contains a kubeconfig for the workload (child) cluster. +/// +/// See [`ScheduledMachineSpec::kubeconfig_secret_ref`] for the full semantics, +/// including resolution order (explicit → auto-discover `-kubeconfig` +/// → management fallback) and the RBAC requirements on the supplied kubeconfig. +/// +/// `deny_unknown_fields` is intentional: a typo like `nameSpace` or an attempt +/// to add a cross-namespace `namespace` field must be a hard error, not a +/// silent miss. Cross-namespace Secret refs are forbidden by design. +#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema, PartialEq, Eq)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub struct KubeconfigSecretRef { + /// Name of the Secret. RFC-1123 DNS subdomain (max 253 chars). + /// The Secret MUST live in the same namespace as the ScheduledMachine — + /// there is no `namespace` field by design. + #[schemars(schema_with = "kubeconfig_secret_name_schema")] + pub name: String, + + /// Key within the Secret's `data` map whose value is the kubeconfig YAML + /// document. Defaults to `value` — CAPI's convention for + /// `-kubeconfig` Secrets generated by the control-plane + /// provider. Common overrides: `kubeconfig` (some k0smotron flows), + /// `admin.conf` (kubeadm-style). + #[serde(default = "default_kubeconfig_secret_key")] + #[schemars(schema_with = "kubeconfig_secret_key_schema")] + pub key: String, +} + // ============================================================================ // NodeTaint / TaintEffect - User-declared taints on the provisioned Node // ============================================================================ diff --git a/src/crd_tests.rs b/src/crd_tests.rs index 2227901..a32d86c 100644 --- a/src/crd_tests.rs +++ b/src/crd_tests.rs @@ -314,6 +314,7 @@ mod tests { kill_switch: false, node_taints: vec![], kill_if_commands: None, + kubeconfig_secret_ref: None, }; // Test that it serializes without errors @@ -321,6 +322,11 @@ mod tests { assert!(json_output.contains("mon-fri")); assert!(json_output.contains("192.168.1.100")); assert!(json_output.contains("bootstrap")); + // skip_serializing_if = Option::is_none must elide the field entirely. + assert!( + !json_output.contains("kubeconfigSecretRef"), + "kubeconfigSecretRef must be omitted when None to preserve clean YAML for single-cluster SMs" + ); } #[test] @@ -530,6 +536,7 @@ mod tests { kill_switch: false, node_taints: vec![], kill_if_commands: None, + kubeconfig_secret_ref: None, } } @@ -1074,4 +1081,295 @@ mod tests { "error should explain: {err}" ); } + + // ======================================================================== + // KubeconfigSecretRef — child-cluster kubeconfig reference tests (TDD) + // + // These tests pin the contract for the new optional field that points a + // ScheduledMachine at a child-cluster kubeconfig Secret in its own + // namespace. The CRD type is the source of truth; YAML schema bounds and + // serde defaults are asserted here so a regression flips a test. + // ======================================================================== + + #[test] + fn test_kubeconfig_secret_ref_default_key_is_value() { + // Posit: when `key` is omitted in JSON, serde fills in the CAPI + // convention default ("value"). This keeps the common case zero-config. + let json = serde_json::json!({ "name": "alpha-kubeconfig" }); + let parsed: KubeconfigSecretRef = serde_json::from_value(json) + .expect("KubeconfigSecretRef must accept a JSON object with only `name`"); + assert_eq!(parsed.name, "alpha-kubeconfig"); + assert_eq!( + parsed.key, "value", + "default key must be `value` per CAPI's -kubeconfig convention" + ); + } + + #[test] + fn test_kubeconfig_secret_ref_explicit_key_round_trips() { + let original = KubeconfigSecretRef { + name: "team-a-kubeconfig".to_string(), + key: "kubeconfig".to_string(), + }; + let serialized = serde_json::to_value(&original).expect("must serialize"); + let parsed: KubeconfigSecretRef = + serde_json::from_value(serialized.clone()).expect("must round-trip"); + assert_eq!(parsed.name, "team-a-kubeconfig"); + assert_eq!(parsed.key, "kubeconfig"); + // camelCase JSON field names are required for kube-rs / OpenAPI conformance. + let s = serialized.to_string(); + assert!(s.contains("\"name\""), "expected camelCase `name`: {s}"); + assert!(s.contains("\"key\""), "expected camelCase `key`: {s}"); + } + + #[test] + fn test_kubeconfig_secret_ref_rejects_unknown_fields() { + // `deny_unknown_fields` prevents typo-driven silent failures, e.g. + // `nameSpace` (capital S) or `Name` (capital N) being ignored. + let json = serde_json::json!({ + "name": "alpha-kubeconfig", + "key": "value", + "namespace": "team-a" // not allowed — cross-namespace refs are forbidden + }); + let result: Result = serde_json::from_value(json); + assert!( + result.is_err(), + "KubeconfigSecretRef must reject unknown fields (no cross-namespace, no typos)" + ); + } + + #[test] + fn test_kubeconfig_secret_ref_name_required() { + // `name` has no default — omitting it must error. + let json = serde_json::json!({ "key": "value" }); + let result: Result = serde_json::from_value(json); + assert!( + result.is_err(), + "KubeconfigSecretRef must require `name` (no default)" + ); + } + + #[test] + fn test_spec_with_kubeconfig_secret_ref_round_trips() { + use serde_json::json; + + let spec = ScheduledMachineSpec { + schedule: ScheduleSpec { + days_of_week: vec!["mon-fri".to_string()], + hours_of_day: vec!["9-17".to_string()], + timezone: "UTC".to_string(), + enabled: true, + }, + cluster_name: "alpha".to_string(), + bootstrap_spec: EmbeddedResource(json!({ + "apiVersion": "bootstrap.cluster.x-k8s.io/v1beta1", + "kind": "K0sWorkerConfig", + "spec": {} + })), + infrastructure_spec: EmbeddedResource(json!({ + "apiVersion": "infrastructure.cluster.x-k8s.io/v1beta1", + "kind": "RemoteMachine", + "spec": {"address": "10.0.0.1", "port": 22} + })), + machine_template: None, + priority: 50, + graceful_shutdown_timeout: "5m".to_string(), + node_drain_timeout: "5m".to_string(), + kill_switch: false, + node_taints: vec![], + kill_if_commands: None, + kubeconfig_secret_ref: Some(KubeconfigSecretRef { + name: "alpha-kubeconfig".to_string(), + key: "value".to_string(), + }), + }; + let s = serde_json::to_string(&spec).expect("must serialize"); + assert!(s.contains("kubeconfigSecretRef")); + assert!(s.contains("alpha-kubeconfig")); + let parsed: ScheduledMachineSpec = + serde_json::from_str(&s).expect("must round-trip with kubeconfigSecretRef"); + let r = parsed + .kubeconfig_secret_ref + .expect("ref must survive round-trip"); + assert_eq!(r.name, "alpha-kubeconfig"); + assert_eq!(r.key, "value"); + } + + // ---- Schema-bound assertions ---- + + #[test] + fn test_kubeconfig_secret_ref_name_schema_is_bounded() { + // Schema must enforce RFC-1123 DNS subdomain bounds on `name` so a + // malicious / typo'd CR can't trigger an unbounded Secret GET. + let schema = serde_json::to_value(schemars::schema_for!(KubeconfigSecretRef)) + .expect("schema serializes"); + let name_schema = schema + .pointer("/properties/name") + .or_else(|| schema.pointer("/definitions/KubeconfigSecretRef/properties/name")) + .expect("KubeconfigSecretRef.name property must exist in schema"); + assert!( + name_schema.get("maxLength").is_some(), + "name must have maxLength bound (RFC-1123 DNS subdomain = 253): {name_schema}" + ); + assert!( + name_schema.get("pattern").is_some(), + "name must constrain charset via pattern: {name_schema}" + ); + } + + #[test] + fn test_kubeconfig_secret_ref_key_schema_is_bounded() { + let schema = serde_json::to_value(schemars::schema_for!(KubeconfigSecretRef)) + .expect("schema serializes"); + let key_schema = schema + .pointer("/properties/key") + .or_else(|| schema.pointer("/definitions/KubeconfigSecretRef/properties/key")) + .expect("KubeconfigSecretRef.key property must exist in schema"); + assert!( + key_schema.get("maxLength").is_some(), + "key must have maxLength bound: {key_schema}" + ); + assert!( + key_schema.get("minLength").is_some(), + "key must have minLength bound to forbid empty-string keys: {key_schema}" + ); + } + + #[test] + fn test_spec_kubeconfig_secret_ref_is_optional_in_schema() { + // Backward compat: the new field must NOT be in the `required` list. + let schema = + serde_json::to_value(schemars::schema_for!(ScheduledMachineSpec)).expect("serializes"); + let required = schema + .pointer("/required") + .or_else(|| schema.pointer("/definitions/ScheduledMachineSpec/required")) + .and_then(|v| v.as_array()) + .cloned() + .unwrap_or_default(); + assert!( + !required.iter().any(|v| v == "kubeconfigSecretRef"), + "kubeconfigSecretRef MUST be optional to preserve backward compatibility with existing SMs: required={required:?}" + ); + } + + // ======================================================================== + // EmbeddedResource — metadata accessors + // ======================================================================== + + #[test] + fn test_embedded_metadata_namespace_present() { + use serde_json::json; + let e = EmbeddedResource(json!({ + "apiVersion": "bootstrap.cluster.x-k8s.io/v1beta1", + "kind": "K0sWorkerConfig", + "metadata": { "namespace": "kube-system" }, + "spec": {} + })); + assert_eq!(e.metadata_namespace(), Some("kube-system")); + } + + #[test] + fn test_embedded_metadata_namespace_absent() { + use serde_json::json; + let e = EmbeddedResource(json!({ + "apiVersion": "bootstrap.cluster.x-k8s.io/v1beta1", + "kind": "K0sWorkerConfig", + "spec": {} + })); + assert_eq!(e.metadata_namespace(), None); + } + + #[test] + fn test_embedded_metadata_name_present() { + use serde_json::json; + let e = EmbeddedResource(json!({ + "kind": "K0sWorkerConfig", + "metadata": { "name": "evil-name" }, + "spec": {} + })); + assert_eq!(e.metadata_name(), Some("evil-name")); + } + + #[test] + fn test_embedded_metadata_labels_extracted() { + use serde_json::json; + let e = EmbeddedResource(json!({ + "kind": "K0sWorkerConfig", + "metadata": { "labels": { "team": "payments", "env": "prod" } }, + "spec": {} + })); + let labels = e.metadata_labels(); + assert_eq!(labels.get("team").map(String::as_str), Some("payments")); + assert_eq!(labels.get("env").map(String::as_str), Some("prod")); + assert_eq!(labels.len(), 2); + } + + #[test] + fn test_embedded_metadata_labels_skips_non_string_values() { + use serde_json::json; + // Defensive: non-string values are narrowed out rather than panicking. + let e = EmbeddedResource(json!({ + "kind": "K0sWorkerConfig", + "metadata": { "labels": { "ok": "yes", "bad": 7 } }, + "spec": {} + })); + let labels = e.metadata_labels(); + assert_eq!(labels.get("ok").map(String::as_str), Some("yes")); + assert!( + !labels.contains_key("bad"), + "non-string value must be skipped" + ); + } + + #[test] + fn test_embedded_metadata_annotations_empty_when_absent() { + use serde_json::json; + let e = EmbeddedResource(json!({ "kind": "K0sWorkerConfig", "spec": {} })); + assert!(e.metadata_annotations().is_empty()); + } + + // ======================================================================== + // EmbeddedResource — schema shape (metadata labels/annotations only) + // ======================================================================== + + fn embedded_schema_json() -> serde_json::Value { + let schema = embedded_resource_schema(&mut schemars::SchemaGenerator::default()); + serde_json::to_value(schema).expect("schema should serialise") + } + + #[test] + fn test_embedded_schema_metadata_allows_labels_and_annotations() { + let schema = embedded_schema_json(); + let meta_props = schema + .pointer("/properties/metadata/properties") + .and_then(|v| v.as_object()) + .expect("metadata must declare typed properties"); + assert!( + meta_props.contains_key("labels"), + "metadata.labels must be allowed" + ); + assert!( + meta_props.contains_key("annotations"), + "metadata.annotations must be allowed" + ); + // name/namespace are intentionally NOT declared — they are rejected at + // admission/runtime, not silently accepted. + assert!(!meta_props.contains_key("namespace")); + assert!(!meta_props.contains_key("name")); + } + + #[test] + fn test_embedded_schema_metadata_preserves_unknown_fields() { + // preserve-unknown is REQUIRED so the API server does not prune an + // unknown metadata.namespace before admission policies can reject it. + let schema = embedded_schema_json(); + let preserve = schema + .pointer("/properties/metadata/x-kubernetes-preserve-unknown-fields") + .and_then(serde_json::Value::as_bool); + assert_eq!( + preserve, + Some(true), + "metadata must preserve unknown fields so namespace/name can be rejected, not pruned" + ); + } } diff --git a/src/main.rs b/src/main.rs index 5d4619e..0f3bf5a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -18,10 +18,10 @@ use std::sync::Arc; use anyhow::Result; use clap::Parser; use five_spot::constants::{ - CAPI_GROUP, CAPI_MACHINE_API_VERSION, CAPI_RESOURCE_MACHINES, DEFAULT_LEASE_DURATION_SECS, - DEFAULT_LEASE_GRACE_SECS, DEFAULT_LEASE_NAME, DEFAULT_LEASE_NAMESPACE, - DEFAULT_LEASE_RENEW_DEADLINE_SECS, DEFAULT_LEASE_RETRY_PERIOD_SECS, HEALTH_PORT, - K8S_API_TIMEOUT_SECS, METRICS_PORT, + CAPI_GROUP, CAPI_MACHINE_API_VERSION, CAPI_RESOURCE_MACHINES, CHILD_NODE_EVENT_CHANNEL_CAP, + DEFAULT_LEASE_DURATION_SECS, DEFAULT_LEASE_GRACE_SECS, DEFAULT_LEASE_NAME, + DEFAULT_LEASE_NAMESPACE, DEFAULT_LEASE_RENEW_DEADLINE_SECS, DEFAULT_LEASE_RETRY_PERIOD_SECS, + HEALTH_PORT, K8S_API_TIMEOUT_SECS, METRICS_PORT, }; use five_spot::crd::ScheduledMachine; use five_spot::health::{start_health_server, HealthState}; @@ -29,7 +29,7 @@ use five_spot::labels::LABEL_SCHEDULED_MACHINE; use five_spot::metrics::init_controller_info; use five_spot::reconcilers::{ error_policy, machine_to_scheduled_machine, node_to_scheduled_machines_via_machine, - reconcile_scheduled_machine, Context, + reconcile_scheduled_machine, ChildNodeWatchManager, Context, }; use futures::{StreamExt, TryStreamExt}; use k8s_openapi::api::core::v1::Node; @@ -324,6 +324,27 @@ async fn main() -> Result<()> { } }); + // Child-cluster Node-watch plumbing. + // + // Per-child-cluster Node events (cordon, NotReady, taint drift) must + // trigger a reconcile of the owning ScheduledMachine. We build one + // `ChildNodeWatchManager` shared across all child clusters; the + // cache (`Context.child_clients`) drives it via the `ChildWatchHook` + // trait, spawning one Tokio task per `CacheKey`. Mapped + // `ObjectRef`s flow over this mpsc into the + // Controller's `reconcile_on` stream. + // + // The buffer size (`CHILD_NODE_EVENT_CHANNEL_CAP`) is sized for + // burst tolerance, not steady-state. See the constant's rustdoc. + let (child_node_tx, child_node_rx) = tokio::sync::mpsc::channel(CHILD_NODE_EVENT_CHANNEL_CAP); + let child_watch_manager = ChildNodeWatchManager::new(child_node_tx, machine_store.clone()); + // Install the manager on the cache *after* `Context::new` so the + // first `resolve()` call from any reconcile already sees a live + // hook. Cheap clone — only the inner `Arc` is moved. + context + .child_clients + .set_hook(Arc::new(child_watch_manager)); + let controller = Controller::new(scheduled_machines, Config::default()); controller @@ -343,6 +364,7 @@ async fn main() -> Result<()> { machines_snapshot.iter().map(std::convert::AsRef::as_ref), ) }) + .reconcile_on(tokio_stream::wrappers::ReceiverStream::new(child_node_rx)) .shutdown_on_signal() .run(reconcile_scheduled_machine, error_policy, context) .for_each(|res| async move { diff --git a/src/metrics.rs b/src/metrics.rs index f5dc0f9..2a95d81 100644 --- a/src/metrics.rs +++ b/src/metrics.rs @@ -390,6 +390,61 @@ pub static FINALIZER_CLEANUP_TIMEOUTS_TOTAL: LazyLock = LazyLock::new(| }) }); +/// Total number of child-cluster kubeconfig resolutions, labelled by +/// outcome. Label values: +/// +/// - `management` — neither an explicit ref nor an auto-discovered +/// Secret was present; the controller used the management client. +/// - `child_explicit` — built a child client from +/// `spec.kubeconfigSecretRef`. +/// - `child_auto` — built a child client from the auto-discovered +/// `-kubeconfig` Secret. +/// - `cache_hit` — same `resourceVersion`, returned the cached client. +/// - `rebuild` — `resourceVersion` changed, rebuilt the child client. +/// - `error` — resolution failed (see +/// `fivespot_child_kubeconfig_errors_total` for the breakdown). +pub static CHILD_KUBECONFIG_RESOLUTIONS_TOTAL: LazyLock = LazyLock::new(|| { + register_counter_vec!( + "fivespot_child_kubeconfig_resolutions_total", + "Total number of child-cluster kubeconfig resolutions by outcome", + &["result"] + ) + .unwrap_or_else(|e| { + eprintln!("WARN: Failed to register fivespot_child_kubeconfig_resolutions_total: {e}"); + fallback_counter_vec( + "fivespot_child_kubeconfig_resolutions_total", + "Total number of child-cluster kubeconfig resolutions by outcome", + &["result"], + ) + }) +}); + +/// Total number of child-cluster kubeconfig resolution errors, labelled +/// by reason: +/// - `secret_missing_key` — Secret exists but lacks `data[key]` +/// - `invalid_yaml` — kubeconfig YAML parse failed +/// - `unreachable` — explicit ref 404 / Secret GET network error +/// - `non_404_kube_error` — non-404 kube::Error during auto-discovery GET +/// +/// Operators should alert on a non-trivial rate of any reason: every +/// error here means at least one `ScheduledMachine` is silently failing +/// its Node/Pod operations on the workload cluster. +pub static CHILD_KUBECONFIG_ERRORS_TOTAL: LazyLock = LazyLock::new(|| { + register_counter_vec!( + "fivespot_child_kubeconfig_errors_total", + "Total number of child-cluster kubeconfig resolution errors by reason", + &["reason"] + ) + .unwrap_or_else(|e| { + eprintln!("WARN: Failed to register fivespot_child_kubeconfig_errors_total: {e}"); + fallback_counter_vec( + "fivespot_child_kubeconfig_errors_total", + "Total number of child-cluster kubeconfig resolution errors by reason", + &["reason"], + ) + }) +}); + /// Record a successful reconciliation pub fn record_reconciliation_success(phase: &str, duration_secs: f64) { RECONCILIATIONS_TOTAL @@ -498,6 +553,24 @@ pub fn record_rapid_re_reclaim(namespace: &str, name: &str) { /// Operators should treat any non-zero rate as a signal that orphan CAPI /// Machine / bootstrap / infrastructure resources may exist and need /// manual reconciliation. +/// Record one child-cluster kubeconfig resolution. `result` must be one +/// of the documented outcomes on +/// [`CHILD_KUBECONFIG_RESOLUTIONS_TOTAL`]. +pub fn record_child_kubeconfig_resolution(result: &str) { + CHILD_KUBECONFIG_RESOLUTIONS_TOTAL + .with_label_values(&[result]) + .inc(); +} + +/// Record one child-cluster kubeconfig resolution error. `reason` must +/// be one of the documented categories on +/// [`CHILD_KUBECONFIG_ERRORS_TOTAL`]. +pub fn record_child_kubeconfig_error(reason: &str) { + CHILD_KUBECONFIG_ERRORS_TOTAL + .with_label_values(&[reason]) + .inc(); +} + pub fn record_finalizer_cleanup_timeout() { FINALIZER_CLEANUP_TIMEOUTS_TOTAL.inc(); } diff --git a/src/reconcilers/child_client.rs b/src/reconcilers/child_client.rs new file mode 100644 index 0000000..1fef6a1 --- /dev/null +++ b/src/reconcilers/child_client.rs @@ -0,0 +1,569 @@ +// Copyright (c) 2025 Erick Bourgeois, RBC Capital Markets +// SPDX-License-Identifier: Apache-2.0 +//! # Child-cluster client resolver +//! +//! Resolves the right [`kube::Client`] for a `ScheduledMachine`'s Node and +//! Pod operations. In CAPI + k0smotron / k0rdent topologies, the `Node` +//! objects live inside a *workload* (child) cluster while the +//! `ScheduledMachine`, CAPI `Machine`, bootstrap, and infrastructure CRs +//! live on the *management* cluster. This module is the boundary: it reads +//! the kubeconfig out of a same-namespace Secret, builds a child-cluster +//! `kube::Client`, caches it by `(namespace, secret_name)`, and invalidates +//! the cache on the Secret's `resourceVersion` change. +//! +//! ## Resolution order +//! 1. Explicit `spec.kubeconfigSecretRef` — fail-closed on missing / bad. +//! 2. Auto-discover `-kubeconfig` Secret in the same +//! namespace — 404 falls through silently (preserves the degenerate +//! single-cluster posture). +//! 3. Management client. +//! +//! ## Why no cross-namespace refs +//! The Secret MUST live in the `ScheduledMachine`'s own namespace. Allowing +//! cross-namespace refs would let a tenant in one namespace read a privileged +//! kubeconfig in another — a privilege-escalation surface. The CRD's +//! [`KubeconfigSecretRef`](crate::crd::KubeconfigSecretRef) intentionally has +//! no `namespace` field. +//! +//! ## Cache invalidation +//! Every `resolve()` GETs the Secret to compare `metadata.resourceVersion` +//! against the cached entry; a mismatch rebuilds the client. The GET targets +//! the *management* cluster (cheap, sub-millisecond) — the child cluster is +//! only contacted lazily when downstream code actually uses the returned +//! client. Token / cert rotations performed by CAPI's control-plane provider +//! flip the Secret's `resourceVersion`, so the next reconcile picks up the +//! new credentials with no additional refresh logic. + +use std::collections::HashMap; +use std::sync::{Arc, RwLock}; +use std::time::Instant; + +use k8s_openapi::api::core::v1::Secret; +use kube::{config::Kubeconfig, Api, Client, ResourceExt}; +use tracing::{debug, info, warn}; + +use crate::constants::{CHILD_CLIENT_CACHE_CAP, K8S_API_TIMEOUT_SECS}; +use crate::crd::ScheduledMachine; +use crate::metrics::{record_child_kubeconfig_error, record_child_kubeconfig_resolution}; +use crate::reconcilers::ReconcilerError; + +/// Default key inside the kubeconfig Secret's `data` map — CAPI's +/// `-kubeconfig` Secret writes the kubeconfig under `value`. +/// Used for auto-discovery and as the serde default on +/// [`KubeconfigSecretRef::key`](crate::crd::KubeconfigSecretRef). +pub const DEFAULT_KUBECONFIG_SECRET_KEY: &str = "value"; + +/// Identifies one cached child-cluster `kube::Client` by the Secret backing +/// it. Two `ScheduledMachine`s pointing at the same Secret share one cache +/// entry (and Phase 3 will share one Node watcher). +#[derive(Hash, Eq, PartialEq, Clone, Debug)] +pub struct CacheKey { + pub namespace: String, + pub secret_name: String, +} + +impl CacheKey { + /// Construct a `CacheKey`. Provided as a constructor so tests and future + /// callers don't have to remember the field order. + #[must_use] + pub fn new(namespace: impl Into, secret_name: impl Into) -> Self { + Self { + namespace: namespace.into(), + secret_name: secret_name.into(), + } + } +} + +struct CachedEntry { + client: Client, + resource_version: String, + last_used: Instant, +} + +/// Outcome of resolving the right client for a `ScheduledMachine`'s +/// Node/Pod operations. `Management` and `Child` are kept distinct so the +/// caller can distinguish "no kubeconfig configured" from "kubeconfig +/// resolved successfully" for logging, metrics, and Phase 2 status +/// conditions. +#[derive(Clone)] +pub enum ResolvedClient { + /// No `kubeconfigSecretRef` was set, and no `-kubeconfig` + /// Secret was found in the namespace. The controller falls back to the + /// management client — the degenerate single-cluster posture where + /// management ≡ workload. + Management(Client), + /// A child-cluster client was built from a kubeconfig Secret. + Child { + client: Client, + key: CacheKey, + resource_version: String, + }, +} + +// `kube::Client` does not implement `Debug`, so we provide a hand-rolled +// impl that elides the client and only surfaces the cache key + RV. This +// is enough for `expect_err` diagnostics and ad-hoc tracing. +impl std::fmt::Debug for ResolvedClient { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Management(_) => f + .debug_tuple("Management") + .field(&"") + .finish(), + Self::Child { + key, + resource_version, + .. + } => f + .debug_struct("Child") + .field("key", key) + .field("resource_version", resource_version) + .field("client", &"") + .finish(), + } + } +} + +impl ResolvedClient { + /// Borrow the underlying `kube::Client` regardless of which variant. + /// Use this when the call site doesn't care whether the client targets + /// the management or the child cluster (e.g. `Api::all(client.clone())`). + #[must_use] + pub fn client(&self) -> &Client { + match self { + Self::Management(c) | Self::Child { client: c, .. } => c, + } + } + + /// Consume and return the underlying `kube::Client`. + #[must_use] + pub fn into_client(self) -> Client { + match self { + Self::Management(c) | Self::Child { client: c, .. } => c, + } + } + + /// `true` iff the resolved client targets a child cluster (not management). + /// Used by tests and by Phase 2 metrics labels. + #[must_use] + pub fn is_child(&self) -> bool { + matches!(self, Self::Child { .. }) + } +} + +/// Hook called by the cache when it builds a new child `kube::Client` or +/// evicts a cached one. Implemented by +/// [`crate::reconcilers::child_watch::ChildNodeWatchManager`] to start / +/// cancel per-child Node watchers in lock-step with the client cache. +/// +/// Default behaviour is a no-op so unit tests of `ChildClientCache` — +/// and any deployment that doesn't wire a manager — work unchanged. +pub trait ChildWatchHook: Send + Sync { + /// Called once when a child `Client` is first built for a given + /// `CacheKey`, AND on every rebuild (Secret `resourceVersion` + /// change). Implementations are expected to be idempotent: the + /// manager checks for an existing watcher before starting one. + fn on_child_resolved(&self, key: &CacheKey, client: Client); + + /// Called when a `CacheKey`'s entry is removed from the cache + /// (LRU eviction). The manager should cancel and join any + /// watcher running for this key. + fn on_child_evicted(&self, key: &CacheKey); +} + +/// No-op hook used when no `ChildWatchHook` has been installed. Keeps +/// the cache usable in unit tests + degenerate deployments without +/// requiring an `Option>` everywhere. +struct NoopWatchHook; + +impl ChildWatchHook for NoopWatchHook { + fn on_child_resolved(&self, _: &CacheKey, _: Client) {} + fn on_child_evicted(&self, _: &CacheKey) {} +} + +/// In-memory cache of child-cluster `kube::Client`s keyed by the Secret +/// backing each one. Cheaply cloneable (the inner state is `Arc`-wrapped), +/// so the cache can live on [`crate::reconcilers::Context`] and be shared +/// across every reconcile. +#[derive(Clone)] +pub struct ChildClientCache { + inner: Arc>>, + /// Watch-lifecycle hook. Defaults to a no-op until `set_hook()` is + /// called from `main.rs` once the per-child Node watcher manager + /// has its reflector store wired up. + hook: Arc>>, +} + +impl Default for ChildClientCache { + fn default() -> Self { + Self::new() + } +} + +impl ChildClientCache { + #[must_use] + pub fn new() -> Self { + Self { + inner: Arc::new(RwLock::new(HashMap::new())), + hook: Arc::new(RwLock::new(Arc::new(NoopWatchHook))), + } + } + + /// Install a watch-lifecycle hook. Idempotent: the most recent hook + /// wins (`main.rs` calls this exactly once at startup). When unset + /// (default), the cache behaves as if no per-child Node watcher + /// exists — useful for unit tests and for the degenerate + /// single-cluster posture. + pub fn set_hook(&self, hook: Arc) { + *self + .hook + .write() + .expect("ChildClientCache hook lock poisoned") = hook; + } + + /// Get a cheap clone of the current hook for hot-path use. + fn current_hook(&self) -> Arc { + self.hook + .read() + .expect("ChildClientCache hook lock poisoned") + .clone() + } + + /// Current number of cached child-cluster clients. Exposed for tests + + /// Phase 2 health endpoint. + #[must_use] + pub fn len(&self) -> usize { + self.inner + .read() + .expect("ChildClientCache lock poisoned") + .len() + } + + /// `true` iff the cache currently holds no entries. + #[must_use] + pub fn is_empty(&self) -> bool { + self.len() == 0 + } + + /// Resolve the right client for this SM's Node/Pod operations. + /// + /// See module docs for the resolution order and cache semantics. + /// + /// # Errors + /// - [`ReconcilerError::InvalidConfig`] if the SM is not namespaced + /// - [`ReconcilerError::ChildClusterUnreachable`] if an explicit + /// `spec.kubeconfigSecretRef` cannot be resolved (404, network error) + /// - [`ReconcilerError::KubeconfigSecretMissingKey`] if the Secret exists + /// but lacks the requested `data[key]` + /// - [`ReconcilerError::KubeconfigInvalid`] if the kubeconfig YAML is + /// malformed or a `kube::Client` cannot be built from it + /// - [`ReconcilerError::KubeError`] for non-404 Secret GET failures + pub async fn resolve( + &self, + mgmt_client: &Client, + sm: &ScheduledMachine, + ) -> Result { + let namespace = sm.namespace().ok_or_else(|| { + ReconcilerError::InvalidConfig( + "ScheduledMachine must be namespaced for kubeconfig resolution".to_string(), + ) + })?; + + // 1. Explicit ref — fail-closed. + if let Some(r) = &sm.spec.kubeconfig_secret_ref { + let result = self + .build_or_get_cached(mgmt_client, &namespace, &r.name, &r.key, true) + .await; + // Metric label: distinguish the explicit-ref path from auto + + // surface error/cache-hit/rebuild via the result discriminant. + record_for_outcome(&result, "child_explicit"); + return result; + } + + // 2. Auto-discover `-kubeconfig` Secret. + let auto_name = format!("{}-kubeconfig", sm.spec.cluster_name); + match self + .build_or_get_cached( + mgmt_client, + &namespace, + &auto_name, + DEFAULT_KUBECONFIG_SECRET_KEY, + false, + ) + .await + { + Ok(r) => { + record_for_resolved(&r, "child_auto"); + return Ok(r); + } + // Auto-discovery 404 falls through to management. + Err(ReconcilerError::NotFound(_)) => { + debug!( + namespace = %namespace, + secret = %auto_name, + sm = %sm.name_any(), + "no auto-discovered kubeconfig Secret; using management client" + ); + } + Err(e) => { + record_child_kubeconfig_resolution("error"); + record_error_label(&e); + return Err(e); + } + } + + // 3. Management fallback (degenerate co-located case). + record_child_kubeconfig_resolution("management"); + Ok(ResolvedClient::Management(mgmt_client.clone())) + } + + /// Get a cached child client by `CacheKey`, if one exists. Bumps + /// `last_used`. Returned as a clone (the `kube::Client` is cheaply + /// cloneable). Exposed for tests + the Phase 1.9 watcher module which + /// needs to share the same cached `Client` instance across resolver + + /// watcher. + #[must_use] + pub fn peek(&self, key: &CacheKey) -> Option { + let mut g = self.inner.write().expect("ChildClientCache lock poisoned"); + g.get_mut(key).map(|e| { + e.last_used = Instant::now(); + e.client.clone() + }) + } + + /// Manually evict a cache entry. Used by the watcher module when a + /// per-child Node watch fails terminally (e.g. unauthorized) so the next + /// reconcile rebuilds from scratch. Fires `on_child_evicted` if an + /// entry was actually removed. + pub fn evict(&self, key: &CacheKey) { + let removed = { + let mut g = self.inner.write().expect("ChildClientCache lock poisoned"); + g.remove(key).is_some() + }; + if removed { + self.current_hook().on_child_evicted(key); + } + } + + /// GET the Secret, compare RV against the cache, rebuild the client on + /// miss. `explicit = true` means a `spec.kubeconfigSecretRef` was set; + /// 404 in that case is fail-closed. `explicit = false` is auto-discovery; + /// 404 returns [`ReconcilerError::NotFound`] for the caller to treat as + /// fall-through. + async fn build_or_get_cached( + &self, + mgmt_client: &Client, + namespace: &str, + secret_name: &str, + key: &str, + explicit: bool, + ) -> Result { + let secrets: Api = Api::namespaced(mgmt_client.clone(), namespace); + let secret = match secrets.get(secret_name).await { + Ok(s) => s, + Err(kube::Error::Api(e)) if e.code == 404 => { + if explicit { + return Err(ReconcilerError::ChildClusterUnreachable { + namespace: namespace.to_string(), + name: secret_name.to_string(), + reason: format!( + "explicit kubeconfigSecretRef points at Secret {namespace}/{secret_name} which does not exist" + ), + }); + } + // Auto-discovery: caller treats this as fall-through. + return Err(ReconcilerError::NotFound(format!( + "{namespace}/{secret_name}" + ))); + } + Err(e) => { + if explicit { + return Err(ReconcilerError::ChildClusterUnreachable { + namespace: namespace.to_string(), + name: secret_name.to_string(), + reason: format!("GET Secret failed: {e}"), + }); + } + return Err(e.into()); + } + }; + + let rv = secret.metadata.resource_version.clone().unwrap_or_default(); + let cache_key = CacheKey::new(namespace, secret_name); + + // Cache hit on identical RV: return cached client and bump last_used. + { + let mut g = self.inner.write().expect("ChildClientCache lock poisoned"); + if let Some(entry) = g.get_mut(&cache_key) { + if entry.resource_version == rv { + entry.last_used = Instant::now(); + return Ok(ResolvedClient::Child { + client: entry.client.clone(), + key: cache_key, + resource_version: rv, + }); + } + // RV changed — fall through to rebuild. Logged for ops. + info!( + namespace = %namespace, + secret = %secret_name, + cached_rv = %entry.resource_version, + new_rv = %rv, + "kubeconfig Secret resourceVersion changed; rebuilding child client" + ); + } + } + + // Cache miss / stale: build a new child Client from the Secret data. + let raw = secret + .data + .as_ref() + .and_then(|d| d.get(key)) + .ok_or_else(|| ReconcilerError::KubeconfigSecretMissingKey { + namespace: namespace.to_string(), + name: secret_name.to_string(), + key: key.to_string(), + })?; + + let yaml = std::str::from_utf8(&raw.0).map_err(|e| ReconcilerError::KubeconfigInvalid { + namespace: namespace.to_string(), + name: secret_name.to_string(), + reason: format!("kubeconfig data is not valid UTF-8: {e}"), + })?; + + let kubeconfig = + Kubeconfig::from_yaml(yaml).map_err(|e| ReconcilerError::KubeconfigInvalid { + namespace: namespace.to_string(), + name: secret_name.to_string(), + reason: format!("YAML parse failed: {e}"), + })?; + + let mut config = kube::Config::from_custom_kubeconfig( + kubeconfig, + &kube::config::KubeConfigOptions::default(), + ) + .await + .map_err(|e| ReconcilerError::KubeconfigInvalid { + namespace: namespace.to_string(), + name: secret_name.to_string(), + reason: format!("kubeconfig is not usable to build a Config: {e}"), + })?; + // Apply the same wire timeouts the management client uses (see + // src/main.rs Client construction). Without these a hung child API + // would stall reconciliation indefinitely. + config.read_timeout = Some(std::time::Duration::from_secs(K8S_API_TIMEOUT_SECS)); + config.write_timeout = Some(std::time::Duration::from_secs(K8S_API_TIMEOUT_SECS)); + + let child = Client::try_from(config).map_err(|e| ReconcilerError::KubeconfigInvalid { + namespace: namespace.to_string(), + name: secret_name.to_string(), + reason: format!("could not build kube::Client: {e}"), + })?; + + // Insert (with LRU eviction if at cap) and return. Track whether + // we're replacing an existing entry for the same key (RV change + // / rotation) AND which (if any) different key was LRU-evicted — + // both situations fire `on_child_evicted` so the watcher gets + // cancelled and restarted with the fresh Client. The hook is + // invoked AFTER the inner-map write lock is released so a + // manager re-entering the cache (e.g. via `peek`) doesn't + // deadlock. + let (replaced_same_key, evicted_other): (bool, Option) = { + let mut g = self.inner.write().expect("ChildClientCache lock poisoned"); + let evicted_other = if g.len() >= CHILD_CLIENT_CACHE_CAP && !g.contains_key(&cache_key) + { + let lru = g + .iter() + .min_by_key(|(_, e)| e.last_used) + .map(|(k, _)| k.clone()); + if let Some(ref k) = lru { + warn!( + evicted_namespace = %k.namespace, + evicted_secret = %k.secret_name, + cap = CHILD_CLIENT_CACHE_CAP, + "ChildClientCache at capacity; evicting LRU entry" + ); + g.remove(k); + } + lru + } else { + None + }; + let previous = g.insert( + cache_key.clone(), + CachedEntry { + client: child.clone(), + resource_version: rv.clone(), + last_used: Instant::now(), + }, + ); + (previous.is_some(), evicted_other) + }; + + let hook = self.current_hook(); + if let Some(ev) = evicted_other { + hook.on_child_evicted(&ev); + } + if replaced_same_key { + // RV change: cancel the watcher running with stale credentials + // before the manager starts a new one with the rebuilt Client. + hook.on_child_evicted(&cache_key); + } + hook.on_child_resolved(&cache_key, child.clone()); + + Ok(ResolvedClient::Child { + client: child, + key: cache_key, + resource_version: rv, + }) + } +} + +/// Translate a `Result` from `build_or_get_cached` +/// into the right Prometheus result label. `path` is `"child_explicit"` +/// or `"child_auto"` depending on which arm of `resolve()` is calling. +/// On success, we use `cache_hit` vs `rebuild` semantics — but +/// `build_or_get_cached` doesn't currently distinguish them at the +/// return type level. For now we record only the `path` on success; +/// when Phase 2 cache-stats land, this can split into hit/rebuild. +fn record_for_outcome(result: &Result, path: &str) { + match result { + Ok(r) => record_for_resolved(r, path), + Err(e) => { + record_child_kubeconfig_resolution("error"); + record_error_label(e); + } + } +} + +/// Record one resolution success against the appropriate result label. +/// `Management` overrides `path` because the path argument only +/// describes which arm tried (`child_explicit` / `child_auto`), and +/// the resolver's design lets `build_or_get_cached` only return +/// `Child` — but defensive code in case the type evolves. +fn record_for_resolved(resolved: &ResolvedClient, path: &str) { + let label = match resolved { + ResolvedClient::Management(_) => "management", + ResolvedClient::Child { .. } => path, + }; + record_child_kubeconfig_resolution(label); +} + +/// Map a `ReconcilerError` to a `fivespot_child_kubeconfig_errors_total` +/// reason label. Variants that can't originate from the resolver are +/// folded into `unreachable` as a safe default — they'd indicate a +/// future refactor missed updating this match. +fn record_error_label(err: &ReconcilerError) { + let reason = match err { + ReconcilerError::KubeconfigSecretMissingKey { .. } => "secret_missing_key", + ReconcilerError::KubeconfigInvalid { .. } => "invalid_yaml", + ReconcilerError::ChildClusterUnreachable { .. } => "unreachable", + ReconcilerError::KubeError(_) => "non_404_kube_error", + _ => "unreachable", + }; + record_child_kubeconfig_error(reason); +} + +#[cfg(test)] +#[path = "child_client_tests.rs"] +mod child_client_tests; diff --git a/src/reconcilers/child_client_tests.rs b/src/reconcilers/child_client_tests.rs new file mode 100644 index 0000000..f9776b1 --- /dev/null +++ b/src/reconcilers/child_client_tests.rs @@ -0,0 +1,834 @@ +// Copyright (c) 2025 Erick Bourgeois, RBC Capital Markets +// SPDX-License-Identifier: Apache-2.0 +//! Tests for the [`ChildClientCache`] resolver. Lock the contract for: +//! - Resolution order (explicit → auto-discover → management fallback) +//! - Cache hit (same RV) vs miss (changed RV) vs auto-discovery 404 fallthrough +//! - Error mapping for the three new `ReconcilerError` kubeconfig variants +//! - Bounded-LRU eviction at capacity +//! +//! Uses `tower_test::mock` to drive the management-cluster client; the +//! child client is constructed from a fixture kubeconfig YAML — kube-rs +//! parses it eagerly but the child Client itself only contacts its server +//! when something downstream actually USES the client. The tests never +//! exercise the child Client, so they remain hermetic. + +#[cfg(test)] +#[allow(clippy::module_inception)] +mod tests { + use super::super::*; + use crate::crd::{ + EmbeddedResource, KubeconfigSecretRef, ScheduleSpec, ScheduledMachine, ScheduledMachineSpec, + }; + use http::{Request, Response}; + use kube::api::ObjectMeta; + use kube::client::Body; + use std::pin::pin; + use std::sync::Arc; + use tower_test::mock; + + // ======================================================================== + // Fixtures + // ======================================================================== + + /// Minimal valid kubeconfig that `kube::Config::from_custom_kubeconfig` + /// accepts. `insecure-skip-tls-verify` keeps the parse hermetic — no + /// CA bundle parsing, no DNS resolution. + const FIXTURE_KUBECONFIG_YAML: &str = r#" +apiVersion: v1 +kind: Config +clusters: +- name: child + cluster: + server: https://child.example.test + insecure-skip-tls-verify: true +contexts: +- name: child + context: + cluster: child + user: child +users: +- name: child + user: + token: dummy-token +current-context: child +"#; + + fn mock_client_pair() -> (kube::Client, mock::Handle, Response>) { + let (svc, handle) = mock::pair::, Response>(); + (kube::Client::new(svc, "default"), handle) + } + + fn make_sm( + namespace: &str, + name: &str, + cluster_name: &str, + ref_: Option, + ) -> Arc { + let sm = ScheduledMachine { + metadata: ObjectMeta { + name: Some(name.to_string()), + namespace: Some(namespace.to_string()), + ..Default::default() + }, + spec: ScheduledMachineSpec { + schedule: ScheduleSpec { + days_of_week: vec!["mon-fri".to_string()], + hours_of_day: vec!["9-17".to_string()], + timezone: "UTC".to_string(), + enabled: true, + }, + cluster_name: cluster_name.to_string(), + bootstrap_spec: EmbeddedResource(serde_json::json!({ + "apiVersion": "bootstrap.cluster.x-k8s.io/v1beta1", + "kind": "K0sWorkerConfig", + "spec": {} + })), + infrastructure_spec: EmbeddedResource(serde_json::json!({ + "apiVersion": "infrastructure.cluster.x-k8s.io/v1beta1", + "kind": "RemoteMachine", + "spec": {} + })), + machine_template: None, + priority: 50, + graceful_shutdown_timeout: "5m".to_string(), + node_drain_timeout: "5m".to_string(), + kill_switch: false, + node_taints: vec![], + kill_if_commands: None, + kubeconfig_secret_ref: ref_, + }, + status: None, + }; + Arc::new(sm) + } + + /// JSON body for a Secret GET response containing the fixture kubeconfig. + fn secret_response_body( + namespace: &str, + name: &str, + key: &str, + resource_version: &str, + kubeconfig_yaml: &str, + ) -> Vec { + use base64::Engine; + let b64 = base64::engine::general_purpose::STANDARD.encode(kubeconfig_yaml.as_bytes()); + serde_json::to_vec(&serde_json::json!({ + "apiVersion": "v1", + "kind": "Secret", + "metadata": { + "name": name, + "namespace": namespace, + "resourceVersion": resource_version, + }, + "type": "Opaque", + "data": { key: b64 } + })) + .unwrap() + } + + fn k8s_404_body(name: &str) -> Vec { + serde_json::to_vec(&serde_json::json!({ + "kind": "Status", + "apiVersion": "v1", + "status": "Failure", + "message": format!("secrets \"{name}\" not found"), + "reason": "NotFound", + "code": 404 + })) + .unwrap() + } + + fn k8s_403_body() -> Vec { + serde_json::to_vec(&serde_json::json!({ + "kind": "Status", + "apiVersion": "v1", + "status": "Failure", + "message": "forbidden", + "reason": "Forbidden", + "code": 403 + })) + .unwrap() + } + + fn respond_with(status: u16, body: Vec) -> Response { + Response::builder() + .status(status) + .header("Content-Type", "application/json") + .body(Body::from(body)) + .unwrap() + } + + // ======================================================================== + // Resolution order: management fallback when neither ref nor auto-Secret + // ======================================================================== + + #[tokio::test] + async fn test_resolve_falls_back_to_management_when_auto_discovery_404s() { + let (mgmt, handle) = mock_client_pair(); + let cache = ChildClientCache::new(); + let sm = make_sm("team-a", "sm1", "alpha", None); + + // Server: a single Secret GET that 404s. + let server = tokio::spawn(async move { + let mut h = pin!(handle); + let (req, send) = h.next_request().await.expect("expected one Secret GET"); + assert!( + req.uri() + .path() + .ends_with("/api/v1/namespaces/team-a/secrets/alpha-kubeconfig"), + "expected auto-discovery GET, got: {}", + req.uri() + ); + send.send_response(respond_with(404, k8s_404_body("alpha-kubeconfig"))); + }); + + let resolved = cache + .resolve(&mgmt, &sm) + .await + .expect("auto-discovery 404 must fall through to management"); + assert!( + !resolved.is_child(), + "expected Management variant on auto-discovery 404" + ); + server.await.unwrap(); + assert_eq!(cache.len(), 0, "no entry should be cached after 404"); + } + + // ======================================================================== + // Resolution: explicit ref takes precedence and builds a child client + // ======================================================================== + + #[tokio::test] + async fn test_resolve_uses_explicit_ref_when_present() { + let (mgmt, handle) = mock_client_pair(); + let cache = ChildClientCache::new(); + let sm = make_sm( + "team-a", + "sm1", + "alpha", + Some(KubeconfigSecretRef { + name: "custom-kubeconfig".to_string(), + key: "value".to_string(), + }), + ); + + let server = tokio::spawn(async move { + let mut h = pin!(handle); + let (req, send) = h.next_request().await.expect("expected one Secret GET"); + assert!( + req.uri() + .path() + .ends_with("/api/v1/namespaces/team-a/secrets/custom-kubeconfig"), + "expected explicit-ref GET to custom-kubeconfig, got: {}", + req.uri() + ); + send.send_response(respond_with( + 200, + secret_response_body( + "team-a", + "custom-kubeconfig", + "value", + "42", + FIXTURE_KUBECONFIG_YAML, + ), + )); + }); + + let resolved = cache + .resolve(&mgmt, &sm) + .await + .expect("explicit ref must resolve"); + assert!(resolved.is_child(), "expected Child variant"); + if let ResolvedClient::Child { + key, + resource_version, + .. + } = resolved + { + assert_eq!(key, CacheKey::new("team-a", "custom-kubeconfig")); + assert_eq!(resource_version, "42"); + } + server.await.unwrap(); + assert_eq!(cache.len(), 1); + } + + // ======================================================================== + // Explicit ref takes priority over auto-discovery + // ======================================================================== + + #[tokio::test] + async fn test_resolve_prefers_explicit_ref_over_auto_discovery() { + // With both an explicit ref AND an `-kubeconfig` Secret + // available, the resolver MUST GET the explicit name only — not the + // auto-discovery name. This is asserted by the mock seeing exactly + // one request, and it's the explicit one. + let (mgmt, handle) = mock_client_pair(); + let cache = ChildClientCache::new(); + let sm = make_sm( + "team-a", + "sm1", + "alpha", + Some(KubeconfigSecretRef { + name: "override".to_string(), + key: "value".to_string(), + }), + ); + + let server = tokio::spawn(async move { + let mut h = pin!(handle); + let (req, send) = h.next_request().await.expect("expected one Secret GET"); + let path = req.uri().path().to_string(); + assert!( + path.ends_with("/api/v1/namespaces/team-a/secrets/override"), + "expected explicit `override` Secret GET, got: {path}" + ); + assert!( + !path.contains("alpha-kubeconfig"), + "explicit ref MUST suppress auto-discovery GET: {path}" + ); + send.send_response(respond_with( + 200, + secret_response_body("team-a", "override", "value", "1", FIXTURE_KUBECONFIG_YAML), + )); + }); + + let _ = cache.resolve(&mgmt, &sm).await.expect("must resolve"); + server.await.unwrap(); + } + + // ======================================================================== + // Auto-discovery: -kubeconfig Secret found → child client + // ======================================================================== + + #[tokio::test] + async fn test_resolve_auto_discovers_capi_naming_when_secret_exists() { + let (mgmt, handle) = mock_client_pair(); + let cache = ChildClientCache::new(); + let sm = make_sm("team-a", "sm1", "alpha", None); + + let server = tokio::spawn(async move { + let mut h = pin!(handle); + let (req, send) = h.next_request().await.expect("expected Secret GET"); + assert!(req + .uri() + .path() + .ends_with("/api/v1/namespaces/team-a/secrets/alpha-kubeconfig")); + send.send_response(respond_with( + 200, + secret_response_body( + "team-a", + "alpha-kubeconfig", + "value", + "7", + FIXTURE_KUBECONFIG_YAML, + ), + )); + }); + + let resolved = cache.resolve(&mgmt, &sm).await.expect("must resolve"); + assert!( + resolved.is_child(), + "auto-discovered Secret → Child variant" + ); + server.await.unwrap(); + } + + // ======================================================================== + // Cache: hit on same RV returns cached client (still GETs Secret to check) + // ======================================================================== + + #[tokio::test] + async fn test_cache_hit_on_same_resource_version_does_not_rebuild() { + let (mgmt, handle) = mock_client_pair(); + let cache = ChildClientCache::new(); + let sm = make_sm("team-a", "sm1", "alpha", None); + + // Two resolves: server responds with the same RV both times. + let server = tokio::spawn(async move { + let mut h = pin!(handle); + for _ in 0..2 { + let (_req, send) = h.next_request().await.expect("expected Secret GET"); + send.send_response(respond_with( + 200, + secret_response_body( + "team-a", + "alpha-kubeconfig", + "value", + "100", + FIXTURE_KUBECONFIG_YAML, + ), + )); + } + }); + + let first = cache.resolve(&mgmt, &sm).await.expect("first resolve"); + let second = cache.resolve(&mgmt, &sm).await.expect("second resolve"); + server.await.unwrap(); + + let rv_first = match first { + ResolvedClient::Child { + resource_version, .. + } => resource_version, + _ => panic!("expected child"), + }; + let rv_second = match second { + ResolvedClient::Child { + resource_version, .. + } => resource_version, + _ => panic!("expected child"), + }; + assert_eq!(rv_first, "100"); + assert_eq!(rv_second, "100"); + assert_eq!(cache.len(), 1, "cache must hold exactly one entry"); + } + + // ======================================================================== + // Cache: RV change forces a rebuild + // ======================================================================== + + #[tokio::test] + async fn test_cache_rebuilds_when_resource_version_changes() { + let (mgmt, handle) = mock_client_pair(); + let cache = ChildClientCache::new(); + let sm = make_sm("team-a", "sm1", "alpha", None); + + let server = tokio::spawn(async move { + let mut h = pin!(handle); + // First resolve: RV "1" + let (_req, send) = h.next_request().await.unwrap(); + send.send_response(respond_with( + 200, + secret_response_body( + "team-a", + "alpha-kubeconfig", + "value", + "1", + FIXTURE_KUBECONFIG_YAML, + ), + )); + // Second resolve: RV "2" — rotation event + let (_req, send) = h.next_request().await.unwrap(); + send.send_response(respond_with( + 200, + secret_response_body( + "team-a", + "alpha-kubeconfig", + "value", + "2", + FIXTURE_KUBECONFIG_YAML, + ), + )); + }); + + let r1 = cache.resolve(&mgmt, &sm).await.unwrap(); + let r2 = cache.resolve(&mgmt, &sm).await.unwrap(); + server.await.unwrap(); + + let rv1 = match r1 { + ResolvedClient::Child { + resource_version, .. + } => resource_version, + _ => panic!(), + }; + let rv2 = match r2 { + ResolvedClient::Child { + resource_version, .. + } => resource_version, + _ => panic!(), + }; + assert_eq!(rv1, "1"); + assert_eq!( + rv2, "2", + "RV change must yield the new RV on subsequent resolve" + ); + assert_eq!(cache.len(), 1, "cache stays at one entry (key unchanged)"); + } + + // ======================================================================== + // Error mapping: explicit ref + 404 → ChildClusterUnreachable (fail-closed) + // ======================================================================== + + #[tokio::test] + async fn test_explicit_ref_404_returns_child_cluster_unreachable() { + let (mgmt, handle) = mock_client_pair(); + let cache = ChildClientCache::new(); + let sm = make_sm( + "team-a", + "sm1", + "alpha", + Some(KubeconfigSecretRef { + name: "does-not-exist".to_string(), + key: "value".to_string(), + }), + ); + + let server = tokio::spawn(async move { + let mut h = pin!(handle); + let (_req, send) = h.next_request().await.unwrap(); + send.send_response(respond_with(404, k8s_404_body("does-not-exist"))); + }); + + let err = cache + .resolve(&mgmt, &sm) + .await + .expect_err("explicit ref + 404 must error, not fall back"); + assert!( + matches!(err, ReconcilerError::ChildClusterUnreachable { .. }), + "expected ChildClusterUnreachable, got: {err:?}" + ); + server.await.unwrap(); + } + + // ======================================================================== + // Error mapping: missing data key → KubeconfigSecretMissingKey + // ======================================================================== + + #[tokio::test] + async fn test_missing_data_key_returns_kubeconfig_secret_missing_key() { + let (mgmt, handle) = mock_client_pair(); + let cache = ChildClientCache::new(); + let sm = make_sm( + "team-a", + "sm1", + "alpha", + Some(KubeconfigSecretRef { + name: "kc".to_string(), + key: "missing-key".to_string(), + }), + ); + + let server = tokio::spawn(async move { + let mut h = pin!(handle); + let (_req, send) = h.next_request().await.unwrap(); + // Secret exists but only has `value`, not `missing-key`. + send.send_response(respond_with( + 200, + secret_response_body("team-a", "kc", "value", "1", FIXTURE_KUBECONFIG_YAML), + )); + }); + + let err = cache.resolve(&mgmt, &sm).await.expect_err("must error"); + assert!( + matches!( + err, + ReconcilerError::KubeconfigSecretMissingKey { ref key, .. } if key == "missing-key" + ), + "expected KubeconfigSecretMissingKey, got: {err:?}" + ); + server.await.unwrap(); + } + + // ======================================================================== + // Error mapping: invalid YAML → KubeconfigInvalid + // ======================================================================== + + #[tokio::test] + async fn test_invalid_kubeconfig_yaml_returns_kubeconfig_invalid() { + let (mgmt, handle) = mock_client_pair(); + let cache = ChildClientCache::new(); + let sm = make_sm( + "team-a", + "sm1", + "alpha", + Some(KubeconfigSecretRef { + name: "kc".to_string(), + key: "value".to_string(), + }), + ); + + let server = tokio::spawn(async move { + let mut h = pin!(handle); + let (_req, send) = h.next_request().await.unwrap(); + // Garbage YAML — clearly not a kubeconfig. + send.send_response(respond_with( + 200, + secret_response_body( + "team-a", + "kc", + "value", + "1", + "this is\n : not [valid: yaml: :", + ), + )); + }); + + let err = cache.resolve(&mgmt, &sm).await.expect_err("must error"); + assert!( + matches!(err, ReconcilerError::KubeconfigInvalid { .. }), + "expected KubeconfigInvalid, got: {err:?}" + ); + server.await.unwrap(); + } + + // ======================================================================== + // Error mapping: non-404 auto-discovery error propagates + // ======================================================================== + + #[tokio::test] + async fn test_auto_discovery_403_propagates_as_kube_error() { + let (mgmt, handle) = mock_client_pair(); + let cache = ChildClientCache::new(); + let sm = make_sm("team-a", "sm1", "alpha", None); + + let server = tokio::spawn(async move { + let mut h = pin!(handle); + let (_req, send) = h.next_request().await.unwrap(); + send.send_response(respond_with(403, k8s_403_body())); + }); + + let err = cache + .resolve(&mgmt, &sm) + .await + .expect_err("403 must propagate, not silently fall back"); + // Non-404 auto-discovery is wrapped as KubeError. + assert!( + matches!(err, ReconcilerError::KubeError(_)), + "expected KubeError, got: {err:?}" + ); + server.await.unwrap(); + assert_eq!(cache.len(), 0); + } + + // ======================================================================== + // CacheKey / ResolvedClient helpers + // ======================================================================== + + #[test] + fn test_cache_key_equality() { + let a = CacheKey::new("ns", "name"); + let b = CacheKey::new("ns", "name"); + let c = CacheKey::new("ns", "other"); + assert_eq!(a, b); + assert_ne!(a, c); + } + + #[test] + fn test_default_kubeconfig_secret_key_constant() { + assert_eq!( + DEFAULT_KUBECONFIG_SECRET_KEY, "value", + "default key must be `value` per CAPI convention; flips here will break auto-discovery" + ); + } + + // ======================================================================== + // ChildWatchHook plumbing — verifies the cache fires lifecycle callbacks + // in the right order. The real hook (ChildNodeWatchManager) starts and + // cancels per-child Node watchers in lock-step with these events. + // ======================================================================== + + /// Test double for `ChildWatchHook` — records every call so tests can + /// assert exact lifecycle sequences. + #[derive(Default)] + struct RecordingHook { + events: std::sync::Mutex>, + } + + #[derive(Debug, Clone, PartialEq, Eq)] + enum HookEvent { + Resolved(CacheKey), + Evicted(CacheKey), + } + + impl super::super::ChildWatchHook for RecordingHook { + fn on_child_resolved(&self, key: &CacheKey, _client: kube::Client) { + self.events + .lock() + .unwrap() + .push(HookEvent::Resolved(key.clone())); + } + fn on_child_evicted(&self, key: &CacheKey) { + self.events + .lock() + .unwrap() + .push(HookEvent::Evicted(key.clone())); + } + } + + impl RecordingHook { + fn events(&self) -> Vec { + self.events.lock().unwrap().clone() + } + } + + #[tokio::test] + async fn test_hook_fires_resolved_once_on_initial_build() { + let (mgmt, handle) = mock_client_pair(); + let cache = ChildClientCache::new(); + let hook = Arc::new(RecordingHook::default()); + cache.set_hook(hook.clone()); + + let sm = make_sm("team-a", "sm1", "alpha", None); + let server = tokio::spawn(async move { + let mut h = pin!(handle); + let (_req, send) = h.next_request().await.unwrap(); + send.send_response(respond_with( + 200, + secret_response_body( + "team-a", + "alpha-kubeconfig", + "value", + "1", + FIXTURE_KUBECONFIG_YAML, + ), + )); + }); + let _ = cache.resolve(&mgmt, &sm).await.unwrap(); + server.await.unwrap(); + assert_eq!( + hook.events(), + vec![HookEvent::Resolved(CacheKey::new( + "team-a", + "alpha-kubeconfig" + ))] + ); + } + + #[tokio::test] + async fn test_hook_does_not_fire_on_cache_hit_same_rv() { + // Same RV both times — the cache short-circuits without rebuilding + // and MUST NOT fire on_child_resolved a second time (it would + // re-start a watcher that is already running). + let (mgmt, handle) = mock_client_pair(); + let cache = ChildClientCache::new(); + let hook = Arc::new(RecordingHook::default()); + cache.set_hook(hook.clone()); + let sm = make_sm("team-a", "sm1", "alpha", None); + let server = tokio::spawn(async move { + let mut h = pin!(handle); + for _ in 0..2 { + let (_req, send) = h.next_request().await.unwrap(); + send.send_response(respond_with( + 200, + secret_response_body( + "team-a", + "alpha-kubeconfig", + "value", + "1", + FIXTURE_KUBECONFIG_YAML, + ), + )); + } + }); + let _ = cache.resolve(&mgmt, &sm).await.unwrap(); + let _ = cache.resolve(&mgmt, &sm).await.unwrap(); + server.await.unwrap(); + assert_eq!( + hook.events(), + vec![HookEvent::Resolved(CacheKey::new( + "team-a", + "alpha-kubeconfig" + ))], + "cache hits on identical RV must not re-fire on_child_resolved" + ); + } + + #[tokio::test] + async fn test_hook_fires_evicted_then_resolved_on_rv_change() { + // Token / cert rotation: Secret RV bumps. The watcher running + // with stale credentials must be cancelled before a fresh one + // is started with the rebuilt Client — assert the exact event + // ordering. + let (mgmt, handle) = mock_client_pair(); + let cache = ChildClientCache::new(); + let hook = Arc::new(RecordingHook::default()); + cache.set_hook(hook.clone()); + let sm = make_sm("team-a", "sm1", "alpha", None); + + let server = tokio::spawn(async move { + let mut h = pin!(handle); + // First resolve: RV "1" + let (_req, send) = h.next_request().await.unwrap(); + send.send_response(respond_with( + 200, + secret_response_body( + "team-a", + "alpha-kubeconfig", + "value", + "1", + FIXTURE_KUBECONFIG_YAML, + ), + )); + // Second: RV "2" + let (_req, send) = h.next_request().await.unwrap(); + send.send_response(respond_with( + 200, + secret_response_body( + "team-a", + "alpha-kubeconfig", + "value", + "2", + FIXTURE_KUBECONFIG_YAML, + ), + )); + }); + let _ = cache.resolve(&mgmt, &sm).await.unwrap(); + let _ = cache.resolve(&mgmt, &sm).await.unwrap(); + server.await.unwrap(); + + let key = CacheKey::new("team-a", "alpha-kubeconfig"); + assert_eq!( + hook.events(), + vec![ + HookEvent::Resolved(key.clone()), + HookEvent::Evicted(key.clone()), + HookEvent::Resolved(key), + ] + ); + } + + #[tokio::test] + async fn test_explicit_evict_fires_hook_when_entry_present() { + let (mgmt, handle) = mock_client_pair(); + let cache = ChildClientCache::new(); + let hook = Arc::new(RecordingHook::default()); + cache.set_hook(hook.clone()); + let sm = make_sm("team-a", "sm1", "alpha", None); + let server = tokio::spawn(async move { + let mut h = pin!(handle); + let (_req, send) = h.next_request().await.unwrap(); + send.send_response(respond_with( + 200, + secret_response_body( + "team-a", + "alpha-kubeconfig", + "value", + "1", + FIXTURE_KUBECONFIG_YAML, + ), + )); + }); + let _ = cache.resolve(&mgmt, &sm).await.unwrap(); + server.await.unwrap(); + let key = CacheKey::new("team-a", "alpha-kubeconfig"); + cache.evict(&key); + cache.evict(&key); // second evict on missing key — must not re-fire + assert_eq!( + hook.events(), + vec![HookEvent::Resolved(key.clone()), HookEvent::Evicted(key),] + ); + } + + #[tokio::test] + async fn test_hook_does_not_fire_for_management_fallback() { + // Auto-discovery 404 path — no child client was built, hook stays + // silent. (Watchers are only meaningful when there's a child + // Client to watch with.) + let (mgmt, handle) = mock_client_pair(); + let cache = ChildClientCache::new(); + let hook = Arc::new(RecordingHook::default()); + cache.set_hook(hook.clone()); + let sm = make_sm("team-a", "sm1", "alpha", None); + let server = tokio::spawn(async move { + let mut h = pin!(handle); + let (_req, send) = h.next_request().await.unwrap(); + send.send_response(respond_with(404, k8s_404_body("alpha-kubeconfig"))); + }); + let _ = cache.resolve(&mgmt, &sm).await.unwrap(); + server.await.unwrap(); + assert!( + hook.events().is_empty(), + "management fallback must not invoke the watch hook: {:?}", + hook.events() + ); + } +} diff --git a/src/reconcilers/child_watch.rs b/src/reconcilers/child_watch.rs new file mode 100644 index 0000000..d1698a0 --- /dev/null +++ b/src/reconcilers/child_watch.rs @@ -0,0 +1,239 @@ +// Copyright (c) 2025 Erick Bourgeois, RBC Capital Markets +// SPDX-License-Identifier: Apache-2.0 +//! # Per-child-cluster Node watcher manager +//! +//! Closes the event-driven gap left by the Phase 1 resolver work: now +//! that `Node` and `Pod` operations are correctly routed to the child +//! cluster, this module owns the inverse direction — child-cluster +//! `Node` events being routed back to the management cluster's +//! `Controller` so an SM is reconciled when its bound Node changes +//! `Ready`/`Unschedulable`/etc. +//! +//! ## Architecture +//! +//! 1. The manager implements [`crate::reconcilers::child_client::ChildWatchHook`] +//! and is installed on the cache via +//! `ChildClientCache::set_hook` at startup. +//! 2. When the cache builds (or rebuilds) a child `kube::Client` for a +//! given `CacheKey`, the manager spawns a `kube::runtime::watcher` +//! against `Api::::all(child_client)` and runs it in a Tokio +//! task. Each `Apply` / `InitApply` / `Delete` event is mapped via +//! [`crate::reconcilers::node_to_scheduled_machines_via_machine`] +//! using a snapshot of the management cluster's CAPI Machine +//! reflector store — the canonical, tenant-unforgeable Node→SM +//! mapping path established by the 2026-04-25 security audit. +//! 3. Mapped `ObjectRef`s are pushed onto an +//! `mpsc::Sender` whose corresponding `Receiver` is fed into +//! `Controller::reconcile_on` by `main.rs`. +//! 4. On cache eviction (LRU or RV-change rotation), the manager +//! `JoinHandle::abort()`s the running task. The watcher is +//! `Send + 'static`, abort-safe at every await point (kube-runtime +//! streams + Tokio mpsc both are), so abort never strands resources. +//! +//! ## Why one watcher per Secret, not per SM +//! +//! Two `ScheduledMachine`s in the same namespace pointing at the same +//! `kubeconfigSecretRef` (or both relying on the same +//! `-kubeconfig` auto-discovery) share one cached client +//! AND one Node watcher. Per-SM watchers would multiply the watch +//! count linearly with SM count; per-Secret watchers cap it at the +//! number of distinct child clusters. + +use std::collections::HashMap; +use std::sync::{Arc, Mutex}; + +use futures::StreamExt; +use k8s_openapi::api::core::v1::Node; +use kube::core::DynamicObject; +use kube::runtime::reflector; +use kube::runtime::reflector::ObjectRef; +use kube::runtime::watcher; +use kube::{Api, Client}; +use tokio::sync::mpsc; +use tokio::task::JoinHandle; +use tracing::{debug, info, warn}; + +use crate::crd::ScheduledMachine; +use crate::reconcilers::child_client::{CacheKey, ChildWatchHook}; +use crate::reconcilers::node_to_scheduled_machines_via_machine; + +/// Manages the lifecycle of one per-child-cluster `Node` watcher per +/// active `CacheKey`. Implements [`ChildWatchHook`] so the +/// [`crate::reconcilers::child_client::ChildClientCache`] drives start / +/// cancel events directly. +/// +/// Cheaply cloneable via the shared `Arc`-wrapped state. +#[derive(Clone)] +pub struct ChildNodeWatchManager { + inner: Arc, +} + +struct Inner { + tasks: Mutex>>, + tx: mpsc::Sender>, + machine_store: reflector::Store, +} + +impl ChildNodeWatchManager { + /// Construct a fresh manager. `tx` is the channel into the + /// `Controller::reconcile_on` stream; `machine_store` is the + /// management cluster's CAPI Machine reflector that the canonical + /// Node→SM mapper reads from. + #[must_use] + pub fn new( + tx: mpsc::Sender>, + machine_store: reflector::Store, + ) -> Self { + Self { + inner: Arc::new(Inner { + tasks: Mutex::new(HashMap::new()), + tx, + machine_store, + }), + } + } + + /// Number of currently-running watcher tasks. Exposed for tests + + /// future Phase 2 health endpoint. + #[must_use] + pub fn task_count(&self) -> usize { + self.inner + .tasks + .lock() + .expect("child watch manager lock poisoned") + .len() + } + + /// `true` iff a watcher task is currently tracked (running or just + /// recently aborted but not yet pruned) for the given `CacheKey`. + /// Tests use this to assert lifecycle ordering. + #[must_use] + pub fn has_watcher(&self, key: &CacheKey) -> bool { + self.inner + .tasks + .lock() + .expect("child watch manager lock poisoned") + .contains_key(key) + } +} + +impl ChildWatchHook for ChildNodeWatchManager { + fn on_child_resolved(&self, key: &CacheKey, client: Client) { + // Defensive cancel-then-start: if a stale task exists (shouldn't + // happen because the cache fires `on_child_evicted` before + // re-resolving the same key, but the invariant is cheap to + // enforce), drop it. JoinHandle::drop does NOT abort, so call + // abort() explicitly. + let mut guard = self + .inner + .tasks + .lock() + .expect("child watch manager lock poisoned"); + if let Some(existing) = guard.remove(key) { + existing.abort(); + } + + let tx = self.inner.tx.clone(); + let machine_store = self.inner.machine_store.clone(); + let key_owned = key.clone(); + let api: Api = Api::all(client); + let join = tokio::spawn(run_node_watcher(api, tx, machine_store, key_owned)); + guard.insert(key.clone(), join); + + debug!( + namespace = %key.namespace, + secret = %key.secret_name, + "spawned child-cluster Node watcher" + ); + } + + fn on_child_evicted(&self, key: &CacheKey) { + let mut guard = self + .inner + .tasks + .lock() + .expect("child watch manager lock poisoned"); + if let Some(join) = guard.remove(key) { + join.abort(); + debug!( + namespace = %key.namespace, + secret = %key.secret_name, + "aborted child-cluster Node watcher (cache eviction)" + ); + } + } +} + +/// The watcher loop body. Pure async fn so it's easy to spawn — see +/// [`ChildNodeWatchManager::on_child_resolved`]. +/// +/// Exits when: +/// - the spawning task is `abort()`-ed (Tokio cancels at the next +/// await point), or +/// - the underlying `kube::runtime::watcher` stream terminates +/// (rare — `watcher::watcher` reconnects internally on transient +/// errors, so this typically only happens on terminal auth failure). +async fn run_node_watcher( + api: Api, + tx: mpsc::Sender>, + machine_store: reflector::Store, + key: CacheKey, +) { + info!( + namespace = %key.namespace, + secret = %key.secret_name, + "child-cluster Node watcher starting" + ); + let mut stream = watcher::watcher(api, watcher::Config::default()).boxed(); + while let Some(evt) = stream.next().await { + match evt { + Ok(watcher::Event::Apply(node) | watcher::Event::InitApply(node)) => { + emit_refs_for_node(&node, &machine_store, &tx).await; + } + Ok(watcher::Event::Delete(node)) => { + emit_refs_for_node(&node, &machine_store, &tx).await; + } + // `Init` and `InitDone` are bookkeeping for the initial + // list — no Node payload to map. The first concrete event + // is `InitApply`, which we handle above. + Ok(watcher::Event::Init | watcher::Event::InitDone) => {} + Err(e) => { + warn!( + namespace = %key.namespace, + secret = %key.secret_name, + error = %e, + "child-cluster Node watcher error; kube-runtime will reconnect" + ); + } + } + } + info!( + namespace = %key.namespace, + secret = %key.secret_name, + "child-cluster Node watcher stream ended" + ); +} + +/// Map one Node event into zero-or-more `ObjectRef`s +/// via the canonical CAPI Machine ownership chain and send each ref +/// on the manager's mpsc. Failure to send (receiver dropped) is logged +/// at debug — that means the controller is shutting down; the next +/// shutdown signal will tear the watcher down anyway. +async fn emit_refs_for_node( + node: &Node, + machine_store: &reflector::Store, + tx: &mpsc::Sender>, +) { + let snapshot = machine_store.state(); + let refs = node_to_scheduled_machines_via_machine(node, snapshot.iter().map(AsRef::as_ref)); + for r in refs { + if tx.send(r).await.is_err() { + debug!("controller reconcile_on receiver dropped; skipping Node→SM ref dispatch"); + return; + } + } +} + +#[cfg(test)] +#[path = "child_watch_tests.rs"] +mod child_watch_tests; diff --git a/src/reconcilers/child_watch_tests.rs b/src/reconcilers/child_watch_tests.rs new file mode 100644 index 0000000..d28c459 --- /dev/null +++ b/src/reconcilers/child_watch_tests.rs @@ -0,0 +1,282 @@ +// Copyright (c) 2025 Erick Bourgeois, RBC Capital Markets +// SPDX-License-Identifier: Apache-2.0 +//! Tests for [`super::ChildNodeWatchManager`]. The actual `kube::runtime` +//! `watcher` stream is hard to drive deterministically from a unit test, +//! so we focus on what we own: the lifecycle invariants of the manager +//! itself (spawn-on-resolve, abort-on-evict, idempotent restart, mpsc +//! plumbing into `emit_refs_for_node`). +//! +//! The pure `node_to_scheduled_machines_via_machine` mapper is exercised +//! in `helpers_tests.rs`; the integration of that mapper with a live +//! Node watch stream is left to the Phase 1.10 dual-cluster integration +//! test under `tests/`. + +#[cfg(test)] +#[allow(clippy::module_inception)] +mod tests { + use super::super::*; + use crate::reconcilers::child_client::{CacheKey, ChildWatchHook}; + use http::{Request, Response}; + use k8s_openapi::api::core::v1::Node; + use kube::api::{ApiResource, GroupVersionKind, ObjectMeta}; + use kube::client::Body; + use kube::core::DynamicObject; + use kube::runtime::reflector; + use kube::runtime::reflector::ObjectRef; + use std::collections::BTreeMap; + use std::sync::Arc; + use tokio::sync::mpsc; + use tower_test::mock; + + fn make_mock_client() -> kube::Client { + // We never drive the mock — the test asserts manager state, not + // watcher I/O. The kube::Client just needs to be constructible + // so the manager can call Api::all on it. + let (svc, _handle) = mock::pair::, Response>(); + kube::Client::new(svc, "default") + } + + fn empty_machine_store() -> reflector::Store { + let ar = ApiResource::from_gvk_with_plural( + &GroupVersionKind::gvk("cluster.x-k8s.io", "v1beta1", "Machine"), + "machines", + ); + let writer: reflector::store::Writer = reflector::store::Writer::new(ar); + writer.as_reader() + } + + fn machine_store_with(machines: Vec) -> reflector::Store { + let ar = ApiResource::from_gvk_with_plural( + &GroupVersionKind::gvk("cluster.x-k8s.io", "v1beta1", "Machine"), + "machines", + ); + let mut writer: reflector::store::Writer = reflector::store::Writer::new(ar); + writer.apply_watcher_event(&kube::runtime::watcher::Event::Init); + for m in &machines { + writer.apply_watcher_event(&kube::runtime::watcher::Event::InitApply(m.clone())); + } + writer.apply_watcher_event(&kube::runtime::watcher::Event::InitDone); + writer.as_reader() + } + + /// Synthesise a CAPI Machine whose `status.nodeRef.name` points at + /// `node_name` and whose `metadata.labels[LABEL_SCHEDULED_MACHINE]` + /// is `sm_name` in `namespace`. Matches the shape the mapper + /// `node_to_scheduled_machines_via_machine` expects. + fn machine_for_sm(namespace: &str, sm_name: &str, node_name: &str) -> DynamicObject { + let ar = ApiResource::from_gvk_with_plural( + &GroupVersionKind::gvk("cluster.x-k8s.io", "v1beta1", "Machine"), + "machines", + ); + let mut labels = BTreeMap::new(); + labels.insert( + crate::labels::LABEL_SCHEDULED_MACHINE.to_string(), + sm_name.to_string(), + ); + DynamicObject { + types: Some(kube::api::TypeMeta { + api_version: "cluster.x-k8s.io/v1beta1".to_string(), + kind: "Machine".to_string(), + }), + metadata: ObjectMeta { + name: Some(format!("{sm_name}-machine")), + namespace: Some(namespace.to_string()), + labels: Some(labels), + ..Default::default() + }, + data: serde_json::json!({ + "status": { "nodeRef": { "name": node_name } } + }), + } + // `ar` is unused in the field set but documents intent; the + // mapper reads from `metadata` + `data` only. + .tap(|_| { + let _ = ar; + }) + } + + /// Tiny no-op `Tap` helper so we can keep the trailing `let _ = ar;` + /// readable. Avoids pulling in `tap` as a dep. + trait Tap: Sized { + fn tap(self, f: F) -> Self { + f(&self); + self + } + } + impl Tap for T {} + + // ======================================================================== + // Lifecycle: spawn on resolve, abort on evict + // ======================================================================== + + #[tokio::test] + async fn test_resolve_spawns_watcher_task() { + let (tx, _rx) = mpsc::channel::>(8); + let manager = ChildNodeWatchManager::new(tx, empty_machine_store()); + let key = CacheKey::new("team-a", "alpha-kubeconfig"); + + assert_eq!(manager.task_count(), 0); + manager.on_child_resolved(&key, make_mock_client()); + assert!(manager.has_watcher(&key)); + assert_eq!(manager.task_count(), 1); + } + + #[tokio::test] + async fn test_evict_aborts_watcher_task() { + let (tx, _rx) = mpsc::channel::>(8); + let manager = ChildNodeWatchManager::new(tx, empty_machine_store()); + let key = CacheKey::new("team-a", "alpha-kubeconfig"); + + manager.on_child_resolved(&key, make_mock_client()); + assert_eq!(manager.task_count(), 1); + + manager.on_child_evicted(&key); + assert!(!manager.has_watcher(&key)); + assert_eq!(manager.task_count(), 0); + } + + #[tokio::test] + async fn test_evict_of_unknown_key_is_noop() { + let (tx, _rx) = mpsc::channel::>(8); + let manager = ChildNodeWatchManager::new(tx, empty_machine_store()); + // Evicting before any resolve must not panic and must leave the + // task map empty. + manager.on_child_evicted(&CacheKey::new("nope", "missing")); + assert_eq!(manager.task_count(), 0); + } + + #[tokio::test] + async fn test_resolve_then_evict_then_resolve_restarts_cleanly() { + // Token rotation simulation: resolve, evict (RV change), resolve + // again. Final state must have exactly one task running for the + // key — no zombie tasks left behind by the first invocation. + let (tx, _rx) = mpsc::channel::>(8); + let manager = ChildNodeWatchManager::new(tx, empty_machine_store()); + let key = CacheKey::new("team-a", "alpha-kubeconfig"); + + manager.on_child_resolved(&key, make_mock_client()); + manager.on_child_evicted(&key); + manager.on_child_resolved(&key, make_mock_client()); + + assert_eq!(manager.task_count(), 1); + assert!(manager.has_watcher(&key)); + } + + #[tokio::test] + async fn test_double_resolve_same_key_replaces_old_task() { + // Defensive invariant: even if the cache misses a cancel-then-resolve + // ordering bug, calling on_child_resolved twice for the same key + // must abort the previous task before installing a new one. + let (tx, _rx) = mpsc::channel::>(8); + let manager = ChildNodeWatchManager::new(tx, empty_machine_store()); + let key = CacheKey::new("team-a", "alpha-kubeconfig"); + + manager.on_child_resolved(&key, make_mock_client()); + manager.on_child_resolved(&key, make_mock_client()); + + assert_eq!( + manager.task_count(), + 1, + "back-to-back resolves must not leak watcher tasks" + ); + } + + #[tokio::test] + async fn test_multiple_keys_run_independent_watchers() { + let (tx, _rx) = mpsc::channel::>(8); + let manager = ChildNodeWatchManager::new(tx, empty_machine_store()); + let k1 = CacheKey::new("team-a", "alpha-kubeconfig"); + let k2 = CacheKey::new("team-b", "beta-kubeconfig"); + + manager.on_child_resolved(&k1, make_mock_client()); + manager.on_child_resolved(&k2, make_mock_client()); + assert_eq!(manager.task_count(), 2); + + manager.on_child_evicted(&k1); + assert!(!manager.has_watcher(&k1)); + assert!(manager.has_watcher(&k2)); + assert_eq!(manager.task_count(), 1); + } + + // ======================================================================== + // Node→SM mapping plumbed through mpsc + // ======================================================================== + + #[tokio::test] + async fn test_emit_refs_pushes_owning_sm_on_channel() { + // Direct test of the pure helper that runs inside the watcher + // loop: given a Node and a Machine store with one matching + // entry, the corresponding ObjectRef must arrive on the mpsc. + let (tx, mut rx) = mpsc::channel::>(8); + let store = machine_store_with(vec![machine_for_sm("team-a", "gpu-worker", "worker-01")]); + let node = Node { + metadata: ObjectMeta { + name: Some("worker-01".to_string()), + ..Default::default() + }, + ..Default::default() + }; + + super::super::emit_refs_for_node(&node, &store, &tx).await; + let received = rx + .try_recv() + .expect("expected one ObjectRef on the channel"); + assert_eq!(received.name, "gpu-worker"); + assert_eq!(received.namespace.as_deref(), Some("team-a")); + } + + #[tokio::test] + async fn test_emit_refs_no_matching_machine_pushes_nothing() { + let (tx, mut rx) = mpsc::channel::>(8); + let store = machine_store_with(vec![machine_for_sm( + "team-a", + "gpu-worker", + "different-node", + )]); + let node = Node { + metadata: ObjectMeta { + name: Some("worker-01".to_string()), + ..Default::default() + }, + ..Default::default() + }; + super::super::emit_refs_for_node(&node, &store, &tx).await; + assert!(rx.try_recv().is_err(), "no matching Machine ⇒ no Ref"); + } + + #[tokio::test] + async fn test_emit_refs_silently_drops_when_receiver_closed() { + // Controller shutdown sequence: rx is dropped while the watcher + // is still running. Send must not panic — the watcher's job is + // best-effort, the controller's shutdown signal will tear it + // down separately. + let (tx, rx) = mpsc::channel::>(1); + drop(rx); + let store = machine_store_with(vec![machine_for_sm("team-a", "gpu-worker", "worker-01")]); + let node = Node { + metadata: ObjectMeta { + name: Some("worker-01".to_string()), + ..Default::default() + }, + ..Default::default() + }; + // No panic, no hang. + super::super::emit_refs_for_node(&node, &store, &tx).await; + } + + // ======================================================================== + // ChildClientCache integration: installing the manager as a hook + // ======================================================================== + + #[test] + fn test_manager_implements_child_watch_hook() { + // Compile-time assertion: ChildNodeWatchManager can be stored + // as Arc, which is what ChildClientCache::set_hook + // takes. + fn _accept(_h: Arc) {} + let (tx, _rx) = mpsc::channel::>(1); + let manager: Arc = + Arc::new(ChildNodeWatchManager::new(tx, empty_machine_store())); + _accept(manager); + } +} diff --git a/src/reconcilers/helpers.rs b/src/reconcilers/helpers.rs index 4bb8ec9..5b357b8 100644 --- a/src/reconcilers/helpers.rs +++ b/src/reconcilers/helpers.rs @@ -26,6 +26,9 @@ use std::time::Duration; use chrono::{DateTime, Datelike, Timelike, Utc}; use chrono_tz::Tz; +use k8s_openapi::api::authorization::v1::{ + ResourceAttributes, SelfSubjectAccessReview, SelfSubjectAccessReviewSpec, +}; use k8s_openapi::api::core::v1::ObjectReference; use kube::{ api::{Api, Patch, PatchParams}, @@ -47,8 +50,9 @@ use crate::constants::{ FINALIZER_SCHEDULED_MACHINE, MAX_BACKOFF_SECS, MAX_CLUSTER_NAME_LEN, MAX_DURATION_SECS, MAX_KILL_IF_COMMANDS_COUNT, MAX_KILL_IF_COMMAND_LEN, MAX_RECONCILE_RETRIES, PHASE_ACTIVE, PHASE_ERROR, PHASE_INACTIVE, PHASE_SHUTTING_DOWN, PHASE_TERMINATED, - POD_EVICTION_GRACE_PERIOD_SECS, REASON_GRACE_PERIOD, REASON_KILL_SWITCH, - REASON_RECONCILE_SUCCEEDED, RESERVED_LABEL_PREFIXES, TIMER_REQUEUE_SECS, + POD_EVICTION_GRACE_PERIOD_SECS, RBAC_VERB_CREATE, REASON_GRACE_PERIOD, REASON_KILL_SWITCH, + REASON_RECONCILE_SUCCEEDED, RESERVED_LABEL_PREFIXES, SCHEDULED_MACHINE_LABEL, + TIMER_REQUEUE_SECS, }; use crate::crd::{Condition, NodeRef, ScheduledMachine, ScheduledMachineStatus}; use crate::metrics::{ @@ -910,6 +914,75 @@ pub async fn update_phase_with_grace_period( // Security validation helpers // ============================================================================ +/// Validate the optional `metadata` block of an embedded bootstrap / +/// infrastructure resource. +/// +/// The controller owns the created resource's **identity**: +/// - `metadata.namespace` — always the `ScheduledMachine`'s own namespace +/// (cross-namespace creation is forbidden, threat T1). +/// - `metadata.name` — always the `ScheduledMachine` name (deletion in +/// [`remove_machine_from_cluster`] relies on this). +/// +/// Both are therefore **rejected** if set, rather than silently overridden, so +/// a user gets clear feedback. Only `metadata.labels` and `metadata.annotations` +/// are user-settable, and they are run through [`validate_labels`] so a user +/// cannot forge reserved-prefix keys (e.g. `cluster.x-k8s.io/cluster-name`, +/// which would redirect the machine to another cluster — threat T2). +/// +/// This is the runtime (reconcile-time) half of a two-layer guard; the +/// admission-time half lives in the `ValidatingAdmissionPolicy`. +/// +/// # Errors +/// [`ReconcilerError::ValidationError`] if `metadata.name`/`metadata.namespace` +/// is set, or a label/annotation key uses a reserved prefix. +pub fn validate_embedded_metadata( + embedded: &crate::crd::EmbeddedResource, + field: &str, +) -> Result<(), ReconcilerError> { + if embedded.metadata_namespace().is_some() { + return Err(ReconcilerError::ValidationError(format!( + "{field}.metadata.namespace is not permitted — the controller always creates \ + the resource in the ScheduledMachine's own namespace" + ))); + } + if embedded.metadata_name().is_some() { + return Err(ReconcilerError::ValidationError(format!( + "{field}.metadata.name is not permitted — the controller names the resource \ + after the ScheduledMachine" + ))); + } + + validate_labels( + &embedded.metadata_labels(), + &format!("{field}.metadata.labels"), + )?; + validate_labels( + &embedded.metadata_annotations(), + &format!("{field}.metadata.annotations"), + )?; + Ok(()) +} + +/// Merge the controller-owned tracking labels onto a user-supplied label map. +/// +/// The two reserved labels ([`SCHEDULED_MACHINE_LABEL`] and +/// [`CAPI_CLUSTER_NAME_LABEL`]) are inserted **last** so they always take +/// precedence over any user-provided entry. Users cannot legitimately set these +/// keys anyway — [`validate_embedded_metadata`] rejects reserved prefixes — so +/// this is a defensive guarantee rather than a conflict resolver. +fn merge_owned_labels( + mut labels: BTreeMap, + sm_name: &str, + cluster_name: &str, +) -> BTreeMap { + labels.insert(SCHEDULED_MACHINE_LABEL.to_string(), sm_name.to_string()); + labels.insert( + CAPI_CLUSTER_NAME_LABEL.to_string(), + cluster_name.to_string(), + ); + labels +} + /// Reject label/annotation maps that contain reserved key prefixes. /// /// Users must not be able to override system labels such as @@ -1145,12 +1218,52 @@ pub async fn add_machine_to_cluster( "infrastructure", )?; + // RBAC pre-flight: confirm the controller's service account may create every + // resource type up front, before creating any of them. This fails fast with + // a clear PermissionDenied error (naming the resource) instead of an opaque + // 403 surfacing partway through — which could leave a bootstrap resource + // orphaned because the infrastructure create was denied. The matching + // permission check on the *creating user* (privilege-escalation guard) runs + // at admission via the ValidatingAdmissionPolicy `authorizer` rules. + ensure_can_create( + client, + namespace, + bootstrap_api_version, + bootstrap_kind, + "bootstrap", + ) + .await?; + ensure_can_create( + client, + namespace, + infra_api_version, + infra_kind, + "infrastructure", + ) + .await?; + ensure_can_create( + client, + namespace, + CAPI_MACHINE_API_VERSION_FULL, + "Machine", + "machine", + ) + .await?; + // Validate user-supplied labels and annotations do not use reserved prefixes if let Some(template) = &resource.spec.machine_template { validate_labels(&template.labels, "machineTemplate.labels")?; validate_labels(&template.annotations, "machineTemplate.annotations")?; } + // Validate the embedded bootstrap/infra metadata: reject controller-owned + // identity fields (name/namespace) and reserved-prefix labels/annotations. + validate_embedded_metadata(&resource.spec.bootstrap_spec, "spec.bootstrapSpec")?; + validate_embedded_metadata( + &resource.spec.infrastructure_spec, + "spec.infrastructureSpec", + )?; + let owner_ref = json!({ "apiVersion": API_VERSION_FULL, "kind": "ScheduledMachine", @@ -1171,16 +1284,25 @@ pub async fn add_machine_to_cluster( // NOTE: No ownerReferences here - the bootstrap controller (e.g., k0smotron) needs to // process this resource. We use labels for tracking instead, and the CAPI Machine's // bootstrap.configRef provides the logical relationship. + // + // User-supplied metadata.labels/annotations are merged in first; the + // controller-owned tracking labels are inserted last so they always win + // (users cannot set reserved prefixes anyway — validate_embedded_metadata + // rejects those above). + let bootstrap_labels = merge_owned_labels( + resource.spec.bootstrap_spec.metadata_labels(), + &name, + cluster_name, + ); + let bootstrap_annotations = resource.spec.bootstrap_spec.metadata_annotations(); let bootstrap_obj = json!({ "apiVersion": bootstrap_api_version, "kind": bootstrap_kind, "metadata": { "name": name, "namespace": bootstrap_ns, - "labels": { - "5spot.finos.org/scheduled-machine": name, - CAPI_CLUSTER_NAME_LABEL: cluster_name, - }, + "labels": bootstrap_labels, + "annotations": bootstrap_annotations, }, "spec": bootstrap_spec_inner, }); @@ -1201,16 +1323,20 @@ pub async fn add_machine_to_cluster( // NOTE: No ownerReferences here - the infrastructure controller (e.g., CAPM3, CAPA) needs to // process this resource. We use labels for tracking instead, and the CAPI Machine's // infrastructureRef provides the logical relationship. + let infra_labels = merge_owned_labels( + resource.spec.infrastructure_spec.metadata_labels(), + &name, + cluster_name, + ); + let infra_annotations = resource.spec.infrastructure_spec.metadata_annotations(); let infra_obj = json!({ "apiVersion": infra_api_version, "kind": infra_kind, "metadata": { "name": name, "namespace": infra_ns, - "labels": { - "5spot.finos.org/scheduled-machine": name, - CAPI_CLUSTER_NAME_LABEL: cluster_name, - }, + "labels": infra_labels, + "annotations": infra_annotations, }, "spec": infra_spec_inner, }); @@ -1223,20 +1349,16 @@ pub async fn add_machine_to_cluster( info!(kind = %infra_kind, "Infrastructure resource created"); - // 3. Create CAPI Machine referencing both - let mut machine_labels = std::collections::BTreeMap::new(); - machine_labels.insert(CAPI_CLUSTER_NAME_LABEL.to_string(), cluster_name.clone()); - machine_labels.insert( - "5spot.finos.org/scheduled-machine".to_string(), - name.clone(), - ); - - // Merge in user-provided labels + // 3. Create CAPI Machine referencing both. + // Start from user-provided machineTemplate labels, then stamp the + // controller-owned tracking labels last so they always win. + let mut machine_labels = BTreeMap::new(); if let Some(template) = &resource.spec.machine_template { for (k, v) in &template.labels { machine_labels.insert(k.clone(), v.clone()); } } + let machine_labels = merge_owned_labels(machine_labels, &name, cluster_name); let mut machine_annotations = std::collections::BTreeMap::new(); // Merge in user-provided annotations @@ -1292,6 +1414,99 @@ pub async fn add_machine_to_cluster( /// Post a generic Kubernetes resource via the dynamic API client. /// +/// Derive the lowercase plural resource name from a Kubernetes `kind`. +/// +/// Uses the naive `lowercase(kind) + "s"` rule (e.g. `K0sWorkerConfig` → +/// `k0sworkerconfigs`, `RemoteMachine` → `remotemachines`). This matches the +/// plural CAPI/k0smotron providers register for their CRDs, so the value is +/// safe to use both when POSTing the dynamic resource and when naming the +/// resource in a [`SelfSubjectAccessReview`]. +/// +/// Keeping the plural derivation in one place guarantees the +/// `SelfSubjectAccessReview` checks (see [`ensure_can_create`]) and the actual +/// create call ([`create_dynamic_resource`]) always agree on the resource name +/// — a mismatch would make the access review check the wrong RBAC rule. +fn resource_plural(kind: &str) -> String { + format!("{}s", kind.to_lowercase()) +} + +/// Verify the controller's service account is permitted to `create` the given +/// resource type in `namespace` via a [`SelfSubjectAccessReview`] (SSAR). +/// +/// This is a fail-fast pre-flight check run before any embedded bootstrap / +/// infrastructure resource or the CAPI `Machine` is created. Without it, an +/// RBAC gap would only surface as an opaque `403` partway through +/// [`add_machine_to_cluster`], potentially after some resources were already +/// created. By checking first, the controller reports a clear +/// [`ReconcilerError::PermissionDenied`] naming the resource type and never +/// performs a partial creation. +/// +/// The check targets the controller's *own* identity (`SelfSubjectAccessReview`). +/// The equivalent check on the *creating user* — preventing privilege +/// escalation through the broadly-permissioned controller — is enforced at +/// admission time by the `authorizer` rules in the ValidatingAdmissionPolicy. +/// +/// `resource_label` is a short human label (`"bootstrap"`, `"infrastructure"`, +/// `"machine"`) used only in the error message. +/// +/// # Errors +/// - [`ReconcilerError::PermissionDenied`] — the SSAR returned `allowed = false`. +/// - [`ReconcilerError::KubeError`] — the SSAR API call itself failed. +async fn ensure_can_create( + client: &Client, + namespace: &str, + api_version: &str, + kind: &str, + resource_label: &str, +) -> Result<(), ReconcilerError> { + let (group, version) = parse_api_version(api_version); + let plural = resource_plural(kind); + + let review = SelfSubjectAccessReview { + spec: SelfSubjectAccessReviewSpec { + resource_attributes: Some(ResourceAttributes { + verb: Some(RBAC_VERB_CREATE.to_string()), + group: Some(group.clone()), + version: Some(version), + resource: Some(plural.clone()), + namespace: Some(namespace.to_string()), + ..ResourceAttributes::default() + }), + ..SelfSubjectAccessReviewSpec::default() + }, + ..SelfSubjectAccessReview::default() + }; + + // SelfSubjectAccessReview is a cluster-scoped virtual resource: the API + // evaluates the request against the caller's identity and echoes back the + // decision in `.status`. + let api: Api = Api::all(client.clone()); + let result = api + .create(&kube::api::PostParams::default(), &review) + .await?; + + let status = result.status.unwrap_or_default(); + if status.allowed { + debug!( + resource = %resource_label, + group = %group, + plural = %plural, + "Controller is permitted to create resource type" + ); + return Ok(()); + } + + let reason = status + .reason + .filter(|r| !r.is_empty()) + .unwrap_or_else(|| "no reason provided by the API server".to_string()); + + Err(ReconcilerError::PermissionDenied(format!( + "controller service account is not permitted to create {resource_label} resource \ + '{plural}' in API group '{group}' (namespace '{namespace}'): {reason}" + ))) +} + /// Converts `api_version` and `kind` into a [`kube::api::ApiResource`] and /// issues a `POST` to the namespaced resource endpoint. The function is used /// to create CAPI bootstrap, infrastructure, and `Machine` objects whose types @@ -1307,7 +1522,7 @@ async fn create_dynamic_resource( obj: serde_json::Value, ) -> Result<(), kube::Error> { let (group, version) = parse_api_version(api_version); - let plural = format!("{}s", kind.to_lowercase()); + let plural = resource_plural(kind); let ar = kube::api::ApiResource::from_gvk_with_plural( &kube::api::GroupVersionKind::gvk(&group, &version, kind), @@ -1358,7 +1573,7 @@ async fn delete_dynamic_resource( name: &str, ) -> Result<(), ReconcilerError> { let (group, version) = parse_api_version(api_version); - let plural = format!("{}s", kind.to_lowercase()); + let plural = resource_plural(kind); let ar = kube::api::ApiResource::from_gvk_with_plural( &kube::api::GroupVersionKind::gvk(&group, &version, kind), @@ -2375,6 +2590,15 @@ pub async fn handle_emergency_remove( .await; } + // Resolve the child-cluster client for the Node + Pod operations in + // this flow (drain + clear-reclaim-annotations). The CAPI Machine + // delete and the schedule-disable patch still go through the + // management client below — they live on the management cluster. + // Fail-closed: an unresolvable kubeconfig surfaces as a regular + // reconcile error and back-offs, rather than silently routing + // emergency drain to the management cluster. + let child_for_node = ctx.child_clients.resolve(&ctx.client, &resource).await?; + // Step 3: drain with short emergency timeout. Best-effort — we do // NOT block Machine deletion on a failed drain, because the agent // has already decided the node must leave. The stopwatch is @@ -2382,7 +2606,7 @@ pub async fn handle_emergency_remove( // P95 vs timeout-rate side by side. let drain_start = std::time::Instant::now(); let drain_result = drain_node_with_timeout( - &ctx.client, + child_for_node.client(), node_name, Duration::from_secs(EMERGENCY_DRAIN_TIMEOUT_SECS), ) @@ -2419,8 +2643,10 @@ pub async fn handle_emergency_remove( .await; // Step 6: clear reclaim annotations. Best-effort — failure only - // triggers an idempotent replay on the next reconcile. - clear_reclaim_annotations_best_effort(&ctx.client, node_name).await; + // triggers an idempotent replay on the next reconcile. Reaches into + // the child cluster's Node, so we reuse the resolved client built + // for the drain in step 3. + clear_reclaim_annotations_best_effort(child_for_node.client(), node_name).await; // Step 7: finalise state machine transition to Disabled. update_phase( diff --git a/src/reconcilers/helpers_tests.rs b/src/reconcilers/helpers_tests.rs index e27cb44..0926c4d 100644 --- a/src/reconcilers/helpers_tests.rs +++ b/src/reconcilers/helpers_tests.rs @@ -1944,6 +1944,7 @@ mod tests { kill_switch: false, node_taints: vec![], kill_if_commands: None, + kubeconfig_secret_ref: None, }, status: None, }; @@ -3525,6 +3526,7 @@ mod tests { kill_switch: false, node_taints: vec![], kill_if_commands: None, + kubeconfig_secret_ref: None, }, status: Some(crate::crd::ScheduledMachineStatus { phase: Some("ShuttingDown".to_string()), @@ -4046,4 +4048,355 @@ mod tests { let refs = node_to_scheduled_machines_via_machine(&node, machines.iter()); assert!(refs.is_empty()); } + + // ======================================================================== + // resource_plural — pure unit tests + // ======================================================================== + + #[test] + fn test_resource_plural_bootstrap_kind() { + // K0sWorkerConfig is registered as the plural "k0sworkerconfigs". + assert_eq!( + super::super::resource_plural("K0sWorkerConfig"), + "k0sworkerconfigs" + ); + } + + #[test] + fn test_resource_plural_infrastructure_kind() { + // RemoteMachine is registered as the plural "remotemachines". + assert_eq!( + super::super::resource_plural("RemoteMachine"), + "remotemachines" + ); + } + + #[test] + fn test_resource_plural_machine_matches_capi_constant() { + // The derived plural for the CAPI Machine must match the constant used + // when the controller actually deletes it — otherwise the SSAR check + // and the create/delete call would target different RBAC resources. + assert_eq!( + super::super::resource_plural("Machine"), + crate::constants::CAPI_RESOURCE_MACHINES + ); + } + + #[test] + fn test_resource_plural_already_lowercase() { + // Idempotent on an already-lowercase single-word kind. + assert_eq!(super::super::resource_plural("machine"), "machines"); + } + + // ======================================================================== + // ensure_can_create — SelfSubjectAccessReview mock API tests (TDD) + // ======================================================================== + + /// SelfSubjectAccessReview response body with the given decision. + fn ssar_response_body(allowed: bool, reason: &str) -> Vec { + serde_json::to_vec(&serde_json::json!({ + "apiVersion": "authorization.k8s.io/v1", + "kind": "SelfSubjectAccessReview", + "metadata": {}, + "spec": { "resourceAttributes": {} }, + "status": { "allowed": allowed, "reason": reason } + })) + .unwrap() + } + + // ---- Positive: allowed=true returns Ok ---- + + #[tokio::test] + async fn test_ensure_can_create_allowed_returns_ok() { + let (client, handle) = mock_client_pair(); + + let srv = tokio::spawn(async move { + let mut h = pin!(handle); + let (req, send) = h.next_request().await.expect("expected SSAR POST"); + assert_eq!(req.method(), http::Method::POST); + assert!( + req.uri().path().contains("selfsubjectaccessreviews"), + "should POST a SelfSubjectAccessReview, got: {}", + req.uri().path() + ); + send.send_response( + Response::builder() + .status(201) + .header("content-type", "application/json") + .body(Body::from(ssar_response_body(true, ""))) + .unwrap(), + ); + }); + + super::super::ensure_can_create( + &client, + "default", + "bootstrap.cluster.x-k8s.io/v1beta1", + "K0sWorkerConfig", + "bootstrap", + ) + .await + .expect("allowed=true should return Ok"); + + srv.await.unwrap(); + } + + // ---- Negative: allowed=false returns PermissionDenied ---- + + #[tokio::test] + async fn test_ensure_can_create_denied_returns_permission_denied() { + let (client, handle) = mock_client_pair(); + + let srv = tokio::spawn(async move { + let mut h = pin!(handle); + let (_req, send) = h.next_request().await.expect("expected SSAR POST"); + send.send_response( + Response::builder() + .status(201) + .header("content-type", "application/json") + .body(Body::from(ssar_response_body( + false, + "RBAC: no rules grant create on remotemachines", + ))) + .unwrap(), + ); + }); + + let err = super::super::ensure_can_create( + &client, + "default", + "infrastructure.cluster.x-k8s.io/v1beta1", + "RemoteMachine", + "infrastructure", + ) + .await + .expect_err("allowed=false should return an error"); + + match err { + ReconcilerError::PermissionDenied(msg) => { + assert!( + msg.contains("infrastructure") && msg.contains("remotemachines"), + "error should name the resource type, got: {msg}" + ); + assert!( + msg.contains("RBAC: no rules grant create"), + "error should include the API server reason, got: {msg}" + ); + } + other => panic!("expected PermissionDenied, got: {other:?}"), + } + + srv.await.unwrap(); + } + + // ---- Negative: allowed=false with no reason still denies cleanly ---- + + #[tokio::test] + async fn test_ensure_can_create_denied_without_reason() { + let (client, handle) = mock_client_pair(); + + let srv = tokio::spawn(async move { + let mut h = pin!(handle); + let (_req, send) = h.next_request().await.expect("expected SSAR POST"); + send.send_response( + Response::builder() + .status(201) + .header("content-type", "application/json") + .body(Body::from(ssar_response_body(false, ""))) + .unwrap(), + ); + }); + + let err = super::super::ensure_can_create( + &client, + "default", + "bootstrap.cluster.x-k8s.io/v1beta1", + "K0sWorkerConfig", + "bootstrap", + ) + .await + .expect_err("allowed=false should return an error"); + + match err { + ReconcilerError::PermissionDenied(msg) => { + assert!( + msg.contains("no reason provided"), + "empty reason should fall back to a placeholder, got: {msg}" + ); + } + other => panic!("expected PermissionDenied, got: {other:?}"), + } + + srv.await.unwrap(); + } + + // ---- Error path: SSAR API call itself fails (5xx) ---- + + #[tokio::test] + async fn test_ensure_can_create_api_error_propagates_kube_error() { + let (client, handle) = mock_client_pair(); + + let srv = tokio::spawn(async move { + let mut h = pin!(handle); + let (_req, send) = h.next_request().await.expect("expected SSAR POST"); + send.send_response( + Response::builder() + .status(500) + .header("content-type", "application/json") + .body(Body::from(k8s_error_body(500, "internal server error"))) + .unwrap(), + ); + }); + + let err = super::super::ensure_can_create( + &client, + "default", + "bootstrap.cluster.x-k8s.io/v1beta1", + "K0sWorkerConfig", + "bootstrap", + ) + .await + .expect_err("a 5xx from the API should surface as an error"); + + assert!( + matches!(err, ReconcilerError::KubeError(_)), + "API failure should map to KubeError, got: {err:?}" + ); + + srv.await.unwrap(); + } + + // ======================================================================== + // validate_embedded_metadata — unit tests + // ======================================================================== + + use crate::crd::EmbeddedResource; + + fn embedded(value: serde_json::Value) -> EmbeddedResource { + EmbeddedResource(value) + } + + #[test] + fn test_validate_embedded_metadata_rejects_namespace() { + let e = embedded(serde_json::json!({ + "apiVersion": "bootstrap.cluster.x-k8s.io/v1beta1", + "kind": "K0sWorkerConfig", + "metadata": { "namespace": "kube-system" }, + "spec": {} + })); + let err = super::super::validate_embedded_metadata(&e, "spec.bootstrapSpec") + .expect_err("metadata.namespace must be rejected"); + match err { + ReconcilerError::ValidationError(msg) => { + assert!( + msg.contains("metadata.namespace") && msg.contains("spec.bootstrapSpec"), + "error must name the offending field, got: {msg}" + ); + } + other => panic!("expected ValidationError, got: {other:?}"), + } + } + + #[test] + fn test_validate_embedded_metadata_rejects_name() { + let e = embedded(serde_json::json!({ + "kind": "RemoteMachine", + "metadata": { "name": "attacker-chosen" }, + "spec": {} + })); + let err = super::super::validate_embedded_metadata(&e, "spec.infrastructureSpec") + .expect_err("metadata.name must be rejected"); + assert!(matches!(err, ReconcilerError::ValidationError(m) if m.contains("metadata.name"))); + } + + #[test] + fn test_validate_embedded_metadata_rejects_reserved_label_prefix() { + // A user trying to forge the cluster-name label (threat T2) via the + // embedded resource's labels must be rejected. + let e = embedded(serde_json::json!({ + "kind": "K0sWorkerConfig", + "metadata": { "labels": { "cluster.x-k8s.io/cluster-name": "other-cluster" } }, + "spec": {} + })); + let err = super::super::validate_embedded_metadata(&e, "spec.bootstrapSpec") + .expect_err("reserved-prefix label must be rejected"); + assert!( + matches!(err, ReconcilerError::ValidationError(m) if m.contains("reserved prefix")), + "must reject reserved label prefix" + ); + } + + #[test] + fn test_validate_embedded_metadata_rejects_reserved_annotation_prefix() { + let e = embedded(serde_json::json!({ + "kind": "K0sWorkerConfig", + "metadata": { "annotations": { "5spot.finos.org/foo": "bar" } }, + "spec": {} + })); + assert!(super::super::validate_embedded_metadata(&e, "spec.bootstrapSpec").is_err()); + } + + #[test] + fn test_validate_embedded_metadata_accepts_clean_labels_and_annotations() { + let e = embedded(serde_json::json!({ + "kind": "K0sWorkerConfig", + "metadata": { + "labels": { "team": "payments" }, + "annotations": { "example.com/owner": "alice" } + }, + "spec": {} + })); + super::super::validate_embedded_metadata(&e, "spec.bootstrapSpec") + .expect("clean labels/annotations must be accepted"); + } + + #[test] + fn test_validate_embedded_metadata_accepts_no_metadata() { + let e = embedded(serde_json::json!({ "kind": "K0sWorkerConfig", "spec": {} })); + super::super::validate_embedded_metadata(&e, "spec.bootstrapSpec") + .expect("absent metadata must be accepted"); + } + + // ======================================================================== + // merge_owned_labels — unit tests + // ======================================================================== + + #[test] + fn test_merge_owned_labels_stamps_controller_labels() { + let mut user = BTreeMap::new(); + user.insert("team".to_string(), "payments".to_string()); + let merged = super::super::merge_owned_labels(user, "my-sm", "my-cluster"); + assert_eq!(merged.get("team").map(String::as_str), Some("payments")); + assert_eq!( + merged + .get("5spot.finos.org/scheduled-machine") + .map(String::as_str), + Some("my-sm") + ); + assert_eq!( + merged + .get("cluster.x-k8s.io/cluster-name") + .map(String::as_str), + Some("my-cluster") + ); + } + + #[test] + fn test_merge_owned_labels_controller_labels_win() { + // Even if a user value somehow reaches here for a reserved key, the + // controller-owned value must take precedence (inserted last). + let mut user = BTreeMap::new(); + user.insert( + "cluster.x-k8s.io/cluster-name".to_string(), + "attacker".to_string(), + ); + let merged = super::super::merge_owned_labels(user, "my-sm", "real-cluster"); + assert_eq!( + merged + .get("cluster.x-k8s.io/cluster-name") + .map(String::as_str), + Some("real-cluster"), + "controller-owned label must win over any user value" + ); + } } diff --git a/src/reconcilers/mod.rs b/src/reconcilers/mod.rs index 24a239e..6ad3fbd 100644 --- a/src/reconcilers/mod.rs +++ b/src/reconcilers/mod.rs @@ -14,10 +14,16 @@ //! The most commonly used symbols are re-exported at this level so callers only //! need `use crate::reconcilers::{…}`. +pub mod child_client; +pub mod child_watch; mod helpers; pub mod scheduled_machine; // Re-export main types and functions +pub use child_client::{ + CacheKey, ChildClientCache, ChildWatchHook, ResolvedClient, DEFAULT_KUBECONFIG_SECRET_KEY, +}; +pub use child_watch::ChildNodeWatchManager; #[allow(deprecated)] // re-export of legacy node_to_scheduled_machines for one release pub use helpers::{ build_clear_reclaim_patch, error_policy, evaluate_schedule, machine_to_scheduled_machine, diff --git a/src/reconcilers/scheduled_machine.rs b/src/reconcilers/scheduled_machine.rs index 6a17c4a..d66a005 100644 --- a/src/reconcilers/scheduled_machine.rs +++ b/src/reconcilers/scheduled_machine.rs @@ -121,6 +121,15 @@ pub struct Context { /// keeping the finalizer in place. Use only when an external sweep is in /// place to garbage-collect stuck SMs. pub force_finalizer_on_timeout: bool, + /// Cache of per-`(namespace, secret_name)` child-cluster `kube::Client`s + /// built from kubeconfig Secrets referenced by `ScheduledMachine`s. + /// + /// See [`crate::reconcilers::child_client`] for resolution order, + /// cache invalidation semantics (Secret `resourceVersion`-keyed), and + /// the bounded-LRU eviction policy. Cheaply clonable — the underlying + /// state is `Arc`-wrapped — so the cache is shared across every reconcile + /// (including future per-child Node watchers in Phase 1.9). + pub child_clients: Arc, } impl Context { @@ -149,6 +158,9 @@ impl Context { // Default-true: prefer unblocking namespace deletion over // strict-cleanup. See field rustdoc for the trade-off. force_finalizer_on_timeout: true, + // Empty cache — populated lazily on the first reconcile that + // resolves a kubeconfigSecretRef or an auto-discovered Secret. + child_clients: Arc::new(crate::reconcilers::child_client::ChildClientCache::new()), } } @@ -206,10 +218,65 @@ pub enum ReconcilerError { #[error("Security validation failed: {0}")] ValidationError(String), + /// The controller's service account lacks RBAC permission to `create` a + /// required embedded resource (bootstrap, infrastructure, or the CAPI + /// `Machine`). + /// + /// Surfaced by a pre-flight [`SelfSubjectAccessReview`] in + /// [`crate::reconcilers::helpers::ensure_can_create`] so the failure is a + /// clear, actionable error naming the denied resource type rather than an + /// opaque 403 raised partway through resource creation. Pairs with the + /// admission-time `authorizer` checks in the ValidatingAdmissionPolicy, + /// which enforce the same permission on the *creating user* to prevent + /// privilege escalation through the controller. + /// + /// [`SelfSubjectAccessReview`]: k8s_openapi::api::authorization::v1::SelfSubjectAccessReview + #[error("Permission denied: {0}")] + PermissionDenied(String), + /// An async operation exceeded its configured deadline (e.g. finalizer cleanup). #[error("Operation timed out: {0}")] TimeoutError(String), + /// A kubeconfig Secret was found but lacks the expected data key. + /// + /// Returned when [`crate::reconcilers::child_client::ChildClientCache`] + /// resolves an explicit `spec.kubeconfigSecretRef` (or an auto-discovered + /// `-kubeconfig` Secret) and the requested `data[key]` entry + /// is absent. Auto-discovery may surface this when the Secret exists but + /// stores the kubeconfig under a non-default key. + #[error("Kubeconfig Secret {namespace}/{name} is missing data key {key}")] + KubeconfigSecretMissingKey { + namespace: String, + name: String, + key: String, + }, + + /// A kubeconfig Secret was found but its contents cannot be parsed as a + /// valid kubeconfig (bad YAML, missing clusters/users/contexts, etc.) or a + /// `kube::Client` cannot be built from it. + #[error("Kubeconfig in Secret {namespace}/{name} is invalid: {reason}")] + KubeconfigInvalid { + namespace: String, + name: String, + reason: String, + }, + + /// The child cluster could not be reached via the resolved kubeconfig. + /// Distinct from [`Self::KubeError`] so it gets its own Prometheus error + /// label and a dedicated `ChildClusterReachable=False` condition (Phase 2). + /// + /// Includes the explicit-ref 404 case: when + /// `spec.kubeconfigSecretRef` is set and the named Secret does not exist, + /// the controller fails closed rather than silently falling back to the + /// management client (would route Node operations to the wrong cluster). + #[error("Child cluster {namespace}/{name} unreachable: {reason}")] + ChildClusterUnreachable { + namespace: String, + name: String, + reason: String, + }, + /// Catch-all for unexpected errors from third-party libraries. #[error(transparent)] Other(#[from] anyhow::Error), @@ -410,7 +477,17 @@ fn record_reconciliation_result( record_error("reference_validation"); } ReconcilerError::ValidationError(_) => record_error("validation"), + ReconcilerError::PermissionDenied(_) => record_error("permission_denied"), ReconcilerError::TimeoutError(_) => record_error("timeout"), + ReconcilerError::KubeconfigSecretMissingKey { .. } => { + record_error("kubeconfig_secret_missing_key"); + } + ReconcilerError::KubeconfigInvalid { .. } => { + record_error("kubeconfig_invalid"); + } + ReconcilerError::ChildClusterUnreachable { .. } => { + record_error("child_cluster_unreachable"); + } ReconcilerError::Other(_) => record_error("other"), } } @@ -824,8 +901,28 @@ async fn provision_reclaim_agent_best_effort( return; } let commands = resource.spec.kill_if_commands.as_deref().unwrap_or(&[]); + // Resolve the child-cluster client for the Node + ConfigMap operations. + // The DaemonSet that consumes the ConfigMap runs on workload-cluster + // Nodes, so both the per-node label patch and the ConfigMap apply must + // hit the workload kube-apiserver — not the management one. Best-effort: + // a resolution failure logs and skips reclaim-agent projection rather + // than falling back to management (which would project the ConfigMap + // into a cluster where no consumer DaemonSet exists). + let resolved = match ctx.child_clients.resolve(&ctx.client, resource).await { + Ok(c) => c, + Err(e) => { + warn!( + resource = %resource.name_any(), + node = %node_name, + error = %e, + "Failed to resolve child-cluster client for reclaim-agent projection (non-fatal)" + ); + return; + } + }; if let Err(e) = - super::helpers::reconcile_reclaim_agent_provision(&ctx.client, node_name, commands).await + super::helpers::reconcile_reclaim_agent_provision(resolved.client(), node_name, commands) + .await { warn!( resource = %resource.name_any(), @@ -893,7 +990,22 @@ async fn reconcile_node_taints_best_effort( desired, previously_applied, }; - let outcome = match super::helpers::reconcile_node_taints(&ctx.client, input).await { + // Taints land on the child cluster's Node objects (they live there, not on + // the management cluster). Resolve once at the top; best-effort, so a + // resolver failure logs + skips rather than escalating to Error. + let resolved = match ctx.child_clients.resolve(&ctx.client, resource).await { + Ok(c) => c, + Err(e) => { + warn!( + resource = %resource.name_any(), + node = %node_name, + error = %e, + "Failed to resolve child-cluster client for node-taint reconcile (non-fatal)" + ); + return; + } + }; + let outcome = match super::helpers::reconcile_node_taints(resolved.client(), input).await { Ok(o) => o, Err(e) => { warn!( @@ -991,7 +1103,15 @@ async fn handle_shutting_down_phase( if grace_period_elapsed { info!(resource = %name, namespace = %namespace, "Grace period elapsed - draining node and removing machine"); - // Step 1: Drain the node if it exists + // Step 1: Drain the node if it exists. + // + // `get_node_from_machine` reads the CAPI Machine on the management + // cluster (where Machine objects live) to extract the nodeRef, so it + // stays on the management client. The drain itself, however, hits + // the Node + Pod APIs on the workload cluster — resolve the child + // client and pass it down. Resolution failure here is fail-closed: + // a misconfigured kubeconfigSecretRef must NOT silently drain Pods + // on the management cluster. if let Some(node_name) = get_node_from_machine(&ctx.client, &namespace, &name).await? { info!( resource = %name, @@ -1003,8 +1123,10 @@ async fn handle_shutting_down_phase( // Parse drain timeout let drain_timeout = parse_duration(&resource.spec.node_drain_timeout)?; + let drain_client = ctx.child_clients.resolve(&ctx.client, &resource).await?; + // Attempt to drain the node - match drain_node_with_timeout(&ctx.client, &node_name, drain_timeout).await { + match drain_node_with_timeout(drain_client.client(), &node_name, drain_timeout).await { Ok(()) => { info!( resource = %name, diff --git a/src/reconcilers/scheduled_machine_tests.rs b/src/reconcilers/scheduled_machine_tests.rs index b6a6f9f..3d203c3 100644 --- a/src/reconcilers/scheduled_machine_tests.rs +++ b/src/reconcilers/scheduled_machine_tests.rs @@ -39,6 +39,7 @@ mod tests { kill_switch: false, node_taints: vec![], kill_if_commands: None, + kubeconfig_secret_ref: None, } } diff --git a/tests/integration_child_kubeconfig.rs b/tests/integration_child_kubeconfig.rs new file mode 100644 index 0000000..814065f --- /dev/null +++ b/tests/integration_child_kubeconfig.rs @@ -0,0 +1,416 @@ +// Copyright (c) 2025 Erick Bourgeois, RBC Capital Markets +// SPDX-License-Identifier: Apache-2.0 +//! # Integration test: child-cluster kubeconfig wiring +//! +//! End-to-end exercise of the Phase 1 multi-cluster work — the resolver, +//! the cache, the watcher manager, and the `Context` plumbing — through +//! their public interfaces. Validates that the four moving parts (CRD +//! field → cache resolve → hook fired → manager spawns task) compose +//! correctly without requiring a live Kubernetes cluster. +//! +//! Why hermetic, not a real `kind` cluster: the per-cluster connection +//! semantics are already exercised by the existing real-cluster +//! integration tests (`integration_node_taints.rs`, +//! `integration_emergency_reclaim.rs`); what this test adds is the +//! *integration of the new modules with each other and with Context*, +//! which a unit test inside the modules can't validate. `tower_test` +//! gives us deterministic control over Secret GET ordering + RV bumps +//! without paying the kind-cluster startup cost. +//! +//! A complementary real-cluster integration test (full Active → +//! ShuttingDown cycle with two real clusters) is left as future work +//! once the dev environment standardises on a two-cluster harness. + +use std::pin::pin; +use std::sync::Arc; + +use http::{Request, Response}; +use kube::api::ObjectMeta; +use kube::client::Body; +use kube::runtime::reflector; +use kube::runtime::reflector::ObjectRef; +use kube::{api::ApiResource, api::GroupVersionKind, core::DynamicObject}; +use tokio::sync::mpsc; +use tower_test::mock; + +use five_spot::constants::CHILD_NODE_EVENT_CHANNEL_CAP; +use five_spot::crd::{ + EmbeddedResource, KubeconfigSecretRef, ScheduleSpec, ScheduledMachine, ScheduledMachineSpec, +}; +use five_spot::reconcilers::{ + CacheKey, ChildClientCache, ChildNodeWatchManager, Context, ResolvedClient, +}; + +// Minimal valid kubeconfig. `insecure-skip-tls-verify` keeps the parse +// hermetic — no CA bundle or DNS lookup. +const FIXTURE_KUBECONFIG_YAML: &str = r#" +apiVersion: v1 +kind: Config +clusters: +- name: child + cluster: + server: https://child.example.test + insecure-skip-tls-verify: true +contexts: +- name: child + context: + cluster: child + user: child +users: +- name: child + user: + token: dummy-token +current-context: child +"#; + +fn mock_pair() -> (kube::Client, mock::Handle, Response>) { + let (svc, handle) = mock::pair::, Response>(); + (kube::Client::new(svc, "default"), handle) +} + +fn empty_machine_store() -> reflector::Store { + let ar = ApiResource::from_gvk_with_plural( + &GroupVersionKind::gvk("cluster.x-k8s.io", "v1beta1", "Machine"), + "machines", + ); + let writer: reflector::store::Writer = reflector::store::Writer::new(ar); + writer.as_reader() +} + +fn make_sm( + namespace: &str, + name: &str, + cluster_name: &str, + ref_: Option, +) -> Arc { + Arc::new(ScheduledMachine { + metadata: ObjectMeta { + name: Some(name.to_string()), + namespace: Some(namespace.to_string()), + ..Default::default() + }, + spec: ScheduledMachineSpec { + schedule: ScheduleSpec { + days_of_week: vec!["mon-fri".to_string()], + hours_of_day: vec!["9-17".to_string()], + timezone: "UTC".to_string(), + enabled: true, + }, + cluster_name: cluster_name.to_string(), + bootstrap_spec: EmbeddedResource(serde_json::json!({ + "apiVersion": "bootstrap.cluster.x-k8s.io/v1beta1", + "kind": "K0sWorkerConfig", + "spec": {} + })), + infrastructure_spec: EmbeddedResource(serde_json::json!({ + "apiVersion": "infrastructure.cluster.x-k8s.io/v1beta1", + "kind": "RemoteMachine", + "spec": {} + })), + machine_template: None, + priority: 50, + graceful_shutdown_timeout: "5m".to_string(), + node_drain_timeout: "5m".to_string(), + kill_switch: false, + node_taints: vec![], + kill_if_commands: None, + kubeconfig_secret_ref: ref_, + }, + status: None, + }) +} + +fn secret_response( + namespace: &str, + name: &str, + key: &str, + resource_version: &str, + yaml: &str, +) -> Response { + use base64::Engine; + let b64 = base64::engine::general_purpose::STANDARD.encode(yaml.as_bytes()); + let body = serde_json::to_vec(&serde_json::json!({ + "apiVersion": "v1", + "kind": "Secret", + "metadata": { + "name": name, + "namespace": namespace, + "resourceVersion": resource_version + }, + "type": "Opaque", + "data": { key: b64 } + })) + .unwrap(); + Response::builder() + .status(200) + .header("Content-Type", "application/json") + .body(Body::from(body)) + .unwrap() +} + +fn not_found_response(name: &str) -> Response { + let body = serde_json::to_vec(&serde_json::json!({ + "kind": "Status", + "apiVersion": "v1", + "status": "Failure", + "message": format!("secrets \"{name}\" not found"), + "reason": "NotFound", + "code": 404 + })) + .unwrap(); + Response::builder() + .status(404) + .header("Content-Type", "application/json") + .body(Body::from(body)) + .unwrap() +} + +/// Wire a full Context with cache + watch manager, exactly as main.rs +/// does, and exercise the integration through the `resolve()` entry +/// point. Asserts that: +/// - The cache GETs the right Secret +/// - A child client is built and returned +/// - The watcher manager spawns exactly one task for the resulting key +/// - The mpsc receiver is wired (we don't drain it; we only verify the +/// channel pair exists and the manager holds the sender end) +#[tokio::test] +async fn explicit_ref_resolves_and_starts_watcher() { + let (mgmt, handle) = mock_pair(); + + // Build Context exactly like main.rs: with the same Arc-cache and + // watch manager wiring. + let context = Arc::new(Context::new(mgmt.clone(), 0, 1)); + let (tx, _rx) = mpsc::channel::>(CHILD_NODE_EVENT_CHANNEL_CAP); + let manager = ChildNodeWatchManager::new(tx, empty_machine_store()); + let manager_for_assert = manager.clone(); + context.child_clients.set_hook(Arc::new(manager)); + + let sm = make_sm( + "team-a", + "gpu-worker", + "alpha", + Some(KubeconfigSecretRef { + name: "custom-kubeconfig".to_string(), + key: "value".to_string(), + }), + ); + + let server = tokio::spawn(async move { + let mut h = pin!(handle); + let (req, send) = h.next_request().await.expect("expected Secret GET"); + assert!( + req.uri() + .path() + .ends_with("/api/v1/namespaces/team-a/secrets/custom-kubeconfig"), + "expected GET on the explicitly referenced Secret, got {}", + req.uri() + ); + send.send_response(secret_response( + "team-a", + "custom-kubeconfig", + "value", + "1", + FIXTURE_KUBECONFIG_YAML, + )); + }); + + let resolved = context + .child_clients + .resolve(&context.client, &sm) + .await + .expect("resolve must succeed for a valid explicit ref"); + server.await.unwrap(); + + assert!( + resolved.is_child(), + "explicit ref must yield a Child client, got {resolved:?}" + ); + let key = CacheKey::new("team-a", "custom-kubeconfig"); + assert!( + manager_for_assert.has_watcher(&key), + "watch manager must have spawned a watcher for the resolved CacheKey" + ); + assert_eq!(manager_for_assert.task_count(), 1); +} + +#[tokio::test] +async fn auto_discovery_falls_back_to_management_with_no_watcher() { + let (mgmt, handle) = mock_pair(); + let context = Arc::new(Context::new(mgmt.clone(), 0, 1)); + let (tx, _rx) = mpsc::channel::>(CHILD_NODE_EVENT_CHANNEL_CAP); + let manager = ChildNodeWatchManager::new(tx, empty_machine_store()); + let manager_for_assert = manager.clone(); + context.child_clients.set_hook(Arc::new(manager)); + + let sm = make_sm("team-a", "gpu-worker", "alpha", None); + + let server = tokio::spawn(async move { + let mut h = pin!(handle); + let (req, send) = h.next_request().await.unwrap(); + // Auto-discovery: alpha → alpha-kubeconfig + assert!(req + .uri() + .path() + .ends_with("/api/v1/namespaces/team-a/secrets/alpha-kubeconfig")); + send.send_response(not_found_response("alpha-kubeconfig")); + }); + + let resolved = context + .child_clients + .resolve(&context.client, &sm) + .await + .expect("auto-discovery 404 must fall through to management"); + server.await.unwrap(); + + assert!( + !resolved.is_child(), + "no Secret → Management variant, got {resolved:?}" + ); + assert_eq!( + manager_for_assert.task_count(), + 0, + "management fallback must NOT spawn a watcher" + ); +} + +#[tokio::test] +async fn rv_rotation_restarts_watcher_cleanly() { + // Token rotation: same Secret, new resourceVersion. The watcher + // running with stale credentials must be cancelled and a fresh one + // started before the resolve returns. + let (mgmt, handle) = mock_pair(); + let context = Arc::new(Context::new(mgmt.clone(), 0, 1)); + let (tx, _rx) = mpsc::channel::>(CHILD_NODE_EVENT_CHANNEL_CAP); + let manager = ChildNodeWatchManager::new(tx, empty_machine_store()); + let manager_for_assert = manager.clone(); + context.child_clients.set_hook(Arc::new(manager)); + + let sm = make_sm("team-a", "gpu-worker", "alpha", None); + + let server = tokio::spawn(async move { + let mut h = pin!(handle); + // First resolve: RV "1" + let (_, send) = h.next_request().await.unwrap(); + send.send_response(secret_response( + "team-a", + "alpha-kubeconfig", + "value", + "1", + FIXTURE_KUBECONFIG_YAML, + )); + // Second resolve: RV "2" (rotation) + let (_, send) = h.next_request().await.unwrap(); + send.send_response(secret_response( + "team-a", + "alpha-kubeconfig", + "value", + "2", + FIXTURE_KUBECONFIG_YAML, + )); + }); + + let _ = context + .child_clients + .resolve(&context.client, &sm) + .await + .unwrap(); + assert_eq!(manager_for_assert.task_count(), 1); + + let _ = context + .child_clients + .resolve(&context.client, &sm) + .await + .unwrap(); + server.await.unwrap(); + // Still exactly one watcher — the old one was aborted, a new one started. + assert_eq!( + manager_for_assert.task_count(), + 1, + "RV rotation must keep exactly one running watcher per CacheKey" + ); +} + +#[tokio::test] +async fn cache_evict_cancels_watcher() { + let (mgmt, handle) = mock_pair(); + let cache = ChildClientCache::new(); + let (tx, _rx) = mpsc::channel::>(CHILD_NODE_EVENT_CHANNEL_CAP); + let manager = ChildNodeWatchManager::new(tx, empty_machine_store()); + let manager_for_assert = manager.clone(); + cache.set_hook(Arc::new(manager)); + + let sm = make_sm( + "team-a", + "gpu-worker", + "alpha", + Some(KubeconfigSecretRef { + name: "kc".to_string(), + key: "value".to_string(), + }), + ); + + let server = tokio::spawn(async move { + let mut h = pin!(handle); + let (_, send) = h.next_request().await.unwrap(); + send.send_response(secret_response( + "team-a", + "kc", + "value", + "1", + FIXTURE_KUBECONFIG_YAML, + )); + }); + + let _ = cache.resolve(&mgmt, &sm).await.unwrap(); + server.await.unwrap(); + assert_eq!(manager_for_assert.task_count(), 1); + + cache.evict(&CacheKey::new("team-a", "kc")); + assert_eq!( + manager_for_assert.task_count(), + 0, + "explicit cache evict must propagate to the watch manager" + ); +} + +#[tokio::test] +async fn explicit_ref_404_fails_closed_and_does_not_spawn_watcher() { + // Fail-closed contract: misconfigured kubeconfigSecretRef must NOT + // silently route Node operations to the management cluster. The + // resolver returns an error AND no watcher gets spawned for the + // non-existent Secret. + let (mgmt, handle) = mock_pair(); + let cache = ChildClientCache::new(); + let (tx, _rx) = mpsc::channel::>(CHILD_NODE_EVENT_CHANNEL_CAP); + let manager = ChildNodeWatchManager::new(tx, empty_machine_store()); + let manager_for_assert = manager.clone(); + cache.set_hook(Arc::new(manager)); + + let sm = make_sm( + "team-a", + "gpu-worker", + "alpha", + Some(KubeconfigSecretRef { + name: "missing".to_string(), + key: "value".to_string(), + }), + ); + + let server = tokio::spawn(async move { + let mut h = pin!(handle); + let (_, send) = h.next_request().await.unwrap(); + send.send_response(not_found_response("missing")); + }); + + let result = cache.resolve(&mgmt, &sm).await; + server.await.unwrap(); + + assert!( + result.is_err(), + "explicit ref + 404 must error, got {:?}", + result + .ok() + .map(|r| matches!(r, ResolvedClient::Management(_))) + ); + assert_eq!(manager_for_assert.task_count(), 0); +}