Skip to content

Commit 840e808

Browse files
author
Nont
committed
Tighten the VAP rule for all managed resources
Signed-off-by: Nont <[email protected]>
1 parent 42ebac7 commit 840e808

File tree

6 files changed

+406
-291
lines changed

6 files changed

+406
-291
lines changed

‎charts/member-agent-arc/templates/serviceaccount.yaml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
apiVersion: v1
22
kind: ServiceAccount
33
metadata:
4+
# the name is also used in the managed resource validating admission webhook.
5+
# please make the change accordingly when you change the name.
46
name: fleet-member-agent-sa
57
namespace: fleet-system
68
labels:

‎pkg/webhook/managedresource/createordelete_test.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -371,9 +371,11 @@ func TestGetVAPWithMutator(t *testing.T) {
371371
// Verify initial state
372372
if vap == nil {
373373
t.Fatal("getVAPWithMutator() returned nil VAP")
374+
return
374375
}
375376
if mutateFunc == nil {
376377
t.Fatal("getVAPWithMutator() returned nil mutate function")
378+
return
377379
}
378380

379381
// Verify mutate function works
@@ -411,9 +413,11 @@ func TestGetVAPBindingWithMutator(t *testing.T) {
411413
// Verify initial state
412414
if vapb == nil {
413415
t.Fatal("getVAPBindingWithMutator() returned nil VAP binding")
416+
return
414417
}
415418
if mutateFunc == nil {
416419
t.Fatal("getVAPBindingWithMutator() returned nil mutate function")
420+
return
417421
}
418422

419423
// Verify mutate function works

‎pkg/webhook/managedresource/validatingadmissionpolicy.go‎

Lines changed: 12 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -42,58 +42,30 @@ func mutateValidatingAdmissionPolicy(vap *admv1.ValidatingAdmissionPolicy, isHub
4242
{
4343
RuleWithOperations: admv1.RuleWithOperations{
4444
Rule: admv1.Rule{
45-
APIGroups: []string{""},
46-
Resources: []string{"namespaces"},
47-
APIVersions: []string{"v1"},
48-
},
49-
Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete},
50-
},
51-
},
52-
{
53-
RuleWithOperations: admv1.RuleWithOperations{
54-
Rule: admv1.Rule{
55-
APIGroups: []string{""},
56-
Resources: []string{"resourcequotas"},
57-
APIVersions: []string{"v1"},
58-
},
59-
Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete},
60-
},
61-
ResourceNames: []string{"default"},
62-
},
63-
{
64-
RuleWithOperations: admv1.RuleWithOperations{
65-
Rule: admv1.Rule{
66-
APIGroups: []string{"networking.k8s.io"},
67-
Resources: []string{"networkpolicies"},
45+
APIGroups: []string{"*"},
46+
Resources: []string{"*"},
6847
APIVersions: []string{"*"},
6948
},
7049
Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete},
7150
},
72-
ResourceNames: []string{"default"},
7351
},
7452
},
7553
},
7654
Validations: []admv1.Validation{
7755
{
78-
Expression: `"system:masters" in request.userInfo.groups || "system:serviceaccounts:kube-system" in request.userInfo.groups || "system:serviceaccounts:fleet-system" in request.userInfo.groups || "system:serviceaccounts:openshift-kube-controller-manager" in request.userInfo.groups`,
79-
Message: "Create, Update, or Delete operations on ARM managed resources is forbidden",
80-
Reason: &forbidden,
56+
Expression: `(
57+
request.userInfo.username == "aksService" ||
58+
request.userInfo.username == "fleet-member-agent-sa")
59+
&& (
60+
"system:masters" in request.userInfo.groups ||
61+
"system:serviceaccounts:kube-system" in request.userInfo.groups ||
62+
"system:serviceaccounts:fleet-system" in request.userInfo.groups ||
63+
"system:serviceaccounts:openshift-kube-controller-manager" in request.userInfo.groups)`,
64+
Message: "Create, Update, or Delete operations on ARM managed resources is forbidden",
65+
Reason: &forbidden,
8166
},
8267
},
8368
}
84-
85-
if isHub {
86-
vap.Spec.MatchConstraints.ResourceRules = append(vap.Spec.MatchConstraints.ResourceRules, admv1.NamedRuleWithOperations{
87-
RuleWithOperations: admv1.RuleWithOperations{
88-
Rule: admv1.Rule{
89-
APIGroups: []string{"placement.kubernetes-fleet.io"},
90-
Resources: []string{"*"},
91-
APIVersions: []string{"*"},
92-
},
93-
Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete},
94-
},
95-
})
96-
}
9769
}
9870

9971
func getValidatingAdmissionPolicyBinding() *admv1.ValidatingAdmissionPolicyBinding {

‎pkg/webhook/managedresource/validatingadmissionpolicy_test.go‎

Lines changed: 38 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -16,60 +16,52 @@ import (
1616
func TestGetValidatingAdmissionPolicy(t *testing.T) {
1717
t.Parallel()
1818

19-
t.Run("member", func(t *testing.T) {
20-
t.Parallel()
19+
tests := []struct {
20+
name string
21+
isHub bool
22+
}{
23+
{
24+
name: "member cluster",
25+
isHub: false,
26+
},
27+
{
28+
name: "hub cluster",
29+
isHub: true,
30+
},
31+
}
2132

22-
vap := getValidatingAdmissionPolicy(false)
33+
for _, tt := range tests {
34+
t.Run(tt.name, func(t *testing.T) {
35+
t.Parallel()
2336

24-
unwantedRule := admv1.NamedRuleWithOperations{
25-
RuleWithOperations: admv1.RuleWithOperations{
26-
Rule: admv1.Rule{
27-
APIGroups: []string{"placement.kubernetes-fleet.io"},
28-
Resources: []string{"clusterresourceplacements"},
29-
APIVersions: []string{"*"},
37+
vap := getValidatingAdmissionPolicy(tt.isHub)
38+
39+
// Both hub and member clusters should have the same single rule that covers all resources
40+
expectedRule := admv1.NamedRuleWithOperations{
41+
RuleWithOperations: admv1.RuleWithOperations{
42+
Rule: admv1.Rule{
43+
APIGroups: []string{"*"},
44+
Resources: []string{"*"},
45+
APIVersions: []string{"*"},
46+
},
47+
Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete},
3048
},
31-
Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete},
32-
},
33-
}
34-
35-
if vap.Spec.MatchConstraints != nil {
36-
for _, rule := range vap.Spec.MatchConstraints.ResourceRules {
37-
if diff := cmp.Diff(unwantedRule, rule); diff == "" {
38-
t.Errorf("getValidatingAdmissionPolicy(false) contains unwanted rule %+v", unwantedRule)
39-
}
4049
}
41-
}
42-
})
4350

44-
t.Run("hub", func(t *testing.T) {
45-
t.Parallel()
51+
if vap.Spec.MatchConstraints == nil {
52+
t.Fatal("MatchConstraints should not be nil")
53+
}
4654

47-
vap := getValidatingAdmissionPolicy(true)
55+
if len(vap.Spec.MatchConstraints.ResourceRules) != 1 {
56+
t.Errorf("Expected exactly 1 resource rule, got %d", len(vap.Spec.MatchConstraints.ResourceRules))
57+
}
4858

49-
wantedRule := admv1.NamedRuleWithOperations{
50-
RuleWithOperations: admv1.RuleWithOperations{
51-
Rule: admv1.Rule{
52-
APIGroups: []string{"placement.kubernetes-fleet.io"},
53-
Resources: []string{"*"},
54-
APIVersions: []string{"*"},
55-
},
56-
Operations: []admv1.OperationType{admv1.Create, admv1.Update, admv1.Delete},
57-
},
58-
}
59-
60-
found := false
61-
if vap.Spec.MatchConstraints != nil {
62-
for _, rule := range vap.Spec.MatchConstraints.ResourceRules {
63-
if diff := cmp.Diff(wantedRule, rule); diff == "" {
64-
found = true
65-
break
66-
}
59+
actualRule := vap.Spec.MatchConstraints.ResourceRules[0]
60+
if diff := cmp.Diff(expectedRule, actualRule); diff != "" {
61+
t.Errorf("Resource rule mismatch (-want +got):\n%s", diff)
6762
}
68-
}
69-
if !found {
70-
t.Errorf("getValidatingAdmissionPolicy(true) missing expected rule %+v", wantedRule)
71-
}
72-
})
63+
})
64+
}
7365
}
7466

7567
func TestMutateValidatingAdmissionPolicy(t *testing.T) {

‎test/e2e/framework/cluster.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ func NewCluster(name, svcAccountName string, scheme *runtime.Scheme, pp trackers
6262
func GetClusterClient(cluster *Cluster) {
6363
clusterConfig := GetClientConfig(cluster)
6464
impersonateClusterConfig := GetImpersonateClientConfig(cluster)
65-
systemMastersConfig := GetSystemMastersClientConfig(cluster)
65+
systemMastersConfig := GetAKSServiceSystemMastersClientConfig(cluster)
6666

6767
restConfig, err := clusterConfig.ClientConfig()
6868
if err != nil {
@@ -106,13 +106,13 @@ func GetClientConfig(cluster *Cluster) clientcmd.ClientConfig {
106106
})
107107
}
108108

109-
func GetSystemMastersClientConfig(cluster *Cluster) clientcmd.ClientConfig {
109+
func GetAKSServiceSystemMastersClientConfig(cluster *Cluster) clientcmd.ClientConfig {
110110
return clientcmd.NewNonInteractiveDeferredLoadingClientConfig(
111111
&clientcmd.ClientConfigLoadingRules{ExplicitPath: kubeconfigPath},
112112
&clientcmd.ConfigOverrides{
113113
CurrentContext: cluster.ClusterName,
114114
AuthInfo: api.AuthInfo{
115-
Impersonate: "system",
115+
Impersonate: "aksService",
116116
ImpersonateGroups: []string{"system:masters"},
117117
},
118118
})

0 commit comments

Comments
 (0)