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.

Sign up for GitHub

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

Jump to bottom

Find a new home for bootstrap tokens consts & utilities #64627

Closed
timothysc opened this issue Jun 1, 2018 · 22 comments · Fixed by #67356
Closed

Find a new home for bootstrap tokens consts & utilities #64627

timothysc opened this issue Jun 1, 2018 · 22 comments · Fixed by #67356
Assignees
@timothysc @luxas @yliaog
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle.

Comments

@timothysc
Copy link
Member

timothysc commented Jun 1, 2018 •
edited
Loading

**This is a CLEANUP/ REFACTOR **:
During 1.11 release, as part of 33f59e4 there were concerns raised by @lavalamp regarding whether the aforementioned code belongs there, or in a different location.

from slack:

Yeah, an "api" that's ONLY in the *go* client, under a random tools directory, is not really acceptable. Doesn't work for other languages, and it can't be enforced correctly, etc. If it's just a convention then some combination of: it shouldn't be called an api (?), should be documented such that you can repeat from other languages, should be implemented in a control plane component and not a client, should be promoted to an actual api.
And if it's specific to kubeadm, which it appears to be (?), then it shouldn't be in the general go client.
Also I'm very nervous about a security-critical feature being non obviously buried here

The purpose of this issue is to find that home in the 1.12 cycle.

/kind cleanup
/cc @kubernetes/sig-cluster-lifecycle-misc @kubernetes/sig-api-machinery-misc
@timothysc timothysc added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Jun 1, 2018
@timothysc timothysc added this to the v1.12 milestone Jun 1, 2018
@k8s-ci-robot k8s-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Jun 1, 2018
@rosti rosti mentioned this issue Jun 4, 2018
Closed
@jennybuckley
Copy link

/cc @yliaog

@yliaog
Copy link
Contributor

yliaog commented Jun 8, 2018

kubernetes/client-go#114 is the PR that put the bootstrap tokens under client-go, it seems there were some discussions about where to put them, and there were concerns raised about putting them under client-go.

The requirement is it needs to be accessible from under the following three directories:
k8s.io/kubernetes/cmd/kubeadm/app
k8s.io/kubernetes/pkg/controller/bootstrap
k8s.io/kubernetes/apiserver/plugin/pkg/authenticator/token/bootstrap

May I suggest to create another staging directory for them, something like:
staging/src/k8s.io/kubeadm/bootstrap/token/

@liggitt
Copy link
Member

liggitt commented Jun 8, 2018

kubeadm uses the bootstrap token authentication mechanism, but it does not belong to kubeadm. in particular, k8s.io/kubernetes/apiserver will not have kubeadm as a dependency.

of those locations, I would probably move them under k8s.io/kubernetes/apiserver/plugin/pkg/authenticator/token

@lavalamp
Copy link
Member

lavalamp commented Jun 8, 2018 via email

@sttts sttts added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jun 20, 2018
@k8s-github-robot k8s-github-robot removed this from the v1.12 milestone Jun 25, 2018
@luxas
Copy link
Member

luxas commented Jun 30, 2018

@lavalamp k8s.io/bootstrap SGTM. I think it needs to be in the "core" organization as stuff in core will depend on it (e.g. the apiserver)

@sttts
Copy link
Contributor

sttts commented Jun 30, 2018

k8s.io/bootstrap or k8s.io/bootstrap-auth both sound fine, maybe the later. Do we expect non-auth content?

@luxas
Copy link
Member

luxas commented Jun 30, 2018

@sttts the bootstrap tokens are used for both JWS validation and auth.

@sttts
Copy link
Contributor

sttts commented Jun 30, 2018

@luxas sounds good. I guess the code will depend on k8s.io/apimachinery+code-generator and we will depend on it in kube? This means it must become a staging repo.

How about creating a PR moving the code into staging/src/k8s.io/bootstrap?

@luxas
Copy link
Member

luxas commented Jun 30, 2018

@luxas sounds good. I guess the code will depend on k8s.io/apimachinery+code-generator and we will depend on it in kube? This means it must become a staging repo.

Yep, I think that is the best option we have here.

How about creating a PR moving the code into staging/src/k8s.io/bootstrap?

👍
@liztio will look into this and help out as well. Right now she's OOO AFAIK, though.
We'll discuss the exact implementation plans we have in SIG Cluster Lifecycle and (hopefully) be able
to create a short proposal for how to proceed in more detail.
/assign @liztio

@luxas luxas added this to the v1.12 milestone Jun 30, 2018
@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Issue: Up-to-date for process

@liztio @luxas @timothysc

Issue Labels
  • sig/api-machinery sig/cluster-lifecycle: Issue will be escalated to these SIGs if needed.
  • priority/important-soon: Escalate to the issue owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
  • kind/cleanup: Adding tests, refactoring, fixing old bugs.
Help

@yliaog
Copy link
Contributor

yliaog commented Jul 2, 2018

staging/src/k8s.io/bootstrap looks good.

@luxas
Copy link
Member

luxas commented Jul 2, 2018

@yliaog do you want to help with this? I can guide you how to get started and we'll do this teamwork everyone together 😄?

@yliaog
Copy link
Contributor

yliaog commented Jul 2, 2018

Sure, I can help work on it.

@yliaog yliaog mentioned this issue Aug 14, 2018
Merged
@timothysc timothysc assigned yliaog and unassigned liztio Aug 21, 2018
@cblecker
Copy link
Member

When you're ready for the new repo to be created, you can submit a request here:
https://github.com/kubernetes/org/issues/new?template=repo-create.md

@guineveresaenger
Copy link
Contributor

@timothysc in order for this to qualify for 1.12 release, it and all related PRs need the label priority/critical-urgent. Can you consider and triage?

@luxas luxas added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Sep 5, 2018
@yliaog
Copy link
Contributor

yliaog commented Sep 6, 2018

@timothysc @luxas could you help add the label priority/critical-urgent to the PR (#67356), so it can merge?

@liggitt
Copy link
Member

liggitt commented Sep 6, 2018

Why is it critical? What is broken or blocked if it merges at the beginning of 1.13 instead of now?

@yliaog
Copy link
Contributor

yliaog commented Sep 6, 2018

client-go would still carry it in 1.12 in that case, it won't be broken, but less than ideal. The PR is simply moving code, hence fairly safe. That said I'm fine to wait for 1.13.

@lavalamp
Copy link
Member

lavalamp commented Sep 7, 2018 via email

@guineveresaenger
Copy link
Contributor

@lavalamp, @LiGgit - since it's currently code freeze, I'm suggesting holding off until master reopens in a couple weeks. I know it's frustrating. :(

@guineveresaenger
Copy link
Contributor

Actually: it looks like the PR is ready and only missing critical/urgent label. Again, since technically it's not urgent, I leave the decision up to you.

@dims
Copy link
Member

dims commented Sep 18, 2018

/milestone clear

(to match the fact that the PR was moved out of 1.12)

@k8s-ci-robot k8s-ci-robot removed this from the v1.12 milestone Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@timothysc timothysc

@luxas luxas

@yliaog yliaog

Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Moved staging/src/k8s.io/client-go/tools/bootstrap to staging/src/k8s… yliaog/kubernetes