Deep Review: 20260324-190453-pr-236

In the deletion path, non-NotFound errors from ConfigMap Gets at lines 69–70 and lines 82–84 are logged but fall through to RemoveCleanupFinalizer at line 86. If a transient API error prevents reading a ConfigMap, the App's cleanup finalizer is removed and the App can be deleted, potentially orphaning ConfigMaps. Since RDD has no garbage collector, orphaned resources with owned finalizers would linger permanently.

	case apierrors.IsNotFound(err):
		// LimaVM is gone. Clean up any ConfigMaps that may have been left behind.
		inputCM := &corev1.ConfigMap{}
		if cmErr := r.Get(ctx, client.ObjectKey{Name: inputConfigMapName, Namespace: namespace}, inputCM); cmErr == nil {
			if cmErr := r.Delete(ctx, inputCM); cmErr != nil && !apierrors.IsNotFound(cmErr) {
				return ctrl.Result{}, fmt.Errorf("failed to delete input ConfigMap during cleanup: %w", cmErr)
			}
		} else if !apierrors.IsNotFound(cmErr) {
			log.Error(cmErr, "Failed to fetch input ConfigMap during cleanup")
		}

		templateCM := &corev1.ConfigMap{}
		templateCMName := limaVMName + "-template"
		if cmErr := r.Get(ctx, client.ObjectKey{Name: templateCMName, Namespace: namespace}, templateCM); cmErr == nil {
			if cmErr := base.RemoveOwnedFinalizer(ctx, r.Client, templateCM, v1alpha1.AppKind); cmErr != nil {
				return ctrl.Result{}, fmt.Errorf("failed to remove finalizer from template ConfigMap: %w", cmErr)
			}
			if cmErr := r.Delete(ctx, templateCM); cmErr != nil && !apierrors.IsNotFound(cmErr) {
				return ctrl.Result{}, fmt.Errorf("failed to delete template ConfigMap during cleanup: %w", cmErr)
			}
		} else if !apierrors.IsNotFound(cmErr) {
			log.Error(cmErr, "Failed to fetch template ConfigMap during cleanup")
		}

		return ctrl.Result{}, base.RemoveCleanupFinalizer(ctx, r.Client, &app)

In the embedded single-user kube-apiserver, transient Get errors are very rare, making the practical risk low. But the defensive fix is straightforward.

Fix: Return errors from ConfigMap Gets instead of logging and continuing, so the cleanup finalizer is not removed until both ConfigMaps are confirmed absent or successfully deleted.

If a stale input ConfigMap exists from a previous crash (between creating the CM at line 140 and the LimaVM at line 162), the controller reuses it without updating its Data to match r.LimaTemplateData. If the binary was updated between crashes, the LimaVM would be created from outdated template data.

	if apierrors.IsNotFound(limaVMErr) {
		inputCM := &corev1.ConfigMap{}
		err := r.Get(ctx, client.ObjectKey{Name: inputConfigMapName, Namespace: namespace}, inputCM)
		if err != nil && !apierrors.IsNotFound(err) {
			return ctrl.Result{}, err
		}
		if apierrors.IsNotFound(err) {
			inputCM = &corev1.ConfigMap{
				ObjectMeta: metav1.ObjectMeta{
					Name:      inputConfigMapName,
					Namespace: namespace,
				},
				Data: map[string]string{
					limav1alpha1.TemplateConfigMapKey: r.LimaTemplateData,
				},
			}
			if err := ctrl.SetControllerReference(&app, inputCM, r.Scheme); err != nil {
				return ctrl.Result{}, fmt.Errorf("failed to set owner reference on input ConfigMap: %w", err)
			}
			if err := r.Create(ctx, inputCM); err != nil {
				return ctrl.Result{}, fmt.Errorf("failed to create input ConfigMap: %w", err)
			}
		}

The practical risk is low: the window is narrow (crash between two sequential API calls), and template data is embedded so it only changes with binary updates (which restart the controller). But an explicit update-or-create pattern would eliminate the concern.

Fix: Use controllerutil.CreateOrUpdate or update the Data and OwnerReference when the ConfigMap already exists.

The comment says 32768 is a byte limit on the whole object, but 32768 is actually the per-field character limit on the condition message (from maxLength: 32768 in the CRD validation). The whole-object limit is 1.5 MB. The comment should reference the CRD field validation, not a whole-object byte limit.

// ConditionMessageMaxLen is the maximum number of runes allowed in a Kubernetes
// condition message. The API server enforces a 32768-byte limit on the whole
// object; keeping messages within this bound avoids rejections from operations
// that produce very long error strings (e.g. accumulated retry errors from
// image downloads).
const ConditionMessageMaxLen = 32768

The new function has no unit test. A table-driven test covering empty string, under-limit, at-limit, and over-limit cases (including multi-byte runes) would prevent regressions on the boundary logic.