Skip to content

Commit

Permalink
backport of commit b09c114
Browse files Browse the repository at this point in the history
  • Loading branch information
tgross authored Jul 10, 2024
1 parent fd57122 commit bdcc3cc
Show file tree
Hide file tree
Showing 12 changed files with 287 additions and 129 deletions.
19 changes: 19 additions & 0 deletions .changelog/23502.txt
Original file line number Diff line number Diff line change
@@ -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
```
37 changes: 37 additions & 0 deletions command/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
71 changes: 71 additions & 0 deletions command/helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package command

import (
"bytes"
"errors"
"fmt"
"io"
"net/http"
Expand Down Expand Up @@ -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"))
}
})
}

}
32 changes: 16 additions & 16 deletions command/plugin_status_csi.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
3 changes: 1 addition & 2 deletions command/quota_inspect.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
23 changes: 13 additions & 10 deletions command/quota_status.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
`
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
}
25 changes: 14 additions & 11 deletions command/scaling_policy_info.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions command/scaling_policy_info_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
74 changes: 62 additions & 12 deletions command/service_info.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}
}

}
Loading

0 comments on commit bdcc3cc

Please sign in to comment.