Runtime: Prevent resource protos from being mutated after being read (#…

…6233)

* Runtime: Prevent resource protos from being mutated after being read

* Fix rename issue

* Fix version update test

runtime/catalog_cache.go

@@ -258,7 +258,7 @@ func (c *catalogCache) create(name *runtimev1.ResourceName, refs []*runtimev1.Re
 
 // rename renames a resource in the catalog and sets the r.Meta.RenamedFrom field.
 func (c *catalogCache) rename(name, newName *runtimev1.ResourceName) error {
-	r, err := c.get(name, false, false)
+	r, err := c.get(name, false, true)
 	if err != nil {
 		return err
 	}
@@ -278,7 +278,7 @@ func (c *catalogCache) rename(name, newName *runtimev1.ResourceName) error {
 
 // clearRenamedFrom clears the r.Meta.RenamedFrom field without bumping version numbers.
 func (c *catalogCache) clearRenamedFrom(name *runtimev1.ResourceName) error {
-	r, err := c.get(name, false, false)
+	r, err := c.get(name, false, true)
 	if err != nil {
 		return err
 	}
@@ -295,7 +295,7 @@ func (c *catalogCache) clearRenamedFrom(name *runtimev1.ResourceName) error {
 
 // updateMeta updates the meta fields of a resource.
 func (c *catalogCache) updateMeta(name *runtimev1.ResourceName, refs []*runtimev1.ResourceName, owner *runtimev1.ResourceName, paths []string) error {
-	r, err := c.get(name, false, false)
+	r, err := c.get(name, false, true)
 	if err != nil {
 		return err
 	}
@@ -315,18 +315,19 @@ func (c *catalogCache) updateMeta(name *runtimev1.ResourceName, refs []*runtimev
 // updateSpec updates the spec field of a resource.
 // It uses the spec from the passed resource and disregards its other fields.
 func (c *catalogCache) updateSpec(name *runtimev1.ResourceName, from *runtimev1.Resource) error {
-	r, err := c.get(name, false, false)
+	r, err := c.get(name, false, true)
 	if err != nil {
 		return err
 	}
-	// NOTE: No need to unlink/link because no indexed fields are edited.
+	c.unlink(r)
 	err = c.ctrl.reconciler(name.Kind).AssignSpec(from, r)
 	if err != nil {
 		return err
 	}
 	r.Meta.Version++
 	r.Meta.SpecVersion++
 	r.Meta.SpecUpdatedOn = timestamppb.Now()
+	c.link(r)
 	c.dirty[nameStr(r.Meta.Name)] = r.Meta.Name
 	c.addEvent(r.Meta.Name, r, runtimev1.ResourceEvent_RESOURCE_EVENT_WRITE)
 	return nil
@@ -335,26 +336,27 @@ func (c *catalogCache) updateSpec(name *runtimev1.ResourceName, from *runtimev1.
 // updateState updates the state field of a resource.
 // It uses the state from the passed resource and disregards its other fields.
 func (c *catalogCache) updateState(name *runtimev1.ResourceName, from *runtimev1.Resource) error {
-	r, err := c.get(name, true, false)
+	r, err := c.get(name, true, true)
 	if err != nil {
 		return err
 	}
-	// NOTE: No need to unlink/link because no indexed fields are edited.
+	c.unlink(r)
 	err = c.ctrl.reconciler(name.Kind).AssignState(from, r)
 	if err != nil {
 		return err
 	}
 	r.Meta.Version++
 	r.Meta.StateVersion++
 	r.Meta.StateUpdatedOn = timestamppb.Now()
+	c.link(r)
 	c.dirty[nameStr(r.Meta.Name)] = r.Meta.Name
 	c.addEvent(r.Meta.Name, r, runtimev1.ResourceEvent_RESOURCE_EVENT_WRITE)
 	return nil
 }
 
 // updateError updates the reconcile_error field of a resource.
 func (c *catalogCache) updateError(name *runtimev1.ResourceName, reconcileErr error) error {
-	r, err := c.get(name, true, false)
+	r, err := c.get(name, true, true)
 	if err != nil {
 		return err
 	}
@@ -366,11 +368,12 @@ func (c *catalogCache) updateError(name *runtimev1.ResourceName, reconcileErr er
 		// Since bumping the state version usually invalidates derived things, we don't want to do it redundantly.
 		return nil
 	}
-	// NOTE: No need to unlink/link because no indexed fields are edited.
+	c.unlink(r)
 	r.Meta.ReconcileError = errStr
 	r.Meta.Version++
 	r.Meta.StateVersion++
 	r.Meta.StateUpdatedOn = timestamppb.Now()
+	c.link(r)
 	c.dirty[nameStr(r.Meta.Name)] = r.Meta.Name
 	c.addEvent(r.Meta.Name, r, runtimev1.ResourceEvent_RESOURCE_EVENT_WRITE)
 	return nil
@@ -379,7 +382,7 @@ func (c *catalogCache) updateError(name *runtimev1.ResourceName, reconcileErr er
 // updateDeleted sets the deleted_on field of a resource (a soft delete).
 // Afterwards, the resource can still be accessed by passing withDeleted to the getters.
 func (c *catalogCache) updateDeleted(name *runtimev1.ResourceName) error {
-	r, err := c.get(name, false, false)
+	r, err := c.get(name, false, true)
 	if err != nil {
 		return err
 	}
@@ -397,16 +400,18 @@ func (c *catalogCache) updateDeleted(name *runtimev1.ResourceName) error {
 // updateStatus updates the ephemeral status fields on a resource.
 // The values of these fields are reset next time a catalog cache is created.
 func (c *catalogCache) updateStatus(name *runtimev1.ResourceName, status runtimev1.ReconcileStatus, reconcileOn time.Time) error {
-	r, err := c.get(name, true, false)
+	r, err := c.get(name, true, true)
 	if err != nil {
 		return err
 	}
+	c.unlink(r)
 	r.Meta.ReconcileStatus = status
 	if reconcileOn.IsZero() {
 		r.Meta.ReconcileOn = nil
 	} else {
 		r.Meta.ReconcileOn = timestamppb.New(reconcileOn)
 	}
+	c.link(r)
 	c.addEvent(r.Meta.Name, r, runtimev1.ResourceEvent_RESOURCE_EVENT_WRITE)
 	return nil
 }

runtime/reconcilers/alert.go

@@ -360,7 +360,7 @@ func (r *AlertReconciler) setTriggerFalse(ctx context.Context, n *runtimev1.Reso
 	r.C.Lock(ctx)
 	defer r.C.Unlock(ctx)
 
-	self, err := r.C.Get(ctx, n, false)
+	self, err := r.C.Get(ctx, n, true)
 	if err != nil {
 		return err
 	}

runtime/reconcilers/model.go

@@ -550,7 +550,7 @@ func (r *ModelReconciler) updateTriggerFalse(ctx context.Context, n *runtimev1.R
 	r.C.Lock(ctx)
 	defer r.C.Unlock(ctx)
 
-	self, err := r.C.Get(ctx, n, false)
+	self, err := r.C.Get(ctx, n, true)
 	if err != nil {
 		return err
 	}

runtime/reconcilers/project_parser.go

@@ -416,13 +416,9 @@ func (r *ProjectParserReconciler) reconcileResources(ctx context.Context, inst *
 // reconcileResourcesDiff is similar to reconcileResources, but uses a diff from parser.Reparse instead of doing a full comparison of all resources.
 func (r *ProjectParserReconciler) reconcileResourcesDiff(ctx context.Context, inst *drivers.Instance, self *runtimev1.Resource, parser *compilerv1.Parser, diff *compilerv1.Diff) error {
 	// Gather resource to delete so we can check for renames.
-	deleteResources := make([]*runtimev1.Resource, 0, len(diff.Deleted))
+	deleteResources := make([]*runtimev1.ResourceName, 0, len(diff.Deleted))
 	for _, n := range diff.Deleted {
-		r, err := r.C.Get(ctx, runtime.ResourceNameFromCompiler(n), false)
-		if err != nil {
-			return err
-		}
-		deleteResources = append(deleteResources, r)
+		deleteResources = append(deleteResources, runtime.ResourceNameFromCompiler(n))
 	}
 
 	// Updates
@@ -444,13 +440,17 @@ func (r *ProjectParserReconciler) reconcileResourcesDiff(ctx context.Context, in
 
 		// Rename if possible
 		renamed := false
-		for idx, rr := range deleteResources {
-			if rr == nil {
+		for idx, rn := range deleteResources {
+			if rn == nil {
 				// Already renamed
 				continue
 			}
 
-			var err error
+			rr, err := r.C.Get(ctx, rn, false)
+			if err != nil {
+				return err
+			}
+
 			renamed, err = r.attemptRename(ctx, inst, self, def, rr)
 			if err != nil {
 				return err
@@ -472,13 +472,13 @@ func (r *ProjectParserReconciler) reconcileResourcesDiff(ctx context.Context, in
 	}
 
 	// Deletes
-	for _, rr := range deleteResources {
+	for _, rn := range deleteResources {
 		// The ones that got renamed were set to nil
-		if rr == nil {
+		if rn == nil {
 			continue
 		}
 
-		err := r.C.Delete(ctx, rr.Meta.Name)
+		err := r.C.Delete(ctx, rn)
 		if err != nil {
 			return err
 		}
@@ -565,16 +565,24 @@ func (r *ProjectParserReconciler) putParserResourceDef(ctx context.Context, inst
 		return r.C.Create(ctx, n, refs, self.Meta.Name, def.Paths, false, res)
 	}
 
-	// The name may have changed to a different case (e.g. aAa -> Aaa)
+	// Handle changed name and/or path
 	if n.Kind == existing.Meta.Name.Kind && n.Name != existing.Meta.Name.Name {
+		// The name may have changed to a different case (e.g. aAa -> Aaa).
+		// Note that this also updates the paths (updating them separately with UpdateMeta would be considered a mutation of a renamed resource, which requires falling back to a less optimal reconciliation).
 		err := r.C.UpdateName(ctx, existing.Meta.Name, n, self.Meta.Name, def.Paths)
 		if err != nil {
 			return err
 		}
+	} else if !slices.Equal(existing.Meta.FilePaths, def.Paths) {
+		// The path may have been changed. Usually this case is covered in the UpdateName case above because changing a file path usually changes the name.
+		err := r.C.UpdateMeta(ctx, n, existing.Meta.Refs, self.Meta.Name, def.Paths)
+		if err != nil {
+			return err
+		}
 	}
 
-	// Update meta if refs or file paths changed
-	if !slices.Equal(existing.Meta.FilePaths, def.Paths) || !equalResourceNames(existing.Meta.Refs, refs) {
+	// Update meta if refs changed
+	if !equalResourceNames(existing.Meta.Refs, refs) {
 		err := r.C.UpdateMeta(ctx, n, refs, self.Meta.Name, def.Paths)
 		if err != nil {
 			return err

runtime/reconcilers/refresh_trigger.go

@@ -146,7 +146,7 @@ func (r *RefreshTriggerReconciler) Reconcile(ctx context.Context, n *runtimev1.R
 
 	// Handle generic resource triggers
 	for _, rn := range trigger.Spec.Resources {
-		res, err := r.C.Get(ctx, rn, false)
+		res, err := r.C.Get(ctx, rn, true)
 		if err != nil {
 			// Skip triggers for non-existent resources
 			if !errors.Is(err, drivers.ErrResourceNotFound) {

runtime/reconcilers/report.go

@@ -208,7 +208,7 @@ func (r *ReportReconciler) setTriggerFalse(ctx context.Context, n *runtimev1.Res
 	r.C.Lock(ctx)
 	defer r.C.Unlock(ctx)
 
-	self, err := r.C.Get(ctx, n, false)
+	self, err := r.C.Get(ctx, n, true)
 	if err != nil {
 		return err
 	}

runtime/reconcilers/source.go

@@ -308,7 +308,7 @@ func (r *SourceReconciler) setTriggerFalse(ctx context.Context, n *runtimev1.Res
 	r.C.Lock(ctx)
 	defer r.C.Unlock(ctx)
 
-	self, err := r.C.Get(ctx, n, false)
+	self, err := r.C.Get(ctx, n, true)
 	if err != nil {
 		return err
 	}

runtime/testruntime/reconcile.go

@@ -65,10 +65,9 @@ func RefreshAndWait(t testing.TB, rt *runtime.Runtime, id string, n *runtimev1.R
 	ctrl, err := rt.Controller(context.Background(), id)
 	require.NoError(t, err)
 
-	// Get spec version before refresh
-	r, err := ctrl.Get(context.Background(), n, false)
+	// Get resource before refresh
+	rPrev, err := ctrl.Get(context.Background(), n, false)
 	require.NoError(t, err)
-	prevSpecVersion := r.Meta.SpecVersion
 
 	// Create refresh trigger
 	trgName := &runtimev1.ResourceName{Kind: runtime.ResourceKindRefreshTrigger, Name: time.Now().String()}
@@ -87,8 +86,12 @@ func RefreshAndWait(t testing.TB, rt *runtime.Runtime, id string, n *runtimev1.R
 	err = ctrl.WaitUntilIdle(context.Background(), false)
 	require.NoError(t, err)
 
+	// Get resource after refresh
+	rNew, err := ctrl.Get(context.Background(), n, false)
+	require.NoError(t, err)
+
 	// Check the resource's spec version has increased
-	require.Greater(t, r.Meta.SpecVersion, prevSpecVersion)
+	require.Greater(t, rNew.Meta.SpecVersion, rPrev.Meta.SpecVersion)
 }
 
 func RequireReconcileState(t testing.TB, rt *runtime.Runtime, id string, lenResources, lenReconcileErrs, lenParseErrs int) {