diff --git a/agent/grpc-external/services/resource/delete_test.go b/agent/grpc-external/services/resource/delete_test.go index 3b8a8099021d..5f5d7d7e2192 100644 --- a/agent/grpc-external/services/resource/delete_test.go +++ b/agent/grpc-external/services/resource/delete_test.go @@ -16,7 +16,6 @@ import ( "github.com/hashicorp/consul/acl/resolver" "github.com/hashicorp/consul/internal/resource" "github.com/hashicorp/consul/internal/resource/demo" - "github.com/hashicorp/consul/internal/storage" "github.com/hashicorp/consul/proto-public/pbresource" ) @@ -34,10 +33,6 @@ func TestDelete_InputValidation(t *testing.T) { artistId.Type = nil return artistId }, - "no tenancy": func(artistId, _ *pbresource.ID) *pbresource.ID { - artistId.Tenancy = nil - return artistId - }, "no name": func(artistId, _ *pbresource.ID) *pbresource.ID { artistId.Name = "" return artistId @@ -102,21 +97,23 @@ func TestDelete_ACLs(t *testing.T) { t.Run(desc, func(t *testing.T) { server := testServer(t) client := testClient(t, server) - - mockACLResolver := &MockACLResolver{} - mockACLResolver.On("ResolveTokenAndDefaultMeta", mock.Anything, mock.Anything, mock.Anything). - Return(tc.authz, nil) - server.ACLResolver = mockACLResolver demo.RegisterTypes(server.Registry) artist, err := demo.GenerateV2Artist() require.NoError(t, err) - artist, err = server.Backend.WriteCAS(context.Background(), artist) + // Write test resource to delete. + rsp, err := client.Write(context.Background(), &pbresource.WriteRequest{Resource: artist}) require.NoError(t, err) - // exercise ACL - _, err = client.Delete(testContext(t), &pbresource.DeleteRequest{Id: artist.Id}) + // Mock is put in place after the above "write" since the "write" must also pass the ACL check. + mockACLResolver := &MockACLResolver{} + mockACLResolver.On("ResolveTokenAndDefaultMeta", mock.Anything, mock.Anything, mock.Anything). + Return(tc.authz, nil) + server.ACLResolver = mockACLResolver + + // Exercise ACL. + _, err = client.Delete(testContext(t), &pbresource.DeleteRequest{Id: rsp.Resource.Id}) tc.assertErrFn(err) }) } @@ -134,15 +131,20 @@ func TestDelete_Success(t *testing.T) { recordLabel, err := demo.GenerateV1RecordLabel("LoonyTunes") require.NoError(t, err) - recordLabel, err = server.Backend.WriteCAS(ctx, recordLabel) + writeRsp, err := client.Write(ctx, &pbresource.WriteRequest{Resource: recordLabel}) require.NoError(t, err) + recordLabel = writeRsp.Resource + originalRecordLabelId := clone(recordLabel.Id) artist, err := demo.GenerateV2Artist() require.NoError(t, err) - artist, err = server.Backend.WriteCAS(ctx, artist) + writeRsp, err = client.Write(ctx, &pbresource.WriteRequest{Resource: artist}) require.NoError(t, err) + artist = writeRsp.Resource + originalArtistId := clone(artist.Id) - // Pick the resource to be deleted based on type's scope + // Pick the resource to be deleted based on type's scope and mod tenancy + // based on the tenancy test case. deleteId := modFn(artist.Id, recordLabel.Id) deleteReq := tc.deleteReqFn(recordLabel) if proto.Equal(deleteId.Type, demo.TypeV2Artist) { @@ -154,19 +156,25 @@ func TestDelete_Success(t *testing.T) { require.NoError(t, err) // Verify deleted - _, err = server.Backend.Read(ctx, storage.StrongConsistency, deleteId) + _, err = client.Read(ctx, &pbresource.ReadRequest{Id: deleteId}) require.Error(t, err) - require.ErrorIs(t, err, storage.ErrNotFound) + require.Equal(t, codes.NotFound.String(), status.Code(err).String()) + + // Derive tombstone name from resource that was deleted. + tname := tombstoneName(originalRecordLabelId) + if proto.Equal(deleteId.Type, demo.TypeV2Artist) { + tname = tombstoneName(originalArtistId) + } // Verify tombstone created _, err = client.Read(ctx, &pbresource.ReadRequest{ Id: &pbresource.ID{ - Name: tombstoneName(deleteReq.Id), + Name: tname, Type: resource.TypeV1Tombstone, Tenancy: deleteReq.Id.Tenancy, }, }) - require.NoError(t, err, "expected tombstome to be found") + require.NoError(t, err, "expected tombstone to be found") }) } }) diff --git a/agent/grpc-external/services/resource/list_by_owner_test.go b/agent/grpc-external/services/resource/list_by_owner_test.go index 4a60e3ac9460..11c6027c0b64 100644 --- a/agent/grpc-external/services/resource/list_by_owner_test.go +++ b/agent/grpc-external/services/resource/list_by_owner_test.go @@ -34,10 +34,6 @@ func TestListByOwner_InputValidation(t *testing.T) { artistId.Type = nil return artistId }, - "no tenancy": func(artistId, _ *pbresource.ID) *pbresource.ID { - artistId.Tenancy = nil - return artistId - }, "no name": func(artistId, _ *pbresource.ID) *pbresource.ID { artistId.Name = "" return artistId diff --git a/agent/grpc-external/services/resource/read_test.go b/agent/grpc-external/services/resource/read_test.go index ff312c82ca00..ff027fa726de 100644 --- a/agent/grpc-external/services/resource/read_test.go +++ b/agent/grpc-external/services/resource/read_test.go @@ -33,10 +33,6 @@ func TestRead_InputValidation(t *testing.T) { artistId.Type = nil return artistId }, - "no tenancy": func(artistId, _ *pbresource.ID) *pbresource.ID { - artistId.Tenancy = nil - return artistId - }, "no name": func(artistId, _ *pbresource.ID) *pbresource.ID { artistId.Name = "" return artistId diff --git a/agent/grpc-external/services/resource/server.go b/agent/grpc-external/services/resource/server.go index 29b18bd964c8..52d993ad4991 100644 --- a/agent/grpc-external/services/resource/server.go +++ b/agent/grpc-external/services/resource/server.go @@ -133,8 +133,6 @@ func validateId(id *pbresource.ID, errorPrefix string) error { switch { case id.Type == nil: field = "type" - case id.Tenancy == nil: - field = "tenancy" case id.Name == "": field = "name" } @@ -142,6 +140,18 @@ func validateId(id *pbresource.ID, errorPrefix string) error { if field != "" { return status.Errorf(codes.InvalidArgument, "%s.%s is required", errorPrefix, field) } + + // Better UX: Allow callers to pass in nil tenancy. Defaulting and inheritance of tenancy + // from the request token will take place further down in the call flow. + if id.Tenancy == nil { + id.Tenancy = &pbresource.Tenancy{ + Partition: "", + Namespace: "", + // TODO(spatel): Remove when peerTenancy introduced. + PeerName: "local", + } + } + resource.Normalize(id.Tenancy) return nil diff --git a/agent/grpc-external/services/resource/server_test.go b/agent/grpc-external/services/resource/server_test.go index 7f745952e5e5..8f3967259a51 100644 --- a/agent/grpc-external/services/resource/server_test.go +++ b/agent/grpc-external/services/resource/server_test.go @@ -246,6 +246,11 @@ func tenancyCases() map[string]func(artistId, recordlabelId *pbresource.ID) *pbr id.Tenancy.Namespace = "" return id }, + "namespaced resource inherits tokens partition and namespace when tenacy nil": func(artistId, _ *pbresource.ID) *pbresource.ID { + id := clone(artistId) + id.Tenancy = nil + return id + }, "partitioned resource provides nonempty partition": func(_, recordLabelId *pbresource.ID) *pbresource.ID { return recordLabelId }, @@ -259,6 +264,11 @@ func tenancyCases() map[string]func(artistId, recordlabelId *pbresource.ID) *pbr id.Tenancy.Partition = "" return id }, + "partitioned resource inherits tokens partition when tenancy nil": func(_, recordLabelId *pbresource.ID) *pbresource.ID { + id := clone(recordLabelId) + id.Tenancy = nil + return id + }, } return tenancyCases } diff --git a/agent/grpc-external/services/resource/write_status.go b/agent/grpc-external/services/resource/write_status.go index 263814237eb3..0c0395a267ba 100644 --- a/agent/grpc-external/services/resource/write_status.go +++ b/agent/grpc-external/services/resource/write_status.go @@ -120,8 +120,6 @@ func (s *Server) validateWriteStatusRequest(req *pbresource.WriteStatusRequest) field = "id" case req.Id.Type == nil: field = "id.type" - case req.Id.Tenancy == nil: - field = "id.tenancy" case req.Id.Name == "": field = "id.name" case req.Id.Uid == "": @@ -169,6 +167,17 @@ func (s *Server) validateWriteStatusRequest(req *pbresource.WriteStatusRequest) return nil, status.Error(codes.InvalidArgument, "status.observed_generation is not valid") } + // Better UX: Allow callers to pass in nil tenancy. Defaulting and inheritance of tenancy + // from the request token will take place further down in the call flow. + if req.Id.Tenancy == nil { + req.Id.Tenancy = &pbresource.Tenancy{ + Partition: "", + Namespace: "", + // TODO(spatel): Remove when peerTenancy introduced. + PeerName: "local", + } + } + // Lowercase resource.Normalize(req.Id.Tenancy) diff --git a/agent/grpc-external/services/resource/write_status_test.go b/agent/grpc-external/services/resource/write_status_test.go index 622cbcccc746..5b71983475d9 100644 --- a/agent/grpc-external/services/resource/write_status_test.go +++ b/agent/grpc-external/services/resource/write_status_test.go @@ -85,10 +85,6 @@ func TestWriteStatus_InputValidation(t *testing.T) { typ: demo.TypeV2Artist, modFn: func(req *pbresource.WriteStatusRequest) { req.Id.Type = nil }, }, - "no tenancy": { - typ: demo.TypeV2Artist, - modFn: func(req *pbresource.WriteStatusRequest) { req.Id.Tenancy = nil }, - }, "no name": { typ: demo.TypeV2Artist, modFn: func(req *pbresource.WriteStatusRequest) { req.Id.Name = "" }, @@ -236,6 +232,10 @@ func TestWriteStatus_Tenancy_Defaults(t *testing.T) { req.Id.Tenancy.Namespace = "" }, }, + "namespaced resource inherits tokens partition and namespace when tenancy nil": { + scope: resource.ScopeNamespace, + modFn: func(req *pbresource.WriteStatusRequest) { req.Id.Tenancy = nil }, + }, "partitioned resource provides nonempty partition": { scope: resource.ScopePartition, modFn: func(req *pbresource.WriteStatusRequest) {}, diff --git a/agent/grpc-external/services/resource/write_test.go b/agent/grpc-external/services/resource/write_test.go index d472a6ec7414..755f8ef2ccbc 100644 --- a/agent/grpc-external/services/resource/write_test.go +++ b/agent/grpc-external/services/resource/write_test.go @@ -42,10 +42,6 @@ func TestWrite_InputValidation(t *testing.T) { artist.Id.Type = nil return artist }, - "no tenancy": func(artist, _ *pbresource.Resource) *pbresource.Resource { - artist.Id.Tenancy = nil - return artist - }, "no name": func(artist, _ *pbresource.Resource) *pbresource.Resource { artist.Id.Name = "" return artist @@ -106,7 +102,7 @@ func TestWrite_OwnerValidation(t *testing.T) { }, "no owner tenancy": { modReqFn: func(req *pbresource.WriteRequest) { req.Resource.Owner.Tenancy = nil }, - errorContains: "resource.owner.tenancy", + errorContains: "resource.owner", }, "no owner name": { modReqFn: func(req *pbresource.WriteRequest) { req.Resource.Owner.Name = "" }, @@ -253,6 +249,13 @@ func TestWrite_Create_Success(t *testing.T) { }, expectedTenancy: resource.DefaultNamespacedTenancy(), }, + "namespaced resource inherits tokens partition and namespace when tenancy nil": { + modFn: func(artist, _ *pbresource.Resource) *pbresource.Resource { + artist.Id.Tenancy = nil + return artist + }, + expectedTenancy: resource.DefaultNamespacedTenancy(), + }, "partitioned resource provides nonempty partition": { modFn: func(_, recordLabel *pbresource.Resource) *pbresource.Resource { return recordLabel @@ -273,6 +276,13 @@ func TestWrite_Create_Success(t *testing.T) { }, expectedTenancy: resource.DefaultPartitionedTenancy(), }, + "partitioned resource inherits tokens partition when tenancy nil": { + modFn: func(_, recordLabel *pbresource.Resource) *pbresource.Resource { + recordLabel.Id.Tenancy = nil + return recordLabel + }, + expectedTenancy: resource.DefaultPartitionedTenancy(), + }, // TODO(spatel): Add cluster scope tests when we have an actual cluster scoped resource (e.g. partition) } for desc, tc := range testCases {