Copilot commented on code in PR #2962:
URL: https://github.com/apache/hugegraph/pull/2962#discussion_r2938103164
##########
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java:
##########
@@ -326,6 +325,23 @@ public Status changePeerList(String peerList) {
latch.countDown();
});
latch.await();
+
Review Comment:
`changePeerList()` blocks on `latch.await()` with no timeout. If the Raft
`changePeers()` callback is never invoked (e.g., node not started / RPC
failure), this call can hang indefinitely and tie up the caller thread.
Consider using a timed await (e.g., based on the configured RPC timeout) and
return a failed `Status` when the timeout elapses.
##########
hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/raft/IpAuthHandlerTest.java:
##########
@@ -0,0 +1,120 @@
+/*
+ * 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 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() {
+ // "localhost" should resolve to "127.0.0.1" via
InetAddress.getAllByName()
+ // This verifies the core fix: hostname allowlists match numeric
remote addresses
+ IpAuthHandler handler = IpAuthHandler.getInstance(
+ Collections.singleton("localhost"));
+ Assert.assertTrue(isIpAllowed(handler, "127.0.0.1"));
+ }
Review Comment:
`testHostnameResolvesToIp()` assumes that resolving `localhost` will include
`127.0.0.1`, which can be false on IPv6-only / custom resolver setups (where
`localhost` may resolve only to `::1`). To avoid a flaky test, consider
querying `InetAddress.getAllByName("localhost")` inside the test and asserting
the handler allows the returned addresses (or at least one of them).
--
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]
