Validate rack name (apache#14336)

### Motivation If the rack name set in the following case: - Enabled rack aware placement policy, and user set rack name which contains "/" in addition to the head and tail of the string. Such as "/r/a" or "r/a/b" - Enabled region aware placement policy, and user set the rack name which contains multiple "/" in addition to the head and tail of the string. Such as "/region/region/a" or "region/a/rack/b"
datastax · Feb 27, 2022 · 25408f5 · 25408f5
1 parent 145f29a
commit 25408f5
Show file tree

Hide file tree

Showing 2 changed files with 73 additions and 0 deletions.
diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/Bookies.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/Bookies.java
@@ -43,6 +43,7 @@
 import org.apache.bookkeeper.discover.RegistrationClient;
 import org.apache.bookkeeper.meta.MetadataClientDriver;
 import org.apache.bookkeeper.net.BookieId;
+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.policies.data.BookieInfo;
@@ -55,6 +56,7 @@
 @Produces(MediaType.APPLICATION_JSON)
 @Slf4j
 public class Bookies extends AdminResource {
+    private static final String PATH_SEPARATOR = "/";
 
     @GET
     @Path("/racks-info")
@@ -162,6 +164,20 @@ public void updateBookieRackInfo(@Suspended final AsyncResponse asyncResponse,
             throw new RestException(Status.PRECONDITION_FAILED, "Bookie 'group' parameters is missing");
         }
 
+        // validate rack name
+        int separatorCnt = StringUtils.countMatches(
+            StringUtils.strip(bookieInfo.getRack(), PATH_SEPARATOR), PATH_SEPARATOR);
+        boolean isRackEnabled = pulsar().getConfiguration().isBookkeeperClientRackawarePolicyEnabled();
+        boolean isRegionEnabled = pulsar().getConfiguration().isBookkeeperClientRegionawarePolicyEnabled();
+        if (isRackEnabled && ((isRegionEnabled && separatorCnt != 1) || (!isRegionEnabled && separatorCnt != 0))) {
+            asyncResponse.resume(new RestException(Status.PRECONDITION_FAILED, "Bookie 'rack' parameter is invalid, "
+                + "When `RackawareEnsemblePlacementPolicy` is enabled, the rack name is not allowed to contain "
+                + "slash (`/`) except for the beginning and end of the rack name string. "
+                + "When `RegionawareEnsemblePlacementPolicy` is enabled, the rack name can only contain "
+                + "one slash (`/`) except for the beginning and end of the rack name string."));
+            return;
+        }
+
         getPulsarResources().getBookieResources()
                 .update(optionalBookiesRackConfiguration -> {
                     BookiesRackConfiguration brc = optionalBookiesRackConfiguration

diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/BookiesApiTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/BookiesApiTest.java
@@ -18,12 +18,14 @@
  */
 package org.apache.pulsar.broker.admin;
 
+import static org.mockito.Mockito.doReturn;
 import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.assertTrue;
 import static org.testng.Assert.fail;
 
 import java.util.Optional;
 import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.broker.ServiceConfiguration;
 import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest;
 import org.apache.pulsar.client.admin.PulsarAdminException;
 import org.apache.pulsar.common.policies.data.BookieInfo;
@@ -122,6 +124,61 @@ public void testBasic() throws Exception {
                 .get()
                 .getValue()
                 .size());
+
+        // test invalid rack name
+        // use rack aware placement policy
+        String errorMsg = "Bookie 'rack' parameter is invalid, When `RackawareEnsemblePlacementPolicy` is enabled, "
+            + "the rack name is not allowed to contain slash (`/`) except for the beginning and end of the rack name "
+            + "string. When `RegionawareEnsemblePlacementPolicy` is enabled, the rack name can only contain "
+            + "one slash (`/`) except for the beginning and end of the rack name string.";
+
+        BookieInfo newInfo3 = BookieInfo.builder()
+            .rack("/rack/a")
+            .hostname("127.0.0.2")
+            .build();
+        try {
+            admin.bookies().updateBookieRackInfo(bookie0, "default", newInfo3);
+            fail();
+        } catch (PulsarAdminException e) {
+            assertEquals(412, e.getStatusCode());
+            assertEquals(errorMsg, e.getMessage());
+        }
+
+        BookieInfo newInfo4 = BookieInfo.builder()
+            .rack("/rack")
+            .hostname("127.0.0.2")
+            .build();
+        try {
+            admin.bookies().updateBookieRackInfo(bookie0, "default", newInfo4);
+        } catch (PulsarAdminException e) {
+            fail();
+        }
+
+        // enable region aware placement policy
+        ServiceConfiguration configuration = new ServiceConfiguration();
+        configuration.setBookkeeperClientRegionawarePolicyEnabled(true);
+        doReturn(configuration).when(pulsar).getConfiguration();
+        BookieInfo newInfo5 = BookieInfo.builder()
+            .rack("/region/rack/a")
+            .hostname("127.0.0.2")
+            .build();
+        try {
+            admin.bookies().updateBookieRackInfo(bookie0, "default", newInfo5);
+            fail();
+        } catch (PulsarAdminException e) {
+            assertEquals(412, e.getStatusCode());
+            assertEquals(errorMsg, e.getMessage());
+        }
+
+        BookieInfo newInfo6 = BookieInfo.builder()
+            .rack("/region/rack/")
+            .hostname("127.0.0.2")
+            .build();
+        try {
+            admin.bookies().updateBookieRackInfo(bookie0, "default", newInfo6);
+        } catch (PulsarAdminException e) {
+            fail();
+        }
     }
 
 }