fix: fix the validation webhook

cmd/hubagent/main.go

@@ -156,7 +156,8 @@ func main() {
 
 	if opts.EnableWebhook {
 		whiteListedUsers := strings.Split(opts.WhiteListedUsers, ",")
-		if err := SetupWebhook(mgr, options.WebhookClientConnectionType(opts.WebhookClientConnectionType), opts.WebhookServiceName, whiteListedUsers, opts.EnableGuardRail, opts.EnableV1Beta1APIs, opts.DenyModifyMemberClusterLabels, opts.EnableWorkload); err != nil {
+		if err := SetupWebhook(mgr, options.WebhookClientConnectionType(opts.WebhookClientConnectionType), opts.WebhookServiceName, whiteListedUsers,
+			opts.EnableGuardRail, opts.EnableV1Beta1APIs, opts.DenyModifyMemberClusterLabels, opts.EnableWorkload, opts.NetworkingAgentsEnabled); err != nil {
 			klog.ErrorS(err, "unable to set up webhook")
 			exitWithErrorFunc()
 		}
@@ -188,7 +189,8 @@ func main() {
 }
 
 // SetupWebhook generates the webhook cert and then set up the webhook configurator.
-func SetupWebhook(mgr manager.Manager, webhookClientConnectionType options.WebhookClientConnectionType, webhookServiceName string, whiteListedUsers []string, enableGuardRail, isFleetV1Beta1API bool, denyModifyMemberClusterLabels bool, enableWorkload bool) error {
+func SetupWebhook(mgr manager.Manager, webhookClientConnectionType options.WebhookClientConnectionType, webhookServiceName string,
+	whiteListedUsers []string, enableGuardRail, isFleetV1Beta1API bool, denyModifyMemberClusterLabels bool, enableWorkload bool, networkingAgentsEnabled bool) error {
 	// Generate self-signed key and crt files in FleetWebhookCertDir for the webhook server to start.
 	w, err := webhook.NewWebhookConfig(mgr, webhookServiceName, FleetWebhookPort, &webhookClientConnectionType, FleetWebhookCertDir, enableGuardRail, denyModifyMemberClusterLabels, enableWorkload)
 	if err != nil {
@@ -199,7 +201,7 @@ func SetupWebhook(mgr manager.Manager, webhookClientConnectionType options.Webho
 		klog.ErrorS(err, "unable to add WebhookConfig")
 		return err
 	}
-	if err = webhook.AddToManager(mgr, whiteListedUsers, denyModifyMemberClusterLabels); err != nil {
+	if err = webhook.AddToManager(mgr, whiteListedUsers, denyModifyMemberClusterLabels, networkingAgentsEnabled); err != nil {
 		klog.ErrorS(err, "unable to register webhooks to the manager")
 		return err
 	}

pkg/webhook/add_handler.go

@@ -16,13 +16,13 @@ import (
 func init() {
 	// AddToManagerFleetResourceValidator is a function to register fleet guard rail resource validator to the webhook server
 	AddToManagerFleetResourceValidator = fleetresourcehandler.Add
+	AddToManagerMemberclusterValidator = membercluster.Add
 	// AddToManagerFuncs is a list of functions to register webhook validators and mutators to the webhook server
 	AddToManagerFuncs = append(AddToManagerFuncs, clusterresourceplacement.AddMutating)
 	AddToManagerFuncs = append(AddToManagerFuncs, clusterresourceplacement.Add)
 	AddToManagerFuncs = append(AddToManagerFuncs, resourceplacement.Add)
 	AddToManagerFuncs = append(AddToManagerFuncs, pod.Add)
 	AddToManagerFuncs = append(AddToManagerFuncs, replicaset.Add)
-	AddToManagerFuncs = append(AddToManagerFuncs, membercluster.Add)
 	AddToManagerFuncs = append(AddToManagerFuncs, clusterresourceoverride.Add)
 	AddToManagerFuncs = append(AddToManagerFuncs, resourceoverride.Add)
 	AddToManagerFuncs = append(AddToManagerFuncs, clusterresourceplacementeviction.Add)

pkg/webhook/membercluster/membercluster_validating_webhook.go

@@ -26,15 +26,19 @@ var (
 )
 
 type memberClusterValidator struct {
-	client  client.Client
-	decoder webhook.AdmissionDecoder
+	client                  client.Client
+	decoder                 webhook.AdmissionDecoder
+	networkingAgentsEnabled bool
 }
 
 // Add registers the webhook for K8s bulit-in object types.
-func Add(mgr manager.Manager) error {
+func Add(mgr manager.Manager, networkingAgentsEnabled bool) {
 	hookServer := mgr.GetWebhookServer()
-	hookServer.Register(ValidationPath, &webhook.Admission{Handler: &memberClusterValidator{client: mgr.GetClient(), decoder: admission.NewDecoder(mgr.GetScheme())}})
-	return nil
+	hookServer.Register(ValidationPath, &webhook.Admission{Handler: &memberClusterValidator{
+		client:                  mgr.GetClient(),
+		decoder:                 admission.NewDecoder(mgr.GetScheme()),
+		networkingAgentsEnabled: networkingAgentsEnabled,
+	}})
 }
 
 // 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
 		}
 
 		if mc.Spec.DeleteOptions != nil && mc.Spec.DeleteOptions.ValidationMode == clusterv1beta1.DeleteValidationModeSkip {
-			klog.V(2).InfoS("Skipping validation for member cluster DELETE", "memberCluster", mcObjectName)
-			return admission.Allowed("Skipping validation for member cluster DELETE")
+			klog.V(2).InfoS("Skipping validation for member cluster DELETE when the validation mode is set to skip", "memberCluster", mcObjectName)
+			return admission.Allowed("Skipping validation for member cluster DELETE when the validation mode is set to skip")
+		}
+		if !v.networkingAgentsEnabled {
+			klog.V(2).InfoS("Networking agents disabled; skipping ServiceExport validation", "memberCluster", mcObjectName)
+			return admission.Allowed("Networking agents disabled; skipping ServiceExport validation")
 		}
 
 		klog.V(2).InfoS("Validating webhook member cluster DELETE", "memberCluster", mcObjectName)

pkg/webhook/membercluster/membercluster_validating_webhook_test.go

@@ -0,0 +1,141 @@
+package membercluster
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+	"strings"
+	"testing"
+
+	admissionv1 "k8s.io/api/admission/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
+
+	clusterv1beta1 "github.com/kubefleet-dev/kubefleet/apis/cluster/v1beta1"
+	"github.com/kubefleet-dev/kubefleet/pkg/utils"
+
+	fleetnetworkingv1alpha1 "go.goms.io/fleet-networking/api/v1alpha1"
+
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
+)
+
+func TestHandleDelete(t *testing.T) {
+	t.Parallel()
+
+	testCases := map[string]struct {
+		networkingEnabled bool
+		validationMode    clusterv1beta1.DeleteValidationMode
+		wantAllowed       bool
+		wantMessageSubstr string
+	}{
+		"networking-disabled-allows-delete": {
+			networkingEnabled: false,
+			wantAllowed:       true,
+			validationMode:    clusterv1beta1.DeleteValidationModeStrict,
+		},
+		"networking-enabled-denies-delete": {
+			networkingEnabled: true,
+			wantAllowed:       false,
+			validationMode:    clusterv1beta1.DeleteValidationModeStrict,
+			wantMessageSubstr: "Please delete serviceExport",
+		},
+		"delete-options-skip-bypasses-validation": {
+			networkingEnabled: true,
+			wantAllowed:       true,
+			validationMode:    clusterv1beta1.DeleteValidationModeSkip,
+		},
+	}
+
+	for name, tc := range testCases {
+		tc := tc
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			mcName := fmt.Sprintf("member-%s", name)
+			namespaceName := fmt.Sprintf(utils.NamespaceNameFormat, mcName)
+			svcExport := newInternalServiceExport(mcName, namespaceName)
+
+			validator := newMemberClusterValidatorForTest(t, tc.networkingEnabled, svcExport)
+			mc := &clusterv1beta1.MemberCluster{ObjectMeta: metav1.ObjectMeta{Name: mcName}}
+			mc.Spec.DeleteOptions = &clusterv1beta1.DeleteOptions{ValidationMode: tc.validationMode}
+			req := buildDeleteRequestFromObject(t, mc)
+
+			resp := validator.Handle(context.Background(), req)
+			if resp.Allowed != tc.wantAllowed {
+				t.Fatalf("Handle() got response: %+v, want allowed %t", resp, tc.wantAllowed)
+			}
+			if tc.wantMessageSubstr != "" {
+				if resp.Result == nil || !strings.Contains(resp.Result.Message, tc.wantMessageSubstr) {
+					t.Fatalf("Handle()  got response result: %v,  want contain: %q", resp.Result, tc.wantMessageSubstr)
+				}
+			}
+		})
+	}
+}
+
+func newMemberClusterValidatorForTest(t *testing.T, networkingEnabled bool, objs ...client.Object) *memberClusterValidator {
+	t.Helper()
+
+	scheme := runtime.NewScheme()
+	if err := clusterv1beta1.AddToScheme(scheme); err != nil {
+		t.Fatalf("failed to add member cluster scheme: %v", err)
+	}
+	if err := fleetnetworkingv1alpha1.AddToScheme(scheme); err != nil {
+		t.Fatalf("failed to add fleet networking scheme: %v", err)
+	}
+	scheme.AddKnownTypes(fleetnetworkingv1alpha1.GroupVersion,
+		&fleetnetworkingv1alpha1.InternalServiceExport{},
+		&fleetnetworkingv1alpha1.InternalServiceExportList{},
+	)
+	metav1.AddToGroupVersion(scheme, fleetnetworkingv1alpha1.GroupVersion)
+
+	fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...).Build()
+	decoder := admission.NewDecoder(scheme)
+
+	return &memberClusterValidator{
+		client:                  fakeClient,
+		decoder:                 decoder,
+		networkingAgentsEnabled: networkingEnabled,
+	}
+}
+
+func buildDeleteRequestFromObject(t *testing.T, mc *clusterv1beta1.MemberCluster) admission.Request {
+	t.Helper()
+
+	raw, err := json.Marshal(mc)
+	if err != nil {
+		t.Fatalf("failed to marshal member cluster: %v", err)
+	}
+
+	return admission.Request{
+		AdmissionRequest: admissionv1.AdmissionRequest{
+			Operation: admissionv1.Delete,
+			Name:      mc.Name,
+			OldObject: runtime.RawExtension{Raw: raw},
+		},
+	}
+}
+
+func newInternalServiceExport(clusterID, namespace string) *fleetnetworkingv1alpha1.InternalServiceExport {
+	return &fleetnetworkingv1alpha1.InternalServiceExport{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "sample-service",
+			Namespace: namespace,
+		},
+		Spec: fleetnetworkingv1alpha1.InternalServiceExportSpec{
+			ServiceReference: fleetnetworkingv1alpha1.ExportedObjectReference{
+				ClusterID:       clusterID,
+				Kind:            "Service",
+				Namespace:       "work",
+				Name:            "sample-service",
+				ResourceVersion: "1",
+				Generation:      1,
+				UID:             types.UID("svc-uid"),
+				NamespacedName:  "work/sample-service",
+			},
+		},
+	}
+}

pkg/webhook/webhook.go

@@ -131,14 +131,16 @@ var (
 
 var AddToManagerFuncs []func(manager.Manager) error
 var AddToManagerFleetResourceValidator func(manager.Manager, []string, bool) error
+var AddToManagerMemberclusterValidator func(manager.Manager, bool)
 
 // AddToManager adds all Controllers to the Manager
-func AddToManager(m manager.Manager, whiteListedUsers []string, denyModifyMemberClusterLabels bool) error {
+func AddToManager(m manager.Manager, whiteListedUsers []string, denyModifyMemberClusterLabels bool, networkingAgentsEnabled bool) error {
 	for _, f := range AddToManagerFuncs {
 		if err := f(m); err != nil {
 			return err
 		}
 	}
+	AddToManagerMemberclusterValidator(m, networkingAgentsEnabled)
 	return AddToManagerFleetResourceValidator(m, whiteListedUsers, denyModifyMemberClusterLabels)
 }
 

test/e2e/join_and_leave_test.go

@@ -17,9 +17,7 @@ limitations under the License.
 package e2e
 
 import (
-	"errors"
 	"fmt"
-	"reflect"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -163,34 +161,8 @@ var _ = Describe("Test member cluster join and leave flow", Label("joinleave"),
 				}
 			})
 
-			It("Should fail the unjoin requests", func() {
-				for idx := range allMemberClusters {
-					memberCluster := allMemberClusters[idx]
-					mcObj := &clusterv1beta1.MemberCluster{
-						ObjectMeta: metav1.ObjectMeta{
-							Name: memberCluster.ClusterName,
-						},
-					}
-					err := hubClient.Delete(ctx, mcObj)
-					Expect(err).ShouldNot(Succeed(), "Want the deletion to be denied")
-					var statusErr *apierrors.StatusError
-					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{})))
-					Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("Please delete serviceExport test-namespace/test-svc in the member cluster before leaving, request is denied"))
-				}
-			})
-
-			It("Deleting the internalServiceExports", func() {
-				for idx := range allMemberClusterNames {
-					memberCluster := allMemberClusters[idx]
-					namespaceName := fmt.Sprintf(utils.NamespaceNameFormat, memberCluster.ClusterName)
-
-					internalSvcExportKey := types.NamespacedName{Namespace: namespaceName, Name: internalServiceExportName}
-					var export fleetnetworkingv1alpha1.InternalServiceExport
-					Expect(hubClient.Get(ctx, internalSvcExportKey, &export)).Should(Succeed(), "Failed to get internalServiceExport")
-					Expect(hubClient.Delete(ctx, &export)).To(Succeed(), "Failed to delete internalServiceExport")
-				}
-			})
-
+			// The network agent is not turned on by default in the e2e so we are still able to leave when we have internalServiceExport
+			// TODO: add a test case for the network agent is turned on in the fleet network repository.
 			It("Should be able to trigger the member cluster DELETE", func() {
 				setAllMemberClustersToLeave()
 			})
@@ -362,22 +334,7 @@ var _ = Describe("Test member cluster join and leave flow", Label("joinleave"),
 				}
 			})
 
-			It("Should fail the unjoin requests", func() {
-				for idx := range allMemberClusters {
-					memberCluster := allMemberClusters[idx]
-					mcObj := &clusterv1beta1.MemberCluster{
-						ObjectMeta: metav1.ObjectMeta{
-							Name: memberCluster.ClusterName,
-						},
-					}
-					err := hubClient.Delete(ctx, mcObj)
-					Expect(err).ShouldNot(Succeed(), "Want the deletion to be denied")
-					var statusErr *apierrors.StatusError
-					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{})))
-					Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("Please delete serviceExport test-namespace/test-svc in the member cluster before leaving, request is denied"))
-				}
-			})
-
+			// 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
 			It("Updating the member cluster to skip validation", func() {
 				for idx := range allMemberClusterNames {
 					memberCluster := allMemberClusters[idx]