Skip to content

Commit fe0f3f3

Browse files
audrastumpaudrastump
authored
feat: block member label modification through DP (#36)
1 parent afb85c4 commit fe0f3f3

File tree

9 files changed

+467
-34
lines changed

9 files changed

+467
-34
lines changed

‎cmd/hubagent/main.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@ func main() {
171171

172172
if opts.EnableWebhook {
173173
whiteListedUsers := strings.Split(opts.WhiteListedUsers, ",")
174-
if err := SetupWebhook(mgr, options.WebhookClientConnectionType(opts.WebhookClientConnectionType), opts.WebhookServiceName, whiteListedUsers, opts.EnableGuardRail, opts.EnableV1Beta1APIs); err != nil {
174+
if err := SetupWebhook(mgr, options.WebhookClientConnectionType(opts.WebhookClientConnectionType), opts.WebhookServiceName, whiteListedUsers, opts.EnableGuardRail, opts.EnableV1Beta1APIs, opts.DenyModifyMemberClusterLabels); err != nil {
175175
klog.ErrorS(err, "unable to set up webhook")
176176
exitWithErrorFunc()
177177
}
@@ -203,9 +203,9 @@ func main() {
203203
}
204204

205205
// SetupWebhook generates the webhook cert and then set up the webhook configurator.
206-
func SetupWebhook(mgr manager.Manager, webhookClientConnectionType options.WebhookClientConnectionType, webhookServiceName string, whiteListedUsers []string, enableGuardRail, isFleetV1Beta1API bool) error {
206+
func SetupWebhook(mgr manager.Manager, webhookClientConnectionType options.WebhookClientConnectionType, webhookServiceName string, whiteListedUsers []string, enableGuardRail, isFleetV1Beta1API bool, denyModifyMemberClusterLabels bool) error {
207207
// Generate self-signed key and crt files in FleetWebhookCertDir for the webhook server to start.
208-
w, err := webhook.NewWebhookConfig(mgr, webhookServiceName, FleetWebhookPort, &webhookClientConnectionType, FleetWebhookCertDir, enableGuardRail)
208+
w, err := webhook.NewWebhookConfig(mgr, webhookServiceName, FleetWebhookPort, &webhookClientConnectionType, FleetWebhookCertDir, enableGuardRail, denyModifyMemberClusterLabels)
209209
if err != nil {
210210
klog.ErrorS(err, "fail to generate WebhookConfig")
211211
return err
@@ -214,7 +214,7 @@ func SetupWebhook(mgr manager.Manager, webhookClientConnectionType options.Webho
214214
klog.ErrorS(err, "unable to add WebhookConfig")
215215
return err
216216
}
217-
if err = webhook.AddToManager(mgr, whiteListedUsers, isFleetV1Beta1API); err != nil {
217+
if err = webhook.AddToManager(mgr, whiteListedUsers, isFleetV1Beta1API, denyModifyMemberClusterLabels); err != nil {
218218
klog.ErrorS(err, "unable to register webhooks to the manager")
219219
return err
220220
}

‎cmd/hubagent/options/options.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,8 @@ type Options struct {
9898
EnableStagedUpdateRunAPIs bool
9999
// EnableEvictionAPIs enables to agents to watch the eviction and placement disruption budget CRs.
100100
EnableEvictionAPIs bool
101+
// DenyModifyMemberClusterLabels indicates if the member cluster labels cannot be modified by groups (excluding system:masters)
102+
DenyModifyMemberClusterLabels bool
101103
}
102104

103105
// NewOptions builds an empty options.
@@ -158,6 +160,7 @@ func (o *Options) AddFlags(flags *flag.FlagSet) {
158160
flags.DurationVar(&o.ForceDeleteWaitTime.Duration, "force-delete-wait-time", 15*time.Minute, "The duration the hub agent waits before force deleting a member cluster.")
159161
flags.BoolVar(&o.EnableStagedUpdateRunAPIs, "enable-staged-update-run-apis", true, "If set, the agents will watch for the ClusterStagedUpdateRun APIs.")
160162
flags.BoolVar(&o.EnableEvictionAPIs, "enable-eviction-apis", true, "If set, the agents will watch for the Eviction and PlacementDisruptionBudget APIs.")
163+
flags.BoolVar(&o.DenyModifyMemberClusterLabels, "deny-modify-member-cluster-labels", false, "If set, users not in the system:masters cannot modify member cluster labels.")
161164

162165
o.RateLimiterOpts.AddFlags(flags)
163166
}

‎cmd/hubagent/options/validation_test.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,12 @@ limitations under the License.
1717
package options
1818

1919
import (
20+
"flag"
2021
"testing"
2122
"time"
2223

2324
"github.com/google/go-cmp/cmp"
25+
"github.com/onsi/gomega"
2426
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2527
"k8s.io/apimachinery/pkg/util/validation/field"
2628
)
@@ -102,3 +104,13 @@ func TestValidateControllerManagerConfiguration(t *testing.T) {
102104
})
103105
}
104106
}
107+
108+
func TestAddFlags(t *testing.T) {
109+
g := gomega.NewWithT(t)
110+
opts := NewOptions()
111+
112+
flags := flag.NewFlagSet("deny-modify-member-cluster-labels", flag.ExitOnError)
113+
opts.AddFlags(flags)
114+
115+
g.Expect(opts.DenyModifyMemberClusterLabels).To(gomega.BeFalse(), "deny-modify-member-cluster-labels should be false by default")
116+
}

‎pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -35,23 +35,25 @@ const (
3535
)
3636

3737
// Add registers the webhook for K8s built-in object types.
38-
func Add(mgr manager.Manager, whiteListedUsers []string, isFleetV1Beta1API bool) error {
38+
func Add(mgr manager.Manager, whiteListedUsers []string, isFleetV1Beta1API bool, denyModifyMemberClusterLabels bool) error {
3939
hookServer := mgr.GetWebhookServer()
4040
handler := &fleetResourceValidator{
41-
client: mgr.GetClient(),
42-
whiteListedUsers: whiteListedUsers,
43-
isFleetV1Beta1API: isFleetV1Beta1API,
44-
decoder: admission.NewDecoder(mgr.GetScheme()),
41+
client: mgr.GetClient(),
42+
whiteListedUsers: whiteListedUsers,
43+
isFleetV1Beta1API: isFleetV1Beta1API,
44+
decoder: admission.NewDecoder(mgr.GetScheme()),
45+
denyModifyMemberClusterLabels: denyModifyMemberClusterLabels,
4546
}
4647
hookServer.Register(ValidationPath, &webhook.Admission{Handler: handler})
4748
return nil
4849
}
4950

5051
type fleetResourceValidator struct {
51-
client client.Client
52-
whiteListedUsers []string
53-
isFleetV1Beta1API bool
54-
decoder webhook.AdmissionDecoder
52+
client client.Client
53+
whiteListedUsers []string
54+
isFleetV1Beta1API bool
55+
decoder webhook.AdmissionDecoder
56+
denyModifyMemberClusterLabels bool
5557
}
5658

5759
// Handle receives the request then allows/denies the request to modify fleet resources.
@@ -133,7 +135,7 @@ func (v *fleetResourceValidator) handleMemberCluster(req admission.Request) admi
133135
}
134136
isFleetMC := utils.IsFleetAnnotationPresent(oldMC.Annotations)
135137
if isFleetMC {
136-
return validation.ValidateFleetMemberClusterUpdate(currentMC, oldMC, req, v.whiteListedUsers)
138+
return validation.ValidateFleetMemberClusterUpdate(currentMC, oldMC, req, v.whiteListedUsers, v.denyModifyMemberClusterLabels)
137139
}
138140
return validation.ValidatedUpstreamMemberClusterUpdate(currentMC, oldMC, req, v.whiteListedUsers)
139141
}

‎pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook_test.go‎

Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -634,6 +634,189 @@ func TestHandleMemberCluster(t *testing.T) {
634634
},
635635
wantResponse: admission.Allowed(fmt.Sprintf(validation.ResourceAllowedFormat, "aks-support", utils.GenerateGroupString([]string{"system:authenticated"}), admissionv1.Delete, &utils.MCMetaGVK, "", types.NamespacedName{Name: "test-mc"})),
636636
},
637+
638+
"allow label modification by system:masters when flag is set to true": {
639+
req: admission.Request{
640+
AdmissionRequest: admissionv1.AdmissionRequest{
641+
Name: "test-mc",
642+
Object: runtime.RawExtension{
643+
Raw: func() []byte {
644+
updatedMC := &clusterv1beta1.MemberCluster{
645+
ObjectMeta: metav1.ObjectMeta{
646+
Name: "test-mc",
647+
Labels: map[string]string{"key1": "value1"},
648+
Annotations: map[string]string{
649+
"fleet.azure.com/cluster-resource-id": "test-cluster-resource-id",
650+
},
651+
},
652+
}
653+
raw, _ := json.Marshal(updatedMC)
654+
return raw
655+
}(),
656+
},
657+
OldObject: runtime.RawExtension{
658+
Raw: fleetMCObjectBytes,
659+
},
660+
UserInfo: authenticationv1.UserInfo{
661+
Username: "mastersUser",
662+
Groups: []string{"system:masters"},
663+
},
664+
RequestKind: &utils.MCMetaGVK,
665+
Operation: admissionv1.Update,
666+
},
667+
},
668+
resourceValidator: fleetResourceValidator{
669+
decoder: decoder,
670+
denyModifyMemberClusterLabels: true,
671+
},
672+
wantResponse: admission.Allowed(fmt.Sprintf(validation.ResourceAllowedFormat, "mastersUser", utils.GenerateGroupString([]string{"system:masters"}), admissionv1.Update, &utils.MCMetaGVK, "", types.NamespacedName{Name: "test-mc"})),
673+
},
674+
"deny label modification by non system:masters user when flag is set to true": {
675+
req: admission.Request{
676+
AdmissionRequest: admissionv1.AdmissionRequest{
677+
Name: "test-mc",
678+
Object: runtime.RawExtension{
679+
Raw: func() []byte {
680+
updatedMC := &clusterv1beta1.MemberCluster{
681+
ObjectMeta: metav1.ObjectMeta{
682+
Name: "test-mc",
683+
Labels: map[string]string{"key1": "value1"},
684+
Annotations: map[string]string{
685+
"fleet.azure.com/cluster-resource-id": "test-cluster-resource-id",
686+
},
687+
},
688+
}
689+
raw, _ := json.Marshal(updatedMC)
690+
return raw
691+
}(),
692+
},
693+
OldObject: runtime.RawExtension{
694+
Raw: func() []byte {
695+
oldMC := &clusterv1beta1.MemberCluster{
696+
ObjectMeta: metav1.ObjectMeta{
697+
Name: "test-mc",
698+
Labels: map[string]string{"key1": "value2"},
699+
Annotations: map[string]string{
700+
"fleet.azure.com/cluster-resource-id": "test-cluster-resource-id",
701+
},
702+
},
703+
}
704+
raw, _ := json.Marshal(oldMC)
705+
return raw
706+
}(),
707+
},
708+
UserInfo: authenticationv1.UserInfo{
709+
Username: "nonSystemMastersUser",
710+
Groups: []string{"system:authenticated"},
711+
},
712+
RequestKind: &utils.MCMetaGVK,
713+
Operation: admissionv1.Update,
714+
},
715+
},
716+
resourceValidator: fleetResourceValidator{
717+
decoder: decoder,
718+
denyModifyMemberClusterLabels: true,
719+
},
720+
wantResponse: admission.Denied(fmt.Sprintf(validation.DeniedModifyMemberClusterLabels)),
721+
},
722+
"deny label modification by fleet agent when flag is set to true": {
723+
req: admission.Request{
724+
AdmissionRequest: admissionv1.AdmissionRequest{
725+
Name: "test-mc",
726+
Object: runtime.RawExtension{
727+
Raw: func() []byte {
728+
updatedMC := &clusterv1beta1.MemberCluster{
729+
ObjectMeta: metav1.ObjectMeta{
730+
Name: "test-mc",
731+
Labels: map[string]string{"key1": "value1"},
732+
Annotations: map[string]string{
733+
"fleet.azure.com/cluster-resource-id": "test-cluster-resource-id",
734+
},
735+
},
736+
}
737+
raw, _ := json.Marshal(updatedMC)
738+
return raw
739+
}(),
740+
},
741+
OldObject: runtime.RawExtension{
742+
Raw: func() []byte {
743+
oldMC := &clusterv1beta1.MemberCluster{
744+
ObjectMeta: metav1.ObjectMeta{
745+
Name: "test-mc",
746+
Labels: map[string]string{"key1": "value2"},
747+
Annotations: map[string]string{
748+
"fleet.azure.com/cluster-resource-id": "test-cluster-resource-id",
749+
},
750+
},
751+
}
752+
raw, _ := json.Marshal(oldMC)
753+
return raw
754+
}(),
755+
},
756+
UserInfo: authenticationv1.UserInfo{
757+
Username: "system:serviceaccount:fleet-system:hub-agent-sa",
758+
Groups: []string{"system:serviceaccounts"},
759+
},
760+
RequestKind: &utils.MCMetaGVK,
761+
Operation: admissionv1.Update,
762+
},
763+
},
764+
resourceValidator: fleetResourceValidator{
765+
decoder: decoder,
766+
denyModifyMemberClusterLabels: true,
767+
},
768+
wantResponse: admission.Denied(fmt.Sprintf(validation.DeniedModifyMemberClusterLabels)),
769+
},
770+
"allow label modification by non system:masters resources when flag is set to false": {
771+
req: admission.Request{
772+
AdmissionRequest: admissionv1.AdmissionRequest{
773+
Name: "test-mc",
774+
Object: runtime.RawExtension{
775+
Raw: func() []byte {
776+
updatedMC := &clusterv1beta1.MemberCluster{
777+
ObjectMeta: metav1.ObjectMeta{
778+
Name: "test-mc",
779+
Labels: map[string]string{"key1": "value1"},
780+
Annotations: map[string]string{
781+
"fleet.azure.com/cluster-resource-id": "test-cluster-resource-id",
782+
},
783+
},
784+
}
785+
raw, _ := json.Marshal(updatedMC)
786+
return raw
787+
}(),
788+
},
789+
OldObject: runtime.RawExtension{
790+
Raw: func() []byte {
791+
oldMC := &clusterv1beta1.MemberCluster{
792+
ObjectMeta: metav1.ObjectMeta{
793+
Name: "test-mc",
794+
Labels: map[string]string{"key1": "value2"},
795+
Annotations: map[string]string{
796+
"fleet.azure.com/cluster-resource-id": "test-cluster-resource-id",
797+
},
798+
},
799+
}
800+
raw, _ := json.Marshal(oldMC)
801+
return raw
802+
}(),
803+
},
804+
UserInfo: authenticationv1.UserInfo{
805+
Username: "nonMastersUser",
806+
Groups: []string{"system:authenticated"},
807+
},
808+
RequestKind: &utils.MCMetaGVK,
809+
Operation: admissionv1.Update,
810+
},
811+
},
812+
resourceValidator: fleetResourceValidator{
813+
decoder: decoder,
814+
denyModifyMemberClusterLabels: false,
815+
},
816+
wantResponse: admission.Allowed(fmt.Sprintf(validation.ResourceAllowedFormat,
817+
"nonMastersUser", utils.GenerateGroupString([]string{"system:authenticated"}), admissionv1.Update, &utils.MCMetaGVK, "",
818+
types.NamespacedName{Name: "test-mc"})),
819+
},
637820
}
638821

639822
for testName, testCase := range testCases {

‎pkg/webhook/validation/uservalidation.go‎

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,11 @@ const (
3232
aksSupportUser = "aks-support"
3333
serviceAccountFmt = "system:serviceaccount:fleet-system:%s"
3434

35-
allowedModifyResource = "user in groups is allowed to modify resource"
36-
deniedModifyResource = "user in groups is not allowed to modify resource"
37-
deniedAddFleetAnnotation = "no user is allowed to add a fleet pre-fixed annotation to an upstream member cluster"
38-
deniedRemoveFleetAnnotation = "no user is allowed to remove all fleet pre-fixed annotations from a fleet member cluster"
35+
allowedModifyResource = "user in groups is allowed to modify resource"
36+
deniedModifyResource = "user in groups is not allowed to modify resource"
37+
deniedAddFleetAnnotation = "no user is allowed to add a fleet pre-fixed annotation to an upstream member cluster"
38+
deniedRemoveFleetAnnotation = "no user is allowed to remove all fleet pre-fixed annotations from a fleet member cluster"
39+
DeniedModifyMemberClusterLabels = "users are not allowed to modify labels through hub cluster directly"
3940

4041
ResourceAllowedFormat = "user: '%s' in '%s' is allowed to %s resource %+v/%s: %+v"
4142
ResourceDeniedFormat = "user: '%s' in '%s' is not allowed to %s resource %+v/%s: %+v"
@@ -62,7 +63,7 @@ func ValidateUserForFleetCRD(req admission.Request, whiteListedUsers []string, g
6263
func ValidateUserForResource(req admission.Request, whiteListedUsers []string) admission.Response {
6364
namespacedName := types.NamespacedName{Name: req.Name, Namespace: req.Namespace}
6465
userInfo := req.UserInfo
65-
if isAdminGroupUserOrWhiteListedUser(whiteListedUsers, userInfo) || isUserAuthenticatedServiceAccount(userInfo) || isUserKubeScheduler(userInfo) || isUserKubeControllerManager(userInfo) || isNodeGroupUser(userInfo) || isAKSSupportUser(userInfo) {
66+
if isAdminGroupUserOrWhiteListedUser(whiteListedUsers, userInfo) || isUserAuthenticatedServiceAccount(userInfo) || isUserKubeScheduler(userInfo) || isUserKubeControllerManager(userInfo) || isUserInGroup(userInfo, nodeGroup) || isAKSSupportUser(userInfo) {
6667
klog.V(3).InfoS(allowedModifyResource, "user", userInfo.Username, "groups", userInfo.Groups, "operation", req.Operation, "GVK", req.RequestKind, "subResource", req.SubResource, "namespacedName", namespacedName)
6768
return admission.Allowed(fmt.Sprintf(ResourceAllowedFormat, userInfo.Username, utils.GenerateGroupString(userInfo.Groups), req.Operation, req.RequestKind, req.SubResource, namespacedName))
6869
}
@@ -93,7 +94,7 @@ func ValidateV1Alpha1MemberClusterUpdate(currentMC, oldMC fleetv1alpha1.MemberCl
9394
}
9495

9596
// ValidateFleetMemberClusterUpdate checks to see if user had updated the fleet member cluster resource and allows/denies the request.
96-
func ValidateFleetMemberClusterUpdate(currentMC, oldMC clusterv1beta1.MemberCluster, req admission.Request, whiteListedUsers []string) admission.Response {
97+
func ValidateFleetMemberClusterUpdate(currentMC, oldMC clusterv1beta1.MemberCluster, req admission.Request, whiteListedUsers []string, denyModifyMemberClusterLabels bool) admission.Response {
9798
namespacedName := types.NamespacedName{Name: currentMC.GetName()}
9899
userInfo := req.UserInfo
99100
if areAllFleetAnnotationsRemoved(currentMC.Annotations, oldMC.Annotations) {
@@ -107,6 +108,17 @@ func ValidateFleetMemberClusterUpdate(currentMC, oldMC clusterv1beta1.MemberClus
107108
if err != nil {
108109
return admission.Denied(err.Error())
109110
}
111+
112+
// Users are no longer allowed to modify labels of fleet member cluster through webhook.
113+
// This will be disabled until member labels are accessible through CLI
114+
if denyModifyMemberClusterLabels {
115+
isLabelUpdated := isMapFieldUpdated(currentMC.GetLabels(), oldMC.GetLabels())
116+
if isLabelUpdated && !isUserInGroup(userInfo, mastersGroup) {
117+
klog.V(2).InfoS(DeniedModifyMemberClusterLabels, "user", userInfo.Username, "groups", userInfo.Groups, "operation", req.Operation, "GVK", req.RequestKind, "subResource", req.SubResource, "namespacedName", namespacedName)
118+
return admission.Denied(DeniedModifyMemberClusterLabels)
119+
}
120+
}
121+
110122
isAnnotationUpdated := isFleetAnnotationUpdated(currentMC.Annotations, oldMC.Annotations)
111123
if isObjUpdated || isAnnotationUpdated {
112124
return ValidateUserForResource(req, whiteListedUsers)
@@ -162,9 +174,9 @@ func isAKSSupportUser(userInfo authenticationv1.UserInfo) bool {
162174
return userInfo.Username == aksSupportUser
163175
}
164176

165-
// isNodeGroupUser returns true if user belongs to system:nodes group.
166-
func isNodeGroupUser(userInfo authenticationv1.UserInfo) bool {
167-
return slices.Contains(userInfo.Groups, nodeGroup)
177+
// isUserInGroup returns true if user belongs to the specified groupName.
178+
func isUserInGroup(userInfo authenticationv1.UserInfo, groupName string) bool {
179+
return slices.Contains(userInfo.Groups, groupName)
168180
}
169181

170182
// isMemberClusterMapFieldUpdated return true if member cluster label is updated.

0 commit comments

Comments
 (0)