GEODE-9235: Allow binding to all addresses #6435

alb3rtobr · 2021-05-05T20:43:00Z

AS A geode contributor
I WANT TO be able to start members with option --bind-address=0.0.0.0, --bind-address=:: or --bind-address=*
SO THAT all TCP endpoints are bound to all interfaces.

It is not currently possible to start cluster members binding to all interfaces. That is using --bind-address=0.0.0.0 for ipv4 or --bind-address=:: for ipv6.

Geode binds the locator and server traffic port by default to 0.0.0.0, but the membership ports are bound by default to the local address.

Changing the default membership binding is not the best option, as it could cause problems in existing Geode deployments. But using --bind-address=0.0.0.0, --bind-address=:: or --bind-address=* should be accepted.

There is a use case that needs this binding, that is using Istio with Geode. One of the requirements set by Istio towards applications trying to integrate with it is that the application listening ports need to be bound to 0.0.0.0 address (which listens on all interfaces).

It is currently not possible to start a cluster using wildcard address due to if --bind-address is specified, its value will be used for all tcp endpoints and the udp membership endpoint created by JGroups. And as JGroups does not accept binding to all addresses, and exception is thrown. This behavior can be verified with WildcardAddressBindingDUnitTest.java tests.

...gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StartServerCommand.java

dschneider-pivotal

I didn't see any problems with this change. Consider the minor refactor I suggested

mhansonp · 2021-05-06T17:47:14Z

Great description!

alb3rtobr · 2021-05-12T16:40:05Z

@dschneider-pivotal I have implemented your suggestion. I was waiting some days to do it in case more reviews with suggestions were added. Thanks!

alb3rtobr · 2021-05-12T22:30:52Z

CI failed due to flaky test (https://issues.apache.org/jira/browse/GEODE-9242 )

alb3rtobr · 2021-06-23T08:57:26Z

rebase done in order to fix conflicts

Bill

Great PR @alb3rtobr !! In addition to comments and requests below please update the command-line options help for LocatorLauncher (bind-address) and ServerLauncher (bind-address, server-bind-address).

The bigger issue is that I don't see a test that verifies that if --bind-address=* on a multi-homed host, that both (health monitor) and (client's cache) services bind on all interfaces. I realize this isn't possible in a plain old DUnit test. But what about a Dockerized one along the lines of SingleServerSNIAcceptanceTest?

FYI and for other reviewers, this is my summary of endpoint configuration before/after this ticket:

...va/org/apache/geode/management/internal/cli/commands/StartLocatorCommandIntegrationTest.java

Bill · 2021-06-25T16:41:08Z

geode-common/src/main/java/org/apache/geode/internal/inet/LocalHostUtil.java

  }

+  public static boolean isWildcardAddress(String address) {
+    return "0.0.0.0".equals(address) || "::".equals(address) || isWildcardCharacter(address);


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):

/** * 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; }

Bill · 2021-06-25T16:50:49Z

geode-common/src/main/java/org/apache/geode/internal/inet/LocalHostUtil.java

        }
      }
    } else {
+      if (isWildcardAddress(host.toString())) {


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.

Bill · 2021-06-25T17:15:03Z

geode-common/src/main/java/org/apache/geode/internal/inet/LocalHostUtil.java

+    return "0.0.0.0".equals(address) || "::".equals(address) || isWildcardCharacter(address);
+  }
+
+  public static boolean isWildcardCharacter(String address) {


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.

Bill · 2021-06-25T17:20:46Z

geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java

+            address = LocalHostUtil.getAnyLocalAddress();
+          } else {
+            address = InetAddress.getByName(addressString);
+          }


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?

geode-docs/tools_modules/gfsh/command-pages/start.html.md.erb

Bill · 2021-06-25T18:02:54Z

...fsh/src/main/java/org/apache/geode/management/internal/cli/commands/StartLocatorCommand.java

    Properties gemfireProperties = new Properties();

+    StartMemberUtils.setPropertyIfNotNull(gemfireProperties, ConfigurationProperties.BIND_ADDRESS,
+        bindAddress);


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)?

Bill · 2021-06-25T18:05:22Z

.../src/main/java/org/apache/geode/distributed/internal/membership/gms/fd/GMSHealthMonitor.java

-      serverSocket = createServerSocket(localAddress.getInetAddress(),
-          services.getConfig().getMembershipPortRange());
+      InetAddress address = localAddress.getInetAddress();
+      String bindAddrStr = services.getConfig().getBindAddress();


Ooh good catch remembering to fix the health monitor endpoint too!

Bill · 2021-06-25T18:05:45Z

...in/java/org/apache/geode/distributed/internal/membership/gms/messenger/JGroupsMessenger.java

    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)) {


.../src/main/java/org/apache/geode/distributed/internal/membership/gms/fd/GMSHealthMonitor.java

alb3rtobr · 2021-07-28T13:59:14Z

Thanks for the review @Bill . I will try the dockerized test. In the meanwhile Im moving the PR to draft.

onichols-pivotal requested review from albertogpz and davebarnes97 May 5, 2021 20:44

alb3rtobr force-pushed the feature/GEODE-9235 branch from c1adfef to 0a7cb21 Compare May 5, 2021 22:33

alb3rtobr marked this pull request as ready for review May 6, 2021 07:52

alb3rtobr requested review from Bill, agingade, boglesby, dickcav, dschneider-pivotal, echobravopapa, jdeppe-pivotal, jinmeiliao, kirklund, mhansonp, nabarunnag and onichols-pivotal as code owners May 6, 2021 07:52

alb3rtobr requested a review from a user May 6, 2021 07:52

alb3rtobr force-pushed the feature/GEODE-9235 branch from 0a7cb21 to 8c08aab Compare May 6, 2021 07:54

dschneider-pivotal reviewed May 6, 2021

View reviewed changes

...gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StartServerCommand.java Outdated Show resolved Hide resolved

dschneider-pivotal approved these changes May 6, 2021

View reviewed changes

ghost approved these changes May 12, 2021

View reviewed changes

mhansonp approved these changes May 13, 2021

View reviewed changes

jdeppe-pivotal approved these changes May 19, 2021

View reviewed changes

jinmeiliao approved these changes May 20, 2021

View reviewed changes

kirklund approved these changes Jun 11, 2021

View reviewed changes

echobravopapa requested a review from kamilla1201 June 14, 2021 18:11

GEODE-9235: Allow binding to all addresses

487bc70

GEODE-9235: Changes after dschneider-pivotal review

f2ef6cd

alb3rtobr force-pushed the feature/GEODE-9235 branch from 886ac86 to f2ef6cd Compare June 23, 2021 08:55

Bill requested changes Jun 25, 2021

View reviewed changes

alb3rtobr marked this pull request as draft July 28, 2021 13:56

alb3rtobr closed this Oct 21, 2021

GEODE-9235: Allow binding to all addresses #6435

GEODE-9235: Allow binding to all addresses #6435

Uh oh!

Conversation

alb3rtobr commented May 5, 2021

Uh oh!

Uh oh!

dschneider-pivotal left a comment

Choose a reason for hiding this comment

Uh oh!

mhansonp commented May 6, 2021

Uh oh!

alb3rtobr commented May 12, 2021

Uh oh!

alb3rtobr commented May 12, 2021

Uh oh!

alb3rtobr commented Jun 23, 2021

Uh oh!

Bill left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Bill Jun 25, 2021

Choose a reason for hiding this comment

Uh oh!

Bill Jun 25, 2021

Choose a reason for hiding this comment

Uh oh!

Bill Jun 25, 2021

Choose a reason for hiding this comment

Uh oh!

Bill Jun 25, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Bill Jun 25, 2021

Choose a reason for hiding this comment

Uh oh!

Bill Jun 25, 2021

Choose a reason for hiding this comment

Uh oh!

Bill Jun 25, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alb3rtobr commented Jul 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants