BootstrapSigner and TokenCleaner controllers #36101

jbeda · 2016-11-02T22:13:00Z

This is part of kubernetes/enhancements#130 and is an implementation of kubernetes/community#189.

Work that needs to be done yet in this PR:

~~e2e tests~~ Will come in new PR.
flag to disable this by default

Native support for token based bootstrap flow.  This includes signing a well known ConfigMap in the `kube-public` namespace and cleaning out expired tokens.

@kubernetes/sig-cluster-lifecycle @dgoodwin @roberthbailey @mikedanese

This change is

dgoodwin

Just adding some thoughts from an early pass through.

Is it possible to play with this right now? I've made two attempts to get this deployed and running (built my own hyperkube image and deployed it with kubeadm, and the new dind stuff that hit our ML this morning), in both cases I can't seem to trigger anything happening. I've created bootstrap tokens per my WIP PR but no config maps show up in kube-public.

dgoodwin · 2016-11-03T12:05:56Z

pkg/controller/bootstrap/bootstrapsigner.go

+	maxRetries := options.MaxRetries
+	if maxRetries == 0 {
+		maxRetries = 10
+	}


Would DefaultBootstrapSignerOptions be a better place to define that default?

This is the pattern that other modules follow. Folks don't have to use DefaultBootstrapSignerOptions so this deals with that case.

dgoodwin · 2016-11-03T12:21:38Z

pkg/controller/bootstrap/bootstrapsigner.go

+	}
+	configMapSelector := fields.SelectorFromSet(map[string]string{api.ObjectNameField: options.ConfigMapName})
+	e.configMaps, e.configMapsController = cache.NewInformer(
+		&cache.ListWatch{


Why do we monitor the config map itself? I would have expected monitoring secrets and then eventually API servers, and assumed that when a change was detected the config map is considered "ours" to be completely re-synced.

There is no way to monitor "API servers". At some point we may have a way to update this directly but we aren' there yet. To start with this config map will be written as part of set up and rarely changed. As we move to HA we can update it as the HA set changes and this controller will automatically react and sign.

dgoodwin · 2016-11-03T12:24:29Z

pkg/controller/bootstrap/bootstrapsigner.go

+		if _, ok := ret[tokenID]; ok {
+			glog.V(3).Infof("Duplicate bootstrap tokens found for id %s, ignoring on in %s/%s", tokenID, secret.Namespace, secret.Name)
+			continue
+		}


I'm creating these as prefix-tokenID so hopefully it won't be possible for a duplicate ID to appear here.

The name doesn't matter here as we are keying off of the secret type.

dgoodwin · 2016-11-03T13:02:02Z

pkg/controller/bootstrap/tokencleaner.go

+	MaxRetries int
+}
+
+// DefaultBootstrapSignerOptions returns a set of default options for creating a


Just a missed ref to wrong struct name in comment here.

dgoodwin · 2016-11-03T13:10:38Z

pkg/controller/bootstrap/bootstrapsigner.go

+// signConfigMap computes the signatures on our latest cached objects and writes
+// back if necessary.
+func (e *BootstrapSigner) signConfigMap() {
+	configMaps := e.listConfigMaps()


Trying to get this to run so I could poke around but not having much luck yet, I am curious why multiple config maps are referenced here? (would have expected just the one)

Fixed to just grab the one. Was easy enough either way.

dgoodwin · 2016-11-03T16:30:38Z

pkg/controller/bootstrap/bootstrapsigner_test.go

+	ret := &api.Secret{
+		ObjectMeta: api.ObjectMeta{
+			Namespace:       api.NamespaceSystem,
+			Name:            "token-" + tokenID,


Not sure it matters but these are "bootstrap-token-" on my end, if so which should we sync on?

Again, we key off of the secret type so this doesn't matter.

dgoodwin · 2016-11-03T16:33:12Z

pkg/controller/bootstrap/bootstrapsigner_test.go

+		core.NewUpdateAction(unversioned.GroupVersionResource{Resource: "configmaps"},
+			api.NamespacePublic,
+			newConfigMap("tokenID", "eyJhbGciOiJIUzI1NiIsImtpZCI6InRva2VuSUQifQ..QAvK9DAjF0hSyASEkH1MOTB5rJMmbWEY9j-z1NSYILE")),
+	}


This testing stuff looks interesting, I need to see if I can make use of this for unit testing the kubeadm token commands.

mikedanese · 2016-11-04T17:09:35Z

cmd/kube-controller-manager/app/controllermanager.go

+	bootstrap.NewBootstrapSigner(
+		client("bootstrap-signer"),
+		bootstrap.DefaultBootstrapSignerOptions(),
+	).Run()


Run should be started with the stop chan that is in scope here like other controllers. It should not have an explicit stop method, but should stop when the channel is closed.

Cool -- the pattern changed from when I first wrote this. Also note that stopping controllers is currently pretty busted. It would be better to use ctx. But that is too big a change. See: https://github.com/kubernetes/kubernetes/blob/master/pkg/client/cache/controller.go#L125

mikedanese · 2016-11-04T17:15:05Z

pkg/controller/bootstrap/bootstrapsigner.go

+}
+
+// listConfigMaps lists all of the cached config maps
+func (e *BootstrapSigner) listConfigMaps() []*api.ConfigMap {


you are reimplementing listers. https://github.com/kubernetes/kubernetes/blob/master/pkg/client/listers/core/internalversion/zz_generated.configmap.go#L42

Unfortunately, Listers take an indexer. I don't have one of those as I don't need to watch across namespaces. It seems silly to create an indexer just to avoid writing a 7 line function.

mikedanese · 2016-11-04T17:15:54Z

pkg/controller/bootstrap/bootstrapsigner.go

+	return items
+}
+
+func (e *BootstrapSigner) listSecrets() []*api.Secret {


same as above, use a lister

Same -- not indexing this so I can't easily use a lister.

mikedanese · 2016-11-04T17:17:52Z

pkg/controller/bootstrap/bootstrapsigner.go

+		options.SecretResync,
+		cache.ResourceEventHandlerFuncs{
+			AddFunc:    func(_ interface{}) { e.signConfigMap() },
+			UpdateFunc: func(_, _ interface{}) { e.signConfigMap() },


why not accept a config map from signConfigMap() ? Then you only check configmaps that changed.

This simplifies the flow. We want to resign whenever we get a new/changed/deleted token or if the config map changes. In the token change case we don't have the config map already. This way everything just funnels down to a single "reconcile the world" function.

If we were signing a lot of config maps it would make sense to be more surgical.

luxas · 2016-11-17T13:51:54Z

@jbeda Are you planning to get back to this soon so we can have it lgtm'd when master is opening for merges again?
This will be one of the most critical pieces for us to get in in time for v1.6, so the sooner we have something like this the better

jbeda · 2016-11-21T23:14:16Z

@luxas -- yeah. I'll be getting back to this soon. Just coming up from air from the Heptio launch now.

luxas · 2016-11-22T07:37:36Z

Yeah, really cool. Understand that totally, guess it has been a hectic week.
Now it seems like you've figured out what to do :)

jbeda · 2017-02-10T01:13:36Z

@deads2k @mikedanese PTAL

Talked to @smarterclayton quickly and I think I got to the root of what he was looking for.

ericchiang · 2017-02-10T20:43:37Z

Per conversation in the sig-auth channel with @luxas and @liggitt, there we issues with using token.csv.

@liggitt suggested we add a "BootstrapTokenAuthenticator" to handle these cases. That authenticator would look for secrets of type bootstrap.kubernetes.io/token:

apiVersion: v1
kind: Secret
metadata:
  name: simple-bootstrapper
  namespace: kube-system # Only watch tokens in the "kube-system" namespace?
data:
  token-secret: ( bearer token )
  token-id: ( token id )
type: bootstrap.kubernetes.io/token

A request that authenticates using the "( bearer token )" would get the username system:bootstrap:( token id ) and the group system:bootstrappers.

In the short term admins will have to manually create a cluster role binding the group or name to the existing system:node-bootstrapper[0].

cc @liggitt @luxas @jbeda does that accurately summarize the conversation? I'm happy to go add this if it does.

[0]

kubernetes/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/cluster-roles.yaml

Lines 603 to 619 in 03bde62

    
           - apiVersion: rbac.authorization.k8s.io/v1beta1 
        
             kind: ClusterRole 
        
             metadata: 
        
               creationTimestamp: null 
        
               labels: 
        
                 kubernetes.io/bootstrapping: rbac-defaults 
        
               name: system:node-bootstrapper 
        
             rules: 
        
             - apiGroups: 
        
               - certificates.k8s.io 
        
               resources: 
        
               - certificatesigningrequests 
        
               verbs: 
        
               - create 
        
               - get 
        
               - list 
        
               - watch

Move these form core API to a separate package (pkg/bootstrap/api). This also creates the constant for the new kube-public namespace.

liggitt · 2017-02-10T20:47:41Z

cc @liggitt @luxas @jbeda does that accurately summarize the conversation? I'm happy to go add this if it does.

that sounds about right to me. the authenticator bits are distinct from this PR or from any bootstrap bindings if you want to get started in parallel

jbeda · 2017-02-10T20:48:37Z

@ericchiang Yes -- that is the idea. That would be another PR, obviously. I'd propose we document this as part of https://github.com/kubernetes/community/blob/master/contributors/design-proposals/bootstrap-discovery.md (or perhaps a companion proposal)

This adds these to the list of controllers the Controller Manager can start. But as these are alpha, they are also currently disabled by default.

luxas · 2017-02-10T20:50:31Z

cc @liggitt @luxas @jbeda does that accurately summarize the conversation? I'm happy to go add this if it does.

👍

mikedanese · 2017-02-10T22:00:33Z

/lgtm

k8s-github-robot · 2017-02-10T23:59:39Z

Automatic merge from submit-queue (batch tested with PRs 38252, 41122, 36101, 41017, 41264)

@jbeda

Automatic merge from submit-queue (batch tested with PRs 41378, 41413, 40743, 41155, 41385) Expose the constants in pkg/controller/bootstrap and add a validate token function **What this PR does / why we need it**: In order to hook up #36101 against kubeadm, we have to expose the constants and add a function to validate the token **Special notes for your reviewer**: **Release note**: ```release-note NONE ``` cc @jbeda @mikedanese @pires @dmmcquay

@liggitt

Automatic merge from submit-queue (batch tested with PRs 41812, 41665, 40007, 41281, 41771) kube-apiserver: add a bootstrap token authenticator for TLS bootstrapping Follows up on #36101 Still needs: * More tests. * To be hooked up to the API server. - Do I have to do that in a separate PR after k8s.io/apiserver is synced? * Docs (kubernetes.io PR). * Figure out caching strategy. * Release notes. cc @kubernetes/sig-auth-api-reviews @liggitt @luxas @jbeda ```release-notes Added a new secret type "bootstrap.kubernetes.io/token" for dynamically creating TLS bootstrapping bearer tokens. ```

@jbeda

Automatic merge from submit-queue Remove the kube-discovery binary from the tree **What this PR does / why we need it**: kube-discovery was a temporary solution to implementing proposal: https://github.com/kubernetes/community/blob/master/contributors/design-proposals/bootstrap-discovery.md However, this functionality is now gonna be implemented in the core for v1.6 and will fully replace kube-discovery: - #36101 - #41281 - #41417 So due to that `kube-discovery` isn't used in any v1.6 code, it should be removed. The image `gcr.io/google_containers/kube-discovery-${ARCH}:1.0` should and will continue to exist so kubeadm <= v1.5 continues to work. **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # **Special notes for your reviewer**: **Release note**: ```release-note Remove cmd/kube-discovery from the tree since it's not necessary anymore ``` @jbeda @dgoodwin @mikedanese @dmmcquay @lukemarsden @errordeveloper @pires

jbeda added area/controller-manager sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Nov 2, 2016

googlebot added the cla: yes label Nov 2, 2016

jbeda force-pushed the bootstrap-signer branch from 75ca384 to 0b6ea76 Compare November 2, 2016 22:24

k8s-github-robot assigned bprashanth Nov 2, 2016

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 2, 2016

dgoodwin reviewed Nov 3, 2016

View reviewed changes

mikedanese assigned mikedanese and unassigned bprashanth Nov 4, 2016

mikedanese reviewed Nov 4, 2016

View reviewed changes

luxas added this to the v1.6 milestone Nov 12, 2016

luxas mentioned this pull request Nov 25, 2016

kubeadm and kube-discovery should use kubeconfig instead of a custom type kubernetes/kubeadm#58

Closed

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 8, 2016

jbeda force-pushed the bootstrap-signer branch from 0b6ea76 to 612312c Compare December 13, 2016 21:24

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 13, 2016

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 13, 2016

jbeda force-pushed the bootstrap-signer branch 3 times, most recently from ac1bba7 to 7235e14 Compare December 13, 2016 22:22

jbeda added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Dec 13, 2016

jbeda force-pushed the bootstrap-signer branch 2 times, most recently from 18cb472 to 6ac4bc8 Compare December 14, 2016 00:15

luxas removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Feb 10, 2017

jbeda force-pushed the bootstrap-signer branch from 7228e56 to fed564a Compare February 10, 2017 19:40

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2017

jbeda added 4 commits February 10, 2017 12:47

Creates constants for bootstrap tokens.

20e1f8e

Move these form core API to a separate package (pkg/bootstrap/api). This also creates the constant for the new kube-public namespace.

Introduces BootstrapSigner controller

22d6b86

Introduce TokenCleaner to clean out expired bootstrap tokens

2ee7db4

Creates new kube-public namespace as a peer to kube-system

da93009

Wire in bootstrap and token controllers.

415e208

This adds these to the list of controllers the Controller Manager can start. But as these are alpha, they are also currently disabled by default.

jbeda force-pushed the bootstrap-signer branch from fed564a to 415e208 Compare February 10, 2017 20:49

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 10, 2017

k8s-github-robot merged commit 866aa73 into kubernetes:master Feb 10, 2017

ericchiang mentioned this pull request Feb 11, 2017

kube-apiserver: add a bootstrap token authenticator for TLS bootstrapping #41281

Merged

This was referenced Feb 14, 2017

Expose the constants in pkg/controller/bootstrap and add a validate token function #41385

Merged

Add a doc about the kubeadm design and phases implementation kubernetes/kubeadm#156

Merged

luxas mentioned this pull request Feb 24, 2017

Remove the kube-discovery binary from the tree #42070

Merged

zhangxiaoyu-zidif mentioned this pull request Oct 17, 2017

what's kube-public? Issue with k8s.io/editdocs/docs/concepts/overview/working-with-objects/namespaces/ kubernetes/website#5933

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BootstrapSigner and TokenCleaner controllers #36101

BootstrapSigner and TokenCleaner controllers #36101

jbeda commented Nov 2, 2016 •

edited by 0xmichalis

dgoodwin left a comment

dgoodwin Nov 3, 2016

jbeda Dec 13, 2016

dgoodwin Nov 3, 2016

jbeda Dec 13, 2016

dgoodwin Nov 3, 2016

jbeda Dec 13, 2016

dgoodwin Nov 3, 2016

jbeda Dec 13, 2016

dgoodwin Nov 3, 2016

jbeda Dec 13, 2016

dgoodwin Nov 3, 2016

jbeda Dec 13, 2016

dgoodwin Nov 3, 2016

mikedanese Nov 4, 2016

jbeda Dec 13, 2016

mikedanese Nov 4, 2016

jbeda Dec 13, 2016

mikedanese Nov 4, 2016

jbeda Dec 13, 2016

mikedanese Nov 4, 2016 •

edited

jbeda Dec 13, 2016

luxas commented Nov 17, 2016

jbeda commented Nov 21, 2016

luxas commented Nov 22, 2016

jbeda commented Feb 10, 2017

ericchiang commented Feb 10, 2017

liggitt commented Feb 10, 2017

jbeda commented Feb 10, 2017

luxas commented Feb 10, 2017

mikedanese commented Feb 10, 2017

k8s-github-robot commented Feb 10, 2017

BootstrapSigner and TokenCleaner controllers #36101

BootstrapSigner and TokenCleaner controllers #36101

Conversation

jbeda commented Nov 2, 2016 • edited by 0xmichalis

dgoodwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedanese Nov 4, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luxas commented Nov 17, 2016

jbeda commented Nov 21, 2016

luxas commented Nov 22, 2016

jbeda commented Feb 10, 2017

ericchiang commented Feb 10, 2017

liggitt commented Feb 10, 2017

jbeda commented Feb 10, 2017

luxas commented Feb 10, 2017

mikedanese commented Feb 10, 2017

k8s-github-robot commented Feb 10, 2017

jbeda commented Nov 2, 2016 •

edited by 0xmichalis

mikedanese Nov 4, 2016 •

edited