You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by revans2 <gi...@git.apache.org> on 2018/09/28 19:43:15 UTC
[GitHub] zookeeper pull request #648: ZOOKEEPER-3156: Add in option to canonicalize h...
GitHub user revans2 opened a pull request:
https://github.com/apache/zookeeper/pull/648
ZOOKEEPER-3156: Add in option to canonicalize host name
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/revans2/zookeeper ZOOKEEPER-3156-3.4
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/648.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #648
----
commit 7ecd6c9151c0e7be749cfead5f141d673a8663f6
Author: Robert Evans <ev...@...>
Date: 2018-09-26T20:40:26Z
ZOOKEEPER-3156: Add in option to canonicalize host name
----
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/648
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2302/
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/zookeeper/pull/648
So I am running into some issues with the tests as all of the methods to InetSocketAddress are marked as final and as such I cannot mock them. This means I would need a set of host names that are real, and accessible from any build host you would want to run the test on and never going to be taken down, or I would need to create another class that wraps InetSocketAddress for the canonicalization that would allow me to do the mocking at that level. Not sure which you would prefer.
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/zookeeper/pull/648
There were some comments in the other pull request about the test code. I will address them in both places.
---
[GitHub] zookeeper pull request #648: ZOOKEEPER-3156: Add in option to canonicalize h...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/648#discussion_r221613169
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -997,12 +999,31 @@ private void startConnect(InetSocketAddress addr) throws IOException {
setName(getName().replaceAll("\\(.*\\)",
"(" + addr.getHostName() + ":" + addr.getPort() + ")"));
if (ZooKeeperSaslClient.isEnabled()) {
+ String hostName = addr.getHostName();
+
+ boolean canonicalize = true;
+ try {
+ canonicalize = Boolean.parseBoolean(System.getProperty(ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME, "true"));
+ } catch (IllegalArgumentException ea) {
+ //ignored ...
+ }
+
+ if (canonicalize) {
+ InetAddress ia = addr.getAddress();
+ if (ia == null) {
+ throw new IllegalArgumentException("Connection address should have already been resolved by the HostProvider.");
+ }
+ //Update the actual address so we are
+ hostName = ia.getCanonicalHostName();
--- End diff --
You might want to do the following:
```java
String canonicalHostName = ia.getCanonicalHostName();
if (!canonicalHostName.equals(ia.getHostAddress())) {
hostName = canonicalHostName;
}
```
In order to avoid using literal IP address when security check fails.
---
[GitHub] zookeeper pull request #648: ZOOKEEPER-3156: Add in option to canonicalize h...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/648#discussion_r221612869
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -997,12 +999,31 @@ private void startConnect(InetSocketAddress addr) throws IOException {
setName(getName().replaceAll("\\(.*\\)",
"(" + addr.getHostName() + ":" + addr.getPort() + ")"));
if (ZooKeeperSaslClient.isEnabled()) {
+ String hostName = addr.getHostName();
+
+ boolean canonicalize = true;
+ try {
+ canonicalize = Boolean.parseBoolean(System.getProperty(ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME, "true"));
+ } catch (IllegalArgumentException ea) {
+ //ignored ...
+ }
+
+ if (canonicalize) {
+ InetAddress ia = addr.getAddress();
+ if (ia == null) {
+ throw new IllegalArgumentException("Connection address should have already been resolved by the HostProvider.");
+ }
+ //Update the actual address so we are
+ hostName = ia.getCanonicalHostName();
+ LOG.debug("Canonicalized address to {}", hostName);
--- End diff --
+1
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/648
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2347/
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on the issue:
https://github.com/apache/zookeeper/pull/648
Do we have PowerMockito on ZK classpath for tests ?
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/zookeeper/pull/648
I just added in a test and made the code match closer to how master/3.5 work for the canonical host name.
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/648
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2322/
---
[GitHub] zookeeper pull request #648: ZOOKEEPER-3156: Add in option to canonicalize h...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/648#discussion_r221612755
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -997,12 +999,31 @@ private void startConnect(InetSocketAddress addr) throws IOException {
setName(getName().replaceAll("\\(.*\\)",
"(" + addr.getHostName() + ":" + addr.getPort() + ")"));
if (ZooKeeperSaslClient.isEnabled()) {
+ String hostName = addr.getHostName();
+
+ boolean canonicalize = true;
+ try {
+ canonicalize = Boolean.parseBoolean(System.getProperty(ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME, "true"));
+ } catch (IllegalArgumentException ea) {
+ //ignored ...
+ }
+
+ if (canonicalize) {
+ InetAddress ia = addr.getAddress();
+ if (ia == null) {
+ throw new IllegalArgumentException("Connection address should have already been resolved by the HostProvider.");
--- End diff --
Would you please change this to something like 'Unable to canonicalize address {}, because it's not resolveable'?
---
[GitHub] zookeeper pull request #648: ZOOKEEPER-3156: Add in option to canonicalize h...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/648#discussion_r221376870
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -990,6 +992,27 @@ private void sendPing() {
private boolean saslLoginFailed = false;
private void startConnect(InetSocketAddress addr) throws IOException {
+ boolean canonicalize = true;
+ try {
+ canonicalize = Boolean.parseBoolean(System.getProperty(ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME, "true"));
+ } catch (IllegalArgumentException ea) {
+ //ignored ...
+ }
+
+ if (canonicalize) {
+ try {
+ InetAddress ia = addr.getAddress();
+ LOG.warn("ia {}", ia);
+ if (ia == null) {
+ ia = InetAddress.getByName(addr.getHostName());
+ }
+ String host = (ia != null) ? ia.getCanonicalHostName() : addr.getHostName();
+ addr = new InetSocketAddress(InetAddress.getByAddress(host, ia.getAddress()), addr.getPort());
--- End diff --
I put it here because there is a possible race. The entire reason we have the setup we do is so that we can change the nodes in the cluster without changing the config on the client side. If the code that establishes the connection uses a different address from the code that creates the principal there is the possibility, because of the magic of DNS caching, that the connection would be made to a different box from the one the SASL client is expecting. If that happens, and we have the default Client section in the jaas conf, it will not result in an error. Instead the ZK client logs something about SASL failed and still connects but is not authorized to do anything. If it failed and retried a different node I would be happy to move it, but it does not and that failure would not be transparent to the end user.
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on the issue:
https://github.com/apache/zookeeper/pull/648
Overall looks good to me
What about adding a test case ?
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/zookeeper/pull/648
Thanks
---
[GitHub] zookeeper pull request #648: ZOOKEEPER-3156: Add in option to canonicalize h...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 closed the pull request at:
https://github.com/apache/zookeeper/pull/648
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/zookeeper/pull/648
I tried to add in powermock too, but it conflicts with mockito (power mock uses a much older version of mockito-core). I'll try and use a different API for powermock instead of mockito.
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/648
@eolivelli I like your idea about the unit test.
---
[GitHub] zookeeper pull request #648: ZOOKEEPER-3156: Add in option to canonicalize h...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/648#discussion_r221368841
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -990,6 +992,27 @@ private void sendPing() {
private boolean saslLoginFailed = false;
private void startConnect(InetSocketAddress addr) throws IOException {
+ boolean canonicalize = true;
+ try {
+ canonicalize = Boolean.parseBoolean(System.getProperty(ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME, "true"));
+ } catch (IllegalArgumentException ea) {
+ //ignored ...
+ }
+
+ if (canonicalize) {
+ try {
+ InetAddress ia = addr.getAddress();
+ LOG.warn("ia {}", ia);
+ if (ia == null) {
+ ia = InetAddress.getByName(addr.getHostName());
+ }
+ String host = (ia != null) ? ia.getCanonicalHostName() : addr.getHostName();
+ addr = new InetSocketAddress(InetAddress.getByAddress(host, ia.getAddress()), addr.getPort());
--- End diff --
I'm thinking of how much value does it have to replace the address with the canonicalized version for the entire client. We might want to implement this strictly and only for `ZooKeeperSaslClient`.
I'd rather move this logic to the creation of `ZooKeeperSaslClient`.
Something like:
```java
zooKeeperSaslClient = new ZooKeeperSaslClient(principalUserName + "/" + canonicalize ? addr.getAddress().getCanonicalHostName() : addr.getHostName());
```
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/648
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2323/
---
[GitHub] zookeeper pull request #648: ZOOKEEPER-3156: Add in option to canonicalize h...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/648#discussion_r221367697
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -990,6 +992,27 @@ private void sendPing() {
private boolean saslLoginFailed = false;
private void startConnect(InetSocketAddress addr) throws IOException {
+ boolean canonicalize = true;
+ try {
+ canonicalize = Boolean.parseBoolean(System.getProperty(ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME, "true"));
+ } catch (IllegalArgumentException ea) {
+ //ignored ...
+ }
+
+ if (canonicalize) {
--- End diff --
You only want to do this if SASL client is enabled. Hence the name of the switch contains `sasl.client`.
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/648
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2467/
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/zookeeper/pull/648
Looks like it is mockito-all not powermock-api-mockito. I can try and change it and see.
---
[GitHub] zookeeper pull request #648: ZOOKEEPER-3156: Add in option to canonicalize h...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/648#discussion_r221367723
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -990,6 +992,27 @@ private void sendPing() {
private boolean saslLoginFailed = false;
private void startConnect(InetSocketAddress addr) throws IOException {
+ boolean canonicalize = true;
+ try {
+ canonicalize = Boolean.parseBoolean(System.getProperty(ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME, "true"));
+ } catch (IllegalArgumentException ea) {
+ //ignored ...
+ }
+
+ if (canonicalize) {
+ try {
+ InetAddress ia = addr.getAddress();
+ LOG.warn("ia {}", ia);
--- End diff --
Debugging?
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/648
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2469/
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/648
Committed. Thanks @revans2 !
Please close this PR.
---
[GitHub] zookeeper pull request #648: ZOOKEEPER-3156: Add in option to canonicalize h...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/648#discussion_r221389383
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -997,12 +999,31 @@ private void startConnect(InetSocketAddress addr) throws IOException {
setName(getName().replaceAll("\\(.*\\)",
"(" + addr.getHostName() + ":" + addr.getPort() + ")"));
if (ZooKeeperSaslClient.isEnabled()) {
+ String hostName = addr.getHostName();
+
+ boolean canonicalize = true;
+ try {
+ canonicalize = Boolean.parseBoolean(System.getProperty(ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME, "true"));
+ } catch (IllegalArgumentException ea) {
+ //ignored ...
+ }
+
+ if (canonicalize) {
+ InetAddress ia = addr.getAddress();
+ if (ia == null) {
+ throw new IllegalArgumentException("Connection address should have already been resolved by the HostProvider.");
+ }
+ //Update the actual address so we are
+ hostName = ia.getCanonicalHostName();
+ LOG.debug("Canonicalized address to {}", hostName);
--- End diff --
Nit: if LOG.isDebugEnabled
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/648
retest this please
---
[GitHub] zookeeper pull request #648: ZOOKEEPER-3156: Add in option to canonicalize h...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/648#discussion_r221367991
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -990,6 +992,27 @@ private void sendPing() {
private boolean saslLoginFailed = false;
private void startConnect(InetSocketAddress addr) throws IOException {
+ boolean canonicalize = true;
+ try {
+ canonicalize = Boolean.parseBoolean(System.getProperty(ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME, "true"));
+ } catch (IllegalArgumentException ea) {
+ //ignored ...
+ }
+
+ if (canonicalize) {
+ try {
+ InetAddress ia = addr.getAddress();
+ LOG.warn("ia {}", ia);
+ if (ia == null) {
+ ia = InetAddress.getByName(addr.getHostName());
--- End diff --
If the original address is unresolved, you should throw exception immediately. That means HostProvider already tried to resolve the address, but failed. No point trying it again here.
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/zookeeper/pull/648
Sorry yes. I'll get on the test case...
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/648
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2284/
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/648
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2551/
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/648
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2299/
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/zookeeper/pull/648
Thanks for all of the reviews I just rebased and squashed commits.
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/648
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2326/
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/zookeeper/pull/648
@anmolnar I think I have addressed all of your comments. Please have another look.
---
[GitHub] zookeeper pull request #648: ZOOKEEPER-3156: Add in option to canonicalize h...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/648#discussion_r221377101
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -990,6 +992,27 @@ private void sendPing() {
private boolean saslLoginFailed = false;
private void startConnect(InetSocketAddress addr) throws IOException {
+ boolean canonicalize = true;
+ try {
+ canonicalize = Boolean.parseBoolean(System.getProperty(ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME, "true"));
+ } catch (IllegalArgumentException ea) {
+ //ignored ...
+ }
+
+ if (canonicalize) {
+ try {
+ InetAddress ia = addr.getAddress();
+ LOG.warn("ia {}", ia);
+ if (ia == null) {
+ ia = InetAddress.getByName(addr.getHostName());
+ }
+ String host = (ia != null) ? ia.getCanonicalHostName() : addr.getHostName();
+ addr = new InetSocketAddress(InetAddress.getByAddress(host, ia.getAddress()), addr.getPort());
--- End diff --
But you said that we already have the IP address, which is what it is going to use, so never mind, it should be fine.
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/zookeeper/pull/648
I just addressed the review comments if things look good I am happy to squash commits. I'll also put up pull requests to the other lines, now that it looks like we have the majority of the code worked out.
---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/648
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2282/
---