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

[0.2.0-rc.0] ELB deletion fails for admins who are not cluster owners #1042

Closed
errordeveloper opened this issue Jul 22, 2019 · 11 comments

Comments

@errordeveloper
Copy link
Member

commented Jul 22, 2019

I just did this:

$ ./eksctl delete cluster --name=cluster-9 --region=eu-west-1
[ℹ]  using region eu-west-1
[ℹ]  deleting EKS cluster "cluster-9"
[ℹ]  cleaning up LoadBalancer services
[✖]  Unauthorized
$

First of all, the error message is too ambiguous, and secondly I wonder what we should about it.

Until now, anyone who has sufficient access to AWS was able to delete a cluster. New behaviour requires access to Kubernetes API, and the way things work at the moment, not every AWS admin who is capable of deleting a cluster will be able to access Kubernetes API.

Let's discuss possible ways of addressing this, but perhaps error message can be improved first anyway with more context.

@gemagomez

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

First of all, the error message is too ambiguous, and secondly I wonder what we should about it.

Let's raise an issue to fix this on a later release. This doesn't sound like a release stopper.

@gemagomez

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

Until now, anyone who has sufficient access to AWS was able to delete a cluster. New behaviour requires access to Kubernetes API, and the way things work at the moment, not every AWS admin who is capable of deleting a cluster will be able to access Kubernetes API.

Why not? what is missing?

@errordeveloper

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2019

Let's raise an issue to fix this on a later release. This doesn't sound like a release stopper.

I disagree, because this would be very simple to fix. It affects user experience, especially to do with a new feature we just added. I guarantee most users will get confused by what this error actually means.

@errordeveloper

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2019

Why not? what is missing?

Any AWS user can creates an EKS cluster if they have sufficient level of access.
Once they created a cluster, only they have access to Kubernetes API.
They are required to take extra steps in order to allow other users in their organisation to access the Kubernetes API, and that is entirely up to them.

Generally speaking, until now anyone who has sufficient access to delete resources in AWS was able to attempt deleting a cluster (albeit in some cases it did go smoothly, but happy-path worked). #1010 broke this, by requiring access to Kubernetes API for deleting resource in Kubernetes, so in most cases only cluster owner is no capable of attempting cluster deletion.

@gemagomez

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

I disagree, because this would be very simple to fix. It affects user experience, especially to do with a new feature we just added. I guarantee most users will get confused by what this error actually means.

Ok, fix it then!

@2opremio

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

I don't think it's a release stopper either, the PR already brings a lot of value as is.

I am completely in for fixing the error message though. And, if you propose to fix it completely t by simply printing a warning on the unauthorized error and giving up on ELB deletion (there may be no load balancer services to begin with) I am fine with that too (it's a minor change).

@errordeveloper

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2019

I think converting error to a warning with more context would definitely satisfy the general concern for the purpose of 0.2.0.

@2opremio

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

Actually, I am having second thoughts about that solution. We should definitely improve the error message but simply demoting the error to a warning will lead to a deletion failure later on if there are any loadbalancer services.

not every AWS admin who is capable of deleting a cluster will be able to access Kubernetes API.

After thinking about it, it doesn't make sense to me that someone has rights to destroy a cluster (be an admin at the infrastructure level) but not be able to list services. Can't we simply have that as a requirement?

@errordeveloper

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2019

@2opremio

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

Fons, I am happy for you to make a call on this, just wanted to outline the perspective as I see it at present. It is hard to judge what is better or worst, as we don't have any data to construct a more scientific argument.

It was good discussion! (the current error messages are crap and we had to fix it)

I am however, settling on simply requiring to list/delete Services when deleting a cluster for now. Maybe we can refine it later on see #1044

@marccarre

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

Resolved by #1044, now merged in release-0.2 and master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.