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

Force user to identify the exact cluster they want #138

Open
mmazur opened this issue Sep 9, 2020 · 5 comments
Open

Force user to identify the exact cluster they want #138

mmazur opened this issue Sep 9, 2020 · 5 comments

Comments

@mmazur
Copy link
Contributor

mmazur commented Sep 9, 2020

All cluster commands should refuse to do anything if the provided cluster handle is ambiguous (there's more than one cluster that matches).

This is currently not the case with at least describe cluster, possibly more.

@cben
Copy link
Contributor

cben commented Jul 1, 2021

Read-only commands like describe cluster could well display all matching clusters. That would be consistent with kubectl describe behavior, and it would be neccessary if we want #108.

OTOH destructive commands like ocm delete user should clearly force this 👍

Hmm, looking at GetCluster() helper, it has code to detect multiple matches but alas it requests Size(1), which means it won't see them, just silently pick the first 😨 Sending fix

@cben
Copy link
Contributor

cben commented Jul 1, 2021

Well, it's not as severe as I thought because name is unique (per org?), and can't be changed after creation, as are id and external_id. (we're not presently matching on display_name.)
So for normal users I don't see a deliberate way to cause a collision, and random collisions extremely unlikely
.

But if you are a priviledged internal user seeing multiple orgs, I'm not sure if we enforce name uniqueness between orgs? If not, then this may be bad. Anyway, fixing

@cben
Copy link
Contributor

cben commented Jul 1, 2021

Sorry, false alarm 😌
Requesting size=1 returns at most 1 back, but does count them all into the total field:

  "page": 1,
  "size": 1,
  "total": 2,

and the code of GetCluster() looks at total ✔️
I verified by changing the query so it will get multiple matches, and ocm exited with status 1 and message:

Error: Failed to get cluster '...': There are 12 clusters with identifier or name '...'

@cben
Copy link
Contributor

cben commented Jul 1, 2021

@mmazur are you aware of specific commands, other than describe, that behave wrong?

The following command all use GetCluster():

cmd/ocm/create/idp/cmd.go
cmd/ocm/create/user/cmd.go
cmd/ocm/create/upgradepolicy/cmd.go
cmd/ocm/tunnel/cmd.go
cmd/ocm/resume/cluster/cmd.go
cmd/ocm/create/machinepool/cmd.go
cmd/ocm/create/ingress/cmd.go
cmd/ocm/hibernate/cluster/cmd.go
cmd/ocm/edit/cluster/cmd.go
cmd/ocm/edit/machinepool/cmd.go
cmd/ocm/list/idp/cmd.go
cmd/ocm/list/addon/cmd.go
cmd/ocm/list/user/cmd.go
cmd/ocm/list/upgradepolicy/cmd.go
cmd/ocm/list/ingress/cmd.go
cmd/ocm/list/machinepool/cmd.go
cmd/ocm/cluster/login/login.go
cmd/ocm/delete/machinepool/cmd.go
cmd/ocm/edit/ingress/cmd.go
cmd/ocm/delete/idp/cmd.go
cmd/ocm/delete/user/cmd.go
cmd/ocm/delete/upgradepolicy/cmd.go
cmd/ocm/delete/ingress/cmd.go

@mmazur
Copy link
Contributor Author

mmazur commented Jul 13, 2021

Nope, I'm not aware of any more issues. Maybe it's only cluster describe that needs a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants