From 87666ef42f55b24f8d888a85845910495fa2371b Mon Sep 17 00:00:00 2001 From: Ben Wheatley Date: Mon, 28 Aug 2023 18:40:08 +0100 Subject: [PATCH] consoles: Make DirectoryRoleBinding optional Currently, the workloads controller has a hard dependency on `DirectoryRoleBinding`s, as provided by the RBAC controller, in order to create consoles. If you try to do this, without having the RBAC controller installed, then the reconcile loop gets stuck, as the rolebinding to access the console can't be provisioned. This is a bit presumptuous; it should be possible to run consoles without DRBs, e.g. if you just want to reference plain `User` kinds in `additionalAttachSubjects`. This change adds a flag, which makes the usage of `DirectoryRoleBinding` optional. The flag defaults to true, meaning that this isn't a breaking change. We intended to use this in conjunction with [Google Groups for RBAC][0] as an alternative. [0]: https://cloud.google.com/kubernetes-engine/docs/how-to/google-groups-rbac --- cmd/workloads-manager/main.go | 36 +++---- config/base/managers/workloads.yaml | 1 + controllers/workloads/console/controller.go | 98 ++++++++++++++++--- .../console/integration/suite_test.go | 11 ++- pkg/recutil/reconcile.go | 20 ++++ 5 files changed, 128 insertions(+), 38 deletions(-) diff --git a/cmd/workloads-manager/main.go b/cmd/workloads-manager/main.go index b1ab4627..bfdc134b 100644 --- a/cmd/workloads-manager/main.go +++ b/cmd/workloads-manager/main.go @@ -24,14 +24,15 @@ import ( var ( scheme = runtime.NewScheme() - app = kingpin.New("workloads-manager", "Manages workloads.crd.gocardless.com resources").Version(cmd.VersionStanza()) - contextName = app.Flag("context-name", "Distinct name for the context this controller runs within. Usually the user-facing name of the kubernetes context for the cluster").Envar("CONTEXT_NAME").String() - pubsubProjectId = app.Flag("pubsub-project-id", "ID for the project containing the Pub/Sub topic for console event publishing").Envar("PUBSUB_PROJECT_ID").String() - pubsubTopicId = app.Flag("pubsub-topic-id", "ID of the topic to publish lifecycle event messages").Envar("PUBSUB_TOPIC_ID").String() - enableSessionRecording = app.Flag("session-recording", "Enable session recording features").Envar("ENABLE_SESSION_RECORDING").Default("false").Bool() - sessionSidecarImage = app.Flag("session-sidecar-image", "Container image to use for the session recording sidecar container").Envar("SESSION_SIDECAR_IMAGE").Default("").String() - sessionPubsubProjectId = app.Flag("session-pubsub-project-id", "ID for the project containing the Pub/Sub topic for session recording").Envar("SESSION_PUBSUB_PROJECT_ID").Default("").String() - sessionPubsubTopicId = app.Flag("session-pubsub-topic-id", "ID of the topic to publish session recording data to").Envar("SESSION_PUBSUB_TOPIC_ID").Default("").String() + app = kingpin.New("workloads-manager", "Manages workloads.crd.gocardless.com resources").Version(cmd.VersionStanza()) + contextName = app.Flag("context-name", "Distinct name for the context this controller runs within. Usually the user-facing name of the kubernetes context for the cluster").Envar("CONTEXT_NAME").String() + pubsubProjectId = app.Flag("pubsub-project-id", "ID for the project containing the Pub/Sub topic for console event publishing").Envar("PUBSUB_PROJECT_ID").String() + pubsubTopicId = app.Flag("pubsub-topic-id", "ID of the topic to publish lifecycle event messages").Envar("PUBSUB_TOPIC_ID").String() + enableDirectoryRoleBinding = app.Flag("directory-role-binding", "Use DirectoryRoleBinding for provisioning RBAC against console objects").Envar("ENABLE_DIRECTORY_ROLE_BINDING").Default("true").Bool() + enableSessionRecording = app.Flag("session-recording", "Enable session recording features").Envar("ENABLE_SESSION_RECORDING").Default("false").Bool() + sessionSidecarImage = app.Flag("session-sidecar-image", "Container image to use for the session recording sidecar container").Envar("SESSION_SIDECAR_IMAGE").Default("").String() + sessionPubsubProjectId = app.Flag("session-pubsub-project-id", "ID for the project containing the Pub/Sub topic for session recording").Envar("SESSION_PUBSUB_PROJECT_ID").Default("").String() + sessionPubsubTopicId = app.Flag("session-pubsub-topic-id", "ID of the topic to publish session recording data to").Envar("SESSION_PUBSUB_TOPIC_ID").Default("").String() commonOpts = cmd.NewCommonOptions(app).WithMetrics(app) ) @@ -93,15 +94,16 @@ func main() { // controller if err = (&consolecontroller.ConsoleReconciler{ - Client: mgr.GetClient(), - LifecycleRecorder: lifecycleRecorder, - ConsoleIdBuilder: idBuilder, - Log: ctrl.Log.WithName("controllers").WithName("console"), - Scheme: mgr.GetScheme(), - EnableSessionRecording: *enableSessionRecording, - SessionSidecarImage: *sessionSidecarImage, - SessionPubsubProjectId: *sessionPubsubProjectId, - SessionPubsubTopicId: *sessionPubsubTopicId, + Client: mgr.GetClient(), + LifecycleRecorder: lifecycleRecorder, + ConsoleIdBuilder: idBuilder, + Log: ctrl.Log.WithName("controllers").WithName("console"), + Scheme: mgr.GetScheme(), + EnableDirectoryRoleBinding: *enableDirectoryRoleBinding, + EnableSessionRecording: *enableSessionRecording, + SessionSidecarImage: *sessionSidecarImage, + SessionPubsubProjectId: *sessionPubsubProjectId, + SessionPubsubTopicId: *sessionPubsubTopicId, }).SetupWithManager(ctx, mgr); err != nil { app.Fatalf("failed to create controller: %v", err) } diff --git a/config/base/managers/workloads.yaml b/config/base/managers/workloads.yaml index 20775d55..eba91570 100644 --- a/config/base/managers/workloads.yaml +++ b/config/base/managers/workloads.yaml @@ -30,6 +30,7 @@ rules: - rbac.authorization.k8s.io resources: - roles + - rolebindings verbs: - "*" - apiGroups: diff --git a/controllers/workloads/console/controller.go b/controllers/workloads/console/controller.go index e9f76657..f86fe48a 100644 --- a/controllers/workloads/console/controller.go +++ b/controllers/workloads/console/controller.go @@ -62,6 +62,7 @@ const ( ConsoleAuthorisation = "consoleauthorisation" ConsoleTemplate = "consoletemplate" Role = "role" + RoleBinding = "rolebinding" DirectoryRoleBinding = "directoryrolebinding" DefaultTTLBeforeRunning = 1 * time.Hour @@ -88,6 +89,9 @@ type ConsoleReconciler struct { ConsoleIdBuilder workloadsv1alpha1.ConsoleIdBuilder Log logr.Logger Scheme *runtime.Scheme + // Use DRBs for RBAC on console objects + EnableDirectoryRoleBinding bool + // Enable injection of console session recording using tlog EnableSessionRecording bool // The image reference for the sidecar to inject to stream session @@ -140,10 +144,10 @@ func (r *ConsoleReconciler) createOrUpdateUserRbac(logger logr.Logger, ctx conte // Create or update the user role role := buildUserRole(req.NamespacedName, csl.Status.PodName) if err := r.createOrUpdate(ctx, logger, csl, role, Role, recutil.RoleDiff); err != nil { - return err + return errors.Wrap(err, "failed to create role for user") } - // Create or update the directory role binding + // Create or update the role binding subjects := append( tpl.Spec.AdditionalAttachSubjects, rbacv1.Subject{Kind: "User", Name: csl.Spec.User}, @@ -153,9 +157,16 @@ func (r *ConsoleReconciler) createOrUpdateUserRbac(logger logr.Logger, ctx conte subjects = append(subjects, authorisation.Spec.Authorisations...) } - drb := buildUserDirectoryRoleBinding(req.NamespacedName, role, subjects) - if err := r.createOrUpdate(ctx, logger, csl, drb, DirectoryRoleBinding, recutil.DirectoryRoleBindingDiff); err != nil { - return err + if r.EnableDirectoryRoleBinding { + drb := buildUserDirectoryRoleBinding(req.NamespacedName, role, subjects) + if err := r.createOrUpdate(ctx, logger, csl, drb, DirectoryRoleBinding, recutil.DirectoryRoleBindingDiff); err != nil { + return errors.Wrap(err, "failed to create directory rolebinding for user") + } + } else { + rb := buildUserRoleBinding(req.NamespacedName, role, subjects) + if err := r.createOrUpdate(ctx, logger, csl, rb, RoleBinding, recutil.RoleBindingDiff); err != nil { + return errors.Wrap(err, "failed to create rolebinding for user") + } } return nil @@ -168,7 +179,7 @@ func (r *ConsoleReconciler) createOrUpdateServiceRbac(logger logr.Logger, ctx co // result is to allow the console pod to read information about itself, which is required by our wrapper so that it can // exit the sidecar when the main console container exits. - // We already create roles and directory rolebindings with the same name as + // We already create roles and rolebindings with the same name as // the console to provide permissions on the job/pod. Therefore for the name // of these objects, suffix the console name with '-svc'. rbacName := types.NamespacedName{ @@ -179,13 +190,20 @@ func (r *ConsoleReconciler) createOrUpdateServiceRbac(logger logr.Logger, ctx co // Create or update the service role role := buildServiceRole(rbacName, csl.Status.PodName) if err := r.createOrUpdate(ctx, logger, csl, role, Role, recutil.RoleDiff); err != nil { - return err + return errors.Wrap(err, "failed to create role for service") } // Create or update the service rolebinding - drb := buildServiceRoleBinding(rbacName, role, tpl.Spec.Template.Spec.ServiceAccountName) - if err := r.createOrUpdate(ctx, logger, csl, drb, DirectoryRoleBinding, recutil.DirectoryRoleBindingDiff); err != nil { - return err + if r.EnableDirectoryRoleBinding { + drb := buildServiceDirectoryRoleBinding(rbacName, role, tpl.Spec.Template.Spec.ServiceAccountName) + if err := r.createOrUpdate(ctx, logger, csl, drb, DirectoryRoleBinding, recutil.DirectoryRoleBindingDiff); err != nil { + return errors.Wrap(err, "failed to create directory rolebinding for service") + } + } else { + rb := buildServiceRoleBinding(rbacName, role, tpl.Spec.Template.Spec.ServiceAccountName) + if err := r.createOrUpdate(ctx, logger, csl, rb, RoleBinding, recutil.RoleBindingDiff); err != nil { + return errors.Wrap(err, "failed to create rolebinding for service") + } } return nil @@ -969,7 +987,7 @@ func buildServiceRole(name types.NamespacedName, podName string) *rbacv1.Role { } } -func buildServiceRoleBinding(name types.NamespacedName, role *rbacv1.Role, serviceAccountName string) *rbacv1alpha1.DirectoryRoleBinding { +func buildServiceDirectoryRoleBinding(name types.NamespacedName, role *rbacv1.Role, serviceAccountName string) *rbacv1alpha1.DirectoryRoleBinding { // There are cases where the console template lacks a service account // In these cases the pods and jobs will run under the "default" account // for the namespace @@ -997,6 +1015,32 @@ func buildServiceRoleBinding(name types.NamespacedName, role *rbacv1.Role, servi } } +func buildServiceRoleBinding(name types.NamespacedName, role *rbacv1.Role, serviceAccountName string) *rbacv1.RoleBinding { + // There are cases where the console template lacks a service account + // In these cases the pods and jobs will run under the "default" account + // for the namespace + if len(serviceAccountName) == 0 { + serviceAccountName = "default" + } + return &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: name.Name, + Namespace: name.Namespace, + }, + Subjects: []rbacv1.Subject{ + { + Kind: "ServiceAccount", + Name: serviceAccountName, + }, + }, + RoleRef: rbacv1.RoleRef{ + APIGroup: rbacv1.GroupName, + Kind: "Role", + Name: name.Name, + }, + } +} + func buildUserRole(name types.NamespacedName, podName string) *rbacv1.Role { return &rbacv1.Role{ ObjectMeta: metav1.ObjectMeta{ @@ -1043,6 +1087,21 @@ func buildUserDirectoryRoleBinding(name types.NamespacedName, role *rbacv1.Role, } } +func buildUserRoleBinding(name types.NamespacedName, role *rbacv1.Role, subjects []rbacv1.Subject) *rbacv1.RoleBinding { + return &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: name.Name, + Namespace: name.Namespace, + }, + Subjects: subjects, + RoleRef: rbacv1.RoleRef{ + APIGroup: rbacv1.GroupName, + Kind: "Role", + Name: name.Name, + }, + } +} + func (r *ConsoleReconciler) createAuthorisationObjects(ctx context.Context, logger logr.Logger, csl *workloadsv1alpha1.Console, name types.NamespacedName, subjects []rbacv1.Subject) error { authorisation := &workloadsv1alpha1.ConsoleAuthorisation{ ObjectMeta: metav1.ObjectMeta{ @@ -1061,7 +1120,7 @@ func (r *ConsoleReconciler) createAuthorisationObjects(ctx context.Context, logg return errors.Wrap(err, "failed to create consoleauthorisation") } - // We already create roles and directory rolebindings with the same name as + // We already create roles and rolebindings with the same name as // the console to provide permissions on the job/pod. Therefore for the name // of these objects, suffix the console name with '-authorisation'. rbacName := types.NamespacedName{ @@ -1089,10 +1148,17 @@ func (r *ConsoleReconciler) createAuthorisationObjects(ctx context.Context, logg return errors.Wrap(err, "failed to create role for consoleauthorisation") } - // Create or update the directory role binding - drb := buildUserDirectoryRoleBinding(rbacName, role, subjects) - if err := r.createOrUpdate(ctx, logger, csl, drb, DirectoryRoleBinding, recutil.DirectoryRoleBindingDiff); err != nil { - return errors.Wrap(err, "failed to create directory rolebinding for consoleauthorisation") + // Create or update the role binding + if r.EnableDirectoryRoleBinding { + drb := buildUserDirectoryRoleBinding(rbacName, role, subjects) + if err := r.createOrUpdate(ctx, logger, csl, drb, DirectoryRoleBinding, recutil.DirectoryRoleBindingDiff); err != nil { + return errors.Wrap(err, "failed to create directory rolebinding for consoleauthorisation") + } + } else { + rb := buildUserRoleBinding(rbacName, role, subjects) + if err := r.createOrUpdate(ctx, logger, csl, rb, RoleBinding, recutil.RoleBindingDiff); err != nil { + return errors.Wrap(err, "failed to create rolebinding for consoleauthorisation") + } } return nil diff --git a/controllers/workloads/console/integration/suite_test.go b/controllers/workloads/console/integration/suite_test.go index 370fee5d..b41e7f89 100644 --- a/controllers/workloads/console/integration/suite_test.go +++ b/controllers/workloads/console/integration/suite_test.go @@ -99,11 +99,12 @@ var _ = BeforeSuite(func() { }) err = (&consolecontroller.ConsoleReconciler{ - Client: mgr.GetClient(), - LifecycleRecorder: lifecycleRecorder, - Log: ctrl.Log.WithName("controllers").WithName("console"), - Scheme: mgr.GetScheme(), - ConsoleIdBuilder: workloadsv1alpha1.NewConsoleIdBuilder("test"), + Client: mgr.GetClient(), + LifecycleRecorder: lifecycleRecorder, + Log: ctrl.Log.WithName("controllers").WithName("console"), + Scheme: mgr.GetScheme(), + ConsoleIdBuilder: workloadsv1alpha1.NewConsoleIdBuilder("test"), + EnableDirectoryRoleBinding: true, }).SetupWithManager(context.TODO(), mgr) Expect(err).ToNot(HaveOccurred()) diff --git a/pkg/recutil/reconcile.go b/pkg/recutil/reconcile.go index 976391d9..bacd9af4 100644 --- a/pkg/recutil/reconcile.go +++ b/pkg/recutil/reconcile.go @@ -212,3 +212,23 @@ func DirectoryRoleBindingDiff(expectedObj runtime.Object, existingObj runtime.Ob return operation } + +// RoleBindingDiff is a DiffFunc for RoleBindings +func RoleBindingDiff(expectedObj runtime.Object, existingObj runtime.Object) Outcome { + expected := expectedObj.(*rbacv1.RoleBinding) + existing := existingObj.(*rbacv1.RoleBinding) + + operation := None + + if !reflect.DeepEqual(expected.Subjects, existing.Subjects) { + existing.Subjects = expected.Subjects + operation = Update + } + + if !reflect.DeepEqual(expected.RoleRef, existing.RoleRef) { + existing.RoleRef = expected.RoleRef + operation = Update + } + + return operation +}