From 4e4253f2c75ddbcb762e922dcbfa118f34a1bbe4 Mon Sep 17 00:00:00 2001 From: deads2k Date: Wed, 24 May 2017 16:26:29 -0400 Subject: [PATCH] simplify controller wiring --- pkg/cmd/server/origin/controller.go | 40 ++--- .../server/origin/controller/interfaces.go | 7 + .../origin/controller/serviceaccount.go | 4 +- pkg/cmd/server/start/start_master.go | 156 +++++++----------- test/integration/buildcontroller_test.go | 6 +- 5 files changed, 96 insertions(+), 117 deletions(-) diff --git a/pkg/cmd/server/origin/controller.go b/pkg/cmd/server/origin/controller.go index 843a87f8927e..fb2a8bf39784 100644 --- a/pkg/cmd/server/origin/controller.go +++ b/pkg/cmd/server/origin/controller.go @@ -17,12 +17,12 @@ import ( // NewOpenShiftControllerPreStartInitializers returns list of initializers for controllers // that needed to be run before any other controller is started. -// Typically this has to done for the serviceaccount-tokens controller as it provides +// Typically this has to done for the serviceaccount-token controller as it provides // tokens to other controllers. func (c *MasterConfig) NewOpenShiftControllerPreStartInitializers() (map[string]controller.InitFunc, error) { ret := map[string]controller.InitFunc{} - saTokens := controller.ServiceAccountTokensControllerOptions{ + saToken := controller.ServiceAccountTokenControllerOptions{ RootClientBuilder: kubecontroller.SimpleControllerClientBuilder{ ClientConfig: &c.PrivilegedLoopbackClientConfig, }, @@ -35,17 +35,17 @@ func (c *MasterConfig) NewOpenShiftControllerPreStartInitializers() (map[string] var err error - saTokens.PrivateKey, err = serviceaccount.ReadPrivateKey(c.Options.ServiceAccountConfig.PrivateKeyFile) + saToken.PrivateKey, err = serviceaccount.ReadPrivateKey(c.Options.ServiceAccountConfig.PrivateKeyFile) if err != nil { return nil, fmt.Errorf("error reading signing key for Service Account Token Manager: %v", err) } if len(c.Options.ServiceAccountConfig.MasterCA) > 0 { - saTokens.RootCA, err = ioutil.ReadFile(c.Options.ServiceAccountConfig.MasterCA) + saToken.RootCA, err = ioutil.ReadFile(c.Options.ServiceAccountConfig.MasterCA) if err != nil { return nil, fmt.Errorf("error reading master ca file for Service Account Token Manager: %s: %v", c.Options.ServiceAccountConfig.MasterCA, err) } - if _, err := cert.ParseCertsPEM(saTokens.RootCA); err != nil { + if _, err := cert.ParseCertsPEM(saToken.RootCA); err != nil { return nil, fmt.Errorf("error parsing master ca file for Service Account Token Manager: %s: %v", c.Options.ServiceAccountConfig.MasterCA, err) } } @@ -63,13 +63,14 @@ func (c *MasterConfig) NewOpenShiftControllerPreStartInitializers() (map[string] // if we have a rootCA bundle add that too. The rootCA will be used when hitting the default master service, since those are signed // using a different CA by default. The rootCA's key is more closely guarded than ours and if it is compromised, that power could // be used to change the trusted signers for every pod anyway, so we're already effectively trusting it. - if len(saTokens.RootCA) > 0 { - saTokens.ServiceServingCA = append(saTokens.ServiceServingCA, saTokens.RootCA...) - saTokens.ServiceServingCA = append(saTokens.ServiceServingCA, []byte("\n")...) + if len(saToken.RootCA) > 0 { + saToken.ServiceServingCA = append(saToken.ServiceServingCA, saToken.RootCA...) + saToken.ServiceServingCA = append(saToken.ServiceServingCA, []byte("\n")...) } - saTokens.ServiceServingCA = append(saTokens.ServiceServingCA, serviceServingCA...) + saToken.ServiceServingCA = append(saToken.ServiceServingCA, serviceServingCA...) } - ret["serviceaccount-tokens"] = saTokens.RunController + // this matches the upstream name + ret["serviceaccount-token"] = saToken.RunController return ret, nil } @@ -77,13 +78,14 @@ func (c *MasterConfig) NewOpenShiftControllerPreStartInitializers() (map[string] func (c *MasterConfig) NewOpenshiftControllerInitializers() (map[string]controller.InitFunc, error) { ret := map[string]controller.InitFunc{} + // TODO this overrides an upstream controller, so move this to where we initialize upstream controllers serviceAccount := controller.ServiceAccountControllerOptions{ ManagedNames: c.Options.ServiceAccountConfig.ManagedNames, } ret["serviceaccount"] = serviceAccount.RunController - ret["serviceaccount-pull-secrets"] = controller.RunServiceAccountPullSecretsController - ret["origin-namespace"] = controller.RunOriginNamespaceController + ret["openshift.io/serviceaccount-pull-secrets"] = controller.RunServiceAccountPullSecretsController + ret["openshift.io/origin-namespace"] = controller.RunOriginNamespaceController // initialize build controller storageVersion := c.Options.EtcdStorageConfig.OpenShiftStorageVersion @@ -97,9 +99,9 @@ func (c *MasterConfig) NewOpenshiftControllerInitializers() (map[string]controll AdmissionPluginConfig: c.Options.AdmissionConfig.PluginConfig, Codec: codec, } - ret["build"] = buildControllerConfig.RunController - ret["build-pod"] = controller.RunBuildPodController - ret["build-config-change"] = controller.RunBuildConfigChangeController + ret["openshift.io/build"] = buildControllerConfig.RunController + ret["openshift.io/build-pod"] = controller.RunBuildPodController + ret["openshift.io/build-config-change"] = controller.RunBuildConfigChangeController // initialize apps.openshift.io controllers vars, err := c.GetOpenShiftClientEnvVars() @@ -107,16 +109,16 @@ func (c *MasterConfig) NewOpenshiftControllerInitializers() (map[string]controll return nil, err } deployer := controller.DeployerControllerConfig{ImageName: c.ImageFor("deployer"), Codec: codec, ClientEnvVars: vars} - ret["deployer"] = deployer.RunController + ret["openshift.io/deployer"] = deployer.RunController deploymentConfig := controller.DeploymentConfigControllerConfig{Codec: codec} - ret["deploymentconfig"] = deploymentConfig.RunController + ret["openshift.io/deploymentconfig"] = deploymentConfig.RunController deploymentTrigger := controller.DeploymentTriggerControllerConfig{Codec: codec} - ret["deploymenttrigger"] = deploymentTrigger.RunController + ret["openshift.io/deploymenttrigger"] = deploymentTrigger.RunController templateInstance := controller.TemplateInstanceControllerConfig{} - ret["templateinstance"] = templateInstance.RunController + ret["openshift.io/templateinstance"] = templateInstance.RunController return ret, nil } diff --git a/pkg/cmd/server/origin/controller/interfaces.go b/pkg/cmd/server/origin/controller/interfaces.go index 23136fc1aabc..7cd17273a4fe 100644 --- a/pkg/cmd/server/origin/controller/interfaces.go +++ b/pkg/cmd/server/origin/controller/interfaces.go @@ -89,3 +89,10 @@ func (b OpenshiftControllerClientBuilder) OpenshiftTemplateClient(name string) ( } return templateclient.NewForConfig(clientConfig) } + +// FromKubeInitFunc adapts a kube init func to an openshift one +func FromKubeInitFunc(initFn kubecontroller.InitFunc) InitFunc { + return func(ctx ControllerContext) (bool, error) { + return initFn(ctx.KubeControllerContext) + } +} diff --git a/pkg/cmd/server/origin/controller/serviceaccount.go b/pkg/cmd/server/origin/controller/serviceaccount.go index c46a44f61850..6211caa67946 100644 --- a/pkg/cmd/server/origin/controller/serviceaccount.go +++ b/pkg/cmd/server/origin/controller/serviceaccount.go @@ -41,7 +41,7 @@ func (c *ServiceAccountControllerOptions) RunController(ctx ControllerContext) ( return true, nil } -type ServiceAccountTokensControllerOptions struct { +type ServiceAccountTokenControllerOptions struct { RootCA []byte ServiceServingCA []byte PrivateKey interface{} @@ -49,7 +49,7 @@ type ServiceAccountTokensControllerOptions struct { RootClientBuilder controller.SimpleControllerClientBuilder } -func (c *ServiceAccountTokensControllerOptions) RunController(ctx ControllerContext) (bool, error) { +func (c *ServiceAccountTokenControllerOptions) RunController(ctx ControllerContext) (bool, error) { go sacontroller.NewTokensController( ctx.DeprecatedOpenshiftInformers.KubernetesInformers().Core().V1().ServiceAccounts(), ctx.DeprecatedOpenshiftInformers.KubernetesInformers().Core().V1().Secrets(), diff --git a/pkg/cmd/server/start/start_master.go b/pkg/cmd/server/start/start_master.go index 6b3f0735be04..d05ba95f9b52 100644 --- a/pkg/cmd/server/start/start_master.go +++ b/pkg/cmd/server/start/start_master.go @@ -610,11 +610,24 @@ func startControllers(oc *origin.MasterConfig, kc *kubernetes.MasterConfig) erro ClientConfig: &oc.PrivilegedLoopbackClientConfig, } - // openshiftControllerPreContext represents a context used to start controllers that - // requires privileged 'rootClientBuilder' iow. have to start before any other - // controller starts (including kubernetes controllers). + availableResources, err := kctrlmgr.GetAvailableResources(rootClientBuilder) + if err != nil { + return err + } + openshiftControllerContext := origincontrollers.ControllerContext{ - KubeControllerContext: kctrlmgr.ControllerContext{Options: *controllerManagerOptions}, + KubeControllerContext: kctrlmgr.ControllerContext{ + ClientBuilder: controller.SAControllerClientBuilder{ + ClientConfig: restclient.AnonymousClientConfig(&oc.PrivilegedLoopbackClientConfig), + CoreClient: oc.PrivilegedLoopbackKubernetesClientsetExternal.Core(), + AuthenticationClient: oc.PrivilegedLoopbackKubernetesClientsetExternal.Authentication(), + Namespace: "kube-system", + }, + InformerFactory: oc.Informers.KubernetesInformers(), + Options: *controllerManagerOptions, + AvailableResources: availableResources, + Stop: utilwait.NeverStop, + }, ClientBuilder: origincontrollers.OpenshiftControllerClientBuilder{ ControllerClientBuilder: controller.SAControllerClientBuilder{ ClientConfig: restclient.AnonymousClientConfig(&oc.PrivilegedLoopbackClientConfig), @@ -633,13 +646,12 @@ func startControllers(oc *origin.MasterConfig, kc *kubernetes.MasterConfig) erro if err != nil { return err } - if started, err := preStartControllers["serviceaccount-tokens"](openshiftControllerContext); err != nil { - glog.Fatalf("Error starting serviceaccount-tokens controller") - return err + if started, err := preStartControllers["serviceaccount-token"](openshiftControllerContext); err != nil { + return fmt.Errorf("Error starting serviceaccount-token controller: %v", err) } else if !started { - glog.Warningf("Skipping serviceaccount-tokens controller") + glog.Warningf("Skipping serviceaccount-token controller") } - glog.Infof("Started serviceaccount-tokens controller") + glog.Infof("Started serviceaccount-token controller") // The service account controllers require informers in order to create service account tokens // for other controllers, which means we need to start their informers (which use the privileged @@ -648,28 +660,43 @@ func startControllers(oc *origin.MasterConfig, kc *kubernetes.MasterConfig) erro oc.RunSecurityAllocationController() - saClientBuilder := controller.SAControllerClientBuilder{ - ClientConfig: restclient.AnonymousClientConfig(&oc.PrivilegedLoopbackClientConfig), - CoreClient: oc.PrivilegedLoopbackKubernetesClientsetExternal.Core(), - AuthenticationClient: oc.PrivilegedLoopbackKubernetesClientsetExternal.Authentication(), - Namespace: "kube-system", + // These controllers are special-cased upstream. We'll need custom init functions for them downstream. + // As we make them less special, we should re-visit this + kc.RunNodeController() + kc.RunScheduler() + + _, _, _, binderClient, err := oc.GetServiceAccountClients(bootstrappolicy.InfraPersistentVolumeBinderControllerServiceAccountName) + if err != nil { + glog.Fatalf("Could not get client for persistent volume binder controller: %v", err) } - availableResources, err := kctrlmgr.GetAvailableResources(rootClientBuilder) + + _, _, _, attachDetachControllerClient, err := oc.GetServiceAccountClients(bootstrappolicy.InfraPersistentVolumeAttachDetachControllerServiceAccountName) if err != nil { - return err + glog.Fatalf("Could not get client for attach detach controller: %v", err) } - controllerContext := kctrlmgr.ControllerContext{ - ClientBuilder: saClientBuilder, - InformerFactory: oc.Informers.KubernetesInformers(), - Options: *controllerManagerOptions, - AvailableResources: availableResources, - Stop: utilwait.NeverStop, + _, _, _, serviceLoadBalancerClient, err := oc.GetServiceAccountClients(bootstrappolicy.InfraServiceLoadBalancerControllerServiceAccountName) + if err != nil { + glog.Fatalf("Could not get client for pod gc controller: %v", err) + } + kc.RunPersistentVolumeController(binderClient, oc.Options.PolicyConfig.OpenShiftInfrastructureNamespace, oc.ImageFor("recycler"), bootstrappolicy.InfraPersistentVolumeRecyclerControllerServiceAccountName) + kc.RunPersistentVolumeAttachDetachController(attachDetachControllerClient) + kc.RunServiceLoadBalancerController(serviceLoadBalancerClient) + + openshiftControllerInitializers, err := oc.NewOpenshiftControllerInitializers() + if err != nil { + return err + } + for name, initFn := range kctrlmgr.NewControllerInitializers() { + if _, ok := openshiftControllerInitializers[name]; ok { + // don't overwrite, openshift takes priority + continue + } + openshiftControllerInitializers[name] = origincontrollers.FromKubeInitFunc(initFn) } - controllerInitializers := kctrlmgr.NewControllerInitializers() - // TODO I think this should become a blacklist kept in sync during rebases with a unit test. allowedControllers := sets.NewString( + // TODO I think this kube part should become a blacklist kept in sync during rebases with a unit test. "endpoint", "replicationcontroller", "podgc", @@ -690,87 +717,30 @@ func startControllers(oc *origin.MasterConfig, kc *kubernetes.MasterConfig) erro // "tokencleaner", // These controllers need to have their own init functions until we extend the upstream controller config - // TODO we have a different set of managed names which need to be plumbed through. - // "serviceaccount", // TODO this controller takes different evaluators. // "resourcequota", - ) - - for controllerName, initFn := range controllerInitializers { - // TODO remove this. Only call one to start to prove the principle - if !allowedControllers.Has(controllerName) { - glog.Warningf("%q is skipped", controllerName) - continue - } - if !controllerContext.IsControllerEnabled(controllerName) { - glog.Warningf("%q is disabled", controllerName) - continue - } - glog.V(1).Infof("Starting %q", controllerName) - started, err := initFn(controllerContext) - if err != nil { - glog.Errorf("Error starting %q", controllerName) - return err - } - if !started { - glog.Warningf("Skipping %q", controllerName) - continue - } - glog.Infof("Started %q", controllerName) - } - - // These controllers are special-cased upstream. We'll need custom init functions for them downstream. - // As we make them less special, we should re-visit this - kc.RunNodeController() - kc.RunScheduler() - - _, _, _, binderClient, err := oc.GetServiceAccountClients(bootstrappolicy.InfraPersistentVolumeBinderControllerServiceAccountName) - if err != nil { - glog.Fatalf("Could not get client for persistent volume binder controller: %v", err) - } - - _, _, _, attachDetachControllerClient, err := oc.GetServiceAccountClients(bootstrappolicy.InfraPersistentVolumeAttachDetachControllerServiceAccountName) - if err != nil { - glog.Fatalf("Could not get client for attach detach controller: %v", err) - } - - _, _, _, serviceLoadBalancerClient, err := oc.GetServiceAccountClients(bootstrappolicy.InfraServiceLoadBalancerControllerServiceAccountName) - if err != nil { - glog.Fatalf("Could not get client for pod gc controller: %v", err) - } - kc.RunPersistentVolumeController(binderClient, oc.Options.PolicyConfig.OpenShiftInfrastructureNamespace, oc.ImageFor("recycler"), bootstrappolicy.InfraPersistentVolumeRecyclerControllerServiceAccountName) - kc.RunPersistentVolumeAttachDetachController(attachDetachControllerClient) - kc.RunServiceLoadBalancerController(serviceLoadBalancerClient) - - glog.Infof("Started Kubernetes Controllers") - openshiftControllerContext.Stop = controllerContext.Stop - - openshiftControllerInitializers, err := oc.NewOpenshiftControllerInitializers() - if err != nil { - return err - } - - allowedOpenshiftControllers := sets.NewString( + // TODO we manage this one differently, so its wired by openshift but overrides upstreams "serviceaccount", - "serviceaccount-pull-secrets", - "origin-namespace", - "deployer", - "deploymentconfig", - "deploymenttrigger", + + "openshift.io/serviceaccount-pull-secrets", + "openshift.io/origin-namespace", + "openshift.io/deployer", + "openshift.io/deploymentconfig", + "openshift.io/deploymenttrigger", ) if configapi.IsBuildEnabled(&oc.Options) { - allowedOpenshiftControllers.Insert("build") - allowedOpenshiftControllers.Insert("build-pod") - allowedOpenshiftControllers.Insert("build-config-change") + allowedControllers.Insert("openshift.io/build") + allowedControllers.Insert("openshift.io/build-pod") + allowedControllers.Insert("openshift.io/build-config-change") } if oc.Options.TemplateServiceBrokerConfig != nil { - allowedOpenshiftControllers.Insert("templateinstance") + allowedControllers.Insert("openshift.io/templateinstance") } for controllerName, initFn := range openshiftControllerInitializers { // TODO remove this. Only call one to start to prove the principle - if !allowedOpenshiftControllers.Has(controllerName) { + if !allowedControllers.Has(controllerName) { glog.Warningf("%q is skipped", controllerName) continue } diff --git a/test/integration/buildcontroller_test.go b/test/integration/buildcontroller_test.go index a9e0c4132b31..2223dc443cdc 100644 --- a/test/integration/buildcontroller_test.go +++ b/test/integration/buildcontroller_test.go @@ -153,13 +153,13 @@ func setupBuildControllerTest(counts controllerCount, t *testing.T) (*client.Cli openshiftControllerInitializers, err := openshiftConfig.NewOpenshiftControllerInitializers() for i := 0; i < counts.BuildControllers; i++ { - _, err := openshiftControllerInitializers["build"](openshiftControllerContext) + _, err := openshiftControllerInitializers["openshift.io/build"](openshiftControllerContext) if err != nil { t.Fatal(err) } } for i := 0; i < counts.BuildPodControllers; i++ { - _, err := openshiftControllerInitializers["build-pod"](openshiftControllerContext) + _, err := openshiftControllerInitializers["openshift.io/build-pod"](openshiftControllerContext) if err != nil { t.Fatal(err) } @@ -168,7 +168,7 @@ func setupBuildControllerTest(counts controllerCount, t *testing.T) (*client.Cli openshiftConfig.RunImageTriggerController() } for i := 0; i < counts.ConfigChangeControllers; i++ { - _, err := openshiftControllerInitializers["build-config-change"](openshiftControllerContext) + _, err := openshiftControllerInitializers["openshift.io/build-config-change"](openshiftControllerContext) if err != nil { t.Fatal(err) }