Skip to content

Commit 58b6de2

Browse files
ryanzhang-ossryanzhang-oss
authored
fix: fix the validation webhook (#359)
1 parent 0f449dc commit 58b6de2

File tree

6 files changed

+168
-58
lines changed

6 files changed

+168
-58
lines changed

‎cmd/hubagent/main.go‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,8 @@ func main() {
156156

157157
if opts.EnableWebhook {
158158
whiteListedUsers := strings.Split(opts.WhiteListedUsers, ",")
159-
if err := SetupWebhook(mgr, options.WebhookClientConnectionType(opts.WebhookClientConnectionType), opts.WebhookServiceName, whiteListedUsers, opts.EnableGuardRail, opts.EnableV1Beta1APIs, opts.DenyModifyMemberClusterLabels, opts.EnableWorkload); err != nil {
159+
if err := SetupWebhook(mgr, options.WebhookClientConnectionType(opts.WebhookClientConnectionType), opts.WebhookServiceName, whiteListedUsers,
160+
opts.EnableGuardRail, opts.EnableV1Beta1APIs, opts.DenyModifyMemberClusterLabels, opts.EnableWorkload, opts.NetworkingAgentsEnabled); err != nil {
160161
klog.ErrorS(err, "unable to set up webhook")
161162
exitWithErrorFunc()
162163
}
@@ -188,7 +189,8 @@ func main() {
188189
}
189190

190191
// SetupWebhook generates the webhook cert and then set up the webhook configurator.
191-
func SetupWebhook(mgr manager.Manager, webhookClientConnectionType options.WebhookClientConnectionType, webhookServiceName string, whiteListedUsers []string, enableGuardRail, isFleetV1Beta1API bool, denyModifyMemberClusterLabels bool, enableWorkload bool) error {
192+
func SetupWebhook(mgr manager.Manager, webhookClientConnectionType options.WebhookClientConnectionType, webhookServiceName string,
193+
whiteListedUsers []string, enableGuardRail, isFleetV1Beta1API bool, denyModifyMemberClusterLabels bool, enableWorkload bool, networkingAgentsEnabled bool) error {
192194
// Generate self-signed key and crt files in FleetWebhookCertDir for the webhook server to start.
193195
w, err := webhook.NewWebhookConfig(mgr, webhookServiceName, FleetWebhookPort, &webhookClientConnectionType, FleetWebhookCertDir, enableGuardRail, denyModifyMemberClusterLabels, enableWorkload)
194196
if err != nil {
@@ -199,7 +201,7 @@ func SetupWebhook(mgr manager.Manager, webhookClientConnectionType options.Webho
199201
klog.ErrorS(err, "unable to add WebhookConfig")
200202
return err
201203
}
202-
if err = webhook.AddToManager(mgr, whiteListedUsers, denyModifyMemberClusterLabels); err != nil {
204+
if err = webhook.AddToManager(mgr, whiteListedUsers, denyModifyMemberClusterLabels, networkingAgentsEnabled); err != nil {
203205
klog.ErrorS(err, "unable to register webhooks to the manager")
204206
return err
205207
}

‎pkg/webhook/add_handler.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,13 @@ import (
1616
func init() {
1717
// AddToManagerFleetResourceValidator is a function to register fleet guard rail resource validator to the webhook server
1818
AddToManagerFleetResourceValidator = fleetresourcehandler.Add
19+
AddToManagerMemberclusterValidator = membercluster.Add
1920
// AddToManagerFuncs is a list of functions to register webhook validators and mutators to the webhook server
2021
AddToManagerFuncs = append(AddToManagerFuncs, clusterresourceplacement.AddMutating)
2122
AddToManagerFuncs = append(AddToManagerFuncs, clusterresourceplacement.Add)
2223
AddToManagerFuncs = append(AddToManagerFuncs, resourceplacement.Add)
2324
AddToManagerFuncs = append(AddToManagerFuncs, pod.Add)
2425
AddToManagerFuncs = append(AddToManagerFuncs, replicaset.Add)
25-
AddToManagerFuncs = append(AddToManagerFuncs, membercluster.Add)
2626
AddToManagerFuncs = append(AddToManagerFuncs, clusterresourceoverride.Add)
2727
AddToManagerFuncs = append(AddToManagerFuncs, resourceoverride.Add)
2828
AddToManagerFuncs = append(AddToManagerFuncs, clusterresourceplacementeviction.Add)

‎pkg/webhook/membercluster/membercluster_validating_webhook.go‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,15 +26,19 @@ var (
2626
)
2727

2828
type memberClusterValidator struct {
29-
client client.Client
30-
decoder webhook.AdmissionDecoder
29+
client client.Client
30+
decoder webhook.AdmissionDecoder
31+
networkingAgentsEnabled bool
3132
}
3233

3334
// Add registers the webhook for K8s bulit-in object types.
34-
func Add(mgr manager.Manager) error {
35+
func Add(mgr manager.Manager, networkingAgentsEnabled bool) {
3536
hookServer := mgr.GetWebhookServer()
36-
hookServer.Register(ValidationPath, &webhook.Admission{Handler: &memberClusterValidator{client: mgr.GetClient(), decoder: admission.NewDecoder(mgr.GetScheme())}})
37-
return nil
37+
hookServer.Register(ValidationPath, &webhook.Admission{Handler: &memberClusterValidator{
38+
client: mgr.GetClient(),
39+
decoder: admission.NewDecoder(mgr.GetScheme()),
40+
networkingAgentsEnabled: networkingAgentsEnabled,
41+
}})
3842
}
3943

4044
// Handle memberClusterValidator checks to see if member cluster has valid fields.
@@ -50,8 +54,12 @@ func (v *memberClusterValidator) Handle(ctx context.Context, req admission.Reque
5054
}
5155

5256
if mc.Spec.DeleteOptions != nil && mc.Spec.DeleteOptions.ValidationMode == clusterv1beta1.DeleteValidationModeSkip {
53-
klog.V(2).InfoS("Skipping validation for member cluster DELETE", "memberCluster", mcObjectName)
54-
return admission.Allowed("Skipping validation for member cluster DELETE")
57+
klog.V(2).InfoS("Skipping validation for member cluster DELETE when the validation mode is set to skip", "memberCluster", mcObjectName)
58+
return admission.Allowed("Skipping validation for member cluster DELETE when the validation mode is set to skip")
59+
}
60+
if !v.networkingAgentsEnabled {
61+
klog.V(2).InfoS("Networking agents disabled; skipping ServiceExport validation", "memberCluster", mcObjectName)
62+
return admission.Allowed("Networking agents disabled; skipping ServiceExport validation")
5563
}
5664

5765
klog.V(2).InfoS("Validating webhook member cluster DELETE", "memberCluster", mcObjectName)

‎pkg/webhook/membercluster/membercluster_validating_webhook_test.go‎

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
package membercluster
2+
3+
import (
4+
"context"
5+
"encoding/json"
6+
"fmt"
7+
"strings"
8+
"testing"
9+
10+
admissionv1 "k8s.io/api/admission/v1"
11+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
12+
"k8s.io/apimachinery/pkg/runtime"
13+
"k8s.io/apimachinery/pkg/types"
14+
15+
clusterv1beta1 "github.com/kubefleet-dev/kubefleet/apis/cluster/v1beta1"
16+
"github.com/kubefleet-dev/kubefleet/pkg/utils"
17+
18+
fleetnetworkingv1alpha1 "go.goms.io/fleet-networking/api/v1alpha1"
19+
20+
"sigs.k8s.io/controller-runtime/pkg/client"
21+
"sigs.k8s.io/controller-runtime/pkg/client/fake"
22+
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
23+
)
24+
25+
func TestHandleDelete(t *testing.T) {
26+
t.Parallel()
27+
28+
testCases := map[string]struct {
29+
networkingEnabled bool
30+
validationMode clusterv1beta1.DeleteValidationMode
31+
wantAllowed bool
32+
wantMessageSubstr string
33+
}{
34+
"networking-disabled-allows-delete": {
35+
networkingEnabled: false,
36+
wantAllowed: true,
37+
validationMode: clusterv1beta1.DeleteValidationModeStrict,
38+
},
39+
"networking-enabled-denies-delete": {
40+
networkingEnabled: true,
41+
wantAllowed: false,
42+
validationMode: clusterv1beta1.DeleteValidationModeStrict,
43+
wantMessageSubstr: "Please delete serviceExport",
44+
},
45+
"delete-options-skip-bypasses-validation": {
46+
networkingEnabled: true,
47+
wantAllowed: true,
48+
validationMode: clusterv1beta1.DeleteValidationModeSkip,
49+
},
50+
}
51+
52+
for name, tc := range testCases {
53+
tc := tc
54+
t.Run(name, func(t *testing.T) {
55+
t.Parallel()
56+
57+
mcName := fmt.Sprintf("member-%s", name)
58+
namespaceName := fmt.Sprintf(utils.NamespaceNameFormat, mcName)
59+
svcExport := newInternalServiceExport(mcName, namespaceName)
60+
61+
validator := newMemberClusterValidatorForTest(t, tc.networkingEnabled, svcExport)
62+
mc := &clusterv1beta1.MemberCluster{ObjectMeta: metav1.ObjectMeta{Name: mcName}}
63+
mc.Spec.DeleteOptions = &clusterv1beta1.DeleteOptions{ValidationMode: tc.validationMode}
64+
req := buildDeleteRequestFromObject(t, mc)
65+
66+
resp := validator.Handle(context.Background(), req)
67+
if resp.Allowed != tc.wantAllowed {
68+
t.Fatalf("Handle() got response: %+v, want allowed %t", resp, tc.wantAllowed)
69+
}
70+
if tc.wantMessageSubstr != "" {
71+
if resp.Result == nil || !strings.Contains(resp.Result.Message, tc.wantMessageSubstr) {
72+
t.Fatalf("Handle() got response result: %v, want contain: %q", resp.Result, tc.wantMessageSubstr)
73+
}
74+
}
75+
})
76+
}
77+
}
78+
79+
func newMemberClusterValidatorForTest(t *testing.T, networkingEnabled bool, objs ...client.Object) *memberClusterValidator {
80+
t.Helper()
81+
82+
scheme := runtime.NewScheme()
83+
if err := clusterv1beta1.AddToScheme(scheme); err != nil {
84+
t.Fatalf("failed to add member cluster scheme: %v", err)
85+
}
86+
if err := fleetnetworkingv1alpha1.AddToScheme(scheme); err != nil {
87+
t.Fatalf("failed to add fleet networking scheme: %v", err)
88+
}
89+
scheme.AddKnownTypes(fleetnetworkingv1alpha1.GroupVersion,
90+
&fleetnetworkingv1alpha1.InternalServiceExport{},
91+
&fleetnetworkingv1alpha1.InternalServiceExportList{},
92+
)
93+
metav1.AddToGroupVersion(scheme, fleetnetworkingv1alpha1.GroupVersion)
94+
95+
fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...).Build()
96+
decoder := admission.NewDecoder(scheme)
97+
98+
return &memberClusterValidator{
99+
client: fakeClient,
100+
decoder: decoder,
101+
networkingAgentsEnabled: networkingEnabled,
102+
}
103+
}
104+
105+
func buildDeleteRequestFromObject(t *testing.T, mc *clusterv1beta1.MemberCluster) admission.Request {
106+
t.Helper()
107+
108+
raw, err := json.Marshal(mc)
109+
if err != nil {
110+
t.Fatalf("failed to marshal member cluster: %v", err)
111+
}
112+
113+
return admission.Request{
114+
AdmissionRequest: admissionv1.AdmissionRequest{
115+
Operation: admissionv1.Delete,
116+
Name: mc.Name,
117+
OldObject: runtime.RawExtension{Raw: raw},
118+
},
119+
}
120+
}
121+
122+
func newInternalServiceExport(clusterID, namespace string) *fleetnetworkingv1alpha1.InternalServiceExport {
123+
return &fleetnetworkingv1alpha1.InternalServiceExport{
124+
ObjectMeta: metav1.ObjectMeta{
125+
Name: "sample-service",
126+
Namespace: namespace,
127+
},
128+
Spec: fleetnetworkingv1alpha1.InternalServiceExportSpec{
129+
ServiceReference: fleetnetworkingv1alpha1.ExportedObjectReference{
130+
ClusterID: clusterID,
131+
Kind: "Service",
132+
Namespace: "work",
133+
Name: "sample-service",
134+
ResourceVersion: "1",
135+
Generation: 1,
136+
UID: types.UID("svc-uid"),
137+
NamespacedName: "work/sample-service",
138+
},
139+
},
140+
}
141+
}

‎pkg/webhook/webhook.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,14 +131,16 @@ var (
131131

132132
var AddToManagerFuncs []func(manager.Manager) error
133133
var AddToManagerFleetResourceValidator func(manager.Manager, []string, bool) error
134+
var AddToManagerMemberclusterValidator func(manager.Manager, bool)
134135

135136
// AddToManager adds all Controllers to the Manager
136-
func AddToManager(m manager.Manager, whiteListedUsers []string, denyModifyMemberClusterLabels bool) error {
137+
func AddToManager(m manager.Manager, whiteListedUsers []string, denyModifyMemberClusterLabels bool, networkingAgentsEnabled bool) error {
137138
for _, f := range AddToManagerFuncs {
138139
if err := f(m); err != nil {
139140
return err
140141
}
141142
}
143+
AddToManagerMemberclusterValidator(m, networkingAgentsEnabled)
142144
return AddToManagerFleetResourceValidator(m, whiteListedUsers, denyModifyMemberClusterLabels)
143145
}
144146

‎test/e2e/join_and_leave_test.go‎

Lines changed: 3 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,7 @@ limitations under the License.
1717
package e2e
1818

1919
import (
20-
"errors"
2120
"fmt"
22-
"reflect"
2321

2422
. "github.com/onsi/ginkgo/v2"
2523
. "github.com/onsi/gomega"
@@ -163,34 +161,8 @@ var _ = Describe("Test member cluster join and leave flow", Label("joinleave"),
163161
}
164162
})
165163

166-
It("Should fail the unjoin requests", func() {
167-
for idx := range allMemberClusters {
168-
memberCluster := allMemberClusters[idx]
169-
mcObj := &clusterv1beta1.MemberCluster{
170-
ObjectMeta: metav1.ObjectMeta{
171-
Name: memberCluster.ClusterName,
172-
},
173-
}
174-
err := hubClient.Delete(ctx, mcObj)
175-
Expect(err).ShouldNot(Succeed(), "Want the deletion to be denied")
176-
var statusErr *apierrors.StatusError
177-
Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Delete memberCluster call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&apierrors.StatusError{})))
178-
Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("Please delete serviceExport test-namespace/test-svc in the member cluster before leaving, request is denied"))
179-
}
180-
})
181-
182-
It("Deleting the internalServiceExports", func() {
183-
for idx := range allMemberClusterNames {
184-
memberCluster := allMemberClusters[idx]
185-
namespaceName := fmt.Sprintf(utils.NamespaceNameFormat, memberCluster.ClusterName)
186-
187-
internalSvcExportKey := types.NamespacedName{Namespace: namespaceName, Name: internalServiceExportName}
188-
var export fleetnetworkingv1alpha1.InternalServiceExport
189-
Expect(hubClient.Get(ctx, internalSvcExportKey, &export)).Should(Succeed(), "Failed to get internalServiceExport")
190-
Expect(hubClient.Delete(ctx, &export)).To(Succeed(), "Failed to delete internalServiceExport")
191-
}
192-
})
193-
164+
// The network agent is not turned on by default in the e2e so we are still able to leave when we have internalServiceExport
165+
// TODO: add a test case for the network agent is turned on in the fleet network repository.
194166
It("Should be able to trigger the member cluster DELETE", func() {
195167
setAllMemberClustersToLeave()
196168
})
@@ -362,22 +334,7 @@ var _ = Describe("Test member cluster join and leave flow", Label("joinleave"),
362334
}
363335
})
364336

365-
It("Should fail the unjoin requests", func() {
366-
for idx := range allMemberClusters {
367-
memberCluster := allMemberClusters[idx]
368-
mcObj := &clusterv1beta1.MemberCluster{
369-
ObjectMeta: metav1.ObjectMeta{
370-
Name: memberCluster.ClusterName,
371-
},
372-
}
373-
err := hubClient.Delete(ctx, mcObj)
374-
Expect(err).ShouldNot(Succeed(), "Want the deletion to be denied")
375-
var statusErr *apierrors.StatusError
376-
Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Delete memberCluster call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&apierrors.StatusError{})))
377-
Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("Please delete serviceExport test-namespace/test-svc in the member cluster before leaving, request is denied"))
378-
}
379-
})
380-
337+
// It does not really matter here as the network agent is not turned on by default in the e2e so we are still able to leave when we have internalServiceExport
381338
It("Updating the member cluster to skip validation", func() {
382339
for idx := range allMemberClusterNames {
383340
memberCluster := allMemberClusters[idx]

0 commit comments

Comments
 (0)