From 24f95667bf9e6d28cb79a4d68b6775611f84c5a7 Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Mon, 8 Mar 2021 12:06:10 -0800 Subject: [PATCH] Migrate clusterroleaggregation controller to apply --- .../clusterroleaggregation_controller.go | 51 +++++- .../clusterroleaggregation_controller_test.go | 159 ++++++++++++------ 2 files changed, 154 insertions(+), 56 deletions(-) diff --git a/pkg/controller/clusterroleaggregation/clusterroleaggregation_controller.go b/pkg/controller/clusterroleaggregation/clusterroleaggregation_controller.go index 6eb4d9a5266..15a5fc16571 100644 --- a/pkg/controller/clusterroleaggregation/clusterroleaggregation_controller.go +++ b/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) { diff --git a/pkg/controller/clusterroleaggregation/clusterroleaggregation_controller_test.go b/pkg/controller/clusterroleaggregation/clusterroleaggregation_controller_test.go index de007cdd3f3..2895816d02f 100644 --- a/pkg/controller/clusterroleaggregation/clusterroleaggregation_controller_test.go +++ b/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 - startingClusterRoles []*rbacv1.ClusterRole - clusterRoleToSync string - expectedClusterRole *rbacv1.ClusterRole + name string + startingClusterRoles []*rbacv1.ClusterRole + clusterRoleToSync string + 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", - expectedClusterRole: combinedRole([]map[string]string{{"foo": "bar"}}, hammerRules()), + clusterRoleToSync: "combined", + 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", - expectedClusterRole: combinedRole([]map[string]string{{"foo": "bar"}}), + clusterRoleToSync: "combined", + 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", - expectedClusterRole: combinedRole([]map[string]string{{"foo": "bar"}}, chiselRules(), hammerRules()), + clusterRoleToSync: "combined", + 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", - expectedClusterRole: combinedRole([]map[string]string{{"foo": "bar"}, {"foo": "not-bar"}}, chiselRules(), hammerRules(), sawRules()), + clusterRoleToSync: "combined", + 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", - expectedClusterRole: combinedRole([]map[string]string{{"foo": "bar"}, {"foo": "not-bar"}}, chiselRules(), hammerRules(), sawRules()), + clusterRoleToSync: "combined", + 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", - expectedClusterRole: nil, + clusterRoleToSync: "combined", + expectedClusterRoleApply: nil, }} - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - indexer := cache.NewIndexer(controller.KeyFunc, cache.Indexers{}) - objs := []runtime.Object{} - for _, obj := range test.startingClusterRoles { - objs = append(objs, obj) - indexer.Add(obj) - } - fakeClient := fakeclient.NewSimpleClientset(objs...) - c := ClusterRoleAggregationController{ - clusterRoleClient: fakeClient.RbacV1(), - clusterRoleLister: rbaclisters.NewClusterRoleLister(indexer), - } - err := c.syncClusterRole(test.clusterRoleToSync) - if err != nil { - t.Fatal(err) - } + for _, serverSideApplyEnabled := range []bool{true, false} { + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + indexer := cache.NewIndexer(controller.KeyFunc, cache.Indexers{}) + objs := []runtime.Object{} + for _, obj := range test.startingClusterRoles { + objs = append(objs, obj) + indexer.Add(obj) + } + fakeClient := fakeclient.NewSimpleClientset(objs...) - if test.expectedClusterRole == nil { - if len(fakeClient.Actions()) != 0 { + // The default reactor doesn't support apply, so we need our own (trivial) reactor + 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] - 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))) - - } - }) + action := fakeClient.Actions()[0] + if !action.Matches("patch", "clusterroles") { + t.Fatalf("unexpected action %#v", action) + } + applyAction, ok := action.(clienttesting.PatchAction) + if !ok { + t.Fatalf("unexpected action %#v", action) + } + ac := &rbacv1ac.ClusterRoleApplyConfiguration{} + 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))) + } + } + }) + } } }