resource: preserve deferred deletion metadata on non-CAS writes (#19674)

agent/grpc-external/services/resource/write.go

@@ -182,9 +182,11 @@ func (s *Server) Write(ctx context.Context, req *pbresource.WriteRequest) (*pbre
 			// just want to update the current resource.
 			input.Id = existing.Id
 
-			// User is doing a non-CAS write, use the current version.
+			// User is doing a non-CAS write, use the current version and preserve
+			// deferred deletion metadata if not present.
 			if input.Version == "" {
 				input.Version = existing.Version
+				preserveDeferredDeletionMetadata(input, existing)
 			}
 
 			// Check the stored version matches the user-given version.
@@ -404,7 +406,7 @@ func vetIfDeleteRelated(input, existing *pbresource.Resource, tenancyMarkedForDe
 		errMetadataSame := ensureMetadataSameExceptFor(input, existing, resource.DeletionTimestampKey)
 		errDataUnchanged := ensureDataUnchanged(input, existing)
 		if errMetadataSame == nil && errDataUnchanged == nil {
-			return status.Error(codes.InvalidArgument, "no-op write of resource marked for deletion not allowed")
+			return status.Error(codes.InvalidArgument, "cannot no-op write resource marked for deletion")
 		}
 	}
 
@@ -437,3 +439,33 @@ func vetIfDeleteRelated(input, existing *pbresource.Resource, tenancyMarkedForDe
 
 	return nil
 }
+
+// preserveDeferredDeletionMetadata only applies to user writes (Version == "") which is a precondition.
+func preserveDeferredDeletionMetadata(input, existing *pbresource.Resource) {
+	// preserve existing deletionTimestamp if not provided in input
+	if !resource.IsMarkedForDeletion(input) && resource.IsMarkedForDeletion(existing) {
+		if input.Metadata == nil {
+			input.Metadata = make(map[string]string)
+		}
+		input.Metadata[resource.DeletionTimestampKey] = existing.Metadata[resource.DeletionTimestampKey]
+	}
+
+	// Only preserve finalizers if the is key absent from input and present in existing.
+	// If the key is present in input, the user clearly wants to remove finalizers!
+	inputHasKey := false
+	if input.Metadata != nil {
+		_, inputHasKey = input.Metadata[resource.FinalizerKey]
+	}
+
+	existingHasKey := false
+	if existing.Metadata != nil {
+		_, existingHasKey = existing.Metadata[resource.FinalizerKey]
+	}
+
+	if !inputHasKey && existingHasKey {
+		if input.Metadata == nil {
+			input.Metadata = make(map[string]string)
+		}
+		input.Metadata[resource.FinalizerKey] = existing.Metadata[resource.FinalizerKey]
+	}
+}

agent/grpc-external/services/resource/write_test.go

@@ -1011,7 +1011,7 @@ func TestWrite_ResourceFrozenAfterMarkedForDeletion(t *testing.T) {
 	testCases := map[string]testCase{
 		"no-op write rejected": {
 			modFn:       func(res *pbresource.Resource) {},
-			errContains: "no-op write of resource marked for deletion not allowed",
+			errContains: "cannot no-op write resource marked for deletion",
 		},
 		"remove one finalizer": {
 			modFn: func(res *pbresource.Resource) {
@@ -1088,3 +1088,147 @@ func TestWrite_ResourceFrozenAfterMarkedForDeletion(t *testing.T) {
 		})
 	}
 }
+
+func TestWrite_NonCASWritePreservesFinalizers(t *testing.T) {
+	type testCase struct {
+		existingMeta map[string]string
+		inputMeta    map[string]string
+		expectedMeta map[string]string
+	}
+	testCases := map[string]testCase{
+		"input nil metadata preserves existing finalizers": {
+			inputMeta:    nil,
+			existingMeta: map[string]string{resource.FinalizerKey: "finalizer1 finalizer2"},
+			expectedMeta: map[string]string{resource.FinalizerKey: "finalizer1 finalizer2"},
+		},
+		"input metadata and no finalizer key preserves existing finalizers": {
+			inputMeta:    map[string]string{},
+			existingMeta: map[string]string{resource.FinalizerKey: "finalizer1 finalizer2"},
+			expectedMeta: map[string]string{resource.FinalizerKey: "finalizer1 finalizer2"},
+		},
+		"input metadata and with empty finalizer key overwrites existing finalizers": {
+			inputMeta:    map[string]string{resource.FinalizerKey: ""},
+			existingMeta: map[string]string{resource.FinalizerKey: "finalizer1 finalizer2"},
+			expectedMeta: map[string]string{resource.FinalizerKey: ""},
+		},
+		"input metadata with one finalizer key overwrites multiple existing finalizers": {
+			inputMeta:    map[string]string{resource.FinalizerKey: "finalizer2"},
+			existingMeta: map[string]string{resource.FinalizerKey: "finalizer1 finalizer2"},
+			expectedMeta: map[string]string{resource.FinalizerKey: "finalizer2"},
+		},
+	}
+
+	for desc, tc := range testCases {
+		t.Run(desc, func(t *testing.T) {
+			server, client, ctx := testDeps(t)
+			demo.RegisterTypes(server.Registry)
+
+			// Create the resource based on tc.existingMetadata
+			builder := rtest.Resource(demo.TypeV1Artist, "joydivision").
+				WithTenancy(resource.DefaultNamespacedTenancy()).
+				WithData(t, &pbdemo.Artist{Name: "Joy"})
+
+			if tc.existingMeta != nil {
+				for k, v := range tc.existingMeta {
+					builder.WithMeta(k, v)
+				}
+			}
+			res := builder.Write(t, client)
+
+			// Build resource for user write based on tc.inputMetadata
+			builder = rtest.Resource(demo.TypeV1Artist, res.Id.Name).
+				WithTenancy(resource.DefaultNamespacedTenancy()).
+				WithData(t, &pbdemo.Artist{Name: "Joy Division"})
+
+			if tc.inputMeta != nil {
+				for k, v := range tc.inputMeta {
+					builder.WithMeta(k, v)
+				}
+			}
+			userRes := builder.Build()
+
+			// Perform the user write
+			rsp, err := client.Write(ctx, &pbresource.WriteRequest{Resource: userRes})
+			require.NoError(t, err)
+
+			// Verify write result preserved metadata based on testcase.expecteMetadata
+			for k := range tc.expectedMeta {
+				require.Equal(t, tc.expectedMeta[k], rsp.Resource.Metadata[k])
+			}
+			require.Equal(t, len(tc.expectedMeta), len(rsp.Resource.Metadata))
+		})
+	}
+}
+
+func TestWrite_NonCASWritePreservesDeletionTimestamp(t *testing.T) {
+	type testCase struct {
+		existingMeta map[string]string
+		inputMeta    map[string]string
+		expectedMeta map[string]string
+	}
+
+	// deletionTimestamp has to be generated via Delete() call and can't be embedded in testdata
+	// even though testcase desc refers to it.
+	testCases := map[string]testCase{
+		"input metadata no deletion timestamp preserves existing deletion timestamp and removes single finalizer": {
+			inputMeta:    map[string]string{resource.FinalizerKey: "finalizer1"},
+			existingMeta: map[string]string{resource.FinalizerKey: "finalizer1 finalizer2"},
+			expectedMeta: map[string]string{resource.FinalizerKey: "finalizer1"},
+		},
+		"input metadata no deletion timestamp preserves existing deletion timestamp and removes all finalizers": {
+			inputMeta:    map[string]string{resource.FinalizerKey: ""},
+			existingMeta: map[string]string{resource.FinalizerKey: "finalizer1 finalizer2"},
+			expectedMeta: map[string]string{resource.FinalizerKey: ""},
+		},
+	}
+
+	for desc, tc := range testCases {
+		t.Run(desc, func(t *testing.T) {
+			server, client, ctx := testDeps(t)
+			demo.RegisterTypes(server.Registry)
+
+			// Create the resource based on tc.existingMetadata
+			builder := rtest.Resource(demo.TypeV1Artist, "joydivision").
+				WithTenancy(resource.DefaultNamespacedTenancy()).
+				WithData(t, &pbdemo.Artist{Name: "Joy Division"})
+
+			if tc.existingMeta != nil {
+				for k, v := range tc.existingMeta {
+					builder.WithMeta(k, v)
+				}
+			}
+			res := builder.Write(t, client)
+
+			// Mark for deletion
+			_, err := client.Delete(ctx, &pbresource.DeleteRequest{Id: res.Id})
+			require.NoError(t, err)
+
+			// Re-read the deleted res for future comparison of deletionTimestamp
+			delRsp, err := client.Read(ctx, &pbresource.ReadRequest{Id: res.Id})
+			require.NoError(t, err)
+
+			// Build resource for user write based on tc.inputMetadata
+			builder = rtest.Resource(demo.TypeV1Artist, res.Id.Name).
+				WithTenancy(resource.DefaultNamespacedTenancy()).
+				WithData(t, &pbdemo.Artist{Name: "Joy Division"})
+
+			if tc.inputMeta != nil {
+				for k, v := range tc.inputMeta {
+					builder.WithMeta(k, v)
+				}
+			}
+			userRes := builder.Build()
+
+			// Perform the non-CAS user write
+			rsp, err := client.Write(ctx, &pbresource.WriteRequest{Resource: userRes})
+			require.NoError(t, err)
+
+			// Verify write result preserved metadata based on testcase.expecteMetadata
+			for k := range tc.expectedMeta {
+				require.Equal(t, tc.expectedMeta[k], rsp.Resource.Metadata[k])
+			}
+			// Verify deletion timestamp preserved even though it wasn't passed in to the write
+			require.Equal(t, delRsp.Resource.Metadata[resource.DeletionTimestampKey], rsp.Resource.Metadata[resource.DeletionTimestampKey])
+		})
+	}
+}

internal/resource/resource.go

@@ -67,10 +67,21 @@ func AddFinalizer(res *pbresource.Resource, finalizer string) {
 func RemoveFinalizer(res *pbresource.Resource, finalizer string) {
 	finalizerSet := GetFinalizers(res)
 	finalizerSet.Remove(finalizer)
+
+	if finalizerSet.Cardinality() == 0 {
+		// Remove key if no finalizers to prevent dual representations of
+		// the same state.
+		_, keyExists := res.Metadata[FinalizerKey]
+		if keyExists {
+			delete(res.Metadata, FinalizerKey)
+		}
+	} else {
+		// Add/update key
 		if res.Metadata == nil {
 			res.Metadata = map[string]string{}
 		}
 		res.Metadata[FinalizerKey] = strings.Join(finalizerSet.ToSlice(), " ")
+	}
 }
 
 // GetFinalizers returns the set of finalizers for the given resource.

proto-public/pbresource/resource.pb.go

@@ -399,6 +399,7 @@ type Resource struct {
 	// can treat its timestamp component as the resource's modification time.
 	Generation string `protobuf:"bytes,4,opt,name=generation,proto3" json:"generation,omitempty"`
 	// Metadata contains key/value pairs of arbitrary metadata about the resource.
+	// "deletionTimestamp" and "finalizers" keys are reserved for internal use.
 	Metadata map[string]string `protobuf:"bytes,5,rep,name=metadata,proto3" json:"metadata,omitempty" protobuf_key:"bytes,1,opt,name=key,proto3" protobuf_val:"bytes,2,opt,name=value,proto3"`
 	// Status is used by controllers to communicate the result of attempting to
 	// reconcile and apply the resource (e.g. surface semantic validation errors)

proto-public/pbresource/resource.proto

@@ -99,6 +99,7 @@ message Resource {
   string generation = 4;
 
   // Metadata contains key/value pairs of arbitrary metadata about the resource.
+  // "deletionTimestamp" and "finalizers" keys are reserved for internal use.
   map<string, string> metadata = 5;
 
   // Status is used by controllers to communicate the result of attempting to