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 k8s dependencies to v1.24 #466

Closed
wants to merge 2 commits into from

Conversation

kinarashah
Copy link

No description provided.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 21, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot
Copy link
Contributor

Welcome @kinarashah!

It looks like this is your first PR to kubernetes-sigs/aws-iam-authenticator 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/aws-iam-authenticator has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

Hi @kinarashah. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 21, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kinarashah
To complete the pull request process, please assign nckturner after the PR has been reviewed.
You can assign the PR to them by writing /assign @nckturner in a comment when ready.

The full list of commands accepted by this bot can be found 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

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 22, 2022
@kinarashah
Copy link
Author

@wongma7 @micahhausler Hi, I just signed CLA..could you please take a look and add /ok-to-test if all is good?

@k8s-ci-robot
Copy link
Contributor

@kinarashah: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 2, 2022
@nilesh-akhade
Copy link

@micahhausler Would you please merge this PR? Go modules causing conflict in our CI, because of the missing package v1alpha1 in client-go, which is used by token_test.go

@pallav-trilio
Copy link

pallav-trilio commented Sep 12, 2022

@micahhausler can you please merge this PR. Go modules causing issues due to which are unable to use v1 storages apis, because of the missing package v1alpha1 in client-go

@madhurtrilio
Copy link

@micahhausler please merge this soon as it has become a blocker for us to update our packages

@nilesh-akhade
Copy link

Those who are awaiting for merge, you can go ahead and fork this branch and use the replace directive in your go.mod.

For lazy people, add the following line in your go.mod

replace sigs.k8s.io/aws-iam-authenticator => github.com/catalogicsoftware/aws-iam-authenticator v0.5.10

@melnikalex
Copy link
Contributor

We can’t merge this CR as it's gonna break clients that still have kubectls and kubeconfigs configured for v1alpha1 api.

In a nutshell, we'll need to:

  • import 2 versions of client-go (this will be much easier if we start using go mods instead of vendors)
  • support both v1alpha1 and v1 options from KUBERNETES_EXEC_INFO
  • default to v1beta1 if no KUBERNETES_EXEC_INFO var is provided

If you could pick this up that'd be amazing. But if not, someone from AWS will pick it up in the next month or so.

For more context see:

@wongma7
Copy link
Contributor

wongma7 commented Nov 30, 2022

We can’t merge this CR as it's gonna break clients that still have kubectls and kubeconfigs configured for v1alpha1 api.

no it won't, authenticator does not depend on client-go for this at all, whatever the apiversion is in the kubeconfig the ExecCredential returned is set to the same and this works for v1, v1alpha1, v1beta1, because the ExecCredential returned is valid for all of those.

REF: https://github.com/kubernetes-sigs/aws-iam-authenticator/blob/master/pkg/token/token.go#L350

{
Name: "KUBERNETES_EXEC_INFO with v1alpha1",
EnvKey: "KUBERNETES_EXEC_INFO",
ExpectApiVersion: clientauthv1alpha1.SchemeGroupVersion.String(),
Copy link
Contributor

@wongma7 wongma7 Nov 30, 2022

Choose a reason for hiding this comment

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

keep the test but replace this with string client.authentication.k8s.io/v1alpha1.

If there is further doubt about whether simply updating client-go could affect authenticator's compatibility to client.authentication.k8s.io/v1alpha1 i suggest someone implement in a follow-up PR an e2e test to validate kubeconfigs with all versions, v1alpha1 v1beta1, v1. (Could be me, IDK, I'm not volunteering at this time just suggesting)

Copy link
Contributor

Choose a reason for hiding this comment

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

The v1alpha1 test was already removed in a previous PR so this is no longer relevant to this PR.

@nckturner
Copy link
Contributor

/close

Vendor was also removed in a previous PR so I'm going to close this in favor of a new PR.

@k8s-ci-robot
Copy link
Contributor

@nckturner: Closed this PR.

In response to this:

/close

Vendor was also removed in a previous PR so I'm going to close this in favor of a new PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants