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

refactor(aws-auth): replace aws-iam-authenticator with aws eks get-token #362

Merged
merged 1 commit into from
Apr 7, 2020

Conversation

metral
Copy link
Contributor

@metral metral commented Mar 30, 2020

Proposed changes

refactor(aws-auth): replace aws-iam-authenticator with aws eks get-token.

Note: for existing clusters, this change will recompute the kubeconfig used,
as its arguments and settings get updated to work with aws eks get-token.
It should not affect cluster access.

Examples of kubeconfig changes:

image
image

Related issues (optional)

Closes #304

Copy link
Member

@lblackstone lblackstone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change makes sense, but updating the kubeconfig will show a replace for the cluster and resources on the cluster during preview. It shouldn't actually cause a replacement, but we should make sure to check this behavior and maybe proactively notify users to expect that.

@metral
Copy link
Contributor Author

metral commented Mar 30, 2020

but updating the kubeconfig will show a replace for the cluster and resources on the cluster during preview

Not sure I'm following, could you elaborate?

Here's the preview of an update on an existing cluster from examples/cluster, the same for the examples from the OP. This PR updates the kubeconfig and it's dependents like the provider, it does not replace it.

image

@lblackstone
Copy link
Member

For the pulumi-kubernetes provider, if the kubeconfig changes in a first-class Provider resource, the preview will show a replace for resources using that Provider.

This wouldn't be the case if the EKS cluster was managed in a separate stack, as this only happens when the kubeconfig is a computed value during preview.

@metral
Copy link
Contributor Author

metral commented Mar 30, 2020

if the kubeconfig changes in a first-class Provider resource, the preview will show a replace for resources using that Provider.

Ah, right. Got it now.
Seems that the conservative replace is related to #338.

Other than the CHANGELOG updates in the PR, and notifying the community in Slack, any other suggestions for how best to roll this out and inform users that are not watching either or both?

@lblackstone
Copy link
Member

Other than the CHANGELOG updates in the PR, and notifying the community in Slack, any other suggestions for how best to roll this out and inform users that are not watching either or both?

I think that's sufficient. If users have questions about it, you can point them to the CHANGELOG as a canonical reference.

@metral
Copy link
Contributor Author

metral commented Mar 31, 2020

@lukehoban Feedback?

@metral
Copy link
Contributor Author

metral commented Apr 2, 2020

if the kubeconfig changes in a first-class Provider resource, the preview will show a replace for resources using that Provider.

In practice cluster access is not affected, but because the kubeconfig changes, it's dependencies from the k8s provider on will be signaled to be replaced during previews.

Thinking through this more, it seems best to signal this change along with the rest of the unreleased improvements and pending PRs by bumping and cutting a new minor release: 0.19.0.

Do folks agree?

Copy link
Member

@lukehoban lukehoban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am very nervous about the fact this will look like all resources are being replaced. This will be nontrivially disruptive for every user of this package.

I do think we should prioritize figuring out how to improve the kubernetes provider to allow these sorts of changes without previews thinking everything needs to replace.

But in the immediate term - can we include a clear message in README that indicates users may see kubernetes.Provider replacements during preview when updating to this version, but that replacements will not be required in practice. (Are we sure that is true?)

README.md Outdated Show resolved Hide resolved
@lblackstone
Copy link
Member

replacements will not be required in practice. (Are we sure that is true?)

As long as the actual cluster doesn't change, it will not replace the resources. The problem with preview is that the value of the kubeconfig isn't computed yet, so we can't tell for certain if the resources still exist, hence the replacement.

@metral
Copy link
Contributor Author

metral commented Apr 3, 2020

But in the immediate term - can we include a clear message in README that indicates users may see kubernetes.Provider replacements during preview when updating to this version, but that replacements will not be required in practice. (Are we sure that is true?)

@lblackstone @lukehoban

Tests were conducted with two stacks using latest upstream p/eks v0.18.24

  1. EKS cluster & k8s resources (k8s.Deployment) co-located in the same stack, and
  2. a k8s resources (kx.Secret & k8s.Deployment) only stack, that ref'd and deployed into the cluster from the 1st stack.

Neither preview nor updates for either stack with this PR showed a replacement of any k8s resources, only updates to the kubeconfig, and the provider.

It does not appear that this PR change hits the conservative replacement for k8s resources when the kubeconfig changes. IIUC this may be because only the auth exec commands change from the aws-iam-authenticator to aws eks get-token, and not the cluster name and endpoint, which I believe is what p/k8s anchors on - is this correct Levi?


01-cluster preview & update:

image


02-apps preview & update:

image

@lblackstone
Copy link
Member

not the cluster name and endpoint, which I believe is what p/k8s anchors on - is this correct Levi?

Yes, that's right. Thanks for verifying.

@metral metral force-pushed the metral/eks-get-token branch 2 times, most recently from 84049ac to 7783356 Compare April 3, 2020 17:43
@metral
Copy link
Contributor Author

metral commented Apr 3, 2020

Given the above tests, do we feel confident merging this?

Thinking through this more, it seems best to signal this change along with the rest of the unreleased improvements and pending PRs by bumping and cutting a new minor release: 0.19.0.

We could always cut a new release and highlight this change in the CHANGELOG more prominently if that would help signal users.

@lblackstone
Copy link
Member

I double-checked the logic in the k8s provider code, and it will preview a replace iff the kubeconfig is a computed value during preview.

However, Mike's test scenarios seem to show that this the kubeconfig value is resolved by the time this logic runs, both for the separate stack and single stack cases.

Here's the relevant logic in the k8s provider:

https://github.com/pulumi/pulumi-kubernetes/blob/39e2c0fc8d9ad3de5752bc2c0a2cdcadb0f50b7f/pkg/provider/provider.go#L270-L278

https://github.com/pulumi/pulumi-kubernetes/blob/39e2c0fc8d9ad3de5752bc2c0a2cdcadb0f50b7f/pkg/provider/provider.go#L314-L339

@lblackstone
Copy link
Member

I tested this locally as well, and wasn't able to cause a replace in preview. I think this is safe to merge.

Note: for existing clusters, this change will recompute the kubeconfig used,
as its arguments and settings get updated to work with `aws eks get-token`.
It should not affect cluster access.
metral added a commit that referenced this pull request Apr 21, 2020
metral added a commit that referenced this pull request Apr 21, 2020
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.

Replace dependency on aws-iam-authenticator with aws cli
3 participants