Grant EKSCTL_ADMIN_ROLE admin access to eksctl clusters #933
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wongma7 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 |
k8s-ci-robot
added
approved
|
/test pull-aws-ebs-csi-driver-external-test-eks |
|
|
|
/test pull-aws-ebs-csi-driver-external-test-eks |
k8s-ci-robot
added
size/L
|
2nd commit contains changes backported from fsx edition of scripts https://github.com/kubernetes-sigs/aws-fsx-csi-driver/commits/master/hack/e2e |
| function kops_patch_cluster_file() { | ||
| CLUSTER_FILE=${1} # input must be json | ||
| CLUSTER_FILE=${1} # input must be yaml |
There was a problem hiding this comment.
This is a function I'd like to see done in something other than bash. Good candidate for moving reusable parts of this to the test-infra repository? (Not PR blocking, but something we can consider for the future).
There was a problem hiding this comment.
yes, see the comment.
I know I have gone back and forth on this a lot, I decided we should use Go. You can see here https://github.com/kubernetes-sigs/aws-ebs-csi-driver/pull/927/files#diff-1174eb3051f9c1a9e1f65c760347ef47446d5b93c712925b446b29ff591184e8R36 I'm trying it out and it's fine.
|
|
||
| loudecho "Patching cluster $CLUSTER_NAME with $KOPS_PATCH_FILE" | ||
|
|
||
| # Temporary intermediate files for patching | ||
| # Temporary intermediate files for patching, don't mutate CLUSTER_FILE until |
There was a problem hiding this comment.
Does this create the cluster.yaml file before the test? Is this something we can use KOPS_FEATURE_FLAGS="SpecOverrideFlag" for? Also, are these changes in kops.sh necessary for this PR?
There was a problem hiding this comment.
cluster.yaml gets created by the dry run create somewhere else. if it already exists, the pathcing won't hapepn.
No these changes aren't necessary, I just wanted to piggyback the changes from fsx repo
There was a problem hiding this comment.
to be clear the pr desription is only relevant for first commit.
commit 2 and 3 are snuck in
There was a problem hiding this comment.
Ok, might want to break it up or update the PR description.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
Is this a bug fix or adding new feature?
What is this PR about? / Why do we need it? this is part of the effort to get eksctl test CI in every PR.
To get Kubernetes API access to an EKS cluster you need to either be a) cluster creator or b) added to the aws-auth ConfigMap. Of course if you do not have Kubernetes API access in the first place then you can't edit the configmap, so the cluster creator has to do some bootstrapping. This PR enables the cluster creator to bootstrap one additional EKSCTL_ADMIN_ROLE with admin access to the cluster.
By contrast, with kops, if you have AWS API access to the kops cluster config in S3, then you can give yourself Kubernetes API access.
So why do we need one extra admin besides the cluster creator in the first place...I don't want to divulge too much pointless implementation detail about how the CI works but basically, the scripts are being run by an IAM entity in this account: https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes/sig-cloud-provider/aws/sig-aws-credentials.yaml#L17. The cluster creator will be that IAM entity. In case we need to access EKS clusters' Kubernetes API, we don't want to reuse that entity's creds. Better to at cluster-creation time grant a different, dedicated role the permission to access Kubernetes API via EKSCTL_ADMIN_ROLE.
What testing is done? will run eks test