Deep Review: 20260413-120825-pr-278

    "v1.32.6+k3s1",
    "v1.32.7+k3s1",
    "v1.32.8+k3s1",
    "v1.32.9+k3s1",
    "v1.32.10+k3s1",
    "v1.32.11+k3s3",
    "v1.32.12+k3s1",
    "v1.32.13+k3s1",
    "v1.33.0+k3s1",
    "v1.33.1+k3s1",
    "v1.33.2+k3s1",

parseK3sVersions strips both the leading v and the +k3s* suffix, so the lookup set becomes {"1.32.11", "1.33.7", ...}. A user setting kubernetes.version=1.32.11 passes validation at app_controller.go:127. The controller then creates the LimaVM, and the provisioning script builds INSTALL_K3S_VERSION from the bare version plus a hardcoded +k3s1 suffix — a tag that does not exist on the k3s release page. curl -sfL https://get.k3s.io | sh - aborts under set -o errexit and the VM never reaches Running.

	if app.Spec.Kubernetes.Enabled && app.Spec.Kubernetes.Version != "" {
		supportedVersions, err := parseK3sVersions(r.K3sVersionsData)
		if err != nil {
			return ctrl.Result{}, fmt.Errorf("failed to load k3s versions: %w", err)
		}
		if _, ok := supportedVersions[app.Spec.Kubernetes.Version]; !ok {
			unsupported := apimeta.SetStatusCondition(&app.Status.Conditions, metav1.Condition{
				Type:               "Ready",
				Status:             metav1.ConditionFalse,
				Reason:             "UnsupportedKubernetesVersion",
				Message:            fmt.Sprintf("Kubernetes version %q is not supported; see the bundled k3s-versions.json for valid versions", app.Spec.Kubernetes.Version),

# pkg/controllers/app/app/lima-template.yaml:57
export INSTALL_K3S_VERSION="v${PARAM_KUBERNETES_VERSION}+k3s1"

4 of 35 bundled versions (~11%) hit this. The validator's contract ("if it's in the list, it works") is established and immediately violated.

Fix: have parseK3sVersions return a map from bare semver to the full release identifier (e.g., {"1.32.11": "v1.32.11+k3s3"}). Thread the full tag through applySpecToTemplate as a new PARAM_K3S_RELEASE, and rewrite the template line to export INSTALL_K3S_VERSION="${PARAM_K3S_RELEASE}". Drop the hardcoded +k3s1.

			log.Error(err, "Unable to fetch App singleton")
		}
		return ctrl.Result{}, client.IgnoreNotFound(err)
	}

	hostHome, err := os.UserHomeDir()
	if err != nil {
		return ctrl.Result{}, fmt.Errorf("failed to get host home directory: %w", err)
	}

	// Handle deletion, delete the LimaVM and wait for it to finish cleaning up.
	if base.IsBeingDeleted(&app) {
		log.Info("App resource is being deleted, performing cleanup")

The os.UserHomeDir() lookup runs unconditionally at the top of every reconcile, before base.IsBeingDeleted(&app) is evaluated. If HOME ever becomes unset or inaccessible (e.g., the daemon is restarted under a launchd context that clears the environment), the finalizer cleanup path deadlocks: the App can never be deleted even though the cleanup logic does not need the host home directory. The correct placement is after the deletion branch and the cleanup-finalizer setup, co-located with the first caller of applySpecToTemplate.

Fix: move the os.UserHomeDir() call below the base.IsBeingDeleted block. Better: cache the result once at controller setup time (see S1).

	}

	namespace := app.GetResourceNamespace()

	// Validate the requested Kubernetes version against the bundled version list.
	if app.Spec.Kubernetes.Enabled && app.Spec.Kubernetes.Version != "" {
		supportedVersions, err := parseK3sVersions(r.K3sVersionsData)
		if err != nil {
			return ctrl.Result{}, fmt.Errorf("failed to load k3s versions: %w", err)
		}
		if _, ok := supportedVersions[app.Spec.Kubernetes.Version]; !ok {

KubernetesSpec.Version is +optional with no +kubebuilder:default, so a user creating an App with {kubernetes: {enabled: true}} and no version sets Version="". The check short-circuits, the reconciler proceeds, the template exports INSTALL_K3S_VERSION="v+k3s1", and k3s install fails with a non-existent version tag. The CLI rdd set explicitly supports clearing the version field (set.bats:124-131), so this is a reachable state.

    assert_output "1.32.2"
}

# --- Clear: reuses App from previous test ---

@test "rdd set clears string field with empty value" {
    run -0 get_app_field '.spec.kubernetes.version'
    assert_output "1.32.2"

    rdd set kubernetes.version=

    run -0 get_app_field '.spec.kubernetes.version'
    assert_output ""
}

# --- Dry-run: reuses App from previous tests ---

@test "rdd set --dry-run validates without persisting" {

This is exactly the class of bad input the PR set out to prevent, but the empty-string case falls through into the VM. The user only sees a CreateFailed or k3s install error with no pointer to the spec mistake.

Fix: drop the Version != "" clause and let the empty-string lookup naturally fail with UnsupportedKubernetesVersion. Optionally tailor the message to "version is required when enabled" when empty. A +kubebuilder:default matching a bundled version would close this at the API layer.

-if app.Spec.Kubernetes.Enabled && app.Spec.Kubernetes.Version != "" {
+if app.Spec.Kubernetes.Enabled {

		return ctrl.Result{}, nil
	}

	namespace := app.GetResourceNamespace()

	// Validate the requested Kubernetes version against the bundled version list.
	if app.Spec.Kubernetes.Enabled && app.Spec.Kubernetes.Version != "" {
		supportedVersions, err := parseK3sVersions(r.K3sVersionsData)
		if err != nil {
			return ctrl.Result{}, fmt.Errorf("failed to load k3s versions: %w", err)
		}
		if _, ok := supportedVersions[app.Spec.Kubernetes.Version]; !ok {
			unsupported := apimeta.SetStatusCondition(&app.Status.Conditions, metav1.Condition{
				Type:               "Ready",
				Status:             metav1.ConditionFalse,
				Reason:             "UnsupportedKubernetesVersion",
				Message:            fmt.Sprintf("Kubernetes version %q is not supported; see the bundled k3s-versions.json for valid versions", app.Spec.Kubernetes.Version),
				ObservedGeneration: app.Generation,
				LastTransitionTime: metav1.Now(),
			})
			if unsupported {
				if err := r.Status().Update(ctx, &app); err != nil {
					log.Error(err, "Unable to update App status for unsupported version")
					return ctrl.Result{}, err
				}
			}
			return ctrl.Result{}, nil
		}
	}

	// Check whether the LimaVM already exists. If not, create the input ConfigMap and LimaVM.
	limaVM := &limav1alpha1.LimaVM{}
	limaVMErr := r.Get(ctx, client.ObjectKey{Name: limaVMName, Namespace: namespace}, limaVM)
	if limaVMErr != nil && !apierrors.IsNotFound(limaVMErr) {

The mirror loop at lines 268-284 only writes condition types the LimaVM exposes (Created, Running). LimaVM never sets Ready. So once the validation block has stamped Ready=False on the App and the user does rdd set kubernetes.version=1.32.0 (or kubernetes.enabled=false), the condition persists indefinitely. kubectl get app -o yaml will show Ready=False even though the VM is happily running k3s. The new BATS tests don't catch this because every test calls delete_app first, which destroys the conditions.

	// The priority chain above returns after every other action, so the App's
	// resourceVersion from the initial Get is still current — no re-read needed.
	// Truncate messages defensively: the LimaVM controller already truncates at
	// source, but guarding here ensures a future bypass can't cause the App
	// status update to fail CRD validation.
	statusChanged := false
	for _, cond := range limaVM.Status.Conditions {
		statusChanged = apimeta.SetStatusCondition(&app.Status.Conditions, metav1.Condition{
			Type:               cond.Type,
			Status:             cond.Status,
			Reason:             cond.Reason,
			Message:            base.TruncateConditionMessage(cond.Message),
			ObservedGeneration: app.Generation,
			LastTransitionTime: cond.LastTransitionTime,
		}) || statusChanged
	}
	if statusChanged {
		if err := r.Status().Update(ctx, &app); err != nil {
			log.Error(err, "Unable to update App status")
			return ctrl.Result{}, err
		}
	}

	return ctrl.Result{}, nil
}

// SetupWithManager sets up the controller with the Manager.

Fix: when validation passes (or Kubernetes is disabled entirely), drop the prior Ready condition and requeue. Using apimeta.RemoveStatusCondition followed by r.Status().Update is the minimal change:

+if apimeta.RemoveStatusCondition(&app.Status.Conditions, "Ready") {
+    if err := r.Status().Update(ctx, &app); err != nil {
+        return ctrl.Result{}, err
+    }
+    return ctrl.Result{}, nil
+}

	LimaTemplateData string
	K3sVersionsData  string
}

func applySpecToTemplate(baseTemplate string, spec v1alpha1.AppSpec, hostHome string) string {
	return baseTemplate + fmt.Sprintf(
		"\nparam:\n  CONTAINER_ENGINE: %s\n  HOST_HOME: %s\n  KUBERNETES_ENABLED: %v\n  KUBERNETES_VERSION: %s\n",
		spec.ContainerEngine.Name,
		hostHome,
		spec.Kubernetes.Enabled,
		spec.Kubernetes.Version,
	)
}

The previous line read HOST_HOME: \"{{.Home}}\" — the literal string was double-quoted in the YAML output. Commit e57a182 replaces {{.Home}} with os.UserHomeDir() (correct fix for the literal-template bug) but drops the quotes. On Windows, os.UserHomeDir() returns %USERPROFILE%, which commonly includes spaces (C:\Users\Jan Dubois) and can legally contain # in usernames, which turns into a YAML comment. Even on Unix, a user with a # or : in their home path would trip the YAML parser.

Fix: use %q so the value is always emitted as a quoted YAML scalar. Lima's param parser accepts double-quoted scalars — the existing template uses them elsewhere (e.g., hostSocket: "{{.Home}}/.rd/docker.sock").

-"\nparam:\n  CONTAINER_ENGINE: %s\n  HOST_HOME: %s\n  KUBERNETES_ENABLED: %v\n  KUBERNETES_VERSION: %s\n",
+"\nparam:\n  CONTAINER_ENGINE: %s\n  HOST_HOME: %q\n  KUBERNETES_ENABLED: %v\n  KUBERNETES_VERSION: %s\n",

@test "wait for k3s to be ready" {
    # k3s is downloaded and installed on first use; allow extra time.
    try --max 36 --delay 10 -- rdd limavm shell "${VM_NAME}" sudo systemctl is-active k3s
}

@test "kubernetes nodes are Ready" {
    run -0 rdd limavm shell "${VM_NAME}" sudo k3s kubectl get nodes
    assert_output --partial "Ready"
}

@test "stop VM after Kubernetes test" {
    rdd set running=false
    rdd ctl wait --for=condition=Running=False \
        limavm/"${VM_NAME}" --namespace "${RDD_NAMESPACE}" --timeout=60s

kubectl get nodes prints either Ready or NotReady in the STATUS column. A substring search for Ready matches both, so the test gives a false positive when the node is still NotReady (cordoned, network init, transient failures). Single-node k3s usually flips Ready quickly, so this rarely fires in practice — but the test claims to verify readiness and doesn't.

Fix: use kubectl wait and let it block on the actual condition. No assert_output needed.

 @test "kubernetes nodes are Ready" {
-    run -0 rdd limavm shell "${VM_NAME}" sudo k3s kubectl get nodes
-    assert_output --partial "Ready"
+    run -0 rdd limavm shell "${VM_NAME}" sudo k3s kubectl wait \
+        --for=condition=Ready nodes --all --timeout=60s
 }