From dfe259b209d03ace265102b0a1fb407de66536dd Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Wed, 3 Jul 2024 15:57:20 -0400 Subject: [PATCH] CLI: fix prefix matching across multiple commands Several commands that inspect objects where the names are user-controlled share a bug where the user cannot inspect the object if it has a name that is an exact prefix of the name of another object (in the same namespace, where applicable). For example, the object "test" can't be inspected if there's an object with the name "testing". Copy existing logic we have for jobs, node pools, etc. to the impacted commands: * `plugin status` * `quota inspect` * `quota status` * `scaling policy info` * `service info` * `volume deregister` * `volume detach` * `volume status` If we get multiple objects for the prefix query, we check if any of them are an exact match and use that object instead of returning an error. Where possible because the prefix query signatures are the same, use a generic function that can be shared across multiple commands. Fixes: https://github.com/hashicorp/nomad/issues/13920 Fixes: https://github.com/hashicorp/nomad/issues/17132 Fixes: https://github.com/hashicorp/nomad/issues/23236 Ref: https://hashicorp.atlassian.net/browse/NET-10054 Ref: https://hashicorp.atlassian.net/browse/NET-10055 --- .changelog/23502.txt | 19 ++++++++ command/helpers.go | 37 +++++++++++++++ command/helpers_test.go | 71 +++++++++++++++++++++++++++ command/plugin_status_csi.go | 32 ++++++------- command/quota_inspect.go | 3 +- command/quota_status.go | 23 +++++---- command/scaling_policy_info.go | 25 +++++----- command/scaling_policy_info_test.go | 4 +- command/service_info.go | 74 ++++++++++++++++++++++++----- command/volume_deregister.go | 34 +++++++------ command/volume_detach.go | 34 +++++++------ command/volume_status_csi.go | 60 ++++++++--------------- 12 files changed, 287 insertions(+), 129 deletions(-) create mode 100644 .changelog/23502.txt diff --git a/.changelog/23502.txt b/.changelog/23502.txt new file mode 100644 index 00000000000..d676cc88d37 --- /dev/null +++ b/.changelog/23502.txt @@ -0,0 +1,19 @@ +```release-note:bug +cli: Fixed bug where the `plugin status` command would fail if the plugin ID was a prefix of another plugin ID +``` + +```release-note:bug +cli: Fixed bug where the `quota status` and `quota inspect` commands would fail if the quota name was a prefix of another quota name +``` + +```release-note:bug +cli: Fixed bug where the `scaling policy info` command would fail if the policy ID was a prefix of another policy ID +``` + +```release-note:bug +cli: Fixed bug where the `service info` command would fail if the service name was a prefix of another service name in the same namespace +``` + +```release-note:bug +cli: Fixed bug where the `volume deregister`, `volume detach`, and `volume status` commands would fail if the volume ID was a prefix of another volume ID in the same namespace +``` diff --git a/command/helpers.go b/command/helpers.go index d89c21e71e8..fbc5fadca10 100644 --- a/command/helpers.go +++ b/command/helpers.go @@ -765,3 +765,40 @@ func isTty() bool { _, isStdoutTerminal := term.GetFdInfo(os.Stdout) return isStdinTerminal && isStdoutTerminal } + +// getByPrefix makes a prefix list query and tries to find an exact match if +// available, or returns a list of options if multiple objects match the prefix +// and there's no exact match +func getByPrefix[T any]( + objName string, + queryFn func(*api.QueryOptions) ([]*T, *api.QueryMeta, error), + prefixCompareFn func(obj *T, prefix string) bool, + opts *api.QueryOptions, +) (*T, []*T, error) { + objs, _, err := queryFn(opts) + if err != nil { + return nil, nil, fmt.Errorf("Error querying %s: %s", objName, err) + } + switch len(objs) { + case 0: + return nil, nil, fmt.Errorf("No %s with prefix or ID %q found", objName, opts.Prefix) + case 1: + return objs[0], nil, nil + default: + // List queries often sort by by CreateIndex, not by ID, so we need to + // search for exact matches but account for multiple exact ID matches + // across namespaces + var match *T + exactMatchesCount := 0 + for _, obj := range objs { + if prefixCompareFn(obj, opts.Prefix) { + exactMatchesCount++ + match = obj + } + } + if exactMatchesCount == 1 { + return match, nil, nil + } + return nil, objs, nil + } +} diff --git a/command/helpers_test.go b/command/helpers_test.go index 25402b92432..5d5645b54d2 100644 --- a/command/helpers_test.go +++ b/command/helpers_test.go @@ -5,6 +5,7 @@ package command import ( "bytes" + "errors" "fmt" "io" "net/http" @@ -731,3 +732,73 @@ func Test_extractJobSpecEnvVars(t *testing.T) { }, result) }) } + +// TestHelperGetByPrefix exercises the generic getByPrefix function used by +// commands to find a single match by prefix or return matching results if there +// are multiple +func TestHelperGetByPrefix(t *testing.T) { + + type testStub struct{ ID string } + + testCases := []struct { + name string + queryObjs []*testStub + queryErr error + queryPrefix string + + expectMatch *testStub + expectPossible []*testStub + expectErr string + }{ + { + name: "query error", + queryErr: errors.New("foo"), + expectErr: "Error querying stubs: foo", + }, + { + name: "multiple prefix matches with exact match", + queryObjs: []*testStub{ + {ID: "testing"}, + {ID: "testing123"}, + }, + queryPrefix: "testing", + expectMatch: &testStub{ID: "testing"}, + }, + { + name: "multiple prefix matches no exact match", + queryObjs: []*testStub{ + {ID: "testing"}, + {ID: "testing123"}, + }, + queryPrefix: "test", + expectPossible: []*testStub{{ID: "testing"}, {ID: "testing123"}}, + }, + { + name: "no matches", + queryObjs: []*testStub{}, + queryPrefix: "test", + expectErr: "No stubs with prefix or ID \"test\" found", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + + match, possible, err := getByPrefix[testStub]("stubs", + func(*api.QueryOptions) ([]*testStub, *api.QueryMeta, error) { + return tc.queryObjs, nil, tc.queryErr + }, + func(stub *testStub, prefix string) bool { return stub.ID == prefix }, + &api.QueryOptions{Prefix: tc.queryPrefix}) + + if tc.expectErr != "" { + must.EqError(t, err, tc.expectErr) + } else { + must.NoError(t, err) + must.Eq(t, tc.expectMatch, match, must.Sprint("expected exact match")) + must.Eq(t, tc.expectPossible, possible, must.Sprint("expected prefix matches")) + } + }) + } + +} diff --git a/command/plugin_status_csi.go b/command/plugin_status_csi.go index 2e5231d13e7..bbc69a0857a 100644 --- a/command/plugin_status_csi.go +++ b/command/plugin_status_csi.go @@ -40,28 +40,28 @@ func (c *PluginStatusCommand) csiStatus(client *api.Client, id string) int { return 0 } - // filter by plugin if a plugin ID was passed - plugs, _, err := client.CSIPlugins().List(&api.QueryOptions{Prefix: id}) + // get a CSI plugin that matches the given prefix or a list of all matches if an + // exact match is not found. + pluginStub, possible, err := getByPrefix[api.CSIPluginListStub]("plugins", client.CSIPlugins().List, + func(plugin *api.CSIPluginListStub, prefix string) bool { return plugin.ID == prefix }, + &api.QueryOptions{ + Prefix: id, + Namespace: c.namespace, + }) if err != nil { - c.Ui.Error(fmt.Sprintf("Error querying CSI plugins: %s", err)) + c.Ui.Error(fmt.Sprintf("Error listing plugins: %s", err)) return 1 } - if len(plugs) == 0 { - c.Ui.Error(fmt.Sprintf("No plugins(s) with prefix or ID %q found", id)) - return 1 - } - if len(plugs) > 1 { - if id != plugs[0].ID { - out, err := c.csiFormatPlugins(plugs) - if err != nil { - c.Ui.Error(fmt.Sprintf("Error formatting: %s", err)) - return 1 - } - c.Ui.Error(fmt.Sprintf("Prefix matched multiple plugins\n\n%s", out)) + if len(possible) > 0 { + out, err := c.csiFormatPlugins(possible) + if err != nil { + c.Ui.Error(fmt.Sprintf("Error formatting: %s", err)) return 1 } + c.Ui.Error(fmt.Sprintf("Prefix matched multiple plugins\n\n%s", out)) + return 1 } - id = plugs[0].ID + id = pluginStub.ID // Lookup matched a single plugin plug, _, err := client.CSIPlugins().Info(id, nil) diff --git a/command/quota_inspect.go b/command/quota_inspect.go index a7a077a47f6..f6a510186b5 100644 --- a/command/quota_inspect.go +++ b/command/quota_inspect.go @@ -93,9 +93,8 @@ func (c *QuotaInspectCommand) Run(args []string) int { return 1 } - // Do a prefix lookup quotas := client.Quotas() - spec, possible, err := getQuota(quotas, name) + spec, possible, err := getQuotaByPrefix(quotas, name) if err != nil { c.Ui.Error(fmt.Sprintf("Error retrieving quota: %s", err)) return 1 diff --git a/command/quota_status.go b/command/quota_status.go index a396de87217..b3f9e257c33 100644 --- a/command/quota_status.go +++ b/command/quota_status.go @@ -32,10 +32,10 @@ General Options: ` + generalOptionsUsage(usageOptsDefault) + ` Status Specific Options: - + -json Output the latest quota status information in a JSON format. - + -t Format and display quota status information using a Go template. ` @@ -91,14 +91,12 @@ func (c *QuotaStatusCommand) Run(args []string) int { return 1 } - // Do a prefix lookup quotas := client.Quotas() - spec, possible, err := getQuota(quotas, name) + spec, possible, err := getQuotaByPrefix(quotas, name) if err != nil { c.Ui.Error(fmt.Sprintf("Error retrieving quota: %s", err)) return 1 } - if len(possible) != 0 { c.Ui.Error(fmt.Sprintf("Prefix matched multiple quotas\n\n%s", formatQuotaSpecs(possible))) return 1 @@ -252,20 +250,25 @@ func formatQuotaLimitInt(value *int) string { return strconv.Itoa(v) } -func getQuota(client *api.Quotas, quota string) (match *api.QuotaSpec, possible []*api.QuotaSpec, err error) { +func getQuotaByPrefix(client *api.Quotas, quota string) (match *api.QuotaSpec, possible []*api.QuotaSpec, err error) { // Do a prefix lookup quotas, _, err := client.PrefixList(quota, nil) if err != nil { return nil, nil, err } - l := len(quotas) - switch { - case l == 0: + switch len(quotas) { + case 0: return nil, nil, fmt.Errorf("Quota %q matched no quotas", quota) - case l == 1: + case 1: return quotas[0], nil, nil default: + // find exact match if possible + for _, q := range quotas { + if q.Name == quota { + return q, nil, nil + } + } return nil, quotas, nil } } diff --git a/command/scaling_policy_info.go b/command/scaling_policy_info.go index 67d415ce475..d8b62251f43 100644 --- a/command/scaling_policy_info.go +++ b/command/scaling_policy_info.go @@ -141,24 +141,27 @@ func (s *ScalingPolicyInfoCommand) Run(args []string) int { } policyID = sanitizeUUIDPrefix(policyID) - policies, _, err := client.Scaling().ListPolicies(&api.QueryOptions{ - Prefix: policyID, - }) + + // get a policy that matches the given prefix or a list of all matches if an + // exact match is not found. + policyStub, possible, err := getByPrefix[api.ScalingPolicyListStub]("scaling policies", + client.Scaling().ListPolicies, + func(policy *api.ScalingPolicyListStub, prefix string) bool { return policy.ID == prefix }, + &api.QueryOptions{ + Prefix: policyID, + }) if err != nil { s.Ui.Error(fmt.Sprintf("Error querying scaling policy: %v", err)) return 1 } - if len(policies) == 0 { - s.Ui.Error(fmt.Sprintf("No scaling policies with prefix or id %q found", policyID)) - return 1 - } - if len(policies) > 1 { - out := formatScalingPolicies(policies, length) + if len(possible) > 0 { + out := formatScalingPolicies(possible, length) s.Ui.Error(fmt.Sprintf("Prefix matched multiple scaling policies\n\n%s", out)) - return 0 + return 1 } + policyID = policyStub.ID - policy, _, err := client.Scaling().GetPolicy(policies[0].ID, nil) + policy, _, err := client.Scaling().GetPolicy(policyID, nil) if err != nil { s.Ui.Error(fmt.Sprintf("Error querying scaling policy: %s", err)) return 1 diff --git a/command/scaling_policy_info_test.go b/command/scaling_policy_info_test.go index 7346c7834bf..2150911d212 100644 --- a/command/scaling_policy_info_test.go +++ b/command/scaling_policy_info_test.go @@ -58,8 +58,8 @@ func TestScalingPolicyInfoCommand_Run(t *testing.T) { if code := cmd.Run([]string{"-address=" + url, "scaling_policy_info"}); code != 1 { t.Fatalf("expected cmd run exit code 1, got: %d", code) } - if out := ui.ErrorWriter.String(); !strings.Contains(out, `No scaling policies with prefix or id "scaling_policy_inf" found`) { - t.Fatalf("expected 'no policies found' within output: %v", out) + if out := ui.ErrorWriter.String(); !strings.Contains(out, `No scaling policies with prefix or ID "scaling_policy_inf" found`) { + t.Fatalf("expected 'No scaling policies with prefix' within output: %v", out) } // Generate a test job. diff --git a/command/service_info.go b/command/service_info.go index c4880dc80a4..a91126499d4 100644 --- a/command/service_info.go +++ b/command/service_info.go @@ -123,23 +123,15 @@ func (s *ServiceInfoCommand) Run(args []string) int { Prefix: serviceID, Namespace: ns, } - services, _, err := client.Services().List(&opts) + + ns, serviceID, possible, err := getServiceByPrefix(client.Services(), &opts) if err != nil { s.Ui.Error(fmt.Sprintf("Error listing service registrations: %s", err)) return 1 } - switch len(services) { - case 0: - s.Ui.Error(fmt.Sprintf("No service registrations with prefix %q found", serviceID)) - return 1 - case 1: - ns = services[0].Namespace - if len(services[0].Services) > 0 { // should always be valid - serviceID = services[0].Services[0].ServiceName - } - default: + if len(possible) > 0 { s.Ui.Error(fmt.Sprintf("Prefix matched multiple services\n\n%s", - formatServiceListOutput(s.Meta.namespace, services))) + formatServiceListOutput(s.Meta.namespace, possible))) return 1 } @@ -294,3 +286,61 @@ func argsWithNewPageToken(osArgs []string, nextToken string) string { } return strings.Join(newArgs, " ") } + +func getServiceByPrefix(client *api.Services, opts *api.QueryOptions) (ns, id string, possible []*api.ServiceRegistrationListStub, err error) { + possible, _, err = client.List(opts) + if err != nil { + return + } + + switch len(possible) { + case 0: + err = fmt.Errorf("No service registrations with prefix %q found", opts.Prefix) + return + case 1: // single namespace + ns = possible[0].Namespace + services := possible[0].Services + switch len(services) { + case 0: + // should never happen because we should never get an empty stub + err = fmt.Errorf("No service registrations with prefix %q found", opts.Prefix) + return + case 1: + id = services[0].ServiceName + possible = nil + return + default: + for _, service := range services { + if service.ServiceName == opts.Prefix { // exact match + id = service.ServiceName + possible = nil + return + } + } + return + } + default: // multiple namespaces, so we passed '*' namespace arg + exactMatchesCount := 0 + for _, stub := range possible { + for _, service := range stub.Services { + if service.ServiceName == opts.Prefix { + id = service.ServiceName + exactMatchesCount++ + continue + } + } + } + switch exactMatchesCount { + case 0: + // should never happen because we should never get an empty stub + err = fmt.Errorf("No service registrations with prefix %q found", opts.Prefix) + return + case 1: + possible = nil + return + default: + return + } + } + +} diff --git a/command/volume_deregister.go b/command/volume_deregister.go index ab625a1b108..c4c78cfcc2d 100644 --- a/command/volume_deregister.go +++ b/command/volume_deregister.go @@ -5,7 +5,6 @@ package command import ( "fmt" - "sort" "strings" "github.com/hashicorp/nomad/api" @@ -96,29 +95,28 @@ func (c *VolumeDeregisterCommand) Run(args []string) int { return 1 } - // Prefix search for the volume - vols, _, err := client.CSIVolumes().List(&api.QueryOptions{Prefix: volID}) + // get a CSI volume that matches the given prefix or a list of all matches if an + // exact match is not found. + volStub, possible, err := getByPrefix[api.CSIVolumeListStub]("volumes", client.CSIVolumes().List, + func(vol *api.CSIVolumeListStub, prefix string) bool { return vol.ID == prefix }, + &api.QueryOptions{ + Prefix: volID, + Namespace: c.namespace, + }) if err != nil { - c.Ui.Error(fmt.Sprintf("Error querying volumes: %s", err)) - return 1 - } - if len(vols) == 0 { - c.Ui.Error(fmt.Sprintf("No volumes(s) with prefix or ID %q found", volID)) + c.Ui.Error(fmt.Sprintf("Error listing volumes: %s", err)) return 1 } - if len(vols) > 1 { - if (volID != vols[0].ID) || (c.allNamespaces() && vols[0].ID == vols[1].ID) { - sort.Slice(vols, func(i, j int) bool { return vols[i].ID < vols[j].ID }) - out, err := csiFormatSortedVolumes(vols) - if err != nil { - c.Ui.Error(fmt.Sprintf("Error formatting: %s", err)) - return 1 - } - c.Ui.Error(fmt.Sprintf("Prefix matched multiple volumes\n\n%s", out)) + if len(possible) > 0 { + out, err := csiFormatVolumes(possible, false, "") + if err != nil { + c.Ui.Error(fmt.Sprintf("Error formatting: %s", err)) return 1 } + c.Ui.Error(fmt.Sprintf("Prefix matched multiple volumes\n\n%s", out)) + return 1 } - volID = vols[0].ID + volID = volStub.ID // Confirm the -force flag if force { diff --git a/command/volume_detach.go b/command/volume_detach.go index a6b8b72e42c..851d804a2f6 100644 --- a/command/volume_detach.go +++ b/command/volume_detach.go @@ -5,7 +5,6 @@ package command import ( "fmt" - "sort" "strings" "github.com/hashicorp/nomad/api" @@ -118,29 +117,28 @@ func (c *VolumeDetachCommand) Run(args []string) int { // given. if len(nodes) == 0 { - // Prefix search for the volume - vols, _, err := client.CSIVolumes().List(&api.QueryOptions{Prefix: volID}) + // get a CSI volume that matches the given prefix or a list of all + // matches if an exact match is not found. + volStub, possible, err := getByPrefix[api.CSIVolumeListStub]("volumes", client.CSIVolumes().List, + func(vol *api.CSIVolumeListStub, prefix string) bool { return vol.ID == prefix }, + &api.QueryOptions{ + Prefix: volID, + Namespace: c.namespace, + }) if err != nil { - c.Ui.Error(fmt.Sprintf("Error querying volumes: %s", err)) + c.Ui.Error(fmt.Sprintf("Error listing volumes: %s", err)) return 1 } - if len(vols) == 0 { - c.Ui.Error(fmt.Sprintf("No volumes(s) with prefix or ID %q found", volID)) - return 1 - } - if len(vols) > 1 { - if (volID != vols[0].ID) || (c.allNamespaces() && vols[0].ID == vols[1].ID) { - sort.Slice(vols, func(i, j int) bool { return vols[i].ID < vols[j].ID }) - out, err := csiFormatSortedVolumes(vols) - if err != nil { - c.Ui.Error(fmt.Sprintf("Error formatting: %s", err)) - return 1 - } - c.Ui.Error(fmt.Sprintf("Prefix matched multiple volumes\n\n%s", out)) + if len(possible) > 0 { + out, err := csiFormatVolumes(possible, false, "") + if err != nil { + c.Ui.Error(fmt.Sprintf("Error formatting: %s", err)) return 1 } + c.Ui.Error(fmt.Sprintf("Prefix matched multiple volumes\n\n%s", out)) + return 1 } - volID = vols[0].ID + volID = volStub.ID vol, _, err := client.CSIVolumes().Info(volID, nil) if err != nil { diff --git a/command/volume_status_csi.go b/command/volume_status_csi.go index 60df37aa08c..85d302743c2 100644 --- a/command/volume_status_csi.go +++ b/command/volume_status_csi.go @@ -26,50 +26,30 @@ func (c *VolumeStatusCommand) csiStatus(client *api.Client, id string) int { return c.listVolumes(client) } - // Prefix search for the volume - vols, _, err := client.CSIVolumes().List(&api.QueryOptions{Prefix: id}) + // get a CSI volume that matches the given prefix or a list of all matches if an + // exact match is not found. + volStub, possible, err := getByPrefix[api.CSIVolumeListStub]("volumes", client.CSIVolumes().List, + func(vol *api.CSIVolumeListStub, prefix string) bool { return vol.ID == prefix }, + &api.QueryOptions{ + Prefix: id, + Namespace: c.namespace, + }) if err != nil { - c.Ui.Error(fmt.Sprintf("Error querying volumes: %s", err)) - return 1 - } - if len(vols) == 0 { - c.Ui.Error(fmt.Sprintf("No volumes(s) with prefix or ID %q found", id)) + c.Ui.Error(fmt.Sprintf("Error listing volumes: %s", err)) return 1 } - - var ns string - - if len(vols) == 1 { - // need to set id from the actual ID because it might be a prefix - id = vols[0].ID - ns = vols[0].Namespace - } - - // List sorts by CreateIndex, not by ID, so we need to search for - // exact matches but account for multiple exact ID matches across - // namespaces - if len(vols) > 1 { - exactMatchesCount := 0 - for _, vol := range vols { - if vol.ID == id { - exactMatchesCount++ - ns = vol.Namespace - } - } - if exactMatchesCount != 1 { - out, err := c.csiFormatVolumes(vols) - if err != nil { - c.Ui.Error(fmt.Sprintf("Error formatting: %s", err)) - return 1 - } - c.Ui.Error(fmt.Sprintf("Prefix matched multiple volumes\n\n%s", out)) + if len(possible) > 0 { + out, err := csiFormatVolumes(possible, c.json, c.template) + if err != nil { + c.Ui.Error(fmt.Sprintf("Error formatting: %s", err)) return 1 } + c.Ui.Error(fmt.Sprintf("Prefix matched multiple volumes\n\n%s", out)) + return 1 } // Try querying the volume - client.SetNamespace(ns) - vol, _, err := client.CSIVolumes().Info(id, nil) + vol, _, err := client.CSIVolumes().Info(volStub.ID, nil) if err != nil { c.Ui.Error(fmt.Sprintf("Error querying volume: %s", err)) return 1 @@ -98,7 +78,7 @@ func (c *VolumeStatusCommand) listVolumes(client *api.Client) int { // No output if we have no volumes c.Ui.Error("No CSI volumes") } else { - str, err := c.csiFormatVolumes(vols) + str, err := csiFormatVolumes(vols, c.json, c.template) if err != nil { c.Ui.Error(fmt.Sprintf("Error formatting: %s", err)) return 1 @@ -170,12 +150,12 @@ NEXT_PLUGIN: return code } -func (c *VolumeStatusCommand) csiFormatVolumes(vols []*api.CSIVolumeListStub) (string, error) { +func csiFormatVolumes(vols []*api.CSIVolumeListStub, json bool, template string) (string, error) { // Sort the output by volume id sort.Slice(vols, func(i, j int) bool { return vols[i].ID < vols[j].ID }) - if c.json || len(c.template) > 0 { - out, err := Format(c.json, c.template, vols) + if json || len(template) > 0 { + out, err := Format(json, template, vols) if err != nil { return "", fmt.Errorf("format error: %v", err) }