Copilot commented on code in PR #2962: URL: https://github.com/apache/hugegraph/pull/2962#discussion_r2936750379
########## 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 `localhost` resolves to `127.0.0.1`. On some environments `localhost` may resolve only to IPv6 (`::1`), which would fail the test even if the handler logic is correct. Consider deriving expected addresses from `InetAddress.getAllByName("localhost")` and asserting that at least one returned `getHostAddress()` is allowed (or explicitly accept both `127.0.0.1` and `::1`). ########## 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")); + } + + @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 can trigger real DNS lookups and be slow/flaky depending on resolver configuration and network availability. Prefer an input that fails fast without external DNS (e.g., a syntactically invalid hostname), or mock/stub `InetAddress.getAllByName()` so the test remains deterministic. ########## hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/auth/IpAuthHandler.java: ########## @@ -48,6 +51,25 @@ public static IpAuthHandler getInstance(Set<String> allowedIps) { return instance; } + /** + * Returns the existing singleton instance, or null if not yet initialized. + * Should only be called after getInstance(Set) has been called during startup. + */ + public static IpAuthHandler getInstance() { + return instance; + } + + /** + * Refreshes the resolved IP allowlist from a new set of hostnames or IPs. + * Should be called when the Raft peer list changes via RaftEngine#changePeerList(). + * Note: DNS-only changes (e.g. container restart with new IP, same hostname) + * are not automatically detected and still require a process restart. + */ + public void refresh(Set<String> newAllowedIps) { + this.resolvedIps = resolveAll(newAllowedIps); + log.info("IpAuthHandler allowlist refreshed, resolved {} entries", resolvedIps.size()); Review Comment: The refresh log message says "resolved {} entries" but the set size includes the original allowlist strings (hostnames) plus any resolved IPs, and also includes entries that failed to resolve (since they remain in the set). This makes the metric misleading for troubleshooting. Consider changing the wording (e.g., "allowlist size") or logging both input size and number of resolved addresses separately. -- 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]
