Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Issue 5999] Support create/update tenant with empty cluster #6027

Merged
merged 9 commits into from
Feb 16, 2020
Merged

[Issue 5999] Support create/update tenant with empty cluster #6027

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f7772ec
Support create/update tenant with empty cluster.
murong00 Jan 10, 2020
bdadf6c
treat a blank cluster as a cluster is not specified
murong00 Jan 13, 2020
3c690b5
Improve non cluster logic
murong00 Jan 14, 2020
20a8def
Merge branch 'master' of https://github.com/apache/pulsar into branch…
murong00 Feb 11, 2020
65f8e0a
Improve the error message when clusters are empty
murong00 Feb 11, 2020
7af32b1
Remove unused test methond
murong00 Feb 11, 2020
f531363
Fix some unit test
murong00 Feb 14, 2020
319ebe2
Merge branch 'master' of https://github.com/apache/pulsar into branch…
murong00 Feb 14, 2020
dbf2c41
Fix a unit test
murong00 Feb 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
*/
package org.apache.pulsar.broker.admin.impl;

import io.swagger.annotations.ApiParam;
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;
Expand All @@ -31,6 +30,7 @@
import javax.ws.rs.PathParam;
import javax.ws.rs.core.Response.Status;

import org.apache.commons.lang3.StringUtils;
import org.apache.pulsar.broker.admin.AdminResource;
import org.apache.pulsar.broker.web.RestException;
import org.apache.pulsar.common.naming.NamedEntity;
Expand All @@ -41,7 +41,9 @@
import org.slf4j.LoggerFactory;

import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
import io.swagger.annotations.ApiOperation;
import io.swagger.annotations.ApiParam;
import io.swagger.annotations.ApiResponse;
import io.swagger.annotations.ApiResponses;
import static org.apache.pulsar.broker.cache.ConfigurationCacheService.POLICIES;
Expand Down Expand Up @@ -220,10 +222,21 @@ public void deleteTenant(
}
}

private Set<String> getNonBlankClusters(Set<String> allowedClusters) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of this check?

If a caller passes a list like ["", ""] the request will fail because the empty string is not a valid cluster in allowedClusters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check is only used to make the request with blank clusters valid，i.e. use new TenantInfo() for create/update.

Set<String> nonBlankClusters = Sets.newHashSet();
for (String ac : allowedClusters) {
if (!StringUtils.isBlank(ac)) {
nonBlankClusters.add(ac);
}
}
return nonBlankClusters;
}

private void validateClusters(TenantInfo info) {
List<String> nonexistentClusters;
try {
if (info == null) {
// treat a blank cluster as a cluster is not specified
if (info == null || getNonBlankClusters(info.getAllowedClusters()).isEmpty()) {
info = new TenantInfo();
}
Set<String> availableClusters = clustersListCache().get();
Expand Down
39 changes: 39 additions & 0 deletions pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTest2.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -861,6 +861,45 @@ public void testTenantWithNonexistentClusters() throws Exception {
}
}

@Test
public void testTenantWithBlankClusterName() throws Exception {
List<String> availableClusters = admin.clusters().getClusters();

// Check blank cluster
String blankCluster = "";
assertFalse(availableClusters.contains(blankCluster));

Set<String> allowedClusters = Sets.newHashSet(blankCluster);
TenantInfo tenantInfo = new TenantInfo(Sets.newHashSet("role1", "role2"), allowedClusters);

String testTenant = "test-tenant";

// When create tenant with blank clusters, it should be successful
try {
admin.tenants().createTenant(testTenant, tenantInfo);
} catch (PulsarAdminException e) {
e.printStackTrace();
fail("Should be successful");
}

// Check existing tenant
assertTrue(admin.tenants().getTenants().contains(testTenant));

Set<String> allowedClusters1 = Sets.newHashSet(blankCluster);
TenantInfo tenantInfo1 = new TenantInfo(Sets.newHashSet("role1"), allowedClusters1);

// When update tenant with blank clusters, it should be successful
try {
admin.tenants().updateTenant(testTenant, tenantInfo1);
} catch (PulsarAdminException e) {
e.printStackTrace();
fail("Should be successful");
}

assertEquals(admin.tenants().getTenantInfo(testTenant).getAdminRoles().size(), 1);
assertEquals(admin.tenants().getTenantInfo(testTenant).getAdminRoles().toArray()[0].toString(), "role1");
}

@Test
public void brokerNamespaceIsolationPolicies() throws Exception {

Expand Down