From 4f74fcf6b110a7ae80745c02cebeab85eece75b6 Mon Sep 17 00:00:00 2001 From: Adam Harwayne Date: Wed, 8 May 2019 17:32:22 -0700 Subject: [PATCH 1/4] Namespace reconciler creates a service account and adds RoleBinding for Broker Ingress. --- config/200-broker-clusterrole.yaml | 8 +++ config/500-controller.yaml | 2 +- docs/broker/README.md | 8 ++- pkg/reconciler/namespace/namespace.go | 51 ++++++++++++++----- pkg/reconciler/namespace/namespace_test.go | 17 ++++--- pkg/reconciler/namespace/resources/names.go | 13 +++-- .../namespace/resources/role_binding.go | 6 +-- .../namespace/resources/service_account.go | 4 +- 8 files changed, 75 insertions(+), 34 deletions(-) diff --git a/config/200-broker-clusterrole.yaml b/config/200-broker-clusterrole.yaml index b3de29dc20c..83bf1265ff0 100644 --- a/config/200-broker-clusterrole.yaml +++ b/config/200-broker-clusterrole.yaml @@ -26,3 +26,11 @@ rules: - "get" - "list" - "watch" + +--- + +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: eventing-broker-ingress +rules: [] diff --git a/config/500-controller.yaml b/config/500-controller.yaml index b7a4dffd1bf..d50f03fa69f 100644 --- a/config/500-controller.yaml +++ b/config/500-controller.yaml @@ -45,7 +45,7 @@ spec: - name: BROKER_INGRESS_IMAGE value: github.com/knative/eventing/cmd/broker/ingress - name: BROKER_INGRESS_SERVICE_ACCOUNT - value: default + value: eventing-broker-ingress - name: BROKER_FILTER_IMAGE value: github.com/knative/eventing/cmd/broker/filter - name: BROKER_FILTER_SERVICE_ACCOUNT diff --git a/docs/broker/README.md b/docs/broker/README.md index f9b0125c8ab..4a5805592d7 100644 --- a/docs/broker/README.md +++ b/docs/broker/README.md @@ -112,26 +112,30 @@ kubectl -n default get broker default #### Manual Setup In order to setup a `Broker` manually, we must first create the required -`ServiceAccount` and give it the proper RBAC permissions. This setup is required +`ServiceAccount`s and give them the proper RBAC permissions. This setup is required once per namespace. These instructions will use the `default` namespace, but you can replace it with any namespace you want to install a `Broker` into. Create the `ServiceAccount`. ```shell +kubectl -n default create serviceaccount eventing-broker-ingress kubectl -n default create serviceaccount eventing-broker-filter ``` Then give it the needed RBAC permissions: ```shell +kubectl -n default create rolebinding eventing-broker-ingress \ + --clusterrole=eventing-broker-ingress \ + --user=eventing-broker-ingress kubectl -n default create rolebinding eventing-broker-filter \ --clusterrole=eventing-broker-filter \ --user=eventing-broker-filter ``` Note that the previous commands uses three different objects, all named -`eventing-broker-filter`. The `ClusterRole` is installed with Knative Eventing +`eventing-broker-ingress` or `eventing-broker-filter`. The `ClusterRole` is installed with Knative Eventing [here](../../config/200-broker-clusterrole.yaml). The `ServiceAccount` was created two commands prior. The `RoleBinding` is created with this command. diff --git a/pkg/reconciler/namespace/namespace.go b/pkg/reconciler/namespace/namespace.go index 6d4ce457693..128c6538d82 100644 --- a/pkg/reconciler/namespace/namespace.go +++ b/pkg/reconciler/namespace/namespace.go @@ -162,27 +162,52 @@ func (r *Reconciler) reconcile(ctx context.Context, ns *corev1.Namespace) error if ns.DeletionTimestamp != nil { return nil } + isa, err := r.reconcileBrokerServiceAccount(ctx, ns, resources.MakeServiceAccount(ns.Name, resources.IngressServiceAccountName)) + if err != nil { + logging.FromContext(ctx).Error("Unable to reconcile the Broker Ingress Service Account for the namespace", zap.Error(err)) + return err + } + + // Tell tracker to reconcile this namespace whenever the Service Account changes. + if err = r.tracker.Track(utils.ObjectRef(isa, serviceAccountGVK), ns); err != nil { + logging.FromContext(ctx).Error("Unable to track changes to ServiceAccount", zap.Error(err)) + return err + } + + irb, err := r.reconcileBrokerRBAC(ctx, ns, isa, + resources.MakeRoleBinding(resources.IngressRoleBindingName, isa, resources.IngressClusterRoleName)) + if err != nil { + logging.FromContext(ctx).Error("Unable to reconcile the Broker Ingress Service Account RBAC for the namespace", zap.Error(err)) + return err + } + + // Tell tracker to reconcile this namespace whenever the RoleBinding changes. + if err = r.tracker.Track(utils.ObjectRef(irb, roleBindingGVK), ns); err != nil { + logging.FromContext(ctx).Error("Unable to track changes to RoleBinding", zap.Error(err)) + return err + } - sa, err := r.reconcileBrokerFilterServiceAccount(ctx, ns) + fsa, err := r.reconcileBrokerServiceAccount(ctx, ns, resources.MakeServiceAccount(ns.Name, resources.FilterServiceAccountName)) if err != nil { logging.FromContext(ctx).Error("Unable to reconcile the Broker Filter Service Account for the namespace", zap.Error(err)) return err } // Tell tracker to reconcile this namespace whenever the Service Account changes. - if err = r.tracker.Track(utils.ObjectRef(sa, serviceAccountGVK), ns); err != nil { + if err = r.tracker.Track(utils.ObjectRef(fsa, serviceAccountGVK), ns); err != nil { logging.FromContext(ctx).Error("Unable to track changes to ServiceAccount", zap.Error(err)) return err } - rb, err := r.reconcileBrokerFilterRBAC(ctx, ns, sa) + frb, err := r.reconcileBrokerRBAC(ctx, ns, fsa, + resources.MakeRoleBinding(resources.FilterRoleBindingName, fsa, resources.FilterClusterRoleName)) if err != nil { logging.FromContext(ctx).Error("Unable to reconcile the Broker Filter Service Account RBAC for the namespace", zap.Error(err)) return err } // Tell tracker to reconcile this namespace whenever the RoleBinding changes. - if err = r.tracker.Track(utils.ObjectRef(rb, roleBindingGVK), ns); err != nil { + if err = r.tracker.Track(utils.ObjectRef(frb, roleBindingGVK), ns); err != nil { logging.FromContext(ctx).Error("Unable to track changes to RoleBinding", zap.Error(err)) return err } @@ -202,14 +227,13 @@ func (r *Reconciler) reconcile(ctx context.Context, ns *corev1.Namespace) error return nil } -// reconcileBrokerFilterServiceAccount reconciles the Broker's filter service account for Namespace 'ns'. -func (r *Reconciler) reconcileBrokerFilterServiceAccount(ctx context.Context, ns *corev1.Namespace) (*corev1.ServiceAccount, error) { - current, err := r.KubeClientSet.CoreV1().ServiceAccounts(ns.Name).Get(resources.ServiceAccountName, metav1.GetOptions{}) +// reconcileBrokerServiceAccount reconciles the Broker's service account for Namespace 'ns'. +func (r *Reconciler) reconcileBrokerServiceAccount(ctx context.Context, ns *corev1.Namespace, sa *corev1.ServiceAccount) (*corev1.ServiceAccount, error) { + current, err := r.KubeClientSet.CoreV1().ServiceAccounts(ns.Name).Get(sa.Name, metav1.GetOptions{}) // If the resource doesn't exist, we'll create it. if k8serrors.IsNotFound(err) { - sa := resources.MakeServiceAccount(ns.Name) - sa, err := r.KubeClientSet.CoreV1().ServiceAccounts(ns.Name).Create(sa) + sa, err = r.KubeClientSet.CoreV1().ServiceAccounts(ns.Name).Create(sa) if err != nil { return nil, err } @@ -223,14 +247,13 @@ func (r *Reconciler) reconcileBrokerFilterServiceAccount(ctx context.Context, ns return current, nil } -// reconcileBrokerFilterRBAC reconciles the Broker's filter service account RBAC for the Namespace 'ns'. -func (r *Reconciler) reconcileBrokerFilterRBAC(ctx context.Context, ns *corev1.Namespace, sa *corev1.ServiceAccount) (*rbacv1.RoleBinding, error) { - current, err := r.KubeClientSet.RbacV1().RoleBindings(ns.Name).Get(resources.RoleBindingName, metav1.GetOptions{}) +// reconcileBrokerRBAC reconciles the Broker's service account RBAC for the Namespace 'ns'. +func (r *Reconciler) reconcileBrokerRBAC(ctx context.Context, ns *corev1.Namespace, sa *corev1.ServiceAccount, rb *rbacv1.RoleBinding) (*rbacv1.RoleBinding, error) { + current, err := r.KubeClientSet.RbacV1().RoleBindings(ns.Name).Get(resources.FilterRoleBindingName, metav1.GetOptions{}) // If the resource doesn't exist, we'll create it. if k8serrors.IsNotFound(err) { - rb := resources.MakeRoleBinding(sa) - rb, err := r.KubeClientSet.RbacV1().RoleBindings(ns.Name).Create(rb) + rb, err = r.KubeClientSet.RbacV1().RoleBindings(ns.Name).Create(rb) if err != nil { return nil, err } diff --git a/pkg/reconciler/namespace/namespace_test.go b/pkg/reconciler/namespace/namespace_test.go index 9c4e09ca8ab..ab8f549d4ad 100644 --- a/pkg/reconciler/namespace/namespace_test.go +++ b/pkg/reconciler/namespace/namespace_test.go @@ -144,8 +144,8 @@ func TestAllCases(t *testing.T) { }, WantCreates: []metav1.Object{ resources.MakeBroker(testNS), - resources.MakeServiceAccount(testNS), - resources.MakeRoleBinding(resources.MakeServiceAccount(testNS)), + resources.MakeServiceAccount(testNS, resources.FilterServiceAccountName), + resources.MakeRoleBinding(resources.FilterRoleBindingName, resources.MakeServiceAccount(testNS, resources.FilterServiceAccountName), resources.FilterClusterRoleName), }, }, { Name: "Namespace enabled, broker exists", @@ -164,8 +164,8 @@ func TestAllCases(t *testing.T) { Eventf(corev1.EventTypeNormal, "NamespaceReconciled", "Namespace reconciled: \"test-namespace\""), }, WantCreates: []metav1.Object{ - resources.MakeServiceAccount(testNS), - resources.MakeRoleBinding(resources.MakeServiceAccount(testNS)), + resources.MakeServiceAccount(testNS, resources.FilterServiceAccountName), + resources.MakeRoleBinding(resources.FilterRoleBindingName, resources.MakeServiceAccount(testNS, resources.FilterServiceAccountName), resources.FilterClusterRoleName), }, }, { Name: "Namespace enabled, broker exists with no label", @@ -189,7 +189,7 @@ func TestAllCases(t *testing.T) { NewNamespace(testNS, WithNamespaceLabeled(resources.InjectionEnabledLabels()), ), - resources.MakeServiceAccount(testNS), + resources.MakeServiceAccount(testNS, resources.FilterServiceAccountName), }, Key: testNS, SkipNamespaceValidation: true, @@ -201,7 +201,8 @@ func TestAllCases(t *testing.T) { }, WantCreates: []metav1.Object{ resources.MakeBroker(testNS), - resources.MakeRoleBinding(resources.MakeServiceAccount(testNS)), + resources.MakeServiceAccount(testNS, resources.FilterServiceAccountName), + resources.MakeRoleBinding(resources.FilterRoleBindingName, resources.MakeServiceAccount(testNS, resources.FilterServiceAccountName), resources.FilterClusterRoleName), }, }, { Name: "Namespace enabled, role binding exists", @@ -209,7 +210,7 @@ func TestAllCases(t *testing.T) { NewNamespace(testNS, WithNamespaceLabeled(resources.InjectionEnabledLabels()), ), - resources.MakeRoleBinding(resources.MakeServiceAccount(testNS)), + resources.MakeRoleBinding(resources.FilterRoleBindingName, resources.MakeServiceAccount(testNS, resources.FilterServiceAccountName), resources.FilterClusterRoleName), }, Key: testNS, SkipNamespaceValidation: true, @@ -221,7 +222,7 @@ func TestAllCases(t *testing.T) { }, WantCreates: []metav1.Object{ resources.MakeBroker(testNS), - resources.MakeServiceAccount(testNS), + resources.MakeServiceAccount(testNS, resources.FilterServiceAccountName), }, }, // TODO: we need a existing default un-owned test. diff --git a/pkg/reconciler/namespace/resources/names.go b/pkg/reconciler/namespace/resources/names.go index d33da008ed0..03987dc4b64 100644 --- a/pkg/reconciler/namespace/resources/names.go +++ b/pkg/reconciler/namespace/resources/names.go @@ -17,8 +17,13 @@ limitations under the License. package resources const ( - DefaultBrokerName = "default" - ServiceAccountName = "eventing-broker-filter" - RoleBindingName = "eventing-broker-filter" - ClusterRoleName = "eventing-broker-filter" + DefaultBrokerName = "default" + + FilterServiceAccountName = "eventing-broker-filter" + FilterRoleBindingName = "eventing-broker-filter" + FilterClusterRoleName = "eventing-broker-filter" + + IngressServiceAccountName = "eventing-broker-ingress" + IngressRoleBindingName = "eventing-broker-ingress" + IngressClusterRoleName = "eventing-broker-ingress" ) diff --git a/pkg/reconciler/namespace/resources/role_binding.go b/pkg/reconciler/namespace/resources/role_binding.go index 4ff07e27617..94770c26560 100644 --- a/pkg/reconciler/namespace/resources/role_binding.go +++ b/pkg/reconciler/namespace/resources/role_binding.go @@ -24,17 +24,17 @@ import ( // MakeRoleBinding creates a RoleBinding object for the Broker's filter // service account 'sa' in the Namespace 'ns'. -func MakeRoleBinding(sa *corev1.ServiceAccount) *rbacv1.RoleBinding { +func MakeRoleBinding(name string, sa *corev1.ServiceAccount, clusterRoleName string) *rbacv1.RoleBinding { return &rbacv1.RoleBinding{ ObjectMeta: metav1.ObjectMeta{ Namespace: sa.Namespace, - Name: RoleBindingName, + Name: name, Labels: OwnedLabels(), }, RoleRef: rbacv1.RoleRef{ APIGroup: "rbac.authorization.k8s.io", Kind: "ClusterRole", - Name: ClusterRoleName, + Name: clusterRoleName, }, Subjects: []rbacv1.Subject{ { diff --git a/pkg/reconciler/namespace/resources/service_account.go b/pkg/reconciler/namespace/resources/service_account.go index 5e06a690f41..a1e55a2b06a 100644 --- a/pkg/reconciler/namespace/resources/service_account.go +++ b/pkg/reconciler/namespace/resources/service_account.go @@ -22,11 +22,11 @@ import ( ) // MakeServiceAccount creates a ServiceAccount object for the Namespace 'ns'. -func MakeServiceAccount(namespace string) *corev1.ServiceAccount { +func MakeServiceAccount(namespace, name string) *corev1.ServiceAccount { return &corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{ Namespace: namespace, - Name: ServiceAccountName, + Name: name, Labels: OwnedLabels(), }, } From e3aedf67d0d150fb2f13c098dd80a403e85d1d98 Mon Sep 17 00:00:00 2001 From: Adam Harwayne Date: Wed, 15 May 2019 16:05:55 -0700 Subject: [PATCH 2/4] Make a single function for reconcileBrokerServiceAccountAndRBAC. --- pkg/reconciler/namespace/namespace.go | 75 +++++------- pkg/reconciler/namespace/namespace_test.go | 134 ++++++++++++++++----- 2 files changed, 134 insertions(+), 75 deletions(-) diff --git a/pkg/reconciler/namespace/namespace.go b/pkg/reconciler/namespace/namespace.go index 128c6538d82..d51a9537708 100644 --- a/pkg/reconciler/namespace/namespace.go +++ b/pkg/reconciler/namespace/namespace.go @@ -56,8 +56,8 @@ const ( // Name of the corev1.Events emitted from the reconciliation process. brokerCreated = "BrokerCreated" - serviceAccountCreated = "BrokerFilterServiceAccountCreated" - serviceAccountRBACCreated = "BrokerFilterServiceAccountRBACCreated" + serviceAccountCreated = "BrokerServiceAccountCreated" + serviceAccountRBACCreated = "BrokerServiceAccountRBACCreated" ) var ( @@ -162,66 +162,47 @@ func (r *Reconciler) reconcile(ctx context.Context, ns *corev1.Namespace) error if ns.DeletionTimestamp != nil { return nil } - isa, err := r.reconcileBrokerServiceAccount(ctx, ns, resources.MakeServiceAccount(ns.Name, resources.IngressServiceAccountName)) - if err != nil { - logging.FromContext(ctx).Error("Unable to reconcile the Broker Ingress Service Account for the namespace", zap.Error(err)) - return err + if err := r.reconcileServiceAccountAndRoleBinding(ctx, ns, resources.IngressServiceAccountName, resources.IngressRoleBindingName, resources.IngressClusterRoleName); err != nil { + return fmt.Errorf("broker ingress: %v", err) } - - // Tell tracker to reconcile this namespace whenever the Service Account changes. - if err = r.tracker.Track(utils.ObjectRef(isa, serviceAccountGVK), ns); err != nil { - logging.FromContext(ctx).Error("Unable to track changes to ServiceAccount", zap.Error(err)) - return err + if err := r.reconcileServiceAccountAndRoleBinding(ctx, ns, resources.FilterServiceAccountName, resources.FilterRoleBindingName, resources.FilterClusterRoleName); err != nil { + return fmt.Errorf("broker filter: %v", err) } - irb, err := r.reconcileBrokerRBAC(ctx, ns, isa, - resources.MakeRoleBinding(resources.IngressRoleBindingName, isa, resources.IngressClusterRoleName)) + b, err := r.reconcileBroker(ctx, ns) if err != nil { - logging.FromContext(ctx).Error("Unable to reconcile the Broker Ingress Service Account RBAC for the namespace", zap.Error(err)) - return err + return fmt.Errorf("broker: %v", err) } - // Tell tracker to reconcile this namespace whenever the RoleBinding changes. - if err = r.tracker.Track(utils.ObjectRef(irb, roleBindingGVK), ns); err != nil { - logging.FromContext(ctx).Error("Unable to track changes to RoleBinding", zap.Error(err)) - return err + // Tell tracker to reconcile this namespace whenever the Broker changes. + if err = r.tracker.Track(utils.ObjectRef(b, brokerGVK), ns); err != nil { + return fmt.Errorf("track broker: %v", err) } - fsa, err := r.reconcileBrokerServiceAccount(ctx, ns, resources.MakeServiceAccount(ns.Name, resources.FilterServiceAccountName)) + return nil +} + +// reconcileServiceAccountAndRoleBinding reconciles the service account and role binding for +// Namespace 'ns'. +func (r *Reconciler) reconcileServiceAccountAndRoleBinding(ctx context.Context, ns *corev1.Namespace, saName, rbName, clusterRoleName string) error { + sa, err := r.reconcileBrokerServiceAccount(ctx, ns, resources.MakeServiceAccount(ns.Name, saName)) if err != nil { - logging.FromContext(ctx).Error("Unable to reconcile the Broker Filter Service Account for the namespace", zap.Error(err)) - return err + return fmt.Errorf("service account: %v", err) } // Tell tracker to reconcile this namespace whenever the Service Account changes. - if err = r.tracker.Track(utils.ObjectRef(fsa, serviceAccountGVK), ns); err != nil { - logging.FromContext(ctx).Error("Unable to track changes to ServiceAccount", zap.Error(err)) - return err + if err = r.tracker.Track(utils.ObjectRef(sa, serviceAccountGVK), ns); err != nil { + return fmt.Errorf("track service account: %v", err) } - frb, err := r.reconcileBrokerRBAC(ctx, ns, fsa, - resources.MakeRoleBinding(resources.FilterRoleBindingName, fsa, resources.FilterClusterRoleName)) + rb, err := r.reconcileBrokerRBAC(ctx, ns, sa, resources.MakeRoleBinding(rbName, sa, clusterRoleName)) if err != nil { - logging.FromContext(ctx).Error("Unable to reconcile the Broker Filter Service Account RBAC for the namespace", zap.Error(err)) - return err + return fmt.Errorf("role binding: %v", err) } // Tell tracker to reconcile this namespace whenever the RoleBinding changes. - if err = r.tracker.Track(utils.ObjectRef(frb, roleBindingGVK), ns); err != nil { - logging.FromContext(ctx).Error("Unable to track changes to RoleBinding", zap.Error(err)) - return err - } - - b, err := r.reconcileBroker(ctx, ns) - if err != nil { - logging.FromContext(ctx).Error("Unable to reconcile Broker for the namespace", zap.Error(err)) - return err - } - - // Tell tracker to reconcile this namespace whenever the Broker changes. - if err = r.tracker.Track(utils.ObjectRef(b, brokerGVK), ns); err != nil { - logging.FromContext(ctx).Error("Unable to track changes to Broker", zap.Error(err)) - return err + if err = r.tracker.Track(utils.ObjectRef(rb, roleBindingGVK), ns); err != nil { + return fmt.Errorf("track role binding: %v", err) } return nil @@ -238,7 +219,7 @@ func (r *Reconciler) reconcileBrokerServiceAccount(ctx context.Context, ns *core return nil, err } r.Recorder.Event(ns, corev1.EventTypeNormal, serviceAccountCreated, - fmt.Sprintf("Service account created for the Broker '%s'", sa.Name)) + fmt.Sprintf("Service account '%s' created for the Broker", sa.Name)) return sa, nil } else if err != nil { return nil, err @@ -249,7 +230,7 @@ func (r *Reconciler) reconcileBrokerServiceAccount(ctx context.Context, ns *core // reconcileBrokerRBAC reconciles the Broker's service account RBAC for the Namespace 'ns'. func (r *Reconciler) reconcileBrokerRBAC(ctx context.Context, ns *corev1.Namespace, sa *corev1.ServiceAccount, rb *rbacv1.RoleBinding) (*rbacv1.RoleBinding, error) { - current, err := r.KubeClientSet.RbacV1().RoleBindings(ns.Name).Get(resources.FilterRoleBindingName, metav1.GetOptions{}) + current, err := r.KubeClientSet.RbacV1().RoleBindings(ns.Name).Get(rb.Name, metav1.GetOptions{}) // If the resource doesn't exist, we'll create it. if k8serrors.IsNotFound(err) { @@ -258,7 +239,7 @@ func (r *Reconciler) reconcileBrokerRBAC(ctx context.Context, ns *corev1.Namespa return nil, err } r.Recorder.Event(ns, corev1.EventTypeNormal, serviceAccountRBACCreated, - fmt.Sprintf("Service account RBAC created for the Broker Filter '%s'", rb.Name)) + fmt.Sprintf("Service account RBAC '%s' created for the Broker", rb.Name)) return rb, nil } else if err != nil { return nil, err diff --git a/pkg/reconciler/namespace/namespace_test.go b/pkg/reconciler/namespace/namespace_test.go index ab8f549d4ad..4cfe7343604 100644 --- a/pkg/reconciler/namespace/namespace_test.go +++ b/pkg/reconciler/namespace/namespace_test.go @@ -93,6 +93,21 @@ func TestNew(t *testing.T) { } func TestAllCases(t *testing.T) { + // Events + saIngressEvent := Eventf(corev1.EventTypeNormal, "BrokerServiceAccountCreated", "Service account 'eventing-broker-ingress' created for the Broker") + rbIngressEvent := Eventf(corev1.EventTypeNormal, "BrokerServiceAccountRBACCreated", "Service account RBAC 'eventing-broker-ingress' created for the Broker") + saFilterEvent := Eventf(corev1.EventTypeNormal, "BrokerServiceAccountCreated", "Service account 'eventing-broker-filter' created for the Broker") + rbFilterEvent := Eventf(corev1.EventTypeNormal, "BrokerServiceAccountRBACCreated", "Service account RBAC 'eventing-broker-filter' created for the Broker") + brokerEvent := Eventf(corev1.EventTypeNormal, "BrokerCreated", "Default eventing.knative.dev Broker created.") + nsEvent := Eventf(corev1.EventTypeNormal, "NamespaceReconciled", "Namespace reconciled: \"test-namespace\"") + + // Object + broker := resources.MakeBroker(testNS) + saIngress := resources.MakeServiceAccount(testNS, resources.IngressServiceAccountName) + rbIngress := resources.MakeRoleBinding(resources.IngressRoleBindingName, resources.MakeServiceAccount(testNS, resources.IngressServiceAccountName), resources.IngressClusterRoleName) + saFilter := resources.MakeServiceAccount(testNS, resources.FilterServiceAccountName) + rbFilter := resources.MakeRoleBinding(resources.FilterRoleBindingName, resources.MakeServiceAccount(testNS, resources.FilterServiceAccountName), resources.FilterClusterRoleName) + table := TableTest{{ Name: "bad workqueue key", // Make sure Reconcile handles bad keys. @@ -124,7 +139,7 @@ func TestAllCases(t *testing.T) { }, Key: testNS, WantEvents: []string{ - Eventf(corev1.EventTypeNormal, "NamespaceReconciled", "Namespace reconciled: \"test-namespace\""), + nsEvent, }, }, { Name: "Namespace enabled", @@ -137,15 +152,19 @@ func TestAllCases(t *testing.T) { SkipNamespaceValidation: true, WantErr: false, WantEvents: []string{ - Eventf(corev1.EventTypeNormal, "BrokerFilterServiceAccountCreated", "Service account created for the Broker 'eventing-broker-filter'"), - Eventf(corev1.EventTypeNormal, "BrokerFilterServiceAccountRBACCreated", "Service account RBAC created for the Broker Filter 'eventing-broker-filter'"), - Eventf(corev1.EventTypeNormal, "BrokerCreated", "Default eventing.knative.dev Broker created."), - Eventf(corev1.EventTypeNormal, "NamespaceReconciled", "Namespace reconciled: \"test-namespace\""), + saIngressEvent, + rbIngressEvent, + saFilterEvent, + rbFilterEvent, + brokerEvent, + nsEvent, }, WantCreates: []metav1.Object{ - resources.MakeBroker(testNS), - resources.MakeServiceAccount(testNS, resources.FilterServiceAccountName), - resources.MakeRoleBinding(resources.FilterRoleBindingName, resources.MakeServiceAccount(testNS, resources.FilterServiceAccountName), resources.FilterClusterRoleName), + broker, + saIngress, + rbIngress, + saFilter, + rbFilter, }, }, { Name: "Namespace enabled, broker exists", @@ -159,13 +178,17 @@ func TestAllCases(t *testing.T) { SkipNamespaceValidation: true, WantErr: false, WantEvents: []string{ - Eventf(corev1.EventTypeNormal, "BrokerFilterServiceAccountCreated", "Service account created for the Broker 'eventing-broker-filter'"), - Eventf(corev1.EventTypeNormal, "BrokerFilterServiceAccountRBACCreated", "Service account RBAC created for the Broker Filter 'eventing-broker-filter'"), - Eventf(corev1.EventTypeNormal, "NamespaceReconciled", "Namespace reconciled: \"test-namespace\""), + saIngressEvent, + rbIngressEvent, + saFilterEvent, + rbFilterEvent, + nsEvent, }, WantCreates: []metav1.Object{ - resources.MakeServiceAccount(testNS, resources.FilterServiceAccountName), - resources.MakeRoleBinding(resources.FilterRoleBindingName, resources.MakeServiceAccount(testNS, resources.FilterServiceAccountName), resources.FilterClusterRoleName), + saIngress, + rbIngress, + saFilter, + rbFilter, }, }, { Name: "Namespace enabled, broker exists with no label", @@ -184,45 +207,100 @@ func TestAllCases(t *testing.T) { SkipNamespaceValidation: true, WantErr: false, }, { - Name: "Namespace enabled, service account exists", + Name: "Namespace enabled, ingress service account exists", Objects: []runtime.Object{ NewNamespace(testNS, WithNamespaceLabeled(resources.InjectionEnabledLabels()), ), - resources.MakeServiceAccount(testNS, resources.FilterServiceAccountName), + saIngress, }, Key: testNS, SkipNamespaceValidation: true, WantErr: false, WantEvents: []string{ - Eventf(corev1.EventTypeNormal, "BrokerFilterServiceAccountRBACCreated", "Service account RBAC created for the Broker Filter 'eventing-broker-filter'"), - Eventf(corev1.EventTypeNormal, "BrokerCreated", "Default eventing.knative.dev Broker created."), - Eventf(corev1.EventTypeNormal, "NamespaceReconciled", "Namespace reconciled: \"test-namespace\""), + rbIngressEvent, + saFilterEvent, + rbFilterEvent, + brokerEvent, + nsEvent, }, WantCreates: []metav1.Object{ - resources.MakeBroker(testNS), - resources.MakeServiceAccount(testNS, resources.FilterServiceAccountName), - resources.MakeRoleBinding(resources.FilterRoleBindingName, resources.MakeServiceAccount(testNS, resources.FilterServiceAccountName), resources.FilterClusterRoleName), + broker, + rbIngress, + saFilter, + rbFilter, }, }, { - Name: "Namespace enabled, role binding exists", + Name: "Namespace enabled, ingress role binding exists", Objects: []runtime.Object{ NewNamespace(testNS, WithNamespaceLabeled(resources.InjectionEnabledLabels()), ), - resources.MakeRoleBinding(resources.FilterRoleBindingName, resources.MakeServiceAccount(testNS, resources.FilterServiceAccountName), resources.FilterClusterRoleName), + rbIngress, }, Key: testNS, SkipNamespaceValidation: true, WantErr: false, WantEvents: []string{ - Eventf(corev1.EventTypeNormal, "BrokerFilterServiceAccountCreated", "Service account created for the Broker 'eventing-broker-filter'"), - Eventf(corev1.EventTypeNormal, "BrokerCreated", "Default eventing.knative.dev Broker created."), - Eventf(corev1.EventTypeNormal, "NamespaceReconciled", "Namespace reconciled: \"test-namespace\""), + saIngressEvent, + saFilterEvent, + rbFilterEvent, + brokerEvent, + nsEvent, }, WantCreates: []metav1.Object{ - resources.MakeBroker(testNS), - resources.MakeServiceAccount(testNS, resources.FilterServiceAccountName), + broker, + saIngress, + saFilter, + rbFilter, + }, + }, { + Name: "Namespace enabled, filter service account exists", + Objects: []runtime.Object{ + NewNamespace(testNS, + WithNamespaceLabeled(resources.InjectionEnabledLabels()), + ), + saFilter, + }, + Key: testNS, + SkipNamespaceValidation: true, + WantErr: false, + WantEvents: []string{ + saIngressEvent, + rbIngressEvent, + rbFilterEvent, + brokerEvent, + nsEvent, + }, + WantCreates: []metav1.Object{ + broker, + saIngress, + rbIngress, + rbFilter, + }, + }, { + Name: "Namespace enabled, filter role binding exists", + Objects: []runtime.Object{ + NewNamespace(testNS, + WithNamespaceLabeled(resources.InjectionEnabledLabels()), + ), + rbFilter, + }, + Key: testNS, + SkipNamespaceValidation: true, + WantErr: false, + WantEvents: []string{ + saIngressEvent, + rbIngressEvent, + saFilterEvent, + brokerEvent, + nsEvent, + }, + WantCreates: []metav1.Object{ + broker, + saIngress, + rbIngress, + saFilter, }, }, // TODO: we need a existing default un-owned test. From 8464d2542b0387b52ce0cce184a4880f58605d01 Mon Sep 17 00:00:00 2001 From: Adam Harwayne Date: Wed, 15 May 2019 17:04:58 -0700 Subject: [PATCH 3/4] Add the Ingress SA creation to the 'manual' test. --- test/e2e/broker_event_transformation_test.go | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/test/e2e/broker_event_transformation_test.go b/test/e2e/broker_event_transformation_test.go index 150fbad7bf4..4751f663a5f 100644 --- a/test/e2e/broker_event_transformation_test.go +++ b/test/e2e/broker_event_transformation_test.go @@ -43,10 +43,13 @@ Note: the number denotes the sequence of the event that flows in this test case. */ func TestEventTransformationForTrigger(t *testing.T) { const ( - brokerName = "e2e-eventtransformation-broker" - saName = "eventing-broker-filter" + brokerName = "e2e-eventtransformation-broker" + saIngressName = "eventing-broker-ingress" + saFilterName = "eventing-broker-filter" + // This ClusterRole is installed in Knative Eventing setup, see https://github.com/knative/eventing/tree/master/docs/broker#manual-setup. - crName = "eventing-broker-filter" + crIngressName = "eventing-broker-ingress" + crFilterName = "eventing-broker-filter" any = v1alpha1.TriggerAnyFilter eventType1 = "type1" @@ -66,9 +69,13 @@ func TestEventTransformationForTrigger(t *testing.T) { defer TearDown(clients, ns, cleaner, t.Logf) // creates ServiceAccount and ClusterRoleBinding with default cluster-admin role - err := CreateServiceAccountAndBinding(clients, saName, crName, ns, t.Logf, cleaner) + err := CreateServiceAccountAndBinding(clients, saIngressName, crIngressName, ns, t.Logf, cleaner) + if err != nil { + t.Fatalf("Failed to create the Ingress ServiceAccount and ServiceAccountRoleBinding: %v", err) + } + err = CreateServiceAccountAndBinding(clients, saFilterName, crFilterName, ns, t.Logf, cleaner) if err != nil { - t.Fatalf("Failed to create the ServiceAccount and ServiceAccountRoleBinding: %v", err) + t.Fatalf("Failed to create the Filter ServiceAccount and ServiceAccountRoleBinding: %v", err) } // create a new broker From fcdced0f73d92385f76df7c408e0c456b0606a50 Mon Sep 17 00:00:00 2001 From: Adam Harwayne Date: Mon, 20 May 2019 14:26:50 -0700 Subject: [PATCH 4/4] PR comments. --- docs/broker/README.md | 8 ++++++-- pkg/reconciler/namespace/namespace.go | 8 ++++---- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/docs/broker/README.md b/docs/broker/README.md index 4a5805592d7..9c2c3893984 100644 --- a/docs/broker/README.md +++ b/docs/broker/README.md @@ -275,9 +275,9 @@ spec: Broker and Trigger are intended to be black boxes. How they are implemented should not matter to the end user. This section describes the specific -implementation that is currently in the repository. However, **the implmentation +implementation that is currently in the repository. However, **the implementation may change at any time, absolutely no guarantees are made about the -implmentation**. +implementation**. ### Namespace @@ -287,6 +287,10 @@ Namespaces are reconciled by the `knative-eventing-injection: enabled`. If that label is present, then the `Namespace Reconciler` reconciles: +1. Creates the Broker Ingress' `ServiceAccount`, `eventing-ingress-filter`. +1. Ensures that `ServiceAccount` has the requisite RBAC permissions by giving it + the [`eventing-broker-ingress`](../../config/200-broker-clusterrole.yaml) + `Role`. 1. Creates the Broker Filter's `ServiceAccount`, `eventing-broker-filter`. 1. Ensures that `ServiceAccount` has the requisite RBAC permissions by giving it the [`eventing-broker-filter`](../../config/200-broker-clusterrole.yaml) diff --git a/pkg/reconciler/namespace/namespace.go b/pkg/reconciler/namespace/namespace.go index d51a9537708..f11c5146d68 100644 --- a/pkg/reconciler/namespace/namespace.go +++ b/pkg/reconciler/namespace/namespace.go @@ -187,22 +187,22 @@ func (r *Reconciler) reconcile(ctx context.Context, ns *corev1.Namespace) error func (r *Reconciler) reconcileServiceAccountAndRoleBinding(ctx context.Context, ns *corev1.Namespace, saName, rbName, clusterRoleName string) error { sa, err := r.reconcileBrokerServiceAccount(ctx, ns, resources.MakeServiceAccount(ns.Name, saName)) if err != nil { - return fmt.Errorf("service account: %v", err) + return fmt.Errorf("service account '%s': %v", saName, err) } // Tell tracker to reconcile this namespace whenever the Service Account changes. if err = r.tracker.Track(utils.ObjectRef(sa, serviceAccountGVK), ns); err != nil { - return fmt.Errorf("track service account: %v", err) + return fmt.Errorf("track service account '%s': %v", sa.Name, err) } rb, err := r.reconcileBrokerRBAC(ctx, ns, sa, resources.MakeRoleBinding(rbName, sa, clusterRoleName)) if err != nil { - return fmt.Errorf("role binding: %v", err) + return fmt.Errorf("role binding '%s': %v", rbName, err) } // Tell tracker to reconcile this namespace whenever the RoleBinding changes. if err = r.tracker.Track(utils.ObjectRef(rb, roleBindingGVK), ns); err != nil { - return fmt.Errorf("track role binding: %v", err) + return fmt.Errorf("track role binding '%s': %v", rb.Name, err) } return nil