From 9630180b7543b4bb2fde9df2f844ee7bf5e2ecad Mon Sep 17 00:00:00 2001 From: Jim Minter Date: Wed, 17 May 2017 18:14:18 +0100 Subject: [PATCH] template broker should use SAR, not impersonation --- pkg/authorization/authorizer/subjects_test.go | 1 + pkg/authorization/util/util.go | 41 +++ pkg/client/cache/index.go | 6 - pkg/client/cache/template.go | 49 ---- pkg/cmd/server/bootstrappolicy/constants.go | 4 +- .../server/bootstrappolicy/infra_sa_policy.go | 85 +++++- pkg/cmd/server/bootstrappolicy/policy.go | 16 ++ pkg/cmd/server/origin/master.go | 9 +- pkg/cmd/server/origin/master_config.go | 9 + pkg/cmd/server/origin/run_components.go | 14 +- pkg/cmd/server/start/start_master.go | 2 + pkg/controller/shared/shared_informer.go | 5 - pkg/controller/shared/template_informers.go | 63 ----- pkg/template/api/constants.go | 4 + pkg/template/api/helpers.go | 11 + .../controller/templateinstance_controller.go | 251 +++++++++++++----- .../internalversion/expansion_generated.go | 8 - .../internalversion/template_expansion.go | 27 ++ .../template/v1/expansion_generated.go | 8 - .../listers/template/v1/template_expansion.go | 27 ++ .../registry/templateinstance/etcd/etcd.go | 39 ++- .../registry/templateinstance/strategy.go | 75 ++++-- pkg/template/servicebroker/bind.go | 44 ++- pkg/template/servicebroker/catalog.go | 8 +- pkg/template/servicebroker/deprovision.go | 6 +- pkg/template/servicebroker/lastoperation.go | 4 + pkg/template/servicebroker/provision.go | 115 +++++--- pkg/template/servicebroker/servicebroker.go | 101 ++++--- .../test-scripts/clusterrole.yaml | 4 +- pkg/template/servicebroker/unbind.go | 4 + .../templateinstance_impersonation.go | 48 ++-- .../templates/templateinstance_security.go | 6 +- .../templates/templateservicebroker_e2e.go | 4 +- .../templateservicebroker_security.go | 2 +- test/integration/authorization_test.go | 5 +- .../bootstrap_cluster_role_bindings.yaml | 14 + .../bootstrap_cluster_roles.yaml | 85 +++++- 37 files changed, 869 insertions(+), 335 deletions(-) create mode 100644 pkg/authorization/util/util.go delete mode 100644 pkg/client/cache/template.go delete mode 100644 pkg/controller/shared/template_informers.go create mode 100644 pkg/template/generated/listers/template/internalversion/template_expansion.go create mode 100644 pkg/template/generated/listers/template/v1/template_expansion.go diff --git a/pkg/authorization/authorizer/subjects_test.go b/pkg/authorization/authorizer/subjects_test.go index 11b3738ad239..02cdacb1d7f7 100644 --- a/pkg/authorization/authorizer/subjects_test.go +++ b/pkg/authorization/authorizer/subjects_test.go @@ -49,6 +49,7 @@ func TestSubjects(t *testing.T) { "system:serviceaccount:openshift-infra:build-controller", "system:serviceaccount:openshift-infra:deployer-controller", "system:serviceaccount:openshift-infra:deploymentconfig-controller", + "system:serviceaccount:openshift-infra:template-instance-controller", ), expectedGroups: sets.NewString("RootUsers", "system:cluster-admins", "system:cluster-readers", "system:masters", "system:nodes"), } diff --git a/pkg/authorization/util/util.go b/pkg/authorization/util/util.go new file mode 100644 index 000000000000..88aa8768a181 --- /dev/null +++ b/pkg/authorization/util/util.go @@ -0,0 +1,41 @@ +package util + +import ( + "errors" + + kerrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apiserver/pkg/authentication/user" + "k8s.io/kubernetes/pkg/apis/authorization" + "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/authorization/internalversion" +) + +func AddUserToSAR(user user.Info, sar *authorization.SubjectAccessReview) *authorization.SubjectAccessReview { + sar.Spec.User = user.GetName() + copy(sar.Spec.Groups, user.GetGroups()) + sar.Spec.Extra = map[string]authorization.ExtraValue{} + + for k, v := range user.GetExtra() { + sar.Spec.Extra[k] = authorization.ExtraValue(v) + } + + return sar +} + +func Authorize(sarClient internalversion.SubjectAccessReviewInterface, user user.Info, resourceAttributes *authorization.ResourceAttributes) error { + sar := AddUserToSAR(user, &authorization.SubjectAccessReview{ + Spec: authorization.SubjectAccessReviewSpec{ + ResourceAttributes: resourceAttributes, + }, + }) + + resp, err := sarClient.Create(sar) + if err == nil && resp != nil && resp.Status.Allowed { + return nil + } + + if err == nil { + err = errors.New(resp.Status.Reason) + } + return kerrors.NewForbidden(schema.GroupResource{Group: resourceAttributes.Group, Resource: resourceAttributes.Resource}, resourceAttributes.Name, err) +} diff --git a/pkg/client/cache/index.go b/pkg/client/cache/index.go index 00d8ed0afd76..396f2f98d1d2 100644 --- a/pkg/client/cache/index.go +++ b/pkg/client/cache/index.go @@ -7,12 +7,10 @@ import ( buildutil "github.com/openshift/origin/pkg/build/util" deployapi "github.com/openshift/origin/pkg/deploy/api" imageapi "github.com/openshift/origin/pkg/image/api" - templateapi "github.com/openshift/origin/pkg/template/api" ) const ( ImageStreamReferenceIndex = "imagestreamref" - TemplateUIDIndex = "templateuid" ) // ImageStreamReferenceIndexFunc is a default index function that indexes based on image stream references. @@ -75,7 +73,3 @@ func ImageStreamReferenceIndexFunc(obj interface{}) ([]string, error) { } return nil, fmt.Errorf("image stream reference index not implemented for %#v", obj) } - -func TemplateUIDIndexFunc(obj interface{}) ([]string, error) { - return []string{string(obj.(*templateapi.Template).UID)}, nil -} diff --git a/pkg/client/cache/template.go b/pkg/client/cache/template.go deleted file mode 100644 index ae5f5575f4fc..000000000000 --- a/pkg/client/cache/template.go +++ /dev/null @@ -1,49 +0,0 @@ -package cache - -import ( - "k8s.io/client-go/tools/cache" - - templateapi "github.com/openshift/origin/pkg/template/api" -) - -type StoreToTemplateLister interface { - List() ([]*templateapi.Template, error) - ListByNamespace(namespace string) ([]*templateapi.Template, error) - GetByUID(uid string) (*templateapi.Template, error) -} - -type StoreToTemplateListerImpl struct { - cache.Indexer -} - -func (s *StoreToTemplateListerImpl) List() ([]*templateapi.Template, error) { - list := s.Indexer.List() - - templates := make([]*templateapi.Template, len(list)) - for i, template := range list { - templates[i] = template.(*templateapi.Template) - } - return templates, nil -} - -func (s *StoreToTemplateListerImpl) GetByUID(uid string) (*templateapi.Template, error) { - templates, err := s.Indexer.ByIndex(TemplateUIDIndex, uid) - if err != nil || len(templates) == 0 { - return nil, err - } - return templates[0].(*templateapi.Template), nil -} - -func (s *StoreToTemplateListerImpl) ListByNamespace(namespace string) ([]*templateapi.Template, error) { - list, err := s.Indexer.ByIndex(cache.NamespaceIndex, namespace) - if err != nil { - return nil, err - } - - templates := make([]*templateapi.Template, len(list)) - for i, template := range list { - templates[i] = template.(*templateapi.Template) - } - - return templates, nil -} diff --git a/pkg/cmd/server/bootstrappolicy/constants.go b/pkg/cmd/server/bootstrappolicy/constants.go index 88a636cf430f..c3a5b838e65c 100644 --- a/pkg/cmd/server/bootstrappolicy/constants.go +++ b/pkg/cmd/server/bootstrappolicy/constants.go @@ -62,7 +62,7 @@ const ( RegistryViewerRoleName = "registry-viewer" RegistryEditorRoleName = "registry-editor" - TemplateServiceBrokerClientRoleName = "templateservicebroker-client" + TemplateServiceBrokerClientRoleName = "system:openshift:templateservicebroker-client" BuildStrategyDockerRoleName = "system:build-strategy-docker" BuildStrategyCustomRoleName = "system:build-strategy-custom" @@ -129,4 +129,6 @@ const ( BuildStrategyJenkinsPipelineRoleBindingName = BuildStrategyJenkinsPipelineRoleName + "-binding" OpenshiftSharedResourceViewRoleBindingName = OpenshiftSharedResourceViewRoleName + "s" + + TemplateInstanceControllerRoleBindingName = TemplateInstanceControllerRoleName ) diff --git a/pkg/cmd/server/bootstrappolicy/infra_sa_policy.go b/pkg/cmd/server/bootstrappolicy/infra_sa_policy.go index 9d565750c4e4..30d12460601a 100644 --- a/pkg/cmd/server/bootstrappolicy/infra_sa_policy.go +++ b/pkg/cmd/server/bootstrappolicy/infra_sa_policy.go @@ -6,17 +6,17 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" kapi "k8s.io/kubernetes/pkg/api" + "k8s.io/kubernetes/pkg/apis/authorization" "k8s.io/kubernetes/pkg/apis/certificates" "k8s.io/kubernetes/pkg/apis/extensions" "k8s.io/kubernetes/pkg/apis/storage" - // we need the conversions registered for our init block authorizationapi "github.com/openshift/origin/pkg/authorization/api" - _ "github.com/openshift/origin/pkg/authorization/api/install" authorizationapiv1 "github.com/openshift/origin/pkg/authorization/api/v1" buildapi "github.com/openshift/origin/pkg/build/api" deployapi "github.com/openshift/origin/pkg/deploy/api" imageapi "github.com/openshift/origin/pkg/image/api" + templateapi "github.com/openshift/origin/pkg/template/api" // we need the conversions registered for our init block _ "github.com/openshift/origin/pkg/authorization/api/install" @@ -56,6 +56,17 @@ const ( InfraNodeBootstrapServiceAccountName = "node-bootstrapper" NodeBootstrapRoleName = "system:node-bootstrapper" + + // template instance controller watches for TemplateInstance object creation + // and instantiates templates as a result. + InfraTemplateInstanceControllerServiceAccountName = "template-instance-controller" + TemplateInstanceControllerRoleName = "system:openshift:controller:template-instance-controller" + + // template service broker is an open service broker-compliant API + // implementation which serves up OpenShift templates. It uses the + // TemplateInstance backend for most of the heavy lifting. + InfraTemplateServiceBrokerServiceAccountName = "template-service-broker" + TemplateServiceBrokerControllerRoleName = "system:openshift:template-service-broker" ) type InfraServiceAccounts struct { @@ -588,4 +599,74 @@ func init() { panic(err) } + err = InfraSAs.addServiceAccount( + InfraTemplateInstanceControllerServiceAccountName, + authorizationapi.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: TemplateInstanceControllerRoleName, + }, + Rules: []authorizationapi.PolicyRule{ + { + APIGroups: []string{authorization.GroupName}, + Verbs: sets.NewString("create"), + Resources: sets.NewString("subjectaccessreviews"), + }, + { + APIGroups: []string{templateapi.GroupName}, + Verbs: sets.NewString("get", "list", "watch"), + Resources: sets.NewString("templateinstances"), + }, + { + APIGroups: []string{templateapi.GroupName}, + Verbs: sets.NewString("update"), + Resources: sets.NewString("templateinstances/status"), + }, + }, + }, + ) + if err != nil { + panic(err) + } + + err = InfraSAs.addServiceAccount( + InfraTemplateServiceBrokerServiceAccountName, + authorizationapi.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: TemplateServiceBrokerControllerRoleName, + }, + Rules: []authorizationapi.PolicyRule{ + { + APIGroups: []string{authorization.GroupName}, + Verbs: sets.NewString("create"), + Resources: sets.NewString("subjectaccessreviews"), + }, + { + APIGroups: []string{templateapi.GroupName}, + Verbs: sets.NewString("get", "create", "update", "delete"), + Resources: sets.NewString("brokertemplateinstances"), + }, + { + APIGroups: []string{templateapi.GroupName}, + // "assign" is required for the API server to accept creation of + // TemplateInstance objects with the requester username set to an + // identity which is not the API caller. + Verbs: sets.NewString("get", "create", "delete", "assign"), + Resources: sets.NewString("templateinstances"), + }, + { + APIGroups: []string{kapi.GroupName}, + Verbs: sets.NewString("get", "list", "create", "delete"), + Resources: sets.NewString("secrets"), + }, + { + APIGroups: []string{kapi.GroupName}, + Verbs: sets.NewString("list"), + Resources: sets.NewString("services"), + }, + }, + }, + ) + if err != nil { + panic(err) + } } diff --git a/pkg/cmd/server/bootstrappolicy/policy.go b/pkg/cmd/server/bootstrappolicy/policy.go index b97ccdc15332..3c19fae6cccb 100644 --- a/pkg/cmd/server/bootstrappolicy/policy.go +++ b/pkg/cmd/server/bootstrappolicy/policy.go @@ -1185,6 +1185,22 @@ func GetOpenshiftBootstrapClusterRoleBindings() []authorizationapi.ClusterRoleBi RoleRef: kapi.ObjectReference{Name: BuildStrategyJenkinsPipelineRoleName}, Subjects: []kapi.ObjectReference{{Kind: authorizationapi.SystemGroupKind, Name: AuthenticatedGroup}}, }, + + { + ObjectMeta: metav1.ObjectMeta{ + Name: TemplateInstanceControllerRoleBindingName, + }, + RoleRef: kapi.ObjectReference{ + Name: EditRoleName, + }, + Subjects: []kapi.ObjectReference{ + { + Kind: authorizationapi.ServiceAccountKind, + Name: InfraTemplateInstanceControllerServiceAccountName, + Namespace: DefaultOpenShiftInfraNamespace, + }, + }, + }, } } diff --git a/pkg/cmd/server/origin/master.go b/pkg/cmd/server/origin/master.go index 9be6ac9d4314..3bd1f9540a8e 100644 --- a/pkg/cmd/server/origin/master.go +++ b/pkg/cmd/server/origin/master.go @@ -512,9 +512,9 @@ func (c *MasterConfig) InstallProtectedAPI(server *apiserver.GenericAPIServer) ( templateapi.ServiceBrokerRoot, templateservicebroker.NewBroker( c.PrivilegedLoopbackClientConfig, - c.PrivilegedLoopbackOpenShiftClient, - c.PrivilegedLoopbackKubernetesClientsetInternal.Core(), - c.Informers, + c.PrivilegedLoopbackKubernetesClientsetInternal, + c.Options.PolicyConfig.OpenShiftInfrastructureNamespace, + c.TemplateInformers.Template().InternalVersion().Templates(), c.Options.TemplateServiceBrokerConfig.TemplateNamespaces, ), ) @@ -895,12 +895,13 @@ func (c *MasterConfig) GetRestStorage() map[schema.GroupVersion]map[string]rest. } if c.Options.TemplateServiceBrokerConfig != nil { - templateInstanceStorage, err := templateinstanceetcd.NewREST(c.RESTOptionsGetter, c.PrivilegedLoopbackOpenShiftClient) + templateInstanceStorage, templateInstanceStatusStorage, err := templateinstanceetcd.NewREST(c.RESTOptionsGetter, c.PrivilegedLoopbackKubernetesClientsetInternal) checkStorageErr(err) brokerTemplateInstanceStorage, err := brokertemplateinstanceetcd.NewREST(c.RESTOptionsGetter) checkStorageErr(err) storage[templateapiv1.SchemeGroupVersion]["templateinstances"] = templateInstanceStorage + storage[templateapiv1.SchemeGroupVersion]["templateinstances/status"] = templateInstanceStatusStorage storage[templateapiv1.SchemeGroupVersion]["brokertemplateinstances"] = brokerTemplateInstanceStorage } diff --git a/pkg/cmd/server/origin/master_config.go b/pkg/cmd/server/origin/master_config.go index c89ab4951b3e..c5ab90cb8912 100644 --- a/pkg/cmd/server/origin/master_config.go +++ b/pkg/cmd/server/origin/master_config.go @@ -97,6 +97,8 @@ import ( "github.com/openshift/origin/pkg/service" serviceadmit "github.com/openshift/origin/pkg/service/admission" "github.com/openshift/origin/pkg/serviceaccounts" + templateinformer "github.com/openshift/origin/pkg/template/generated/informers/internalversion" + templateclient "github.com/openshift/origin/pkg/template/generated/internalclientset" usercache "github.com/openshift/origin/pkg/user/cache" groupregistry "github.com/openshift/origin/pkg/user/registry/group" groupstorage "github.com/openshift/origin/pkg/user/registry/group/etcd" @@ -185,6 +187,7 @@ type MasterConfig struct { Informers shared.InformerFactory AuthorizationInformers authorizationinformer.SharedInformerFactory + TemplateInformers templateinformer.SharedInformerFactory } // BuildMasterConfig builds and returns the OpenShift master configuration based on the @@ -225,6 +228,10 @@ func BuildMasterConfig(options configapi.MasterConfig) (*MasterConfig, error) { if err != nil { return nil, err } + templateClient, err := templateclient.NewForConfig(privilegedLoopbackClientConfig) + if err != nil { + return nil, err + } customListerWatchers := shared.DefaultListerWatcherOverrides{} if err := addAuthorizationListerWatchers(customListerWatchers, restOptsGetter); err != nil { @@ -236,6 +243,7 @@ func BuildMasterConfig(options configapi.MasterConfig) (*MasterConfig, error) { internalkubeInformerFactory := kinternalinformers.NewSharedInformerFactory(privilegedLoopbackKubeClientsetInternal, defaultInformerResyncPeriod) externalkubeInformerFactory := kinformers.NewSharedInformerFactory(privilegedLoopbackKubeClientsetExternal, defaultInformerResyncPeriod) authorizationInformers := authorizationinformer.NewSharedInformerFactory(authorizationClient, defaultInformerResyncPeriod) + templateInformers := templateinformer.NewSharedInformerFactory(templateClient, defaultInformerResyncPeriod) informerFactory := shared.NewInformerFactory(internalkubeInformerFactory, externalkubeInformerFactory, privilegedLoopbackKubeClientsetInternal, privilegedLoopbackOpenShiftClient, customListerWatchers, defaultInformerResyncPeriod) imageTemplate := variable.NewDefaultImageTemplate() @@ -346,6 +354,7 @@ func BuildMasterConfig(options configapi.MasterConfig) (*MasterConfig, error) { PrivilegedLoopbackKubernetesClientsetExternal: privilegedLoopbackKubeClientsetExternal, Informers: informerFactory, AuthorizationInformers: authorizationInformers, + TemplateInformers: templateInformers, } // ensure that the limit range informer will be started diff --git a/pkg/cmd/server/origin/run_components.go b/pkg/cmd/server/origin/run_components.go index 2149175357dc..34e285f17acd 100644 --- a/pkg/cmd/server/origin/run_components.go +++ b/pkg/cmd/server/origin/run_components.go @@ -37,6 +37,7 @@ import ( osclient "github.com/openshift/origin/pkg/client" oscache "github.com/openshift/origin/pkg/client/cache" configapi "github.com/openshift/origin/pkg/cmd/server/api" + "github.com/openshift/origin/pkg/cmd/server/bootstrappolicy" "github.com/openshift/origin/pkg/cmd/server/crypto" cmdutil "github.com/openshift/origin/pkg/cmd/util" deployclient "github.com/openshift/origin/pkg/deploy/generated/internalclientset/typed/apps/internalversion" @@ -59,6 +60,7 @@ import ( servingcertcontroller "github.com/openshift/origin/pkg/service/controller/servingcert" serviceaccountcontrollers "github.com/openshift/origin/pkg/serviceaccounts/controllers" templatecontroller "github.com/openshift/origin/pkg/template/controller" + templateclient "github.com/openshift/origin/pkg/template/generated/internalclientset" unidlingcontroller "github.com/openshift/origin/pkg/unidling/controller" ) @@ -569,7 +571,17 @@ func (c *MasterConfig) RunUnidlingController() { } func (c *MasterConfig) RunTemplateController() { - go templatecontroller.NewTemplateInstanceController(c.PrivilegedLoopbackClientConfig).Run(utilwait.NeverStop) + config, oc, kc, _, err := c.GetServiceAccountClients(bootstrappolicy.InfraTemplateInstanceControllerServiceAccountName) + if err != nil { + glog.Fatalf("Could not get client: %v", err) + } + + templateClient, err := templateclient.NewForConfig(config) + if err != nil { + glog.Fatalf("Could not get client: %v", err) + } + + go templatecontroller.NewTemplateInstanceController(oc, kc, templateClient.TemplateClient, c.TemplateInformers.Template().InternalVersion().TemplateInstances()).Run(5, utilwait.NeverStop) } func (c *MasterConfig) RunOriginToRBACSyncControllers() { diff --git a/pkg/cmd/server/start/start_master.go b/pkg/cmd/server/start/start_master.go index ec5b4093656c..df52080c3def 100644 --- a/pkg/cmd/server/start/start_master.go +++ b/pkg/cmd/server/start/start_master.go @@ -447,12 +447,14 @@ func (m *Master) Start() error { openshiftConfig.Informers.Start(utilwait.NeverStop) openshiftConfig.Informers.StartCore(utilwait.NeverStop) openshiftConfig.AuthorizationInformers.Start(utilwait.NeverStop) + openshiftConfig.TemplateInformers.Start(utilwait.NeverStop) }() } else { openshiftConfig.Informers.InternalKubernetesInformers().Start(utilwait.NeverStop) openshiftConfig.Informers.KubernetesInformers().Start(utilwait.NeverStop) openshiftConfig.Informers.Start(utilwait.NeverStop) openshiftConfig.AuthorizationInformers.Start(utilwait.NeverStop) + openshiftConfig.TemplateInformers.Start(utilwait.NeverStop) } return nil diff --git a/pkg/controller/shared/shared_informer.go b/pkg/controller/shared/shared_informer.go index bc9d5ad8f94d..b39b6e2360e5 100644 --- a/pkg/controller/shared/shared_informer.go +++ b/pkg/controller/shared/shared_informer.go @@ -27,7 +27,6 @@ type InformerFactory interface { DeploymentConfigs() DeploymentConfigInformer BuildConfigs() BuildConfigInformer - Templates() TemplateInformer Builds() BuildInformer ImageStreams() ImageStreamInformer SecurityContextConstraints() SecurityContextConstraintsInformer @@ -137,10 +136,6 @@ func (f *sharedInformerFactory) BuildConfigs() BuildConfigInformer { return &buildConfigInformer{sharedInformerFactory: f} } -func (f *sharedInformerFactory) Templates() TemplateInformer { - return &templateInformer{sharedInformerFactory: f} -} - func (f *sharedInformerFactory) Builds() BuildInformer { return &buildInformer{sharedInformerFactory: f} } diff --git a/pkg/controller/shared/template_informers.go b/pkg/controller/shared/template_informers.go deleted file mode 100644 index 6111e9055c39..000000000000 --- a/pkg/controller/shared/template_informers.go +++ /dev/null @@ -1,63 +0,0 @@ -package shared - -import ( - "reflect" - - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/watch" - "k8s.io/client-go/tools/cache" - kapi "k8s.io/kubernetes/pkg/api" - - oscache "github.com/openshift/origin/pkg/client/cache" - templateapi "github.com/openshift/origin/pkg/template/api" -) - -type TemplateInformer interface { - Informer() cache.SharedIndexInformer - Indexer() cache.Indexer - Lister() oscache.StoreToTemplateLister -} - -type templateInformer struct { - *sharedInformerFactory -} - -func (f *templateInformer) Informer() cache.SharedIndexInformer { - f.lock.Lock() - defer f.lock.Unlock() - - informerObj := &templateapi.Template{} - informerType := reflect.TypeOf(informerObj) - informer, exists := f.informers[informerType] - if exists { - return informer - } - - informer = cache.NewSharedIndexInformer( - &cache.ListWatch{ - ListFunc: func(options metav1.ListOptions) (runtime.Object, error) { - return f.originClient.Templates(kapi.NamespaceAll).List(options) - }, - WatchFunc: func(options metav1.ListOptions) (watch.Interface, error) { - return f.originClient.Templates(kapi.NamespaceAll).Watch(options) - }, - }, - informerObj, - f.defaultResync, - cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc, oscache.TemplateUIDIndex: oscache.TemplateUIDIndexFunc}, - ) - f.informers[informerType] = informer - - return informer -} - -func (f *templateInformer) Indexer() cache.Indexer { - informer := f.Informer() - return informer.GetIndexer() -} - -func (f *templateInformer) Lister() oscache.StoreToTemplateLister { - informer := f.Informer() - return &oscache.StoreToTemplateListerImpl{Indexer: informer.GetIndexer()} -} diff --git a/pkg/template/api/constants.go b/pkg/template/api/constants.go index 832f7661717b..6cd2e531a01b 100644 --- a/pkg/template/api/constants.go +++ b/pkg/template/api/constants.go @@ -43,4 +43,8 @@ const ( // ServiceMetadataIconClass is the key for the template iconClass as returned // in the services.metadata map from a service broker catalog response ServiceMetadataIconClass = "console.openshift.io/iconClass" + + // TemplateUIDIndex is the name of an index on the generated template lister, + // initialised and used by the template service broker. + TemplateUIDIndex = "templateuid" ) diff --git a/pkg/template/api/helpers.go b/pkg/template/api/helpers.go index 3cc614d34b5b..1d83c72f0d37 100644 --- a/pkg/template/api/helpers.go +++ b/pkg/template/api/helpers.go @@ -45,5 +45,16 @@ func AddObjectsToTemplate(template *Template, objects []runtime.Object, targetVe } return nil +} + +func FilterTemplateInstanceCondition(conditions []TemplateInstanceCondition, condType TemplateInstanceConditionType) []TemplateInstanceCondition { + newConditions := make([]TemplateInstanceCondition, 0, len(conditions)+1) + + for _, c := range conditions { + if c.Type != condType { + newConditions = append(newConditions, c) + } + } + return newConditions } diff --git a/pkg/template/controller/templateinstance_controller.go b/pkg/template/controller/templateinstance_controller.go index cecc648a36c5..6e0e940966aa 100644 --- a/pkg/template/controller/templateinstance_controller.go +++ b/pkg/template/controller/templateinstance_controller.go @@ -1,68 +1,101 @@ package controller import ( + "errors" "fmt" + "time" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + utilerrors "k8s.io/apimachinery/pkg/util/errors" utilruntime "k8s.io/apimachinery/pkg/util/runtime" - watch "k8s.io/apimachinery/pkg/watch" + "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apiserver/pkg/authentication/user" - "k8s.io/client-go/rest" "k8s.io/client-go/tools/cache" + "k8s.io/client-go/util/workqueue" kapi "k8s.io/kubernetes/pkg/api" + "k8s.io/kubernetes/pkg/apis/authorization" + kclientsetinternal "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset" "k8s.io/kubernetes/pkg/kubectl/resource" "github.com/openshift/origin/pkg/api/latest" - authclient "github.com/openshift/origin/pkg/auth/client" + "github.com/openshift/origin/pkg/authorization/util" "github.com/openshift/origin/pkg/client" "github.com/openshift/origin/pkg/config/cmd" templateapi "github.com/openshift/origin/pkg/template/api" templateapiv1 "github.com/openshift/origin/pkg/template/api/v1" - templateclientset "github.com/openshift/origin/pkg/template/generated/internalclientset" + "github.com/openshift/origin/pkg/template/generated/informers/internalversion/template/internalversion" internalversiontemplate "github.com/openshift/origin/pkg/template/generated/internalclientset/typed/template/internalversion" + templatelister "github.com/openshift/origin/pkg/template/generated/listers/template/internalversion" ) +const updateRetries = 3 + type TemplateInstanceController struct { - restconfig rest.Config + oc *client.Client + kc kclientsetinternal.Interface templateclient internalversiontemplate.TemplateInterface - controller cache.Controller + + lister templatelister.TemplateInstanceLister + informer cache.SharedIndexInformer + + queue workqueue.RateLimitingInterface } -func NewTemplateInstanceController(restconfig rest.Config) *TemplateInstanceController { - c := TemplateInstanceController{restconfig: restconfig, templateclient: templateclientset.NewForConfigOrDie(&restconfig).Template()} - _, c.controller = cache.NewInformer( - &cache.ListWatch{ - ListFunc: func(options metav1.ListOptions) (runtime.Object, error) { - return c.templateclient.TemplateInstances(kapi.NamespaceAll).List(options) - }, - WatchFunc: func(options metav1.ListOptions) (watch.Interface, error) { - return c.templateclient.TemplateInstances(kapi.NamespaceAll).Watch(options) - }, +func NewTemplateInstanceController(oc *client.Client, kc kclientsetinternal.Interface, templateclient internalversiontemplate.TemplateInterface, informer internalversion.TemplateInstanceInformer) *TemplateInstanceController { + c := &TemplateInstanceController{ + oc: oc, + kc: kc, + templateclient: templateclient, + lister: informer.Lister(), + informer: informer.Informer(), + queue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "TemplateInstanceController"), + } + + c.informer.AddEventHandler(cache.ResourceEventHandlerFuncs{ + AddFunc: func(obj interface{}) { + c.enqueue(obj.(*templateapi.TemplateInstance)) }, - &templateapi.TemplateInstance{}, - 0, - cache.ResourceEventHandlerFuncs{ - AddFunc: func(obj interface{}) { - c.handle(obj.(*templateapi.TemplateInstance)) - }, - UpdateFunc: func(_, obj interface{}) { - c.handle(obj.(*templateapi.TemplateInstance)) - }, - DeleteFunc: func(obj interface{}) { - }, + UpdateFunc: func(_, obj interface{}) { + c.enqueue(obj.(*templateapi.TemplateInstance)) }, - ) + DeleteFunc: func(obj interface{}) { + }, + }) - return &c + return c } -func (c *TemplateInstanceController) Run(stop <-chan struct{}) { - c.controller.Run(stop) +func (c *TemplateInstanceController) getTemplateInstanceCopy(key string) (*templateapi.TemplateInstance, error) { + namespace, name, err := cache.SplitMetaNamespaceKey(key) + if err != nil { + return nil, err + } + + templateInstance, err := c.lister.TemplateInstances(namespace).Get(name) + if err != nil { + return nil, err + } + + templateInstanceCopy, err := kapi.Scheme.DeepCopy(templateInstance) + if err != nil { + return nil, err + } + + return templateInstanceCopy.(*templateapi.TemplateInstance), nil } -func (c *TemplateInstanceController) handle(templateInstance *templateapi.TemplateInstance) error { +func (c *TemplateInstanceController) sync(key string) error { + templateInstance, err := c.getTemplateInstanceCopy(key) + if apierrors.IsNotFound(err) { + return nil + } + if err != nil { + return err + } + for _, condition := range templateInstance.Status.Conditions { if condition.Type == templateapi.TemplateInstanceReady && condition.Status == kapi.ConditionTrue || condition.Type == templateapi.TemplateInstanceInstantiateFailure && condition.Status == kapi.ConditionTrue { @@ -70,34 +103,100 @@ func (c *TemplateInstanceController) handle(templateInstance *templateapi.Templa } } - err := c.provision(templateInstance) - if err == nil { - templateInstance.Status.Conditions = []templateapi.TemplateInstanceCondition{ - { + provisionErr := c.provision(templateInstance) + + for try := 0; try < updateRetries; try++ { + templateInstance.Status.Conditions = templateapi.FilterTemplateInstanceCondition(templateInstance.Status.Conditions, templateapi.TemplateInstanceReady) + templateInstance.Status.Conditions = templateapi.FilterTemplateInstanceCondition(templateInstance.Status.Conditions, templateapi.TemplateInstanceInstantiateFailure) + + if provisionErr == nil { + templateInstance.Status.Conditions = append(templateInstance.Status.Conditions, templateapi.TemplateInstanceCondition{ Type: templateapi.TemplateInstanceReady, Status: kapi.ConditionTrue, LastTransitionTime: metav1.Now(), Reason: "Created", - }, - } + }) - } else { - templateInstance.Status.Conditions = []templateapi.TemplateInstanceCondition{ - { + } else { + templateInstance.Status.Conditions = append(templateInstance.Status.Conditions, templateapi.TemplateInstanceCondition{ Type: templateapi.TemplateInstanceInstantiateFailure, Status: kapi.ConditionTrue, LastTransitionTime: metav1.Now(), Reason: "Failed", - Message: err.Error(), - }, + Message: provisionErr.Error(), + }) + } + + _, err = c.templateclient.TemplateInstances(templateInstance.Namespace).UpdateStatus(templateInstance) + if err == nil { + break + } + + utilruntime.HandleError(fmt.Errorf("TemplateInstance status update failed (%d/%d): %v", try+1, updateRetries, err)) + + if apierrors.IsConflict(err) && try+1 < updateRetries { + // we can re-Get only because TemplateInstance.Spec is immutable. + templateInstance, err = c.getTemplateInstanceCopy(key) + if err != nil { + utilruntime.HandleError(fmt.Errorf("TemplateInstance get failed: %v", err)) + if apierrors.IsNotFound(err) { + return nil + } + return err + } } } - _, err = c.templateclient.TemplateInstances(templateInstance.Namespace).Update(templateInstance) + return err +} + +func (c *TemplateInstanceController) Run(workers int, stopCh <-chan struct{}) { + defer utilruntime.HandleCrash() + defer c.queue.ShutDown() + + if !cache.WaitForCacheSync(stopCh, c.informer.HasSynced) { + return + } + + for i := 0; i < workers; i++ { + go wait.Until(c.runWorker, time.Second, stopCh) + } + + <-stopCh +} + +func (c *TemplateInstanceController) runWorker() { + for c.processNextWorkItem() { + } +} + +func (c *TemplateInstanceController) processNextWorkItem() bool { + key, quit := c.queue.Get() + if quit { + return false + } + defer c.queue.Done(key) + + err := c.sync(key.(string)) + if err == nil { + c.queue.Forget(key) + return true + } + + utilruntime.HandleError(fmt.Errorf("%v failed with: %v", key, err)) + c.queue.AddRateLimited(key) + + return true +} + +func (c *TemplateInstanceController) enqueue(obj interface{}) { + key, err := cache.DeletionHandlingMetaNamespaceKeyFunc(obj) if err != nil { - utilruntime.HandleError(fmt.Errorf("TemplateInstance update failed: %v", err)) + utilruntime.HandleError(fmt.Errorf("Couldn't get key for object %#v: %v", obj, err)) + return } - return err + + c.queue.Add(key) } func (c *TemplateInstanceController) provision(templateInstance *templateapi.TemplateInstance) error { @@ -107,18 +206,16 @@ func (c *TemplateInstanceController) provision(templateInstance *templateapi.Tem u := &user.DefaultInfo{Name: templateInstance.Spec.Requester.Username} - impersonatingConfig := authclient.NewImpersonatingConfig(u, c.restconfig) - impersonatedOC, err := client.New(&impersonatingConfig) - if err != nil { - return err - } - - impersonatedKC, err := authclient.NewImpersonatingKubernetesClientset(u, c.restconfig) - if err != nil { + if err := util.Authorize(c.kc.Authorization().SubjectAccessReviews(), u, &authorization.ResourceAttributes{ + Namespace: templateInstance.Namespace, + Verb: "get", + Group: kapi.GroupName, + Resource: "secrets", + }); err != nil { return err } - secret, err := impersonatedKC.Core().Secrets(templateInstance.Namespace).Get(templateInstance.Spec.Secret.Name, metav1.GetOptions{}) + secret, err := c.kc.Core().Secrets(templateInstance.Namespace).Get(templateInstance.Spec.Secret.Name, metav1.GetOptions{}) if err != nil { return err } @@ -141,7 +238,16 @@ func (c *TemplateInstanceController) provision(templateInstance *templateapi.Tem } } - template, err = impersonatedOC.TemplateConfigs(templateInstance.Namespace).Create(template) + if err := util.Authorize(c.kc.Authorization().SubjectAccessReviews(), u, &authorization.ResourceAttributes{ + Namespace: templateInstance.Namespace, + Verb: "create", + Group: templateapi.GroupName, + Resource: "templateconfigs", + }); err != nil { + return err + } + + template, err = c.oc.TemplateConfigs(templateInstance.Namespace).Create(template) if err != nil { return err } @@ -169,16 +275,41 @@ func (c *TemplateInstanceController) provision(templateInstance *templateapi.Tem ObjectTyper: kapi.Scheme, ClientMapper: resource.ClientMapperFunc(func(mapping *meta.RESTMapping) (resource.RESTClient, error) { if latest.OriginKind(mapping.GroupVersionKind) { - return impersonatedOC, nil + return c.oc, nil } - return impersonatedKC.Core().RESTClient(), nil + return c.kc.Core().RESTClient(), nil }), }, - Op: cmd.Create, + Op: func(info *resource.Info, namespace string, obj runtime.Object) (runtime.Object, error) { + if len(info.Namespace) > 0 { + namespace = info.Namespace + } + if namespace == "" { + return nil, errors.New("namespace was empty") + } + if info.Mapping.Resource == "" { + return nil, errors.New("resource was empty") + } + if err := util.Authorize(c.kc.Authorization().SubjectAccessReviews(), u, &authorization.ResourceAttributes{ + Namespace: namespace, + Verb: "create", + Group: info.Mapping.GroupVersionKind.Group, + Resource: info.Mapping.Resource, + }); err != nil { + return nil, err + } + return obj, nil + }, } errs = bulk.Run(&kapi.List{Items: template.Objects}, templateInstance.Namespace) if len(errs) > 0 { - return errs[0] + return utilerrors.NewAggregate(errs) + } + + bulk.Op = cmd.Create + errs = bulk.Run(&kapi.List{Items: template.Objects}, templateInstance.Namespace) + if len(errs) > 0 { + return utilerrors.NewAggregate(errs) } return nil diff --git a/pkg/template/generated/listers/template/internalversion/expansion_generated.go b/pkg/template/generated/listers/template/internalversion/expansion_generated.go index 02a22779ba2b..3e6778d37f77 100644 --- a/pkg/template/generated/listers/template/internalversion/expansion_generated.go +++ b/pkg/template/generated/listers/template/internalversion/expansion_generated.go @@ -6,14 +6,6 @@ package internalversion // BrokerTemplateInstanceLister. type BrokerTemplateInstanceListerExpansion interface{} -// TemplateListerExpansion allows custom methods to be added to -// TemplateLister. -type TemplateListerExpansion interface{} - -// TemplateNamespaceListerExpansion allows custom methods to be added to -// TemplateNamespaeLister. -type TemplateNamespaceListerExpansion interface{} - // TemplateInstanceListerExpansion allows custom methods to be added to // TemplateInstanceLister. type TemplateInstanceListerExpansion interface{} diff --git a/pkg/template/generated/listers/template/internalversion/template_expansion.go b/pkg/template/generated/listers/template/internalversion/template_expansion.go new file mode 100644 index 000000000000..14f97dfd9f2b --- /dev/null +++ b/pkg/template/generated/listers/template/internalversion/template_expansion.go @@ -0,0 +1,27 @@ +package internalversion + +import ( + "github.com/openshift/origin/pkg/template/api" + "k8s.io/apimachinery/pkg/api/errors" +) + +// TemplateListerExpansion allows custom methods to be added to +// TemplateLister. +type TemplateListerExpansion interface { + GetByUID(uid string) (*api.Template, error) +} + +// TemplateNamespaceListerExpansion allows custom methods to be added to +// TemplateNamespaceLister. +type TemplateNamespaceListerExpansion interface{} + +func (s templateLister) GetByUID(uid string) (*api.Template, error) { + templates, err := s.indexer.ByIndex(api.TemplateUIDIndex, uid) + if err != nil { + return nil, err + } + if len(templates) == 0 { + return nil, errors.NewNotFound(api.Resource("template"), uid) + } + return templates[0].(*api.Template), nil +} diff --git a/pkg/template/generated/listers/template/v1/expansion_generated.go b/pkg/template/generated/listers/template/v1/expansion_generated.go index 6c30da309652..da22e3774ebc 100644 --- a/pkg/template/generated/listers/template/v1/expansion_generated.go +++ b/pkg/template/generated/listers/template/v1/expansion_generated.go @@ -6,14 +6,6 @@ package v1 // BrokerTemplateInstanceLister. type BrokerTemplateInstanceListerExpansion interface{} -// TemplateListerExpansion allows custom methods to be added to -// TemplateLister. -type TemplateListerExpansion interface{} - -// TemplateNamespaceListerExpansion allows custom methods to be added to -// TemplateNamespaeLister. -type TemplateNamespaceListerExpansion interface{} - // TemplateInstanceListerExpansion allows custom methods to be added to // TemplateInstanceLister. type TemplateInstanceListerExpansion interface{} diff --git a/pkg/template/generated/listers/template/v1/template_expansion.go b/pkg/template/generated/listers/template/v1/template_expansion.go new file mode 100644 index 000000000000..22f961504d20 --- /dev/null +++ b/pkg/template/generated/listers/template/v1/template_expansion.go @@ -0,0 +1,27 @@ +package v1 + +import ( + "github.com/openshift/origin/pkg/template/api" + "k8s.io/apimachinery/pkg/api/errors" +) + +// TemplateListerExpansion allows custom methods to be added to +// TemplateLister. +type TemplateListerExpansion interface { + GetByUID(uid string) (*api.Template, error) +} + +// TemplateNamespaceListerExpansion allows custom methods to be added to +// TemplateNamespaceLister. +type TemplateNamespaceListerExpansion interface{} + +func (s templateLister) GetByUID(uid string) (*api.Template, error) { + templates, err := s.indexer.ByIndex(api.TemplateUIDIndex, uid) + if err != nil { + return nil, err + } + if len(templates) == 0 { + return nil, errors.NewNotFound(api.Resource("template"), uid) + } + return templates[0].(*api.Template), nil +} diff --git a/pkg/template/registry/templateinstance/etcd/etcd.go b/pkg/template/registry/templateinstance/etcd/etcd.go index e34458c19d83..e460c09e76cd 100644 --- a/pkg/template/registry/templateinstance/etcd/etcd.go +++ b/pkg/template/registry/templateinstance/etcd/etcd.go @@ -1,12 +1,15 @@ package etcd import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/generic" "k8s.io/apiserver/pkg/registry/generic/registry" + kapirest "k8s.io/apiserver/pkg/registry/rest" kapi "k8s.io/kubernetes/pkg/api" + kclientset "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset" - "github.com/openshift/origin/pkg/client" "github.com/openshift/origin/pkg/template/api" rest "github.com/openshift/origin/pkg/template/registry/templateinstance" "github.com/openshift/origin/pkg/util/restoptions" @@ -18,8 +21,8 @@ type REST struct { } // NewREST returns a RESTStorage object that will work against templateinstances. -func NewREST(optsGetter restoptions.Getter, oc *client.Client) (*REST, error) { - strategy := rest.NewStrategy(oc) +func NewREST(optsGetter restoptions.Getter, kc kclientset.Interface) (*REST, *StatusREST, error) { + strategy := rest.NewStrategy(kc) store := ®istry.Store{ Copier: kapi.Scheme, @@ -34,8 +37,34 @@ func NewREST(optsGetter restoptions.Getter, oc *client.Client) (*REST, error) { options := &generic.StoreOptions{RESTOptions: optsGetter, AttrFunc: rest.GetAttrs} if err := store.CompleteWithOptions(options); err != nil { - return nil, err + return nil, nil, err } - return &REST{store}, nil + statusStore := *store + statusStore.UpdateStrategy = rest.StatusStrategy + + return &REST{store}, &StatusREST{&statusStore}, nil +} + +// StatusREST implements the REST endpoint for changing the status of a templateInstance. +type StatusREST struct { + store *registry.Store +} + +// StatusREST implements Patcher +var _ = kapirest.Patcher(&StatusREST{}) + +// New creates a new templateInstance resource +func (r *StatusREST) New() runtime.Object { + return &api.TemplateInstance{} +} + +// Get retrieves the object from the storage. It is required to support Patch. +func (r *StatusREST) Get(ctx request.Context, name string, options *metav1.GetOptions) (runtime.Object, error) { + return r.store.Get(ctx, name, options) +} + +// Update alters the status subset of an object. +func (r *StatusREST) Update(ctx request.Context, name string, objInfo kapirest.UpdatedObjectInfo) (runtime.Object, bool, error) { + return r.store.Update(ctx, name, objInfo) } diff --git a/pkg/template/registry/templateinstance/strategy.go b/pkg/template/registry/templateinstance/strategy.go index a2c4b102d99e..8b5cfdb0873c 100644 --- a/pkg/template/registry/templateinstance/strategy.go +++ b/pkg/template/registry/templateinstance/strategy.go @@ -13,23 +13,23 @@ import ( "k8s.io/apiserver/pkg/storage" "k8s.io/apiserver/pkg/storage/names" kapi "k8s.io/kubernetes/pkg/api" + "k8s.io/kubernetes/pkg/apis/authorization" + kclientset "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset" - authorizationapi "github.com/openshift/origin/pkg/authorization/api" - "github.com/openshift/origin/pkg/client" + "github.com/openshift/origin/pkg/authorization/util" templateapi "github.com/openshift/origin/pkg/template/api" "github.com/openshift/origin/pkg/template/api/validation" - userapi "github.com/openshift/origin/pkg/user/api" ) // templateInstanceStrategy implements behavior for Templates type templateInstanceStrategy struct { runtime.ObjectTyper names.NameGenerator - oc *client.Client + kc kclientset.Interface } -func NewStrategy(oc *client.Client) *templateInstanceStrategy { - return &templateInstanceStrategy{kapi.Scheme, names.SimpleNameGenerator, oc} +func NewStrategy(kc kclientset.Interface) *templateInstanceStrategy { + return &templateInstanceStrategy{kapi.Scheme, names.SimpleNameGenerator, kc} } // NamespaceScoped is true for templateinstances. @@ -39,6 +39,10 @@ func (templateInstanceStrategy) NamespaceScoped() bool { // PrepareForUpdate clears fields that are not allowed to be set by end users on update. func (templateInstanceStrategy) PrepareForUpdate(ctx apirequest.Context, obj, old runtime.Object) { + curr := obj.(*templateapi.TemplateInstance) + prev := old.(*templateapi.TemplateInstance) + + curr.Status = prev.Status } // Canonicalize normalizes the object after validation. @@ -55,6 +59,8 @@ func (templateInstanceStrategy) PrepareForCreate(ctx apirequest.Context, obj run Username: user.GetName(), } } + + templateInstance.Status = templateapi.TemplateInstanceStatus{} } // Validate validates a new templateinstance. @@ -124,20 +130,55 @@ func (s *templateInstanceStrategy) validateImpersonation(templateInstance *templ } if templateInstance.Spec.Requester.Username != userinfo.GetName() { - sar := authorizationapi.AddUserToSAR(userinfo, - &authorizationapi.SubjectAccessReview{ - Action: authorizationapi.Action{ - Verb: "impersonate", - Group: userapi.GroupName, - Resource: authorizationapi.UserResource, - ResourceName: templateInstance.Spec.Requester.Username, - }, - }) - resp, err := s.oc.SubjectAccessReviews().Create(sar) - if err != nil || resp == nil || !resp.Allowed { + if err := util.Authorize(s.kc.Authorization().SubjectAccessReviews(), userinfo, &authorization.ResourceAttributes{ + Namespace: templateInstance.Namespace, + Verb: "assign", + Group: templateapi.GroupName, + Resource: "templateinstances", + }); err != nil { return field.ErrorList{field.Forbidden(field.NewPath("spec.impersonateUser"), "impersonation forbidden")} } } return nil } + +type statusStrategy struct { + runtime.ObjectTyper + names.NameGenerator +} + +var StatusStrategy = statusStrategy{kapi.Scheme, names.SimpleNameGenerator} + +func (statusStrategy) NamespaceScoped() bool { + return true +} + +func (statusStrategy) AllowCreateOnUpdate() bool { + return false +} + +func (statusStrategy) AllowUnconditionalUpdate() bool { + return false +} + +func (statusStrategy) PrepareForCreate(ctx apirequest.Context, obj runtime.Object) { +} + +func (statusStrategy) PrepareForUpdate(ctx apirequest.Context, obj, old runtime.Object) { + curr := obj.(*templateapi.TemplateInstance) + prev := old.(*templateapi.TemplateInstance) + + curr.Spec = prev.Spec +} + +func (statusStrategy) Canonicalize(obj runtime.Object) { +} + +func (statusStrategy) Validate(ctx apirequest.Context, obj runtime.Object) field.ErrorList { + return validation.ValidateTemplateInstance(obj.(*templateapi.TemplateInstance)) +} + +func (statusStrategy) ValidateUpdate(ctx apirequest.Context, obj, old runtime.Object) field.ErrorList { + return validation.ValidateTemplateInstanceUpdate(obj.(*templateapi.TemplateInstance), old.(*templateapi.TemplateInstance)) +} diff --git a/pkg/template/servicebroker/bind.go b/pkg/template/servicebroker/bind.go index 29eb04d1daff..846550a6d500 100644 --- a/pkg/template/servicebroker/bind.go +++ b/pkg/template/servicebroker/bind.go @@ -10,9 +10,11 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/selection" + "k8s.io/apiserver/pkg/authentication/user" kapi "k8s.io/kubernetes/pkg/api" - "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/core/internalversion" + "k8s.io/kubernetes/pkg/apis/authorization" + "github.com/openshift/origin/pkg/authorization/util" "github.com/openshift/origin/pkg/openservicebroker/api" templateapi "github.com/openshift/origin/pkg/template/api" ) @@ -25,10 +27,19 @@ func makeEnvVariableName(str string) string { return strings.ToUpper(strings.Replace(str, "-", "_", -1)) } -func (b *Broker) getServices(impersonatedKC internalversion.ServicesGetter, namespace, instanceID string) (map[string]string, *api.Response) { +func (b *Broker) getServices(u user.Info, namespace, instanceID string) (map[string]string, *api.Response) { requirement, _ := labels.NewRequirement(templateapi.TemplateInstanceLabel, selection.Equals, []string{instanceID}) - serviceList, err := impersonatedKC.Services(namespace).List(metav1.ListOptions{LabelSelector: labels.NewSelector().Add(*requirement).String()}) + if err := util.Authorize(b.kc.Authorization().SubjectAccessReviews(), u, &authorization.ResourceAttributes{ + Namespace: namespace, + Verb: "list", + Group: kapi.GroupName, + Resource: "services", + }); err != nil { + return nil, api.Forbidden(err) + } + + serviceList, err := b.kc.Core().Services(namespace).List(metav1.ListOptions{LabelSelector: labels.NewSelector().Add(*requirement).String()}) if err != nil { if kerrors.IsForbidden(err) { return nil, api.Forbidden(err) @@ -59,10 +70,19 @@ func (b *Broker) getServices(impersonatedKC internalversion.ServicesGetter, name return services, nil } -func (b *Broker) getSecrets(impersonatedKC internalversion.SecretsGetter, namespace, instanceID string) (map[string]string, *api.Response) { +func (b *Broker) getSecrets(u user.Info, namespace, instanceID string) (map[string]string, *api.Response) { requirement, _ := labels.NewRequirement(templateapi.TemplateInstanceLabel, selection.Equals, []string{instanceID}) - secretList, err := impersonatedKC.Secrets(namespace).List(metav1.ListOptions{LabelSelector: labels.NewSelector().Add(*requirement).String()}) + if err := util.Authorize(b.kc.Authorization().SubjectAccessReviews(), u, &authorization.ResourceAttributes{ + Namespace: namespace, + Verb: "list", + Group: kapi.GroupName, + Resource: "secrets", + }); err != nil { + return nil, api.Forbidden(err) + } + + secretList, err := b.kc.Core().Secrets(namespace).List(metav1.ListOptions{LabelSelector: labels.NewSelector().Add(*requirement).String()}) if err != nil { if kerrors.IsForbidden(err) { return nil, api.Forbidden(err) @@ -88,6 +108,10 @@ func (b *Broker) getSecrets(impersonatedKC internalversion.SecretsGetter, namesp } func (b *Broker) Bind(instanceID, bindingID string, breq *api.BindRequest) *api.Response { + if err := b.waitForReady(); err != nil { + return api.InternalServerError(err) + } + if errs := ValidateBindRequest(breq); len(errs) > 0 { return api.BadRequest(errs.ToAggregate()) } @@ -97,11 +121,7 @@ func (b *Broker) Bind(instanceID, bindingID string, breq *api.BindRequest) *api. } impersonate := breq.Parameters[templateapi.RequesterUsernameParameterKey] - - impersonatedKC, _, _, err := b.getClientsForUsername(impersonate) - if err != nil { - return api.InternalServerError(err) - } + u := &user.DefaultInfo{Name: impersonate} brokerTemplateInstance, err := b.templateclient.BrokerTemplateInstances().Get(instanceID, metav1.GetOptions{}) if err != nil { @@ -122,12 +142,12 @@ func (b *Broker) Bind(instanceID, bindingID string, breq *api.BindRequest) *api. namespace := brokerTemplateInstance.Spec.TemplateInstance.Namespace - services, resp := b.getServices(impersonatedKC.Core(), namespace, instanceID) + services, resp := b.getServices(u, namespace, instanceID) if resp != nil { return resp } - secrets, resp := b.getSecrets(impersonatedKC.Core(), namespace, instanceID) + secrets, resp := b.getSecrets(u, namespace, instanceID) if resp != nil { return resp } diff --git a/pkg/template/servicebroker/catalog.go b/pkg/template/servicebroker/catalog.go index 16a1b906460e..0a4f238742aa 100644 --- a/pkg/template/servicebroker/catalog.go +++ b/pkg/template/servicebroker/catalog.go @@ -4,6 +4,8 @@ import ( "net/http" "strings" + "k8s.io/apimachinery/pkg/labels" + jsschema "github.com/lestrrat/go-jsschema" oapi "github.com/openshift/origin/pkg/api" @@ -108,10 +110,14 @@ func serviceFromTemplate(template *templateapi.Template) *api.Service { } func (b *Broker) Catalog() *api.Response { + if err := b.waitForReady(); err != nil { + return api.InternalServerError(err) + } + var services []*api.Service for namespace := range b.templateNamespaces { - templates, err := b.lister.ListByNamespace(namespace) + templates, err := b.lister.Templates(namespace).List(labels.Everything()) if err != nil { return api.InternalServerError(err) } diff --git a/pkg/template/servicebroker/deprovision.go b/pkg/template/servicebroker/deprovision.go index 25ea32517cb6..8e82ccd44261 100644 --- a/pkg/template/servicebroker/deprovision.go +++ b/pkg/template/servicebroker/deprovision.go @@ -10,6 +10,10 @@ import ( ) func (b *Broker) Deprovision(instanceID string) *api.Response { + if err := b.waitForReady(); err != nil { + return api.InternalServerError(err) + } + brokerTemplateInstance, err := b.templateclient.BrokerTemplateInstances().Get(instanceID, metav1.GetOptions{}) if err != nil { if kerrors.IsNotFound(err) { @@ -23,7 +27,7 @@ func (b *Broker) Deprovision(instanceID string) *api.Response { return api.InternalServerError(err) } - err = b.secretsGetter.Secrets(brokerTemplateInstance.Spec.Secret.Namespace).Delete(brokerTemplateInstance.Spec.Secret.Name, metav1.NewPreconditionDeleteOptions(string(brokerTemplateInstance.Spec.Secret.UID))) + err = b.kc.Core().Secrets(brokerTemplateInstance.Spec.Secret.Namespace).Delete(brokerTemplateInstance.Spec.Secret.Name, metav1.NewPreconditionDeleteOptions(string(brokerTemplateInstance.Spec.Secret.UID))) if err != nil && !kerrors.IsNotFound(err) { return api.InternalServerError(err) } diff --git a/pkg/template/servicebroker/lastoperation.go b/pkg/template/servicebroker/lastoperation.go index c94c5aa7706f..6de1d3e2c79e 100644 --- a/pkg/template/servicebroker/lastoperation.go +++ b/pkg/template/servicebroker/lastoperation.go @@ -13,6 +13,10 @@ import ( ) func (b *Broker) LastOperation(instanceID string, operation api.Operation) *api.Response { + if err := b.waitForReady(); err != nil { + return api.InternalServerError(err) + } + if operation != api.OperationProvisioning { return api.BadRequest(errors.New("invalid operation")) } diff --git a/pkg/template/servicebroker/provision.go b/pkg/template/servicebroker/provision.go index babb0068a39c..594d077730ab 100644 --- a/pkg/template/servicebroker/provision.go +++ b/pkg/template/servicebroker/provision.go @@ -1,23 +1,22 @@ package servicebroker import ( - "errors" "net/http" "reflect" - internalversiontemplate "github.com/openshift/origin/pkg/template/generated/internalclientset/typed/template/internalversion" kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apiserver/pkg/authentication/user" kapi "k8s.io/kubernetes/pkg/api" - "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/core/internalversion" + "k8s.io/kubernetes/pkg/apis/authorization" - authorizationapi "github.com/openshift/origin/pkg/authorization/api" + "github.com/openshift/origin/pkg/authorization/util" "github.com/openshift/origin/pkg/openservicebroker/api" templateapi "github.com/openshift/origin/pkg/template/api" ) -func (b *Broker) ensureSecret(impersonatedKC internalversion.SecretsGetter, namespace string, instanceID string, preq *api.ProvisionRequest, didWork *bool) (*kapi.Secret, *api.Response) { +func (b *Broker) ensureSecret(u user.Info, namespace string, instanceID string, preq *api.ProvisionRequest, didWork *bool) (*kapi.Secret, *api.Response) { secret := &kapi.Secret{ ObjectMeta: metav1.ObjectMeta{Name: instanceID}, Data: map[string][]byte{}, @@ -29,14 +28,32 @@ func (b *Broker) ensureSecret(impersonatedKC internalversion.SecretsGetter, name } } - createdSec, err := impersonatedKC.Secrets(namespace).Create(secret) + if err := util.Authorize(b.kc.Authorization().SubjectAccessReviews(), u, &authorization.ResourceAttributes{ + Namespace: namespace, + Verb: "create", + Group: kapi.GroupName, + Resource: "secrets", + }); err != nil { + return nil, api.Forbidden(err) + } + + createdSec, err := b.kc.Core().Secrets(namespace).Create(secret) if err == nil { *didWork = true return createdSec, nil } if kerrors.IsAlreadyExists(err) { - existingSec, err := impersonatedKC.Secrets(namespace).Get(secret.Name, metav1.GetOptions{}) + if err := util.Authorize(b.kc.Authorization().SubjectAccessReviews(), u, &authorization.ResourceAttributes{ + Namespace: namespace, + Verb: "get", + Group: kapi.GroupName, + Resource: "secrets", + }); err != nil { + return nil, api.Forbidden(err) + } + + existingSec, err := b.kc.Core().Secrets(namespace).Get(secret.Name, metav1.GetOptions{}) if err == nil && reflect.DeepEqual(secret.Data, existingSec.Data) { return existingSec, nil } @@ -50,26 +67,44 @@ func (b *Broker) ensureSecret(impersonatedKC internalversion.SecretsGetter, name return nil, api.InternalServerError(err) } -func (b *Broker) ensureTemplateInstance(impersonatedTemplateclient internalversiontemplate.TemplateInterface, namespace string, instanceID string, template *templateapi.Template, secret *kapi.Secret, impersonate string, didWork *bool) (*templateapi.TemplateInstance, *api.Response) { +func (b *Broker) ensureTemplateInstance(u user.Info, namespace string, instanceID string, template *templateapi.Template, secret *kapi.Secret, didWork *bool) (*templateapi.TemplateInstance, *api.Response) { templateInstance := &templateapi.TemplateInstance{ ObjectMeta: metav1.ObjectMeta{Name: instanceID}, Spec: templateapi.TemplateInstanceSpec{ Template: *template, Secret: kapi.LocalObjectReference{Name: secret.Name}, Requester: &templateapi.TemplateInstanceRequester{ - Username: impersonate, + Username: u.GetName(), }, }, } - createdTemplateInstance, err := impersonatedTemplateclient.TemplateInstances(namespace).Create(templateInstance) + if err := util.Authorize(b.kc.Authorization().SubjectAccessReviews(), u, &authorization.ResourceAttributes{ + Namespace: namespace, + Verb: "create", + Group: templateapi.GroupName, + Resource: "templateinstances", + }); err != nil { + return nil, api.Forbidden(err) + } + + createdTemplateInstance, err := b.templateclient.TemplateInstances(namespace).Create(templateInstance) if err == nil { *didWork = true return createdTemplateInstance, nil } if kerrors.IsAlreadyExists(err) { - existingTemplateInstance, err := impersonatedTemplateclient.TemplateInstances(namespace).Get(templateInstance.Name, metav1.GetOptions{}) + if err := util.Authorize(b.kc.Authorization().SubjectAccessReviews(), u, &authorization.ResourceAttributes{ + Namespace: namespace, + Verb: "get", + Group: templateapi.GroupName, + Resource: "templateinstances", + }); err != nil { + return nil, api.Forbidden(err) + } + + existingTemplateInstance, err := b.templateclient.TemplateInstances(namespace).Get(templateInstance.Name, metav1.GetOptions{}) if err == nil && reflect.DeepEqual(templateInstance.Spec, existingTemplateInstance.Spec) { return existingTemplateInstance, nil } @@ -132,22 +167,34 @@ func (b *Broker) ensureBrokerTemplateInstance(namespace, instanceID string, didW } func (b *Broker) Provision(instanceID string, preq *api.ProvisionRequest) *api.Response { + if err := b.waitForReady(); err != nil { + return api.InternalServerError(err) + } + if errs := ValidateProvisionRequest(preq); len(errs) > 0 { return api.BadRequest(errs.ToAggregate()) } namespace := preq.Parameters[templateapi.NamespaceParameterKey] impersonate := preq.Parameters[templateapi.RequesterUsernameParameterKey] - - impersonatedKC, _, impersonatedTemplateclient, err := b.getClientsForUsername(impersonate) - if err != nil { - return api.InternalServerError(err) - } + u := &user.DefaultInfo{Name: impersonate} template, err := b.lister.GetByUID(preq.ServiceID) if err != nil { return api.BadRequest(err) } + if template == nil { + templates, err := b.lister.List(labels.Everything()) + if err != nil { + return api.InternalServerError(err) + } + for _, t := range templates { + if string(t.UID) == preq.ServiceID { + template = t + break + } + } + } if template == nil { return api.BadRequest(kerrors.NewNotFound(templateapi.Resource("templates"), preq.ServiceID)) } @@ -155,20 +202,26 @@ func (b *Broker) Provision(instanceID string, preq *api.ProvisionRequest) *api.R return api.BadRequest(kerrors.NewNotFound(templateapi.Resource("templates"), preq.ServiceID)) } - lsar := authorizationapi.AddUserToLSAR(&user.DefaultInfo{Name: impersonate}, - &authorizationapi.LocalSubjectAccessReview{ - Action: authorizationapi.Action{ - Verb: "create", - Group: templateapi.GroupName, - Resource: "templateinstances", - }, - }) - lsarResp, err := b.localSAR.LocalSubjectAccessReviews(namespace).Create(lsar) - if err != nil || lsarResp == nil || !lsarResp.Allowed { - if err == nil { - err = errors.New("forbidden") + // TODO: enable SAR for template - at the moment I think this doesn't work + // properly because group information isn't populated in u. + /* + if err := util.Authorize(b.kc.Authorization().SubjectAccessReviews(), u, &authorization.ResourceAttributes{ + Namespace: template.Namespace, + Verb: "get", + Group: templateapi.GroupName, + Resource: "templates", + }); err != nil { + return api.Forbidden(err) } - return api.Forbidden(kerrors.NewForbidden(templateapi.LegacyResource("templateinstances"), instanceID, err)) + */ + + if err := util.Authorize(b.kc.Authorization().SubjectAccessReviews(), u, &authorization.ResourceAttributes{ + Namespace: namespace, + Verb: "create", + Group: templateapi.GroupName, + Resource: "templateinstances", + }); err != nil { + return api.Forbidden(err) } didWork := false @@ -178,12 +231,12 @@ func (b *Broker) Provision(instanceID string, preq *api.ProvisionRequest) *api.R return resp } - secret, resp := b.ensureSecret(impersonatedKC.Core(), namespace, instanceID, preq, &didWork) + secret, resp := b.ensureSecret(u, namespace, instanceID, preq, &didWork) if resp != nil { return resp } - templateInstance, resp := b.ensureTemplateInstance(impersonatedTemplateclient.Template(), namespace, instanceID, template, secret, impersonate, &didWork) + templateInstance, resp := b.ensureTemplateInstance(u, namespace, instanceID, template, secret, &didWork) if resp != nil { return resp } diff --git a/pkg/template/servicebroker/servicebroker.go b/pkg/template/servicebroker/servicebroker.go index 90908b1755ea..20b1d74b2ed3 100644 --- a/pkg/template/servicebroker/servicebroker.go +++ b/pkg/template/servicebroker/servicebroker.go @@ -1,62 +1,99 @@ package servicebroker import ( - "k8s.io/apiserver/pkg/authentication/user" + "errors" + "fmt" + "time" + + "github.com/golang/glog" + + utilruntime "k8s.io/apimachinery/pkg/util/runtime" restclient "k8s.io/client-go/rest" + "k8s.io/client-go/tools/cache" kclientset "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset" - "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/core/internalversion" - authclient "github.com/openshift/origin/pkg/auth/client" "github.com/openshift/origin/pkg/client" - "github.com/openshift/origin/pkg/client/cache" - "github.com/openshift/origin/pkg/controller/shared" + "github.com/openshift/origin/pkg/cmd/server/bootstrappolicy" + "github.com/openshift/origin/pkg/serviceaccounts" + templateapi "github.com/openshift/origin/pkg/template/api" + templateinformer "github.com/openshift/origin/pkg/template/generated/informers/internalversion/template/internalversion" templateclientset "github.com/openshift/origin/pkg/template/generated/internalclientset" internalversiontemplate "github.com/openshift/origin/pkg/template/generated/internalclientset/typed/template/internalversion" + templatelister "github.com/openshift/origin/pkg/template/generated/listers/template/internalversion" ) type Broker struct { - secretsGetter internalversion.SecretsGetter - localSAR client.LocalSubjectAccessReviewsNamespacer + oc *client.Client + kc kclientset.Interface templateclient internalversiontemplate.TemplateInterface restconfig restclient.Config - lister cache.StoreToTemplateLister + informer cache.SharedIndexInformer + lister templatelister.TemplateLister templateNamespaces map[string]struct{} + ready chan struct{} } -func NewBroker(restconfig restclient.Config, localSAR client.LocalSubjectAccessReviewsNamespacer, secretsGetter internalversion.SecretsGetter, informers shared.InformerFactory, namespaces []string) *Broker { +func NewBroker(privrestconfig restclient.Config, privkc kclientset.Interface, infraNamespace string, informer templateinformer.TemplateInformer, namespaces []string) *Broker { + informer.Informer().AddIndexers(cache.Indexers{ + templateapi.TemplateUIDIndex: func(obj interface{}) ([]string, error) { + return []string{string(obj.(*templateapi.Template).UID)}, nil + }}) + templateNamespaces := map[string]struct{}{} for _, namespace := range namespaces { templateNamespaces[namespace] = struct{}{} } - return &Broker{ - secretsGetter: secretsGetter, - localSAR: localSAR, - templateclient: templateclientset.NewForConfigOrDie(&restconfig).Template(), - restconfig: restconfig, - lister: informers.Templates().Lister(), + b := &Broker{ + informer: informer.Informer(), + lister: informer.Lister(), templateNamespaces: templateNamespaces, + ready: make(chan struct{}), } -} -func (b *Broker) getClientsForUsername(username string) (kclientset.Interface, client.Interface, templateclientset.Interface, error) { - u := &user.DefaultInfo{Name: username} + go func() { + // the broker is initialised asynchronously because fetching the service + // account token requires the main API server to be running - oc, err := authclient.NewImpersonatingOpenShiftClient(u, b.restconfig) - if err != nil { - return nil, nil, nil, err - } + glog.Infof("Template service broker: waiting for authentication token") - kc, err := authclient.NewImpersonatingKubernetesClientset(u, b.restconfig) - if err != nil { - return nil, nil, nil, err - } + restconfig, oc, kc, _, err := serviceaccounts.Clients( + privrestconfig, + &serviceaccounts.ClientLookupTokenRetriever{Client: privkc}, + infraNamespace, + bootstrappolicy.InfraTemplateServiceBrokerServiceAccountName, + ) + if err != nil { + utilruntime.HandleError(fmt.Errorf("Template service broker: failed to initialize: %v", err)) + return + } - impersonatingConfig := authclient.NewImpersonatingConfig(u, b.restconfig) - templateclient, err := templateclientset.NewForConfig(&impersonatingConfig) - if err != nil { - return nil, nil, nil, err - } + b.oc = oc + b.kc = kc + b.templateclient = templateclientset.NewForConfigOrDie(restconfig).Template() + + glog.Infof("Template service broker: waiting for informer sync") - return kc, oc, templateclient, nil + for !b.informer.HasSynced() { + time.Sleep(100 * time.Millisecond) + } + + glog.Infof("Template service broker: ready") + + close(b.ready) + }() + + return b +} + +func (b *Broker) waitForReady() error { + timer := time.NewTimer(10 * time.Second) + defer timer.Stop() + + select { + case <-b.ready: + return nil + case <-timer.C: + return errors.New("timeout waiting for broker to be ready") + } } diff --git a/pkg/template/servicebroker/test-scripts/clusterrole.yaml b/pkg/template/servicebroker/test-scripts/clusterrole.yaml index 9b6a61e5b987..5f085b54136c 100644 --- a/pkg/template/servicebroker/test-scripts/clusterrole.yaml +++ b/pkg/template/servicebroker/test-scripts/clusterrole.yaml @@ -4,9 +4,9 @@ items: - kind: ClusterRoleBinding apiVersion: v1 metadata: - name: templateservicebroker-client-binding - roleRef: name: templateservicebroker-client + roleRef: + name: system:openshift:templateservicebroker-client groupNames: - system:unauthenticated - system:authenticated diff --git a/pkg/template/servicebroker/unbind.go b/pkg/template/servicebroker/unbind.go index 123c06eedaf4..eb6e1337409b 100644 --- a/pkg/template/servicebroker/unbind.go +++ b/pkg/template/servicebroker/unbind.go @@ -10,6 +10,10 @@ import ( ) func (b *Broker) Unbind(instanceID, bindingID string) *api.Response { + if err := b.waitForReady(); err != nil { + return api.InternalServerError(err) + } + brokerTemplateInstance, err := b.templateclient.BrokerTemplateInstances().Get(instanceID, metav1.GetOptions{}) if err != nil { if kerrors.IsNotFound(err) { diff --git a/test/extended/templates/templateinstance_impersonation.go b/test/extended/templates/templateinstance_impersonation.go index 3cf8bef47aef..652b6b6fd04f 100644 --- a/test/extended/templates/templateinstance_impersonation.go +++ b/test/extended/templates/templateinstance_impersonation.go @@ -32,9 +32,6 @@ var _ = g.Describe("[templates] templateinstance impersonation tests", func() { edituser2 *userapi.User // project edit viewuser *userapi.User // project view - clusterrole *authorizationapi.ClusterRole - clusterrolebinding *authorizationapi.ClusterRoleBinding - dummytemplateinstance *templateapi.TemplateInstance tests []struct { @@ -119,27 +116,38 @@ var _ = g.Describe("[templates] templateinstance impersonation tests", func() { } // additional plumbing to enable impersonateuser to impersonate edituser1 - clusterrole, err = cli.AdminClient().ClusterRoles().Create(&authorizationapi.ClusterRole{ + role, err := cli.AdminClient().Roles(cli.Namespace()).Create(&authorizationapi.Role{ ObjectMeta: metav1.ObjectMeta{ - Name: cli.Namespace() + "-impersonate", + Name: "impersonater", }, Rules: []authorizationapi.PolicyRule{ { - Verbs: sets.NewString("impersonate"), - APIGroups: []string{userapi.GroupName}, - Resources: sets.NewString(authorizationapi.UserResource), - ResourceNames: sets.NewString(edituser1.Name), + Verbs: sets.NewString("assign"), + APIGroups: []string{templateapi.GroupName}, + Resources: sets.NewString("templateinstances"), }, }, }) o.Expect(err).NotTo(o.HaveOccurred()) - clusterrolebinding, err = cli.AdminClient().ClusterRoleBindings().Create(&authorizationapi.ClusterRoleBinding{ + _, err = cli.AdminClient().PolicyBindings(cli.Namespace()).Create(&authorizationapi.PolicyBinding{ ObjectMeta: metav1.ObjectMeta{ - Name: cli.Namespace() + "-impersonate", + Name: authorizationapi.GetPolicyBindingName(cli.Namespace()), + }, + PolicyRef: kapi.ObjectReference{ + Name: "default", + Namespace: cli.Namespace(), + }, + }) + o.Expect(err).NotTo(o.HaveOccurred()) + + _, err = cli.AdminClient().RoleBindings(cli.Namespace()).Create(&authorizationapi.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "impersonater-binding", }, RoleRef: kapi.ObjectReference{ - Name: clusterrole.Name, + Name: role.Name, + Namespace: cli.Namespace(), }, Subjects: []kapi.ObjectReference{ { @@ -157,11 +165,6 @@ var _ = g.Describe("[templates] templateinstance impersonation tests", func() { deleteUser(cli, edituser1) deleteUser(cli, edituser2) deleteUser(cli, viewuser) - - err := cli.AdminClient().ClusterRoles().Delete(clusterrole.Name) - o.Expect(err).NotTo(o.HaveOccurred()) - err = cli.AdminClient().ClusterRoleBindings().Delete(clusterrolebinding.Name) - o.Expect(err).NotTo(o.HaveOccurred()) }) g.It("should pass impersonation creation tests", func() { @@ -197,7 +200,16 @@ var _ = g.Describe("[templates] templateinstance impersonation tests", func() { templateinstance, err := cli.AdminTemplateClient().Template().TemplateInstances(cli.Namespace()).Create(templateinstancecopy.(*templateapi.TemplateInstance)) o.Expect(err).NotTo(o.HaveOccurred()) - newtemplateinstance, err := cli.TemplateClient().Template().TemplateInstances(cli.Namespace()).Update(templateinstance) + var newtemplateinstance *templateapi.TemplateInstance + for try := 0; try < 3; try++ { + newtemplateinstance, err = cli.TemplateClient().Template().TemplateInstances(cli.Namespace()).Update(templateinstance) + if kerrors.IsConflict(err) { + templateinstance, err = cli.TemplateClient().Template().TemplateInstances(cli.Namespace()).Get(templateinstance.Name, metav1.GetOptions{}) + o.Expect(err).NotTo(o.HaveOccurred()) + } else { + break + } + } if !test.expectCreateUpdateSuccess { o.Expect(err).To(o.HaveOccurred()) o.Expect(kerrors.IsInvalid(err) || kerrors.IsForbidden(err)).To(o.BeTrue()) diff --git a/test/extended/templates/templateinstance_security.go b/test/extended/templates/templateinstance_security.go index 6a31b8527e51..4f8a7adf80b4 100644 --- a/test/extended/templates/templateinstance_security.go +++ b/test/extended/templates/templateinstance_security.go @@ -109,14 +109,14 @@ var _ = g.Describe("[templates] templateinstance security tests", func() { }, }, { - by: "checking adminuser can create a privileged object", + by: "checking adminuser can't create a privileged object", user: adminuser, namespace: cli.Namespace(), objects: []runtime.Object{dummyrolebinding}, - expectCondition: templateapi.TemplateInstanceReady, + expectCondition: templateapi.TemplateInstanceInstantiateFailure, checkOK: func(namespace string) bool { _, err := cli.AdminClient().RoleBindings(namespace).Get(dummyrolebinding.Name, metav1.GetOptions{}) - return err == nil + return err != nil && kerrors.IsNotFound(err) }, }, } diff --git a/test/extended/templates/templateservicebroker_e2e.go b/test/extended/templates/templateservicebroker_e2e.go index e730831cdb68..0b5ab844cf86 100644 --- a/test/extended/templates/templateservicebroker_e2e.go +++ b/test/extended/templates/templateservicebroker_e2e.go @@ -43,7 +43,7 @@ var _ = g.Describe("[templates] templateservicebroker end-to-end test", func() { g.BeforeEach(func() { var err error - template, err = cli.Client().Templates("openshift").Get("cakephp-mysql-persistent", metav1.GetOptions{}) + template, err = cli.TemplateClient().Template().Templates("openshift").Get("cakephp-mysql-persistent", metav1.GetOptions{}) o.Expect(err).NotTo(o.HaveOccurred()) privatetemplate, err = cli.Client().Templates(cli.Namespace()).Create(&templateapi.Template{ @@ -55,7 +55,7 @@ var _ = g.Describe("[templates] templateservicebroker end-to-end test", func() { clusterrolebinding, err = cli.AdminClient().ClusterRoleBindings().Create(&authorizationapi.ClusterRoleBinding{ ObjectMeta: metav1.ObjectMeta{ - Name: cli.Namespace() + "templateservicebroker-client-binding", + Name: cli.Namespace() + "templateservicebroker-client", }, RoleRef: kapi.ObjectReference{ Name: bootstrappolicy.TemplateServiceBrokerClientRoleName, diff --git a/test/extended/templates/templateservicebroker_security.go b/test/extended/templates/templateservicebroker_security.go index 72854164b8fe..f20f7bfaef34 100644 --- a/test/extended/templates/templateservicebroker_security.go +++ b/test/extended/templates/templateservicebroker_security.go @@ -48,7 +48,7 @@ var _ = g.Describe("[templates] templateservicebroker security test", func() { clusterrolebinding, err = cli.AdminClient().ClusterRoleBindings().Create(&authorizationapi.ClusterRoleBinding{ ObjectMeta: metav1.ObjectMeta{ - Name: cli.Namespace() + "templateservicebroker-client-binding", + Name: cli.Namespace() + "templateservicebroker-client", }, RoleRef: kapi.ObjectReference{ Name: bootstrappolicy.TemplateServiceBrokerClientRoleName, diff --git a/test/integration/authorization_test.go b/test/integration/authorization_test.go index 01d0f3d02a4c..1125669fad08 100644 --- a/test/integration/authorization_test.go +++ b/test/integration/authorization_test.go @@ -386,6 +386,7 @@ var globalDeploymentConfigGetterUsers = sets.NewString( "system:serviceaccount:openshift-infra:unidling-controller", "system:serviceaccount:openshift-infra:deployment-trigger-controller", "system:serviceaccount:openshift-infra:deploymentconfig-controller", + "system:serviceaccount:openshift-infra:template-instance-controller", ) type resourceAccessReviewTest struct { @@ -1601,7 +1602,7 @@ func TestOldLocalResourceAccessReviewEndpoint(t *testing.T) { expectedResponse := &authorizationapi.ResourceAccessReviewResponse{ Namespace: namespace, - Users: sets.NewString("harold", "system:serviceaccount:kube-system:generic-garbage-collector", "system:serviceaccount:kube-system:namespace-controller", "system:serviceaccount:hammer-project:builder", "system:admin"), + Users: sets.NewString("harold", "system:serviceaccount:kube-system:generic-garbage-collector", "system:serviceaccount:kube-system:namespace-controller", "system:serviceaccount:openshift-infra:template-instance-controller", "system:serviceaccount:hammer-project:builder", "system:admin"), Groups: sets.NewString("system:cluster-admins", "system:masters", "system:cluster-readers", "system:serviceaccounts:hammer-project"), } if (actualResponse.Namespace != expectedResponse.Namespace) || @@ -1628,7 +1629,7 @@ func TestOldLocalResourceAccessReviewEndpoint(t *testing.T) { expectedResponse := &authorizationapi.ResourceAccessReviewResponse{ Namespace: namespace, - Users: sets.NewString("harold", "system:serviceaccount:kube-system:generic-garbage-collector", "system:serviceaccount:kube-system:namespace-controller", "system:serviceaccount:hammer-project:builder", "system:admin"), + Users: sets.NewString("harold", "system:serviceaccount:kube-system:generic-garbage-collector", "system:serviceaccount:kube-system:namespace-controller", "system:serviceaccount:openshift-infra:template-instance-controller", "system:serviceaccount:hammer-project:builder", "system:admin"), Groups: sets.NewString("system:cluster-admins", "system:masters", "system:cluster-readers", "system:serviceaccounts:hammer-project"), } if (actualResponse.Namespace != expectedResponse.Namespace) || diff --git a/test/testdata/bootstrappolicy/bootstrap_cluster_role_bindings.yaml b/test/testdata/bootstrappolicy/bootstrap_cluster_role_bindings.yaml index bbc6590459c7..a28b77b99b83 100644 --- a/test/testdata/bootstrappolicy/bootstrap_cluster_role_bindings.yaml +++ b/test/testdata/bootstrappolicy/bootstrap_cluster_role_bindings.yaml @@ -242,6 +242,20 @@ items: - kind: SystemGroup name: system:authenticated userNames: null +- apiVersion: v1 + groupNames: null + kind: ClusterRoleBinding + metadata: + creationTimestamp: null + name: system:openshift:controller:template-instance-controller + roleRef: + name: edit + subjects: + - kind: ServiceAccount + name: template-instance-controller + namespace: openshift-infra + userNames: + - system:serviceaccount:openshift-infra:template-instance-controller - apiVersion: v1 groupNames: null kind: ClusterRoleBinding diff --git a/test/testdata/bootstrappolicy/bootstrap_cluster_roles.yaml b/test/testdata/bootstrappolicy/bootstrap_cluster_roles.yaml index b83820ea8a9f..0b5040d9af00 100644 --- a/test/testdata/bootstrappolicy/bootstrap_cluster_roles.yaml +++ b/test/testdata/bootstrappolicy/bootstrap_cluster_roles.yaml @@ -2785,7 +2785,7 @@ items: annotations: authorization.openshift.io/system-only: "true" creationTimestamp: null - name: templateservicebroker-client + name: system:openshift:templateservicebroker-client rules: - apiGroups: null attributeRestrictions: null @@ -3404,6 +3404,89 @@ items: - list - update - watch +- apiVersion: v1 + kind: ClusterRole + metadata: + annotations: + authorization.openshift.io/system-only: "true" + creationTimestamp: null + name: system:openshift:controller:template-instance-controller + rules: + - apiGroups: + - authorization.k8s.io + attributeRestrictions: null + resources: + - subjectaccessreviews + verbs: + - create + - apiGroups: + - template.openshift.io + attributeRestrictions: null + resources: + - templateinstances + verbs: + - get + - list + - watch + - apiGroups: + - template.openshift.io + attributeRestrictions: null + resources: + - templateinstances/status + verbs: + - update +- apiVersion: v1 + kind: ClusterRole + metadata: + annotations: + authorization.openshift.io/system-only: "true" + creationTimestamp: null + name: system:openshift:template-service-broker + rules: + - apiGroups: + - authorization.k8s.io + attributeRestrictions: null + resources: + - subjectaccessreviews + verbs: + - create + - apiGroups: + - template.openshift.io + attributeRestrictions: null + resources: + - brokertemplateinstances + verbs: + - create + - delete + - get + - update + - apiGroups: + - template.openshift.io + attributeRestrictions: null + resources: + - templateinstances + verbs: + - assign + - create + - delete + - get + - apiGroups: + - "" + attributeRestrictions: null + resources: + - secrets + verbs: + - create + - delete + - get + - list + - apiGroups: + - "" + attributeRestrictions: null + resources: + - services + verbs: + - list - apiVersion: v1 kind: ClusterRole metadata: