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.

Sign up for GitHub

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 delete tenancy aware #18476

Merged
merged 1 commit into from
Aug 16, 2023
Merged

resource: Make resource delete tenancy aware #18476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
38 changes: 27 additions & 11 deletions agent/grpc-external/services/resource/delete.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,13 @@ import (
// - Errors with Aborted if the requested Version does not match the stored Version.
// - Errors with PermissionDenied if ACL check fails
func (s *Server) Delete(ctx context.Context, req *pbresource.DeleteRequest) (*pbresource.DeleteResponse, error) {
if err := validateDeleteRequest(req); err != nil {
return nil, err
}

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

// TODO(spatel): Refactor _ and entMeta in NET-4919
authz, authzContext, err := s.getAuthorizer(tokenFromContext(ctx), acl.DefaultEnterpriseMeta())
entMeta := v2TenancyToV1EntMeta(req.Id.Tenancy)
authz, authzContext, err := s.getAuthorizer(tokenFromContext(ctx), entMeta)
if err != nil {
return nil, err
}
Expand All @@ -48,6 +44,10 @@ func (s *Server) Delete(ctx context.Context, req *pbresource.DeleteRequest) (*pb
if req.Version == "" || req.Id.Uid == "" {
consistency = storage.StrongConsistency
}

// Apply defaults when tenancy units empty.
v1EntMetaToV2Tenancy(reg, entMeta, req.Id.Tenancy)

existing, err := s.Backend.Read(ctx, consistency, req.Id)
switch {
case errors.Is(err, storage.ErrNotFound):
Expand Down Expand Up @@ -144,15 +144,31 @@ func (s *Server) maybeCreateTombstone(ctx context.Context, deleteId *pbresource.
}
}

func validateDeleteRequest(req *pbresource.DeleteRequest) error {
func (s *Server) validateDeleteRequest(req *pbresource.DeleteRequest) (*resource.Registration, error) {
if req.Id == nil {
return status.Errorf(codes.InvalidArgument, "id is required")
return nil, status.Errorf(codes.InvalidArgument, "id is required")
}

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

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

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

return reg, nil
}

// Maintains a deterministic mapping between a resource and it's tombstone's
Expand Down
120 changes: 68 additions & 52 deletions agent/grpc-external/services/resource/delete_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"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/resolver"
"github.com/hashicorp/consul/internal/resource"
Expand All @@ -25,35 +26,36 @@ func TestDelete_InputValidation(t *testing.T) {

demo.RegisterTypes(server.Registry)

testCases := map[string]func(*pbresource.DeleteRequest){
"no id": func(req *pbresource.DeleteRequest) { req.Id = nil },
"no type": func(req *pbresource.DeleteRequest) { req.Id.Type = nil },
"no tenancy": func(req *pbresource.DeleteRequest) { req.Id.Tenancy = nil },
"no name": func(req *pbresource.DeleteRequest) { req.Id.Name = "" },

// TODO(spatel): Refactor tenancy as part of NET-4919
//
// clone necessary to not pollute DefaultTenancy
// "tenancy partition not default": func(req *pbresource.DeleteRequest) {
// req.Id.Tenancy = clone(req.Id.Tenancy)
// req.Id.Tenancy.Partition = ""
// },
// "tenancy namespace not default": func(req *pbresource.DeleteRequest) {
// req.Id.Tenancy = clone(req.Id.Tenancy)
// req.Id.Tenancy.Namespace = ""
// },
// "tenancy peername not local": func(req *pbresource.DeleteRequest) {
// req.Id.Tenancy = clone(req.Id.Tenancy)
// req.Id.Tenancy.PeerName = ""
// },
testCases := map[string]func(artistId, recordLabelId *pbresource.ID) *pbresource.ID{
"no id": func(artistId, recordLabelId *pbresource.ID) *pbresource.ID {
return nil
},
"no type": func(artistId, _ *pbresource.ID) *pbresource.ID {
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
},
"partition scoped resource with namespace": func(_, recordLabelId *pbresource.ID) *pbresource.ID {
recordLabelId.Tenancy.Namespace = "ishouldnothaveanamespace"
return recordLabelId
},
}
for desc, modFn := range testCases {
t.Run(desc, func(t *testing.T) {
res, err := demo.GenerateV2Artist()
recordLabel, err := demo.GenerateV1RecordLabel("LoonyTunes")
require.NoError(t, err)

req := &pbresource.DeleteRequest{Id: res.Id, Version: ""}
modFn(req)
artist, err := demo.GenerateV2Artist()
require.NoError(t, err)

req := &pbresource.DeleteRequest{Id: modFn(artist.Id, recordLabel.Id), Version: ""}

_, err = client.Delete(testContext(t), req)
require.Error(t, err)
Expand Down Expand Up @@ -125,34 +127,48 @@ func TestDelete_Success(t *testing.T) {

for desc, tc := range deleteTestCases() {
t.Run(desc, func(t *testing.T) {
server, client, ctx := testDeps(t)
demo.RegisterTypes(server.Registry)
artist, err := demo.GenerateV2Artist()
require.NoError(t, err)

rsp, err := client.Write(ctx, &pbresource.WriteRequest{Resource: artist})
require.NoError(t, err)
artistId := clone(rsp.Resource.Id)
artist = rsp.Resource

// delete
_, err = client.Delete(ctx, tc.deleteReqFn(artist))
require.NoError(t, err)

// verify deleted
_, err = server.Backend.Read(ctx, storage.StrongConsistency, artistId)
require.Error(t, err)
require.ErrorIs(t, err, storage.ErrNotFound)

// verify tombstone created
_, err = client.Read(ctx, &pbresource.ReadRequest{
Id: &pbresource.ID{
Name: tombstoneName(artistId),
Type: resource.TypeV1Tombstone,
Tenancy: artist.Id.Tenancy,
},
})
require.NoError(t, err)
for tenancyDesc, modFn := range tenancyCases() {
t.Run(tenancyDesc, func(t *testing.T) {
server, client, ctx := testDeps(t)
demo.RegisterTypes(server.Registry)

recordLabel, err := demo.GenerateV1RecordLabel("LoonyTunes")
require.NoError(t, err)
recordLabel, err = server.Backend.WriteCAS(ctx, recordLabel)
require.NoError(t, err)

artist, err := demo.GenerateV2Artist()
require.NoError(t, err)
artist, err = server.Backend.WriteCAS(ctx, artist)
require.NoError(t, err)

// Pick the resource to be deleted based on type's scope
deleteId := modFn(artist.Id, recordLabel.Id)
deleteReq := tc.deleteReqFn(recordLabel)
if proto.Equal(deleteId.Type, demo.TypeV2Artist) {
deleteReq = tc.deleteReqFn(artist)
}

// Delete
_, err = client.Delete(ctx, deleteReq)
require.NoError(t, err)

// Verify deleted
_, err = server.Backend.Read(ctx, storage.StrongConsistency, deleteId)
require.Error(t, err)
require.ErrorIs(t, err, storage.ErrNotFound)

// Verify tombstone created
_, err = client.Read(ctx, &pbresource.ReadRequest{
Id: &pbresource.ID{
Name: tombstoneName(deleteReq.Id),
Type: resource.TypeV1Tombstone,
Tenancy: deleteReq.Id.Tenancy,
},
})
require.NoError(t, err, "expected tombstome to be found")
})
}
})
}
}
Expand Down
43 changes: 1 addition & 42 deletions agent/grpc-external/services/resource/read_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ package resource

import (
"context"
"strings"
"testing"

"github.com/stretchr/testify/mock"
Expand Down Expand Up @@ -157,47 +156,7 @@ func TestRead_GroupVersionMismatch(t *testing.T) {
func TestRead_Success(t *testing.T) {
for desc, tc := range readTestCases() {
t.Run(desc, func(t *testing.T) {
tenancyCases := map[string]func(artistId, recordlabelId *pbresource.ID) *pbresource.ID{
"namespaced resource provides nonempty partition and namespace": func(artistId, recordLabelId *pbresource.ID) *pbresource.ID {
return artistId
},
"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 partition when empty": func(artistId, _ *pbresource.ID) *pbresource.ID {
id := clone(artistId)
id.Tenancy.Partition = ""
return id
},
"namespaced resource inherits tokens namespace when empty": func(artistId, _ *pbresource.ID) *pbresource.ID {
id := clone(artistId)
id.Tenancy.Namespace = ""
return id
},
"namespaced resource inherits tokens partition and namespace when empty": func(artistId, _ *pbresource.ID) *pbresource.ID {
id := clone(artistId)
id.Tenancy.Partition = ""
id.Tenancy.Namespace = ""
return id
},
"partitioned resource provides nonempty partition": func(_, recordLabelId *pbresource.ID) *pbresource.ID {
return recordLabelId
},
"partitioned resource provides uppercase partition": func(_, recordLabelId *pbresource.ID) *pbresource.ID {
id := clone(recordLabelId)
id.Tenancy.Partition = strings.ToUpper(recordLabelId.Tenancy.Partition)
return id
},
"partitioned resource inherits tokens partition when empty": func(_, recordLabelId *pbresource.ID) *pbresource.ID {
id := clone(recordLabelId)
id.Tenancy.Partition = ""
return id
},
}
for tenancyDesc, modFn := range tenancyCases {
for tenancyDesc, modFn := range tenancyCases() {
t.Run(tenancyDesc, func(t *testing.T) {
server := testServer(t)
demo.RegisterTypes(server.Registry)
Expand Down
48 changes: 48 additions & 0 deletions agent/grpc-external/services/resource/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package resource
import (
"context"
"fmt"
"strings"
"testing"

"github.com/stretchr/testify/mock"
Expand Down Expand Up @@ -129,3 +130,50 @@ func modifyArtist(t *testing.T, res *pbresource.Resource) *pbresource.Resource {
res.Data = data
return res
}

// 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
func tenancyCases() map[string]func(artistId, recordlabelId *pbresource.ID) *pbresource.ID {
tenancyCases := map[string]func(artistId, recordlabelId *pbresource.ID) *pbresource.ID{
"namespaced resource provides nonempty partition and namespace": func(artistId, recordLabelId *pbresource.ID) *pbresource.ID {
return artistId
},
"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 partition when empty": func(artistId, _ *pbresource.ID) *pbresource.ID {
id := clone(artistId)
id.Tenancy.Partition = ""
return id
},
"namespaced resource inherits tokens namespace when empty": func(artistId, _ *pbresource.ID) *pbresource.ID {
id := clone(artistId)
id.Tenancy.Namespace = ""
return id
},
"namespaced resource inherits tokens partition and namespace when empty": func(artistId, _ *pbresource.ID) *pbresource.ID {
id := clone(artistId)
id.Tenancy.Partition = ""
id.Tenancy.Namespace = ""
return id
},
"partitioned resource provides nonempty partition": func(_, recordLabelId *pbresource.ID) *pbresource.ID {
return recordLabelId
},
"partitioned resource provides uppercase partition": func(_, recordLabelId *pbresource.ID) *pbresource.ID {
id := clone(recordLabelId)
id.Tenancy.Partition = strings.ToUpper(recordLabelId.Tenancy.Partition)
return id
},
"partitioned resource inherits tokens partition when empty": func(_, recordLabelId *pbresource.ID) *pbresource.ID {
id := clone(recordLabelId)
id.Tenancy.Partition = ""
return id
},
}
return tenancyCases
}