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

Revert "Revert "Removing dependency on Authenticator binary" (#446)" #653

Closed
wants to merge 1 commit into from

Conversation

toabctl
Copy link

@toabctl toabctl commented Apr 22, 2021

This reintroduces the switch to use "aws eks get-token" instead of the
aws-iam-authenticator. The reason (see [0]) why that switch got
reverted was, that eksctl wasn't able to handle the situation where
aws-iam-authenticator was not there. But that changed (see [1] and
[2]) so switching to aws-cli for getting a token should be good now.

This reverts commit d6e021b.

[0] #446
[1] eksctl-io/eksctl#788
[2] eksctl-io/eksctl#993

…#446)"

This reintroduces the switch to use "aws eks get-token" instead of the
aws-iam-authenticator. The reason (see [0]) why that switch got
reverted was, that eksctl wasn't able to handle the situation where
aws-iam-authenticator was not there. But that changed (see [1] and
[2]) so switching to aws-cli for getting a token should be good now.

This reverts commit d6e021b.

[0] awslabs#446
[1] eksctl-io/eksctl#788
[2] eksctl-io/eksctl#993
@toabctl
Copy link
Author

toabctl commented Apr 23, 2021

@abeer91 @M00nF1sh could you have a look please given that you did the commit and review for the revert?

@toabctl
Copy link
Author

toabctl commented May 12, 2021

Friendly ping, @abeer91 @M00nF1sh

@toabctl
Copy link
Author

toabctl commented May 27, 2021

anybody else who could have a look here?

@toabctl
Copy link
Author

toabctl commented Aug 25, 2021

friendly ping ... what's the problem with looking into this?

@M00nF1sh
Copy link
Member

/lgtm

@toabctl thanks for contributing and sorry for the late response. The pings just get buried in hundreds of emails :(.
I'll ping our worker AMI team internally to make the final decision.

@toabctl
Copy link
Author

toabctl commented Dec 5, 2021

@M00nF1sh another friendly ping after some more month....

@toabctl
Copy link
Author

toabctl commented May 17, 2022

@M00nF1sh friendly ping. it's now half a year. can you please review this?

@M00nF1sh
Copy link
Member

@toabctl hmm, just pinged peers in nodes team to review this PR.
seems this is a breaking change on for certain eksctl versions, i'll let ppl from nodes teams to comment on this

@mmerkes
Copy link
Member

mmerkes commented Sep 14, 2022

Sorry this PR has been languishing. Overall, I like the change. However, there is one blocker. For K8s version 1.24, client.authentication.k8s.io/v1alpha1 is no longer supported, so we now have a dependency on this AWS CLI change. The AmazonLinux yum repos are using an old AWS CLI version, which doesn't include that change. They're working on upgrading the CLI soon, and when it's available, our AMIs will automatically pull it in and we can go forward with this change.

If you can update this PR with client.authentication.k8s.io/v1beta1, we'll revisit this as soon as the AWS CLI is updated and try to merge this.

@cartermckinnon We on the same page here?

@mmerkes
Copy link
Member

mmerkes commented Sep 14, 2022

Actually, I'll update the API version as part of this issue, so you'll just need to rebase after that.

@toabctl
Copy link
Author

toabctl commented Sep 20, 2022

Actually, I'll update the API version as part of this issue, so you'll just need to rebase after that.

ack

@mmerkes
Copy link
Member

mmerkes commented Nov 1, 2022

We've been discussing this internally. The blocker has been that the awscli version in the yum repos is fairly out of date, and while we're working on installing the awscli via another method in this PR, it's not an option in all regions. We have limited control over the availability of the awscli, but we do have control over the authenticator availability as we publish those binaries. For that reason, I don't think we're going to go this direction. This issue could be resolved soon, but we'd be in the same boat if the API updated again. We like the simplification and reduced dependencies here, but we may still need to provide the authenticator here if other customers possibly depend on that.

I'm going to close this for now. If there's advantages for doing this other than the simplification, let me know and we can reconsider.

@mmerkes mmerkes closed this Nov 1, 2022
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

Successfully merging this pull request may close these issues.

3 participants