resource: ListByOwner returns empty list on non-existent tenancy (#19742

)

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

@@ -145,7 +145,7 @@ func (s *Server) maybeCreateTombstone(ctx context.Context, deleteId *pbresource.
 		Id: &pbresource.ID{
 			Type:    resource.TypeV1Tombstone,
 			Tenancy: deleteId.Tenancy,
-			Name:    tombstoneName(deleteId),
+			Name:    TombstoneNameFor(deleteId),
 			Uid:     ulid.Make().String(),
 		},
 		Generation: ulid.Make().String(),
@@ -203,7 +203,7 @@ func (s *Server) ensureDeleteRequestValid(req *pbresource.DeleteRequest) (*resou
 
 // Maintains a deterministic mapping between a resource and it's tombstone's
 // name by embedding the resources's Uid in the name.
-func tombstoneName(deleteId *pbresource.ID) string {
+func TombstoneNameFor(deleteId *pbresource.ID) string {
 	// deleteId.Name is just included for easier identification
 	return fmt.Sprintf("tombstone-%v-%v", deleteId.Name, strings.ToLower(deleteId.Uid))
 }

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

@@ -223,9 +223,9 @@ func TestDelete_Success(t *testing.T) {
 					require.Equal(t, codes.NotFound.String(), status.Code(err).String())
 
 					// Derive tombstone name from resource that was deleted.
-					tname := tombstoneName(originalRecordLabelId)
+					tname := TombstoneNameFor(originalRecordLabelId)
 					if proto.Equal(deleteId.Type, demo.TypeV2Artist) {
-						tname = tombstoneName(originalArtistId)
+						tname = TombstoneNameFor(originalArtistId)
 					}
 
 					// Verify tombstone created
@@ -263,7 +263,7 @@ func TestDelete_TombstoneDeletionDoesNotCreateNewTombstone(t *testing.T) {
 	// verify artist's tombstone created
 	rsp2, err := client.Read(ctx, &pbresource.ReadRequest{
 		Id: &pbresource.ID{
-			Name:    tombstoneName(artist.Id),
+			Name:    TombstoneNameFor(artist.Id),
 			Type:    resource.TypeV1Tombstone,
 			Tenancy: artist.Id.Tenancy,
 		},

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

@@ -42,11 +42,6 @@ func (s *Server) ListByOwner(ctx context.Context, req *pbresource.ListByOwnerReq
 		return nil, status.Errorf(codes.Internal, "failed list acl: %v", err)
 	}
 
-	// Check tenancy exists for the v2 resource.
-	if err = tenancyExists(reg, s.TenancyBridge, req.Owner.Tenancy, codes.InvalidArgument); err != nil {
-		return nil, err
-	}
-
 	// Get owned resources.
 	children, err := s.Backend.ListByOwner(ctx, req.Owner)
 	if err != nil {
@@ -109,14 +104,8 @@ func (s *Server) ensureListByOwnerRequestValid(req *pbresource.ListByOwnerReques
 		return nil, err
 	}
 
-	// Error when partition scoped and namespace not empty.
-	if reg.Scope == resource.ScopePartition && req.Owner.Tenancy.Namespace != "" {
-		return nil, status.Errorf(
-			codes.InvalidArgument,
-			"partition scoped type %s cannot have a namespace. got: %s",
-			resource.ToGVK(req.Owner.Type),
-			req.Owner.Tenancy.Namespace,
-		)
+	if err = validateScopedTenancy(reg.Scope, reg.Type, req.Owner.Tenancy); err != nil {
+		return nil, err
 	}
 
 	return reg, nil

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

@@ -9,18 +9,20 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/hashicorp/consul/acl"
-	"github.com/hashicorp/consul/internal/resource"
-	"github.com/hashicorp/consul/internal/resource/demo"
-	"github.com/hashicorp/consul/proto-public/pbresource"
-	"github.com/hashicorp/consul/proto/private/prototest"
 	"github.com/oklog/ulid/v2"
-
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 	"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"
+	"github.com/hashicorp/consul/internal/resource/demo"
+	"github.com/hashicorp/consul/internal/resource/resourcetest"
+	"github.com/hashicorp/consul/proto-public/pbresource"
+	pbdemo "github.com/hashicorp/consul/proto/private/pbdemo/v1"
+	"github.com/hashicorp/consul/proto/private/prototest"
 )
 
 func TestListByOwner_InputValidation(t *testing.T) {
@@ -29,87 +31,103 @@ func TestListByOwner_InputValidation(t *testing.T) {
 	demo.RegisterTypes(server.Registry)
 
 	type testCase struct {
-		modFn       func(artistId, recordlabelId *pbresource.ID) *pbresource.ID
+		modFn       func(artistId, recordlabelId, executiveId *pbresource.ID) *pbresource.ID
 		errContains string
 	}
 	testCases := map[string]testCase{
 		"no owner": {
-			modFn: func(artistId, recordLabelId *pbresource.ID) *pbresource.ID {
+			modFn: func(_, _, _ *pbresource.ID) *pbresource.ID {
 				return nil
 			},
 			errContains: "owner is required",
 		},
 		"no type": {
-			modFn: func(artistId, _ *pbresource.ID) *pbresource.ID {
+			modFn: func(artistId, _, _ *pbresource.ID) *pbresource.ID {
 				artistId.Type = nil
 				return artistId
 			},
 			errContains: "owner.type is required",
 		},
 		"no name": {
-			modFn: func(artistId, _ *pbresource.ID) *pbresource.ID {
+			modFn: func(artistId, _, _ *pbresource.ID) *pbresource.ID {
 				artistId.Name = ""
 				return artistId
 			},
 			errContains: "owner.name invalid",
 		},
 		"name mixed case": {
-			modFn: func(artistId, _ *pbresource.ID) *pbresource.ID {
+			modFn: func(artistId, _, _ *pbresource.ID) *pbresource.ID {
 				artistId.Name = "U2"
 				return artistId
 			},
 			errContains: "owner.name invalid",
 		},
 		"name too long": {
-			modFn: func(artistId, _ *pbresource.ID) *pbresource.ID {
+			modFn: func(artistId, _, _ *pbresource.ID) *pbresource.ID {
 				artistId.Name = strings.Repeat("n", resource.MaxNameLength+1)
 				return artistId
 			},
 			errContains: "owner.name invalid",
 		},
 		"partition mixed case": {
-			modFn: func(artistId, _ *pbresource.ID) *pbresource.ID {
+			modFn: func(artistId, _, _ *pbresource.ID) *pbresource.ID {
 				artistId.Tenancy.Partition = "Default"
 				return artistId
 			},
 			errContains: "owner.tenancy.partition invalid",
 		},
 		"partition too long": {
-			modFn: func(artistId, _ *pbresource.ID) *pbresource.ID {
+			modFn: func(artistId, _, _ *pbresource.ID) *pbresource.ID {
 				artistId.Tenancy.Partition = strings.Repeat("p", resource.MaxNameLength+1)
 				return artistId
 			},
 			errContains: "owner.tenancy.partition invalid",
 		},
 		"namespace mixed case": {
-			modFn: func(artistId, _ *pbresource.ID) *pbresource.ID {
+			modFn: func(artistId, _, _ *pbresource.ID) *pbresource.ID {
 				artistId.Tenancy.Namespace = "Default"
 				return artistId
 			},
 			errContains: "owner.tenancy.namespace invalid",
 		},
 		"namespace too long": {
-			modFn: func(artistId, _ *pbresource.ID) *pbresource.ID {
+			modFn: func(artistId, _, _ *pbresource.ID) *pbresource.ID {
 				artistId.Tenancy.Namespace = strings.Repeat("n", resource.MaxNameLength+1)
 				return artistId
 			},
 			errContains: "owner.tenancy.namespace invalid",
 		},
 		"no uid": {
-			modFn: func(artistId, _ *pbresource.ID) *pbresource.ID {
+			modFn: func(artistId, _, _ *pbresource.ID) *pbresource.ID {
 				artistId.Uid = ""
 				return artistId
 			},
 			errContains: "owner uid is required",
 		},
 		"partition scope with non-empty namespace": {
-			modFn: func(_, recordLabelId *pbresource.ID) *pbresource.ID {
+			modFn: func(_, recordLabelId, _ *pbresource.ID) *pbresource.ID {
 				recordLabelId.Uid = ulid.Make().String()
 				recordLabelId.Tenancy.Namespace = "ishouldnothaveanamespace"
 				return recordLabelId
 			},
 			errContains: "cannot have a namespace",
 		},
+		"cluster scope with non-empty partition": {
+			modFn: func(_, _, executiveId *pbresource.ID) *pbresource.ID {
+				executiveId.Uid = ulid.Make().String()
+				executiveId.Tenancy.Partition = "ishouldnothaveapartition"
+				return executiveId
+			},
+			errContains: "cannot have a partition",
+		},
+		"cluster scope with non-empty namespace": {
+			modFn: func(_, _, executiveId *pbresource.ID) *pbresource.ID {
+				executiveId.Uid = ulid.Make().String()
+				executiveId.Tenancy.Namespace = "ishouldnothaveanamespace"
+				return executiveId
+			},
+			errContains: "cannot have a namespace",
+		},
 	}
 	for desc, tc := range testCases {
 		t.Run(desc, func(t *testing.T) {
@@ -119,8 +137,11 @@ func TestListByOwner_InputValidation(t *testing.T) {
 			recordLabel, err := demo.GenerateV1RecordLabel("looney-tunes")
 			require.NoError(t, err)
 
+			executive, err := demo.GenerateV1Executive("marvin", "CEO")
+			require.NoError(t, err)
+
 			// Each test case picks which resource to use based on the resource type's scope.
-			req := &pbresource.ListByOwnerRequest{Owner: tc.modFn(artist.Id, recordLabel.Id)}
+			req := &pbresource.ListByOwnerRequest{Owner: tc.modFn(artist.Id, recordLabel.Id, executive.Id)}
 
 			_, err = client.ListByOwner(testContext(t), req)
 			require.Error(t, err)
@@ -197,36 +218,29 @@ func TestListByOwner_Many(t *testing.T) {
 
 func TestListByOwner_OwnerTenancyDoesNotExist(t *testing.T) {
 	type testCase struct {
-		modFn       func(artistId, recordlabelId *pbresource.ID) *pbresource.ID
-		errContains string
+		modFn func(artistId, recordlabelId *pbresource.ID) *pbresource.ID
 	}
 	tenancyCases := map[string]testCase{
-		"partition not found when namespace scoped": {
+		"namespace scoped owner with non-existent partition": {
 			modFn: func(artistId, _ *pbresource.ID) *pbresource.ID {
 				id := clone(artistId)
-				id.Uid = "doesnotmatter"
 				id.Tenancy.Partition = "boguspartition"
 				return id
 			},
-			errContains: "partition not found",
 		},
-		"namespace not found when namespace scoped": {
+		"namespace scoped owner with non-existent namespace": {
 			modFn: func(artistId, _ *pbresource.ID) *pbresource.ID {
 				id := clone(artistId)
-				id.Uid = "doesnotmatter"
 				id.Tenancy.Namespace = "bogusnamespace"
 				return id
 			},
-			errContains: "namespace not found",
 		},
-		"partition not found when partition scoped": {
+		"partition scoped owner with non-existent partition": {
 			modFn: func(_, recordLabelId *pbresource.ID) *pbresource.ID {
 				id := clone(recordLabelId)
-				id.Uid = "doesnotmatter"
 				id.Tenancy.Partition = "boguspartition"
 				return id
 			},
-			errContains: "partition not found",
 		},
 	}
 	for desc, tc := range tenancyCases {
@@ -235,21 +249,21 @@ func TestListByOwner_OwnerTenancyDoesNotExist(t *testing.T) {
 			demo.RegisterTypes(server.Registry)
 			client := testClient(t, server)
 
-			recordLabel, err := demo.GenerateV1RecordLabel("looney-tunes")
-			require.NoError(t, err)
-			recordLabel, err = server.Backend.WriteCAS(testContext(t), recordLabel)
-			require.NoError(t, err)
+			recordLabel := resourcetest.Resource(demo.TypeV1RecordLabel, "looney-tunes").
+				WithTenancy(resource.DefaultPartitionedTenancy()).
+				WithData(t, &pbdemo.RecordLabel{Name: "Looney Tunes"}).
+				Write(t, client)
 
-			artist, err := demo.GenerateV2Artist()
-			require.NoError(t, err)
-			artist, err = server.Backend.WriteCAS(testContext(t), artist)
-			require.NoError(t, err)
+			artist := resourcetest.Resource(demo.TypeV1Artist, "blur").
+				WithTenancy(resource.DefaultNamespacedTenancy()).
+				WithData(t, &pbdemo.Artist{Name: "Blur"}).
+				WithOwner(recordLabel.Id).
+				Write(t, client)
 
-			// Verify non-existant tenancy units in owner err with invalid arg.
-			_, err = client.ListByOwner(testContext(t), &pbresource.ListByOwnerRequest{Owner: tc.modFn(artist.Id, recordLabel.Id)})
-			require.Error(t, err)
-			require.Equal(t, codes.InvalidArgument.String(), status.Code(err).String())
-			require.ErrorContains(t, err, tc.errContains)
+			// Verify non-existant tenancy units in owner return empty list.
+			rsp, err := client.ListByOwner(testContext(t), &pbresource.ListByOwnerRequest{Owner: tc.modFn(artist.Id, recordLabel.Id)})
+			require.NoError(t, err)
+			require.Empty(t, rsp.Resources)
 		})
 	}
 }

internal/resource/reaper/controller.go

@@ -7,11 +7,12 @@ import (
 	"context"
 	"time"
 
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+
 	"github.com/hashicorp/consul/internal/controller"
 	"github.com/hashicorp/consul/internal/resource"
 	"github.com/hashicorp/consul/proto-public/pbresource"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 )
 
 const (

internal/tenancy/internal/bridge/tenancy_bridge.go

@@ -6,6 +6,9 @@ package bridge
 import (
 	"context"
 
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+
 	"github.com/hashicorp/consul/internal/resource"
 	"github.com/hashicorp/consul/proto-public/pbresource"
 	pbtenancy "github.com/hashicorp/consul/proto-public/pbtenancy/v2beta1"
@@ -30,7 +33,7 @@ func NewV2TenancyBridge() *V2TenancyBridge {
 }
 
 func (b *V2TenancyBridge) NamespaceExists(partition, namespace string) (bool, error) {
-	rsp, err := b.client.Read(context.Background(), &pbresource.ReadRequest{
+	_, err := b.client.Read(context.Background(), &pbresource.ReadRequest{
 		Id: &pbresource.ID{
 			Name: namespace,
 			Tenancy: &pbresource.Tenancy{
@@ -39,7 +42,14 @@ func (b *V2TenancyBridge) NamespaceExists(partition, namespace string) (bool, er
 			Type: pbtenancy.NamespaceType,
 		},
 	})
-	return rsp != nil && rsp.Resource != nil, err
+	switch {
+	case err == nil:
+		return true, nil
+	case status.Code(err) == codes.NotFound:
+		return false, nil
+	default:
+		return false, err
+	}
 }
 
 func (b *V2TenancyBridge) IsNamespaceMarkedForDeletion(partition, namespace string) (bool, error) {

internal/tenancy/internal/types/namespace.go

@@ -7,6 +7,8 @@ import (
 	"fmt"
 	"strings"
 
+	"google.golang.org/protobuf/types/known/anypb"
+
 	"github.com/hashicorp/consul/agent/dns"
 	"github.com/hashicorp/consul/internal/resource"
 	"github.com/hashicorp/consul/proto-public/pbresource"
@@ -18,11 +20,26 @@ func RegisterNamespace(r resource.Registry) {
 		Type:     pbtenancy.NamespaceType,
 		Proto:    &pbtenancy.Namespace{},
 		Scope:    resource.ScopePartition,
+		Mutate:   MutateNamespace,
 		Validate: ValidateNamespace,
 		// ACLs: TODO
 	})
 }
 
+// MutateNamespace sets the resource data if nil since description and
+// other fields are optional.
+func MutateNamespace(res *pbresource.Resource) error {
+	if res.Data != nil {
+		return nil
+	}
+	data, err := anypb.New(&pbtenancy.Namespace{})
+	if err != nil {
+		return err
+	}
+	res.Data = data
+	return nil
+}
+
 func ValidateNamespace(res *pbresource.Resource) error {
 	var ns pbtenancy.Namespace