diff --git a/agent/grpc-external/services/resource/list_test.go b/agent/grpc-external/services/resource/list_test.go index e640e38921..64026b7d34 100644 --- a/agent/grpc-external/services/resource/list_test.go +++ b/agent/grpc-external/services/resource/list_test.go @@ -119,83 +119,7 @@ func TestList_Many(t *testing.T) { func TestList_Tenancy_Defaults_And_Normalization(t *testing.T) { // Test units of tenancy get defaulted correctly when empty. ctx := context.Background() - testCases := map[string]struct { - typ *pbresource.Type - tenancy *pbresource.Tenancy - }{ - "namespaced type with empty partition": { - typ: demo.TypeV2Artist, - tenancy: &pbresource.Tenancy{ - Partition: "", - Namespace: resource.DefaultNamespaceName, - PeerName: "local", - }, - }, - "namespaced type with empty namespace": { - typ: demo.TypeV2Artist, - tenancy: &pbresource.Tenancy{ - Partition: resource.DefaultPartitionName, - Namespace: "", - PeerName: "local", - }, - }, - "namespaced type with empty partition and namespace": { - typ: demo.TypeV2Artist, - tenancy: &pbresource.Tenancy{ - Partition: "", - Namespace: "", - PeerName: "local", - }, - }, - "namespaced type with uppercase partition and namespace": { - typ: demo.TypeV2Artist, - tenancy: &pbresource.Tenancy{ - Partition: "DEFAULT", - Namespace: "DEFAULT", - PeerName: "local", - }, - }, - "namespaced type with wildcard partition and empty namespace": { - typ: demo.TypeV2Artist, - tenancy: &pbresource.Tenancy{ - Partition: "*", - Namespace: "", - PeerName: "local", - }, - }, - "namespaced type with empty partition and wildcard namespace": { - typ: demo.TypeV2Artist, - tenancy: &pbresource.Tenancy{ - Partition: "", - Namespace: "*", - PeerName: "local", - }, - }, - "partitioned type with empty partition": { - typ: demo.TypeV1RecordLabel, - tenancy: &pbresource.Tenancy{ - Partition: "", - Namespace: "", - PeerName: "local", - }, - }, - "partitioned type with uppercase partition": { - typ: demo.TypeV1RecordLabel, - tenancy: &pbresource.Tenancy{ - Partition: "DEFAULT", - Namespace: "", - PeerName: "local", - }, - }, - "partitioned type with wildcard partition": { - typ: demo.TypeV1RecordLabel, - tenancy: &pbresource.Tenancy{ - Partition: "*", - PeerName: "local", - }, - }, - } - for desc, tc := range testCases { + for desc, tc := range wildcardTenancyCases() { t.Run(desc, func(t *testing.T) { server := testServer(t) demo.RegisterTypes(server.Registry) diff --git a/agent/grpc-external/services/resource/server_test.go b/agent/grpc-external/services/resource/server_test.go index 629c90f922..7f745952e5 100644 --- a/agent/grpc-external/services/resource/server_test.go +++ b/agent/grpc-external/services/resource/server_test.go @@ -21,6 +21,7 @@ import ( "github.com/hashicorp/consul/agent/grpc-external/testutils" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/internal/resource" + "github.com/hashicorp/consul/internal/resource/demo" "github.com/hashicorp/consul/internal/storage/inmem" "github.com/hashicorp/consul/proto-public/pbresource" pbdemov2 "github.com/hashicorp/consul/proto/private/pbdemo/v2" @@ -131,6 +132,90 @@ func modifyArtist(t *testing.T, res *pbresource.Resource) *pbresource.Resource { return res } +// wildcardTenancyCases returns permutations of tenancy and type scope used as input +// to endpoints that accept wildcards for tenancy. +func wildcardTenancyCases() map[string]struct { + typ *pbresource.Type + tenancy *pbresource.Tenancy +} { + return map[string]struct { + typ *pbresource.Type + tenancy *pbresource.Tenancy + }{ + "namespaced type with empty partition": { + typ: demo.TypeV2Artist, + tenancy: &pbresource.Tenancy{ + Partition: "", + Namespace: resource.DefaultNamespaceName, + PeerName: "local", + }, + }, + "namespaced type with empty namespace": { + typ: demo.TypeV2Artist, + tenancy: &pbresource.Tenancy{ + Partition: resource.DefaultPartitionName, + Namespace: "", + PeerName: "local", + }, + }, + "namespaced type with empty partition and namespace": { + typ: demo.TypeV2Artist, + tenancy: &pbresource.Tenancy{ + Partition: "", + Namespace: "", + PeerName: "local", + }, + }, + "namespaced type with uppercase partition and namespace": { + typ: demo.TypeV2Artist, + tenancy: &pbresource.Tenancy{ + Partition: "DEFAULT", + Namespace: "DEFAULT", + PeerName: "local", + }, + }, + "namespaced type with wildcard partition and empty namespace": { + typ: demo.TypeV2Artist, + tenancy: &pbresource.Tenancy{ + Partition: "*", + Namespace: "", + PeerName: "local", + }, + }, + "namespaced type with empty partition and wildcard namespace": { + typ: demo.TypeV2Artist, + tenancy: &pbresource.Tenancy{ + Partition: "", + Namespace: "*", + PeerName: "local", + }, + }, + "partitioned type with empty partition": { + typ: demo.TypeV1RecordLabel, + tenancy: &pbresource.Tenancy{ + Partition: "", + Namespace: "", + PeerName: "local", + }, + }, + "partitioned type with uppercase partition": { + typ: demo.TypeV1RecordLabel, + tenancy: &pbresource.Tenancy{ + Partition: "DEFAULT", + Namespace: "", + PeerName: "local", + }, + }, + "partitioned type with wildcard partition": { + typ: demo.TypeV1RecordLabel, + tenancy: &pbresource.Tenancy{ + Partition: "*", + PeerName: "local", + }, + }, + } +} + // tenancyCases returns permutations of valid tenancy structs in a resource id to use as inputs. // - the id is for a recordLabel when the resource is partition scoped // - the id is for an artist when the resource is namespace scoped diff --git a/agent/grpc-external/services/resource/watch.go b/agent/grpc-external/services/resource/watch.go index 6f321fbc1a..6039613b8f 100644 --- a/agent/grpc-external/services/resource/watch.go +++ b/agent/grpc-external/services/resource/watch.go @@ -10,28 +10,26 @@ import ( "google.golang.org/grpc/status" "github.com/hashicorp/consul/acl" + "github.com/hashicorp/consul/internal/resource" "github.com/hashicorp/consul/internal/storage" "github.com/hashicorp/consul/proto-public/pbresource" ) func (s *Server) WatchList(req *pbresource.WatchListRequest, stream pbresource.ResourceService_WatchListServer) error { - if err := validateWatchListRequest(req); err != nil { - return err - } - - // check type exists - reg, err := s.resolveType(req.Type) + reg, err := s.validateWatchListRequest(req) if err != nil { return err } - // TODO(spatel): Refactor _ and entMeta as part of NET-4914 - authz, authzContext, err := s.getAuthorizer(tokenFromContext(stream.Context()), acl.DefaultEnterpriseMeta()) + // v1 ACL subsystem is "wildcard" aware so just pass on through. + entMeta := v2TenancyToV1EntMeta(req.Tenancy) + token := tokenFromContext(stream.Context()) + authz, authzContext, err := s.getAuthorizer(token, entMeta) if err != nil { return err } - // check acls + // Check list ACL. err = reg.ACLs.List(authz, authzContext) switch { case acl.IsErrPermissionDenied(err): @@ -40,6 +38,9 @@ func (s *Server) WatchList(req *pbresource.WatchListRequest, stream pbresource.R return status.Errorf(codes.Internal, "failed list acl: %v", err) } + // Ensure we're defaulting correctly when request tenancy units are empty. + v1EntMetaToV2Tenancy(reg, entMeta, req.Tenancy) + unversionedType := storage.UnversionedTypeFrom(req.Type) watch, err := s.Backend.WatchList( stream.Context(), @@ -66,6 +67,15 @@ func (s *Server) WatchList(req *pbresource.WatchListRequest, stream pbresource.R continue } + // Need to rebuild authorizer per resource since wildcard inputs may + // result in different tenancies. Consider caching per tenancy if this + // is deemed expensive. + entMeta = v2TenancyToV1EntMeta(event.Resource.Id.Tenancy) + authz, authzContext, err = s.getAuthorizer(token, entMeta) + if err != nil { + return err + } + // filter out items that don't pass read ACLs err = reg.ACLs.Read(authz, authzContext, event.Resource.Id) switch { @@ -81,15 +91,37 @@ func (s *Server) WatchList(req *pbresource.WatchListRequest, stream pbresource.R } } -func validateWatchListRequest(req *pbresource.WatchListRequest) error { +func (s *Server) validateWatchListRequest(req *pbresource.WatchListRequest) (*resource.Registration, error) { var field string switch { case req.Type == nil: field = "type" case req.Tenancy == nil: field = "tenancy" - default: - return nil } - return status.Errorf(codes.InvalidArgument, "%s is required", field) + + if field != "" { + return nil, status.Errorf(codes.InvalidArgument, "%s is required", field) + } + + // Check type exists. + reg, err := s.resolveType(req.Type) + if err != nil { + return nil, err + } + + // Lowercase + resource.Normalize(req.Tenancy) + + // Error when partition scoped and namespace not empty. + if reg.Scope == resource.ScopePartition && req.Tenancy.Namespace != "" { + return nil, status.Errorf( + codes.InvalidArgument, + "partition scoped type %s cannot have a namespace. got: %s", + resource.ToGVK(req.Type), + req.Tenancy.Namespace, + ) + } + + return reg, nil } diff --git a/agent/grpc-external/services/resource/watch_test.go b/agent/grpc-external/services/resource/watch_test.go index 7574c155da..051264441b 100644 --- a/agent/grpc-external/services/resource/watch_test.go +++ b/agent/grpc-external/services/resource/watch_test.go @@ -12,6 +12,7 @@ import ( "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/grpc-external/testutils" + "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" @@ -20,6 +21,7 @@ import ( "github.com/stretchr/testify/require" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "google.golang.org/protobuf/proto" ) func TestWatchList_InputValidation(t *testing.T) { @@ -31,12 +33,16 @@ func TestWatchList_InputValidation(t *testing.T) { testCases := map[string]func(*pbresource.WatchListRequest){ "no type": func(req *pbresource.WatchListRequest) { req.Type = nil }, "no tenancy": func(req *pbresource.WatchListRequest) { req.Tenancy = nil }, + "partitioned type provides non-empty namespace": func(req *pbresource.WatchListRequest) { + req.Type = demo.TypeV1RecordLabel + req.Tenancy.Namespace = "bad" + }, } for desc, modFn := range testCases { t.Run(desc, func(t *testing.T) { req := &pbresource.WatchListRequest{ Type: demo.TypeV2Album, - Tenancy: demo.TenancyDefault, + Tenancy: resource.DefaultNamespacedTenancy(), } modFn(req) @@ -58,7 +64,7 @@ func TestWatchList_TypeNotFound(t *testing.T) { stream, err := client.WatchList(context.Background(), &pbresource.WatchListRequest{ Type: demo.TypeV2Artist, - Tenancy: demo.TenancyDefault, + Tenancy: resource.DefaultNamespacedTenancy(), NamePrefix: "", }) require.NoError(t, err) @@ -80,7 +86,7 @@ func TestWatchList_GroupVersionMatches(t *testing.T) { // create a watch stream, err := client.WatchList(ctx, &pbresource.WatchListRequest{ Type: demo.TypeV2Artist, - Tenancy: demo.TenancyDefault, + Tenancy: resource.DefaultNamespacedTenancy(), NamePrefix: "", }) require.NoError(t, err) @@ -111,6 +117,54 @@ func TestWatchList_GroupVersionMatches(t *testing.T) { require.Equal(t, pbresource.WatchEvent_OPERATION_DELETE, rsp.Operation) } +func TestWatchList_Tenancy_Defaults_And_Normalization(t *testing.T) { + // Test units of tenancy get lowercased and defaulted correctly when empty. + for desc, tc := range wildcardTenancyCases() { + t.Run(desc, func(t *testing.T) { + ctx := context.Background() + server := testServer(t) + client := testClient(t, server) + demo.RegisterTypes(server.Registry) + + // Create a watch. + stream, err := client.WatchList(ctx, &pbresource.WatchListRequest{ + Type: tc.typ, + Tenancy: tc.tenancy, + NamePrefix: "", + }) + require.NoError(t, err) + rspCh := handleResourceStream(t, stream) + + // Testcase will pick one of recordLabel or artist based on scope of type. + recordLabel, err := demo.GenerateV1RecordLabel("LooneyTunes") + require.NoError(t, err) + artist, err := demo.GenerateV2Artist() + require.NoError(t, err) + + // Create and verify upsert event received. + recordLabel, err = server.Backend.WriteCAS(ctx, recordLabel) + require.NoError(t, err) + artist, err = server.Backend.WriteCAS(ctx, artist) + require.NoError(t, err) + + var expected *pbresource.Resource + switch { + case proto.Equal(tc.typ, demo.TypeV1RecordLabel): + expected = recordLabel + case proto.Equal(tc.typ, demo.TypeV2Artist): + expected = artist + default: + require.Fail(t, "unsupported type", tc.typ) + } + + rsp := mustGetResource(t, rspCh) + require.Equal(t, pbresource.WatchEvent_OPERATION_UPSERT, rsp.Operation) + prototest.AssertDeepEqual(t, expected, rsp.Resource) + }) + + } +} + func TestWatchList_GroupVersionMismatch(t *testing.T) { // Given a watch on TypeArtistV1 that only differs from TypeArtistV2 by GroupVersion // When a resource of TypeArtistV2 is created/updated/deleted @@ -125,7 +179,7 @@ func TestWatchList_GroupVersionMismatch(t *testing.T) { // create a watch for TypeArtistV1 stream, err := client.WatchList(ctx, &pbresource.WatchListRequest{ Type: demo.TypeV1Artist, - Tenancy: demo.TenancyDefault, + Tenancy: resource.DefaultNamespacedTenancy(), NamePrefix: "", }) require.NoError(t, err)