Merge pull request #63761 from liggitt/aggregated-bootstrap-race

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Avoid copying aggregated admin/edit/view roles during bootstrap

Fixes #63760

At apiserver startup, prior to reconciling cluster roles, the following roles (if they exist) are copied:
* admin -> system:aggregate-to-admin
* edit -> system:aggregate-to-edit
* view -> system:aggregate-to-view

This was added in 1.9 as part of role aggregation to ensure custom permissions added to the admin/edit/view roles were preserved, prior to making the admin/edit/view roles aggregated (since the permissions of an aggregated role are controller-managed)

When starting multiple members of a new HA cluster simultaneously, the following race can occur:
* t=0, server 1,2,3 start up
* t=1, server 1 finds no admin/edit/view roles exist, begins role reconciliation and creates the aggregated `admin` role
* t=2, server 2 finds and copies the `admin` role created by server 1 to `system:aggregate-to-admin`

If this race is encountered, it results in `system:aggregate-to-admin` being an aggregated role, and its permissions subject to being overwritten by the aggregating controller. To prevent this from happening, the permission-preserving copy should only copy over roles that are not yet aggregated.

To correct this in clusters that have already encountered it, role reconciliation should remove aggregation from a role that is not expected to be aggregated at all.

```release-note
corrects a race condition in bootstrapping aggregated cluster roles in new HA clusters
```

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()

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 {

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.