From a674335ccc0b09dc2d25b0cc3e5b4548a28c3a2a Mon Sep 17 00:00:00 2001
From: Jordan Liggitt <jliggitt@redhat.com>
Date: Sun, 13 May 2018 15:07:38 -0400
Subject: [PATCH] Avoid copying aggregated admin/edit/view roles during
 bootstrap

---
 .../rbac/reconciliation/reconcile_role.go     |  5 ++++
 .../reconciliation/reconcile_role_test.go     | 26 +++++++++++++++++++
 pkg/registry/rbac/rest/storage_rbac.go        |  4 +++
 3 files changed, 35 insertions(+)

diff --git a/pkg/registry/rbac/reconciliation/reconcile_role.go b/pkg/registry/rbac/reconciliation/reconcile_role.go
index b460599101..1ab840c759 100644
--- a/pkg/registry/rbac/reconciliation/reconcile_role.go
+++ b/pkg/registry/rbac/reconciliation/reconcile_role.go
@@ -214,6 +214,11 @@ func computeReconciledRole(existing, expected RuleOwner, removeExtraPermissions
 	_, result.MissingAggregationRuleSelectors = aggregationRuleCovers(existing.GetAggregationRule(), expected.GetAggregationRule())
 
 	switch {
+	case expected.GetAggregationRule() == nil && existing.GetAggregationRule() != nil:
+		// we didn't expect this to be an aggregated role at all, remove the existing aggregation
+		result.Role.SetAggregationRule(nil)
+		result.Operation = ReconcileUpdate
+
 	case !removeExtraPermissions && len(result.MissingAggregationRuleSelectors) > 0:
 		// add missing rules in the union case
 		aggregationRule := result.Role.GetAggregationRule()
diff --git a/pkg/registry/rbac/reconciliation/reconcile_role_test.go b/pkg/registry/rbac/reconciliation/reconcile_role_test.go
index 1d30c9cad6..6a7f632b48 100644
--- a/pkg/registry/rbac/reconciliation/reconcile_role_test.go
+++ b/pkg/registry/rbac/reconciliation/reconcile_role_test.go
@@ -350,6 +350,32 @@ func TestComputeReconciledRoleAggregationRules(t *testing.T) {
 			expectedReconciledRole:       aggregatedRole(aggregationrule([]map[string]string{{"alpha": "bravo"}, {"foo": "bar"}})),
 			expectedReconciliationNeeded: true,
 		},
+		"unexpected aggregation": {
+			// desired role is not aggregated
+			expectedRole: role(rules("pods", "nodes", "secrets"), nil, nil),
+			// existing role is aggregated
+			actualRole:             aggregatedRole(aggregationrule([]map[string]string{{"alpha": "bravo"}})),
+			removeExtraPermissions: false,
+
+			// reconciled role should have desired permissions and not be aggregated
+			expectedReconciledRole:       role(rules("pods", "nodes", "secrets"), nil, nil),
+			expectedReconciliationNeeded: true,
+		},
+		"unexpected aggregation with differing permissions": {
+			// desired role is not aggregated
+			expectedRole: role(rules("pods", "nodes", "secrets"), nil, nil),
+			// existing role is aggregated and has other permissions
+			actualRole: func() *rbac.ClusterRole {
+				r := aggregatedRole(aggregationrule([]map[string]string{{"alpha": "bravo"}}))
+				r.Rules = rules("deployments")
+				return r
+			}(),
+			removeExtraPermissions: false,
+
+			// reconciled role should have aggregation removed, preserve differing permissions, and include desired permissions
+			expectedReconciledRole:       role(rules("deployments", "pods", "nodes", "secrets"), nil, nil),
+			expectedReconciliationNeeded: true,
+		},
 	}
 
 	for k, tc := range tests {
diff --git a/pkg/registry/rbac/rest/storage_rbac.go b/pkg/registry/rbac/rest/storage_rbac.go
index b2bdb4bbb7..1622d067b3 100644
--- a/pkg/registry/rbac/rest/storage_rbac.go
+++ b/pkg/registry/rbac/rest/storage_rbac.go
@@ -320,6 +320,10 @@ func primeAggregatedClusterRoles(clusterRolesToAggregate map[string]string, clus
 		if err != nil {
 			return err
 		}
+		if existingRole.AggregationRule != nil {
+			// the old role already moved to an aggregated role, so there are no custom rules to migrate at this point
+			return nil
+		}
 		glog.V(1).Infof("migrating %v to %v", existingRole.Name, newName)
 		existingRole.Name = newName
 		existingRole.ResourceVersion = "" // clear this so the object can be created.