# Deep Review: 20260414-144550-pr-302

| | |
|---|---|
| **Date** | 2026-04-14 14:45 |
| **Repo** | [rancher-sandbox/rancher-desktop-daemon](https://github.com/rancher-sandbox/rancher-desktop-daemon) |
| **Round** | 1 |
| **Author** | [@Nino-K](https://github.com/Nino-K) |
| **PR** | [#302](https://github.com/rancher-sandbox/rancher-desktop-daemon/pull/302) — Validating admission controller app settings |
| **Branch** | `validating-admission-controller-app-settings` |
| **Commits** | `af58638` Add validating admission webhook for App settings<br>`a9f48a7` Add bats tests to verify the validating admission controller |
| **Reviewers** | Claude Opus 4.6, Codex GPT 5.4, Gemini 3.1 Pro |
| **Verdict** | **Merge with fixes** — remove redundant containerEngine check and cache the parsed k3s-versions map; other items are nice-to-have. |
| **Wall-clock time** | `21 min 56 s` |

## Executive Summary

This PR introduces a validating admission webhook for the singleton `App`
resource and wires it into the shared webhook framework already used by
LimaVM and Notary. Validation covers three invariants: `containerEngine.name`
must be `moby` or `containerd`, `kubernetes.version` must appear in the
bundled `k3s-versions.json`, and a non-empty version is required when
`kubernetes.enabled` is true. BATS coverage is added via `rdd ctl apply
--dry-run=server`, and the existing `set.bats` is adjusted to clear
`kubernetes.enabled` and `kubernetes.version` atomically so it does not
trip the new rule.

The implementation is clean and pattern-consistent, but two items deserve
attention before merge. First, the `containerEngine.name` check in the
webhook is unreachable — the CRD schema already enforces the same enum
and the API server validates it before the webhook runs. Second, the
webhook re-parses `k3s-versions.json` on every admission request; parsing
it once at controller setup would both eliminate redundant work and
surface a malformed fixture at startup instead of deferring the error to
the first user request. The remaining findings are test-coverage gaps:
no assertion for the `UPDATE` admission path, a hard-coded k3s version
that will age out of the bundled fixture, and missing unit tests for
`parseK3sVersions`'s normalization behavior.

**Structure**: 0 critical, 0 important, 5 suggestions, plus 1 future
design observation and 1 documentation fix.

---

## Critical Issues

None.

---

## Important Issues

None.

---

## Suggestions

S1. **`containerEngine.name` check in webhook is unreachable** — `pkg/controllers/app/app/controllers/app_webhook.go:38-41` [Claude Opus 4.6]

```go
engine := app.Spec.ContainerEngine.Name
if engine != "moby" && engine != "containerd" {
    return nil, fmt.Errorf("spec.containerEngine.name %q is invalid; must be \"moby\" or \"containerd\"", engine)
}
```

The CRD schema at `pkg/controllers/app/app/crd.yaml:60-62` already pins
`containerEngine.name` to `enum: [moby, containerd]` with a default of
`moby`. The Kubernetes admission pipeline validates the CRD schema
before dispatching to validating webhooks, so any value outside the
enum is rejected by the API server before `AppValidator.validate` ever
sees the object. The webhook branch is dead code, and it introduces a
maintenance hazard: if a third engine is added to the CRD enum, the
webhook would silently reject it until someone remembers to update both
places.

The BATS case `"dry-run rejects invalid containerEngine name"`
(`bats/tests/32-app-controllers/app.bats:402`) keeps passing regardless
because its assertion is `assert_output --partial "containerEngine.name"`,
which matches the CRD error (`Unsupported value: "kvm": supported
values: "moby", "containerd"`) just as well as the webhook error.

Fix: drop the engine check from `validate` entirely and rely on the
CRD enum as the authoritative source.

S2. **`parseK3sVersions` re-parses embedded JSON on every admission request** — `pkg/controllers/app/app/controllers/app_webhook.go:48-56` [Claude Opus 4.6, Gemini 3.1 Pro]

```go
if k8s.Version != "" {
    supported, err := parseK3sVersions(v.K3sVersionsData)
    if err != nil {
        return nil, fmt.Errorf("failed to load supported Kubernetes versions: %w", err)
    }
    if _, ok := supported[k8s.Version]; !ok {
        return nil, fmt.Errorf("spec.kubernetes.version %q is not supported; see the bundled k3s-versions.json for valid versions", k8s.Version)
    }
}
```

`K3sVersionsData` is a compile-time `//go:embed` constant; it never
changes at runtime. Re-parsing it on every create/update is wasted work
and, more importantly, defers malformed-JSON detection to the first
user request that happens to set a version. Parse once in a constructor
so a bad fixture blocks controller startup instead of failing admission.

Fix: introduce `NewAppValidator(k3sVersionsData string) (*AppValidator,
error)` that calls `parseK3sVersions` once and stores the resulting
map. Replace `AppValidator{K3sVersionsData: k3sVersionsData}` in
`controller.go:123` with a `NewAppValidator(...)` call whose error
propagates out of `setupWebhook`. The per-request path then becomes a
single map lookup. Gemini flagged this as important; the runtime cost
is negligible for RDD's single-user workload, so it is recorded here
as a suggestion — but the fail-fast-at-startup argument still stands
on its own.

S3. **Hard-coded `K3S_VERSION="1.32.0"` will age out of the bundled fixture** — `bats/tests/32-app-controllers/app.bats:14, 451-465` [Codex GPT 5.4]

```bash
K3S_VERSION="1.32.0"

@test "dry-run accepts supported kubernetes.version" {
    run -0 rdd ctl apply --dry-run=server -f - <<EOF
...
    version: "${K3S_VERSION}"
EOF
}
```

The positive-path test pins the oldest version in the current bundled
`k3s-versions.json` (`v1.32.0+k3s1`). The webhook validates against
whatever versions are in the file, not against a stable constant, so a
future bundle refresh that drops `1.32.0` will break this test even
though the webhook itself is still correct. The failure mode converts
a routine k3s refresh into an unrelated test break.

Fix: derive the expected version from the fixture at test runtime
(e.g. `jq -r '.versions[0]' k3s-versions.json | sed -e 's/^v//' -e
's/+.*//'`), or pin to a field intended to stay current such as the
bundle's `channels.stable` entry.

S4. **Webhook's `UPDATE` operation has no BATS coverage** — `pkg/controllers/app/app/controller.go:119-122`, `bats/tests/32-app-controllers/app.bats:372-465` [Codex GPT 5.4]

```go
Operations: []admissionregistrationv1.OperationType{
    admissionregistrationv1.Create,
    admissionregistrationv1.Update,
},
```

The webhook is registered for both `Create` and `Update`, and
`ValidateUpdate` is separate plumbing from `ValidateCreate` in
`app_webhook.go:27-30`. Every new dry-run case in the BATS suite uses
a create-shaped `rdd ctl apply` against `name: app` with no pre-existing
object — the update path is never exercised. A regression that only
affects updates (e.g., someone bypasses `validate` on the update path
or a framework change silently stops routing updates to the webhook)
would still ship green.

Fix: add at least one `--dry-run=server` update test that creates a
valid `App`, then reapplies an invalid spec (e.g., an unsupported
`kubernetes.version`) and asserts the update is rejected while the
stored object is unchanged.

S5. **`parseK3sVersions` normalization has no unit tests** — `pkg/controllers/app/app/controllers/k3s_versions.go` [Claude Opus 4.6, Codex GPT 5.4]

The version-stripping logic (`TrimPrefix("v")` and `Index("+")` slice)
handles the current bundled format but is only exercised indirectly by
BATS tests that go through the full admission pipeline. Edge cases are
not covered: multiple versions that normalize to the same key (e.g.
`v1.32.0+k3s1` and `v1.32.0+k3s2`), an empty `versions` array, entries
without a `v` prefix, entries without a `+` suffix, and malformed JSON.
A future format tweak in the bundled JSON would only be caught
indirectly through a BATS failure.

Fix: add a short table-driven unit test in `k3s_versions_test.go`
covering the above cases. `parseK3sVersions` is a pure function — it
is the ideal shape for a unit test.

---

## Design Observations

**Concerns**

- *(future)* **Concurrent writers to `App.Status.Conditions` without SSA** — `pkg/controllers/app/app/controllers/app_controller.go` [Gemini 3.1 Pro]. The App reconciler mirrors LimaVM conditions into `App.Status.Conditions` via `r.Status().Update()`, while another controller (`EngineReconciler`) writes `ContainerEngineReady` to the same condition list. Two writers on the same subresource without Server-Side Apply risks lost updates and `resourceVersion` conflicts under load. Out of scope for this PR, which is only adding the admission webhook, but worth filing as a follow-up.

**Strengths**

- *(in-scope)* **Clean reuse of the shared webhook framework** — `pkg/controllers/app/app/controller.go:114-132` [Claude Opus 4.6, Codex GPT 5.4]. The setup path uses the existing `base.WebhookConfig` / `SetupWebhookForResource` abstraction, exactly matching the LimaVM and Notary webhooks. Certificate management, port allocation via `SharedControllerManager`, and the `base.WebhookController` interface are all already established; nothing App-specific was bolted on.
- *(in-scope)* **Validation lives at the admission boundary, not the reconciler** — [Codex GPT 5.4]. Rejecting invalid desired state at admission time prevents the reconciler from having to reason about partially-invalid specs or creating transient child resources for specs that should never be accepted. This is the right boundary for these invariants.
- *(in-scope)* **Version normalization bridges two representations** — `pkg/controllers/app/app/controllers/k3s_versions.go:21-36` [Claude Opus 4.6, Gemini 3.1 Pro]. Stripping the `v` prefix and `+k3s*` suffix from the bundled entries lets users specify versions in the natural `1.32.0` form in the App spec while still validating against the exact-format entries in the upstream fixture.

## Testing Assessment

BATS coverage correctly exercises the full admission pipeline (API
server → CRD schema → validating webhook) through
`rdd ctl apply --dry-run=server`, which is the right integration level
for this change. `set.bats` is adjusted sensibly to clear
`kubernetes.enabled` and `kubernetes.version` atomically so it doesn't
trip the new "empty version when enabled" rule.

Gaps ranked by risk:
1. **No `UPDATE` path coverage** (S4). The webhook declares both
   `Create` and `Update` operations, but every new case is a create.
2. **No unit tests for `parseK3sVersions`** (S5). Pure function, easy
   to cover, currently only exercised indirectly through BATS.
3. **Hard-coded version in positive-path test** (S3). Will age out of
   the bundled fixture over time.
4. **No test for `kubernetes.enabled=false` with an unsupported version**
   — the webhook still validates `kubernetes.version` when enabled is
   false (the `k8s.Version != ""` check at `app_webhook.go:48` does not
   gate on `Enabled`), which may be intentional but is not documented
   by a test.

## Documentation Assessment

`docs/design/api_app.md:90` states that `spec.kubernetes.version`
"Defaults to `1.30.2`", but the App type has no `+kubebuilder:default`
for that field and the CRD schema carries no default. The mismatch
predates this PR, but the new webhook now **rejects** `enabled=true`
with an empty version, which makes the stale docs user-visible:
someone who follows the design doc and omits `version` expecting the
`1.30.2` default will now get an admission error. Update the doc to
describe the actual behavior — that `kubernetes.version` has no default
and must be set explicitly when `kubernetes.enabled` is true. [Codex GPT 5.4]

The new code itself is adequately documented: exported methods on
`AppValidator` have docstrings, and `parseK3sVersions` has a clear
comment describing the normalization behavior.

## Commit Structure

Clean two-commit split: `af58638` adds the webhook implementation and
simultaneously adapts the existing `set.bats` test (which otherwise
would break), and `a9f48a7` adds the new BATS coverage. The `set.bats`
adaptation is correctly bundled with the webhook commit so that each
commit in isolation leaves the test suite green.

---

## Agent Performance Retro

### [Claude]

Claude was the only agent to notice that the `containerEngine.name` check
in the webhook is unreachable because CRD enum validation fires before
validating webhooks in the admission pipeline. That finding required
correlating the webhook code against `crd.yaml` and knowing the order of
the admission pipeline — Codex and Gemini both read the webhook code but
did not make that connection. Claude also co-raised the parse-once
optimization at the correct severity (suggestion) without inflating it.
Review content was concise and well-targeted.

### [Codex]

Codex contributed the two most practical test-coverage findings: the
missing `UPDATE` path and the hard-coded `1.32.0` that will age out of
the bundled fixture. Neither is a bug today, but both will bite later,
and Codex was the only agent to raise them. It was also the only agent
to catch the stale `docs/design/api_app.md` claim about a `1.30.2`
default, which the new webhook makes user-visible. Codex did not flag
the redundant containerEngine check even though its "testing assessment"
section correctly notes that create/update coverage is uneven — the
dead-code observation was one inference step away.

### [Gemini]

Gemini independently raised the same parse-once observation as Claude
but inflated the severity to *important*. The runtime cost is trivial
for a single-user desktop app; the real value of the fix is fail-fast
startup validation, which is a suggestion, not an important issue. The
finding was downgraded during consolidation. Gemini also surfaced a
pre-existing design concern about concurrent writers to `App.Status.Conditions`
without SSA — useful context for follow-up work even though it is out
of scope for this PR. Gemini missed both the dead-code containerEngine
check and the test-coverage gaps.

### Summary

| | Claude Opus 4.6 | Codex GPT 5.4 | Gemini 3.1 Pro |
|---|---|---|---|
| Duration | 7m 23s | 3m 29s | 2m 14s |
| Findings | 2S | 2S | 1S |
| Tool calls | 22 (Read 11, Grep 5, Bash 4) | 34 (shell 28, stdin 6) | 14 (read_file 8, grep_search 6) |
| Design observations | 3 | 2 | 3 |
| False positives | 0 | 0 | 0 |
| Unique insights | 1 | 3 | 1 |
| Files reviewed | 5 | 5 | 5 |
| Coverage misses | 0 | 0 | 2 |
| **Totals** | **2S** | **2S** | **1S** |
| Downgraded | 0 | 0 | 1 (I→S) |
| Dropped | 0 | 0 | 0 |


Claude delivered the highest-impact finding of this review (S1). Codex
delivered the most findings that will affect future maintenance (S3,
S4, documentation fix). Gemini's only novel contribution was a
forward-looking design note; its main finding overlapped with Claude's
and was miscalibrated high. For a small change like this one, Claude
and Codex together covered everything actionable.

**Reconciliation**: Gemini's I1 (JSON re-parsed on every request,
*important*) was downgraded to *suggestion* S2 during consolidation.
Rationale: the runtime cost is negligible for RDD's single-user
workload, and the fail-fast-at-startup argument — while valid — does
not meet the important-severity bar. Claude raised the same finding at
suggestion severity (its S2), which matches the consolidated view.

---

## Review Process Notes

### Repo context updates

- Add the following to `deep-review-context.md` under a new
  "Admission webhook coverage" bullet: *When reviewing webhook
  validation changes, cross-check every webhook rule against the
  matching CRD schema (`crd.yaml`). CRD schema validation runs before
  validating webhooks in the admission pipeline, so any check that
  duplicates a CRD `enum`, `pattern`, `maxLength`, or `required`
  constraint is dead code. Flag such checks as unreachable.* This
  principle is general enough to apply to every future CRD webhook
  review, not just this PR.

- Add the following to `deep-review-context.md`: *When a webhook is
  registered for both `Create` and `Update` operations, verify the
  BATS/integration suite exercises both paths. Controller-runtime
  dispatches these through separate `ValidateCreate` and
  `ValidateUpdate` methods, so a regression that only affects one
  path can ship green if only the other is tested.* This targets a
  recurring gap: dry-run create-shaped assertions are the easy thing
  to write and the update path is routinely forgotten.

---

## Appendix: Original Reviews

### [Claude] — Pass 1

I'll review this PR systematically — reading all changed files, tracing into the base webhook framework, and checking the k3s-versions.json format.

Now let me trace into the base webhook framework, check the k3s-versions.json, and look at the existing reconciler validation that's being replaced.

Let me check the App spec types, the k3s-versions.json format, and whether any validation was removed from the reconciler.

Let me check a few more things — the `rdd set` client-side validation, any unit tests, and git blame.

Now let me verify the relationship between CRD schema validation and webhook validation, and check for unit tests.

I now have enough context to write the review. All changed files read, caller/callee chains traced, CRD schema and webhook framework verified.

---

## PR #302 — Validating admission controller app settings

| Attribute | Value |
|-----------|-------|
| **Verdict** | Approve with suggestions |
| **Risk** | Low |
| **Reviewed files** | 5/5 |

### Executive Summary

This PR adds a validating admission webhook for App resources. It validates `containerEngine.name` against an allowed set, `kubernetes.version` against the bundled k3s-versions.json, and rejects an empty version when Kubernetes is enabled. The implementation follows established webhook patterns (matching the LimaVM and Mock controller webhooks). Two suggestions: the `containerEngine.name` check in the webhook is unreachable because the CRD schema `enum` rejects first, and `parseK3sVersions` re-parses JSON on every validation call instead of once at init time.

### Findings

#### Critical Issues

None.

#### Important Issues

None.

#### Suggestions

S1. **`containerEngine.name` validation in webhook is unreachable** — `pkg/controllers/app/app/controllers/app_webhook.go:39-41`

```go
engine := app.Spec.ContainerEngine.Name
if engine != "moby" && engine != "containerd" {
    return nil, fmt.Errorf("spec.containerEngine.name %q is invalid; must be \"moby\" or \"containerd\"", engine)
}
```

The CRD schema at `pkg/controllers/app/app/crd.yaml:60-62` has `enum: [moby, containerd]` on the `name` field. In the Kubernetes admission pipeline, CRD schema validation runs *before* validating webhooks. The API server rejects values not in the enum before the webhook is called, so this check never triggers. It also creates a maintenance hazard: if a third engine is added to the CRD enum but not the webhook, the webhook would silently become a blocking bottleneck.

Fix: Remove the `containerEngine.name` check from the webhook and rely on the CRD schema enum, which is the authoritative source. The CRD error message (`Unsupported value: "kvm": supported values: "moby", "containerd"`) is sufficiently clear.

```diff
 func (v *AppValidator) validate(app *v1alpha1.App) (ctrlwebhookadmission.Warnings, error) {
-	engine := app.Spec.ContainerEngine.Name
-	if engine != "moby" && engine != "containerd" {
-		return nil, fmt.Errorf("spec.containerEngine.name %q is invalid; must be \"moby\" or \"containerd\"", engine)
-	}
-
 	k8s := app.Spec.Kubernetes
```

The BATS test "dry-run rejects invalid containerEngine name" (`app.bats:402`) still passes because it asserts `--partial "containerEngine.name"`, which matches the CRD validation error message. (suggestion, regression)

S2. **`parseK3sVersions` re-parsed on every validation call** — `pkg/controllers/app/app/controllers/app_webhook.go:49`

```go
if k8s.Version != "" {
    supported, err := parseK3sVersions(v.K3sVersionsData)
```

`K3sVersionsData` is an embedded string that never changes at runtime. Every create/update request that sets a version re-parses the JSON and rebuilds the `map[string]struct{}`. In RDD's single-user context this is negligible, but the fix is trivial: parse once when `AppValidator` is constructed.

Fix: Parse the versions at init time and store the result in the struct.

```diff
 type AppValidator struct {
-	K3sVersionsData string
+	supportedVersions map[string]struct{}
+}
+
+func NewAppValidator(k3sVersionsData string) (*AppValidator, error) {
+	versions, err := parseK3sVersions(k3sVersionsData)
+	if err != nil {
+		return nil, err
+	}
+	return &AppValidator{supportedVersions: versions}, nil
 }
```

Then in `controller.go:123`, construct via `NewAppValidator(k3sVersionsData)`. This also surfaces a malformed k3s-versions.json at startup rather than deferring the error to the first validation request. (suggestion, regression)

### Design Observations

**Strengths**

- Clean use of the existing `base.WebhookConfig` / `SetupWebhookForResource` framework. The webhook setup in `controller.go:114-131` exactly follows the pattern established by the LimaVM controller, making the codebase consistent.
- Separating validation into `AppValidator` (testable independently) from the controller registration keeps concerns clean.
- The version stripping in `parseK3sVersions` (removing `v` prefix and `+k3s*` suffix) is straightforward and well-commented, matching the user-facing version format in the App spec.

### Testing Assessment

Good integration test coverage through BATS. The dry-run tests exercise the full admission path (API server + CRD schema + webhook) without persisting state, which is efficient. The `set.bats` fix correctly adapts to the new webhook constraint.

Untested scenarios, ranked by risk:

1. **`kubernetes.enabled=false` with an unsupported version** — The webhook rejects this (the `k8s.Version != ""` check at `app_webhook.go:48` is independent of `Enabled`), but no test verifies it. Whether this is desired behavior depends on product intent — should a user be able to store a version they plan to use later, even if k8s is currently disabled? If the current behavior is intentional, a test documenting it would prevent accidental regression.
2. **Unit tests for `parseK3sVersions`** — Edge cases like duplicate base versions (e.g., `v1.32.0+k3s1` and `v1.32.0+k3s2` mapping to the same `1.32.0`), empty versions list, and malformed JSON are best caught by unit tests. The BATS tests only exercise the happy path and one rejection case.

### Documentation Assessment

Exported types and methods have accurate docstrings. The `k3sVersionsJSON` type comment at `k3s_versions.go:13` is minimal ("mirrors the k3s-versions.json versions") but adequate for an internal struct. No user-facing documentation changes needed.

### Commit Structure

Clean two-commit structure: `af58638` adds the webhook implementation and adapts the existing `set.bats` test, `a9f48a7` adds comprehensive BATS tests. The `set.bats` change is correctly in the first commit because the webhook would break the existing test.

### Acknowledged Limitations

- **`set.bats:128`** — Comment `# Clear both together: webhook rejects an empty version when enabled=true.` acknowledges the behavioral change. The previous test (`rdd set kubernetes.version=`) would fail with the webhook because the App still has `kubernetes.enabled=true` from the prior test. Clearing both atomically is the correct adaptation.

### Coverage Summary

| File | Status |
|------|--------|
| `bats/tests/32-app-controllers/app.bats` | S1 (tangential: test assertions match CRD error, not webhook error) |
| `bats/tests/32-app-controllers/set.bats` | Reviewed, no issues |
| `pkg/controllers/app/app/controller.go` | S2 (tangential: constructs `AppValidator` without pre-parsing) |
| `pkg/controllers/app/app/controllers/app_webhook.go` | S1, S2 |
| `pkg/controllers/app/app/controllers/k3s_versions.go` | Reviewed, no issues |

### [Codex] — Pass 1

### Executive Summary

This PR moves App settings validation from reconcile time into a validating admission webhook and adds BATS coverage for the new rejection paths. The webhook wiring and live admission behavior looked correct in review; the only issues I found are in the new test coverage, which is missing the update path it registers and hard-codes a supported k3s version that will age out of the bundled fixture.

### Findings

#### Critical Issues

None.

#### Important Issues

None.

#### Suggestions

S1. **Use a version fixture derived from `k3s-versions.json` instead of pinning `1.32.0`** — `bats/tests/32-app-controllers/app.bats:14,451-463` (suggestion, enhancement)

```bash
K3S_VERSION="1.32.0"

@test "dry-run accepts supported kubernetes.version" {
    run -0 rdd ctl apply --dry-run=server -f - <<EOF
...
    version: "${K3S_VERSION}"
EOF
}
```

The new positive-path admission test at lines 451-463 depends on the hard-coded `K3S_VERSION="1.32.0"` from line 14, but the validator at `pkg/controllers/app/app/controllers/app_webhook.go:48-54` accepts whatever appears in the bundled `k3s-versions.json`, not a stable API constant. That means a future k3s bundle refresh can fail this test even when the webhook is still correct, simply because `1.32.0` has been pruned from the file. This turns a data refresh into an unrelated test break.

Fix: derive the expected “supported” version from the checked-in JSON fixture at test runtime, or pick a version from a field intended to remain current (for example the bundle’s `stable` channel) instead of pinning the oldest currently-listed version.

S2. **The new BATS suite never exercises the webhook’s `UPDATE` operation** — `pkg/controllers/app/app/controller.go:119-122`, `bats/tests/32-app-controllers/app.bats:372-465` (suggestion, enhancement)

```go
Operations: []admissionregistrationv1.OperationType{
    admissionregistrationv1.Create,
    admissionregistrationv1.Update,
},
```

```bash
@test "dry-run accepts valid App settings" {
    run -0 rdd ctl apply --dry-run=server -f - <<EOF
...
EOF
}
```

The webhook is explicitly registered for both create and update at lines 119-122, but every new dry-run case in `app.bats` is a create-shaped `apply` against `name: app` with no pre-existing object. That leaves the update path unverified even though `ValidateUpdate()` in `pkg/controllers/app/app/controllers/app_webhook.go:27-29` is separate plumbing and is the path `rdd set` relies on once the singleton already exists. A regression that only affects updates would still merge with this test suite green.

Fix: add at least one `--dry-run=server` update test that first creates a valid `App`, then reapplies an invalid spec and asserts the update is rejected while the stored object remains unchanged.

### Design Observations

**Strengths**

The webhook is added through the existing shared-controller webhook framework rather than inventing App-specific startup logic. That keeps certificate management, readiness, and configuration creation aligned with the patterns already used by Notary and LimaVM.

Moving validation from reconcile time to admission time is the right boundary for these invariants. It prevents the controller from having to reason about partially-invalid desired state and avoids creating transient child resources for specs that should never be accepted.

### Testing Assessment

1. The highest-risk gap is the missing `UPDATE` admission test for an existing `App`; the webhook advertises `UPDATE`, but the new suite only covers create-shaped requests.
2. There is still no direct non-dry-run `rdd ctl apply` rejection test for an invalid `containerEngine.name`; current non-dry-run coverage only checks the unsupported-version path through `rdd set`.
3. `parseK3sVersions()` has no unit tests for its normalization behavior (`v` stripping and `+k3s` suffix removal), so a future format tweak in the bundled JSON would only be caught indirectly.

### Documentation Assessment

`docs/design/api_app.md:90` still says `spec.kubernetes.version` “Defaults to `1.30.2`”, but the App type has no default annotation for that field and the new webhook now rejects `kubernetes.enabled=true` when the version is empty. This doc mismatch predates the PR, but the new admission rule makes it more user-visible; the design doc should be updated to describe the actual behavior.

### Coverage Summary

- `bats/tests/32-app-controllers/app.bats` — S1, S2
- `bats/tests/32-app-controllers/set.bats` — Reviewed, no issues
- `pkg/controllers/app/app/controller.go` — S2
- `pkg/controllers/app/app/controllers/app_webhook.go` — Reviewed, no issues
- `pkg/controllers/app/app/controllers/k3s_versions.go` — Reviewed, no issues


### [Gemini] — Pass 1

### Executive Summary

This PR introduces a validating admission webhook to enforce invariants on the singleton `App` resource, verifying `containerEngine.name` and checking `kubernetes.version` against the bundled `k3s-versions.json`. While the validation logic and the accompanying BATS tests are well-written and correct, the JSON parsing logic is re-executed on every single webhook request instead of being cached at startup, which introduces unnecessary overhead.

### Findings

#### Critical Issues
None.

#### Important Issues

I1. **Inefficient JSON parsing on every webhook request** — `pkg/controllers/app/app/controllers/app_webhook.go:49` (important, enhancement)

```go
	if k8s.Version != "" {
		supported, err := parseK3sVersions(v.K3sVersionsData)
		if err != nil {
			return nil, fmt.Errorf("failed to load supported Kubernetes versions: %w", err)
		}
```

The admission webhook parses the embedded `k3s-versions.json` string on every single validation request. Because the embedded string is static, parsing it repeatedly adds unnecessary runtime overhead. Furthermore, this delays reporting a malformed JSON file until a user actually attempts to apply an App resource. The JSON should be parsed exactly once during controller setup, caching the result so that malformed data prevents the controller from starting, and runtime validation becomes an O(1) map lookup.

Fix: Extract an initialization function `NewAppValidator` in `app_webhook.go`, and call it from `setupWebhook` in `controller.go`.

In `pkg/controllers/app/app/controllers/app_webhook.go`:
```diff
-type AppValidator struct {
-	K3sVersionsData string
-}
+type AppValidator struct {
+	SupportedVersions map[string]struct{}
+}
+
+func NewAppValidator(k3sVersionsData string) (*AppValidator, error) {
+	supported, err := parseK3sVersions(k3sVersionsData)
+	if err != nil {
+		return nil, fmt.Errorf("failed to load supported Kubernetes versions: %w", err)
+	}
+	return &AppValidator{SupportedVersions: supported}, nil
+}
 
 func (v *AppValidator) validate(app *v1alpha1.App) (ctrlwebhookadmission.Warnings, error) {
 // ...
 	if k8s.Version != "" {
-		supported, err := parseK3sVersions(v.K3sVersionsData)
-		if err != nil {
-			return nil, fmt.Errorf("failed to load supported Kubernetes versions: %w", err)
-		}
-		if _, ok := supported[k8s.Version]; !ok {
+		if _, ok := v.SupportedVersions[k8s.Version]; !ok {
 			return nil, fmt.Errorf("spec.kubernetes.version %q is not supported; see the bundled k3s-versions.json for valid versions", k8s.Version)
 		}
 	}
```

In `pkg/controllers/app/app/controller.go`:
```diff
 func (c *controller) setupWebhook(mgr ctrl.Manager) error {
+	validator, err := controllers.NewAppValidator(k3sVersionsData)
+	if err != nil {
+		return err
+	}
+
 	validatingConfig := base.WebhookConfig[*v1alpha1.App]{
 		Name:        appValidatorConfigName,
 		WebhookName: appValidatorWebhookName,
 		WebhookPort: c.webhookPort,
 		Operations: []admissionregistrationv1.OperationType{
 			admissionregistrationv1.Create,
 			admissionregistrationv1.Update,
 		},
-		Validator: &controllers.AppValidator{K3sVersionsData: k3sVersionsData},
+		Validator: validator,
 	}
```

#### Suggestions
None.

### Design Observations

**Concerns**
- **Concurrent `App` status updates** (pre-existing): While tracing the `App` controller lifecycle, I noticed that `AppReconciler.Reconcile` (`pkg/controllers/app/app/controllers/app_controller.go`) writes `App.Status.Conditions` using a non-SSA `r.Status().Update()` call. The system allows multiple concurrent writers (e.g., `EngineReconciler` writing `ContainerEngineReady`). This will eventually lead to `Conflict` errors, stalling status updates or causing race conditions. Consider replacing `Update()` with Server-Side Apply (SSA) via `r.Status().Patch(ctx, app, client.Apply, client.FieldOwner("app-controller"))`, or implementing a retry-on-conflict loop.

**Strengths**
- Testing the admission webhook with `--dry-run=server` directly in BATS correctly integrates with the real API server machinery and fully exercises the webhook execution path.
- The version stripping logic intelligently bridges the gap between how versions are represented internally (`v1.32.0+k3s1`) and how users are expected to configure them in the CRD (`1.32.0`).

### Testing Assessment
1. The BATS tests adequately cover the validating admission controller. They correctly simulate invalid fields and check for the proper rejections while also ensuring valid specifications safely navigate the webhook logic.

### Coverage Summary
- `bats/tests/32-app-controllers/app.bats` — Reviewed, no issues
- `bats/tests/32-app-controllers/set.bats` — Reviewed, no issues
- `pkg/controllers/app/app/controller.go` — I1
- `pkg/controllers/app/app/controllers/app_webhook.go` — I1
- `pkg/controllers/app/app/controllers/k3s_versions.go` — I1