-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Allow configurable backend modes for aws-iam-authenticator #9500
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
Welcome @WarpRat! |
Hi @WarpRat. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
I signed it |
/ok-to-test you'll need to run |
@fred-vogt could you take a look at this? Thanks! |
@WarpRat, @hakman - had to update my KOPS custom build instructions to work for a Mac. Good news - got builds / uploads working from my Mac Test plan:
|
No worries. Thanks! :) |
@WarpRat, @hakman - I did the first test - it doesn't work. Looks like kubectl:
$ aws-iam-authenticator token -i <cluster-name> # works kube-api-server:
|
I think there are a couple changes we should make here:
The cert issue might be related to @johngmyers's recent cert issuance refactoring. I wish we had automated test coverage for this feature but the prerequisite AWS setup is nontrivial for our prow jobs. It's probably worth upgrading to 0.5.1 first before we continue with any cert troubleshooting though. |
+1 what @rifelpet said, if someone has the --backend-mode flag set with a version of the authenticator < 0.5.0 it'll just cause the authentication to fail with |
Thanks for pointing me in the right direction on those tests @rifelpet I also noticed that we were going to hit this issue with the cluster-id if a user didn't have a mounted file as one of the backend options, since normally the user defines that in config.yaml: Lines 72 to 76 in 24ff70e
I added the same conditional around mounting that config file to remove the config cli flag and set the cluster-id to the master's public API DNS address. |
@@ -37,9 +37,16 @@ spec: | |||
image: {{ or .Authentication.Aws.Image "602401143452.dkr.ecr.us-west-2.amazonaws.com/amazon/aws-iam-authenticator:v0.4.0" }} | |||
args: | |||
- server | |||
{{- if or (not .Authentication.Aws.BackendMode) (contains "MountedFile" .Authentication.Aws.BackendMode) }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have two version of the the addon manifest (two template files) one for k8s 1.10+ and one for k8s 1.12+. Given that we only updated the 1.12+ manifest to authenticator 0.5.1: eae3fd8#diff-49ab9104184d27772708fb0ecd00e0aa we dont need to add BackendMode functionality to the 1.10+ manifest, so we can probably remove all the changes to this file.
upup/models/cloudup/resources/addons/authentication.aws/k8s-1.12.yaml.template
Outdated
Show resolved
Hide resolved
16e2cdf
to
bdf6014
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, a few documentation nits and youll need to rerun ./hack/updated-expected.sh since some of the outputs changed due to the recent 1.19 alpha release.
/assign @rdrgmnzs
in case you have any additional feedback.
Just reviewed, I think you guys covered everything. LGTM for landing once the expected test results are fixed. |
/retest |
/lgtm thanks for sticking with it! looking forward to using these new fields |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rifelpet, WarpRat 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 |
/test pull-kops-e2e-cni-kuberouter |
This is a small configuration change to allow configuring a custom list of backend modes for aws-iam-authenticator (#9468) The available options are MountedFile, EKSConfigMap, or CRD. Any number of these can be selected in order of preference in a comma separated list. The default is still MountedFile.