Skip to content

Commit 529d71f

Browse files
ArvindthiruArvindthiru
authored
feat: deny update/delete operations on tolerations (#679)
1 parent 6fd006e commit 529d71f

File tree

6 files changed

+236
-12
lines changed

6 files changed

+236
-12
lines changed

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ metadata:
6262
name: kind-cluster-1
6363
spec:
6464
identity:
65-
name: hub-agent-sa
65+
name: fleet-member-agent-cluster-1
6666
kind: ServiceAccount
6767
namespace: fleet-system
6868
apiGroup: ""

‎examples/fleet_v1beta1_membercluster.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ metadata:
44
name: kind-cluster-1
55
spec:
66
identity:
7-
name: hub-agent-sa
7+
name: fleet-member-agent-cluster-1
88
kind: ServiceAccount
99
namespace: fleet-system
1010
apiGroup: ""

‎pkg/utils/validator/clusterresourceplacement.go‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -251,14 +251,27 @@ func validateTolerations(tolerations []placementv1beta1.Toleration) error {
251251
}
252252
}
253253
}
254-
if _, exists := tolerationMap[toleration]; exists {
254+
if tolerationMap[toleration] {
255255
allErr = append(allErr, fmt.Errorf(uniqueTolerationErrFmt, toleration))
256256
}
257257
tolerationMap[toleration] = true
258258
}
259259
return apiErrors.NewAggregate(allErr)
260260
}
261261

262+
func IsTolerationsUpdatedOrDeleted(oldTolerations []placementv1beta1.Toleration, newTolerations []placementv1beta1.Toleration) bool {
263+
newTolerationsMap := make(map[placementv1beta1.Toleration]bool)
264+
for _, newToleration := range newTolerations {
265+
newTolerationsMap[newToleration] = true
266+
}
267+
for _, oldToleration := range oldTolerations {
268+
if !newTolerationsMap[oldToleration] {
269+
return true
270+
}
271+
}
272+
return false
273+
}
274+
262275
func validateTopologySpreadConstraints(topologyConstraints []placementv1beta1.TopologySpreadConstraint) error {
263276
allErr := make([]error, 0)
264277
for _, tc := range topologyConstraints {

‎pkg/utils/validator/clusterresourceplacement_test.go‎

Lines changed: 165 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ Licensed under the MIT license.
66
package validator
77

88
import (
9-
"regexp"
9+
"strings"
1010
"testing"
1111

1212
corev1 "k8s.io/api/core/v1"
@@ -1163,14 +1163,170 @@ func TestValidateTolerations(t *testing.T) {
11631163
if (gotErr != nil) != testCase.wantErr {
11641164
t.Errorf("validateTolerations() error = %v, wantErr %v", gotErr, testCase.wantErr)
11651165
}
1166-
if testCase.wantErr {
1167-
match, err := regexp.MatchString(testCase.wantErrMsg, gotErr.Error())
1168-
if err != nil {
1169-
t.Errorf("validateTolerations() failed to compile pattern: %s", testCase.wantErrMsg)
1170-
}
1171-
if !match {
1172-
t.Errorf("validateTolerations() failed to find expected error message = %s, in error = %s", testCase.wantErrMsg, gotErr.Error())
1173-
}
1166+
if testCase.wantErr && !strings.Contains(gotErr.Error(), testCase.wantErrMsg) {
1167+
t.Errorf("validateTolerations() failed to find expected error message = %s, in error = %s", testCase.wantErrMsg, gotErr.Error())
1168+
}
1169+
})
1170+
}
1171+
}
1172+
1173+
func TestIsTolerationsUpdatedOrDeleted(t *testing.T) {
1174+
tests := map[string]struct {
1175+
oldTolerations []placementv1beta1.Toleration
1176+
newTolerations []placementv1beta1.Toleration
1177+
want bool
1178+
}{
1179+
"old tolerations is nil": {
1180+
newTolerations: []placementv1beta1.Toleration{
1181+
{
1182+
Key: "key1",
1183+
Operator: corev1.TolerationOpEqual,
1184+
Value: "value1",
1185+
Effect: corev1.TaintEffectNoSchedule,
1186+
},
1187+
{
1188+
Key: "key2",
1189+
Operator: corev1.TolerationOpExists,
1190+
Effect: corev1.TaintEffectNoSchedule,
1191+
},
1192+
},
1193+
want: false,
1194+
},
1195+
"new tolerations is nil": {
1196+
oldTolerations: []placementv1beta1.Toleration{
1197+
{
1198+
Key: "key1",
1199+
Operator: corev1.TolerationOpEqual,
1200+
Value: "value1",
1201+
Effect: corev1.TaintEffectNoSchedule,
1202+
},
1203+
{
1204+
Key: "key2",
1205+
Operator: corev1.TolerationOpExists,
1206+
Effect: corev1.TaintEffectNoSchedule,
1207+
},
1208+
},
1209+
want: true,
1210+
},
1211+
"one toleration was updated in new tolerations": {
1212+
oldTolerations: []placementv1beta1.Toleration{
1213+
{
1214+
Key: "key1",
1215+
Operator: corev1.TolerationOpEqual,
1216+
Value: "value1",
1217+
Effect: corev1.TaintEffectNoSchedule,
1218+
},
1219+
{
1220+
Key: "key2",
1221+
Operator: corev1.TolerationOpExists,
1222+
Effect: corev1.TaintEffectNoSchedule,
1223+
},
1224+
},
1225+
newTolerations: []placementv1beta1.Toleration{
1226+
{
1227+
Key: "key1",
1228+
Operator: corev1.TolerationOpEqual,
1229+
Value: "value1",
1230+
Effect: corev1.TaintEffectNoSchedule,
1231+
},
1232+
{
1233+
Key: "key3",
1234+
Operator: corev1.TolerationOpExists,
1235+
Effect: corev1.TaintEffectNoSchedule,
1236+
},
1237+
},
1238+
want: true,
1239+
},
1240+
"one toleration was deleted in new tolerations": {
1241+
oldTolerations: []placementv1beta1.Toleration{
1242+
{
1243+
Key: "key1",
1244+
Operator: corev1.TolerationOpEqual,
1245+
Value: "value1",
1246+
Effect: corev1.TaintEffectNoSchedule,
1247+
},
1248+
{
1249+
Key: "key2",
1250+
Operator: corev1.TolerationOpExists,
1251+
Effect: corev1.TaintEffectNoSchedule,
1252+
},
1253+
},
1254+
newTolerations: []placementv1beta1.Toleration{
1255+
{
1256+
Key: "key1",
1257+
Operator: corev1.TolerationOpEqual,
1258+
Value: "value1",
1259+
Effect: corev1.TaintEffectNoSchedule,
1260+
},
1261+
},
1262+
want: true,
1263+
},
1264+
"old tolerations, new tolerations are same": {
1265+
oldTolerations: []placementv1beta1.Toleration{
1266+
{
1267+
Key: "key1",
1268+
Operator: corev1.TolerationOpEqual,
1269+
Value: "value1",
1270+
Effect: corev1.TaintEffectNoSchedule,
1271+
},
1272+
{
1273+
Key: "key2",
1274+
Operator: corev1.TolerationOpExists,
1275+
Effect: corev1.TaintEffectNoSchedule,
1276+
},
1277+
},
1278+
newTolerations: []placementv1beta1.Toleration{
1279+
{
1280+
Key: "key1",
1281+
Operator: corev1.TolerationOpEqual,
1282+
Value: "value1",
1283+
Effect: corev1.TaintEffectNoSchedule,
1284+
},
1285+
{
1286+
Key: "key2",
1287+
Operator: corev1.TolerationOpExists,
1288+
Effect: corev1.TaintEffectNoSchedule,
1289+
},
1290+
},
1291+
want: false,
1292+
},
1293+
"a toleration was added to new tolerations": {
1294+
oldTolerations: []placementv1beta1.Toleration{
1295+
{
1296+
Key: "key1",
1297+
Operator: corev1.TolerationOpEqual,
1298+
Value: "value1",
1299+
Effect: corev1.TaintEffectNoSchedule,
1300+
},
1301+
{
1302+
Key: "key2",
1303+
Operator: corev1.TolerationOpExists,
1304+
Effect: corev1.TaintEffectNoSchedule,
1305+
},
1306+
},
1307+
newTolerations: []placementv1beta1.Toleration{
1308+
{
1309+
Key: "key1",
1310+
Operator: corev1.TolerationOpEqual,
1311+
Value: "value1",
1312+
Effect: corev1.TaintEffectNoSchedule,
1313+
},
1314+
{
1315+
Key: "key2",
1316+
Operator: corev1.TolerationOpExists,
1317+
Effect: corev1.TaintEffectNoSchedule,
1318+
},
1319+
{
1320+
Operator: corev1.TolerationOpExists,
1321+
},
1322+
},
1323+
want: false,
1324+
},
1325+
}
1326+
for testName, testCase := range tests {
1327+
t.Run(testName, func(t *testing.T) {
1328+
if got := IsTolerationsUpdatedOrDeleted(testCase.oldTolerations, testCase.newTolerations); got != testCase.want {
1329+
t.Errorf("IsTolerationsUpdatedOrDeleted() got = %v, want = %v", got, testCase.want)
11741330
}
11751331
})
11761332
}

‎pkg/webhook/clusterresourceplacement/v1beta1_clusterresourceplacement_validating_webhook.go‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,12 +55,23 @@ func (v *clusterResourcePlacementValidator) Handle(_ context.Context, req admiss
5555
if validator.IsPlacementPolicyTypeUpdated(oldCRP.Spec.Policy, crp.Spec.Policy) {
5656
return admission.Denied("placement type is immutable")
5757
}
58+
// handle update case where existing tolerations were updated/deleted
59+
if validator.IsTolerationsUpdatedOrDeleted(getTolerations(oldCRP.Spec.Policy), getTolerations(crp.Spec.Policy)) {
60+
return admission.Denied("tolerations have been updated/deleted, only additions to tolerations are allowed")
61+
}
5862
}
5963
}
6064
klog.V(2).InfoS("user is allowed to modify v1beta1 cluster resource placement", "operation", req.Operation, "user", req.UserInfo.Username, "group", req.UserInfo.Groups, "namespacedName", types.NamespacedName{Name: crp.Name})
6165
return admission.Allowed("any user is allowed to modify v1beta1 CRP")
6266
}
6367

68+
func getTolerations(policy *placementv1beta1.PlacementPolicy) []placementv1beta1.Toleration {
69+
if policy != nil {
70+
return policy.Tolerations
71+
}
72+
return nil
73+
}
74+
6475
// InjectDecoder injects the decoder.
6576
func (v *clusterResourcePlacementValidator) InjectDecoder(d *admission.Decoder) error {
6677
v.decoder = d

‎test/e2e/webhook_test.go‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -285,4 +285,48 @@ var _ = Describe("webhook tests for CRP tolerations", Ordered, func() {
285285
return nil
286286
}, testutils.PollTimeout, testutils.PollInterval).Should(Succeed())
287287
})
288+
289+
It("should deny update on CRP with update to existing toleration", func() {
290+
Eventually(func(g Gomega) error {
291+
var crp placementv1beta1.ClusterResourcePlacement
292+
g.Expect(hubClient.Get(ctx, types.NamespacedName{Name: crpName}, &crp)).Should(Succeed())
293+
newTolerations := []placementv1beta1.Toleration{
294+
{
295+
Key: "key1",
296+
Operator: corev1.TolerationOpEqual,
297+
Value: "value1",
298+
Effect: corev1.TaintEffectNoSchedule,
299+
},
300+
{
301+
Key: "key3",
302+
Operator: corev1.TolerationOpExists,
303+
Effect: corev1.TaintEffectNoSchedule,
304+
},
305+
}
306+
crp.Spec.Policy.Tolerations = newTolerations
307+
err := hubClient.Update(ctx, &crp)
308+
if k8sErrors.IsConflict(err) {
309+
return err
310+
}
311+
var statusErr *k8sErrors.StatusError
312+
g.Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Update CRP call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8sErrors.StatusError{})))
313+
Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("tolerations have been updated/deleted, only additions to tolerations are allowed"))
314+
return nil
315+
}, testutils.PollTimeout, testutils.PollInterval).Should(Succeed())
316+
})
317+
318+
It("should allow update on CRP with adding a new toleration", func() {
319+
Eventually(func(g Gomega) error {
320+
var crp placementv1beta1.ClusterResourcePlacement
321+
g.Expect(hubClient.Get(ctx, types.NamespacedName{Name: crpName}, &crp)).Should(Succeed())
322+
newToleration := placementv1beta1.Toleration{
323+
Key: "key3",
324+
Operator: corev1.TolerationOpEqual,
325+
Value: "value3",
326+
Effect: corev1.TaintEffectNoSchedule,
327+
}
328+
crp.Spec.Policy.Tolerations = append(crp.Spec.Policy.Tolerations, newToleration)
329+
return hubClient.Update(ctx, &crp)
330+
}, testutils.PollTimeout, testutils.PollInterval).Should(Succeed())
331+
})
288332
})

0 commit comments

Comments
 (0)