From 1f860b99d241d4ba8338408bb68c9daf9d6ea88c Mon Sep 17 00:00:00 2001 From: Semir Patel Date: Fri, 14 Apr 2023 08:18:24 -0500 Subject: [PATCH] Fix delete when uid not provided (#16996) --- .../grpc-external/services/resource/delete.go | 33 +++++++++------ .../services/resource/delete_test.go | 40 ++++++++++++------- 2 files changed, 47 insertions(+), 26 deletions(-) diff --git a/agent/grpc-external/services/resource/delete.go b/agent/grpc-external/services/resource/delete.go index 542469d42..a7e439c33 100644 --- a/agent/grpc-external/services/resource/delete.go +++ b/agent/grpc-external/services/resource/delete.go @@ -12,12 +12,14 @@ import ( "github.com/hashicorp/consul/proto-public/pbresource" ) -// TODO(spatel): Move docs to the proto file -// Deletes a resource with the given Id and Version. +// Deletes a resource. +// - To delete a resource regardless of the stored version, set Version = "" +// - Supports deleting a resource by name, hence Id.Uid may be empty. +// - Delete of a previously deleted or non-existent resource is a no-op to support idempotency. +// - Errors with Aborted if the requested Version does not match the stored Version. +// - Errors with PermissionDenied if ACL check fails // -// Pass an empty Version to delete a resource regardless of the stored Version. -// Deletes of previously deleted or non-existent resource are no-ops. -// Returns an Aborted error if the requested Version does not match the stored Version. +// TODO(spatel): Move docs to the proto file func (s *Server) Delete(ctx context.Context, req *pbresource.DeleteRequest) (*pbresource.DeleteResponse, error) { reg, err := s.resolveType(req.Id.Type) if err != nil { @@ -37,23 +39,30 @@ func (s *Server) Delete(ctx context.Context, req *pbresource.DeleteRequest) (*pb return nil, status.Errorf(codes.Internal, "failed write acl: %v", err) } - versionToDelete := req.Version - if versionToDelete == "" { - // Delete resource regardless of the stored Version. Hence, strong read - // necessary to get latest Version + // The storage backend requires a Version and Uid to delete a resource based + // on CAS semantics. When either are not provided, the resource must be read + // with a strongly consistent read to retrieve either or both. + // + // n.b.: There is a chance DeleteCAS may fail with a storage.ErrCASFailure + // if an update occurs between the Read and DeleteCAS. Consider refactoring + // to use retryCAS() similar to the Write endpoint to close this gap. + deleteVersion := req.Version + deleteId := req.Id + if deleteVersion == "" || deleteId.Uid == "" { existing, err := s.Backend.Read(ctx, storage.StrongConsistency, req.Id) switch { case err == nil: - versionToDelete = existing.Version + deleteVersion = existing.Version + deleteId = existing.Id case errors.Is(err, storage.ErrNotFound): - // deletes are idempotent so no-op if resource not found + // Deletes are idempotent so no-op when not found return &pbresource.DeleteResponse{}, nil default: return nil, status.Errorf(codes.Internal, "failed read: %v", err) } } - err = s.Backend.DeleteCAS(ctx, req.Id, versionToDelete) + err = s.Backend.DeleteCAS(ctx, deleteId, deleteVersion) switch { case err == nil: return &pbresource.DeleteResponse{}, nil diff --git a/agent/grpc-external/services/resource/delete_test.go b/agent/grpc-external/services/resource/delete_test.go index 6d6dec75b..9b68de93a 100644 --- a/agent/grpc-external/services/resource/delete_test.go +++ b/agent/grpc-external/services/resource/delete_test.go @@ -80,20 +80,21 @@ func TestDelete_Success(t *testing.T) { t.Run(desc, func(t *testing.T) { server, client, ctx := testDeps(t) demo.Register(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, &pbresource.DeleteRequest{ - Id: rsp.Resource.Id, - Version: tc.versionFn(rsp.Resource), - }) + _, err = client.Delete(ctx, tc.deleteReqFn(artist)) require.NoError(t, err) // verify deleted - _, err = server.Backend.Read(ctx, storage.StrongConsistency, rsp.Resource.Id) + _, err = server.Backend.Read(ctx, storage.StrongConsistency, artistId) require.Error(t, err) require.ErrorIs(t, err, storage.ErrNotFound) }) @@ -111,7 +112,7 @@ func TestDelete_NotFound(t *testing.T) { require.NoError(t, err) // verify delete of non-existant or already deleted resource is a no-op - _, err = client.Delete(ctx, &pbresource.DeleteRequest{Id: artist.Id, Version: tc.versionFn(artist)}) + _, err = client.Delete(ctx, tc.deleteReqFn(artist)) require.NoError(t, err) }) } @@ -141,20 +142,31 @@ func testDeps(t *testing.T) (*Server, pbresource.ResourceServiceClient, context. } type deleteTestCase struct { - // returns the version to use in the test given the passed in resource - versionFn func(*pbresource.Resource) string + deleteReqFn func(r *pbresource.Resource) *pbresource.DeleteRequest } func deleteTestCases() map[string]deleteTestCase { return map[string]deleteTestCase{ - "specific version": { - versionFn: func(r *pbresource.Resource) string { - return r.Version + "version and uid": { + deleteReqFn: func(r *pbresource.Resource) *pbresource.DeleteRequest { + return &pbresource.DeleteRequest{Id: r.Id, Version: r.Version} }, }, - "empty version": { - versionFn: func(r *pbresource.Resource) string { - return "" + "version only": { + deleteReqFn: func(r *pbresource.Resource) *pbresource.DeleteRequest { + r.Id.Uid = "" + return &pbresource.DeleteRequest{Id: r.Id, Version: r.Version} + }, + }, + "uid only": { + deleteReqFn: func(r *pbresource.Resource) *pbresource.DeleteRequest { + return &pbresource.DeleteRequest{Id: r.Id, Version: ""} + }, + }, + "no version or uid": { + deleteReqFn: func(r *pbresource.Resource) *pbresource.DeleteRequest { + r.Id.Uid = "" + return &pbresource.DeleteRequest{Id: r.Id, Version: ""} }, }, }