Fix delete when uid not provided (#16996)

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.
-// Deletes a resource with the given Id and Version.
+// - 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.
+// TODO(spatel): Move docs to the proto file
-// 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.
 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
+	// The storage backend requires a Version and Uid to delete a resource based
-	if versionToDelete == "" {
+	// on CAS semantics. When either are not provided, the resource must be read
-		// Delete resource regardless of the stored Version. Hence, strong read
+	// with a strongly consistent read to retrieve either or both.
-		// necessary to get latest Version
+	//
+	// 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

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{
+			_, err = client.Delete(ctx, tc.deleteReqFn(artist))
-				Id:      rsp.Resource.Id,
-				Version: tc.versionFn(rsp.Resource),
-			})
 			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
+	deleteReqFn func(r *pbresource.Resource) *pbresource.DeleteRequest
-	versionFn func(*pbresource.Resource) string
 }
 
 func deleteTestCases() map[string]deleteTestCase {
 	return map[string]deleteTestCase{
-		"specific version": {
+		"version and uid": {
-			versionFn: func(r *pbresource.Resource) string {
+			deleteReqFn: func(r *pbresource.Resource) *pbresource.DeleteRequest {
-				return r.Version
+				return &pbresource.DeleteRequest{Id: r.Id, Version: r.Version}
 			},
 		},
-		"empty version": {
+		"version only": {
-			versionFn: func(r *pbresource.Resource) string {
+			deleteReqFn: func(r *pbresource.Resource) *pbresource.DeleteRequest {
-				return ""
+				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: ""}
 			},
 		},
 	}