Skip to content

simplify controller wiring #14333

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

Closed
deads2k wants to merge 1 commit into from
Closed

simplify controller wiring #14333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 21 additions & 19 deletions pkg/cmd/server/origin/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
Expand All @@ -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)
}
}
Expand All @@ -63,27 +63,29 @@ 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
}

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
Copy link
Contributor

@mfojtik mfojtik May 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

openshift.io/controllers/ ?

how about some generator with:

ret.AddOpenShiftController(controller.RunServiceAccount...)
ret.AddKubeController(...)

return ret.InitializersMap()

too much?

Copy link
Contributor Author

@deads2k deads2k Jun 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

openshift.io/controllers/ ?

how about some generator with:

ret.AddOpenShiftController(controller.RunServiceAccount...)
ret.AddKubeController(...)

return ret.InitializersMap()
too much?

I think we actually end up creating a method to create NewKubeControllerInitializers and then one method that does NewAllControllerInitializers or something. That will help us prep for splitting apart.

ret["openshift.io/origin-namespace"] = controller.RunOriginNamespaceController

// initialize build controller
storageVersion := c.Options.EtcdStorageConfig.OpenShiftStorageVersion
Expand All @@ -97,26 +99,26 @@ 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()
if err != nil {
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
}
7 changes: 7 additions & 0 deletions pkg/cmd/server/origin/controller/interfaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
4 changes: 2 additions & 2 deletions pkg/cmd/server/origin/controller/serviceaccount.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,15 @@ func (c *ServiceAccountControllerOptions) RunController(ctx ControllerContext) (
return true, nil
}

type ServiceAccountTokensControllerOptions struct {
type ServiceAccountTokenControllerOptions struct {
RootCA []byte
ServiceServingCA []byte
PrivateKey interface{}

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(),
Expand Down
156 changes: 63 additions & 93 deletions pkg/cmd/server/start/start_master.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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
Expand All @@ -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",
Expand All @@ -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
}
Expand Down
6 changes: 3 additions & 3 deletions test/integration/buildcontroller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -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)
}
Expand Down