Skip to content

Commit

Permalink
chore: cleanup reconcilation of roles, role bindings, service accounts (
Browse files Browse the repository at this point in the history 
#605)

* chore: cleanup reconcilation of roles, role bindings, service accounts

Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

* Rebase master
  • Loading branch information
@ishitasequeira
ishitasequeira committed Apr 21, 2022
1 parent 70e3eca commit 68e1197
Show file tree
Hide file tree
Showing 8 changed files with 120 additions and 96 deletions.
3 changes: 2 additions & 1 deletion controllers/argocd/hooks_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
v1 "k8s.io/api/rbac/v1"

argoprojv1alpha1 "github.com/argoproj-labs/argocd-operator/api/v1alpha1"
"github.com/argoproj-labs/argocd-operator/common"
)

var errMsg = errors.New("this is a test error")
Expand All @@ -33,7 +34,7 @@ func testClusterRoleHook(cr *argoprojv1alpha1.ArgoCD, v interface{}, s string) e
func testRoleHook(cr *argoprojv1alpha1.ArgoCD, v interface{}, s string) error {
switch o := v.(type) {
case *v1.Role:
if o.Name == cr.Name+"-"+applicationController {
if o.Name == cr.Name+"-"+common.ArgoCDApplicationControllerComponent {
o.Rules = append(o.Rules, testRules()...)
}
}
Expand Down
44 changes: 44 additions & 0 deletions controllers/argocd/policyrule.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package argocd

import (
v1 "k8s.io/api/rbac/v1"

"github.com/argoproj-labs/argocd-operator/common"
)

func policyRuleForApplicationController() []v1.PolicyRule {
Expand Down Expand Up @@ -173,3 +175,45 @@ func policyRuleForServerClusterRole() []v1.PolicyRule {
},
}
}

func getPolicyRuleList() []struct {
name string
policyRule []v1.PolicyRule
} {
return []struct {
name string
policyRule []v1.PolicyRule
}{
{
name: common.ArgoCDApplicationControllerComponent,
policyRule: policyRuleForApplicationController(),
}, {
name: common.ArgoCDDexServerComponent,
policyRule: policyRuleForDexServer(),
}, {
name: common.ArgoCDServerComponent,
policyRule: policyRuleForServer(),
}, {
name: common.ArgoCDRedisHAComponent,
policyRule: policyRuleForRedisHa(),
},
}
}

func getPolicyRuleClusterRoleList() []struct {
name string
policyRule []v1.PolicyRule
} {
return []struct {
name string
policyRule []v1.PolicyRule
}{
{
name: common.ArgoCDApplicationControllerComponent,
policyRule: policyRuleForApplicationController(),
}, {
name: common.ArgoCDServerComponent,
policyRule: policyRuleForServerClusterRole(),
},
}
}
68 changes: 29 additions & 39 deletions controllers/argocd/role.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"os"
"reflect"

v1 "k8s.io/api/rbac/v1"
"k8s.io/apimachinery/pkg/api/errors"
Expand All @@ -13,16 +14,10 @@ import (
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"

argoprojv1a1 "github.com/argoproj-labs/argocd-operator/api/v1alpha1"
"github.com/argoproj-labs/argocd-operator/common"
"github.com/argoproj-labs/argocd-operator/controllers/argoutil"
)

const (
applicationController = "argocd-application-controller"
server = "argocd-server"
redisHa = "argocd-redis-ha"
dexServer = "argocd-dex-server"
)

// newRole returns a new Role instance.
func newRole(name string, rules []v1.PolicyRule, cr *argoprojv1a1.ArgoCD) *v1.Role {
return &v1.Role{
Expand Down Expand Up @@ -57,28 +52,20 @@ func newClusterRole(name string, rules []v1.PolicyRule, cr *argoprojv1a1.ArgoCD)

// reconcileRoles will ensure that all ArgoCD Service Accounts are configured.
func (r *ReconcileArgoCD) reconcileRoles(cr *argoprojv1a1.ArgoCD) error {
if _, err := r.reconcileRole(applicationController, policyRuleForApplicationController(), cr); err != nil {
return err
}

if _, err := r.reconcileRole(dexServer, policyRuleForDexServer(), cr); err != nil {
return err
}
params := getPolicyRuleList()

if _, err := r.reconcileRole(server, policyRuleForServer(), cr); err != nil {
return err
}

if _, err := r.reconcileRole(redisHa, policyRuleForRedisHa(), cr); err != nil {
return err
for _, param := range params {
if _, err := r.reconcileRole(param.name, param.policyRule, cr); err != nil {
return err
}
}

if _, err := r.reconcileClusterRole(applicationController, policyRuleForApplicationController(), cr); err != nil {
return err
}
clusterParams := getPolicyRuleClusterRoleList()

if _, err := r.reconcileClusterRole(server, policyRuleForServerClusterRole(), cr); err != nil {
return err
for _, clusterParam := range clusterParams {
if _, err := r.reconcileRole(clusterParam.name, clusterParam.policyRule, cr); err != nil {
return err
}
}

return nil
Expand All @@ -92,7 +79,7 @@ func (r *ReconcileArgoCD) reconcileRole(name string, policyRules []v1.PolicyRule
// create policy rules for each namespace
for _, namespace := range r.ManagedNamespaces.Items {
// only create dexServer and redisHa roles for the namespace where the argocd instance is deployed
if cr.ObjectMeta.Namespace != namespace.Name && (name == dexServer || name == redisHa) {
if cr.ObjectMeta.Namespace != namespace.Name && (name == common.ArgoCDDexServerComponent || name == common.ArgoCDRedisHAComponent) {
break
}
customRole := getCustomRoleName(name)
Expand All @@ -111,7 +98,7 @@ func (r *ReconcileArgoCD) reconcileRole(name string, policyRules []v1.PolicyRule
continue // skip creating default role if custom cluster role is provided
}
roles = append(roles, role)
if name == dexServer && isDexDisabled() {
if name == common.ArgoCDDexServerComponent && isDexDisabled() {
continue // Dex is disabled, do nothing
}

Expand All @@ -127,24 +114,21 @@ func (r *ReconcileArgoCD) reconcileRole(name string, policyRules []v1.PolicyRule
continue
}

if customRole != "" {
// Delete the existing default role if custom role is specified
// Delete the existing default role if custom role is specified
// or if there is an existing Role created for Dex
if customRole != "" || (name == common.ArgoCDDexServerComponent && isDexDisabled()) {
if err := r.Client.Delete(context.TODO(), &existingRole); err != nil {
return nil, err
}
continue
}

if name == dexServer && isDexDisabled() {
// Delete any existing Role created for Dex
if err := r.Client.Delete(context.TODO(), &existingRole); err != nil {
// if the Rules differ, update the Role
if !reflect.DeepEqual(existingRole.Rules, role.Rules) {
existingRole.Rules = role.Rules
if err := r.Client.Update(context.TODO(), &existingRole); err != nil {
return nil, err
}
continue
}
existingRole.Rules = role.Rules
if err := r.Client.Update(context.TODO(), &existingRole); err != nil {
return nil, err
}
roles = append(roles, &existingRole)
}
Expand Down Expand Up @@ -178,8 +162,14 @@ func (r *ReconcileArgoCD) reconcileClusterRole(name string, policyRules []v1.Pol
return nil, r.Client.Delete(context.TODO(), existingClusterRole)
}

existingClusterRole.Rules = clusterRole.Rules
return existingClusterRole, r.Client.Update(context.TODO(), existingClusterRole)
// if the Rules differ, update the Role
if !reflect.DeepEqual(existingClusterRole.Rules, clusterRole.Rules) {
existingClusterRole.Rules = clusterRole.Rules
if err := r.Client.Update(context.TODO(), existingClusterRole); err != nil {
return nil, err
}
}
return existingClusterRole, nil
}

func deleteClusterRoles(c client.Client, clusterRoleList *v1.ClusterRoleList) error {
Expand Down
12 changes: 6 additions & 6 deletions controllers/argocd/role_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,15 @@ func TestReconcileArgoCD_reconcileRole_for_new_namespace(t *testing.T) {
// only 1 role for the Argo CD instance namespace will be created
expectedNumberOfRoles := 1
// check no dexServer role is created for the new namespace with managed-by label
workloadIdentifier := dexServer
workloadIdentifier := common.ArgoCDDexServerComponent
expectedRoleNamespace := a.Namespace
expectedDexServerRules := policyRuleForDexServer()
dexRoles, err := r.reconcileRole(workloadIdentifier, expectedDexServerRules, a)
assert.NoError(t, err)
assert.Equal(t, expectedNumberOfRoles, len(dexRoles))
assert.Equal(t, expectedRoleNamespace, dexRoles[0].ObjectMeta.Namespace)
// check no redisHa role is created for the new namespace with managed-by label
workloadIdentifier = redisHa
workloadIdentifier = common.ArgoCDRedisHAComponent
expectedRedisHaRules := policyRuleForRedisHa()
redisHaRoles, err := r.reconcileRole(workloadIdentifier, expectedRedisHaRules, a)
assert.NoError(t, err)
Expand All @@ -80,10 +80,10 @@ func TestReconcileArgoCD_reconcileRole_dex_disabled(t *testing.T) {
assert.NoError(t, createNamespace(r, a.Namespace, ""))

rules := policyRuleForDexServer()
role := newRole(dexServer, rules, a)
role := newRole(common.ArgoCDDexServerComponent, rules, a)

// Dex is enabled
_, err := r.reconcileRole(dexServer, rules, a)
_, err := r.reconcileRole(common.ArgoCDDexServerComponent, rules, a)
assert.NoError(t, err)
assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: role.Name, Namespace: a.Namespace}, role))
assert.Equal(t, rules, role.Rules)
Expand All @@ -92,7 +92,7 @@ func TestReconcileArgoCD_reconcileRole_dex_disabled(t *testing.T) {
os.Setenv("DISABLE_DEX", "true")
defer os.Unsetenv("DISABLE_DEX")

_, err = r.reconcileRole(dexServer, rules, a)
_, err = r.reconcileRole(common.ArgoCDDexServerComponent, rules, a)
assert.NoError(t, err)
//assert.ErrorContains(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: role.Name, Namespace: a.Namespace}, role), "not found")
//TODO: https://github.com/stretchr/testify/pull/1022 introduced ErrorContains, but is not yet available in a tagged release. Revert to ErrorContains once this becomes available
Expand Down Expand Up @@ -153,7 +153,7 @@ func TestReconcileArgoCD_RoleHooks(t *testing.T) {
assert.NoError(t, createNamespace(r, a.Namespace, ""))
Register(testRoleHook)

roles, err := r.reconcileRole(applicationController, []v1.PolicyRule{}, a)
roles, err := r.reconcileRole(common.ArgoCDApplicationControllerComponent, []v1.PolicyRule{}, a)
role := roles[0]
assert.NoError(t, err)
assert.Equal(t, role.Rules, testRules())
Expand Down
36 changes: 17 additions & 19 deletions controllers/argocd/rolebinding.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,20 +68,15 @@ func newRoleBindingWithname(name string, cr *argoprojv1a1.ArgoCD) *v1.RoleBindin

// reconcileRoleBindings will ensure that all ArgoCD RoleBindings are configured.
func (r *ReconcileArgoCD) reconcileRoleBindings(cr *argoprojv1a1.ArgoCD) error {
if err := r.reconcileRoleBinding(applicationController, policyRuleForApplicationController(), cr); err != nil {
return fmt.Errorf("error reconciling roleBinding for %q: %w", applicationController, err)
}
if err := r.reconcileRoleBinding(dexServer, policyRuleForDexServer(), cr); err != nil {
return fmt.Errorf("error reconciling roleBinding for %q: %w", dexServer, err)
}

if err := r.reconcileRoleBinding(redisHa, policyRuleForRedisHa(), cr); err != nil {
return fmt.Errorf("error reconciling roleBinding for %q: %w", redisHa, err)
}
params := getPolicyRuleList()

if err := r.reconcileRoleBinding(server, policyRuleForServer(), cr); err != nil {
return fmt.Errorf("error reconciling roleBinding for %q: %w", server, err)
for _, param := range params {
if err := r.reconcileRoleBinding(param.name, param.policyRule, cr); err != nil {
return fmt.Errorf("error reconciling roleBinding for %q: %w", param.name, err)
}
}

return nil
}

Expand All @@ -101,7 +96,7 @@ func (r *ReconcileArgoCD) reconcileRoleBinding(name string, rules []v1.PolicyRul

for _, namespace := range r.ManagedNamespaces.Items {
// only create dexServer and redisHa rolebindings for the namespace where the argocd instance is deployed
if cr.ObjectMeta.Namespace != namespace.Name && (name == dexServer || name == redisHa) {
if cr.ObjectMeta.Namespace != namespace.Name && (name == common.ArgoCDDexServerComponent || name == common.ArgoCDRedisHAComponent) {
break
}
// get expected name
Expand All @@ -116,7 +111,7 @@ func (r *ReconcileArgoCD) reconcileRoleBinding(name string, rules []v1.PolicyRul
if !errors.IsNotFound(err) {
return fmt.Errorf("failed to get the rolebinding associated with %s : %s", name, err)
}
if name == dexServer && isDexDisabled() {
if name == common.ArgoCDDexServerComponent && isDexDisabled() {
continue // Dex is disabled, do nothing
}
roleBindingExists = false
Expand Down Expand Up @@ -146,7 +141,7 @@ func (r *ReconcileArgoCD) reconcileRoleBinding(name string, rules []v1.PolicyRul
}

if roleBindingExists {
if name == dexServer && isDexDisabled() {
if name == common.ArgoCDDexServerComponent && isDexDisabled() {
// Delete any existing RoleBinding created for Dex
if err = r.Client.Delete(context.TODO(), existingRoleBinding); err != nil {
return err
Expand All @@ -160,9 +155,12 @@ func (r *ReconcileArgoCD) reconcileRoleBinding(name string, rules []v1.PolicyRul
return err
}
} else {
existingRoleBinding.Subjects = roleBinding.Subjects
if err = r.Client.Update(context.TODO(), existingRoleBinding); err != nil {
return err
// if the Subjects differ, update the role bindings
if !reflect.DeepEqual(roleBinding.Subjects, existingRoleBinding.Subjects) {
existingRoleBinding.Subjects = roleBinding.Subjects
if err = r.Client.Update(context.TODO(), existingRoleBinding); err != nil {
return err
}
}
continue
}
Expand All @@ -182,10 +180,10 @@ func (r *ReconcileArgoCD) reconcileRoleBinding(name string, rules []v1.PolicyRul
}

func getCustomRoleName(name string) string {
if name == applicationController {
if name == common.ArgoCDApplicationControllerComponent {
return os.Getenv(common.ArgoCDControllerClusterRoleEnvName)
}
if name == server {
if name == common.ArgoCDServerComponent {
return os.Getenv(common.ArgoCDServerClusterRoleEnvName)
}
return ""
Expand Down
14 changes: 7 additions & 7 deletions controllers/argocd/rolebinding_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,14 @@ func TestReconcileArgoCD_reconcileRoleBinding_for_new_namespace(t *testing.T) {

// check no dexServer rolebinding is created for the new namespace with managed-by label
roleBinding := &rbacv1.RoleBinding{}
workloadIdentifier := dexServer
workloadIdentifier := common.ArgoCDDexServerComponent
expectedDexServerRules := policyRuleForDexServer()
expectedName := fmt.Sprintf("%s-%s", a.Name, workloadIdentifier)
assert.NoError(t, r.reconcileRoleBinding(workloadIdentifier, expectedDexServerRules, a))
assert.Error(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: expectedName, Namespace: "newTestNamespace"}, roleBinding))

// check no redisHa rolebinding is created for the new namespace with managed-by label
workloadIdentifier = redisHa
workloadIdentifier = common.ArgoCDRedisHAComponent
expectedRedisHaRules := policyRuleForRedisHa()
assert.NoError(t, r.reconcileRoleBinding(workloadIdentifier, expectedRedisHaRules, a))
assert.Error(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: expectedName, Namespace: "newTestNamespace"}, roleBinding))
Expand All @@ -74,19 +74,19 @@ func TestReconcileArgoCD_reconcileRoleBinding_dex_disabled(t *testing.T) {
assert.NoError(t, createNamespace(r, a.Namespace, ""))

rules := policyRuleForDexServer()
rb := newRoleBindingWithname(dexServer, a)
rb := newRoleBindingWithname(common.ArgoCDDexServerComponent, a)

// Dex is enabled, creates a role binding
assert.NoError(t, r.reconcileRoleBinding(dexServer, rules, a))
assert.NoError(t, r.reconcileRoleBinding(common.ArgoCDDexServerComponent, rules, a))
assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: rb.Name, Namespace: a.Namespace}, rb))

// Disable Dex, deletes the existing role binding
os.Setenv("DISABLE_DEX", "true")
defer os.Unsetenv("DISABLE_DEX")

_, err := r.reconcileRole(dexServer, rules, a)
_, err := r.reconcileRole(common.ArgoCDDexServerComponent, rules, a)
assert.NoError(t, err)
assert.NoError(t, r.reconcileRoleBinding(dexServer, rules, a))
assert.NoError(t, r.reconcileRoleBinding(common.ArgoCDDexServerComponent, rules, a))
//assert.ErrorContains(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: rb.Name, Namespace: a.Namespace}, rb), "not found")
//TODO: https://github.com/stretchr/testify/pull/1022 introduced ErrorContains, but is not yet available in a tagged release. Revert to ErrorContains once this becomes available
assert.Error(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: rb.Name, Namespace: a.Namespace}, rb))
Expand Down Expand Up @@ -156,7 +156,7 @@ func TestReconcileArgoCD_reconcileRoleBinding_custom_role(t *testing.T) {

assert.NoError(t, os.Setenv(common.ArgoCDControllerClusterRoleEnvName, "custom-controller-role"))
defer os.Unsetenv(common.ArgoCDControllerClusterRoleEnvName)
assert.NoError(t, r.reconcileRoleBinding(applicationController, p, a))
assert.NoError(t, r.reconcileRoleBinding(common.ArgoCDApplicationControllerComponent, p, a))

expectedName = fmt.Sprintf("%s-%s", a.Name, "argocd-application-controller")
checkForUpdatedRoleRef(t, "custom-controller-role", expectedName)
Expand Down

0 comments on commit 68e1197

Please sign in to comment.