You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/04/12 21:05:30 UTC
[GitHub] [geode] bschuchardt opened a new pull request #6308: GEODE-9139 SSLException in starting up a Locator
bschuchardt opened a new pull request #6308:
URL: https://github.com/apache/geode/pull/6308
These changes preserve the bind-address passed to a Locator by Gfsh and modify the way hostnames are handled when SNI endpoint validation is enabled.
LocatorLauncher: Preserve the bind-address string specified by the user for cluster communications
JGroupsMessenger: Use the bind-address string or the Inetaddress host-name when forming the local identifier so it will be available in the membership view for other members of the cluster.
ServiceConfig: Allow use of DNS when SNI endpoint validation is enabled.
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #6308: GEODE-9139 SSLException in starting up a Locator
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6308:
URL: https://github.com/apache/geode/pull/6308#issuecomment-822678657
This pull request **fixes 1 alert** when merging b67be0484286240963965ab249f53add07d7887d into 5f7abc97b0ad1626e359e36efc12734feb42f7b2 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-8c48db9763843ee4aca2f969b7dbbe550526f5f7)
**fixed alerts:**
* 1 for Use of externally\-controlled format string
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] bschuchardt commented on a change in pull request #6308: GEODE-9139 SSLException in starting up a Locator
Posted by GitBox <gi...@apache.org>.
bschuchardt commented on a change in pull request #6308:
URL: https://github.com/apache/geode/pull/6308#discussion_r620405677
##########
File path: geode-tcp-server/src/main/java/org/apache/geode/distributed/internal/tcpserver/InetSocketWrapper.java
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.geode.distributed.internal.tcpserver;
+
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.util.Objects;
+
+import org.apache.commons.validator.routines.InetAddressValidator;
+
+class InetSocketWrapper {
+ String hostName;
+ protected InetSocketAddress inetSocketAddress;
+ private InetSocketAddress attemptedToResolve;
+
+ protected InetSocketWrapper() {}
+
+ protected InetSocketWrapper(String hostName, int port) {
+ this.hostName = hostName;
+ if (hostName == null) {
+ inetSocketAddress = new InetSocketAddress(port);
+ } else if (InetAddressValidator.getInstance().isValid(hostName)) {
+ // numeric address - use as-is
+ inetSocketAddress = new InetSocketAddress(hostName, port);
+ } else {
+ // non-numeric address - resolve hostname when needed
+ inetSocketAddress = InetSocketAddress.createUnresolved(hostName, port);
+ }
+ }
+
+ public String getHostName() {
+ if (hostName != null) {
+ return hostName;
+ }
Review comment:
I found that this code is necessary to preserve the original string passed in to the constructor. If you look at the constructor for InetSocketAddress you'll see that it discards the hostname parameter if it's a numeric address.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] Bill commented on a change in pull request #6308: GEODE-9139 SSLException in starting up a Locator
Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #6308:
URL: https://github.com/apache/geode/pull/6308#discussion_r612051364
##########
File path: geode-assembly/src/acceptanceTest/java/org/apache/geode/client/sni/DualServerSNIAcceptanceTest.java
##########
@@ -60,7 +61,7 @@
public class DualServerSNIAcceptanceTest {
private static final URL DOCKER_COMPOSE_PATH =
- SingleServerSNIAcceptanceTest.class.getResource("docker-compose.yml");
+ DualServerSNIAcceptanceTest.class.getResource("dual-server-docker-compose.yml");
Review comment:
good catch!
##########
File path: geode-assembly/src/acceptanceTest/resources/org/apache/geode/client/sni/scripts/geode-starter-2.gfsh
##########
@@ -15,7 +15,7 @@
# limitations under the License.
#
-start locator --name=locator-maeve --connect=false --redirect-output --hostname-for-clients=locator-maeve --properties-file=/geode/config/gemfire.properties --security-properties-file=/geode/config/gfsecurity.properties --J=-Dgemfire.ssl-keystore=/geode/config/locator-maeve-keystore.jks
+start locator --name=locator-maeve --connect=false --redirect-output --hostname-for-clients=locator-maeve --jmx-manager-hostname-for-clients=geode --properties-file=/geode/config/gemfire.properties --security-properties-file=/geode/config/gfsecurity.properties --J=-Dgemfire.ssl-keystore=/geode/config/locator-maeve-keystore.jks
Review comment:
I don't think this script is called any more. Please delete it.
##########
File path: geode-assembly/src/acceptanceTest/java/org/apache/geode/client/sni/GenerateSNIKeyAndTrustStores.java
##########
@@ -51,10 +50,8 @@ public void generateStores() throws Exception {
CertificateMaterial certificate = new CertificateBuilder(365 * 100, "SHA256withRSA")
.commonName(certName)
.issuedBy(ca)
- .sanDnsName("geode") // for inside the docker container
- .sanDnsName("localhost") // for inside the docker container
- .sanIpAddress(InetAddress.getByName("0.0.0.0")) // for inside the docker container
- .sanDnsName(certName) // for client endpoint validation
+ .sanDnsName(certName)
+ .sanDnsName("geode")
Review comment:
This looks better! Leaving off "localhost" and the address of the first interface seems much less hackey.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] Bill commented on a change in pull request #6308: GEODE-9139 SSLException in starting up a Locator
Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #6308:
URL: https://github.com/apache/geode/pull/6308#discussion_r617974408
##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/HostAddress.java
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.geode.distributed.internal.membership.api;
+
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.util.Objects;
+
+import org.apache.commons.validator.routines.InetAddressValidator;
+
+import org.apache.geode.distributed.internal.tcpserver.HostAndPort;
+
+/**
+ * HostAddress is a holder of a host name/address. It is the primary
+ * way to specify a host address that may or may not have been resolved to an InetAddress.
+ *
+ * @See HostAddress which can hold both a host name and a port
+ */
+public class HostAddress {
+
+ private InetSocketAddress address;
+
+ public HostAddress(String hostName) {
+ if (hostName == null) {
+ address = new InetSocketAddress(0);
+ } else if (InetAddressValidator.getInstance().isValid(hostName)) {
+ // numeric address - use as-is
+ address = new InetSocketAddress(hostName, 0);
+ } else {
+ // non-numeric address - resolve hostname when needed
+ address = InetSocketAddress.createUnresolved(hostName, 0);
+ }
+ }
+
+ public HostAddress(InetAddress address) {
+ if (address == null) {
+ throw new IllegalArgumentException("null parameters are not allowed");
+ }
+ this.address = new InetSocketAddress(address, 0);
+ }
+
+ public HostAddress(HostAndPort host) {
+ this.address = host.getSocketInetAddress();
+ }
+
+ /**
+ * Returns an InetSocketAddress for this host and port. An attempt is made to resolve the
+ * host name but if resolution fails an unresolved InetSocketAddress is returned. This return
+ * value will not hold an InetAddress, so calling getAddress() on it will return null.
+ */
+ private InetSocketAddress getInetSocketAddress() {
+ if (address.isUnresolved()) {
+ // note that this leaves the InetAddress null if the hostname isn't resolvable
+ return new InetSocketAddress(address.getHostString(), address.getPort());
+ } else {
+ return this.address;
+ }
+ }
+
+ public String getHostName() {
+ String hostString = address.getHostString();
+ if (hostString != null) {
+ return hostString;
+ }
Review comment:
I looked at `InetSocketAddress` (docs and implementation) and it looks like `getHostName()` already avoids the reverse lookup if the host string is already known. So I believe lines 73-76 are superfluous.
##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/HostAddress.java
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.geode.distributed.internal.membership.api;
+
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.util.Objects;
+
+import org.apache.commons.validator.routines.InetAddressValidator;
+
+import org.apache.geode.distributed.internal.tcpserver.HostAndPort;
+
+/**
+ * HostAddress is a holder of a host name/address. It is the primary
+ * way to specify a host address that may or may not have been resolved to an InetAddress.
+ *
+ * @See HostAddress which can hold both a host name and a port
+ */
+public class HostAddress {
+
+ private InetSocketAddress address;
+
+ public HostAddress(String hostName) {
+ if (hostName == null) {
+ address = new InetSocketAddress(0);
+ } else if (InetAddressValidator.getInstance().isValid(hostName)) {
+ // numeric address - use as-is
+ address = new InetSocketAddress(hostName, 0);
+ } else {
+ // non-numeric address - resolve hostname when needed
+ address = InetSocketAddress.createUnresolved(hostName, 0);
+ }
+ }
+
+ public HostAddress(InetAddress address) {
+ if (address == null) {
+ throw new IllegalArgumentException("null parameters are not allowed");
+ }
+ this.address = new InetSocketAddress(address, 0);
+ }
+
+ public HostAddress(HostAndPort host) {
+ this.address = host.getSocketInetAddress();
+ }
+
+ /**
+ * Returns an InetSocketAddress for this host and port. An attempt is made to resolve the
+ * host name but if resolution fails an unresolved InetSocketAddress is returned. This return
+ * value will not hold an InetAddress, so calling getAddress() on it will return null.
+ */
+ private InetSocketAddress getInetSocketAddress() {
+ if (address.isUnresolved()) {
+ // note that this leaves the InetAddress null if the hostname isn't resolvable
+ return new InetSocketAddress(address.getHostString(), address.getPort());
Review comment:
Shouldn't `HostAddress` cache this value before returning it?
Failing to do so causes us to resolve the hostname every single time the socket address is needed.
If this is a problem here, it's also a problem in the sister-class (or perhaps mother-class) `HostAndPort`—it also fails to cache the value.
Also I recommend calling this method `getSocketInetAddress` to match the one in `HostAndPort`.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #6308: GEODE-9139 SSLException in starting up a Locator
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6308:
URL: https://github.com/apache/geode/pull/6308#issuecomment-825104295
This pull request **fixes 1 alert** when merging 02882146ce4b9dd0322ad585dc812979b9b1af95 into 78598cf19796fe6a98d5b94f5e40892cace68fe5 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-ffb50ac91b199cba8901b2bf998f570d35d82647)
**fixed alerts:**
* 1 for Use of externally\-controlled format string
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #6308: GEODE-9139 SSLException in starting up a Locator
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6308:
URL: https://github.com/apache/geode/pull/6308#issuecomment-820564619
This pull request **introduces 2 alerts** and **fixes 1** when merging 17df7c2e8dd426a71ae7d67a33ca44a38743f909 into 3b4d4acb7a981fec56125cde6d235d5a4db84dff - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-cea1102ad179e124a033f3ff00c478c55eb21550)
**new alerts:**
* 1 for Self assignment
* 1 for Dereferenced variable may be null
**fixed alerts:**
* 1 for Use of externally\-controlled format string
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kamilla1201 commented on a change in pull request #6308: GEODE-9139 SSLException in starting up a Locator
Posted by GitBox <gi...@apache.org>.
kamilla1201 commented on a change in pull request #6308:
URL: https://github.com/apache/geode/pull/6308#discussion_r617113620
##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/HostAddress.java
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.geode.distributed.internal.membership.api;
+
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.util.Objects;
+
+import org.apache.commons.validator.routines.InetAddressValidator;
+
+import org.apache.geode.distributed.internal.tcpserver.HostAndPort;
+
+/**
+ * HostAddress is a holder of a host name/address. It is the primary
+ * way to specify a host address that may or may not have been resolved to an InetAddress.
+ *
+ * @See HostAddress which can hold both a hast name and a port
Review comment:
Typo in "host" here
##########
File path: geode-assembly/src/acceptanceTest/java/org/apache/geode/client/sni/DualServerSNIAcceptanceTest.java
##########
@@ -97,6 +107,17 @@ public void after() {
ensureCacheClosed();
}
+ @AfterClass
+ public static void afterClass() throws Exception {
+ // try {
+ // String output =
+ // docker.get().exec(options("-T"), "locator-maeve",
Review comment:
Maybe it's worth adding a comment explaining why this commented code is useful and should not be removed
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/Locator.java
##########
@@ -101,7 +105,7 @@
*/
public static Locator startLocator(int port, File logFile) throws IOException {
- return startLocator(port, logFile, false, (InetAddress) null, (Properties) null, true, true,
+ return startLocator(port, logFile, false, (HostAddress) null, (Properties) null, true, true,
Review comment:
Here and in a few other places in this file (that call startLocator) casting null args to HostAddress or Properties seems redundant. Is there a reason to keep that anyway?
##########
File path: geode-core/src/main/java/org/apache/geode/internal/DistributionLocator.java
##########
@@ -136,28 +139,27 @@ public static void main(String args[]) {
}
if (!Boolean.getBoolean(InternalDistributedSystem.DISABLE_SHUTDOWN_HOOK_PROPERTY)) {
- final InetAddress faddress = address;
+ final HostAddress faddress = hostAddress;
Runtime.getRuntime()
.addShutdownHook(new LoggingThread("LocatorShutdownThread", false, () -> {
try {
- DistributionLocator.shutdown(port, faddress);
+ DistributionLocator.shutdown(port, faddress.getAddress());
} catch (IOException e) {
e.printStackTrace();
}
}));
}
- lockFile = ManagerInfo.setLocatorStarting(directory, port, address);
+ lockFile = ManagerInfo.setLocatorStarting(directory, port,
+ hostAddress == null ? null : hostAddress.getAddress());
Review comment:
Looks like this could be simplified by creating the `address` variable a bit earlier
```
final InetAddress address = hostAddress == null ? null : hostAddress.getAddress();
lockFile = ManagerInfo.setLocatorStarting(directory, port, address);
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #6308: GEODE-9139 SSLException in starting up a Locator
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6308:
URL: https://github.com/apache/geode/pull/6308#issuecomment-825198234
This pull request **fixes 1 alert** when merging bfd742c3f1f481574214749ad642794fe4c2ebf1 into 78598cf19796fe6a98d5b94f5e40892cace68fe5 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-5835075744f1f918e4f8b947ee5c5cadd4ae5980)
**fixed alerts:**
* 1 for Use of externally\-controlled format string
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] bschuchardt commented on pull request #6308: GEODE-9139 SSLException in starting up a Locator
Posted by GitBox <gi...@apache.org>.
bschuchardt commented on pull request #6308:
URL: https://github.com/apache/geode/pull/6308#issuecomment-822559963
@Bill, I couldn't use the HostAndPort class without making things crazy because bind-address is optional but the locator port is not. I would have to modify HostAndPort to make the host address optional, which would change the meaning of that class too much.
Instead I created a new HostAddress class that holds an InetSocketAddress based either on an InetAddress or a hostName, combining the two parameters that were in the original PR.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] Bill commented on a change in pull request #6308: GEODE-9139 SSLException in starting up a Locator
Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #6308:
URL: https://github.com/apache/geode/pull/6308#discussion_r618833961
##########
File path: geode-tcp-server/src/main/java/org/apache/geode/distributed/internal/tcpserver/InetSocketWrapper.java
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.geode.distributed.internal.tcpserver;
+
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.util.Objects;
+
+import org.apache.commons.validator.routines.InetAddressValidator;
+
+class InetSocketWrapper {
+ String hostName;
+ protected InetSocketAddress inetSocketAddress;
+ private InetSocketAddress attemptedToResolve;
+
+ protected InetSocketWrapper() {}
+
+ protected InetSocketWrapper(String hostName, int port) {
+ this.hostName = hostName;
+ if (hostName == null) {
+ inetSocketAddress = new InetSocketAddress(port);
+ } else if (InetAddressValidator.getInstance().isValid(hostName)) {
+ // numeric address - use as-is
+ inetSocketAddress = new InetSocketAddress(hostName, port);
+ } else {
+ // non-numeric address - resolve hostname when needed
+ inetSocketAddress = InetSocketAddress.createUnresolved(hostName, port);
+ }
+ }
+
+ public String getHostName() {
+ if (hostName != null) {
+ return hostName;
+ }
Review comment:
This checking is superfluous because `InetSocketAddress.getHostName()` does this same `null` check.
##########
File path: geode-tcp-server/src/main/java/org/apache/geode/distributed/internal/tcpserver/HostAddress.java
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.geode.distributed.internal.tcpserver;
+
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+
+
+/**
+ * HostAddress is a holder of a host name/address. It is the primary
+ * way to specify a host address that may or may not have been resolved to an InetAddress.
+ *
+ * @See HostAndPort which can hold both a host name and a port
+ */
+public class HostAddress extends InetSocketWrapper {
+
+ public HostAddress(String hostName) {
+ super(hostName, 0);
+ }
+
+ public HostAddress(InetAddress address) {
+ if (address == null) {
+ throw new IllegalArgumentException("null parameters are not allowed");
+ }
+ inetSocketAddress = new InetSocketAddress(address, 0);
+ }
+
+ public HostAddress(HostAndPort hostAndPort) {
+ hostName = hostAndPort.hostName;
+ inetSocketAddress = hostAndPort.inetSocketAddress;
+ }
+
+ @Override
+ public String toString() {
+ if (hostName != null) {
+ return hostName;
Review comment:
If we get rid of `hostName` in the base class this could change to maybe:
```
final String hostName = getHostName();
if (hostName == null)
return inetSocketAddress.getAddress().toString();
else
return hostName;
```
##########
File path: geode-tcp-server/src/main/java/org/apache/geode/distributed/internal/tcpserver/InetSocketWrapper.java
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.geode.distributed.internal.tcpserver;
+
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.util.Objects;
+
+import org.apache.commons.validator.routines.InetAddressValidator;
+
+class InetSocketWrapper {
+ String hostName;
Review comment:
Why does this class need to track `hostName`? Doesn't `InetSocketAddress` adequately manage that state?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #6308: GEODE-9139 SSLException in starting up a Locator
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6308:
URL: https://github.com/apache/geode/pull/6308#issuecomment-822597042
This pull request **introduces 2 alerts** and **fixes 1** when merging 26aa838e8d49fed1b68ec2aab850548b9b99a012 into 5f7abc97b0ad1626e359e36efc12734feb42f7b2 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-edffa09371f8b767fa17b0c5b099824af7e20ac3)
**new alerts:**
* 2 for Dereferenced variable may be null
**fixed alerts:**
* 1 for Use of externally\-controlled format string
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #6308: GEODE-9139 SSLException in starting up a Locator
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6308:
URL: https://github.com/apache/geode/pull/6308#issuecomment-824404986
This pull request **fixes 1 alert** when merging ac0c736033cba5c8fc887fbd5d8a3ad820cf7617 into f309ffe8df66e7cd1a2fd753cb8398ae4efdd2e4 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-9367d5f37677ff020e0cb0d9940afdba41dc1de8)
**fixed alerts:**
* 1 for Use of externally\-controlled format string
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #6308: GEODE-9139 SSLException in starting up a Locator
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6308:
URL: https://github.com/apache/geode/pull/6308#discussion_r612004007
##########
File path: geode-assembly/src/acceptanceTest/java/org/apache/geode/client/sni/DualServerSNIAcceptanceTest.java
##########
@@ -97,6 +107,17 @@ public void after() {
ensureCacheClosed();
}
+ @AfterClass
+ public static void afterClass() throws Exception {
+ // try {
+ // String output =
+ // docker.get().exec(options("-T"), "locator-maeve",
Review comment:
Why are we adding new commented code here?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] bschuchardt merged pull request #6308: GEODE-9139 SSLException in starting up a Locator
Posted by GitBox <gi...@apache.org>.
bschuchardt merged pull request #6308:
URL: https://github.com/apache/geode/pull/6308
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] bschuchardt commented on a change in pull request #6308: GEODE-9139 SSLException in starting up a Locator
Posted by GitBox <gi...@apache.org>.
bschuchardt commented on a change in pull request #6308:
URL: https://github.com/apache/geode/pull/6308#discussion_r612538954
##########
File path: geode-assembly/src/acceptanceTest/java/org/apache/geode/client/sni/DualServerSNIAcceptanceTest.java
##########
@@ -97,6 +107,17 @@ public void after() {
ensureCacheClosed();
}
+ @AfterClass
+ public static void afterClass() throws Exception {
+ // try {
+ // String output =
+ // docker.get().exec(options("-T"), "locator-maeve",
Review comment:
This commented-out code is extremely useful in debugging problems & I don't want to lose track of it again.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6308: GEODE-9139 SSLException in starting up a Locator
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6308:
URL: https://github.com/apache/geode/pull/6308#discussion_r618409564
##########
File path: gradle/warnings.gradle
##########
@@ -16,6 +16,6 @@
*/
tasks.withType(JavaCompile) {
- options.compilerArgs << '-Xlint:unchecked' << "-Werror"
- options.deprecation = true
+// options.compilerArgs << '-Xlint:unchecked' << "-Werror"
+// options.deprecation = true
}
Review comment:
You probably didn't mean to leave these commented 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #6308: GEODE-9139 SSLException in starting up a Locator
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6308:
URL: https://github.com/apache/geode/pull/6308#issuecomment-821441339
This pull request **introduces 2 alerts** and **fixes 1** when merging 8f7bfaeeea3350e0e1909c6cd20338c8ef763407 into 4636f2969423a02ede2d9e638b5fea7201729ff2 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-35794be8737ae80ee5180673ea4e2f91928c9960)
**new alerts:**
* 1 for Self assignment
* 1 for Dereferenced variable may be null
**fixed alerts:**
* 1 for Use of externally\-controlled format string
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] bschuchardt commented on a change in pull request #6308: GEODE-9139 SSLException in starting up a Locator
Posted by GitBox <gi...@apache.org>.
bschuchardt commented on a change in pull request #6308:
URL: https://github.com/apache/geode/pull/6308#discussion_r620398568
##########
File path: geode-tcp-server/src/main/java/org/apache/geode/distributed/internal/tcpserver/InetSocketWrapper.java
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.geode.distributed.internal.tcpserver;
+
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.util.Objects;
+
+import org.apache.commons.validator.routines.InetAddressValidator;
+
+class InetSocketWrapper {
+ String hostName;
Review comment:
It does not adequately track the original host name. There is a unit test that failed after setting a bind address of 127.0.0.1 and InetSocketAddress somehow converted that to "localhost".
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #6308: GEODE-9139 SSLException in starting up a Locator
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6308:
URL: https://github.com/apache/geode/pull/6308#issuecomment-824241552
This pull request **fixes 1 alert** when merging c5128227793975ba0a34f71c3a98428f81854078 into 1ceca8105f3f253ad03a17a25a44b15963034ac3 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-572d084a51cddd7c9f0a57c6314f481c5092a7d0)
**fixed alerts:**
* 1 for Use of externally\-controlled format string
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] bschuchardt commented on a change in pull request #6308: GEODE-9139 SSLException in starting up a Locator
Posted by GitBox <gi...@apache.org>.
bschuchardt commented on a change in pull request #6308:
URL: https://github.com/apache/geode/pull/6308#discussion_r618474492
##########
File path: gradle/warnings.gradle
##########
@@ -16,6 +16,6 @@
*/
tasks.withType(JavaCompile) {
- options.compilerArgs << '-Xlint:unchecked' << "-Werror"
- options.deprecation = true
+// options.compilerArgs << '-Xlint:unchecked' << "-Werror"
+// options.deprecation = true
}
Review comment:
ew, thanks for catching that Jens. I was struggling with the Gradle upgrade in IJ when I made that change
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #6308: GEODE-9139 SSLException in starting up a Locator
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6308:
URL: https://github.com/apache/geode/pull/6308#issuecomment-821629637
This pull request **introduces 1 alert** and **fixes 1** when merging 74282975f62e9cd34c8679b2a65557a5a1e524cc into e65228ff6286d2f94c374f27c56708a5b6871f23 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-bbff3225cdd3d732983632965191d97fb661dfa1)
**new alerts:**
* 1 for Dereferenced variable may be null
**fixed alerts:**
* 1 for Use of externally\-controlled format string
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] bschuchardt commented on a change in pull request #6308: GEODE-9139 SSLException in starting up a Locator
Posted by GitBox <gi...@apache.org>.
bschuchardt commented on a change in pull request #6308:
URL: https://github.com/apache/geode/pull/6308#discussion_r620410036
##########
File path: geode-tcp-server/src/main/java/org/apache/geode/distributed/internal/tcpserver/HostAddress.java
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.geode.distributed.internal.tcpserver;
+
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+
+
+/**
+ * HostAddress is a holder of a host name/address. It is the primary
+ * way to specify a host address that may or may not have been resolved to an InetAddress.
+ *
+ * @See HostAndPort which can hold both a host name and a port
+ */
+public class HostAddress extends InetSocketWrapper {
+
+ public HostAddress(String hostName) {
+ super(hostName, 0);
+ }
+
+ public HostAddress(InetAddress address) {
+ if (address == null) {
+ throw new IllegalArgumentException("null parameters are not allowed");
+ }
+ inetSocketAddress = new InetSocketAddress(address, 0);
+ }
+
+ public HostAddress(HostAndPort hostAndPort) {
+ hostName = hostAndPort.hostName;
+ inetSocketAddress = hostAndPort.inetSocketAddress;
+ }
+
+ @Override
+ public String toString() {
+ if (hostName != null) {
+ return hostName;
Review comment:
To do this I would have to delete a gfsh unit test that requires the Locator startup command to have a numeric address as the bind address if that is what was specified by the user.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] bschuchardt commented on a change in pull request #6308: GEODE-9139 SSLException in starting up a Locator
Posted by GitBox <gi...@apache.org>.
bschuchardt commented on a change in pull request #6308:
URL: https://github.com/apache/geode/pull/6308#discussion_r617935979
##########
File path: geode-assembly/src/acceptanceTest/java/org/apache/geode/client/sni/DualServerSNIAcceptanceTest.java
##########
@@ -97,6 +107,17 @@ public void after() {
ensureCacheClosed();
}
+ @AfterClass
+ public static void afterClass() throws Exception {
+ // try {
+ // String output =
+ // docker.get().exec(options("-T"), "locator-maeve",
Review comment:
comment added
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #6308: GEODE-9139 SSLException in starting up a Locator
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6308:
URL: https://github.com/apache/geode/pull/6308#issuecomment-827036822
This pull request **fixes 1 alert** when merging 2d2246781d5bc31233a3223074d327e7fbdda927 into 8665c1d97667e1116b5ad371045f00632032378d - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-d301eccd4894e0c50e7ddb29248240232473fc46)
**fixed alerts:**
* 1 for Use of externally\-controlled format string
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org