From af761e2f8d3aee35c2cf081998f556972a92b574 Mon Sep 17 00:00:00 2001 From: Hang Chen Date: Sun, 16 Jan 2022 10:58:15 +0800 Subject: [PATCH] Fix invalid rack name cause bookie join rack failed (#13683) * Fix invalid rackname cause bookie join rack failed --- .../BookieRackAffinityMapping.java | 5 +++- .../BookieRackAffinityMappingTest.java | 26 ++++++++++++++++++- .../policies/data/impl/BookieInfoImpl.java | 10 +++++++ .../pulsar/admin/cli/PulsarAdminToolTest.java | 18 +++++++++++++ .../apache/pulsar/admin/cli/CmdBookies.java | 13 ++++++++++ 5 files changed, 70 insertions(+), 2 deletions(-) diff --git a/pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMapping.java b/pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMapping.java index 18437acb53abb..8790e0ad15708 100644 --- a/pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMapping.java +++ b/pulsar-broker-common/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/pulsar-broker-common/src/test/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMappingTest.java b/pulsar-broker-common/src/test/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMappingTest.java index fc0331dab2df1..50f0797db8307 100644 --- a/pulsar-broker-common/src/test/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMappingTest.java +++ b/pulsar-broker-common/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 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/pulsar-client-admin-api/src/main/java/org/apache/pulsar/common/policies/data/impl/BookieInfoImpl.java b/pulsar-client-admin-api/src/main/java/org/apache/pulsar/common/policies/data/impl/BookieInfoImpl.java index de316e612f3c1..b58498903ab48 100644 --- a/pulsar-client-admin-api/src/main/java/org/apache/pulsar/common/policies/data/impl/BookieInfoImpl.java +++ b/pulsar-client-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 index 560c5b040a483..05636f59984d1 100644 --- 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 index b039dc600df9f..c8b3363d08d41 100644 --- 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 admin) {