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

Update client.authorization to v1beta1 #1426

Merged

Conversation

mdsgabriel
Copy link
Member

Issue #, if available:

Description of changes:
Fix TestDockerKubernetes122AWSIamAuth" and "TestVSphereKubernetes122AWSIamAuth

client.authentication v1alpha1 has been deprecated in K8s 1.22.

Testing (if applicable):

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@eks-distro-bot eks-distro-bot added approved size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 9, 2022
@mdsgabriel mdsgabriel requested review from abhay-krishna, micahhausler, vivek-koppuru and pokearu and removed request for ewollesen March 9, 2022 23:47
@@ -16,7 +16,7 @@ users:
- name: {{.clusterName}}-aws
user:
exec:
apiVersion: client.authentication.k8s.io/v1alpha1
apiVersion: client.authentication.k8s.io/v1beta1
Copy link
Member

Choose a reason for hiding this comment

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

I'm not positive you can do this. The older versions still use the older iam auth which I believe expects the v1alpha1. Worth a sort to verify tho.

Copy link
Member Author

Choose a reason for hiding this comment

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

V1beta1 is available since k8s 1.11

This was tested by manually creating 1.21 and 1.22 clusters and using the generated kubeconfig (cluster name-aws.kubeconfig) to get the list of pods (similar to what's done by the tests that are failing).

What other scenarios should be tested?

Copy link
Member

Choose a reason for hiding this comment

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

v1beta1 apiversion has been available since k8s v1.11, but only since aws-iam-auth v0.5.4. Since 1.21 and below use aws-iam-auth versions previous to v0.5.4, we want to confirm if this change will work on them in a backward compatible manner.

Copy link
Member

Choose a reason for hiding this comment

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

This is the iam auth pr where it was changed

https://github.com/kubernetes-sigs/aws-iam-authenticator/pull/386/files

Like Abhay said, eksd kube versions less than 1.22 are still using the older iam auth which was parsing it as v1alpha. I wasn't sure if they would also work if the version was v1beta1 in the config file. If you tried a 1.21 cluster with this change and it worked, then it must be ok with it. Let's just make sure we still have the 121 test in addition to the new 122 one and then we can merge and make sure tests still pass.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if doing this migration will affect our upgrade use case as well.

Copy link
Member

Choose a reason for hiding this comment

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

We should make sure there is an upgrade test for this.

@jaxesn
Copy link
Member

jaxesn commented Mar 10, 2022

This is also interesting, looks Jyoti is trying to add a migration path to iam auth upstream for this situation

kubernetes-sigs/aws-iam-authenticator#439

@mdsgabriel
Copy link
Member Author

/test eks-anywhere-e2e-presubmit

@mdsgabriel mdsgabriel force-pushed the update-client.authentication-apiVersion branch 3 times, most recently from 3c86faf to 24c5302 Compare March 16, 2022 21:07
@eks-distro-bot eks-distro-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 16, 2022
@mdsgabriel mdsgabriel force-pushed the update-client.authentication-apiVersion branch from 24c5302 to 809242f Compare March 17, 2022 18:40
@eks-distro-bot eks-distro-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 17, 2022
Copy link
Member

@jaxesn jaxesn left a comment

Choose a reason for hiding this comment

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

/lgtm

@eks-distro-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jaxesn, mdsgabriel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@eks-distro-bot eks-distro-bot merged commit 3f5d690 into aws:main Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants