diff --git a/agent/grpc-external/services/resource/write.go b/agent/grpc-external/services/resource/write.go index 76e101f42a..b0dc27e2f0 100644 --- a/agent/grpc-external/services/resource/write.go +++ b/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] + } +} diff --git a/agent/grpc-external/services/resource/write_test.go b/agent/grpc-external/services/resource/write_test.go index f081e7c997..81392b8543 100644 --- a/agent/grpc-external/services/resource/write_test.go +++ b/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]) + }) + } +} diff --git a/internal/resource/resource.go b/internal/resource/resource.go index 2f3a7a05ca..6830f10931 100644 --- a/internal/resource/resource.go +++ b/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 res.Metadata == nil { - res.Metadata = map[string]string{} + + 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(), " ") } - res.Metadata[FinalizerKey] = strings.Join(finalizerSet.ToSlice(), " ") } // GetFinalizers returns the set of finalizers for the given resource. diff --git a/proto-public/pbresource/resource.pb.go b/proto-public/pbresource/resource.pb.go index 889a0314af..d5205ae1f1 100644 --- a/proto-public/pbresource/resource.pb.go +++ b/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) diff --git a/proto-public/pbresource/resource.proto b/proto-public/pbresource/resource.proto index 9e65647587..814e039a44 100644 --- a/proto-public/pbresource/resource.proto +++ b/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 metadata = 5; // Status is used by controllers to communicate the result of attempting to