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

delete command ready #18679

Merged
merged 8 commits into from
Sep 13, 2023
Merged
Show file tree
Hide file tree
Changes from 6 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
15 changes: 15 additions & 0 deletions api/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package api

import (
"fmt"
"net/http"
"strings"

"github.com/hashicorp/consul/proto-public/pbresource"
Expand Down Expand Up @@ -65,6 +66,20 @@ func (resource *Resource) Read(gvk *GVK, resourceName string, q *QueryOptions) (
return out, nil
}

func (resource *Resource) Delete(gvk *GVK, resourceName string, q *QueryOptions) error {
r := resource.c.newRequest("DELETE", strings.ToLower(fmt.Sprintf("/api/%s/%s/%s/%s", gvk.Group, gvk.Version, gvk.Kind, resourceName)))
r.setQueryOptions(q)
_, resp, err := resource.c.doRequest(r)
if err != nil {
return err
}
defer closeResponseBody(resp)
if err := requireHttpCodes(resp, http.StatusNoContent); err != nil {
return err
}
return nil
}

func (resource *Resource) Apply(gvk *GVK, resourceName string, q *QueryOptions, payload *WriteRequest) (*WriteResponse, *WriteMeta, error) {
url := strings.ToLower(fmt.Sprintf("/api/%s/%s/%s/%s", gvk.Group, gvk.Version, gvk.Kind, resourceName))

Expand Down
2 changes: 2 additions & 0 deletions command/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ import (
"github.com/hashicorp/consul/command/reload"
"github.com/hashicorp/consul/command/resource"
resourceapply "github.com/hashicorp/consul/command/resource/apply"
resourcedelete "github.com/hashicorp/consul/command/resource/delete"
resourcelist "github.com/hashicorp/consul/command/resource/list"
resourceread "github.com/hashicorp/consul/command/resource/read"
"github.com/hashicorp/consul/command/rtt"
Expand Down Expand Up @@ -244,6 +245,7 @@ func RegisteredCommands(ui cli.Ui) map[string]mcli.CommandFactory {
entry{"reload", func(ui cli.Ui) (cli.Command, error) { return reload.New(ui), nil }},
entry{"resource", func(cli.Ui) (cli.Command, error) { return resource.New(), nil }},
entry{"resource read", func(ui cli.Ui) (cli.Command, error) { return resourceread.New(ui), nil }},
entry{"resource delete", func(ui cli.Ui) (cli.Command, error) { return resourcedelete.New(ui), nil }},
entry{"resource apply", func(ui cli.Ui) (cli.Command, error) { return resourceapply.New(ui), nil }},
entry{"resource list", func(ui cli.Ui) (cli.Command, error) { return resourcelist.New(ui), nil }},
entry{"rtt", func(ui cli.Ui) (cli.Command, error) { return rtt.New(ui), nil }},
Expand Down
172 changes: 172 additions & 0 deletions command/resource/delete/delete.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: BUSL-1.1

package delete

import (
"errors"
"flag"
"fmt"

"github.com/mitchellh/cli"

"github.com/hashicorp/consul/agent/consul"
"github.com/hashicorp/consul/api"
"github.com/hashicorp/consul/command/flags"
"github.com/hashicorp/consul/command/helpers"
"github.com/hashicorp/consul/command/resource"
"github.com/hashicorp/consul/internal/resourcehcl"
)

func New(ui cli.Ui) *cmd {
c := &cmd{UI: ui}
c.init()
return c
}

type cmd struct {
UI cli.Ui
flags *flag.FlagSet
http *flags.HTTPFlags
help string

filePath string
}

func (c *cmd) init() {
c.flags = flag.NewFlagSet("", flag.ContinueOnError)
c.http = &flags.HTTPFlags{}
c.flags.StringVar(&c.filePath, "f", "", "File path with resource definition")
flags.Merge(c.flags, c.http.ClientFlags())
flags.Merge(c.flags, c.http.ServerFlags())
flags.Merge(c.flags, c.http.MultiTenancyFlags())
flags.Merge(c.flags, c.http.AddPeerName())
c.help = flags.Usage(help, c.flags)
}

func (c *cmd) Run(args []string) int {
var gvk *api.GVK
var resourceName string
var opts *api.QueryOptions

if len(args) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this check since we can rely on flags.args for required arguments

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, removed this part.

c.UI.Error("Please provide required arguments")
return 1
}

if err := c.flags.Parse(args); err != nil {
if !errors.Is(err, flag.ErrHelp) {
c.UI.Error(fmt.Sprintf("Failed to parse args: %v", err))
return 1
}
}

if c.flags.Lookup("f").Value.String() != "" {
if c.filePath != "" {
data, err := helpers.LoadDataSourceNoRaw(c.filePath, nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use the helper function ParseResourceFromFile here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, updated

if err != nil {
c.UI.Error(fmt.Sprintf("Failed to load data: %v", err))
return 1
}
parsedResource, err := resourcehcl.Unmarshal([]byte(data), consul.NewTypeRegistry())
if err != nil {
c.UI.Error(fmt.Sprintf("Failed to decode resource from input file: %v", err))
return 1
}

gvk = &api.GVK{
Group: parsedResource.Id.Type.GetGroup(),
Version: parsedResource.Id.Type.GetGroupVersion(),
Kind: parsedResource.Id.Type.GetKind(),
}
resourceName = parsedResource.Id.GetName()
opts = &api.QueryOptions{
Namespace: parsedResource.Id.Tenancy.GetNamespace(),
Partition: parsedResource.Id.Tenancy.GetPartition(),
Peer: parsedResource.Id.Tenancy.GetPeerName(),
Token: c.http.Token(),
}
} else {
c.UI.Error(fmt.Sprintf("Please provide an input file with resource definition"))
return 1
}
} else {
if len(args) < 2 {
c.UI.Error("Must specify two arguments: resource type and resource name")
return 1
}
var err error
gvk, resourceName, err = resource.GetTypeAndResourceName(args)
if err != nil {
c.UI.Error(fmt.Sprintf("Your argument format is incorrect: %s", err))
return 1
}

inputArgs := args[2:]
if err := c.flags.Parse(inputArgs); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, here we can use the helper function ParseInputParams

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, updated

if errors.Is(err, flag.ErrHelp) {
return 0
}
c.UI.Error(fmt.Sprintf("Failed to parse args: %v", err))
return 1
}
if c.filePath != "" {
c.UI.Warn("We ignored the -f flag if you provide gvk and resource name")
}
opts = &api.QueryOptions{
Namespace: c.http.Namespace(),
Partition: c.http.Partition(),
Peer: c.http.PeerName(),
Token: c.http.Token(),
}
}

client, err := c.http.APIClient()
if err != nil {
c.UI.Error(fmt.Sprintf("Error connect to Consul agent: %s", err))
return 1
}

if err := client.Resource().Delete(gvk, resourceName, opts); err != nil {
c.UI.Error(fmt.Sprintf("Error deleting resource %s.%s.%s/%s: %v", gvk.Group, gvk.Version, gvk.Kind, resourceName, err))
return 1
}

c.UI.Info(fmt.Sprintf("%s.%s.%s/%s deleted", gvk.Group, gvk.Version, gvk.Kind, resourceName))
return 0
}

func (c *cmd) Synopsis() string {
return synopsis
}

func (c *cmd) Help() string {
return flags.Usage(c.help, nil)
}

const synopsis = "Delete resource information"
const help = `
Usage: You have two options to delete the resource specified by the given
type, name, partition, namespace and peer and outputs its JSON representation.

consul resource delete [type] [name] -partition=<default> -namespace=<default> -peer=<local>
consul resource delete -f [resource_file_path]

But you could only use one of the approaches.

Example:

$ consul resource delete catalog.v1alpha1.Service card-processor -partition=billing -namespace=payments -peer=eu
$ consul resource delete -f resource.hcl

In resource.hcl, it could be:
ID {
Type = gvk("catalog.v1alpha1.Service")
Name = "card-processor"
Tenancy {
Namespace = "payments"
Partition = "billing"
PeerName = "eu"
}
}
`
138 changes: 138 additions & 0 deletions command/resource/delete/delete_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: BUSL-1.1
package delete

import (
"testing"

"github.com/mitchellh/cli"
"github.com/stretchr/testify/require"

"github.com/hashicorp/consul/agent"
"github.com/hashicorp/consul/command/resource/apply"
"github.com/hashicorp/consul/testrpc"
)

func TestResourceDeleteInvalidArgs(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a test case where the resource type is in an invalid format
example: args: []string{"test"}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, as discussed above, we throw error if it's not the format of a.b.c, and added one test case above

t.Parallel()

type tc struct {
args []string
expectedCode int
expectedErrMsg string
}

cases := map[string]tc{
"nil args": {
args: nil,
expectedCode: 1,
expectedErrMsg: "Please provide required arguments",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have consistent error messaging across all the CLI commands. The messaging should start with Your argument format is incorrect: ...

See PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good, I picked up the changes in that PR into this PR(only read.go and read_test.go)

},
"empty args": {
args: []string{},
expectedCode: 1,
expectedErrMsg: "Please provide required arguments",
},
"missing file path": {
args: []string{"-f"},
expectedCode: 1,
expectedErrMsg: "Failed to parse args: flag needs an argument: -f",
},
"provide type and name": {
args: []string{"a.b.c"},
expectedCode: 1,
expectedErrMsg: "Must specify two arguments: resource type and resource name",
},
"does not provide resource name after type": {
args: []string{"a.b.c", "-namespace", "default"},
expectedCode: 1,
expectedErrMsg: "Must provide resource name right after type",
},
}

for desc, tc := range cases {
t.Run(desc, func(t *testing.T) {
ui := cli.NewMockUi()
c := New(ui)

require.Equal(t, tc.expectedCode, c.Run(tc.args))
require.Contains(t, ui.ErrorWriter.String(), tc.expectedErrMsg)
})
}
}

func createResource(t *testing.T, a *agent.TestAgent) {
applyUi := cli.NewMockUi()
applyCmd := apply.New(applyUi)

args := []string{
"-http-addr=" + a.HTTPAddr(),
"-token=root",
}

args = append(args, []string{"-f=../testdata/demo.hcl"}...)

code := applyCmd.Run(args)
require.Equal(t, 0, code)
require.Empty(t, applyUi.ErrorWriter.String())
}

func TestResourceDelete(t *testing.T) {
if testing.Short() {
t.Skip("too slow for testing.Short")
}

t.Parallel()

a := agent.NewTestAgent(t, ``)
defer a.Shutdown()
testrpc.WaitForTestAgent(t, a.RPC, "dc1")

defaultCmdArgs := []string{
"-http-addr=" + a.HTTPAddr(),
"-token=root",
}
cases := []struct {
name string
args []string
expectedCode int
errMsg string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we are not using errMsg variable, instead we need output and verify that the message returned is group.verion.kind deleted

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for this one, I took the deleted keyword, the reason is because there are two types of tests, one is from file, the other from command line args, though it's easy to extract the information from args, do you think it's needed to collect the group.version.kind/resourceName from file? I could add it, if it's necessary

createResource bool
}{
{
name: "delete resource in hcl format",
args: []string{"-f=../testdata/demo.hcl"},
expectedCode: 0,
errMsg: "",
createResource: true,
},
{
name: "delete resource in command line format",
args: []string{"demo.v2.Artist", "korn", "-partition=default", "-namespace=default", "-peer=local"},
expectedCode: 0,
errMsg: "",
createResource: true,
},
{
name: "delete resource that doesn't exist in command line format",
args: []string{"demo.v2.Artist", "fake-korn", "-partition=default", "-namespace=default", "-peer=local"},
expectedCode: 0,
errMsg: "",
createResource: false,
},
}

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
ui := cli.NewMockUi()
c := New(ui)
cliArgs := append(tc.args, defaultCmdArgs...)
if tc.createResource {
createResource(t, a)
}
code := c.Run(cliArgs)
require.Equal(t, ui.ErrorWriter.String(), tc.errMsg)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion could be changed to require.Empty(t, ui.ErrorWriter.String())

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

require.Equal(t, tc.expectedCode, code)
})
}
}
19 changes: 19 additions & 0 deletions command/resource/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@ import (
"errors"
"flag"
"fmt"
"strings"

"github.com/hashicorp/consul/agent/consul"
"github.com/hashicorp/consul/api"
"github.com/hashicorp/consul/command/helpers"
"github.com/hashicorp/consul/internal/resourcehcl"
"github.com/hashicorp/consul/proto-public/pbresource"
Expand All @@ -35,3 +37,20 @@ func ParseInputParams(inputArgs []string, flags *flag.FlagSet) error {
}
return nil
}

func GetTypeAndResourceName(args []string) (gvk *api.GVK, resourceName string, e error) {
// it has to be resource name after the type
if strings.HasPrefix(args[1], "-") {
return nil, "", fmt.Errorf("Must provide resource name right after type")
}

s := strings.Split(args[0], ".")
gvk = &api.GVK{
Group: s[0],
Version: s[1],
Kind: s[2],
}
Comment on lines +52 to +56
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will cause a panic if the resource type is not in a.b.c format.
We need a check here to prevent that

if len(s) < 3 {
		return nil, "", fmt.Errorf("Must include resource type argument in group.verion.kind format")
	}

Also, lets add a test case for it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated, and added one more test case for it.
Notice I restrict the condition to

if len(s) != 3 {}


resourceName = args[1]
return
}
Loading