Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

resource: Make resource write tenancy aware #18423

Merged
merged 1 commit into from
Aug 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions agent/consul/tenancy_bridge_oss.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,24 @@

package consul

func (b *V1TenancyBridge) NamespaceExists(partition, namespace string) (bool, error) {
if partition == "default" && namespace == "default" {
func (b *V1TenancyBridge) PartitionExists(partition string) (bool, error) {
if partition == "default" {
return true, nil
}
return false, nil
}

func (b *V1TenancyBridge) PartitionExists(partition string) (bool, error) {
if partition == "default" {
func (b *V1TenancyBridge) IsPartitionMarkedForDeletion(partition string) (bool, error) {
return false, nil
}

func (b *V1TenancyBridge) NamespaceExists(partition, namespace string) (bool, error) {
if partition == "default" && namespace == "default" {
return true, nil
}
return false, nil
}

func (b *V1TenancyBridge) IsNamespaceMarkedForDeletion(partition, namespace string) (bool, error) {
return false, nil
}
4 changes: 2 additions & 2 deletions agent/grpc-external/services/resource/delete.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func (s *Server) Delete(ctx context.Context, req *pbresource.DeleteRequest) (*pb
}

// TODO(spatel): Refactor _ and entMeta in NET-4919
authz, _, err := s.getAuthorizer(tokenFromContext(ctx), acl.DefaultEnterpriseMeta())
authz, authzContext, err := s.getAuthorizer(tokenFromContext(ctx), acl.DefaultEnterpriseMeta())
if err != nil {
return nil, err
}
Expand All @@ -58,7 +58,7 @@ func (s *Server) Delete(ctx context.Context, req *pbresource.DeleteRequest) (*pb
}

// Check ACLs
err = reg.ACLs.Write(authz, existing)
err = reg.ACLs.Write(authz, authzContext, existing)
switch {
case acl.IsErrPermissionDenied(err):
return nil, status.Error(codes.PermissionDenied, err.Error())
Expand Down
48 changes: 48 additions & 0 deletions agent/grpc-external/services/resource/mock_TenancyBridge.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion agent/grpc-external/services/resource/read.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func (s *Server) Read(ctx context.Context, req *pbresource.ReadRequest) (*pbreso
}

// Check V1 tenancy exists for the V2 resource.
if err = v1TenancyExists(reg, s.V1TenancyBridge, req.Id.Tenancy); err != nil {
if err = v1TenancyExists(reg, s.V1TenancyBridge, req.Id.Tenancy, codes.NotFound); err != nil {
return nil, err
}

Expand Down
10 changes: 5 additions & 5 deletions agent/grpc-external/services/resource/read_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -161,20 +161,20 @@ func TestRead_Success(t *testing.T) {
"namespaced resource provides nonempty partition and namespace": func(artistId, recordLabelId *pbresource.ID) *pbresource.ID {
return artistId
},
"namespaced resource provides uppercase namespace and partition": func(artistId, _ *pbresource.ID) *pbresource.ID {
"namespaced resource provides uppercase partition and namespace": func(artistId, _ *pbresource.ID) *pbresource.ID {
id := clone(artistId)
id.Tenancy.Partition = strings.ToUpper(artistId.Tenancy.Partition)
id.Tenancy.Namespace = strings.ToUpper(artistId.Tenancy.Namespace)
return id
},
"namespaced resource inherits tokens namespace when empty": func(artistId, _ *pbresource.ID) *pbresource.ID {
"namespaced resource inherits tokens partition when empty": func(artistId, _ *pbresource.ID) *pbresource.ID {
id := clone(artistId)
id.Tenancy.Namespace = ""
id.Tenancy.Partition = ""
return id
},
"namespaced resource inherits tokens partition when empty": func(artistId, _ *pbresource.ID) *pbresource.ID {
"namespaced resource inherits tokens namespace when empty": func(artistId, _ *pbresource.ID) *pbresource.ID {
id := clone(artistId)
id.Tenancy.Partition = ""
id.Tenancy.Namespace = ""
return id
},
"namespaced resource inherits tokens partition and namespace when empty": func(artistId, _ *pbresource.ID) *pbresource.ID {
Expand Down
35 changes: 31 additions & 4 deletions agent/grpc-external/services/resource/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,9 @@ type ACLResolver interface {
//go:generate mockery --name TenancyBridge --inpackage
type TenancyBridge interface {
PartitionExists(partition string) (bool, error)
NamespaceExists(partition string, namespace string) (bool, error)
IsPartitionMarkedForDeletion(partition string) (bool, error)
NamespaceExists(partition, namespace string) (bool, error)
IsNamespaceMarkedForDeletion(partition, namespace string) (bool, error)
}

func NewServer(cfg Config) *Server {
Expand Down Expand Up @@ -145,14 +147,15 @@ func validateId(id *pbresource.ID, errorPrefix string) error {
return nil
}

func v1TenancyExists(reg *resource.Registration, v1Bridge TenancyBridge, tenancy *pbresource.Tenancy) error {
// v1TenancyExists return an error with the passed in gRPC status code when tenancy partition or namespace do not exist.
func v1TenancyExists(reg *resource.Registration, v1Bridge TenancyBridge, tenancy *pbresource.Tenancy, errCode codes.Code) error {
if reg.Scope == resource.ScopePartition || reg.Scope == resource.ScopeNamespace {
exists, err := v1Bridge.PartitionExists(tenancy.Partition)
switch {
case err != nil:
return err
case !exists:
return status.Errorf(codes.NotFound, "partition resource not found: %v", tenancy.Partition)
return status.Errorf(errCode, "partition resource not found: %v", tenancy.Partition)
}
}

Expand All @@ -162,7 +165,31 @@ func v1TenancyExists(reg *resource.Registration, v1Bridge TenancyBridge, tenancy
case err != nil:
return err
case !exists:
return status.Errorf(codes.NotFound, "namespace resource not found: %v", tenancy.Namespace)
return status.Errorf(errCode, "namespace resource not found: %v", tenancy.Namespace)
}
}
return nil
}

// v1TenancyMarkedForDeletion returns a gRPC InvalidArgument when either partition or namespace is marked for deletion.
func v1TenancyMarkedForDeletion(reg *resource.Registration, v1Bridge TenancyBridge, tenancy *pbresource.Tenancy) error {
if reg.Scope == resource.ScopePartition || reg.Scope == resource.ScopeNamespace {
marked, err := v1Bridge.IsPartitionMarkedForDeletion(tenancy.Partition)
switch {
case err != nil:
return err
case marked:
return status.Errorf(codes.InvalidArgument, "partition marked for deletion: %v", tenancy.Partition)
}
}

if reg.Scope == resource.ScopeNamespace {
marked, err := v1Bridge.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)
}
}
return nil
Expand Down
2 changes: 2 additions & 0 deletions agent/grpc-external/services/resource/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ func testServer(t *testing.T) *Server {
mockTenancyBridge.On("NamespaceExists", resource.DefaultPartitionName, resource.DefaultNamespaceName).Return(true, nil)
mockTenancyBridge.On("PartitionExists", mock.Anything).Return(false, nil)
mockTenancyBridge.On("NamespaceExists", mock.Anything, mock.Anything).Return(false, nil)
mockTenancyBridge.On("IsPartitionMarkedForDeletion", resource.DefaultPartitionName).Return(false, nil)
mockTenancyBridge.On("IsNamespaceMarkedForDeletion", resource.DefaultPartitionName, resource.DefaultNamespaceName).Return(false, nil)

return NewServer(Config{
Logger: testutil.Logger(t),
Expand Down
2 changes: 2 additions & 0 deletions agent/grpc-external/services/resource/testing/testing.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ func RunResourceServiceWithACL(t *testing.T, aclResolver svc.ACLResolver, regist
mockTenancyBridge := &svc.MockTenancyBridge{}
mockTenancyBridge.On("PartitionExists", resource.DefaultPartitionName).Return(true, nil)
mockTenancyBridge.On("NamespaceExists", resource.DefaultPartitionName, resource.DefaultNamespaceName).Return(true, nil)
mockTenancyBridge.On("IsPartitionMarkedForDeletion", resource.DefaultPartitionName).Return(false, nil)
mockTenancyBridge.On("IsNamespaceMarkedForDeletion", resource.DefaultPartitionName, resource.DefaultNamespaceName).Return(false, nil)

svc.NewServer(svc.Config{
Backend: backend,
Expand Down
52 changes: 38 additions & 14 deletions agent/grpc-external/services/resource/write.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,23 +37,20 @@ import (
var errUseWriteStatus = status.Error(codes.InvalidArgument, "resource.status can only be set using the WriteStatus endpoint")

func (s *Server) Write(ctx context.Context, req *pbresource.WriteRequest) (*pbresource.WriteResponse, error) {
if err := validateWriteRequest(req); err != nil {
return nil, err
}

reg, err := s.resolveType(req.Resource.Id.Type)
reg, err := s.validateWriteRequest(req)
if err != nil {
return nil, err
}

// TODO(spatel): Refactor _ and entMeta as part of NET-4911
authz, _, err := s.getAuthorizer(tokenFromContext(ctx), acl.DefaultEnterpriseMeta())
v1EntMeta := v2TenancyToV1EntMeta(req.Resource.Id.Tenancy)
authz, authzContext, err := s.getAuthorizer(tokenFromContext(ctx), v1EntMeta)
if err != nil {
return nil, err
}
v1EntMetaToV2Tenancy(reg, v1EntMeta, req.Resource.Id.Tenancy)

// check acls
err = reg.ACLs.Write(authz, req.Resource)
// ACL check comes before tenancy existence checks to not leak tenancy "existence".
err = reg.ACLs.Write(authz, authzContext, req.Resource)
switch {
case acl.IsErrPermissionDenied(err):
return nil, status.Error(codes.PermissionDenied, err.Error())
Expand All @@ -73,6 +70,16 @@ func (s *Server) Write(ctx context.Context, req *pbresource.WriteRequest) (*pbre
)
}

// Check V1 tenancy exists for the V2 resource
if err = v1TenancyExists(reg, s.V1TenancyBridge, req.Resource.Id.Tenancy, codes.InvalidArgument); err != nil {
return nil, err
}

// Check V1 tenancy not marked for deletion.
if err = v1TenancyMarkedForDeletion(reg, s.V1TenancyBridge, req.Resource.Id.Tenancy); err != nil {
return nil, err
}

if err = reg.Mutate(req.Resource); err != nil {
return nil, status.Errorf(codes.Internal, "failed mutate hook: %v", err.Error())
}
Expand Down Expand Up @@ -258,7 +265,7 @@ func (s *Server) retryCAS(ctx context.Context, vsn string, cas func() error) err
return err
}

func validateWriteRequest(req *pbresource.WriteRequest) error {
func (s *Server) validateWriteRequest(req *pbresource.WriteRequest) (*resource.Registration, error) {
var field string
switch {
case req.Resource == nil:
Expand All @@ -270,17 +277,34 @@ func validateWriteRequest(req *pbresource.WriteRequest) error {
}

if field != "" {
return status.Errorf(codes.InvalidArgument, "%s is required", field)
return nil, status.Errorf(codes.InvalidArgument, "%s is required", field)
}

if err := validateId(req.Resource.Id, "resource.id"); err != nil {
return err
return nil, err
}

if req.Resource.Owner != nil {
if err := validateId(req.Resource.Owner, "resource.owner"); err != nil {
return err
return nil, err
}
}
return nil

// Check type exists.
reg, err := s.resolveType(req.Resource.Id.Type)
if err != nil {
return nil, err
}

// Check scope
if reg.Scope == resource.ScopePartition && req.Resource.Id.Tenancy.Namespace != "" {
return nil, status.Errorf(
codes.InvalidArgument,
"partition scoped resource %s cannot have a namespace. got: %s",
resource.ToGVK(req.Resource.Id.Type),
req.Resource.Id.Tenancy.Namespace,
)
}

return reg, nil
}
Loading