Update dependent resources

[bittrance]

This takes a first stab at reconciling changes to MarimoNotebook:

• pvc: explicitly declare storage attribute immutable (i.e. no reconciling)
• service: attempt in-place change of service
• recreate Marimo pod on changes

Fixes #6 .

Also, debug logging of desired state for pods.

[Copilot]

Pull request overview

This PR improves reconciliation of dependent resources for MarimoNotebook, aiming to ensure spec updates (e.g., auth changes) are reflected by updating the Service in-place and recreating the Pod when its desired spec changes, while explicitly making spec.storage immutable.

Changes:

• Add pod-spec hashing via an annotation to detect spec changes and trigger Pod recreation.
• Switch Service reconciliation to controllerutil.CreateOrUpdate to support in-place updates (e.g., sidecar ports).
• Enforce spec.storage immutability via CRD CEL (x-kubernetes-validations) / kubebuilder XValidation, with tests covering update scenarios.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
pkg/resources/pod.go	Adds pod spec hashing helpers/constants to support change detection.
internal/controller/marimonotebook_controller.go	Recreates Pods when the desired spec hash changes; reconciles Services via CreateOrUpdate; logs desired pod JSON.
internal/controller/marimonotebook_controller_test.go	Adds controller tests for service port updates, pod recreation on env change, and storage immutability rejection.
deploy/install.yaml	Adds CRD validation making spec.storage immutable in the rendered install manifest.
config/crd/bases/marimo.io_marimos.yaml	Adds the same CRD validation in the kubebuilder base CRD.
api/v1alpha1/marimonotebook_types.go	Adds kubebuilder XValidation to make spec.storage immutable.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The controller is logging the full desired Pod JSON (including container env var values). This can leak user-provided secrets into controller logs and can be very noisy. Consider removing this log, logging only a redacted subset (e.g., name/namespace + spec hash), or gating it behind a higher verbosity level and explicitly omitting env/args.

[Copilot]

PodSpecHash silently returns an empty string if JSON marshaling fails, and reconcilePod treats that as a valid hash. If this ever happens, spec changes may not be detected and pods may not be recreated. Consider returning (string, error) and failing/requeueing on error (or at least logging and forcing a recreate).

[Copilot]

Marking spec.storage as fully immutable also prevents setting storage later (nil -> value) and therefore makes it impossible to add sidecars to an existing notebook created without storage (given the existing "storage is required when sidecars are specified" rule). If the intent is "immutable once set", consider a rule like oldSelf == null || self == oldSelf so storage can be added once but not modified thereafter.

Suggested change

	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="storage is immutable"
	// +kubebuilder:validation:XValidation:rule="oldSelf == null || self == oldSelf",message="storage is immutable once set"

[Copilot]

The new pod-recreate-on-change behavior is intended to fix auth reconciliation (issue #6), but there is no controller test covering an update that adds/removes spec.auth.password.secretKeyRef and verifies the Pod is recreated with the expected secret volume mount. Adding a test for this specific regression would help prevent future breakage.

Suggested change

	It("should recreate Pod when auth password secretKeyRef is added and removed", func() {
	By("creating the MarimoNotebook without auth")
	Expect(k8sClient.Create(ctx, notebook)).To(Succeed())
	By("waiting for initial Pod to be created")
	pod := &corev1.Pod{}
	Eventually(func() error {
	return k8sClient.Get(ctx, namespacedName, pod)
	}, timeout, interval).Should(Succeed())
	originalUID := pod.UID
	By("creating a Secret to be used for auth password")
	secret := &corev1.Secret{
	ObjectMeta: metav1.ObjectMeta{
	Name: "auth-secret-" + randString(),
	Namespace: notebook.Namespace,
	},
	StringData: map[string]string{
	"password": "super-secret-password",
	},
	}
	Expect(k8sClient.Create(ctx, secret)).To(Succeed())
	By("updating the notebook to reference the auth password Secret")
	nb := &marimov1alpha1.MarimoNotebook{}
	Expect(k8sClient.Get(ctx, namespacedName, nb)).To(Succeed())
	if nb.Spec.Auth == nil {
	nb.Spec.Auth = &marimov1alpha1.AuthSpec{}
	}
	if nb.Spec.Auth.Password == nil {
	nb.Spec.Auth.Password = &marimov1alpha1.PasswordAuthSpec{}
	}
	nb.Spec.Auth.Password.SecretKeyRef = &corev1.SecretKeySelector{
	LocalObjectReference: corev1.LocalObjectReference{
	Name: secret.Name,
	},
	Key: "password",
	}
	Expect(k8sClient.Update(ctx, nb)).To(Succeed())
	By("verifying Pod is recreated and mounts the auth Secret")
	var secretVolumeName string
	Eventually(func() bool {
	if err := k8sClient.Get(ctx, namespacedName, pod); err != nil {
	return false
	}
	if pod.UID == originalUID {
	return false // still the old pod
	}
	secretVolumeName = ""
	for _, v := range pod.Spec.Volumes {
	if v.Secret != nil && v.Secret.SecretName == secret.Name {
	secretVolumeName = v.Name
	break
	}
	}
	if secretVolumeName == "" {
	return false
	}
	for _, vm := range pod.Spec.Containers[0].VolumeMounts {
	if vm.Name == secretVolumeName {
	return true
	}
	}
	return false
	}, timeout, interval).Should(BeTrue(), "Pod should be recreated and mount the auth Secret")
	authUID := pod.UID
	By("updating the notebook to remove the auth password SecretKeyRef")
	nb2 := &marimov1alpha1.MarimoNotebook{}
	Expect(k8sClient.Get(ctx, namespacedName, nb2)).To(Succeed())
	if nb2.Spec.Auth != nil && nb2.Spec.Auth.Password != nil {
	nb2.Spec.Auth.Password.SecretKeyRef = nil
	}
	Expect(k8sClient.Update(ctx, nb2)).To(Succeed())
	By("verifying Pod is recreated and no longer mounts the auth Secret")
	Eventually(func() bool {
	if err := k8sClient.Get(ctx, namespacedName, pod); err != nil {
	return false
	}
	if pod.UID == authUID {
	return false // still the pod with auth secret
	}
	for _, v := range pod.Spec.Volumes {
	if v.Secret != nil && v.Secret.SecretName == secret.Name {
	// still has the secret volume
	return false
	}
	}
	for _, vm := range pod.Spec.Containers[0].VolumeMounts {
	if vm.Name == secretVolumeName {
	// still mounts the secret volume
	return false
	}
	}
	return true
	}, timeout, interval).Should(BeTrue(), "Pod should be recreated and no longer mount the auth Secret")
	})

[Copilot]

Service reconciliation updates labels, ports, and selector but does not apply other desired ServiceSpec fields (e.g., Type). Even if Type currently defaults to ClusterIP, explicitly setting svc.Spec.Type = desired.Spec.Type (and any other fields BuildService considers authoritative) will keep the actual Service aligned with the desired state and avoid surprising drift.

Suggested change

	svc.Spec.Selector = desired.Spec.Selector
	svc.Spec.Selector = desired.Spec.Selector
	svc.Spec.Type = desired.Spec.Type