Fix invalid rack name cause bookie join rack failed (apache#13683)

* Fix invalid rackname cause bookie join rack failed
drianpauL · Jan 16, 2022 · af761e2 · af761e2
1 parent acc28d5
commit af761e2
Show file tree

Hide file tree

Showing 5 changed files with 70 additions and 2 deletions.
diff --git a/...ommon/src/main/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMapping.java b/...ommon/src/main/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMapping.java
@@ -18,6 +18,7 @@
  */
 package org.apache.pulsar.bookie.rackawareness;
 
+import com.google.api.client.util.Strings;
 import java.net.InetAddress;
 import java.util.ArrayList;
 import java.util.Collections;
@@ -159,7 +160,9 @@ private String getRack(String bookieAddress) {
             }
         }
 
-        if (bi != null) {
+        if (bi != null
+                && !Strings.isNullOrEmpty(bi.getRack())
+                && !bi.getRack().trim().equals("/")) {
             String rack = bi.getRack();
             if (!rack.startsWith("/")) {
                 rack = "/" + rack;

diff --git a/...n/src/test/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMappingTest.java b/...n/src/test/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMappingTest.java
@@ -19,6 +19,7 @@
 package org.apache.pulsar.bookie.rackawareness;
 
 import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNull;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.google.common.collect.Lists;
 import java.util.HashMap;
@@ -77,7 +78,30 @@ public void testBasic() throws Exception {
                 .resolve(Lists.newArrayList(BOOKIE1.getHostName(), BOOKIE2.getHostName(), BOOKIE3.getHostName()));
         assertEquals(racks1.get(0), "/rack0");
         assertEquals(racks1.get(1), "/rack1");
-        assertEquals(racks1.get(2), null);
+        assertNull(racks1.get(2));
+    }
+
+    @Test
+    public void testInvalidRackName() {
+        String data = "{\"group1\": {\"" + BOOKIE1
+                + "\": {\"rack\": \"/\", \"hostname\": \"bookie1.example.com\"}, \"" + BOOKIE2
+                + "\": {\"rack\": \"\", \"hostname\": \"bookie2.example.com\"}}}";
+
+        store.put(BookieRackAffinityMapping.BOOKIE_INFO_ROOT_PATH, data.getBytes(), Optional.empty()).join();
+
+        // Case1: ZKCache is given
+        BookieRackAffinityMapping mapping1 = new BookieRackAffinityMapping();
+        ClientConfiguration bkClientConf1 = new ClientConfiguration();
+        bkClientConf1.setProperty(BookieRackAffinityMapping.METADATA_STORE_INSTANCE, store);
+
+        mapping1.setBookieAddressResolver(BookieSocketAddress.LEGACY_BOOKIEID_RESOLVER);
+        mapping1.setConf(bkClientConf1);
+        List<String> racks1 = mapping1
+                .resolve(Lists.newArrayList(BOOKIE1.getHostName(), BOOKIE2.getHostName(), BOOKIE3.getHostName()));
+
+        assertNull(racks1.get(0));
+        assertNull(racks1.get(1));
+        assertNull(racks1.get(2));
     }
 
     @Test

diff --git a/...t-admin-api/src/main/java/org/apache/pulsar/common/policies/data/impl/BookieInfoImpl.java b/...t-admin-api/src/main/java/org/apache/pulsar/common/policies/data/impl/BookieInfoImpl.java
@@ -21,6 +21,7 @@
 import lombok.AllArgsConstructor;
 import lombok.Data;
 import lombok.NoArgsConstructor;
+import lombok.NonNull;
 import org.apache.pulsar.common.policies.data.BookieInfo;
 
 /**
@@ -40,6 +41,7 @@ public static BookieInfoImplBuilder builder() {
     public static class BookieInfoImplBuilder implements BookieInfo.Builder {
         private String rack;
         private String hostname;
+        private static final String PATH_SEPARATOR = "/";
 
         public BookieInfoImplBuilder rack(String rack) {
             this.rack = rack;
@@ -52,7 +54,15 @@ public BookieInfoImplBuilder hostname(String hostname) {
         }
 
         public BookieInfoImpl build() {
+            checkArgument(rack != null && !rack.isEmpty() && !rack.equals(PATH_SEPARATOR),
+                    "rack name is invalid, it should not be null, empty or '/'");
             return new BookieInfoImpl(rack, hostname);
         }
+
+        public static void checkArgument(boolean expression, @NonNull Object errorMessage) {
+            if (!expression) {
+                throw new IllegalArgumentException(String.valueOf(errorMessage));
+            }
+        }
     }
 }
diff --git a/pulsar-client-tools-test/src/test/java/org/apache/pulsar/admin/cli/PulsarAdminToolTest.java b/pulsar-client-tools-test/src/test/java/org/apache/pulsar/admin/cli/PulsarAdminToolTest.java
@@ -27,6 +27,7 @@
 import static org.mockito.Mockito.when;
 import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.assertNull;
+import static org.testng.Assert.fail;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
@@ -1875,6 +1876,23 @@ public void bookies() throws Exception {
                         .rack("rack-1")
                         .hostname("host-1")
                         .build());
+
+        // test invalid rack name ""
+        try {
+            BookieInfo.builder().rack("").hostname("host-1").build();
+            fail();
+        } catch (IllegalArgumentException e) {
+            assertEquals(e.getMessage(), "rack name is invalid, it should not be null, empty or '/'");
+        }
+
+        // test invalid rack name "/"
+        try {
+            BookieInfo.builder().rack("/").hostname("host-1").build();
+            fail();
+        } catch (IllegalArgumentException e) {
+            assertEquals(e.getMessage(), "rack name is invalid, it should not be null, empty or '/'");
+        }
+
     }
 
     @Test

diff --git a/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdBookies.java b/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdBookies.java
@@ -20,7 +20,9 @@
 
 import com.beust.jcommander.Parameter;
 import com.beust.jcommander.Parameters;
+import com.google.common.base.Strings;
 import java.util.function.Supplier;
+import lombok.NonNull;
 import org.apache.pulsar.client.admin.PulsarAdmin;
 import org.apache.pulsar.common.policies.data.BookieInfo;
 
@@ -74,6 +76,8 @@ void run() throws Exception {
     @Parameters(commandDescription = "Updates the rack placement information for a specific bookie in the cluster "
             + "(note. bookie address format:`address:port`)")
     private class UpdateBookie extends CliCommand {
+        private static final String PATH_SEPARATOR = "/";
+
         @Parameter(names = { "-g", "--group" }, description = "Bookie group name", required = false)
         private String group = "default";
 
@@ -89,12 +93,21 @@ private class UpdateBookie extends CliCommand {
 
         @Override
         void run() throws Exception {
+            checkArgument(!Strings.isNullOrEmpty(bookieRack) && !bookieRack.trim().equals(PATH_SEPARATOR),
+                    "rack name is invalid, it should not be null, empty or '/'");
+
             getAdmin().bookies().updateBookieRackInfo(bookieAddress, group,
                     BookieInfo.builder()
                             .rack(bookieRack)
                             .hostname(bookieHost)
                             .build());
         }
+
+        private void checkArgument(boolean expression, @NonNull Object errorMessage) {
+            if (!expression) {
+                throw new IllegalArgumentException(String.valueOf(errorMessage));
+            }
+        }
     }
 
     public CmdBookies(Supplier<PulsarAdmin> admin) {