resource: Allow nil tenancy (#18618)

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
+			// Mock is put in place after the above "write" since the "write" must also pass the ACL check.
-			_, err = client.Delete(testContext(t), &pbresource.DeleteRequest{Id: artist.Id})
+			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")
 				})
 			}
 		})

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

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

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

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
 }

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)
 

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) {},

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 {