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/10/01 20:32:16 UTC
[GitHub] zookeeper pull request #652: ZOOKEEPER-3156: Add in option to canonicalize h...
GitHub user revans2 opened a pull request:
https://github.com/apache/zookeeper/pull/652
ZOOKEEPER-3156: Add in option to canonicalize host name
This is the master and 3.5 version of #648. It should apply cleanly to both lines, but if you want a separate pull request for each I am happy to do it.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/revans2/zookeeper ZOOKEEPER-3156
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/652.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 #652
----
commit 24aca04adf57d10df123e2d61f55e2eb1344c014
Author: Robert Evans <ev...@...>
Date: 2018-09-26T20:40:26Z
ZOOKEEPER-3156: Add in option to canonicalize host name
----
---
[GitHub] zookeeper pull request #652: 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/652#discussion_r221937665
--- Diff: src/java/test/org/apache/zookeeper/ClientCanonicalizeTest.java ---
@@ -0,0 +1,59 @@
+/**
+ * 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.zookeeper;
+
+import java.io.IOException;
+import org.apache.zookeeper.client.ZKClientConfig;
+import org.junit.Assert;
+import org.junit.Test;
+
+import static org.mockito.Mockito.*;
--- End diff --
Please avoid using asterisk import.
---
[GitHub] zookeeper issue #652: 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/652
Ping @eolivelli @lvfangmin @anmolnar
I think I have addressed all of the concerns on both branches. The test failure here looks unrelated to the changes that I made.
---
[GitHub] zookeeper pull request #652: 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/652#discussion_r221936001
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -793,7 +794,87 @@ public RWServerFoundException(String msg) {
super(msg);
}
}
-
+
+ static class MockableInetSocketAddress {
--- End diff --
I agree. Unfortunately methods of `InetSocketAddress` and `InetAddress` are final and cannot be mocked. There's a Mockito extension which could help, but it's quite recent and an opt-in feature:
https://github.com/mockito/mockito/wiki/What's-new-in-Mockito-2#mock-the-unmockable-opt-in-mocking-of-final-classesmethods
Implementing wrapper classes is good approach, but I wouldn't put them into `ClientCnxn.java`. I suggest creating a new file with wrapper classes (nit: renaming them to \*Wrapper instead of Mockable\* would be better) and also put `getServerPrincipal()` into this file as well.
---
[GitHub] zookeeper issue #652: 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/652
@revans2 Please rebase.
---
[GitHub] zookeeper pull request #652: 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/652#discussion_r221928277
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -793,7 +794,87 @@ public RWServerFoundException(String msg) {
super(msg);
}
}
-
+
+ static class MockableInetSocketAddress {
+ private final InetSocketAddress addr;
+
+ MockableInetSocketAddress(InetSocketAddress addr) {
+ this.addr = addr;
+ }
+
+ public String getHostName() {
+ return addr.getHostName();
+ }
+
+ public MockableInetAddress getAddress() {
+ InetAddress ia = addr.getAddress();
+ return ia == null ? null : new MockableInetAddress(ia);
+ }
+
+ @Override
+ public String toString() {
+ return addr.toString();
+ }
+ }
+
+ static class MockableInetAddress {
+ private final InetAddress ia;
+
+ MockableInetAddress(InetAddress ia) {
+ this.ia = ia;
+ }
+
+ public String getCanonicalHostName() {
+ return ia.getCanonicalHostName();
+ }
+
+ public String getHostAddress() {
+ return ia.getHostAddress();
+ }
+
+ @Override
+ public String toString() {
+ return ia.toString();
+ }
+ }
+ /**
+ * Get the name of the server principal for a SASL client. This is visible for testing purposes.
+ * @param addr the address of the host.
+ * @param clientConfig the configuration for the client.
+ * @return the name of the principal.
+ */
+ static String getServerPrincipal(MockableInetSocketAddress addr, ZKClientConfig clientConfig) {
+ String principalUserName = clientConfig.getProperty(ZKClientConfig.ZK_SASL_CLIENT_USERNAME,
+ ZKClientConfig.ZK_SASL_CLIENT_USERNAME_DEFAULT);
+ String hostName = addr.getHostName();
+
+ boolean canonicalize = true;
+ try {
+ canonicalize = Boolean.parseBoolean(clientConfig.getProperty(ZKClientConfig.ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME,
+ ZKClientConfig.ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME_DEFAULT));
+ } catch (IllegalArgumentException ea) {
--- End diff --
IllegalArgumentException is thrown when you pass an empty string to `getProperty()` which is not going to happen, because we use a constant value for that, but must be caught according to Java.
---
[GitHub] zookeeper pull request #652: 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/652#discussion_r221749153
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -1102,7 +1103,32 @@ private void startConnect(InetSocketAddress addr) throws IOException {
private String getServerPrincipal(InetSocketAddress addr) {
String principalUserName = clientConfig.getProperty(ZKClientConfig.ZK_SASL_CLIENT_USERNAME,
ZKClientConfig.ZK_SASL_CLIENT_USERNAME_DEFAULT);
- String serverPrincipal = principalUserName + "/" + addr.getHostString();
+ String hostName = addr.getHostName();
+
+ boolean canonicalize = true;
+ try {
+ canonicalize = Boolean.parseBoolean(System.getProperty(ZKClientConfig.ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME,
--- End diff --
Great point, missed that on the port.
---
[GitHub] zookeeper issue #652: 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/652
@anmolnar sorry it took me so long. I was out on vacation.
---
[GitHub] zookeeper pull request #652: 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/652#discussion_r221937931
--- Diff: src/java/test/org/apache/zookeeper/ClientCanonicalizeTest.java ---
@@ -0,0 +1,59 @@
+/**
+ * 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.zookeeper;
+
+import java.io.IOException;
+import org.apache.zookeeper.client.ZKClientConfig;
+import org.junit.Assert;
+import org.junit.Test;
+
+import static org.mockito.Mockito.*;
+
+public class ClientCanonicalizeTest extends ZKTestCase {
+ @Test
+ public void testClientCanonicalization() throws IOException, InterruptedException {
+ ClientCnxn.MockableInetSocketAddress addr = mock(ClientCnxn.MockableInetSocketAddress.class);
+ ClientCnxn.MockableInetAddress ia = mock(ClientCnxn.MockableInetAddress.class);
+
+ when(addr.getHostName()).thenReturn("zookeeper.apache.org");
+ when(addr.getAddress()).thenReturn(ia);
+ when(ia.getCanonicalHostName()).thenReturn("zk1.apache.org");
+ when(ia.getHostAddress()).thenReturn("127.0.0.1");
+
+ ZKClientConfig conf = new ZKClientConfig();
+ String principal = ClientCnxn.getServerPrincipal(addr, conf);
+ Assert.assertEquals("zookeeper/zk1.apache.org", principal);
+ }
+
+ @Test
+ public void testClientNoCanonicalization() throws IOException, InterruptedException {
+ ClientCnxn.MockableInetSocketAddress addr = mock(ClientCnxn.MockableInetSocketAddress.class);
+ ClientCnxn.MockableInetAddress ia = mock(ClientCnxn.MockableInetAddress.class);
+
+ when(addr.getHostName()).thenReturn("zookeeper.apache.org");
+ when(addr.getAddress()).thenReturn(ia);
+ when(ia.getCanonicalHostName()).thenReturn("zk1.apache.org");
+ when(ia.getHostAddress()).thenReturn("127.0.0.1");
+
+ //when(addr.get)
+ ZKClientConfig conf = new ZKClientConfig();
+ conf.setProperty(ZKClientConfig.ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME, "false");
+ String principal = ClientCnxn.getServerPrincipal(addr, conf);
+ Assert.assertEquals("zookeeper/zookeeper.apache.org", principal);
--- End diff --
Same here.
---
[GitHub] zookeeper pull request #652: ZOOKEEPER-3156: Add in option to canonicalize h...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/652#discussion_r221835045
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -793,7 +794,87 @@ public RWServerFoundException(String msg) {
super(msg);
}
}
-
+
+ static class MockableInetSocketAddress {
+ private final InetSocketAddress addr;
+
+ MockableInetSocketAddress(InetSocketAddress addr) {
+ this.addr = addr;
+ }
+
+ public String getHostName() {
+ return addr.getHostName();
+ }
+
+ public MockableInetAddress getAddress() {
+ InetAddress ia = addr.getAddress();
+ return ia == null ? null : new MockableInetAddress(ia);
+ }
+
+ @Override
+ public String toString() {
+ return addr.toString();
+ }
+ }
+
+ static class MockableInetAddress {
+ private final InetAddress ia;
+
+ MockableInetAddress(InetAddress ia) {
+ this.ia = ia;
+ }
+
+ public String getCanonicalHostName() {
+ return ia.getCanonicalHostName();
+ }
+
+ public String getHostAddress() {
+ return ia.getHostAddress();
+ }
+
+ @Override
+ public String toString() {
+ return ia.toString();
+ }
+ }
+ /**
+ * Get the name of the server principal for a SASL client. This is visible for testing purposes.
+ * @param addr the address of the host.
+ * @param clientConfig the configuration for the client.
+ * @return the name of the principal.
+ */
+ static String getServerPrincipal(MockableInetSocketAddress addr, ZKClientConfig clientConfig) {
+ String principalUserName = clientConfig.getProperty(ZKClientConfig.ZK_SASL_CLIENT_USERNAME,
+ ZKClientConfig.ZK_SASL_CLIENT_USERNAME_DEFAULT);
+ String hostName = addr.getHostName();
+
+ boolean canonicalize = true;
+ try {
+ canonicalize = Boolean.parseBoolean(clientConfig.getProperty(ZKClientConfig.ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME,
+ ZKClientConfig.ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME_DEFAULT));
+ } catch (IllegalArgumentException ea) {
--- End diff --
Should we print the log when IllegalArgumentException throws?
---
[GitHub] zookeeper issue #652: 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/652
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2301/
---
[GitHub] zookeeper issue #652: 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/652
I just rebased to deal with the directories being moved around.
---
[GitHub] zookeeper pull request #652: 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/652#discussion_r221937882
--- Diff: src/java/test/org/apache/zookeeper/ClientCanonicalizeTest.java ---
@@ -0,0 +1,59 @@
+/**
+ * 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.zookeeper;
+
+import java.io.IOException;
+import org.apache.zookeeper.client.ZKClientConfig;
+import org.junit.Assert;
+import org.junit.Test;
+
+import static org.mockito.Mockito.*;
+
+public class ClientCanonicalizeTest extends ZKTestCase {
+ @Test
+ public void testClientCanonicalization() throws IOException, InterruptedException {
+ ClientCnxn.MockableInetSocketAddress addr = mock(ClientCnxn.MockableInetSocketAddress.class);
+ ClientCnxn.MockableInetAddress ia = mock(ClientCnxn.MockableInetAddress.class);
+
+ when(addr.getHostName()).thenReturn("zookeeper.apache.org");
+ when(addr.getAddress()).thenReturn(ia);
+ when(ia.getCanonicalHostName()).thenReturn("zk1.apache.org");
+ when(ia.getHostAddress()).thenReturn("127.0.0.1");
+
+ ZKClientConfig conf = new ZKClientConfig();
+ String principal = ClientCnxn.getServerPrincipal(addr, conf);
+ Assert.assertEquals("zookeeper/zk1.apache.org", principal);
--- End diff --
Please add some comment to the assert to describe the scenario when it fails.
---
[GitHub] zookeeper issue #652: 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/652
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2321/
---
[GitHub] zookeeper pull request #652: ZOOKEEPER-3156: Add in option to canonicalize h...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/652
---
[GitHub] zookeeper issue #652: 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/652
Ping any hope in getting this merged in?
---
[GitHub] zookeeper pull request #652: ZOOKEEPER-3156: Add in option to canonicalize h...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/652#discussion_r221836418
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -793,7 +794,87 @@ public RWServerFoundException(String msg) {
super(msg);
}
}
-
+
+ static class MockableInetSocketAddress {
--- End diff --
It's adding too much complexity to the code in order to test it, are we able to create the stub to extend the InetSocketAddress in the test case itself?
---
[GitHub] zookeeper issue #652: 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/652
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2300/
---
[GitHub] zookeeper issue #652: 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/652
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2324/
---
[GitHub] zookeeper pull request #652: 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/652#discussion_r221747922
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -1102,7 +1103,32 @@ private void startConnect(InetSocketAddress addr) throws IOException {
private String getServerPrincipal(InetSocketAddress addr) {
String principalUserName = clientConfig.getProperty(ZKClientConfig.ZK_SASL_CLIENT_USERNAME,
ZKClientConfig.ZK_SASL_CLIENT_USERNAME_DEFAULT);
- String serverPrincipal = principalUserName + "/" + addr.getHostString();
+ String hostName = addr.getHostName();
+
+ boolean canonicalize = true;
+ try {
+ canonicalize = Boolean.parseBoolean(System.getProperty(ZKClientConfig.ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME,
--- End diff --
Isn't it clientConfig instead of System ?
---
[GitHub] zookeeper issue #652: 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/652
@anmolnar and @lvfangmin I think I have addressed all of your review comments. I named the new class SaslServerPrincipal but if you have a different idea for a name I am happy to change it.
---
[GitHub] zookeeper issue #652: 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/652
Thanks for all of the reviews I just rebased and squashed commits.
---
[GitHub] zookeeper issue #652: 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/652
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2459/
---
[GitHub] zookeeper issue #652: 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/652
Looks great now
Like for branch-3.4 I think we'd better to have some test
---
[GitHub] zookeeper issue #652: 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/652
Merged to 3.5 and master branches. Thanks @revans2 !
---
[GitHub] zookeeper issue #652: 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/652
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2371/
---
[GitHub] zookeeper issue #652: 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/652
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2549/
---
[GitHub] zookeeper issue #652: 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/652
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2325/
---
[GitHub] zookeeper issue #652: 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/652
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2348/
---
[GitHub] zookeeper issue #652: 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/652
@lvfangmin are u happy with the change?
---
[GitHub] zookeeper pull request #652: 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/652#discussion_r221963829
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -793,7 +794,87 @@ public RWServerFoundException(String msg) {
super(msg);
}
}
-
+
+ static class MockableInetSocketAddress {
+ private final InetSocketAddress addr;
+
+ MockableInetSocketAddress(InetSocketAddress addr) {
+ this.addr = addr;
+ }
+
+ public String getHostName() {
+ return addr.getHostName();
+ }
+
+ public MockableInetAddress getAddress() {
+ InetAddress ia = addr.getAddress();
+ return ia == null ? null : new MockableInetAddress(ia);
+ }
+
+ @Override
+ public String toString() {
+ return addr.toString();
+ }
+ }
+
+ static class MockableInetAddress {
+ private final InetAddress ia;
+
+ MockableInetAddress(InetAddress ia) {
+ this.ia = ia;
+ }
+
+ public String getCanonicalHostName() {
+ return ia.getCanonicalHostName();
+ }
+
+ public String getHostAddress() {
+ return ia.getHostAddress();
+ }
+
+ @Override
+ public String toString() {
+ return ia.toString();
+ }
+ }
+ /**
+ * Get the name of the server principal for a SASL client. This is visible for testing purposes.
+ * @param addr the address of the host.
+ * @param clientConfig the configuration for the client.
+ * @return the name of the principal.
+ */
+ static String getServerPrincipal(MockableInetSocketAddress addr, ZKClientConfig clientConfig) {
+ String principalUserName = clientConfig.getProperty(ZKClientConfig.ZK_SASL_CLIENT_USERNAME,
+ ZKClientConfig.ZK_SASL_CLIENT_USERNAME_DEFAULT);
+ String hostName = addr.getHostName();
+
+ boolean canonicalize = true;
+ try {
+ canonicalize = Boolean.parseBoolean(clientConfig.getProperty(ZKClientConfig.ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME,
+ ZKClientConfig.ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME_DEFAULT));
+ } catch (IllegalArgumentException ea) {
--- End diff --
I don't see any of these methods (parseBoolean() and getProperty()) throw IllegalArgumentException, so you don't need to catch here.
In your original PR, you used System.getProperty() which throws it:
https://github.com/apache/zookeeper/pull/648/files#diff-ae38c175a23a47be52ae31ed1f1518a3R781
But this is master and we use ZKConfig here.
---
[GitHub] zookeeper pull request #652: 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/652#discussion_r221956355
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -793,7 +794,87 @@ public RWServerFoundException(String msg) {
super(msg);
}
}
-
+
+ static class MockableInetSocketAddress {
+ private final InetSocketAddress addr;
+
+ MockableInetSocketAddress(InetSocketAddress addr) {
+ this.addr = addr;
+ }
+
+ public String getHostName() {
+ return addr.getHostName();
+ }
+
+ public MockableInetAddress getAddress() {
+ InetAddress ia = addr.getAddress();
+ return ia == null ? null : new MockableInetAddress(ia);
+ }
+
+ @Override
+ public String toString() {
+ return addr.toString();
+ }
+ }
+
+ static class MockableInetAddress {
+ private final InetAddress ia;
+
+ MockableInetAddress(InetAddress ia) {
+ this.ia = ia;
+ }
+
+ public String getCanonicalHostName() {
+ return ia.getCanonicalHostName();
+ }
+
+ public String getHostAddress() {
+ return ia.getHostAddress();
+ }
+
+ @Override
+ public String toString() {
+ return ia.toString();
+ }
+ }
+ /**
+ * Get the name of the server principal for a SASL client. This is visible for testing purposes.
+ * @param addr the address of the host.
+ * @param clientConfig the configuration for the client.
+ * @return the name of the principal.
+ */
+ static String getServerPrincipal(MockableInetSocketAddress addr, ZKClientConfig clientConfig) {
+ String principalUserName = clientConfig.getProperty(ZKClientConfig.ZK_SASL_CLIENT_USERNAME,
+ ZKClientConfig.ZK_SASL_CLIENT_USERNAME_DEFAULT);
+ String hostName = addr.getHostName();
+
+ boolean canonicalize = true;
+ try {
+ canonicalize = Boolean.parseBoolean(clientConfig.getProperty(ZKClientConfig.ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME,
+ ZKClientConfig.ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME_DEFAULT));
+ } catch (IllegalArgumentException ea) {
--- End diff --
The IllegalArgumentException is coming from the `Boolean.parseBoolean`. If it cannot understand the value as either true or false it will throw the exception and we end up with it being true, the default. If you want a log I am happy to add it, because "no" will be interpreted as true, which might be confusing.
---
[GitHub] zookeeper issue #652: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/652
+1
The new change looks good to me, please rebase to resolve the conflict, will merge this in after that. Sorry for lately reply, somehow lost tracking this session.
---
[GitHub] zookeeper issue #652: 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/652
@revans2 We might need another committer to take a look.
Also you probably need to trigger another build to get a green one.
---
[GitHub] zookeeper pull request #652: 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/652#discussion_r221938215
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -793,7 +794,87 @@ public RWServerFoundException(String msg) {
super(msg);
}
}
-
+
+ static class MockableInetSocketAddress {
+ private final InetSocketAddress addr;
+
+ MockableInetSocketAddress(InetSocketAddress addr) {
+ this.addr = addr;
+ }
+
+ public String getHostName() {
+ return addr.getHostName();
+ }
+
+ public MockableInetAddress getAddress() {
+ InetAddress ia = addr.getAddress();
+ return ia == null ? null : new MockableInetAddress(ia);
+ }
+
+ @Override
+ public String toString() {
+ return addr.toString();
+ }
+ }
+
+ static class MockableInetAddress {
+ private final InetAddress ia;
+
+ MockableInetAddress(InetAddress ia) {
+ this.ia = ia;
+ }
+
+ public String getCanonicalHostName() {
+ return ia.getCanonicalHostName();
+ }
+
+ public String getHostAddress() {
+ return ia.getHostAddress();
+ }
+
+ @Override
+ public String toString() {
+ return ia.toString();
+ }
+ }
+ /**
+ * Get the name of the server principal for a SASL client. This is visible for testing purposes.
+ * @param addr the address of the host.
+ * @param clientConfig the configuration for the client.
+ * @return the name of the principal.
+ */
+ static String getServerPrincipal(MockableInetSocketAddress addr, ZKClientConfig clientConfig) {
+ String principalUserName = clientConfig.getProperty(ZKClientConfig.ZK_SASL_CLIENT_USERNAME,
+ ZKClientConfig.ZK_SASL_CLIENT_USERNAME_DEFAULT);
+ String hostName = addr.getHostName();
+
+ boolean canonicalize = true;
+ try {
+ canonicalize = Boolean.parseBoolean(clientConfig.getProperty(ZKClientConfig.ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME,
+ ZKClientConfig.ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME_DEFAULT));
+ } catch (IllegalArgumentException ea) {
+ //ignored ...
+ }
+
+ if (canonicalize) {
+ MockableInetAddress ia = addr.getAddress();
+ if (ia == null) {
+ throw new IllegalArgumentException("Unable to canonicalize address " + addr + " because it's not resolvable");
+ }
+
+ String canonicalHostName = ia.getCanonicalHostName();
+ //avoid using literal IP address when security check fails
+ if (!canonicalHostName.equals(ia.getHostAddress())) {
--- End diff --
Would you please add test to cover this code path: canonicalization is ON, but getCanonicalHostName() returns IP address.
---
[GitHub] zookeeper issue #652: 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/652
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2303/
---
[GitHub] zookeeper issue #652: 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/652
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2550/
---
[GitHub] zookeeper issue #652: 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/652
retest this please
---
[GitHub] zookeeper issue #652: 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/652
Added in the test.
---
[GitHub] zookeeper pull request #652: 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/652#discussion_r221964223
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -793,7 +794,87 @@ public RWServerFoundException(String msg) {
super(msg);
}
}
-
+
+ static class MockableInetSocketAddress {
+ private final InetSocketAddress addr;
+
+ MockableInetSocketAddress(InetSocketAddress addr) {
+ this.addr = addr;
+ }
+
+ public String getHostName() {
+ return addr.getHostName();
+ }
+
+ public MockableInetAddress getAddress() {
+ InetAddress ia = addr.getAddress();
+ return ia == null ? null : new MockableInetAddress(ia);
+ }
+
+ @Override
+ public String toString() {
+ return addr.toString();
+ }
+ }
+
+ static class MockableInetAddress {
+ private final InetAddress ia;
+
+ MockableInetAddress(InetAddress ia) {
+ this.ia = ia;
+ }
+
+ public String getCanonicalHostName() {
+ return ia.getCanonicalHostName();
+ }
+
+ public String getHostAddress() {
+ return ia.getHostAddress();
+ }
+
+ @Override
+ public String toString() {
+ return ia.toString();
+ }
+ }
+ /**
+ * Get the name of the server principal for a SASL client. This is visible for testing purposes.
+ * @param addr the address of the host.
+ * @param clientConfig the configuration for the client.
+ * @return the name of the principal.
+ */
+ static String getServerPrincipal(MockableInetSocketAddress addr, ZKClientConfig clientConfig) {
+ String principalUserName = clientConfig.getProperty(ZKClientConfig.ZK_SASL_CLIENT_USERNAME,
+ ZKClientConfig.ZK_SASL_CLIENT_USERNAME_DEFAULT);
+ String hostName = addr.getHostName();
+
+ boolean canonicalize = true;
+ try {
+ canonicalize = Boolean.parseBoolean(clientConfig.getProperty(ZKClientConfig.ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME,
+ ZKClientConfig.ZK_SASL_CLIENT_CANONICALIZE_HOSTNAME_DEFAULT));
+ } catch (IllegalArgumentException ea) {
--- End diff --
`Boolean.parseBoolean()` returns true if the string is "true" (ignore case) and false otherwise. No exception thrown.
---