Refactor controller initialization (part 2) by mfojtik · Pull Request #14293 · openshift/origin

mfojtik · 2017-05-23T10:41:23Z

This is a follow up what started in #13996 and is tracked here: https://trello.com/c/ZtOfFpFz/907-5-use-kubernetes-controller-roles

In a nutshell:

We are switching to upstream RBAC roles for upstream controllers (should be already done)
We are switching to new initialization of origin controllers (this PR)

mfojtik · 2017-05-23T10:43:44Z

@enj @sttts attempt to fix the serviceaccount controllers... basically I created "pre" group for them and start them before we run kubernetes controllers. To make it work, I added RootClientBuilder into ControllerContext only for the pre-initialization (actually only serviceaccount-tokens controllers use it). For other origin controllers I erase that field so they don't accidentialy use it.

@deads2k if we go this path, I think we should move the loop where we start the controllers (initFn) into controllers.go as we duplicate the code between pre-initialization and initialization ;-)

mfojtik · 2017-05-23T10:44:22Z

pkg/cmd/server/bootstrappolicy/controller_policy.go

 	// controllerRoleBindings is a slice of roles used for controllers
 	controllerRoleBindings = []rbac.ClusterRoleBinding{}
+
+	ReadWrite = []string{"get", "list", "watch", "create", "update", "patch", "delete", "deletecollection"}


@deads2k copied from kube, i guess this will make better experience for people writing the rules? ;-)

Aren't that accessible constants in kube? If not, maybe they should be public.

they are but i was going to avoid importing the upstream bootstrap package just to get them.

We need the same for auditing. So we probably make them available in k8s.io/apiserver/pkg/endpoints/request

@sttts i will go with TODO for now and once we have stable location we can import

Also, we don't use the ReadWrite or Read variables in controller policy.

mfojtik · 2017-05-23T10:44:58Z

pkg/cmd/server/bootstrappolicy/controller_policy.go

+
+	// serviceaccount-tokens-controller
+	addControllerRole(rbac.ClusterRole{
+		ObjectMeta: metav1.ObjectMeta{Name: saRolePrefix + InfraServiceAccountTokensControllerServiceAccountName},


@deads2k wonder if we want/need this... this controller will use the RootClientBuilder which means privileged client will be used.

nuked this.

mfojtik · 2017-05-23T10:45:15Z

pkg/cmd/server/origin/controller.go

 	"github.com/openshift/origin/pkg/cmd/server/origin/controller"
 )

+func (c *MasterConfig) NewOpenShiftControllerPreInitializers() (map[string]controller.InitFunc, error) {


maybe deserve a better name?

only the SA token controller needs to run first, right?

yeah, not sure about the serviceaccount controller (and pullsecrets/etc.) they ran first in old world.

mfojtik · 2017-05-23T10:45:39Z

pkg/cmd/server/origin/controller/serviceaccount.go

+		options.ServiceAccounts = append(options.ServiceAccounts, sa)
+	}
+
+	//REBASE: add new args to NewServiceAccountsController


@deads2k @sttts still valid comment?

Not sure. What does a blame say? Last rebase or ages ago?

@ncdc addded this in Nov/2016... if this refers to args options then maybe the comment is still valid.

That was a comment that I was reacting to changes made between kube 1.4 and 1.5. It can be deleted.

mfojtik · 2017-05-23T10:46:05Z

pkg/cmd/server/origin/controller/serviceaccount.go

+
+func (c *ServiceAccountTokensControllerOptions) RunController(ctx ControllerContext) (bool, error) {
+	if len(c.ServiceAccountConfig.PrivateKeyFile) == 0 {
+		return true, fmt.Errorf("skipped starting Service Account Token Manager, no private key specified")


@deads2k want confirm that returning true and error here is ok.

A godoc wouldn't harm. It's not obvious what the bool means.

mfojtik · 2017-05-23T10:47:08Z

pkg/cmd/server/start/start_master.go

-	oc.RunServiceAccountsController()
-	oc.RunServiceAccountTokensController(controllerManagerOptions)
-	// used by admission controllers
-	oc.RunServiceAccountPullSecretsControllers()


will come soon

mfojtik · 2017-05-23T10:57:40Z

pkg/authorization/authorizer/scope/authorizer.go

 		denyReason = err.Error()
 	}

+	glog.V(4).Infof("RBAC DENY: user %q groups %q cannot %v", attributes.GetUser().GetName(), attributes.GetUser().GetGroups(), denyReason)


ignore this pathetic attempt to imitate kube messages ;-)

sttts · 2017-05-23T11:29:53Z

pkg/cmd/server/origin/controller/serviceaccount.go

+
+func (c *ServiceAccountControllerOptions) RunController(ctx ControllerContext) (bool, error) {
+	if len(c.ManagedNames) == 0 {
+		return true, fmt.Errorf("Skipped starting Service Account Manager, no managed names specified")


lower case errors

sttts · 2017-05-23T11:31:37Z

pkg/cmd/server/start/start_master.go

 	utilwait "k8s.io/apimachinery/pkg/util/wait"
 	restclient "k8s.io/client-go/rest"
 	kctrlmgr "k8s.io/kubernetes/cmd/kube-controller-manager/app"
+	kubecontroller "k8s.io/kubernetes/cmd/kube-controller-manager/app"


twice the same import?

deads2k · 2017-05-23T12:07:46Z

pkg/cmd/server/bootstrappolicy/controller_policy.go

+	addControllerRole(rbac.ClusterRole{
+		ObjectMeta: metav1.ObjectMeta{Name: saRolePrefix + InfraServiceAccountControllerServiceAccountName},
+		Rules: []rbac.PolicyRule{
+			rbac.NewRule(ReadWrite...).Groups(kapiGroup).Resources("serviceaccounts").RuleOrDie(),


We don't use these in the controller_policy upstream. Mutation is rarely uniform in the controllers and the powers are subtly different. Update can require knowledge of resourceversion, patch doesn't. Delete requires knowledge of names, deletecollection does not.

@deads2k i knew you will not like it... i realized that those rules are too broad for controllers where we know what controllers can/cannot do.

deads2k · 2017-05-23T12:18:35Z

pkg/cmd/server/origin/controller.go

+	serviceAccount := controller.ServiceAccountControllerOptions{
+		ManagedNames: c.Options.ServiceAccountConfig.ManagedNames,
+	}
+	ret["serviceaccount"] = serviceAccount.RunController


this one ought not require pre-init

deads2k · 2017-05-23T12:18:46Z

pkg/cmd/server/origin/controller.go

+	ret["serviceaccount"] = serviceAccount.RunController
+
+	serviceAccountPullSecrets := controller.ServiceAccountPullSecretsControllerOptions{}
+	ret["serviceaccount-pull-secrets"] = serviceAccountPullSecrets.RunController


this one doesn't need to be pre-init

deads2k · 2017-05-23T12:20:07Z

pkg/cmd/server/origin/controller/interfaces.go


+	// RootClient should only be used for pre-initialization controllers
+	// (iow. serviceaccount-tokens controller)
+	RootClientBuilder *controller.SimpleControllerClientBuilder


I would try to wire this down separately into the init function when you create the struct the init function lives on. It's not a preferred means of creating the init function (makes it harder to read), but we really don't want this escaping

i can wire this from MasterConfig I believe and pass it in the struct.

deads2k · 2017-05-23T12:20:51Z

pkg/cmd/server/origin/controller/project.go

+	projectcontroller "github.com/openshift/origin/pkg/project/controller"
+)
+
+type OriginNamespaceControllerOptions struct{}


if we have nothing in here, don't have the struct. Just a bare function.

deads2k · 2017-05-23T12:25:29Z

pkg/cmd/server/origin/controller/serviceaccount.go

+
+func (c *ServiceAccountControllerOptions) RunController(ctx ControllerContext) (bool, error) {
+	if len(c.ManagedNames) == 0 {
+		return true, fmt.Errorf("skipped starting Service Account Manager, no managed names specified")


return false, nil. returning an error will fatal the process.

so use glog?

so use glog?

You can if you like. The caller actually drops a message in the log saying its skipping it.

deads2k · 2017-05-23T12:26:15Z

pkg/cmd/server/origin/controller/serviceaccount.go

+		ctx.DeprecatedOpenshiftInformers.KubernetesInformers().Core().V1().ServiceAccounts(),
+		ctx.DeprecatedOpenshiftInformers.KubernetesInformers().Core().V1().Namespaces(),
+		ctx.ClientBuilder.ClientOrDie(bootstrappolicy.InfraServiceAccountControllerServiceAccountName),
+		options).Run(1, ctx.Stop)


Running only one worker is a little weird.

deads2k · 2017-05-23T12:27:00Z

pkg/cmd/server/origin/controller/serviceaccount.go

+)
+
+type ServiceAccountControllerOptions struct {
+	ManagedNames []string


I like this. Much cleaner than what we had before.

deads2k · 2017-05-23T12:28:32Z

pkg/cmd/server/origin/controller/serviceaccount.go

+
+func (c *ServiceAccountTokensControllerOptions) RunController(ctx ControllerContext) (bool, error) {
+	if len(c.ServiceAccountConfig.PrivateKeyFile) == 0 {
+		return true, fmt.Errorf("skipped starting Service Account Token Manager, no private key specified")


return false, nil. The bool indicates that this controller was enabled, the error means we couldn't start the controller when we were supposed to. So returning false without an error means "skipped" and return an error means, "couldn't fullfill intent, crashloop."

I see this as outstanding.

deads2k · 2017-05-23T12:28:45Z

pkg/cmd/server/origin/controller/serviceaccount.go

+
+	privateKey, err := serviceaccount.ReadPrivateKey(c.ServiceAccountConfig.PrivateKeyFile)
+	if err != nil {
+		return true, fmt.Errorf("error reading signing key for Service Account Token Manager: %v", err)


this one is correct.

deads2k · 2017-05-23T12:29:36Z

pkg/cmd/server/origin/controller/serviceaccount.go

+
+type ServiceAccountTokensControllerOptions struct {
+	ServiceAccountConfig     configapi.ServiceAccountConfig
+	ServiceServingCertConfig configapi.ServiceServingCert


Only the CA file contents please. []byte

Only the CA file contents please. []byte

Reasoning: This should be a runtime optimized config, so content instead of file reference (which is easier for a user) and this controller only needs to know the non-secret information. If we ever separate out these controllers, it doesn't need the full config.

I see this as still outstanding.

deads2k · 2017-05-23T12:34:33Z

pkg/cmd/server/origin/controller/serviceaccount.go

+	return true, nil
+}
+
+type ServiceAccountPullSecretsControllerOptions struct {


no options, no struct.

deads2k · 2017-05-23T12:37:09Z

pkg/cmd/server/start/start_master.go

+	allowedOpenshiftControllers := sets.NewString(
+		// The serviceaccount-token controller has to be started before any other controller
+		// runs in order to be able to issue tokens.
+		"serviceaccount-tokens",


I think we can more or less hard code this one first, then start it directly. The others simply flow from it.

deads2k · 2017-05-23T12:37:33Z

pkg/cmd/server/start/start_master.go

 	}
-	controllerInitializers := kctrlmgr.NewControllerInitializers()

+	oc.RunSecurityAllocationController()


why did this move?

deads2k · 2017-05-23T15:23:39Z

@mfojtik want to clean up what you have so far and merge it rather than growing bigger?

mfojtik · 2017-05-23T15:24:23Z

@deads2k just cleaning up and addressing comments... yeah I would prefer to add this in iteration to prevent rebasing hell.

mfojtik · 2017-05-25T08:49:15Z

@deads2k this was interrupted by #14321 (merge conflict)

@pweil- @jupierce @smarterclayton i would really like to merge this otherwise this will be a rebase hell once the dcut opens. This is improving security of the controllers (well previously there was no security...). Also I would like QA to spend time on running on this to capture bugs associated with new controller privileges.

mfojtik · 2017-05-25T12:21:37Z

flake is: #14208

[test]

mfojtik · 2017-05-25T14:09:56Z

flake : #14353

mfojtik · 2017-05-25T14:10:09Z

[test]

mfojtik · 2017-05-30T13:20:27Z

@smarterclayton seems like this is not broken after I rebased against the shared informers stuff... the serviceaccount-token controller is not creating tokens (seems like the informer is stuck or not started).

smarterclayton · 2017-05-30T14:31:02Z

Once the two controllers are started, we need to start the subset of Kube informers that control secrets and service accounts. See the old code, there's a massive comment right after the controller start

mfojtik · 2017-05-31T09:01:06Z

Once the two controllers are started, we need to start the subset of Kube informers that control secrets and service accounts. See the old code, there's a massive comment right after the controller start

@smarterclayton I saw that comment and I made sure we do start the kube informers right after the controller (serviceaccount-token) is started. That controller indeed starts and origin runs and I even see some tokens creates but there are no SA/secrets for new projects...

mfojtik · 2017-05-31T09:35:56Z

pkg/cmd/server/origin/controller/serviceaccount.go

+func RunServiceAccountPullSecretsController(ctx ControllerContext) (bool, error) {
+	kc := ctx.ClientBuilder.KubeInternalClientOrDie(bootstrappolicy.InfraServiceAccountPullSecretsControllerServiceAccountName)
+
+	serviceaccountcontrollers.NewDockercfgDeletedController(


@smarterclayton hrm... this has to be a goroutine now.... a cost I pay for rebasing this without coffe.

…r initialization

openshift-bot · 2017-05-31T09:45:47Z

Evaluated for origin testextended up to f0d6e70

mfojtik · 2017-05-31T11:18:13Z

flake: #14122

[test]

openshift-bot · 2017-05-31T14:38:56Z

continuous-integration/openshift-jenkins/testextended FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/528/) (Base Commit: 375c727) (Extended Tests: core(builds))

mfojtik · 2017-06-01T11:24:03Z

[test] again

mfojtik · 2017-06-01T13:49:05Z

dockerhub flake (pulling centos...)

[test]

openshift-bot · 2017-06-01T14:13:24Z

Evaluated for origin test up to f0d6e70

openshift-bot · 2017-06-01T15:51:00Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1889/) (Base Commit: 1bf8a17)

mfojtik · 2017-06-05T07:56:44Z

[merge] (lets see if the merge is open now ;-)

mfojtik · 2017-06-05T12:07:54Z

re- [merge]

openshift-bot · 2017-06-05T12:13:21Z

Evaluated for origin merge up to f0d6e70

openshift-bot · 2017-06-05T12:13:28Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/892/) (Base Commit: 90592c5) (Image: devenv-rhel7_6300)

mfojtik requested a review from deads2k May 23, 2017 10:41

mfojtik commented May 23, 2017

View reviewed changes

sttts reviewed May 23, 2017

View reviewed changes

mfojtik force-pushed the controller-init branch 3 times, most recently from 05512c3 to c8ea7e1 Compare May 23, 2017 12:00

deads2k reviewed May 23, 2017

View reviewed changes

mfojtik force-pushed the controller-init branch from 6bd3373 to 047fb4d Compare May 25, 2017 08:45

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2017

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2017

mfojtik force-pushed the controller-init branch from 047fb4d to e832f2a Compare May 30, 2017 13:09

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2017

mfojtik commented May 31, 2017

View reviewed changes

migrate serviceaccount and rest of build controllers to new controlle…

f0d6e70

…r initialization

mfojtik force-pushed the controller-init branch from e832f2a to f0d6e70 Compare May 31, 2017 09:39

openshift-bot merged commit 437dd3c into openshift:master Jun 5, 2017

mfojtik mentioned this pull request Jun 14, 2017

Refactor controller initialization (final round?) #14633

Merged

enj mentioned this pull request Jun 15, 2017

Rework DockercfgTokenDeletedController #13579

Closed

mfojtik deleted the controller-init branch September 5, 2018 21:07

Conversation

mfojtik commented May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mfojtik commented May 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfojtik commented May 23, 2017 •

edited

Loading

mfojtik commented May 25, 2017 •

edited

Loading