Skip to content

feat: deny the member leave when member still has serviceExport#1092

Merged
zhiying-lin merged 2 commits intoAzure:mainfrom
zhiying-lin:deny-member-delete
Apr 10, 2025
Merged

feat: deny the member leave when member still has serviceExport#1092
zhiying-lin merged 2 commits intoAzure:mainfrom
zhiying-lin:deny-member-delete

Conversation

@zhiying-lin
Copy link
Copy Markdown
Contributor

@zhiying-lin zhiying-lin commented Mar 21, 2025

Description of your changes

deny the member leave when member still has serviceExport in the webhook

Fixes #

I have:

  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Special notes for your reviewer

@zhiying-lin zhiying-lin marked this pull request as draft March 21, 2025 08:17
@zhiying-lin zhiying-lin force-pushed the deny-member-delete branch 6 times, most recently from 6240c14 to 701d74c Compare March 27, 2025 02:52
@zhiying-lin zhiying-lin marked this pull request as ready for review March 27, 2025 08:02
michaelawyu
michaelawyu reviewed Apr 3, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@michaelawyu michaelawyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments, PTAL.

Comment thread test/e2e/fleet_guard_rail_test.go Outdated
Comment thread test/e2e/fleet_guard_rail_test.go Outdated
Comment thread test/e2e/fleet_guard_rail_test.go
Comment thread test/e2e/join_and_leave_test.go
Comment thread pkg/webhook/membercluster/membercluster_validating_webhook.go

// Handle memberClusterValidator checks to see if member cluster has valid fields.
func (v *memberClusterValidator) Handle(_ context.Context, req admission.Request) admission.Response {
func (v *memberClusterValidator) Handle(ctx context.Context, req admission.Request) admission.Response {
Copy link
Copy Markdown
Contributor

@michaelawyu michaelawyu Apr 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another naming nit: the added logic might no longer belong in a validator? It's more of a guard 😄

Copy link
Copy Markdown
Contributor

@michaelawyu michaelawyu Apr 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is, the logic seems to be more aligned with those in fleetresourcehandler pkg?

Copy link
Copy Markdown
Contributor Author

@zhiying-lin zhiying-lin Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fleetresourcehandler pkg is aim to validate the CR permissions so far. feel this package is better?

Comment thread pkg/webhook/membercluster/membercluster_validating_webhook.go
mcObjectName := types.NamespacedName{Name: req.Name, Namespace: req.Namespace}
klog.V(2).InfoS("Validating webhook handling member cluster", "operation", req.Operation, "memberCluster", mcObjectName)

if req.Operation == admissionv1.Delete { // Will reject the requests whenever the serviceExport is not deleted
Copy link
Copy Markdown
Contributor

@michaelawyu michaelawyu Apr 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some random thoughts, these are not blocker per se, just something that occurred to me when reading the code:

Copy link
Copy Markdown
Contributor

@michaelawyu michaelawyu Apr 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a webhook that guards against deletion attempts on Azure Fleet Manager from users (or any source other than RP, AKS support, etc.). So technically speaking the added logic is added to prevent RP (in most cases) from triggering the deletion. How would these components interact I guess? The concern is mostly that users won't see the error message that reminds them to delete service exports.

Copy link
Copy Markdown
Contributor

@michaelawyu michaelawyu Apr 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For upstream they have full control so we generally assume that protection is not 100% a must.

Copy link
Copy Markdown
Contributor Author

@zhiying-lin zhiying-lin Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the RP side, we should surface this error to the user. Will test that.

@kaito-pr-agent
Copy link
Copy Markdown

kaito-pr-agent Bot commented Apr 8, 2025

Title

Enhance member leave validation and E2E tests

Description

  • Added validation logic to deny member leave when member still has serviceExport.

  • Updated webhook registration to include DELETE operation.

  • Enhanced E2E tests to cover denial of unjoin requests when serviceExport exists.

  • Refactored test setup to create dummy internalServiceExport for testing.

Changes walkthrough 📝

Relevant files
Enhancement
membercluster_validating_webhook.go
Implement DELETE operation validation in member cluster webhook

pkg/webhook/membercluster/membercluster_validating_webhook.go

  • Added client dependency.
  • Implemented DELETE operation validation.
  • Improved logging and error handling.
    		• +28/-4   
    webhook.go
    Update webhook rules to include DELETE                                     

    pkg/webhook/webhook.go

    • Updated rules to include DELETE operation.
    		 +1/-0     
    fleet_guard_rail_test.go
    Add E2E tests for member leave validation                               

    test/e2e/fleet_guard_rail_test.go

  • Created dummy internalServiceExport for testing.
  • Added tests to verify denial of unjoin requests.
  • Cleaned up internalServiceExport after tests.
    		• +21/-7   
    join_and_leave_test.go
    Refactor and enhance join and leave tests                               

    test/e2e/join_and_leave_test.go

  • Refactored test setup to create dummy internalServiceExport.
  • Added tests to verify deletion of internalServiceExport.
  • Removed finalizer from internalServiceExport for cleanup.
    		• +84/-2   
    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
    		•

    @kaito-pr-agent
    Copy link
    Copy Markdown

    kaito-pr-agent Bot commented Apr 8, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Logic Issue

    The current implementation denies the deletion of a member cluster even if the serviceExport is not deleted. This might lead to unexpected behavior where members cannot leave the cluster until all related serviceExports are manually deleted.

     
    if req.Operation == admissionv1.Delete { // Will reject the requests whenever the serviceExport is not deleted
	klog.V(2).InfoS("Validating webhook member cluster DELETE", "memberCluster", mcObjectName)
	namespaceName := fmt.Sprintf(utils.NamespaceNameFormat, mcObjectName.Name)
	internalServiceExportList := &fleetnetworkingv1alpha1.InternalServiceExportList{}
	if err := v.client.List(ctx, internalServiceExportList, client.InNamespace(namespaceName)); err != nil {
		klog.ErrorS(err, "Failed to list internalServiceExportList when validating")
		return admission.Errored(http.StatusInternalServerError, fmt.Errorf("failed to list internalServiceExportList, please retry the request: %w", err))
	}
	for _, internalServiceExport := range internalServiceExportList.Items {
		if internalServiceExport.DeletionTimestamp.IsZero() {
			klog.Warning("ServiceExport exists in the member cluster, request is denied", "operation", req.Operation, "memberCluster", mcObjectName)
			return admission.Denied(fmt.Sprintf("Please delete serviceExport %s in the member cluster before leaving, request is denied", internalServiceExport.Spec.ServiceReference.NamespacedName))
		}
	}
	return admission.Allowed("Member cluster is ready to leave")
}

    michaelawyu
    michaelawyu approved these changes Apr 9, 2025
    View reviewed changes
    Copy link
    Copy Markdown
    Contributor

    @michaelawyu michaelawyu left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    LGTM

    @zhiying-lin zhiying-lin merged commit 934eaaa into Azure:main Apr 10, 2025
    16 checks passed
    @zhiying-lin zhiying-lin deleted the deny-member-delete branch April 10, 2025 01:52
    ryanzhang-oss pushed a commit to ryanzhang-oss/fleet that referenced this pull request Apr 22, 2025
    @zhiying-lin
    feat: deny the member leave when member still has serviceExport (Azur…
    2fd2f68 
    …e#1092)
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @michaelawyu michaelawyu michaelawyu approved these changes

    Assignees

    No one assigned

    Labels

    Review effort 4/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @zhiying-lin @michaelawyu