resource: freeze resources after marked for deletion (4 of 5) (#19603)

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

@@ -81,8 +81,11 @@ func (s *Server) Delete(ctx context.Context, req *pbresource.DeleteRequest) (*pb
 			return &pbresource.DeleteResponse{}, nil
 		}
 
-		// Mark for deletion and let controllers that put finalizers in place do their thing
+		// Mark for deletion and let controllers that put finalizers in place do their
-		return s.markForDeletion(ctx, existing)
+		// thing. Note we're passing in a clone of the recently read resource since
+		// we've not crossed a network/serialization boundary since the read and we
+		// don't want to mutate the in-mem reference.
+		return s.markForDeletion(ctx, clone(existing))
 	}
 
 	// Continue with an immediate delete
@@ -102,12 +105,8 @@ func (s *Server) Delete(ctx context.Context, req *pbresource.DeleteRequest) (*pb
 }
 
 func (s *Server) markForDeletion(ctx context.Context, res *pbresource.Resource) (*pbresource.DeleteResponse, error) {
-	if res.Metadata == nil {
-		res.Metadata = map[string]string{}
-	}
-	res.Metadata[resource.DeletionTimestampKey] = time.Now().Format(time.RFC3339)
-
 	// Write the deletion timestamp
+	res.Metadata[resource.DeletionTimestampKey] = time.Now().Format(time.RFC3339)
 	_, err := s.Write(ctx, &pbresource.WriteRequest{Resource: res})
 	if err != nil {
 		return nil, err

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

@@ -7,13 +7,14 @@ import (
 	"context"
 	"strings"
 
-	"github.com/hashicorp/go-hclog"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/metadata"
 	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/proto"
 
+	"github.com/hashicorp/go-hclog"
+
 	"github.com/hashicorp/consul/acl"
 	"github.com/hashicorp/consul/acl/resolver"
 	"github.com/hashicorp/consul/internal/resource"
@@ -272,28 +273,27 @@ func validateScopedTenancy(scope resource.Scope, resourceType *pbresource.Type,
 	return nil
 }
 
-// tenancyMarkedForDeletion returns a gRPC InvalidArgument when either partition or namespace is marked for deletion.
+func isTenancyMarkedForDeletion(reg *resource.Registration, tenancyBridge TenancyBridge, tenancy *pbresource.Tenancy) (bool, error) {
-func tenancyMarkedForDeletion(reg *resource.Registration, tenancyBridge TenancyBridge, tenancy *pbresource.Tenancy) error {
 	if reg.Scope == resource.ScopePartition || reg.Scope == resource.ScopeNamespace {
 		marked, err := tenancyBridge.IsPartitionMarkedForDeletion(tenancy.Partition)
-		switch {
+		if err != nil {
-		case err != nil:
+			return false, err
-			return err
+		}
-		case marked:
+		if marked {
-			return status.Errorf(codes.InvalidArgument, "partition marked for deletion: %v", tenancy.Partition)
+			return marked, nil
 		}
 	}
 
 	if reg.Scope == resource.ScopeNamespace {
 		marked, err := tenancyBridge.IsNamespaceMarkedForDeletion(tenancy.Partition, tenancy.Namespace)
-		switch {
+		if err != nil {
-		case err != nil:
+			return false, err
-			return err
-		case marked:
-			return status.Errorf(codes.InvalidArgument, "namespace marked for deletion: %v", tenancy.Namespace)
 		}
+		return marked, nil
 	}
-	return nil
+
+	// Cluster scope has no tenancy so always return false
+	return false, nil
 }
 
 func clone[T proto.Message](v T) T { return proto.Clone(v).(T) }

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

@@ -10,8 +10,10 @@ import (
 	"time"
 
 	"github.com/oklog/ulid/v2"
+	"golang.org/x/exp/maps"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
+	"google.golang.org/protobuf/proto"
 
 	"github.com/hashicorp/consul/acl"
 	"github.com/hashicorp/consul/internal/resource"
@@ -83,9 +85,11 @@ func (s *Server) Write(ctx context.Context, req *pbresource.WriteRequest) (*pbre
 		return nil, err
 	}
 
-	// Check tenancy not marked for deletion.
+	// This is used later in the "create" and "update" paths to block non-delete related writes
-	if err = tenancyMarkedForDeletion(reg, s.TenancyBridge, req.Resource.Id.Tenancy); err != nil {
+	// when a tenancy unit has been marked for deletion.
-		return nil, err
+	tenancyMarkedForDeletion, err := isTenancyMarkedForDeletion(reg, s.TenancyBridge, req.Resource.Id.Tenancy)
+	if err != nil {
+		return nil, status.Errorf(codes.Internal, "failed tenancy marked for deletion check: %v", err)
 	}
 
 	// At the storage backend layer, all writes are CAS operations.
@@ -127,6 +131,16 @@ func (s *Server) Write(ctx context.Context, req *pbresource.WriteRequest) (*pbre
 				return errUseWriteStatus
 			}
 
+			// Reject creation in tenancy unit marked for deletion.
+			if tenancyMarkedForDeletion {
+				return status.Errorf(codes.InvalidArgument, "tenancy marked for deletion: %v", input.Id.Tenancy.String())
+			}
+
+			// Reject attempts to create a resource with a deletionTimestamp.
+			if resource.IsMarkedForDeletion(input) {
+				return status.Errorf(codes.InvalidArgument, "resource.metadata.%s can't be set on resource creation", resource.DeletionTimestampKey)
+			}
+
 			// Generally, we expect resources with owners to be created by controllers,
 			// and they should provide the Uid. In cases where no Uid is given (e.g. the
 			// owner is specified in the resource HCL) we'll look up whatever the current
@@ -208,6 +222,13 @@ func (s *Server) Write(ctx context.Context, req *pbresource.WriteRequest) (*pbre
 				return errUseWriteStatus
 			}
 
+			// If the write is related to a deferred deletion (marking for deletion or removal
+			// of finalizers), make sure nothing else is changed.
+			if err := vetIfDeleteRelated(input, existing, tenancyMarkedForDeletion); err != nil {
+				return err
+			}
+
+			// Otherwise, let the write continue
 		default:
 			return err
 		}
@@ -310,3 +331,109 @@ func (s *Server) ensureWriteRequestValid(req *pbresource.WriteRequest) (*resourc
 
 	return reg, nil
 }
+
+func ensureMetadataSameExceptFor(input *pbresource.Resource, existing *pbresource.Resource, ignoreKey string) error {
+	// Work on copies since we're mutating them
+	inputCopy := maps.Clone(input.Metadata)
+	existingCopy := maps.Clone(existing.Metadata)
+
+	delete(inputCopy, ignoreKey)
+	delete(existingCopy, ignoreKey)
+
+	if !maps.Equal(inputCopy, existingCopy) {
+		return status.Error(codes.InvalidArgument, "cannot modify metadata")
+	}
+
+	return nil
+}
+
+func ensureDataUnchanged(input *pbresource.Resource, existing *pbresource.Resource) error {
+	// Check data last since this could potentially be the most expensive comparison.
+	if !proto.Equal(input.Data, existing.Data) {
+		return status.Error(codes.InvalidArgument, "cannot modify data")
+	}
+	return nil
+}
+
+// ensureFinalizerRemoved ensures at least one finalizer was removed.
+func ensureFinalizerRemoved(input *pbresource.Resource, existing *pbresource.Resource) error {
+	inputFinalizers := resource.GetFinalizers(input)
+	existingFinalizers := resource.GetFinalizers(existing)
+	if !inputFinalizers.IsProperSubset(existingFinalizers) {
+		return status.Error(codes.InvalidArgument, "expected at least one finalizer to be removed")
+	}
+	return nil
+}
+
+func vetIfDeleteRelated(input, existing *pbresource.Resource, tenancyMarkedForDeletion bool) error {
+	// Keep track of whether this write is a normal write or a write that is related
+	// to deferred resource deletion involving setting the deletionTimestamp or the
+	// removal of finalizers.
+	deleteRelated := false
+
+	existingMarked := resource.IsMarkedForDeletion(existing)
+	inputMarked := resource.IsMarkedForDeletion(input)
+
+	// Block removal of deletion timestamp
+	if !inputMarked && existingMarked {
+		return status.Errorf(codes.InvalidArgument, "cannot remove %s", resource.DeletionTimestampKey)
+	}
+
+	// Block modification of existing deletion timestamp
+	if existing.Metadata[resource.DeletionTimestampKey] != "" && (existing.Metadata[resource.DeletionTimestampKey] != input.Metadata[resource.DeletionTimestampKey]) {
+		return status.Errorf(codes.InvalidArgument, "cannot modify %s", resource.DeletionTimestampKey)
+	}
+
+	// Block writes that do more than just adding a deletion timestamp
+	if inputMarked && !existingMarked {
+		deleteRelated = deleteRelated || true
+		// Verify rest of resource is unchanged
+		if err := ensureMetadataSameExceptFor(input, existing, resource.DeletionTimestampKey); err != nil {
+			return err
+		}
+		if err := ensureDataUnchanged(input, existing); err != nil {
+			return err
+		}
+	}
+
+	// Block no-op writes writes to resource that already has a deletion timestamp. The
+	// only valid writes should be removal of finalizers.
+	if inputMarked && existingMarked {
+		deleteRelated = deleteRelated || true
+		// Check if a no-op
+		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")
+		}
+	}
+
+	// Block writes that do more than removing finalizers if previously marked for deletion.
+	if inputMarked && existingMarked && resource.HasFinalizers(existing) {
+		deleteRelated = deleteRelated || true
+		if err := ensureMetadataSameExceptFor(input, existing, resource.FinalizerKey); err != nil {
+			return err
+		}
+		if err := ensureDataUnchanged(input, existing); err != nil {
+			return err
+		}
+		if err := ensureFinalizerRemoved(input, existing); err != nil {
+			return err
+		}
+	}
+
+	// Classify writes that just remove finalizer as deleteRelated regardless of deletion state.
+	if err := ensureFinalizerRemoved(input, existing); err == nil {
+		if err := ensureDataUnchanged(input, existing); err == nil {
+			deleteRelated = deleteRelated || true
+		}
+	}
+
+	// Lastly, block writes when the resource's tenancy unit has been marked for deletion and
+	// the write is not related a valid delete scenario.
+	if tenancyMarkedForDeletion && !deleteRelated {
+		return status.Errorf(codes.InvalidArgument, "cannot write resource when tenancy marked for deletion: %s", existing.Id.Tenancy)
+	}
+
+	return nil
+}

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

@@ -8,6 +8,7 @@ import (
 	"strings"
 	"sync/atomic"
 	"testing"
+	"time"
 
 	"github.com/oklog/ulid/v2"
 	"github.com/stretchr/testify/mock"
@@ -19,8 +20,10 @@ import (
 	"github.com/hashicorp/consul/acl/resolver"
 	"github.com/hashicorp/consul/internal/resource"
 	"github.com/hashicorp/consul/internal/resource/demo"
+	rtest "github.com/hashicorp/consul/internal/resource/resourcetest"
 	"github.com/hashicorp/consul/internal/storage"
 	"github.com/hashicorp/consul/proto-public/pbresource"
+	pbdemo "github.com/hashicorp/consul/proto/private/pbdemo/v1"
 	pbdemov1 "github.com/hashicorp/consul/proto/private/pbdemo/v1"
 	pbdemov2 "github.com/hashicorp/consul/proto/private/pbdemo/v2"
 	"github.com/hashicorp/consul/proto/private/prototest"
@@ -457,7 +460,24 @@ func TestWrite_Create_Tenancy_NotFound(t *testing.T) {
 	}
 }
 
-func TestWrite_Tenancy_MarkedForDeletion(t *testing.T) {
+func TestWrite_Create_With_DeletionTimestamp_Fails(t *testing.T) {
+	server := testServer(t)
+	client := testClient(t, server)
+	demo.RegisterTypes(server.Registry)
+
+	res := rtest.Resource(demo.TypeV1Artist, "blur").
+		WithTenancy(resource.DefaultNamespacedTenancy()).
+		WithData(t, &pbdemov1.Artist{Name: "Blur"}).
+		WithMeta(resource.DeletionTimestampKey, time.Now().Format(time.RFC3339)).
+		Build()
+
+	_, err := client.Write(testContext(t), &pbresource.WriteRequest{Resource: res})
+	require.Error(t, err)
+	require.Equal(t, codes.InvalidArgument.String(), status.Code(err).String())
+	require.Contains(t, err.Error(), resource.DeletionTimestampKey)
+}
+
+func TestWrite_Create_With_TenancyMarkedForDeletion_Fails(t *testing.T) {
 	// Verify resource write fails when its partition or namespace is marked for deletion.
 	testCases := map[string]struct {
 		modFn       func(artist, recordLabel *pbresource.Resource, mockTenancyBridge *MockTenancyBridge) *pbresource.Resource
@@ -468,7 +488,7 @@ func TestWrite_Tenancy_MarkedForDeletion(t *testing.T) {
 				mockTenancyBridge.On("IsPartitionMarkedForDeletion", "ap1").Return(true, nil)
 				return artist
 			},
-			errContains: "partition marked for deletion",
+			errContains: "tenancy marked for deletion",
 		},
 		"namespaced resources namespace marked for deletion": {
 			modFn: func(artist, _ *pbresource.Resource, mockTenancyBridge *MockTenancyBridge) *pbresource.Resource {
@@ -476,14 +496,14 @@ func TestWrite_Tenancy_MarkedForDeletion(t *testing.T) {
 				mockTenancyBridge.On("IsNamespaceMarkedForDeletion", "ap1", "ns1").Return(true, nil)
 				return artist
 			},
-			errContains: "namespace marked for deletion",
+			errContains: "tenancy marked for deletion",
 		},
 		"partitioned resources partition marked for deletion": {
 			modFn: func(_, recordLabel *pbresource.Resource, mockTenancyBridge *MockTenancyBridge) *pbresource.Resource {
 				mockTenancyBridge.On("IsPartitionMarkedForDeletion", "ap1").Return(true, nil)
 				return recordLabel
 			},
-			errContains: "partition marked for deletion",
+			errContains: "tenancy marked for deletion",
 		},
 	}
 	for desc, tc := range testCases {
@@ -903,3 +923,168 @@ func (b *blockOnceBackend) Read(ctx context.Context, consistency storage.ReadCon
 
 	return res, err
 }
+
+func TestEnsureFinalizerRemoved(t *testing.T) {
+	type testCase struct {
+		mod         func(input, existing *pbresource.Resource)
+		errContains string
+	}
+
+	testCases := map[string]testCase{
+		"one finalizer removed from input": {
+			mod: func(input, existing *pbresource.Resource) {
+				resource.AddFinalizer(existing, "f1")
+				resource.AddFinalizer(existing, "f2")
+				resource.AddFinalizer(input, "f1")
+			},
+		},
+		"all finalizers removed from input": {
+			mod: func(input, existing *pbresource.Resource) {
+				resource.AddFinalizer(existing, "f1")
+				resource.AddFinalizer(existing, "f2")
+				resource.AddFinalizer(input, "f1")
+				resource.RemoveFinalizer(input, "f1")
+			},
+		},
+		"all finalizers removed from input and no finalizer key": {
+			mod: func(input, existing *pbresource.Resource) {
+				resource.AddFinalizer(existing, "f1")
+				resource.AddFinalizer(existing, "f2")
+			},
+		},
+		"no finalizers removed from input": {
+			mod: func(input, existing *pbresource.Resource) {
+				resource.AddFinalizer(existing, "f1")
+				resource.AddFinalizer(input, "f1")
+			},
+			errContains: "expected at least one finalizer to be removed",
+		},
+		"input finalizers not proper subset of existing": {
+			mod: func(input, existing *pbresource.Resource) {
+				resource.AddFinalizer(existing, "f1")
+				resource.AddFinalizer(existing, "f2")
+				resource.AddFinalizer(input, "f3")
+			},
+			errContains: "expected at least one finalizer to be removed",
+		},
+		"existing has no finalizers for input to remove": {
+			mod: func(input, existing *pbresource.Resource) {
+				resource.AddFinalizer(input, "f3")
+			},
+			errContains: "expected at least one finalizer to be removed",
+		},
+	}
+
+	for desc, tc := range testCases {
+		t.Run(desc, func(t *testing.T) {
+			input := rtest.Resource(demo.TypeV1Artist, "artist1").
+				WithTenancy(resource.DefaultNamespacedTenancy()).
+				WithData(t, &pbdemov1.Artist{Name: "artist1"}).
+				WithMeta(resource.DeletionTimestampKey, "someTimestamp").
+				Build()
+
+			existing := rtest.Resource(demo.TypeV1Artist, "artist1").
+				WithTenancy(resource.DefaultNamespacedTenancy()).
+				WithData(t, &pbdemov1.Artist{Name: "artist1"}).
+				WithMeta(resource.DeletionTimestampKey, "someTimestamp").
+				Build()
+
+			tc.mod(input, existing)
+
+			err := ensureFinalizerRemoved(input, existing)
+			if tc.errContains != "" {
+				require.Error(t, err)
+				require.Equal(t, codes.InvalidArgument.String(), status.Code(err).String())
+				require.ErrorContains(t, err, tc.errContains)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
+func TestWrite_ResourceFrozenAfterMarkedForDeletion(t *testing.T) {
+	type testCase struct {
+		modFn       func(res *pbresource.Resource)
+		errContains string
+	}
+	testCases := map[string]testCase{
+		"no-op write rejected": {
+			modFn:       func(res *pbresource.Resource) {},
+			errContains: "no-op write of resource marked for deletion not allowed",
+		},
+		"remove one finalizer": {
+			modFn: func(res *pbresource.Resource) {
+				resource.RemoveFinalizer(res, "finalizer1")
+			},
+		},
+		"remove all finalizers": {
+			modFn: func(res *pbresource.Resource) {
+				resource.RemoveFinalizer(res, "finalizer1")
+				resource.RemoveFinalizer(res, "finalizer2")
+			},
+		},
+		"adding finalizer fails": {
+			modFn: func(res *pbresource.Resource) {
+				resource.AddFinalizer(res, "finalizer3")
+			},
+			errContains: "expected at least one finalizer to be removed",
+		},
+		"remove deletionTimestamp fails": {
+			modFn: func(res *pbresource.Resource) {
+				delete(res.Metadata, resource.DeletionTimestampKey)
+			},
+			errContains: "cannot remove deletionTimestamp",
+		},
+		"modify deletionTimestamp fails": {
+			modFn: func(res *pbresource.Resource) {
+				res.Metadata[resource.DeletionTimestampKey] = "bad"
+			},
+			errContains: "cannot modify deletionTimestamp",
+		},
+		"modify data fails": {
+			modFn: func(res *pbresource.Resource) {
+				var err error
+				res.Data, err = anypb.New(&pbdemo.Artist{Name: "New Order"})
+				require.NoError(t, err)
+			},
+			errContains: "cannot modify data",
+		},
+	}
+
+	for desc, tc := range testCases {
+		t.Run(desc, func(t *testing.T) {
+			server, client, ctx := testDeps(t)
+			demo.RegisterTypes(server.Registry)
+
+			// Create a resource with finalizers
+			res := rtest.Resource(demo.TypeV1Artist, "joydivision").
+				WithTenancy(resource.DefaultNamespacedTenancy()).
+				WithData(t, &pbdemo.Artist{Name: "Joy Division"}).
+				WithMeta(resource.FinalizerKey, "finalizer1 finalizer2").
+				Write(t, client)
+
+			// Mark for deletion - resource should now be frozen
+			_, err := client.Delete(ctx, &pbresource.DeleteRequest{Id: res.Id})
+			require.NoError(t, err)
+
+			// Verify marked for deletion
+			rsp, err := client.Read(ctx, &pbresource.ReadRequest{Id: res.Id})
+			require.NoError(t, err)
+			require.True(t, resource.IsMarkedForDeletion(rsp.Resource))
+
+			// Apply test case mods
+			tc.modFn(rsp.Resource)
+
+			// Verify write results
+			_, err = client.Write(ctx, &pbresource.WriteRequest{Resource: rsp.Resource})
+			if tc.errContains == "" {
+				require.NoError(t, err)
+			} else {
+				require.Error(t, err)
+				require.Equal(t, codes.InvalidArgument.String(), status.Code(err).String())
+				require.ErrorContains(t, err, tc.errContains)
+			}
+		})
+	}
+}

internal/tenancy/internal/bridge/tenancy_bridge.go

@@ -6,6 +6,7 @@ package bridge
 import (
 	"context"
 
+	"github.com/hashicorp/consul/internal/resource"
 	"github.com/hashicorp/consul/proto-public/pbresource"
 	pbtenancy "github.com/hashicorp/consul/proto-public/pbtenancy/v2beta1"
 )
@@ -29,7 +30,7 @@ func NewV2TenancyBridge() *V2TenancyBridge {
 }
 
 func (b *V2TenancyBridge) NamespaceExists(partition, namespace string) (bool, error) {
-	read, err := b.client.Read(context.Background(), &pbresource.ReadRequest{
+	rsp, err := b.client.Read(context.Background(), &pbresource.ReadRequest{
 		Id: &pbresource.ID{
 			Name: namespace,
 			Tenancy: &pbresource.Tenancy{
@@ -38,11 +39,11 @@ func (b *V2TenancyBridge) NamespaceExists(partition, namespace string) (bool, er
 			Type: pbtenancy.NamespaceType,
 		},
 	})
-	return read != nil && read.Resource != nil, err
+	return rsp != nil && rsp.Resource != nil, err
 }
 
 func (b *V2TenancyBridge) IsNamespaceMarkedForDeletion(partition, namespace string) (bool, error) {
-	read, err := b.client.Read(context.Background(), &pbresource.ReadRequest{
+	rsp, err := b.client.Read(context.Background(), &pbresource.ReadRequest{
 		Id: &pbresource.ID{
 			Name: namespace,
 			Tenancy: &pbresource.Tenancy{
@@ -51,5 +52,8 @@ func (b *V2TenancyBridge) IsNamespaceMarkedForDeletion(partition, namespace stri
 			Type: pbtenancy.NamespaceType,
 		},
 	})
-	return read.Resource != nil, err
+	if err != nil {
+		return false, err
+	}
+	return resource.IsMarkedForDeletion(rsp.Resource), nil
 }