diff --git a/pkg/controller/management/projects/projects.go b/pkg/controller/management/projects/projects.go index 0caa0f7f0c..51cc7c7149 100644 --- a/pkg/controller/management/projects/projects.go +++ b/pkg/controller/management/projects/projects.go @@ -253,11 +253,19 @@ func newReconciler(kubeClient client.Client, cfg ReconcilerConfig) *reconciler { r.ensureControllerPermissionsFn = r.ensureControllerPermissions r.ensureDefaultUserRolesFn = r.ensureDefaultUserRoles r.ensureExtendedPermissionsFn = r.ensureExtendedPermissions - r.createServiceAccountFn = r.client.Create - r.createRoleFn = r.client.Create - r.createRoleBindingFn = r.client.Create - r.createClusterRoleFn = r.client.Create - r.createClusterRoleBindingFn = r.client.Create + applySSA := func(ctx context.Context, obj client.Object, _ ...client.CreateOption) error { + gvks, _, err := r.client.Scheme().ObjectKinds(obj) + if err != nil { + return fmt.Errorf("could not determine GVK for object: %w", err) + } + obj.GetObjectKind().SetGroupVersionKind(gvks[0]) + return r.client.Patch(ctx, obj, client.Apply, client.ForceOwnership, client.FieldOwner("kargo")) + } + r.createServiceAccountFn = applySSA + r.createRoleFn = applySSA + r.createRoleBindingFn = applySSA + r.createClusterRoleFn = applySSA + r.createClusterRoleBindingFn = applySSA r.deleteClusterRoleFn = r.client.Delete r.deleteClusterRoleBindingFn = r.client.Delete return r @@ -686,22 +694,12 @@ func (r *reconciler) ensureSystemPermissions( for _, roleBinding := range roleBindings { rbLogger := logger.WithValues("roleBinding", roleBinding.Name) if err := r.createRoleBindingFn(ctx, &roleBinding); err != nil { - if !apierrors.IsAlreadyExists(err) { - return fmt.Errorf( - "error creating RoleBinding %q in Project namespace %q: %w", - roleBinding.Name, project.Name, err, - ) - } - if err = r.client.Update(ctx, &roleBinding); err != nil { - return fmt.Errorf( - "error updating existing RoleBinding %q in Project namespace %q: %w", - roleBinding.Name, project.Name, err, - ) - } - rbLogger.Debug("updated RoleBinding") - continue + return fmt.Errorf( + "error applying RoleBinding %q in Project namespace %q: %w", + roleBinding.Name, project.Name, err, + ) } - rbLogger.Debug("created RoleBinding in Project namespace") + rbLogger.Debug("applied RoleBinding in Project namespace") } return nil @@ -766,23 +764,13 @@ func (r *reconciler) ensureControllerPermissions( }, } - if err := r.client.Create(ctx, roleBinding); err != nil { - if !apierrors.IsAlreadyExists(err) { - return fmt.Errorf( - "error creating RoleBinding %q for ServiceAccount %q in Project namespace %q: %w", - roleBinding.Name, sa.Name, project.Name, err, - ) - } - if err = r.client.Update(ctx, roleBinding); err != nil { - return fmt.Errorf( - "error updating existing RoleBinding %q in Project namespace %q: %w", - roleBinding.Name, project.Name, err, - ) - } - saLogger.Debug("updated RoleBinding") - continue + if err := r.createRoleBindingFn(ctx, roleBinding); err != nil { + return fmt.Errorf( + "error applying RoleBinding %q for ServiceAccount %q in Project namespace %q: %w", + roleBinding.Name, sa.Name, project.Name, err, + ) } - saLogger.Debug("created RoleBinding") + saLogger.Debug("applied RoleBinding") } return nil @@ -821,17 +809,14 @@ func (r *reconciler) ensureDefaultUserRoles( }, }, ); err != nil { - if apierrors.IsAlreadyExists(err) { - saLogger.Debug("ServiceAccount already exists in project namespace") - continue - } return fmt.Errorf( - "error creating ServiceAccount %q in project namespace %q: %w", + "error applying ServiceAccount %q in project namespace %q: %w", saName, project.Name, err, ) } + saLogger.Debug("applied ServiceAccount in project namespace") } roles := []*rbacv1.Role{ @@ -976,16 +961,12 @@ func (r *reconciler) ensureDefaultUserRoles( "namespace", project.Name, ) if err := r.createRoleFn(ctx, role); err != nil { - if apierrors.IsAlreadyExists(err) { - roleLogger.Debug("Role already exists in project namespace") - continue - } return fmt.Errorf( - "error creating Role %q in project namespace %q: %w", + "error applying Role %q in project namespace %q: %w", role.Name, project.Name, err, ) } - roleLogger.Debug("created Role in project namespace") + roleLogger.Debug("applied Role in project namespace") } for _, rbName := range allRoles { @@ -1017,16 +998,12 @@ func (r *reconciler) ensureDefaultUserRoles( }, }, ); err != nil { - if apierrors.IsAlreadyExists(err) { - rbLogger.Debug("RoleBinding already exists in project namespace") - continue - } return fmt.Errorf( - "error creating RoleBinding %q in project namespace %q: %w", + "error applying RoleBinding %q in project namespace %q: %w", rbName, project.Name, err, ) } - rbLogger.Debug("created RoleBinding in project namespace") + rbLogger.Debug("applied RoleBinding in project namespace") } // This ClusterRole allows those bound to it to update and delete one specific @@ -1048,16 +1025,12 @@ func (r *reconciler) ensureDefaultUserRoles( }}, }, ); err != nil { - if !apierrors.IsAlreadyExists(err) { - return fmt.Errorf("error creating ClusterRole %q: %w", crName, err) - } - crLogger.Debug("ClusterRole already exists") + return fmt.Errorf("error applying ClusterRole %q: %w", crName, err) } - crLogger.Debug("created ClusterRole") + crLogger.Debug("applied ClusterRole") crbName := crName crbLogger := logger.WithValues("name", crbName) - logger.WithValues() if err := r.createClusterRoleBindingFn( ctx, &rbacv1.ClusterRoleBinding{ @@ -1074,12 +1047,9 @@ func (r *reconciler) ensureDefaultUserRoles( }}, }, ); err != nil { - if !apierrors.IsAlreadyExists(err) { - return fmt.Errorf("error creating ClusterRoleBinding %q: %w", crName, err) - } - crbLogger.Debug("ClusterRoleBinding already exists") + return fmt.Errorf("error applying ClusterRoleBinding %q: %w", crName, err) } - crbLogger.Debug("created ClusterRoleBinding") + crbLogger.Debug("applied ClusterRoleBinding") return nil } @@ -1138,17 +1108,14 @@ func (r *reconciler) ensureExtendedPermissions( }, }, ); err != nil { - if apierrors.IsAlreadyExists(err) { - saLogger.Debug("ServiceAccount already exists in project namespace") - continue - } return fmt.Errorf( - "error creating ServiceAccount %q in project namespace %q: %w", + "error applying ServiceAccount %q in project namespace %q: %w", saName, project.Name, err, ) } + saLogger.Debug("applied ServiceAccount in project namespace") } var clusterRoleBindings []*rbacv1.ClusterRoleBinding @@ -1181,16 +1148,12 @@ func (r *reconciler) ensureExtendedPermissions( for _, crb := range clusterRoleBindings { crbLogger := logger.WithValues("name", crb.Name, "project", project.Name) if err := r.createClusterRoleBindingFn(ctx, crb); err != nil { - if apierrors.IsAlreadyExists(err) { - crbLogger.Debug("ClusterRoleBinding already exists for Project") - continue - } - return fmt.Errorf( - "error creating ClusterRoleBinding %q for Project %q: %w", + "error applying ClusterRoleBinding %q for Project %q: %w", crb.Name, project.Name, err, ) } + crbLogger.Debug("applied ClusterRoleBinding for Project") } rbAnnotations := map[string]string{ @@ -1354,15 +1317,12 @@ func (r *reconciler) ensureExtendedPermissions( ) if err := r.createRoleBindingFn(ctx, rb); err != nil { - if apierrors.IsAlreadyExists(err) { - rbLogger.Debug("RoleBinding already exists in project namespace") - continue - } return fmt.Errorf( - "error creating RoleBinding %q in project namespace %q: %w", + "error applying RoleBinding %q in project namespace %q: %w", rb.Name, project.Name, err, ) } + rbLogger.Debug("applied RoleBinding in project namespace") } return nil diff --git a/pkg/controller/management/projects/projects_test.go b/pkg/controller/management/projects/projects_test.go index 1e6a1a98e0..df693f9300 100644 --- a/pkg/controller/management/projects/projects_test.go +++ b/pkg/controller/management/projects/projects_test.go @@ -1341,7 +1341,7 @@ func TestReconciler_ensureSystemPermissions(t *testing.T) { assertions func(*testing.T, error) }{ { - name: "error creating role binding", + name: "error applying role binding", reconciler: &reconciler{ createRoleBindingFn: func( context.Context, @@ -1352,61 +1352,10 @@ func TestReconciler_ensureSystemPermissions(t *testing.T) { }, }, assertions: func(t *testing.T, err error) { - require.ErrorContains(t, err, "error creating RoleBinding") + require.ErrorContains(t, err, "error applying RoleBinding") require.ErrorContains(t, err, "something went wrong") }, }, - { - name: "error updating existing role binding", - reconciler: &reconciler{ - client: fake.NewClientBuilder().WithInterceptorFuncs(interceptor.Funcs{ - Update: func( - context.Context, - client.WithWatch, - client.Object, - ...client.UpdateOption, - ) error { - return errors.New("something went wrong") - }, - }).Build(), - createRoleBindingFn: func( - context.Context, - client.Object, - ...client.CreateOption, - ) error { - return apierrors.NewAlreadyExists(schema.GroupResource{}, "") - }, - }, - assertions: func(t *testing.T, err error) { - require.ErrorContains(t, err, "error updating existing RoleBinding") - require.ErrorContains(t, err, "something went wrong") - }, - }, - { - name: "success updating existing role binding", - reconciler: &reconciler{ - client: fake.NewClientBuilder().WithInterceptorFuncs(interceptor.Funcs{ - Update: func( - context.Context, - client.WithWatch, - client.Object, - ...client.UpdateOption, - ) error { - return nil - }, - }).Build(), - createRoleBindingFn: func( - context.Context, - client.Object, - ...client.CreateOption, - ) error { - return apierrors.NewAlreadyExists(schema.GroupResource{}, "") - }, - }, - assertions: func(t *testing.T, err error) { - require.NoError(t, err) - }, - }, { name: "success creating role binding", reconciler: &reconciler{ @@ -1548,22 +1497,23 @@ func TestReconciler_ensureControllerPermissions(t *testing.T) { }, }, { - name: "error creating RoleBinding", + name: "error applying RoleBinding", client: fake.NewClientBuilder(). WithScheme(scheme). WithObjects(testControllerSA). WithInterceptorFuncs(interceptor.Funcs{ - Create: func( + Patch: func( context.Context, client.WithWatch, client.Object, - ...client.CreateOption, + client.Patch, + ...client.PatchOption, ) error { return fmt.Errorf("something went wrong") }, }).Build(), assertions: func(t *testing.T, _ client.Client, err error) { - require.ErrorContains(t, err, "error creating RoleBinding") + require.ErrorContains(t, err, "error applying RoleBinding") require.ErrorContains(t, err, "something went wrong") }, }, @@ -1652,34 +1602,6 @@ func TestReconciler_ensureControllerPermissions(t *testing.T) { ) }, }, - { - name: "error updating existing RoleBinding", - client: fake.NewClientBuilder(). - WithScheme(scheme). - WithObjects( - testControllerSA, - &rbacv1.RoleBinding{ - ObjectMeta: metav1.ObjectMeta{ - Name: getRoleBindingName(testControllerSA.Name), - Namespace: testProject.Name, - }, - }, - ). - WithInterceptorFuncs(interceptor.Funcs{ - Update: func( - context.Context, - client.WithWatch, - client.Object, - ...client.UpdateOption, - ) error { - return fmt.Errorf("something went wrong") - }, - }).Build(), - assertions: func(t *testing.T, _ client.Client, err error) { - require.ErrorContains(t, err, "error updating existing RoleBinding") - require.ErrorContains(t, err, "something went wrong") - }, - }, } for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { @@ -1697,7 +1619,7 @@ func TestReconciler_ensureDefaultUserRoles(t *testing.T) { assertions func(*testing.T, error) }{ { - name: "error creating ServiceAccount", + name: "error applying ServiceAccount", reconciler: &reconciler{ createServiceAccountFn: func( context.Context, @@ -1708,19 +1630,19 @@ func TestReconciler_ensureDefaultUserRoles(t *testing.T) { }, }, assertions: func(t *testing.T, err error) { - require.ErrorContains(t, err, "error creating ServiceAccount") + require.ErrorContains(t, err, "error applying ServiceAccount") require.ErrorContains(t, err, "something went wrong") }, }, { - name: "error creating Role", + name: "error applying Role", reconciler: &reconciler{ createServiceAccountFn: func( context.Context, client.Object, ...client.CreateOption, ) error { - return apierrors.NewAlreadyExists(schema.GroupResource{}, "") + return nil }, createRoleFn: func( context.Context, @@ -1731,26 +1653,26 @@ func TestReconciler_ensureDefaultUserRoles(t *testing.T) { }, }, assertions: func(t *testing.T, err error) { - require.ErrorContains(t, err, "error creating Role") + require.ErrorContains(t, err, "error applying Role") require.ErrorContains(t, err, "something went wrong") }, }, { - name: "error creating RoleBinding", + name: "error applying RoleBinding", reconciler: &reconciler{ createServiceAccountFn: func( context.Context, client.Object, ...client.CreateOption, ) error { - return apierrors.NewAlreadyExists(schema.GroupResource{}, "") + return nil }, createRoleFn: func( context.Context, client.Object, ...client.CreateOption, ) error { - return apierrors.NewAlreadyExists(schema.GroupResource{}, "") + return nil }, createRoleBindingFn: func( context.Context, @@ -1761,7 +1683,7 @@ func TestReconciler_ensureDefaultUserRoles(t *testing.T) { }, }, assertions: func(t *testing.T, err error) { - require.ErrorContains(t, err, "error creating RoleBinding") + require.ErrorContains(t, err, "error applying RoleBinding") require.ErrorContains(t, err, "something went wrong") }, }, @@ -1798,12 +1720,12 @@ func TestReconciler_ensureDefaultUserRoles(t *testing.T) { }, }, assertions: func(t *testing.T, err error) { - require.ErrorContains(t, err, "error creating ClusterRole") + require.ErrorContains(t, err, "error applying ClusterRole") require.ErrorContains(t, err, "something went wrong") }, }, { - name: "error creating ClusterRoleBinding", + name: "error applying ClusterRoleBinding", reconciler: &reconciler{ createServiceAccountFn: func( context.Context, @@ -1842,7 +1764,7 @@ func TestReconciler_ensureDefaultUserRoles(t *testing.T) { }, }, assertions: func(t *testing.T, err error) { - require.ErrorContains(t, err, "error creating ClusterRoleBinding") + require.ErrorContains(t, err, "error applying ClusterRoleBinding") require.ErrorContains(t, err, "something went wrong") }, }, @@ -1934,23 +1856,24 @@ func TestReconciler_ensureExtendedPermissions(t *testing.T) { assertions func(*testing.T, client.Client, error) }{ { - name: "error creating ServiceAccount", + name: "error applying ServiceAccount", cfg: ReconcilerConfig{ ControlPlaneServiceAccountName: "test-control-plane", }, client: fake.NewClientBuilder().WithScheme(scheme). WithInterceptorFuncs(interceptor.Funcs{ - Create: func( + Patch: func( context.Context, client.WithWatch, client.Object, - ...client.CreateOption, + client.Patch, + ...client.PatchOption, ) error { return fmt.Errorf("something went wrong") }, }).Build(), assertions: func(t *testing.T, _ client.Client, err error) { - require.ErrorContains(t, err, "error creating ServiceAccount") + require.ErrorContains(t, err, "error applying ServiceAccount") require.ErrorContains(t, err, "something went wrong") }, }, @@ -2260,7 +2183,7 @@ func TestReconciler_ensureExtendedPermissions(t *testing.T) { }, }, { - name: "error creating RoleBinding", + name: "error applying RoleBinding", cfg: ReconcilerConfig{ ControlPlaneServiceAccountName: "test-control-plane", ControlPlaneClusterRoleName: "test-control-plane-role", @@ -2268,11 +2191,12 @@ func TestReconciler_ensureExtendedPermissions(t *testing.T) { client: fake.NewClientBuilder(). WithScheme(scheme). WithInterceptorFuncs(interceptor.Funcs{ - Create: func( + Patch: func( _ context.Context, _ client.WithWatch, obj client.Object, - _ ...client.CreateOption, + _ client.Patch, + _ ...client.PatchOption, ) error { if _, ok := obj.(*rbacv1.RoleBinding); ok { return fmt.Errorf("something went wrong") @@ -2281,12 +2205,12 @@ func TestReconciler_ensureExtendedPermissions(t *testing.T) { }, }).Build(), assertions: func(t *testing.T, _ client.Client, err error) { - require.ErrorContains(t, err, "error creating RoleBinding") + require.ErrorContains(t, err, "error applying RoleBinding") require.ErrorContains(t, err, "something went wrong") }, }, { - name: "error creating ClusterRoleBinding", + name: "error applying ClusterRoleBinding", cfg: ReconcilerConfig{ ArgoCDServiceAccountName: "kargo-argocd-service-account", ArgoCDClusterRoleName: "kargo-argocd-role", @@ -2294,11 +2218,12 @@ func TestReconciler_ensureExtendedPermissions(t *testing.T) { client: fake.NewClientBuilder(). WithScheme(scheme). WithInterceptorFuncs(interceptor.Funcs{ - Create: func( + Patch: func( _ context.Context, _ client.WithWatch, obj client.Object, - _ ...client.CreateOption, + _ client.Patch, + _ ...client.PatchOption, ) error { if _, ok := obj.(*rbacv1.ClusterRoleBinding); ok { return fmt.Errorf("something went wrong") @@ -2307,7 +2232,7 @@ func TestReconciler_ensureExtendedPermissions(t *testing.T) { }, }).Build(), assertions: func(t *testing.T, _ client.Client, err error) { - require.ErrorContains(t, err, "error creating ClusterRoleBinding") + require.ErrorContains(t, err, "error applying ClusterRoleBinding") require.ErrorContains(t, err, "something went wrong") }, }, diff --git a/pkg/controller/management/serviceaccounts/serviceaccounts.go b/pkg/controller/management/serviceaccounts/serviceaccounts.go index 6e54e46f4d..bbbc0c8fb8 100644 --- a/pkg/controller/management/serviceaccounts/serviceaccounts.go +++ b/pkg/controller/management/serviceaccounts/serviceaccounts.go @@ -185,23 +185,21 @@ func (r *reconciler) ensureControllerPermissions(ctx context.Context, sa types.N }, }, } - if err := r.client.Create(ctx, roleBinding); err != nil { - if !apierrors.IsAlreadyExists(err) { - return fmt.Errorf( - "error creating RoleBinding %q for ServiceAccount %q in Project namespace %q: %w", - roleBinding.Name, sa.Name, project.Name, err, - ) - } - if err = r.client.Update(ctx, roleBinding); err != nil { - return fmt.Errorf( - "error updating existing RoleBinding %q in Project namespace %q: %w", - roleBinding.Name, project.Name, err, - ) - } - projectLogger.Debug("updated existing RoleBinding") - } else { - projectLogger.Debug("created RoleBinding") + gvks, _, err := r.client.Scheme().ObjectKinds(roleBinding) + if err != nil { + return fmt.Errorf("could not determine GVK for RoleBinding: %w", err) + } + roleBinding.GetObjectKind().SetGroupVersionKind(gvks[0]) + if err := r.client.Patch( + ctx, roleBinding, client.Apply, + client.ForceOwnership, client.FieldOwner("kargo"), + ); err != nil { + return fmt.Errorf( + "error applying RoleBinding %q for ServiceAccount %q in Project namespace %q: %w", + roleBinding.Name, sa.Name, project.Name, err, + ) } + projectLogger.Debug("applied RoleBinding") } logger.Debug("necessary RoleBindings exist in all Project namespaces") return nil diff --git a/pkg/controller/management/serviceaccounts/serviceaccounts_test.go b/pkg/controller/management/serviceaccounts/serviceaccounts_test.go index e6703f6f23..664f4836f8 100644 --- a/pkg/controller/management/serviceaccounts/serviceaccounts_test.go +++ b/pkg/controller/management/serviceaccounts/serviceaccounts_test.go @@ -362,22 +362,23 @@ func TestEnsureControllerPermissions(t *testing.T) { }, }, { - name: "error creating RoleBinding", + name: "error applying RoleBinding", client: fake.NewClientBuilder(). WithScheme(scheme). WithObjects(testProject). WithInterceptorFuncs(interceptor.Funcs{ - Create: func( + Patch: func( context.Context, client.WithWatch, client.Object, - ...client.CreateOption, + client.Patch, + ...client.PatchOption, ) error { return fmt.Errorf("something went wrong") }, }).Build(), assertions: func(t *testing.T, _ client.Client, err error) { - require.ErrorContains(t, err, "error creating RoleBinding") + require.ErrorContains(t, err, "error applying RoleBinding") require.ErrorContains(t, err, "something went wrong") }, }, @@ -466,34 +467,6 @@ func TestEnsureControllerPermissions(t *testing.T) { ) }, }, - { - name: "error updating existing RoleBinding", - client: fake.NewClientBuilder(). - WithScheme(scheme). - WithObjects( - testProject, - &rbacv1.RoleBinding{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: testProject.Name, - Name: getRoleBindingName(testControllerSARef.Name), - }, - }, - ). - WithInterceptorFuncs(interceptor.Funcs{ - Update: func( - context.Context, - client.WithWatch, - client.Object, - ...client.UpdateOption, - ) error { - return fmt.Errorf("something went wrong") - }, - }).Build(), - assertions: func(t *testing.T, _ client.Client, err error) { - require.ErrorContains(t, err, "error updating existing RoleBinding") - require.ErrorContains(t, err, "something went wrong") - }, - }, } for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) {