Bill commented on a change in pull request #6435:
URL: https://github.com/apache/geode/pull/6435#discussion_r658897048
##########
File path:
geode-assembly/src/integrationTest/java/org/apache/geode/management/internal/cli/commands/StartLocatorCommandIntegrationTest.java
##########
@@ -92,7 +92,7 @@ public void startWithBindAddress() throws Exception {
verify(spy).getProcess(any(), commandLines.capture());
String[] lines = commandLines.getValue();
- assertThat(lines[12]).isEqualTo("--bind-address=127.0.0.1");
+ assertThat(lines).containsOnlyOnce("--bind-address=127.0.0.1");
Review comment:
nice added flexibility
##########
File path:
geode-common/src/main/java/org/apache/geode/internal/inet/LocalHostUtil.java
##########
@@ -242,6 +242,21 @@ public static InetAddress getAnyLocalAddress() {
return new InetSocketAddress(0).getAddress();
}
+ public static boolean isWildcardAddress(String address) {
+ return "0.0.0.0".equals(address) || "::".equals(address) ||
isWildcardCharacter(address);
Review comment:
The first two clauses in the disjunction reiterate logic already in
`InetAddress.isAnyLocalAddress()`. Is there a reason to not construct an
`InetAddress` and use `isAnyLocalAddress()`?
It's documented to do this (and the two subclasses for IPV4 and IPV6 seem to
do the right thing):
```java
/**
* Utility routine to check if the InetAddress in a wildcard address.
* @return a {@code boolean} indicating if the Inetaddress is
* a wildcard address.
* @since 1.4
*/
public boolean isAnyLocalAddress() {
return false;
}
```
##########
File path:
geode-common/src/main/java/org/apache/geode/internal/inet/LocalHostUtil.java
##########
@@ -272,6 +287,9 @@ public static boolean isLocalHost(Object host) {
}
}
} else {
+ if (isWildcardAddress(host.toString())) {
Review comment:
Please reduce one level of nesting by moving this `if` up as an `else
if`. Then the `return isLocalHost…` could be its own `else` block as before
this PR.
##########
File path: geode-docs/tools_modules/gfsh/command-pages/start.html.md.erb
##########
@@ -281,7 +281,7 @@ start locator --name=value [--bind-address=value]
[--force(=value)]
| Name | Description | Default Value |
|--------------|----------------|---------------|
| \\-\\-name | Name to be used for this
<%=vars.product_name%> locator service. If not specified, gfsh generates a
random name. | |
-| \\-\\-bind-address | IP address on which the locator
will be bound. | bind to all addresses |
+| \\-\\-bind-address | IP address on which the locator
will be bound. The '*' character is accepted for wildcard
addresses.<p>**Note:** When the locator is bound to all local addresses, the
default for UDP membership-related traffic is to bind to the local host
machine's address.| bind to all addresses, except for membership endpoints
which are bound to the local host machine's address |
Review comment:
good doc!
##########
File path:
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StartLocatorCommand.java
##########
@@ -183,6 +188,8 @@ ResultModel doStartLocator(
Properties gemfireProperties = new Properties();
+ StartMemberUtils.setPropertyIfNotNull(gemfireProperties,
ConfigurationProperties.BIND_ADDRESS,
+ bindAddress);
Review comment:
I thought it was odd that we had to call `setPropertyIfNotNull()` in
this method (`doStartLocator()`) but we did not over in
`StartServerCommand.doStartServer()`. Looks like it was already being called
over in that other method.
Does that mean that before this code change here, we had a bug where gfsh
`start locator` would ignore the bind address (before this PR)?
##########
File path:
geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/fd/GMSHealthMonitor.java
##########
@@ -877,14 +879,32 @@ public void init(Services<ID> s) throws
MembershipConfigurationException {
@Override
public void started() throws MemberStartupException {
setLocalAddress(services.getMessenger().getMemberID());
+ createServerSocket();
+ startTcpServer(serverSocket);
+ startHeartbeatThread();
+ }
+
+ private void createServerSocket() throws MemberStartupException {
try {
- serverSocket = createServerSocket(localAddress.getInetAddress(),
- services.getConfig().getMembershipPortRange());
+ InetAddress address = localAddress.getInetAddress();
+ String bindAddrStr = services.getConfig().getBindAddress();
Review comment:
Ooh good catch remembering to fix the health monitor endpoint too!
##########
File path:
geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java
##########
@@ -1613,7 +1613,12 @@ public Builder setBindAddress(final String
addressString) {
return this;
} else {
try {
- InetAddress address = InetAddress.getByName(addressString);
+ InetAddress address;
+ if (LocalHostUtil.isWildcardAddress(addressString)) {
+ address = LocalHostUtil.getAnyLocalAddress();
+ } else {
+ address = InetAddress.getByName(addressString);
+ }
Review comment:
If no bind address is specified we set `bindAddress` to `null`.
Apparently, elsewhere in the code, we will interpret that `null` as a wildcard
(per the documentation for the gfsh `start locator --bind-address` option.
How come we have two different paths in this method? Shouldn't we do the
same thing if the user specifies `*` as if the user specifies nothing at all?
##########
File path:
geode-core/src/main/java/org/apache/geode/distributed/internal/direct/DirectChannel.java
##########
@@ -683,14 +683,15 @@ public TCPConduit getConduit() {
private InetAddress initAddress(DistributionConfig dc) {
String bindAddress = dc.getBindAddress();
-
try {
- /*
- * note: had to change the following to make sure the prop wasn't empty
in addition to not
- * null for admin.DistributedSystemFactory
- */
if (bindAddress != null && bindAddress.length() > 0) {
- return InetAddress.getByName(bindAddress);
+ InetAddress address;
+ if (LocalHostUtil.isWildcardAddress(bindAddress)) {
+ address = LocalHostUtil.getAnyLocalAddress();
+ } else {
+ address = InetAddress.getByName(bindAddress);
+ }
+ return address;
Review comment:
OK this is the peer's cache endnpoint a.k.a. P2P ✓
##########
File path:
geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/messenger/JGroupsMessenger.java
##########
@@ -281,7 +281,7 @@ public void init(Services<ID> s) throws
MembershipConfigurationException {
String str = config.getBindAddress();
// JGroups UDP protocol requires a bind address
- if (str == null || str.length() == 0) {
+ if (str == null || str.length() == 0 ||
LocalHostUtil.isWildcardAddress(str)) {
Review comment:
yes!
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java
##########
@@ -1748,6 +1748,9 @@ private InetAddress getBindAddress() throws IOException {
if (bindHostName == null || bindHostName.isEmpty()) {
return null; // pick default local address
}
+ if (LocalHostUtil.isWildcardCharacter(bindHostName)) {
+ return InetAddress.getByName(LocalHostUtil.getWildcardIp());
+ }
Review comment:
This is the client's cache endpoint, configured via
`server-bind-address`.
While not strictly necessary for the Istio use case (since the default
before this PR is to bind to all interfaces), I can see why this is good for
consistency. If somebody uses `--server-bind-address=*` it'll bind to all
interfaces (as it would by default).
If the intention is to allow using a wildcard for `server-bind-address`, can
you please explain why no special handling of that parameter was required in
`StartServerCommand` or `StartLocatorCommand` (the way you had to add special
handling for `bind-address`)?
##########
File path:
geode-common/src/main/java/org/apache/geode/internal/inet/LocalHostUtil.java
##########
@@ -242,6 +242,21 @@ public static InetAddress getAnyLocalAddress() {
return new InetSocketAddress(0).getAddress();
}
+ public static boolean isWildcardAddress(String address) {
+ return "0.0.0.0".equals(address) || "::".equals(address) ||
isWildcardCharacter(address);
+ }
+
+ public static boolean isWildcardCharacter(String address) {
Review comment:
I believe this method returns true if the string is the wildcard
character defined `ServerLauncher` and `LocatorLauncher`'s `--bind-address`
(command line parameter).
Please add method-level javadoc explaining the context in which this is a
"wildcard character".
Need same explanation of context in javadoc on `isWildcardAddress()` too.
##########
File path:
geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/fd/GMSHealthMonitor.java
##########
@@ -877,14 +879,32 @@ public void init(Services<ID> s) throws
MembershipConfigurationException {
@Override
public void started() throws MemberStartupException {
setLocalAddress(services.getMessenger().getMemberID());
+ createServerSocket();
+ startTcpServer(serverSocket);
+ startHeartbeatThread();
+ }
+
+ private void createServerSocket() throws MemberStartupException {
try {
- serverSocket = createServerSocket(localAddress.getInetAddress(),
- services.getConfig().getMembershipPortRange());
+ InetAddress address = localAddress.getInetAddress();
+ String bindAddrStr = services.getConfig().getBindAddress();
+ if (bindAddrStr != null && !bindAddrStr.isEmpty()) {
+ try {
+ if (LocalHostUtil.isWildcardAddress(bindAddrStr)) {
+ address = LocalHostUtil.getAnyLocalAddress();
+ } else {
+ address = InetAddress.getByName(bindAddrStr);
+ }
+ } catch (UnknownHostException e) {
+ logger.error(
+ "Error when configuring {} as bind address in membership,
default address will be used. Exception: {}",
+ services.getConfig().getBindAddress(), e.getMessage());
Review comment:
I was going to recommend logging the full exception but I feel like
`e.getMessage()` is sufficient and reduces log clutter ✓
--
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]
