From a772f615cf62996bf3c4d3e40e03941a9173b705 Mon Sep 17 00:00:00 2001 From: Sebastian Widmer Date: Fri, 26 Jan 2024 11:09:57 +0100 Subject: [PATCH] Implement BillingEntity RBAC reconcile Used a cron job since we never implemented watch for BEs. --- apiserver/billing/rbac.go | 71 ++---------- config/rbac/controller/role.yaml | 9 ++ controller.go | 51 +++++++-- controllers/billingentity_rbac_cronjob.go | 66 +++++++++++ .../billingentity_rbac_cronjob_test.go | 53 +++++++++ controllers/common_test.go | 41 ++++++- pkg/billingrbac/billingrbac.go | 108 ++++++++++++++++++ 7 files changed, 323 insertions(+), 76 deletions(-) create mode 100644 controllers/billingentity_rbac_cronjob.go create mode 100644 controllers/billingentity_rbac_cronjob_test.go create mode 100644 pkg/billingrbac/billingrbac.go diff --git a/apiserver/billing/rbac.go b/apiserver/billing/rbac.go index 4f099e1e..5df5d37d 100644 --- a/apiserver/billing/rbac.go +++ b/apiserver/billing/rbac.go @@ -5,7 +5,6 @@ import ( "fmt" "go.uber.org/multierr" - rbacv1 "k8s.io/api/rbac/v1" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -15,6 +14,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "github.com/appuio/control-api/apiserver/billing/odoostorage" + "github.com/appuio/control-api/pkg/billingrbac" ) // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles;clusterrolebindings,verbs=get;list;watch;create;delete;patch;update;edit @@ -44,68 +44,12 @@ func (c *createRBACWrapper) Create(ctx context.Context, obj runtime.Object, crea return createdObj, fmt.Errorf("could not get name of created object: %w", err) } - viewRoleName := fmt.Sprintf("billingentities-%s-viewer", objName) - viewRole := &rbacv1.ClusterRole{ - ObjectMeta: metav1.ObjectMeta{ - Name: viewRoleName, - }, - Rules: []rbacv1.PolicyRule{ - { - APIGroups: []string{"rbac.appuio.io"}, - Resources: []string{"billingentities"}, - Verbs: []string{"get"}, - ResourceNames: []string{objName}, - }, - }, - } - viewRoleBinding := &rbacv1.ClusterRoleBinding{ - ObjectMeta: metav1.ObjectMeta{ - Name: viewRoleName, - }, - Subjects: []rbacv1.Subject{}, - RoleRef: rbacv1.RoleRef{ - Kind: "ClusterRole", - APIGroup: "rbac.authorization.k8s.io", - Name: viewRoleName, - }, - } - adminRoleName := fmt.Sprintf("billingentities-%s-admin", objName) - adminRole := &rbacv1.ClusterRole{ - ObjectMeta: metav1.ObjectMeta{ - Name: adminRoleName, - }, - Rules: []rbacv1.PolicyRule{ - { - APIGroups: []string{"rbac.appuio.io", "billing.appuio.io"}, - Resources: []string{"billingentities"}, - Verbs: []string{"get", "patch", "update", "edit"}, - ResourceNames: []string{objName}, - }, - { - APIGroups: []string{"rbac.authorization.k8s.io"}, - Resources: []string{"clusterrolebindings"}, - Verbs: []string{"get", "edit", "update", "patch"}, - ResourceNames: []string{viewRoleName, adminRoleName}, - }, - }, - } - adminRoleBinding := &rbacv1.ClusterRoleBinding{ - ObjectMeta: metav1.ObjectMeta{ - Name: adminRoleName, - }, - Subjects: []rbacv1.Subject{ - { - Kind: "User", - APIGroup: "rbac.authorization.k8s.io", - Name: user.GetName(), - }, - }, - RoleRef: rbacv1.RoleRef{ - Kind: "ClusterRole", - APIGroup: "rbac.authorization.k8s.io", - Name: adminRoleName, - }, - } + ar, arb, vr, vrb := billingrbac.ClusterRoles(objName, billingrbac.ClusterRolesParams{ + AllowSubjectsToViewRole: true, + + AdminUsers: []string{user.GetName()}, + }) + toCreate := []client.Object{ar, arb, vr, vrb} rollback := func() error { if deleter, canDelete := c.Storage.(rest.GracefulDeleter); canDelete { @@ -116,7 +60,6 @@ func (c *createRBACWrapper) Create(ctx context.Context, obj runtime.Object, crea return nil } - toCreate := []client.Object{viewRole, viewRoleBinding, adminRole, adminRoleBinding} created := make([]client.Object, 0, len(toCreate)) var createErr error for _, obj := range toCreate { diff --git a/config/rbac/controller/role.yaml b/config/rbac/controller/role.yaml index 0f7f48d6..d6b0131a 100644 --- a/config/rbac/controller/role.yaml +++ b/config/rbac/controller/role.yaml @@ -97,6 +97,13 @@ rules: - get - patch - update +- apiGroups: + - billing.appuio.io + - rbac.appuio.io + resources: + - billingentities + verbs: + - '*' - apiGroups: - organization.appuio.io resources: @@ -179,6 +186,8 @@ rules: - clusterroles verbs: - create + - delete + - edit - get - list - patch diff --git a/controller.go b/controller.go index 13d76202..cd1bb04c 100644 --- a/controller.go +++ b/controller.go @@ -43,7 +43,7 @@ A user of APPUiO Cloud has invited you to join them. Follow https://portal.dev/i APPUiO Cloud is a shared Kubernetes offering based on OpenShift provided by https://vshn.ch. -Unsure what to do next? Accept this invitation using the link above, login to one of the zones listed at https://portal.appuio.cloud/zones, deploy your application. A getting started guide on how to do so, is available at https://docs.appuio.cloud/user/tutorials/getting-started.html. To learn more about APPUiO Cloud in general, please visit https://appuio.cloud. +Unsure what to do next? Accept this invitation using the link above, login to one of the zones listed at https://portal.appuio.cloud/zones, deploy your application. A getting started guide on how to do so, is available at https://docs.appuio.cloud/user/tutorials/getting-started.html. To learn more about APPUiO Cloud in general, please visit https://appuio.cloud. If you have any problems or questions, please email us at support@appuio.ch. @@ -102,7 +102,8 @@ func ControllerCommand() *cobra.Command { billingEntityEmailBodyTemplate := cmd.Flags().String("billingentity-email-body-template", defaultBillingEntityEmailTemplate, "Body for billing entity modification update mails") billingEntityEmailRecipient := cmd.Flags().String("billingentity-email-recipient", "", "Recipient e-mail address for billing entity modification update mails") billingEntityEmailSubject := cmd.Flags().String("billingentity-email-subject", "An APPUiO Billing Entity has been updated", "Subject for billing entity modification update mails") - billingEntityCronInterval := cmd.Flags().String("billingentity-email-cron-interval", "@every 1h", "Cron interval for how frequently billing entity update e-mails are sent") + billingEntityEmailCronInterval := cmd.Flags().String("billingentity-email-cron-interval", "@every 1h", "Cron interval for how frequently billing entity update e-mails are sent") + billingEntityRBACCronInterval := cmd.Flags().String("billingentity-rbac-cron-interval", "@every 3m", "Cron interval for how frequently billing entity rbac is reconciled") saleOrderCompatMode := cmd.Flags().Bool("sale-order-compatibility-mode", false, "Whether to enable compatibility mode for Sales Orders. If enabled, odoo8 billing entity IDs are used to create sales orders in odoo16.") saleOrderStorage := cmd.Flags().String("sale-order-storage", "none", "Type of sale order storage to use. Valid values are `none` and `odoo16`") @@ -212,9 +213,10 @@ func ControllerCommand() *cobra.Command { os.Exit(1) } - cron, err := setupCron( + setupLog.Info("setting up email cron") + emailCron, err := setupEmailCron( ctx, - *billingEntityCronInterval, + *billingEntityEmailCronInterval, mgr, beMailSender, *billingEntityEmailRecipient, @@ -223,7 +225,19 @@ func ControllerCommand() *cobra.Command { setupLog.Error(err, "unable to setup email cron") os.Exit(1) } - cron.Start() + emailCron.Start() + + setupLog.Info("setting up billing entity rbac cron") + beRBACCron, err := setupBillingEntityRBACCron( + ctx, + *billingEntityRBACCronInterval, + mgr, + ) + if err != nil { + setupLog.Error(err, "unable to setup rbac cron") + os.Exit(1) + } + beRBACCron.Start() setupLog.Info("starting manager") if err := mgr.Start(ctx); err != nil { @@ -231,7 +245,10 @@ func ControllerCommand() *cobra.Command { os.Exit(1) } setupLog.Info("Stopping...") - <-cron.Stop().Done() + ecs := emailCron.Stop() + becs := beRBACCron.Stop() + <-ecs.Done() + <-becs.Done() } return cmd @@ -390,7 +407,7 @@ func setupManager( return mgr, err } -func setupCron( +func setupEmailCron( ctx context.Context, crontab string, mgr ctrl.Manager, @@ -406,7 +423,7 @@ func setupCron( ) metrics.Registry.MustRegister(bemail.GetMetrics()) - syncLog := ctrl.Log.WithName("cron") + syncLog := ctrl.Log.WithName("email_cron") c := cron.New() _, err := c.AddFunc(crontab, func() { @@ -423,3 +440,21 @@ func setupCron( } return c, nil } + +func setupBillingEntityRBACCron( + ctx context.Context, + crontab string, + mgr ctrl.Manager, +) (*cron.Cron, error) { + + rbac := &controllers.BillingEntityRBACCronJob{Client: mgr.GetClient()} + syncLog := ctrl.Log.WithName("be_rbac_cron") + c := cron.New() + _, err := c.AddFunc(crontab, func() { + err := rbac.Run(ctx) + if err != nil { + syncLog.Error(err, "Error during periodic job") + } + }) + return c, err +} diff --git a/controllers/billingentity_rbac_cronjob.go b/controllers/billingentity_rbac_cronjob.go new file mode 100644 index 00000000..8bbcc550 --- /dev/null +++ b/controllers/billingentity_rbac_cronjob.go @@ -0,0 +1,66 @@ +package controllers + +import ( + "context" + "fmt" + + "go.uber.org/multierr" + "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log" + + billingv1 "github.com/appuio/control-api/apis/billing/v1" + "github.com/appuio/control-api/pkg/billingrbac" +) + +// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles;clusterrolebindings,verbs=get;list;watch;create;delete;patch;update;edit +// +kubebuilder:rbac:groups=rbac.appuio.io;billing.appuio.io,resources=billingentities,verbs=* + +// BillingEntityRBACCronJob periodically checks billing entities and sends notification emails if appropriate +type BillingEntityRBACCronJob struct { + client.Client +} + +func NewBillingEntityRBACCronJob(client client.Client, eventRecorder record.EventRecorder) BillingEntityRBACCronJob { + return BillingEntityRBACCronJob{ + Client: client, + } +} + +// Run lists all BillingEntity resources and sends notification emails if needed. +func (r *BillingEntityRBACCronJob) Run(ctx context.Context) error { + log := log.FromContext(ctx).WithName("BillingEntityRBACCronJob") + log.Info("Reconciling BillingEntity RBAC") + + list := &billingv1.BillingEntityList{} + err := r.Client.List(ctx, list) + if err != nil { + return fmt.Errorf("could not list billing entities: %w", err) + } + + var errors []error + for _, be := range list.Items { + log := log.WithValues("billingentity", be.Name) + err := r.reconcile(ctx, &be) + if err != nil { + log.Error(err, "could not reconcile billing entity") + errors = append(errors, err) + } + } + return multierr.Combine(errors...) +} + +func (r *BillingEntityRBACCronJob) reconcile(ctx context.Context, be *billingv1.BillingEntity) error { + ar, arBinding, vr, vrBinding := billingrbac.ClusterRoles(be.Name, billingrbac.ClusterRolesParams{ + AllowSubjectsToViewRole: true, + }) + + arErr := r.Client.Patch(ctx, ar, client.Apply, client.ForceOwnership, client.FieldOwner("control-api")) + arBinding.Subjects = nil // we don't want to manage the subjects + arBindingErr := r.Client.Patch(ctx, arBinding, client.Apply, client.ForceOwnership, client.FieldOwner("control-api")) + vrErr := r.Client.Patch(ctx, vr, client.Apply, client.ForceOwnership, client.FieldOwner("control-api")) + vrBinding.Subjects = nil // we don't want to manage the subjects + vrBindingErr := r.Client.Patch(ctx, vrBinding, client.Apply, client.ForceOwnership, client.FieldOwner("control-api")) + + return multierr.Combine(arErr, arBindingErr, vrErr, vrBindingErr) +} diff --git a/controllers/billingentity_rbac_cronjob_test.go b/controllers/billingentity_rbac_cronjob_test.go new file mode 100644 index 00000000..273bdf54 --- /dev/null +++ b/controllers/billingentity_rbac_cronjob_test.go @@ -0,0 +1,53 @@ +package controllers_test + +import ( + "context" + "fmt" + "testing" + + "github.com/stretchr/testify/require" + rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/types" + + . "github.com/appuio/control-api/controllers" +) + +func Test_BillingEntityRBACCronJob_Run(t *testing.T) { + ctx := context.Background() + + be := baseBillingEntity() + c := prepareTest(t, be) + + subject := &BillingEntityRBACCronJob{ + Client: c, + } + + require.NoError(t, subject.Run(ctx)) + + var adminRole rbacv1.ClusterRole + var adminRoleBinding rbacv1.ClusterRoleBinding + adminRoleName := fmt.Sprintf("billingentities-%s-admin", be.Name) + require.NoError(t, c.Get(ctx, types.NamespacedName{Name: adminRoleName}, &adminRole), "admin role should be created") + require.NoError(t, c.Get(ctx, types.NamespacedName{Name: adminRoleName}, &adminRoleBinding), "admin role binding should be created") + + var viewerRole rbacv1.ClusterRole + var viewerRoleBinding rbacv1.ClusterRoleBinding + viewerRoleName := fmt.Sprintf("billingentities-%s-viewer", be.Name) + require.NoError(t, c.Get(ctx, types.NamespacedName{Name: viewerRoleName}, &viewerRole), "viewer role should be created") + require.NoError(t, c.Get(ctx, types.NamespacedName{Name: viewerRoleName}, &viewerRoleBinding), "viewer role binding should be created") + + testSubjects := []rbacv1.Subject{ + { + APIGroup: "rbac.authorization.k8s.io", + Kind: "User", + Name: "testuser", + }, + } + viewerRoleBinding.Subjects = testSubjects + require.NoError(t, c.Update(ctx, &viewerRoleBinding)) + + require.NoError(t, subject.Run(ctx)) + + require.NoError(t, c.Get(ctx, types.NamespacedName{Name: viewerRoleName}, &viewerRoleBinding)) + require.Equal(t, testSubjects, viewerRoleBinding.Subjects, "role bindings should not be changed") +} diff --git a/controllers/common_test.go b/controllers/common_test.go index 7a343028..498f3726 100644 --- a/controllers/common_test.go +++ b/controllers/common_test.go @@ -1,8 +1,12 @@ package controllers_test import ( + "context" + "errors" + "fmt" "testing" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" @@ -25,10 +29,39 @@ func prepareTest(t *testing.T, initObjs ...client.Object) client.WithWatch { utilruntime.Must(billingv1.AddToScheme(scheme)) utilruntime.Must(userv1.AddToScheme(scheme)) - return fake.NewClientBuilder(). - WithScheme(scheme). - WithObjects(initObjs...). - Build() + return &fakeSSA{ + fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(initObjs...). + Build(), + } +} + +// fakeSSA is a fake client that approximates SSA. +// See https://github.com/kubernetes-sigs/controller-runtime/issues/2341#issuecomment-1689885336 +// TODO Migrate to builder.WithInterceptorFuncs once controller-runtime is updated to the latest version. +// See https://github.com/kubernetes/kubernetes/issues/115598 for the upstream issue tracking fake SSA support. +type fakeSSA struct { + client.WithWatch +} + +// Patch approximates SSA by creating objects that don't exist yet. +func (f *fakeSSA) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + // Apply patches are supposed to upsert, but fake client fails if the object doesn't exist, + // if an apply patch occurs for an object that doesn't yet exist, create it. + if patch.Type() != types.ApplyPatchType { + return f.WithWatch.Patch(ctx, obj, patch, opts...) + } + check, ok := obj.DeepCopyObject().(client.Object) + if !ok { + return errors.New("could not check for object in fake client") + } + if err := f.WithWatch.Get(ctx, client.ObjectKeyFromObject(obj), check); apierrors.IsNotFound(err) { + if err := f.WithWatch.Create(ctx, check); err != nil { + return fmt.Errorf("could not inject object creation for fake: %w", err) + } + } + return f.WithWatch.Patch(ctx, obj, patch, opts...) } func requestFor(obj client.Object) ctrl.Request { diff --git a/pkg/billingrbac/billingrbac.go b/pkg/billingrbac/billingrbac.go new file mode 100644 index 00000000..0d14e64b --- /dev/null +++ b/pkg/billingrbac/billingrbac.go @@ -0,0 +1,108 @@ +// billingrbac is the common definition of the RBAC rules for billing entities. +package billingrbac + +import ( + "fmt" + + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +type ClusterRolesParams struct { + AdminUsers []string + ViewerUsers []string + + // AllowSubjectsToViewRole controls whether the admin and viewer roles are allowed to view the role itself. + // Workaround for the portal crashing when trying to load the role. + AllowSubjectsToViewRole bool +} + +// ClusterRoles returns the ClusterRoles and ClusterRoleBindings for the given BillingEntity. +func ClusterRoles(beName string, p ClusterRolesParams) (ar *rbacv1.ClusterRole, arBinding *rbacv1.ClusterRoleBinding, vr *rbacv1.ClusterRole, vrBinding *rbacv1.ClusterRoleBinding) { + viewRoleName := fmt.Sprintf("billingentities-%s-viewer", beName) + viewRole := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: viewRoleName, + }, + Rules: []rbacv1.PolicyRule{ + { + APIGroups: []string{"rbac.appuio.io"}, + Resources: []string{"billingentities"}, + Verbs: []string{"get"}, + ResourceNames: []string{beName}, + }, + }, + } + if p.AllowSubjectsToViewRole { + viewRole.Rules = append(viewRole.Rules, rbacv1.PolicyRule{ + APIGroups: []string{"rbac.authorization.k8s.io"}, + Resources: []string{"clusterroles"}, + Verbs: []string{"get"}, + ResourceNames: []string{viewRoleName}, + }) + } + viewRoleBinding := &rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: viewRoleName, + }, + Subjects: userSubjects(p.ViewerUsers), + RoleRef: rbacv1.RoleRef{ + Kind: "ClusterRole", + APIGroup: "rbac.authorization.k8s.io", + Name: viewRoleName, + }, + } + adminRoleName := fmt.Sprintf("billingentities-%s-admin", beName) + adminRole := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: adminRoleName, + }, + Rules: []rbacv1.PolicyRule{ + { + APIGroups: []string{"rbac.appuio.io", "billing.appuio.io"}, + Resources: []string{"billingentities"}, + Verbs: []string{"get", "patch", "update", "edit"}, + ResourceNames: []string{beName}, + }, + { + APIGroups: []string{"rbac.authorization.k8s.io"}, + Resources: []string{"clusterrolebindings"}, + Verbs: []string{"get", "edit", "update", "patch"}, + ResourceNames: []string{viewRoleName, adminRoleName}, + }, + }, + } + if p.AllowSubjectsToViewRole { + adminRole.Rules = append(adminRole.Rules, rbacv1.PolicyRule{ + APIGroups: []string{"rbac.authorization.k8s.io"}, + Resources: []string{"clusterroles"}, + Verbs: []string{"get"}, + ResourceNames: []string{adminRoleName}, + }) + } + adminRoleBinding := &rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: adminRoleName, + }, + Subjects: userSubjects(p.AdminUsers), + RoleRef: rbacv1.RoleRef{ + Kind: "ClusterRole", + APIGroup: "rbac.authorization.k8s.io", + Name: adminRoleName, + }, + } + + return adminRole, adminRoleBinding, viewRole, viewRoleBinding +} + +func userSubjects(users []string) []rbacv1.Subject { + subjects := make([]rbacv1.Subject, 0, len(users)) + for _, u := range users { + subjects = append(subjects, rbacv1.Subject{ + Kind: "User", + APIGroup: "rbac.authorization.k8s.io", + Name: u, + }) + } + return subjects +}