Properly fix acquire_cluster_admin_role() #87
Conversation
`kubectl` 1.11 doesn't recognize the `--username` and `--password` flags anymore. Because the `prow-tests` image uses `kubectl` 1.8, the Prow jobs are not broken (yet). The changes in #86 don't work because the context is not changed (nor restored), but the function works as long as the current user is an owner of the GCP project (which is not the case for Prow E2E test jobs). This change creates and uses a new context for the cluster admin, created the role binding, then switches back to the original context. This is more secure as no ACL changes are required for the current user, nor project wide ACL changes are performed.
knative-prow-robot
added
the
approved
knative-prow-robot
added
the
size/XS
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adrcunha, steuhs 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 |
| kubectl config set-credentials cluster-admin --username=admin --password=${password} | ||
| kubectl config set-credentials cluster-admin \ | ||
| --username=admin --password=${password} | ||
| kubectl config set-context $(kubectl config current-context) \ |
There was a problem hiding this comment.
ok, I got "new context" from description. Looks like u already updated it.
There was a problem hiding this comment.
Yes, there was indeed a mixup, and I fixed the description to reflect the implementation after I saw your comment. Originally I used a new context, but that was unnecessary and just added more complexity.
| @@ -164,10 +164,16 @@ function acquire_cluster_admin_role() { | |||
| # might not have the necessary permission. | |||
| local password=$(gcloud --format="value(masterAuth.password)" \ | |||
| container clusters describe $2 --zone=$3) | |||
| kubectl config set-credentials cluster-admin --username=admin --password=${password} | |||
| kubectl config set-credentials cluster-admin \ | |||
| --username=admin --password=${password} | |||
There was a problem hiding this comment.
so this will break when prow upgrades to 1.11?
There was a problem hiding this comment.
No, it's backwards compatible.
There was a problem hiding this comment.
in description, it says 1.11 no longer recognizes username and password. then set-credentials won't work, isn't it?
There was a problem hiding this comment.
No. It doesn't recognize them as global flags.
There was a problem hiding this comment.
(compare the original code with the current code; updated description again)
|
/lgtm |
kubectl1.11 doesn't recognize the--usernameand--passwordglobal flags anymore. Because theprow-testsimage useskubectl1.8, the Prow jobs are not broken (yet).The changes in #86 don't work because the context is not changed (nor restored), but the function works as long as the current user is an owner of the GCP project (which is not the case for Prow E2E test jobs).
This change updates the context for the cluster admin, creates the role binding, then reinstates the original context. This is more secure as no ACL changes are required for the current user, nor project wide ACL changes are performed.