Deep Review: 20260414-144550-pr-302

func (v *AppValidator) ValidateDelete(_ context.Context, _ *v1alpha1.App) (ctrlwebhookadmission.Warnings, error) {
	return nil, nil
}

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
	if k8s.Enabled && k8s.Version == "" {
		return nil, errors.New("spec.kubernetes.version must not be empty when spec.kubernetes.enabled is true")
	}

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.

                  name:
                    default: moby
                    description: |-
                      name specifies the container engine to use.
                      Valid values are "moby" (Docker-compatible) and "containerd".
                    enum:
                    - moby
                    - containerd
                    type: string
                required:
                - name
                type: object
              kubernetes:

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.

  kubernetes:
    enabled: false
EOF
}

@test "dry-run rejects invalid containerEngine name" {
    run -1 rdd ctl apply --dry-run=server -f - <<EOF
apiVersion: app.rancherdesktop.io/v1alpha1
kind: App
metadata:
  name: app

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

	k8s := app.Spec.Kubernetes
	if k8s.Enabled && k8s.Version == "" {
		return nil, errors.New("spec.kubernetes.version must not be empty when spec.kubernetes.enabled is true")
	}

	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)
		}
	}

	return nil, nil
}

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.

APP_NAME="app"
VM_NAME="rd"
INPUT_CM_NAME="rd"
APP_VALIDATOR_CONFIG="app-validator"
K3S_VERSION="1.32.0"

delete_app() {
    # Stop the VM first so the LimaVM finalizer clears quickly.
    rdd set running=false 2>/dev/null || true
    rdd ctl delete app "${APP_NAME}" --ignore-not-found

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.

func (c *controller) setupWebhook(mgr ctrl.Manager) error {
	validatingConfig := base.WebhookConfig[*v1alpha1.App]{
		Name:        appValidatorConfigName,
		WebhookName: appValidatorWebhookName,
		WebhookPort: c.webhookPort,
		Operations: []admissionregistrationv1.OperationType{
			admissionregistrationv1.Create,
			admissionregistrationv1.Update,
		},
		Validator: &controllers.AppValidator{K3sVersionsData: k3sVersionsData},
	}

	managers, err := base.SetupWebhookForResource(mgr, &v1alpha1.App{}, validatingConfig)
	if err != nil {

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.

// ValidateCreate validates a new App resource.
func (v *AppValidator) ValidateCreate(_ context.Context, obj *v1alpha1.App) (ctrlwebhookadmission.Warnings, error) {
	return v.validate(obj)
}

// ValidateUpdate validates an updated App resource.
func (v *AppValidator) ValidateUpdate(_ context.Context, _, newObj *v1alpha1.App) (ctrlwebhookadmission.Warnings, error) {
	return v.validate(newObj)
}

// ValidateDelete is a no-op; App deletion is governed by the cleanup finalizer.
func (v *AppValidator) ValidateDelete(_ context.Context, _ *v1alpha1.App) (ctrlwebhookadmission.Warnings, error) {
	return nil, nil
}

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.

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.