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

Remove basic authentication support for k8s 1.19+ #8783

Merged
merged 2 commits into from
Apr 5, 2020

Conversation

hakman
Copy link
Member

@hakman hakman commented Mar 23, 2020

Support for basic auth was removed in Kubernetes 1.19: kubernetes/kubernetes#89069.

Kops e2e test are failing since this was merged. @rifelpet and me tracked this issue and tested that kube-apiserver can start now.
https://testgrid.k8s.io/sig-cluster-lifecycle-kops#kops-aws-updown

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 23, 2020
@hakman hakman force-pushed the remove-basic-auth branch 2 times, most recently from 707c790 to 9ad0f5f Compare March 23, 2020 09:10
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. area/documentation and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 23, 2020
@rifelpet
Copy link
Member

Adding some context:

The updown prow job was failing because the kube-apiserver pod was in a crashloop. There were no logs in /var/log/kube-apiserver.log so the job artifact was empty. I manually launched a cluster using the same create cluster command and docker logs revealed Error: unknown flag: --basic-auth-file which lead us to the k/k commit @hakman linked to above.

Kops creates basic auth credentials for each cluster (visible from a kops export kubecfg). These will no longer work in k8s 1.19.

We came up with a few options on how to proceed:

  1. Switch the prow jobs to use the latest k8s 1.18 rather than 1.19. This is relevant because kops' master branch is still targeting 1.18. Once we create the release-1.18 branch, master will target 1.19 and we can merge any fixes and switch the jobs back to target k8s 1.19. This means that any fixes for this issue will land in kops 1.18 which may or may not be desirable. For precedence, we did this with the kubelet node labeling issue in 1.16 before kops-controller was introduced.
  2. Merge this PR now as well as anything else required to drop basic auth support, guarding all the logic with checks for k8s >=1.19.

My main concern with this dropping of support is providing any sort of workaround for users of basic auth, if possible. Since kops creates these credentials, we'll need to make it known that they will no longer work, as well as no longer create these credentials for new clusters (or even delete them for existing clusters? possible downgrade concerns there...). The k/k commit mentions --token-auth-file which Kops does support but it seems to use a deprecated function as well.

Anyone else have any thoughts or suggestions? This is blocking a lot of visibility into our e2e jobs. I'll add a note to discuss this at office hours as well.

@rifelpet
Copy link
Member

rifelpet commented Mar 23, 2020

Additionally, it'd be great to find out why the /var/log/kube-apiserver.log files were empty. My guess is that the container exited before tee had a chance to write the contents to the file. Maybe this is an opportunity to discontinue use of tee and instead just rely on docker logs or something else? We'd have to update kubetest to get these logs as well. I know it was discussed a while ago but I can't remember where or any decisions about it.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/api and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 5, 2020
Co-Authored-By: John Gardiner Myers <jgmyers@proofpoint.com>
@rifelpet
Copy link
Member

rifelpet commented Apr 5, 2020

Nice, I like this compromise of disabling by default for one k8s version before removing

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 5, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman, rifelpet

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 5, 2020
@k8s-ci-robot k8s-ci-robot merged commit 0f8e4a2 into kubernetes:master Apr 5, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Apr 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api area/documentation area/nodeup cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants