diff --git a/agent/grpc-external/services/resource/delete.go b/agent/grpc-external/services/resource/delete.go index aef2934141..cbe3583c1c 100644 --- a/agent/grpc-external/services/resource/delete.go +++ b/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 - return s.markForDeletion(ctx, existing) + // Mark for deletion and let controllers that put finalizers in place do their + // 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 diff --git a/agent/grpc-external/services/resource/server.go b/agent/grpc-external/services/resource/server.go index e2615ec4b3..b07ebaff38 100644 --- a/agent/grpc-external/services/resource/server.go +++ b/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 tenancyMarkedForDeletion(reg *resource.Registration, tenancyBridge TenancyBridge, tenancy *pbresource.Tenancy) error { +func isTenancyMarkedForDeletion(reg *resource.Registration, tenancyBridge TenancyBridge, tenancy *pbresource.Tenancy) (bool, error) { if reg.Scope == resource.ScopePartition || reg.Scope == resource.ScopeNamespace { marked, err := tenancyBridge.IsPartitionMarkedForDeletion(tenancy.Partition) - switch { - case err != nil: - return err - case marked: - return status.Errorf(codes.InvalidArgument, "partition marked for deletion: %v", tenancy.Partition) + if err != nil { + return false, err + } + if marked { + return marked, nil } } if reg.Scope == resource.ScopeNamespace { marked, err := tenancyBridge.IsNamespaceMarkedForDeletion(tenancy.Partition, tenancy.Namespace) - switch { - case err != nil: - return err - case marked: - return status.Errorf(codes.InvalidArgument, "namespace marked for deletion: %v", tenancy.Namespace) + if err != nil { + return false, err } + 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) } diff --git a/agent/grpc-external/services/resource/write.go b/agent/grpc-external/services/resource/write.go index 63c87f11a1..76e101f42a 100644 --- a/agent/grpc-external/services/resource/write.go +++ b/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. - if err = tenancyMarkedForDeletion(reg, s.TenancyBridge, req.Resource.Id.Tenancy); err != nil { - return nil, err + // This is used later in the "create" and "update" paths to block non-delete related writes + // when a tenancy unit has been marked for deletion. + 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 +} diff --git a/agent/grpc-external/services/resource/write_test.go b/agent/grpc-external/services/resource/write_test.go index 9f7704b52b..f081e7c997 100644 --- a/agent/grpc-external/services/resource/write_test.go +++ b/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) + } + }) + } +} diff --git a/internal/tenancy/internal/bridge/tenancy_bridge.go b/internal/tenancy/internal/bridge/tenancy_bridge.go index db6a4dd53a..029db1d6e3 100644 --- a/internal/tenancy/internal/bridge/tenancy_bridge.go +++ b/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 }