Merge pull request #41922 from deads2k/rbac-05-reconcile

Automatic merge from submit-queue

make reconcilation generic to handle roles and clusterroles

We have a need to reconcile regular roles, so this pull moves the reconciliation code to use interfaces (still tightly coupled) rather than structs.

@liggitt @kubernetes/sig-auth-pr-reviews

pkg/registry/rbac/reconciliation/reconcile_clusterrole.go

@@ -37,21 +37,38 @@ var (
 	ReconcileNone     ReconcileOperation = "none"
 )
 
+type RuleOwnerModifier interface {
+	Get(namespace, name string) (RuleOwner, error)
+	Create(RuleOwner) (RuleOwner, error)
+	Update(RuleOwner) (RuleOwner, error)
+}
+
+type RuleOwner interface {
+	GetNamespace() string
+	GetName() string
+	GetLabels() map[string]string
+	SetLabels(map[string]string)
+	GetAnnotations() map[string]string
+	SetAnnotations(map[string]string)
+	GetRules() []rbac.PolicyRule
+	SetRules([]rbac.PolicyRule)
+}
+
 type ReconcileClusterRoleOptions struct {
 	// Role is the expected role that will be reconciled
-	Role *rbac.ClusterRole
+	Role RuleOwner
 	// Confirm indicates writes should be performed. When false, results are returned as a dry-run.
 	Confirm bool
 	// RemoveExtraPermissions indicates reconciliation should remove extra permissions from an existing role
 	RemoveExtraPermissions bool
 	// Client is used to look up existing roles, and create/update the role when Confirm=true
-	Client internalversion.ClusterRoleInterface
+	Client RuleOwnerModifier
 }
 
 type ReconcileClusterRoleResult struct {
 	// Role is the reconciled role from the reconciliation operation.
 	// If the reconcile was performed as a dry-run, or the existing role was protected, the reconciled role is not persisted.
-	Role *rbac.ClusterRole
+	Role RuleOwner
 
 	// MissingRules contains expected rules that were missing from the currently persisted role
 	MissingRules []rbac.PolicyRule
@@ -81,12 +98,12 @@ func (o *ReconcileClusterRoleOptions) run(attempts int) (*ReconcileClusterRoleRe
 
 	var result *ReconcileClusterRoleResult
 
-	existing, err := o.Client.Get(o.Role.Name, metav1.GetOptions{})
+	existing, err := o.Client.Get(o.Role.GetNamespace(), o.Role.GetName())
 	switch {
 	case errors.IsNotFound(err):
 		result = &ReconcileClusterRoleResult{
 			Role:         o.Role,
-			MissingRules: o.Role.Rules,
+			MissingRules: o.Role.GetRules(),
 			Operation:    ReconcileCreate,
 		}
 
@@ -144,41 +161,41 @@ func (o *ReconcileClusterRoleOptions) run(attempts int) (*ReconcileClusterRoleRe
 
 // computeReconciledRole returns the role that must be created and/or updated to make the
 // existing role's permissions match the expected role's permissions
-func computeReconciledRole(existing, expected *rbac.ClusterRole, removeExtraPermissions bool) (*ReconcileClusterRoleResult, error) {
+func computeReconciledRole(existing, expected RuleOwner, removeExtraPermissions bool) (*ReconcileClusterRoleResult, error) {
 	result := &ReconcileClusterRoleResult{Operation: ReconcileNone}
 
-	result.Protected = (existing.Annotations[rbac.AutoUpdateAnnotationKey] == "false")
+	result.Protected = (existing.GetAnnotations()[rbac.AutoUpdateAnnotationKey] == "false")
 
 	// Start with a copy of the existing object
 	changedObj, err := api.Scheme.DeepCopy(existing)
 	if err != nil {
 		return nil, err
 	}
-	result.Role = changedObj.(*rbac.ClusterRole)
+	result.Role = changedObj.(RuleOwner)
 
 	// Merge expected annotations and labels
-	result.Role.Annotations = merge(expected.Annotations, result.Role.Annotations)
+	result.Role.SetAnnotations(merge(expected.GetAnnotations(), result.Role.GetAnnotations()))
-	if !reflect.DeepEqual(result.Role.Annotations, existing.Annotations) {
+	if !reflect.DeepEqual(result.Role.GetAnnotations(), existing.GetAnnotations()) {
 		result.Operation = ReconcileUpdate
 	}
-	result.Role.Labels = merge(expected.Labels, result.Role.Labels)
+	result.Role.SetLabels(merge(expected.GetLabels(), result.Role.GetLabels()))
-	if !reflect.DeepEqual(result.Role.Labels, existing.Labels) {
+	if !reflect.DeepEqual(result.Role.GetLabels(), existing.GetLabels()) {
 		result.Operation = ReconcileUpdate
 	}
 
 	// Compute extra and missing rules
-	_, result.ExtraRules = validation.Covers(expected.Rules, existing.Rules)
+	_, result.ExtraRules = validation.Covers(expected.GetRules(), existing.GetRules())
-	_, result.MissingRules = validation.Covers(existing.Rules, expected.Rules)
+	_, result.MissingRules = validation.Covers(existing.GetRules(), expected.GetRules())
 
 	switch {
 	case !removeExtraPermissions && len(result.MissingRules) > 0:
 		// add missing rules in the union case
-		result.Role.Rules = append(result.Role.Rules, result.MissingRules...)
+		result.Role.SetRules(append(result.Role.GetRules(), result.MissingRules...))
 		result.Operation = ReconcileUpdate
 
 	case removeExtraPermissions && (len(result.MissingRules) > 0 || len(result.ExtraRules) > 0):
 		// stomp to expected rules in the non-union case
-		result.Role.Rules = expected.Rules
+		result.Role.SetRules(expected.GetRules())
 		result.Operation = ReconcileUpdate
 	}
 
@@ -198,3 +215,68 @@ func merge(maps ...map[string]string) map[string]string {
 	}
 	return output
 }
+
+type ClusterRoleRuleOwner struct {
+	ClusterRole *rbac.ClusterRole
+}
+
+func (o ClusterRoleRuleOwner) GetNamespace() string {
+	return o.ClusterRole.Namespace
+}
+
+func (o ClusterRoleRuleOwner) GetName() string {
+	return o.ClusterRole.Name
+}
+
+func (o ClusterRoleRuleOwner) GetLabels() map[string]string {
+	return o.ClusterRole.Labels
+}
+
+func (o ClusterRoleRuleOwner) SetLabels(in map[string]string) {
+	o.ClusterRole.Labels = in
+}
+
+func (o ClusterRoleRuleOwner) GetAnnotations() map[string]string {
+	return o.ClusterRole.Annotations
+}
+
+func (o ClusterRoleRuleOwner) SetAnnotations(in map[string]string) {
+	o.ClusterRole.Annotations = in
+}
+
+func (o ClusterRoleRuleOwner) GetRules() []rbac.PolicyRule {
+	return o.ClusterRole.Rules
+}
+
+func (o ClusterRoleRuleOwner) SetRules(in []rbac.PolicyRule) {
+	o.ClusterRole.Rules = in
+}
+
+type ClusterRoleModifier struct {
+	Client internalversion.ClusterRoleInterface
+}
+
+func (c ClusterRoleModifier) Get(namespace, name string) (RuleOwner, error) {
+	ret, err := c.Client.Get(name, metav1.GetOptions{})
+	if err != nil {
+		return nil, err
+	}
+	return ClusterRoleRuleOwner{ClusterRole: ret}, err
+}
+
+func (c ClusterRoleModifier) Create(in RuleOwner) (RuleOwner, error) {
+	ret, err := c.Client.Create(in.(ClusterRoleRuleOwner).ClusterRole)
+	if err != nil {
+		return nil, err
+	}
+	return ClusterRoleRuleOwner{ClusterRole: ret}, err
+}
+
+func (c ClusterRoleModifier) Update(in RuleOwner) (RuleOwner, error) {
+	ret, err := c.Client.Create(in.(ClusterRoleRuleOwner).ClusterRole)
+	if err != nil {
+		return nil, err
+	}
+	return ClusterRoleRuleOwner{ClusterRole: ret}, err
+
+}

pkg/registry/rbac/reconciliation/reconcile_clusterrole_test.go

@@ -256,7 +256,9 @@ func TestComputeReconciledRole(t *testing.T) {
 	}
 
 	for k, tc := range tests {
-		result, err := computeReconciledRole(tc.actualRole, tc.expectedRole, tc.removeExtraPermissions)
+		actualRole := ClusterRoleRuleOwner{ClusterRole: tc.actualRole}
+		expectedRole := ClusterRoleRuleOwner{ClusterRole: tc.expectedRole}
+		result, err := computeReconciledRole(actualRole, expectedRole, tc.removeExtraPermissions)
 		if err != nil {
 			t.Errorf("%s: %v", k, err)
 			continue
@@ -266,7 +268,7 @@ func TestComputeReconciledRole(t *testing.T) {
 			t.Errorf("%s: Expected\n\t%v\ngot\n\t%v", k, tc.expectedReconciliationNeeded, reconciliationNeeded)
 			continue
 		}
-		if reconciliationNeeded && !api.Semantic.DeepEqual(result.Role, tc.expectedReconciledRole) {
+		if reconciliationNeeded && !api.Semantic.DeepEqual(result.Role.(ClusterRoleRuleOwner).ClusterRole, tc.expectedReconciledRole) {
 			t.Errorf("%s: Expected\n\t%#v\ngot\n\t%#v", k, tc.expectedReconciledRole, result.Role)
 		}
 	}

pkg/registry/rbac/rest/storage_rbac.go

@@ -138,8 +138,8 @@ func PostStartHook(hookContext genericapiserver.PostStartHookContext) error {
 		// ensure bootstrap roles are created or reconciled
 		for _, clusterRole := range append(bootstrappolicy.ClusterRoles(), bootstrappolicy.ControllerRoles()...) {
 			opts := reconciliation.ReconcileClusterRoleOptions{
-				Role:    &clusterRole,
+				Role:    reconciliation.ClusterRoleRuleOwner{ClusterRole: &clusterRole},
-				Client:  clientset.ClusterRoles(),
+				Client:  reconciliation.ClusterRoleModifier{Client: clientset.ClusterRoles()},
 				Confirm: true,
 			}
 			err := retry.RetryOnConflict(retry.DefaultBackoff, func() error {