Skip to content

Commit

Permalink
feat: deny update/delete operations on tolerations (#679)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Arvindthiru
Arvindthiru authored Feb 23, 2024
1 parent 6fd006e commit 529d71f
Show file tree
Hide file tree
Showing 6 changed files with 236 additions and 12 deletions.
2 changes: 1 addition & 1 deletion README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ metadata:
name: kind-cluster-1
spec:
identity:
name: hub-agent-sa
name: fleet-member-agent-cluster-1
kind: ServiceAccount
namespace: fleet-system
apiGroup: ""
Expand Down
2 changes: 1 addition & 1 deletion examples/fleet_v1beta1_membercluster.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ metadata:
name: kind-cluster-1
spec:
identity:
name: hub-agent-sa
name: fleet-member-agent-cluster-1
kind: ServiceAccount
namespace: fleet-system
apiGroup: ""
15 changes: 14 additions & 1 deletion pkg/utils/validator/clusterresourceplacement.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,14 +251,27 @@ func validateTolerations(tolerations []placementv1beta1.Toleration) error {
}
}
}
if _, exists := tolerationMap[toleration]; exists {
if tolerationMap[toleration] {
allErr = append(allErr, fmt.Errorf(uniqueTolerationErrFmt, toleration))
}
tolerationMap[toleration] = true
}
return apiErrors.NewAggregate(allErr)
}

func IsTolerationsUpdatedOrDeleted(oldTolerations []placementv1beta1.Toleration, newTolerations []placementv1beta1.Toleration) bool {
newTolerationsMap := make(map[placementv1beta1.Toleration]bool)
for _, newToleration := range newTolerations {
newTolerationsMap[newToleration] = true
}
for _, oldToleration := range oldTolerations {
if !newTolerationsMap[oldToleration] {
return true
}
}
return false
}

func validateTopologySpreadConstraints(topologyConstraints []placementv1beta1.TopologySpreadConstraint) error {
allErr := make([]error, 0)
for _, tc := range topologyConstraints {
Expand Down
174 changes: 165 additions & 9 deletions pkg/utils/validator/clusterresourceplacement_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ Licensed under the MIT license.
package validator

import (
"regexp"
"strings"
"testing"

corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -1163,14 +1163,170 @@ func TestValidateTolerations(t *testing.T) {
if (gotErr != nil) != testCase.wantErr {
t.Errorf("validateTolerations() error = %v, wantErr %v", gotErr, testCase.wantErr)
}
if testCase.wantErr {
match, err := regexp.MatchString(testCase.wantErrMsg, gotErr.Error())
if err != nil {
t.Errorf("validateTolerations() failed to compile pattern: %s", testCase.wantErrMsg)
}
if !match {
t.Errorf("validateTolerations() failed to find expected error message = %s, in error = %s", testCase.wantErrMsg, gotErr.Error())
}
if testCase.wantErr && !strings.Contains(gotErr.Error(), testCase.wantErrMsg) {
t.Errorf("validateTolerations() failed to find expected error message = %s, in error = %s", testCase.wantErrMsg, gotErr.Error())
}
})
}
}

func TestIsTolerationsUpdatedOrDeleted(t *testing.T) {
tests := map[string]struct {
oldTolerations []placementv1beta1.Toleration
newTolerations []placementv1beta1.Toleration
want bool
}{
"old tolerations is nil": {
newTolerations: []placementv1beta1.Toleration{
{
Key: "key1",
Operator: corev1.TolerationOpEqual,
Value: "value1",
Effect: corev1.TaintEffectNoSchedule,
},
{
Key: "key2",
Operator: corev1.TolerationOpExists,
Effect: corev1.TaintEffectNoSchedule,
},
},
want: false,
},
"new tolerations is nil": {
oldTolerations: []placementv1beta1.Toleration{
{
Key: "key1",
Operator: corev1.TolerationOpEqual,
Value: "value1",
Effect: corev1.TaintEffectNoSchedule,
},
{
Key: "key2",
Operator: corev1.TolerationOpExists,
Effect: corev1.TaintEffectNoSchedule,
},
},
want: true,
},
"one toleration was updated in new tolerations": {
oldTolerations: []placementv1beta1.Toleration{
{
Key: "key1",
Operator: corev1.TolerationOpEqual,
Value: "value1",
Effect: corev1.TaintEffectNoSchedule,
},
{
Key: "key2",
Operator: corev1.TolerationOpExists,
Effect: corev1.TaintEffectNoSchedule,
},
},
newTolerations: []placementv1beta1.Toleration{
{
Key: "key1",
Operator: corev1.TolerationOpEqual,
Value: "value1",
Effect: corev1.TaintEffectNoSchedule,
},
{
Key: "key3",
Operator: corev1.TolerationOpExists,
Effect: corev1.TaintEffectNoSchedule,
},
},
want: true,
},
"one toleration was deleted in new tolerations": {
oldTolerations: []placementv1beta1.Toleration{
{
Key: "key1",
Operator: corev1.TolerationOpEqual,
Value: "value1",
Effect: corev1.TaintEffectNoSchedule,
},
{
Key: "key2",
Operator: corev1.TolerationOpExists,
Effect: corev1.TaintEffectNoSchedule,
},
},
newTolerations: []placementv1beta1.Toleration{
{
Key: "key1",
Operator: corev1.TolerationOpEqual,
Value: "value1",
Effect: corev1.TaintEffectNoSchedule,
},
},
want: true,
},
"old tolerations, new tolerations are same": {
oldTolerations: []placementv1beta1.Toleration{
{
Key: "key1",
Operator: corev1.TolerationOpEqual,
Value: "value1",
Effect: corev1.TaintEffectNoSchedule,
},
{
Key: "key2",
Operator: corev1.TolerationOpExists,
Effect: corev1.TaintEffectNoSchedule,
},
},
newTolerations: []placementv1beta1.Toleration{
{
Key: "key1",
Operator: corev1.TolerationOpEqual,
Value: "value1",
Effect: corev1.TaintEffectNoSchedule,
},
{
Key: "key2",
Operator: corev1.TolerationOpExists,
Effect: corev1.TaintEffectNoSchedule,
},
},
want: false,
},
"a toleration was added to new tolerations": {
oldTolerations: []placementv1beta1.Toleration{
{
Key: "key1",
Operator: corev1.TolerationOpEqual,
Value: "value1",
Effect: corev1.TaintEffectNoSchedule,
},
{
Key: "key2",
Operator: corev1.TolerationOpExists,
Effect: corev1.TaintEffectNoSchedule,
},
},
newTolerations: []placementv1beta1.Toleration{
{
Key: "key1",
Operator: corev1.TolerationOpEqual,
Value: "value1",
Effect: corev1.TaintEffectNoSchedule,
},
{
Key: "key2",
Operator: corev1.TolerationOpExists,
Effect: corev1.TaintEffectNoSchedule,
},
{
Operator: corev1.TolerationOpExists,
},
},
want: false,
},
}
for testName, testCase := range tests {
t.Run(testName, func(t *testing.T) {
if got := IsTolerationsUpdatedOrDeleted(testCase.oldTolerations, testCase.newTolerations); got != testCase.want {
t.Errorf("IsTolerationsUpdatedOrDeleted() got = %v, want = %v", got, testCase.want)
}
})
}
Expand Down
11 changes: 11 additions & 0 deletions pkg/webhook/clusterresourceplacement/v1beta1_clusterresourceplacement_validating_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,23 @@ func (v *clusterResourcePlacementValidator) Handle(_ context.Context, req admiss
if validator.IsPlacementPolicyTypeUpdated(oldCRP.Spec.Policy, crp.Spec.Policy) {
return admission.Denied("placement type is immutable")
}
// handle update case where existing tolerations were updated/deleted
if validator.IsTolerationsUpdatedOrDeleted(getTolerations(oldCRP.Spec.Policy), getTolerations(crp.Spec.Policy)) {
return admission.Denied("tolerations have been updated/deleted, only additions to tolerations are allowed")
}
}
}
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})
return admission.Allowed("any user is allowed to modify v1beta1 CRP")
}

func getTolerations(policy *placementv1beta1.PlacementPolicy) []placementv1beta1.Toleration {
if policy != nil {
return policy.Tolerations
}
return nil
}

// InjectDecoder injects the decoder.
func (v *clusterResourcePlacementValidator) InjectDecoder(d *admission.Decoder) error {
v.decoder = d
Expand Down
44 changes: 44 additions & 0 deletions test/e2e/webhook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -285,4 +285,48 @@ var _ = Describe("webhook tests for CRP tolerations", Ordered, func() {
return nil
}, testutils.PollTimeout, testutils.PollInterval).Should(Succeed())
})

It("should deny update on CRP with update to existing toleration", func() {
Eventually(func(g Gomega) error {
var crp placementv1beta1.ClusterResourcePlacement
g.Expect(hubClient.Get(ctx, types.NamespacedName{Name: crpName}, &crp)).Should(Succeed())
newTolerations := []placementv1beta1.Toleration{
{
Key: "key1",
Operator: corev1.TolerationOpEqual,
Value: "value1",
Effect: corev1.TaintEffectNoSchedule,
},
{
Key: "key3",
Operator: corev1.TolerationOpExists,
Effect: corev1.TaintEffectNoSchedule,
},
}
crp.Spec.Policy.Tolerations = newTolerations
err := hubClient.Update(ctx, &crp)
if k8sErrors.IsConflict(err) {
return err
}
var statusErr *k8sErrors.StatusError
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{})))
Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("tolerations have been updated/deleted, only additions to tolerations are allowed"))
return nil
}, testutils.PollTimeout, testutils.PollInterval).Should(Succeed())
})

It("should allow update on CRP with adding a new toleration", func() {
Eventually(func(g Gomega) error {
var crp placementv1beta1.ClusterResourcePlacement
g.Expect(hubClient.Get(ctx, types.NamespacedName{Name: crpName}, &crp)).Should(Succeed())
newToleration := placementv1beta1.Toleration{
Key: "key3",
Operator: corev1.TolerationOpEqual,
Value: "value3",
Effect: corev1.TaintEffectNoSchedule,
}
crp.Spec.Policy.Tolerations = append(crp.Spec.Policy.Tolerations, newToleration)
return hubClient.Update(ctx, &crp)
}, testutils.PollTimeout, testutils.PollInterval).Should(Succeed())
})
})

0 comments on commit 529d71f

Please sign in to comment.