Skip to content

[fix][client] Fix tenant's allowedClusters is not updated when removing cluster#21740

Closed
ubo-dev wants to merge 0 commit intoapache:masterfrom
ubo-dev:master
Closed

[fix][client] Fix tenant's allowedClusters is not updated when removing cluster#21740
ubo-dev wants to merge 0 commit intoapache:masterfrom
ubo-dev:master

Conversation

@ubo-dev
Copy link

@ubo-dev ubo-dev commented Dec 17, 2023

Fixes #21546

Main Issue: #21546

Motivation

After deletion of cluster, tenant's allowed clusters Set is not updated. The deleted clusters is still available in there.

Modifications

I have edited the delete cluster method in CmdCluster file to update tenant's allowed cluster set when deletion of cluster occurs.

Verifying this change

  • Make sure that the change passes the CI checks.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: PR

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 17, 2023
@ubo-dev
Copy link
Author

ubo-dev commented Dec 17, 2023

I did try to add some test for this case but I have struggled a little. In AdminApiTest class there is a test that is for listing, creation, deletion for clusters. When I try to test this case, it does not run through CmdClusters, so I couldn't test it.
admin.clusters().deleteCluster(cluster);
This line takes deletion to a part that is after where my code is appears.
What do you suggest in this situation. Where I can write the test of this case.

@Technoboy-
Copy link
Contributor

I did try to add some test for this case but I have struggled a little. In AdminApiTest class there is a test that is for listing, creation, deletion for clusters. When I try to test this case, it does not run through CmdClusters, so I couldn't test it. admin.clusters().deleteCluster(cluster); This line takes deletion to a part that is after where my code is appears. What do you suggest in this situation. Where I can write the test of this case.

Add test in TestCmdClusters

@Technoboy- Technoboy- added this to the 3.3.0 milestone Dec 22, 2023
@ubo-dev
Copy link
Author

ubo-dev commented Dec 25, 2023

@Technoboy- I have added delete cluster without delete all option test. Could you review it?

@ubo-dev
Copy link
Author

ubo-dev commented Jan 5, 2024

Hey, I've accidentally closed the PR, I've added the required tests and opened it again @Technoboy- Could you review it?

@ubo-dev ubo-dev reopened this Jan 5, 2024
@Technoboy- Technoboy- changed the title [bug] tenant's allowedClusters is not updated when removing a cluster is fixed [bug] Tenant's allowedClusters is not updated when removing a cluster is fixed Jan 10, 2024
@Technoboy- Technoboy- changed the title [bug] Tenant's allowedClusters is not updated when removing a cluster is fixed [fix[bug] Tenant's allowedClusters is not updated when removing a cluster is fixed Jan 10, 2024
@Technoboy- Technoboy- changed the title [fix[bug] Tenant's allowedClusters is not updated when removing a cluster is fixed [fix][cli] Tenant's allowedClusters is not updated when removing a cluster is fixed Jan 10, 2024
@Technoboy- Technoboy- changed the title [fix][cli] Tenant's allowedClusters is not updated when removing a cluster is fixed [fix][client] Tenant's allowedClusters is not updated when removing a cluster is fixed Jan 10, 2024
@Technoboy- Technoboy- changed the title [fix][client] Tenant's allowedClusters is not updated when removing a cluster is fixed [fix][client] Fix tenant's allowedClusters is not updated when removing cluster Jan 10, 2024
} else {
Set<String> clusters = getAdmin().tenants().getTenantInfo(tenant).getAllowedClusters();
clusters.remove(cluster);
getAdmin().tenants().updateTenant(tenant, TenantInfo.builder()
Copy link
Contributor

@poorbarcode poorbarcode Jan 10, 2024

Choose a reason for hiding this comment

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

So far, Pulsar did not allow to create a tenant with empty clusters, see https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java#L268-L273

// empty cluster shouldn't be allowed
if (info == null || info.getAllowedClusters().stream().filter(c -> !StringUtils.isBlank(c))
        .collect(Collectors.toSet()).isEmpty()
        || info.getAllowedClusters().stream().anyMatch(ac -> StringUtils.isBlank(ac))) {
    log.warn("[{}] Failed to validate due to clusters are empty", clientAppId());
    return FutureUtil.failedFuture(new RestException(Status.PRECONDITION_FAILED, "Clusters can not be empty"));
}

If the clusters only have one element (@param cluster), should we delete this tenant or throw an error? If no, it will break the limitation above.

/cc @codelipenghui @Technoboy-

Copy link
Author

Choose a reason for hiding this comment

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

IMO, we should throw an error since tenant with empty clusters is not allowed. @Technoboy- @poorbarcode

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs ready-to-test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] tenant's allowedClusters is not updated when removing a cluster from /clusters admin API

3 participants