Skip to content

Commit 67fbccc

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #99462 from jpbetz/apply-cluster-role-aggregator
Migrate cluster role aggregator to apply
2 parents 5155865 + 24f9566 commit 67fbccc

File tree

2 files changed

+156
-58
lines changed

2 files changed

+156
-58
lines changed

‎pkg/controller/clusterroleaggregation/clusterroleaggregation_controller.go‎

Lines changed: 48 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ import (
2222
"sort"
2323
"time"
2424

25+
"k8s.io/apiserver/pkg/features"
26+
rbacv1ac "k8s.io/client-go/applyconfigurations/rbac/v1"
2527
"k8s.io/klog/v2"
2628

2729
rbacv1 "k8s.io/api/rbac/v1"
@@ -31,11 +33,13 @@ import (
3133
"k8s.io/apimachinery/pkg/labels"
3234
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
3335
"k8s.io/apimachinery/pkg/util/wait"
36+
utilfeature "k8s.io/apiserver/pkg/util/feature"
3437
rbacinformers "k8s.io/client-go/informers/rbac/v1"
3538
rbacclient "k8s.io/client-go/kubernetes/typed/rbac/v1"
3639
rbaclisters "k8s.io/client-go/listers/rbac/v1"
3740
"k8s.io/client-go/tools/cache"
3841
"k8s.io/client-go/util/workqueue"
42+
3943
"k8s.io/kubernetes/pkg/controller"
4044
)
4145

@@ -121,17 +125,58 @@ func (c *ClusterRoleAggregationController) syncClusterRole(key string) error {
121125
return nil
122126
}
123127

124-
// we need to update
128+
if utilfeature.DefaultFeatureGate.Enabled(features.ServerSideApply) {
129+
err = c.applyClusterRoles(sharedClusterRole.Name, newPolicyRules)
130+
if errors.IsUnsupportedMediaType(err) { // TODO: Remove this fallback at least one release after ServerSideApply GA
131+
// When Server Side Apply is not enabled, fallback to Update. This is required when running
132+
// 1.21 since api-server can be 1.20 during the upgrade/downgrade.
133+
// Since Server Side Apply is enabled by default in Beta, this fallback only kicks in
134+
// if the feature has been disabled using its feature flag.
135+
err = c.updateClusterRoles(sharedClusterRole, newPolicyRules)
136+
}
137+
} else {
138+
err = c.updateClusterRoles(sharedClusterRole, newPolicyRules)
139+
}
140+
return err
141+
}
142+
143+
func (c *ClusterRoleAggregationController) applyClusterRoles(name string, newPolicyRules []rbacv1.PolicyRule) error {
144+
clusterRoleApply := rbacv1ac.ClusterRole(name).
145+
WithRules(toApplyPolicyRules(newPolicyRules)...)
146+
147+
opts := metav1.ApplyOptions{FieldManager: "clusterrole-aggregation-controller", Force: true}
148+
_, err := c.clusterRoleClient.ClusterRoles().Apply(context.TODO(), clusterRoleApply, opts)
149+
return err
150+
}
151+
152+
func (c *ClusterRoleAggregationController) updateClusterRoles(sharedClusterRole *rbacv1.ClusterRole, newPolicyRules []rbacv1.PolicyRule) error {
125153
clusterRole := sharedClusterRole.DeepCopy()
126154
clusterRole.Rules = nil
127155
for _, rule := range newPolicyRules {
128156
clusterRole.Rules = append(clusterRole.Rules, *rule.DeepCopy())
129157
}
130-
_, err = c.clusterRoleClient.ClusterRoles().Update(context.TODO(), clusterRole, metav1.UpdateOptions{})
131-
158+
_, err := c.clusterRoleClient.ClusterRoles().Update(context.TODO(), clusterRole, metav1.UpdateOptions{})
132159
return err
133160
}
134161

162+
func toApplyPolicyRules(rules []rbacv1.PolicyRule) []*rbacv1ac.PolicyRuleApplyConfiguration {
163+
var result []*rbacv1ac.PolicyRuleApplyConfiguration
164+
for _, rule := range rules {
165+
result = append(result, toApplyPolicyRule(rule))
166+
}
167+
return result
168+
}
169+
170+
func toApplyPolicyRule(rule rbacv1.PolicyRule) *rbacv1ac.PolicyRuleApplyConfiguration {
171+
result := rbacv1ac.PolicyRule()
172+
result.Resources = rule.Resources
173+
result.ResourceNames = rule.ResourceNames
174+
result.APIGroups = rule.APIGroups
175+
result.NonResourceURLs = rule.NonResourceURLs
176+
result.Verbs = rule.Verbs
177+
return result
178+
}
179+
135180
func ruleExists(haystack []rbacv1.PolicyRule, needle rbacv1.PolicyRule) bool {
136181
for _, curr := range haystack {
137182
if equality.Semantic.DeepEqual(curr, needle) {

‎pkg/controller/clusterroleaggregation/clusterroleaggregation_controller_test.go‎

Lines changed: 108 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,17 @@ import (
2121

2222
rbacv1 "k8s.io/api/rbac/v1"
2323
"k8s.io/apimachinery/pkg/api/equality"
24+
"k8s.io/apimachinery/pkg/api/errors"
2425
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2526
"k8s.io/apimachinery/pkg/runtime"
2627
"k8s.io/apimachinery/pkg/util/diff"
28+
"k8s.io/apimachinery/pkg/util/yaml"
29+
rbacv1ac "k8s.io/client-go/applyconfigurations/rbac/v1"
2730
fakeclient "k8s.io/client-go/kubernetes/fake"
2831
rbaclisters "k8s.io/client-go/listers/rbac/v1"
2932
clienttesting "k8s.io/client-go/testing"
3033
"k8s.io/client-go/tools/cache"
34+
3135
"k8s.io/kubernetes/pkg/controller"
3236
)
3337

@@ -69,29 +73,43 @@ func TestSyncClusterRole(t *testing.T) {
6973
return ret
7074
}
7175

76+
combinedApplyRole := func(selectors []map[string]string, rules ...[]rbacv1.PolicyRule) *rbacv1ac.ClusterRoleApplyConfiguration {
77+
ret := rbacv1ac.ClusterRole("combined")
78+
79+
var r []*rbacv1ac.PolicyRuleApplyConfiguration
80+
for _, currRules := range rules {
81+
r = append(r, toApplyPolicyRules(currRules)...)
82+
}
83+
ret.WithRules(r...)
84+
return ret
85+
}
86+
7287
tests := []struct {
73-
name string
74-
startingClusterRoles []*rbacv1.ClusterRole
75-
clusterRoleToSync string
76-
expectedClusterRole *rbacv1.ClusterRole
88+
name string
89+
startingClusterRoles []*rbacv1.ClusterRole
90+
clusterRoleToSync string
91+
expectedClusterRole *rbacv1.ClusterRole
92+
expectedClusterRoleApply *rbacv1ac.ClusterRoleApplyConfiguration
7793
}{
7894
{
7995
name: "remove dead rules",
8096
startingClusterRoles: []*rbacv1.ClusterRole{
8197
role("hammer", map[string]string{"foo": "bar"}, hammerRules()),
8298
combinedRole([]map[string]string{{"foo": "bar"}}, sawRules()),
8399
},
84-
clusterRoleToSync: "combined",
85-
expectedClusterRole: combinedRole([]map[string]string{{"foo": "bar"}}, hammerRules()),
100+
clusterRoleToSync: "combined",
101+
expectedClusterRole: combinedRole([]map[string]string{{"foo": "bar"}}, hammerRules()),
102+
expectedClusterRoleApply: combinedApplyRole([]map[string]string{{"foo": "bar"}}, hammerRules()),
86103
},
87104
{
88105
name: "strip rules",
89106
startingClusterRoles: []*rbacv1.ClusterRole{
90107
role("hammer", map[string]string{"foo": "not-bar"}, hammerRules()),
91108
combinedRole([]map[string]string{{"foo": "bar"}}, hammerRules()),
92109
},
93-
clusterRoleToSync: "combined",
94-
expectedClusterRole: combinedRole([]map[string]string{{"foo": "bar"}}),
110+
clusterRoleToSync: "combined",
111+
expectedClusterRole: combinedRole([]map[string]string{{"foo": "bar"}}),
112+
expectedClusterRoleApply: combinedApplyRole([]map[string]string{{"foo": "bar"}}),
95113
},
96114
{
97115
name: "select properly and put in order",
@@ -101,8 +119,9 @@ func TestSyncClusterRole(t *testing.T) {
101119
role("saw", map[string]string{"foo": "not-bar"}, sawRules()),
102120
combinedRole([]map[string]string{{"foo": "bar"}}),
103121
},
104-
clusterRoleToSync: "combined",
105-
expectedClusterRole: combinedRole([]map[string]string{{"foo": "bar"}}, chiselRules(), hammerRules()),
122+
clusterRoleToSync: "combined",
123+
expectedClusterRole: combinedRole([]map[string]string{{"foo": "bar"}}, chiselRules(), hammerRules()),
124+
expectedClusterRoleApply: combinedApplyRole([]map[string]string{{"foo": "bar"}}, chiselRules(), hammerRules()),
106125
},
107126
{
108127
name: "select properly with multiple selectors",
@@ -112,8 +131,9 @@ func TestSyncClusterRole(t *testing.T) {
112131
role("saw", map[string]string{"foo": "not-bar"}, sawRules()),
113132
combinedRole([]map[string]string{{"foo": "bar"}, {"foo": "not-bar"}}),
114133
},
115-
clusterRoleToSync: "combined",
116-
expectedClusterRole: combinedRole([]map[string]string{{"foo": "bar"}, {"foo": "not-bar"}}, chiselRules(), hammerRules(), sawRules()),
134+
clusterRoleToSync: "combined",
135+
expectedClusterRole: combinedRole([]map[string]string{{"foo": "bar"}, {"foo": "not-bar"}}, chiselRules(), hammerRules(), sawRules()),
136+
expectedClusterRoleApply: combinedApplyRole([]map[string]string{{"foo": "bar"}, {"foo": "not-bar"}}, chiselRules(), hammerRules(), sawRules()),
117137
},
118138
{
119139
name: "select properly remove duplicates",
@@ -124,59 +144,92 @@ func TestSyncClusterRole(t *testing.T) {
124144
role("other-saw", map[string]string{"foo": "not-bar"}, sawRules()),
125145
combinedRole([]map[string]string{{"foo": "bar"}, {"foo": "not-bar"}}),
126146
},
127-
clusterRoleToSync: "combined",
128-
expectedClusterRole: combinedRole([]map[string]string{{"foo": "bar"}, {"foo": "not-bar"}}, chiselRules(), hammerRules(), sawRules()),
147+
clusterRoleToSync: "combined",
148+
expectedClusterRole: combinedRole([]map[string]string{{"foo": "bar"}, {"foo": "not-bar"}}, chiselRules(), hammerRules(), sawRules()),
149+
expectedClusterRoleApply: combinedApplyRole([]map[string]string{{"foo": "bar"}, {"foo": "not-bar"}}, chiselRules(), hammerRules(), sawRules()),
129150
},
130151
{
131152
name: "no diff skip",
132153
startingClusterRoles: []*rbacv1.ClusterRole{
133154
role("hammer", map[string]string{"foo": "bar"}, hammerRules()),
134155
combinedRole([]map[string]string{{"foo": "bar"}}, hammerRules()),
135156
},
136-
clusterRoleToSync: "combined",
137-
expectedClusterRole: nil,
157+
clusterRoleToSync: "combined",
158+
expectedClusterRoleApply: nil,
138159
}}
139160

140-
for _, test := range tests {
141-
t.Run(test.name, func(t *testing.T) {
142-
indexer := cache.NewIndexer(controller.KeyFunc, cache.Indexers{})
143-
objs := []runtime.Object{}
144-
for _, obj := range test.startingClusterRoles {
145-
objs = append(objs, obj)
146-
indexer.Add(obj)
147-
}
148-
fakeClient := fakeclient.NewSimpleClientset(objs...)
149-
c := ClusterRoleAggregationController{
150-
clusterRoleClient: fakeClient.RbacV1(),
151-
clusterRoleLister: rbaclisters.NewClusterRoleLister(indexer),
152-
}
153-
err := c.syncClusterRole(test.clusterRoleToSync)
154-
if err != nil {
155-
t.Fatal(err)
156-
}
157-
158-
if test.expectedClusterRole == nil {
159-
if len(fakeClient.Actions()) != 0 {
161+
for _, serverSideApplyEnabled := range []bool{true, false} {
162+
for _, test := range tests {
163+
t.Run(test.name, func(t *testing.T) {
164+
indexer := cache.NewIndexer(controller.KeyFunc, cache.Indexers{})
165+
objs := []runtime.Object{}
166+
for _, obj := range test.startingClusterRoles {
167+
objs = append(objs, obj)
168+
indexer.Add(obj)
169+
}
170+
fakeClient := fakeclient.NewSimpleClientset(objs...)
171+
172+
// The default reactor doesn't support apply, so we need our own (trivial) reactor
173+
fakeClient.PrependReactor("patch", "clusterroles", func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) {
174+
if serverSideApplyEnabled == false {
175+
// UnsupportedMediaType
176+
return true, nil, errors.NewGenericServerResponse(415, "get", action.GetResource().GroupResource(), "test", "Apply not supported", 0, true)
177+
}
178+
return true, nil, nil // clusterroleaggregator drops returned objects so no point in constructing them
179+
})
180+
c := ClusterRoleAggregationController{
181+
clusterRoleClient: fakeClient.RbacV1(),
182+
clusterRoleLister: rbaclisters.NewClusterRoleLister(indexer),
183+
}
184+
err := c.syncClusterRole(test.clusterRoleToSync)
185+
if err != nil {
186+
t.Fatal(err)
187+
}
188+
189+
if test.expectedClusterRoleApply == nil {
190+
if len(fakeClient.Actions()) != 0 {
191+
t.Fatalf("unexpected actions %#v", fakeClient.Actions())
192+
}
193+
return
194+
}
195+
196+
expectedActions := 1
197+
if !serverSideApplyEnabled {
198+
expectedActions = 2
199+
}
200+
if len(fakeClient.Actions()) != expectedActions {
160201
t.Fatalf("unexpected actions %#v", fakeClient.Actions())
161202
}
162-
return
163-
}
164-
if len(fakeClient.Actions()) != 1 {
165-
t.Fatalf("unexpected actions %#v", fakeClient.Actions())
166-
}
167-
168-
action := fakeClient.Actions()[0]
169-
if !action.Matches("update", "clusterroles") {
170-
t.Fatalf("unexpected action %#v", action)
171-
}
172-
updateAction, ok := action.(clienttesting.UpdateAction)
173-
if !ok {
174-
t.Fatalf("unexpected action %#v", action)
175-
}
176-
if !equality.Semantic.DeepEqual(updateAction.GetObject().(*rbacv1.ClusterRole), test.expectedClusterRole) {
177-
t.Fatalf("%v", diff.ObjectDiff(test.expectedClusterRole, updateAction.GetObject().(*rbacv1.ClusterRole)))
178-
179-
}
180-
})
203+
204+
action := fakeClient.Actions()[0]
205+
if !action.Matches("patch", "clusterroles") {
206+
t.Fatalf("unexpected action %#v", action)
207+
}
208+
applyAction, ok := action.(clienttesting.PatchAction)
209+
if !ok {
210+
t.Fatalf("unexpected action %#v", action)
211+
}
212+
ac := &rbacv1ac.ClusterRoleApplyConfiguration{}
213+
if err := yaml.Unmarshal(applyAction.GetPatch(), ac); err != nil {
214+
t.Fatalf("error unmarshalling apply request: %v", err)
215+
}
216+
if !equality.Semantic.DeepEqual(ac, test.expectedClusterRoleApply) {
217+
t.Fatalf("%v", diff.ObjectDiff(test.expectedClusterRoleApply, ac))
218+
}
219+
if expectedActions == 2 {
220+
action := fakeClient.Actions()[1]
221+
if !action.Matches("update", "clusterroles") {
222+
t.Fatalf("unexpected action %#v", action)
223+
}
224+
updateAction, ok := action.(clienttesting.UpdateAction)
225+
if !ok {
226+
t.Fatalf("unexpected action %#v", action)
227+
}
228+
if !equality.Semantic.DeepEqual(updateAction.GetObject().(*rbacv1.ClusterRole), test.expectedClusterRole) {
229+
t.Fatalf("%v", diff.ObjectDiff(test.expectedClusterRole, updateAction.GetObject().(*rbacv1.ClusterRole)))
230+
}
231+
}
232+
})
233+
}
181234
}
182235
}

0 commit comments

Comments
 (0)