use service accounts as clients for controllers #33310

deads2k · 2016-09-22T19:55:32Z

Makes it possible for the controller-manager to use service accounts to run individual controllers. To start, this only enables this feature if this particular controller manager has the power to create service account tokens. Otherwise, the full-powered client is used instead. This is a necessary step on the way to subdividing the authority of controllers.

@kubernetes/sig-auth
@erictune I know you care about this
@ncdc fyi

This change is

lavalamp · 2016-09-22T20:15:56Z

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

+//
+// CAUTION: If you update code in this file, you may need to also update code
+//          in contrib/mesos/pkg/controllermanager/controllermanager.go
+package app


move to a sub-package if possible.

move to a sub-package if possible.

Sure.

erictune · 2016-09-23T00:07:34Z

I love that you are making progress on this issue.

I want to get feedback from @cjcullen and @roberthbailey

In follow ons to this PR, David is planning to have each controller of the controller manager talk to the apiserver with a different service account for each sub-process of the controller manager.

That seems good to me because it allows for fault isolation of the many pieces of code in the controller manager.

The thing David is doing this in this PR is he is making it possible to mint several new service account tokens at kube-controller-manager startup time.

CJ, Robert and I had been thinking about having some kind of mapping between Kubernetes service accounts and GCP service accounts. I had thought maybe one way to establish this mapping is to have a controller that sees service accounts made in the kubernetes API, and then does some stuff with the GCP API and provides a GCP-managed credential. It would be harder to do this if you do this PR, because there is no service account object to watch.

Is it possible to require that the service accounts for the controller manager exist ahead of time as addon objects, and that the kube-controller-manager mounts all the tokens it needs? That would make upgrades crazy tricky though.

liggitt · 2016-09-23T00:14:09Z

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

+
+	watcher, err := b.CoreClient.Secrets(b.Namespace).Watch(
+		api.ListOptions{
+			ResourceVersion: "0",


I'm seeing this pop up more and more… why is it needed? Isn't it implicitly 0?

I'm seeing this pop up more and more… why is it needed? Isn't it implicitly 0?

I didn't look it up. I would have expected an implicit "", which I would have expected to be "start from now".

liggitt · 2016-09-23T00:17:56Z

It would be harder to do this if you do this PR, because there is no service account object to watch.

Not sure I understand this comment… this PR is creating service account objects, then watching for their API credentials

erictune · 2016-09-23T00:41:26Z

Oh, sorry. I skimmed it first and made some assumptions.

Okay, my next question is how is the process going to work when someone adds a new control loop to controller manager.

If RBAC were the only form of Authz, then in a single PR, they can:

add a new controller
make the SA for the new controller
make the Role for the new controller
add the SA to the Role

But, if a different authorizer is in use, then it is going to be hard to do that in a single PR.

deads2k · 2016-09-23T03:31:54Z

But, if a different authorizer is in use, then it is going to be hard to do that in a single PR.

I'd like to get to the point of "merge won't pass without RBAC permissions set properly for controller". That would likely be setting an e2e run with RBAC on and the insecure port disabled.

Other authorizers would be able to add the permissions at their leisure. Because of the way the controllers work, the forbiddens would simply cause the controller to requeue items until they were able to update them.

lavalamp · 2016-09-23T17:24:26Z

It's a string, so "" is the default, not "0". It is correct to specify "0".

On Thu, Sep 22, 2016 at 8:32 PM, David Eads notifications@github.com
wrote:

@deads2k commented on this pull request.

In cmd/kube-controller-manager/app/client_builder.go
#33310:
// we need the SA UID to find a matching SA token

sa, err := b.CoreClient.ServiceAccounts(b.Namespace).Get(name)

if err != nil && !apierrors.IsNotFound(err) {
return nil, err
} else if apierrors.IsNotFound(err) {
sa, err = b.CoreClient.ServiceAccounts(b.Namespace).Create(
    &api.ServiceAccount{ObjectMeta: api.ObjectMeta{Namespace: b.Namespace, Name: name}})
if err != nil {
    return nil, err
}
}
+

watcher, err := b.CoreClient.Secrets(b.Namespace).Watch(
api.ListOptions{
    ResourceVersion: "0",
I'm seeing this pop up more and more… why is it needed? Isn't it
implicitly 0?

I didn't look it up. I would have expected an implicit "", which I would
have expected to be "start from now".

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#33310, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAngllzQIUM2cifjbIstHS5tYFHoivDRks5qs0hYgaJpZM4KES9Q
.

liggitt · 2016-09-23T21:00:05Z

What is the distinction between "" and "0"?

lavalamp · 2016-09-23T21:11:36Z

"" gets interpreted as unset, which I believe will be interpreted as "give
me everything starting from 'now', whatever you think 'now' happens to be".
"0" means "give me everything starting from 0"

On Fri, Sep 23, 2016 at 2:00 PM, Jordan Liggitt notifications@github.com
wrote:

What is the distinction between "" and "0"?

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#33310 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAnglnYVUDCEdQpezHwQdM_7TH2sPTh0ks5qtD3dgaJpZM4KES9Q
.

liggitt · 2016-09-23T21:19:19Z

If that's the case, won't "0" cause a 410 error once resourceVersion 0 is outside the watch window?

lavalamp · 2016-09-23T21:43:36Z

Yes, it will. I thought this was test code, though?

On Fri, Sep 23, 2016 at 2:19 PM, Jordan Liggitt notifications@github.com
wrote:

Won't "0" cause a 410 error once resourceVersion 0 is outside the watch
window?

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#33310 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAnglml8DxLsyl3oQ_kXip4kj2o4qC_Gks5qtEJggaJpZM4KES9Q
.

liggitt · 2016-09-23T21:51:25Z

No, not test code. I really thought omitted resourceVersion and resourceVersion "0" were equivalent, and that 0 was special cased in etcd storage and in the watch cache, but I'm not at a computer to verify

ncdc · 2016-09-26T13:22:41Z

See also

kubernetes/pkg/kubectl/cmd/get.go

Lines 229 to 233 in 7000e2c

    
           // watching from resourceVersion 0, starts the watch at ~now and 
        
           // will return an initial watch event.  Starting form ~now, rather 
        
           // the rv of the object will insure that we start the watch from 
        
           // inside the watch window, which the rv of the object might not be. 
        
           rv := "0"

erictune · 2016-09-26T15:46:35Z

Other authorizers would be able to add the permissions at their leisure. Because of the way the controllers work, the forbiddens would simply cause the controller to requeue items until they were able to update them.

If you add a new service accounts called "deployment-controller" in namespace "kube-system", and the deployment controller loop uses that service account, then all the e2es which use non-RBAC authorizers will not pass. So you won't be able to merge that PR. So, is your plan:

change other cluster setup methods in the same PR, so that e2es will pass
make use of the new service accounts conditional, and off by default, and only tested by unit tests.
something I haven't thought of.

deads2k · 2016-09-26T16:57:22Z

If you add a new service accounts called "deployment-controller" in namespace "kube-system", and the deployment controller loop uses that service account, then all the e2es which use non-RBAC authorizers will not pass. So you won't be able to merge that PR. So, is your plan:

Pulling the list from the current CI:

gce node e2e
gce e2e
gke smoke e2e
kubemark gce e2e

Do you know which ones (if any) we'd be able to get to switch to using RBAC? We could also consider things like having the other authorizer whitelist SAs from kube-system (there's already an auto-group) to do anything.

deads2k · 2016-09-29T12:57:43Z

@erictune Are you ok with this in principle? Even if we end up having to make it an option, the structure I've provided makes that pretty easy to do.

If you're ok with it in principle, I can get @liggitt to review the details.

deads2k · 2016-10-04T14:04:08Z

Comments addressed.

liggitt · 2016-10-05T13:20:07Z

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

+		var clientBuilder controller.ControllerClientBuilder
+		if len(s.ServiceAccountKeyFile) > 0 {
+			clientBuilder = controller.SAControllerClientBuilder{
+				ClientConfig: kubeconfig,


need to remove the credentials from the client config (upstream AnonymousClientConfig?)

liggitt · 2016-10-05T13:21:49Z