diff --git a/agent/grpc-external/services/resource/delete.go b/agent/grpc-external/services/resource/delete.go new file mode 100644 index 000000000..5cc4a9573 --- /dev/null +++ b/agent/grpc-external/services/resource/delete.go @@ -0,0 +1,55 @@ +package resource + +import ( + "context" + "errors" + + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + + "github.com/hashicorp/consul/internal/storage" + "github.com/hashicorp/consul/proto-public/pbresource" +) + +// TODO(spatel): Move docs to the proto file +// Deletes a resource with the given Id and Version. +// +// 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. +func (s *Server) Delete(ctx context.Context, req *pbresource.DeleteRequest) (*pbresource.DeleteResponse, error) { + // check type registered + reg, err := s.resolveType(req.Id.Type) + if err != nil { + return nil, err + } + + // TODO(spatel): reg will be used for ACL hooks + _ = reg + + versionToDelete := req.Version + if versionToDelete == "" { + // Delete resource regardless of the stored Version. Hence, strong read + // necessary to get latest Version + existing, err := s.Backend.Read(ctx, storage.StrongConsistency, req.Id) + switch { + case err == nil: + versionToDelete = existing.Version + case errors.Is(err, storage.ErrNotFound): + // deletes are idempotent so no-op if resource 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) + switch { + case err == nil: + return &pbresource.DeleteResponse{}, nil + case errors.Is(err, storage.ErrCASFailure): + return nil, status.Error(codes.Aborted, err.Error()) + default: + return nil, status.Errorf(codes.Internal, "failed delete: %v", err) + } +} diff --git a/agent/grpc-external/services/resource/delete_test.go b/agent/grpc-external/services/resource/delete_test.go new file mode 100644 index 000000000..2cbc7a8df --- /dev/null +++ b/agent/grpc-external/services/resource/delete_test.go @@ -0,0 +1,114 @@ +package resource + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + + "github.com/hashicorp/consul/internal/resource/demo" + "github.com/hashicorp/consul/internal/storage" + "github.com/hashicorp/consul/proto-public/pbresource" +) + +func TestDelete_TypeNotRegistered(t *testing.T) { + t.Parallel() + + _, client, ctx := testDeps(t) + artist, err := demo.GenerateV2Artist() + require.NoError(t, err) + + // delete artist with unregistered type + _, err = client.Delete(ctx, &pbresource.DeleteRequest{Id: artist.Id, Version: ""}) + require.Error(t, err) + require.Equal(t, codes.InvalidArgument.String(), status.Code(err).String()) +} + +func TestDelete_Success(t *testing.T) { + t.Parallel() + + for desc, tc := range deleteTestCases() { + 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) + + // delete + _, err = client.Delete(ctx, &pbresource.DeleteRequest{ + 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) + require.Error(t, err) + require.ErrorIs(t, err, storage.ErrNotFound) + }) + } +} + +func TestDelete_NotFound(t *testing.T) { + t.Parallel() + + for desc, tc := range deleteTestCases() { + t.Run(desc, func(t *testing.T) { + server, client, ctx := testDeps(t) + demo.Register(server.Registry) + artist, err := demo.GenerateV2Artist() + 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)}) + require.NoError(t, err) + }) + } +} + +func TestDelete_VersionMismatch(t *testing.T) { + t.Parallel() + + 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) + + // delete with a version that is different from the stored version + _, err = client.Delete(ctx, &pbresource.DeleteRequest{Id: rsp.Resource.Id, Version: "non-existent-version"}) + require.Error(t, err) + require.Equal(t, codes.Aborted.String(), status.Code(err).String()) + require.ErrorContains(t, err, "CAS operation failed") +} + +func testDeps(t *testing.T) (*Server, pbresource.ResourceServiceClient, context.Context) { + server := testServer(t) + client := testClient(t, server) + return server, client, context.Background() +} + +type deleteTestCase struct { + // returns the version to use in the test given the passed in resource + versionFn func(*pbresource.Resource) string +} + +func deleteTestCases() map[string]deleteTestCase { + return map[string]deleteTestCase{ + "specific version": { + versionFn: func(r *pbresource.Resource) string { + return r.Version + }, + }, + "empty version": { + versionFn: func(r *pbresource.Resource) string { + return "" + }, + }, + } +} diff --git a/agent/grpc-external/services/resource/server.go b/agent/grpc-external/services/resource/server.go index 125b2bce0..34852429d 100644 --- a/agent/grpc-external/services/resource/server.go +++ b/agent/grpc-external/services/resource/server.go @@ -55,11 +55,6 @@ func (s *Server) WriteStatus(ctx context.Context, req *pbresource.WriteStatusReq return &pbresource.WriteStatusResponse{}, nil } -func (s *Server) Delete(ctx context.Context, req *pbresource.DeleteRequest) (*pbresource.DeleteResponse, error) { - // TODO - return &pbresource.DeleteResponse{}, nil -} - //nolint:unparam func (s *Server) resolveType(typ *pbresource.Type) (*resource.Registration, error) { v, ok := s.Registry.Resolve(typ) diff --git a/agent/grpc-external/services/resource/server_test.go b/agent/grpc-external/services/resource/server_test.go index 2a9c94814..05d1e2d68 100644 --- a/agent/grpc-external/services/resource/server_test.go +++ b/agent/grpc-external/services/resource/server_test.go @@ -28,14 +28,6 @@ func TestWriteStatus_TODO(t *testing.T) { require.NotNil(t, resp) } -func TestDelete_TODO(t *testing.T) { - server := testServer(t) - client := testClient(t, server) - resp, err := client.Delete(context.Background(), &pbresource.DeleteRequest{}) - require.NoError(t, err) - require.NotNil(t, resp) -} - func testServer(t *testing.T) *Server { t.Helper()