Migrate clusterroleaggregation controller to apply

pkg/controller/clusterroleaggregation/clusterroleaggregation_controller.go

@@ -22,6 +22,8 @@ import (
 	"sort"
 	"time"
 
+	"k8s.io/apiserver/pkg/features"
+	rbacv1ac "k8s.io/client-go/applyconfigurations/rbac/v1"
 	"k8s.io/klog/v2"
 
 	rbacv1 "k8s.io/api/rbac/v1"
@@ -31,11 +33,13 @@ import (
 	"k8s.io/apimachinery/pkg/labels"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/apimachinery/pkg/util/wait"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	rbacinformers "k8s.io/client-go/informers/rbac/v1"
 	rbacclient "k8s.io/client-go/kubernetes/typed/rbac/v1"
 	rbaclisters "k8s.io/client-go/listers/rbac/v1"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/util/workqueue"
+
 	"k8s.io/kubernetes/pkg/controller"
 )
 
@@ -121,17 +125,58 @@ func (c *ClusterRoleAggregationController) syncClusterRole(key string) error {
 		return nil
 	}
 
-	// we need to update
+	if utilfeature.DefaultFeatureGate.Enabled(features.ServerSideApply) {
+		err = c.applyClusterRoles(sharedClusterRole.Name, newPolicyRules)
+		if errors.IsUnsupportedMediaType(err) { // TODO: Remove this fallback at least one release after ServerSideApply GA
+			// When Server Side Apply is not enabled, fallback to Update. This is required when running
+			// 1.21 since api-server can be 1.20 during the upgrade/downgrade.
+			// Since Server Side Apply is enabled by default in Beta, this fallback only kicks in
+			// if the feature has been disabled using its feature flag.
+			err = c.updateClusterRoles(sharedClusterRole, newPolicyRules)
+		}
+	} else {
+		err = c.updateClusterRoles(sharedClusterRole, newPolicyRules)
+	}
+	return err
+}
+
+func (c *ClusterRoleAggregationController) applyClusterRoles(name string, newPolicyRules []rbacv1.PolicyRule) error {
+	clusterRoleApply := rbacv1ac.ClusterRole(name).
+		WithRules(toApplyPolicyRules(newPolicyRules)...)
+
+	opts := metav1.ApplyOptions{FieldManager: "clusterrole-aggregation-controller", Force: true}
+	_, err := c.clusterRoleClient.ClusterRoles().Apply(context.TODO(), clusterRoleApply, opts)
+	return err
+}
+
+func (c *ClusterRoleAggregationController) updateClusterRoles(sharedClusterRole *rbacv1.ClusterRole, newPolicyRules []rbacv1.PolicyRule) error {
 	clusterRole := sharedClusterRole.DeepCopy()
 	clusterRole.Rules = nil
 	for _, rule := range newPolicyRules {
 		clusterRole.Rules = append(clusterRole.Rules, *rule.DeepCopy())
 	}
-	_, err = c.clusterRoleClient.ClusterRoles().Update(context.TODO(), clusterRole, metav1.UpdateOptions{})
+	_, err := c.clusterRoleClient.ClusterRoles().Update(context.TODO(), clusterRole, metav1.UpdateOptions{})
-
 	return err
 }
 
+func toApplyPolicyRules(rules []rbacv1.PolicyRule) []*rbacv1ac.PolicyRuleApplyConfiguration {
+	var result []*rbacv1ac.PolicyRuleApplyConfiguration
+	for _, rule := range rules {
+		result = append(result, toApplyPolicyRule(rule))
+	}
+	return result
+}
+
+func toApplyPolicyRule(rule rbacv1.PolicyRule) *rbacv1ac.PolicyRuleApplyConfiguration {
+	result := rbacv1ac.PolicyRule()
+	result.Resources = rule.Resources
+	result.ResourceNames = rule.ResourceNames
+	result.APIGroups = rule.APIGroups
+	result.NonResourceURLs = rule.NonResourceURLs
+	result.Verbs = rule.Verbs
+	return result
+}
+
 func ruleExists(haystack []rbacv1.PolicyRule, needle rbacv1.PolicyRule) bool {
 	for _, curr := range haystack {
 		if equality.Semantic.DeepEqual(curr, needle) {

pkg/controller/clusterroleaggregation/clusterroleaggregation_controller_test.go

@@ -21,13 +21,17 @@ import (
 
 	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
+	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/diff"
+	"k8s.io/apimachinery/pkg/util/yaml"
+	rbacv1ac "k8s.io/client-go/applyconfigurations/rbac/v1"
 	fakeclient "k8s.io/client-go/kubernetes/fake"
 	rbaclisters "k8s.io/client-go/listers/rbac/v1"
 	clienttesting "k8s.io/client-go/testing"
 	"k8s.io/client-go/tools/cache"
+
 	"k8s.io/kubernetes/pkg/controller"
 )
 
@@ -69,11 +73,23 @@ func TestSyncClusterRole(t *testing.T) {
 		return ret
 	}
 
+	combinedApplyRole := func(selectors []map[string]string, rules ...[]rbacv1.PolicyRule) *rbacv1ac.ClusterRoleApplyConfiguration {
+		ret := rbacv1ac.ClusterRole("combined")
+
+		var r []*rbacv1ac.PolicyRuleApplyConfiguration
+		for _, currRules := range rules {
+			r = append(r, toApplyPolicyRules(currRules)...)
+		}
+		ret.WithRules(r...)
+		return ret
+	}
+
 	tests := []struct {
-		name                 string
+		name                     string
-		startingClusterRoles []*rbacv1.ClusterRole
+		startingClusterRoles     []*rbacv1.ClusterRole
-		clusterRoleToSync    string
+		clusterRoleToSync        string
-		expectedClusterRole  *rbacv1.ClusterRole
+		expectedClusterRole      *rbacv1.ClusterRole
+		expectedClusterRoleApply *rbacv1ac.ClusterRoleApplyConfiguration
 	}{
 		{
 			name: "remove dead rules",
@@ -81,8 +97,9 @@ func TestSyncClusterRole(t *testing.T) {
 				role("hammer", map[string]string{"foo": "bar"}, hammerRules()),
 				combinedRole([]map[string]string{{"foo": "bar"}}, sawRules()),
 			},
-			clusterRoleToSync:   "combined",
+			clusterRoleToSync:        "combined",
-			expectedClusterRole: combinedRole([]map[string]string{{"foo": "bar"}}, hammerRules()),
+			expectedClusterRole:      combinedRole([]map[string]string{{"foo": "bar"}}, hammerRules()),
+			expectedClusterRoleApply: combinedApplyRole([]map[string]string{{"foo": "bar"}}, hammerRules()),
 		},
 		{
 			name: "strip rules",
@@ -90,8 +107,9 @@ func TestSyncClusterRole(t *testing.T) {
 				role("hammer", map[string]string{"foo": "not-bar"}, hammerRules()),
 				combinedRole([]map[string]string{{"foo": "bar"}}, hammerRules()),
 			},
-			clusterRoleToSync:   "combined",
+			clusterRoleToSync:        "combined",
-			expectedClusterRole: combinedRole([]map[string]string{{"foo": "bar"}}),
+			expectedClusterRole:      combinedRole([]map[string]string{{"foo": "bar"}}),
+			expectedClusterRoleApply: combinedApplyRole([]map[string]string{{"foo": "bar"}}),
 		},
 		{
 			name: "select properly and put in order",
@@ -101,8 +119,9 @@ func TestSyncClusterRole(t *testing.T) {
 				role("saw", map[string]string{"foo": "not-bar"}, sawRules()),
 				combinedRole([]map[string]string{{"foo": "bar"}}),
 			},
-			clusterRoleToSync:   "combined",
+			clusterRoleToSync:        "combined",
-			expectedClusterRole: combinedRole([]map[string]string{{"foo": "bar"}}, chiselRules(), hammerRules()),
+			expectedClusterRole:      combinedRole([]map[string]string{{"foo": "bar"}}, chiselRules(), hammerRules()),
+			expectedClusterRoleApply: combinedApplyRole([]map[string]string{{"foo": "bar"}}, chiselRules(), hammerRules()),
 		},
 		{
 			name: "select properly with multiple selectors",
@@ -112,8 +131,9 @@ func TestSyncClusterRole(t *testing.T) {
 				role("saw", map[string]string{"foo": "not-bar"}, sawRules()),
 				combinedRole([]map[string]string{{"foo": "bar"}, {"foo": "not-bar"}}),
 			},
-			clusterRoleToSync:   "combined",
+			clusterRoleToSync:        "combined",
-			expectedClusterRole: combinedRole([]map[string]string{{"foo": "bar"}, {"foo": "not-bar"}}, chiselRules(), hammerRules(), sawRules()),
+			expectedClusterRole:      combinedRole([]map[string]string{{"foo": "bar"}, {"foo": "not-bar"}}, chiselRules(), hammerRules(), sawRules()),
+			expectedClusterRoleApply: combinedApplyRole([]map[string]string{{"foo": "bar"}, {"foo": "not-bar"}}, chiselRules(), hammerRules(), sawRules()),
 		},
 		{
 			name: "select properly remove duplicates",
@@ -124,8 +144,9 @@ func TestSyncClusterRole(t *testing.T) {
 				role("other-saw", map[string]string{"foo": "not-bar"}, sawRules()),
 				combinedRole([]map[string]string{{"foo": "bar"}, {"foo": "not-bar"}}),
 			},
-			clusterRoleToSync:   "combined",
+			clusterRoleToSync:        "combined",
-			expectedClusterRole: combinedRole([]map[string]string{{"foo": "bar"}, {"foo": "not-bar"}}, chiselRules(), hammerRules(), sawRules()),
+			expectedClusterRole:      combinedRole([]map[string]string{{"foo": "bar"}, {"foo": "not-bar"}}, chiselRules(), hammerRules(), sawRules()),
+			expectedClusterRoleApply: combinedApplyRole([]map[string]string{{"foo": "bar"}, {"foo": "not-bar"}}, chiselRules(), hammerRules(), sawRules()),
 		},
 		{
 			name: "no diff skip",
@@ -133,50 +154,82 @@ func TestSyncClusterRole(t *testing.T) {
 				role("hammer", map[string]string{"foo": "bar"}, hammerRules()),
 				combinedRole([]map[string]string{{"foo": "bar"}}, hammerRules()),
 			},
-			clusterRoleToSync:   "combined",
+			clusterRoleToSync:        "combined",
-			expectedClusterRole: nil,
+			expectedClusterRoleApply: nil,
 		}}
 
-	for _, test := range tests {
+	for _, serverSideApplyEnabled := range []bool{true, false} {
-		t.Run(test.name, func(t *testing.T) {
+		for _, test := range tests {
-			indexer := cache.NewIndexer(controller.KeyFunc, cache.Indexers{})
+			t.Run(test.name, func(t *testing.T) {
-			objs := []runtime.Object{}
+				indexer := cache.NewIndexer(controller.KeyFunc, cache.Indexers{})
-			for _, obj := range test.startingClusterRoles {
+				objs := []runtime.Object{}
-				objs = append(objs, obj)
+				for _, obj := range test.startingClusterRoles {
-				indexer.Add(obj)
+					objs = append(objs, obj)
-			}
+					indexer.Add(obj)
-			fakeClient := fakeclient.NewSimpleClientset(objs...)
+				}
-			c := ClusterRoleAggregationController{
+				fakeClient := fakeclient.NewSimpleClientset(objs...)
-				clusterRoleClient: fakeClient.RbacV1(),
-				clusterRoleLister: rbaclisters.NewClusterRoleLister(indexer),
-			}
-			err := c.syncClusterRole(test.clusterRoleToSync)
-			if err != nil {
-				t.Fatal(err)
-			}
 
-			if test.expectedClusterRole == nil {
+				// The default reactor doesn't support apply, so we need our own (trivial) reactor
-				if len(fakeClient.Actions()) != 0 {
+				fakeClient.PrependReactor("patch", "clusterroles", func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) {
+					if serverSideApplyEnabled == false {
+						// UnsupportedMediaType
+						return true, nil, errors.NewGenericServerResponse(415, "get", action.GetResource().GroupResource(), "test", "Apply not supported", 0, true)
+					}
+					return true, nil, nil // clusterroleaggregator drops returned objects so no point in constructing them
+				})
+				c := ClusterRoleAggregationController{
+					clusterRoleClient: fakeClient.RbacV1(),
+					clusterRoleLister: rbaclisters.NewClusterRoleLister(indexer),
+				}
+				err := c.syncClusterRole(test.clusterRoleToSync)
+				if err != nil {
+					t.Fatal(err)
+				}
+
+				if test.expectedClusterRoleApply == nil {
+					if len(fakeClient.Actions()) != 0 {
+						t.Fatalf("unexpected actions %#v", fakeClient.Actions())
+					}
+					return
+				}
+
+				expectedActions := 1
+				if !serverSideApplyEnabled {
+					expectedActions = 2
+				}
+				if len(fakeClient.Actions()) != expectedActions {
 					t.Fatalf("unexpected actions %#v", fakeClient.Actions())
 				}
-				return
-			}
-			if len(fakeClient.Actions()) != 1 {
-				t.Fatalf("unexpected actions %#v", fakeClient.Actions())
-			}
 
-			action := fakeClient.Actions()[0]
+				action := fakeClient.Actions()[0]
-			if !action.Matches("update", "clusterroles") {
+				if !action.Matches("patch", "clusterroles") {
-				t.Fatalf("unexpected action %#v", action)
+					t.Fatalf("unexpected action %#v", action)
-			}
+				}
-			updateAction, ok := action.(clienttesting.UpdateAction)
+				applyAction, ok := action.(clienttesting.PatchAction)
-			if !ok {
+				if !ok {
-				t.Fatalf("unexpected action %#v", action)
+					t.Fatalf("unexpected action %#v", action)
-			}
+				}
-			if !equality.Semantic.DeepEqual(updateAction.GetObject().(*rbacv1.ClusterRole), test.expectedClusterRole) {
+				ac := &rbacv1ac.ClusterRoleApplyConfiguration{}
-				t.Fatalf("%v", diff.ObjectDiff(test.expectedClusterRole, updateAction.GetObject().(*rbacv1.ClusterRole)))
+				if err := yaml.Unmarshal(applyAction.GetPatch(), ac); err != nil {
-
+					t.Fatalf("error unmarshalling apply request: %v", err)
-			}
+				}
-		})
+				if !equality.Semantic.DeepEqual(ac, test.expectedClusterRoleApply) {
+					t.Fatalf("%v", diff.ObjectDiff(test.expectedClusterRoleApply, ac))
+				}
+				if expectedActions == 2 {
+					action := fakeClient.Actions()[1]
+					if !action.Matches("update", "clusterroles") {
+						t.Fatalf("unexpected action %#v", action)
+					}
+					updateAction, ok := action.(clienttesting.UpdateAction)
+					if !ok {
+						t.Fatalf("unexpected action %#v", action)
+					}
+					if !equality.Semantic.DeepEqual(updateAction.GetObject().(*rbacv1.ClusterRole), test.expectedClusterRole) {
+						t.Fatalf("%v", diff.ObjectDiff(test.expectedClusterRole, updateAction.GetObject().(*rbacv1.ClusterRole)))
+					}
+				}
+			})
+		}
 	}
 }