Copilot commented on code in PR #2962:
URL: https://github.com/apache/hugegraph/pull/2962#discussion_r2938425983
##########
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java:
##########
@@ -313,23 +313,61 @@ public List<Metapb.Member> getMembers() throws
ExecutionException, InterruptedEx
public Status changePeerList(String peerList) {
AtomicReference<Status> result = new AtomicReference<>();
+ Configuration newPeers = new Configuration();
try {
String[] peers = peerList.split(",", -1);
if ((peers.length & 1) != 1) {
throw new PDException(-1, "the number of peer list must be
odd.");
}
- Configuration newPeers = new Configuration();
newPeers.parse(peerList);
CountDownLatch latch = new CountDownLatch(1);
this.raftNode.changePeers(newPeers, status -> {
- result.set(status);
+ // Use compareAndSet so a late callback does not overwrite a
timeout status
+ result.compareAndSet(null, status);
latch.countDown();
});
- latch.await();
+ // Use configured RPC timeout — bare await() would block forever if
+ // the callback is never invoked (e.g. node not started / RPC
failure)
+ boolean completed = latch.await(3L * config.getRpcTimeout(),
+ TimeUnit.MILLISECONDS);
+ if (!completed && result.get() == null) {
+ Status timeoutStatus = new Status(RaftError.EINTERNAL,
+ "changePeerList timed out
after %d ms",
+ 3L * config.getRpcTimeout());
+ if (!result.compareAndSet(null, timeoutStatus)) {
+ // Callback arrived just before us — keep its result
+ timeoutStatus = null;
+ }
+ if (timeoutStatus != null) {
+ log.error("changePeerList to {} timed out after {} ms",
+ peerList, 3L * config.getRpcTimeout());
+ }
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ result.set(new Status(RaftError.EINTERNAL, "changePeerList
interrupted"));
+ log.error("changePeerList to {} was interrupted", peerList, e);
} catch (Exception e) {
log.error("failed to changePeerList to {},{}", peerList, e);
result.set(new Status(-1, e.getMessage()));
}
+
+ // Refresh IpAuthHandler so newly added peers are not blocked
+ if (result.get() != null && result.get().isOk()) {
+ IpAuthHandler handler = IpAuthHandler.getInstance();
+ if (handler != null) {
+ Set<String> newIps = newPeers.getPeers()
+ .stream()
+ .map(PeerId::getIp)
+ .collect(Collectors.toSet());
Review Comment:
`changePeerList()` refreshes `IpAuthHandler` using only
`newPeers.getPeers()`. This drops any learners specified in the configuration
(the codebase supports `/learner` entries via `PeerUtil.parseConfig()`), so
learner connections could still be blocked after a peer list change. Include
`newPeers.getLearners()` when building `newIps` (similar to
`PDService#updatePdRaft`).
##########
hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/RaftEngineIpAuthIntegrationTest.java:
##########
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hugegraph.pd.raft;
+
+import java.util.Collections;
+
+import org.apache.hugegraph.pd.raft.auth.IpAuthHandler;
+import org.apache.hugegraph.testutil.Whitebox;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import com.alipay.sofa.jraft.Closure;
+import com.alipay.sofa.jraft.Node;
+import com.alipay.sofa.jraft.Status;
+import com.alipay.sofa.jraft.conf.Configuration;
+import com.alipay.sofa.jraft.error.RaftError;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.mock;
+
+public class RaftEngineIpAuthIntegrationTest {
+
+ private Node originalRaftNode;
+
+ @Before
+ public void setUp() {
+ // Save original raftNode so we can restore it after the test
+ originalRaftNode = RaftEngine.getInstance().getRaftNode();
+ // Reset IpAuthHandler singleton for a clean state
+ Whitebox.setInternalState(IpAuthHandler.class, "instance", null);
+ }
+
+ @After
+ public void tearDown() {
+ // Restore original raftNode
+ Whitebox.setInternalState(RaftEngine.getInstance(), "raftNode",
originalRaftNode);
+ // Reset IpAuthHandler singleton
+ Whitebox.setInternalState(IpAuthHandler.class, "instance", null);
+ }
+
+ @Test
+ public void testChangePeerListRefreshesIpAuthHandler() throws Exception {
+ // Initialize IpAuthHandler with an old IP
+ IpAuthHandler handler = IpAuthHandler.getInstance(
+ Collections.singleton("10.0.0.1"));
+ Assert.assertTrue(invokeIsIpAllowed(handler, "10.0.0.1"));
+ Assert.assertFalse(invokeIsIpAllowed(handler, "127.0.0.1"));
+
+ // Mock Node to fire the changePeers callback synchronously with
Status.OK()
+ // This simulates a successful peer change without a real Raft cluster
+
+ // Important: fire the closure synchronously or changePeerList() will
+ // block on latch.await() indefinitely — no timeout is configured
+ Node mockNode = mock(Node.class);
Review Comment:
The comment says `changePeerList()` will block indefinitely because
`latch.await()` has no timeout, but `RaftEngine#changePeerList()` now uses a
timed `await(...)`. Please update this comment to match the current behavior
(it will block up to the configured timeout).
##########
hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/IpAuthHandlerTest.java:
##########
@@ -0,0 +1,132 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hugegraph.pd.raft;
+
+import java.net.InetAddress;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
+
+import org.apache.hugegraph.pd.raft.auth.IpAuthHandler;
+import org.apache.hugegraph.testutil.Whitebox;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+public class IpAuthHandlerTest {
+
+ @Before
+ public void setUp() {
+ // Must reset BEFORE each test — earlier suite classes (e.g.
ConfigServiceTest)
+ // initialize RaftEngine which creates the IpAuthHandler singleton
with their
+ // own peer IPs. Without this reset, our getInstance() calls return
the stale
+ // singleton and ignore the allowlist passed by the test.
+ Whitebox.setInternalState(IpAuthHandler.class, "instance", null);
+ }
+
+ @After
+ public void tearDown() {
+ // Must reset AFTER each test — prevents our test singleton from
leaking
+ // into later suite classes that also depend on IpAuthHandler state.
+ Whitebox.setInternalState(IpAuthHandler.class, "instance", null);
+ }
+
+ private boolean isIpAllowed(IpAuthHandler handler, String ip) {
+ return Whitebox.invoke(IpAuthHandler.class,
+ new Class[]{String.class},
+ "isIpAllowed", handler, ip);
+ }
+
+ @Test
+ public void testHostnameResolvesToIp() throws Exception {
+ // "localhost" should resolve to one or more IPs via
InetAddress.getAllByName()
+ // This verifies the core fix: hostname allowlists match numeric
remote addresses
+ // Using dynamic resolution avoids hardcoding "127.0.0.1" which may
not be
+ // returned on IPv6-only or custom resolver environments
+ IpAuthHandler handler = IpAuthHandler.getInstance(
+ Collections.singleton("localhost"));
+ InetAddress[] addresses = InetAddress.getAllByName("localhost");
+ // All resolved addresses should be allowed — resolveAll() adds every
address
+ // returned by getAllByName() so none should be blocked
+ Assert.assertTrue("Expected at least one resolved address",
+ addresses.length > 0);
+ for (InetAddress address : addresses) {
+ Assert.assertTrue(
+ "Expected " + address.getHostAddress() + " to be allowed",
+ isIpAllowed(handler, address.getHostAddress()));
+ }
+ }
+
+ @Test
+ public void testUnresolvableHostnameDoesNotCrash() {
+ // Should log a warning and skip — no exception thrown during
construction
+ IpAuthHandler handler = IpAuthHandler.getInstance(
+ Collections.singleton("nonexistent.invalid.hostname"));
+ // Handler was still created successfully despite bad hostname
+ Assert.assertNotNull(handler);
+ // Unresolvable entry is skipped so no IPs should be allowed
+ Assert.assertFalse(isIpAllowed(handler, "127.0.0.1"));
+ Assert.assertFalse(isIpAllowed(handler, "192.168.0.1"));
Review Comment:
`testUnresolvableHostnameDoesNotCrash()` uses
`nonexistent.invalid.hostname`, which is not guaranteed to be NXDOMAIN (the TLD
is `hostname`, not the reserved `.invalid`). This can make the test flaky in
environments with custom DNS. Use a domain under the reserved `.invalid` TLD
(e.g. `nonexistent.invalid`) or another guaranteed-unresolvable name.
##########
hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/RaftEngineIpAuthIntegrationTest.java:
##########
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hugegraph.pd.raft;
+
+import java.util.Collections;
+
+import org.apache.hugegraph.pd.raft.auth.IpAuthHandler;
+import org.apache.hugegraph.testutil.Whitebox;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import com.alipay.sofa.jraft.Closure;
+import com.alipay.sofa.jraft.Node;
+import com.alipay.sofa.jraft.Status;
+import com.alipay.sofa.jraft.conf.Configuration;
+import com.alipay.sofa.jraft.error.RaftError;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.mock;
+
+public class RaftEngineIpAuthIntegrationTest {
+
+ private Node originalRaftNode;
+
+ @Before
+ public void setUp() {
+ // Save original raftNode so we can restore it after the test
+ originalRaftNode = RaftEngine.getInstance().getRaftNode();
+ // Reset IpAuthHandler singleton for a clean state
+ Whitebox.setInternalState(IpAuthHandler.class, "instance", null);
+ }
+
+ @After
+ public void tearDown() {
+ // Restore original raftNode
+ Whitebox.setInternalState(RaftEngine.getInstance(), "raftNode",
originalRaftNode);
+ // Reset IpAuthHandler singleton
+ Whitebox.setInternalState(IpAuthHandler.class, "instance", null);
+ }
+
+ @Test
+ public void testChangePeerListRefreshesIpAuthHandler() throws Exception {
+ // Initialize IpAuthHandler with an old IP
+ IpAuthHandler handler = IpAuthHandler.getInstance(
+ Collections.singleton("10.0.0.1"));
+ Assert.assertTrue(invokeIsIpAllowed(handler, "10.0.0.1"));
+ Assert.assertFalse(invokeIsIpAllowed(handler, "127.0.0.1"));
+
+ // Mock Node to fire the changePeers callback synchronously with
Status.OK()
+ // This simulates a successful peer change without a real Raft cluster
+
+ // Important: fire the closure synchronously or changePeerList() will
+ // block on latch.await() indefinitely — no timeout is configured
+ Node mockNode = mock(Node.class);
+ doAnswer(invocation -> {
+ Closure closure = invocation.getArgument(1);
+ closure.run(Status.OK());
+ return null;
+ }).when(mockNode).changePeers(any(Configuration.class),
any(Closure.class));
+
+ // Inject mock node into RaftEngine
+ Whitebox.setInternalState(RaftEngine.getInstance(), "raftNode",
mockNode);
+
+ // Call changePeerList with new peer — must be odd count
+ RaftEngine.getInstance().changePeerList("127.0.0.1:8610");
+
+ // Verify IpAuthHandler was refreshed with the new peer IP
+ Assert.assertTrue(invokeIsIpAllowed(handler, "127.0.0.1"));
+ // Old IP should no longer be allowed
+ Assert.assertFalse(invokeIsIpAllowed(handler, "10.0.0.1"));
+ }
+
+ @Test
+ public void testChangePeerListDoesNotRefreshOnFailure() throws Exception {
+ // Initialize IpAuthHandler with original IP
+ IpAuthHandler handler = IpAuthHandler.getInstance(
+ Collections.singleton("10.0.0.1"));
+ Assert.assertTrue(invokeIsIpAllowed(handler, "10.0.0.1"));
+
+ // Mock Node to fire callback with a failed status
+ // Simulates a failed peer change — handler should NOT be refreshed
+
+ // Important: fire the closure synchronously or changePeerList() will
+ // block on latch.await() indefinitely — no timeout is configured
Review Comment:
This comment states `changePeerList()` can block indefinitely due to
`latch.await()` without a timeout, but the implementation now uses a timed
`await(...)`. Update the comment so it stays accurate.
##########
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java:
##########
@@ -313,23 +313,61 @@ public List<Metapb.Member> getMembers() throws
ExecutionException, InterruptedEx
public Status changePeerList(String peerList) {
AtomicReference<Status> result = new AtomicReference<>();
+ Configuration newPeers = new Configuration();
try {
String[] peers = peerList.split(",", -1);
if ((peers.length & 1) != 1) {
throw new PDException(-1, "the number of peer list must be
odd.");
}
- Configuration newPeers = new Configuration();
newPeers.parse(peerList);
CountDownLatch latch = new CountDownLatch(1);
this.raftNode.changePeers(newPeers, status -> {
- result.set(status);
+ // Use compareAndSet so a late callback does not overwrite a
timeout status
+ result.compareAndSet(null, status);
latch.countDown();
});
Review Comment:
`IpAuthHandler` refresh is performed only after `latch.await(...)` and only
when the returned `Status` is OK. If the `changePeers` callback arrives after
the await timeout, the method will return a timeout `Status` and skip the
refresh even if the peer change eventually succeeds, potentially leaving new
peers blocked. Consider performing the refresh inside the `changePeers`
callback when `status.isOk()` (independent of whether the caller timed out).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
