dsmiley commented on code in PR #4320:
URL: https://github.com/apache/solr/pull/4320#discussion_r3256262308
##########
changelog/unreleased/SOLR-18130-cli-tools-solr-connection-support.yml:
##########
@@ -0,0 +1,7 @@
+title: Add --solr-connection option to Solr CLI tools supporting ZooKeeper and
HTTP(S) connections
Review Comment:
Can you clarify if, not only does this *add* an option, but did you change
affected the CLI options to no longer communicate directly with ZK if zkHost is
given?
##########
changelog/unreleased/SOLR-18130-cli-tools-solr-connection-support.yml:
##########
@@ -0,0 +1,7 @@
+title: Add --solr-connection option to Solr CLI tools supporting ZooKeeper and
HTTP(S) connections
+type: changed
Review Comment:
The "type" should mostly be `added` for these because the PR requires a user
to do something different (supply a different param) to leverage what was done.
##########
changelog/unreleased/SOLR-18130-join-query-parser-solr-connection-support.yml:
##########
@@ -0,0 +1,7 @@
+title: Add 'solrConnection' parameter to Join Query Parser to support
ZooKeeper and HTTP(S) connection types
Review Comment:
Suggest clarifying this is for cross-collection joins
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/SolrClientCache.java:
##########
@@ -67,15 +65,19 @@ public void setBasicAuthCredentials(String
basicAuthCredentials) {
this.basicAuthCredentials = basicAuthCredentials;
}
- public void setDefaultZKHost(String zkHost) {
- if (zkHost != null) {
- zkHost = zkHost.split("/")[0];
- if (!zkHost.isEmpty()) {
- defaultZkHost.set(zkHost);
- } else {
- defaultZkHost.set(null);
- }
- }
+ /**
+ * Controls whether ZooKeeper ACL credentials may be propagated to ZooKeeper
hosts used by {@link
+ * CloudSolrClient} instances created by this cache.
+ *
+ * <p>This option is disabled by default for security reasons. Enabling it
may expose ZooKeeper
+ * credentials to external or untrusted ZooKeeper ensembles if arbitrary
cluster connections are
+ * allowed.
+ *
+ * @param useZookeeperACL whether ZooKeeper ACL credentials should be used
by clients created from
+ * this cache
+ */
+ public void setUseZookeeperACL(boolean useZookeeperACL) {
+ this.useZookeeperACL = useZookeeperACL;
}
Review Comment:
I suggest removing this method you added. It's not needed internally, for
one, but moreover it's a very obscure setting! I'd prefer clients move to HTTP
but if a client really wanted to continue to use ZK, they could override
newCloudSolrClient to customize the CloudSolrClient creation. There's a reason
I made that method protected.
##########
solr/solrj-streaming/src/test/org/apache/solr/client/solrj/io/SolrClientCacheTest.java:
##########
@@ -102,12 +90,13 @@ public void testGetClientWithZookeeper() {
}
@Test
- public void testZkACLsUsedWithDifferentChroot() {
+ public void testGetClientWithHttp() {
+ String solrUrl = cluster.getJettySolrRunner(0).getBaseUrl().toString();
try (SolrClientCache cache = new SolrClientCache()) {
- // The same ZK Host is used, so the ZK ACLs should still be applied
- cache.setDefaultZKHost(zkClient().getZkServerAddress() +
"/random/chroot");
- cache.getCloudSolrClient(
-
CloudSolrClient.CloudSolrClientConnection.parse(zkClient().getZkServerAddress()));
Review Comment:
This is a distinction we're loosing but I don't think it matters.
CC @HoustonPutman
##########
changelog/unreleased/SOLR-18130-solr-sql-jdbc-driver-solr-connection-support.yml:
##########
@@ -0,0 +1,7 @@
+title: Add 'StreamFactory' methods for configuring default and
collection-specific Solr connections; deprecate ZooKeeper-based methods.
Review Comment:
Tthis one confuses me. Given the other changelog entries cover other
things, this one can only mean you are trying to discuss pretty much what the
title of this PR says. I suggest:
> Added solrConnection param to solrj-streaming module, expanding use to
HTTP based SolrCloud coordinates. zkHost is deprecated.
##########
solr/core/src/test/org/apache/solr/core/InternalSolrClientCacheTest.java:
##########
Review Comment:
I appreciate your test here, but I think a security feature is better off
with an integration test (we use BATS in solr/packaging/test) to give higher
confidence that, indeed, there is a protection in place, without having to
assume that InternalSolrClientCache is employed where it should be.
##########
solr/core/src/java/org/apache/solr/core/InternalSolrClientCache.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.solr.core;
+
+import org.apache.solr.client.solrj.impl.CloudSolrClient;
+import org.apache.solr.client.solrj.impl.HttpSolrClientBase;
+import org.apache.solr.client.solrj.io.SolrClientCache;
+import org.apache.solr.common.SolrException;
+
+/** A restricted {@link SolrClientCache} that only permits access local solr
cluster */
+public class InternalSolrClientCache extends SolrClientCache {
+
+ private static final String ALLOW_EXTERNAL_CLUSTERS_PROPERTY =
"solr.allow-external-clusters";
Review Comment:
1. we don't use hyphens
2. second position (dot-separated) is a namespace / scope
3. booleans generally end with 'enabled'
I suggest `solr.cloud.external.enabled`.
I don't love "external" I came up with but it's the best my tired self could
think up right now :-)
##########
solr/core/src/java/org/apache/solr/core/InternalSolrClientCache.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.solr.core;
+
+import org.apache.solr.client.solrj.impl.CloudSolrClient;
+import org.apache.solr.client.solrj.impl.HttpSolrClientBase;
+import org.apache.solr.client.solrj.io.SolrClientCache;
+import org.apache.solr.common.SolrException;
+
+/** A restricted {@link SolrClientCache} that only permits access local solr
cluster */
+public class InternalSolrClientCache extends SolrClientCache {
+
+ private static final String ALLOW_EXTERNAL_CLUSTERS_PROPERTY =
"solr.allow-external-clusters";
+
+ public InternalSolrClientCache(
+ HttpSolrClientBase httpSolrClient,
CloudSolrClient.CloudSolrClientConnection solrConnection) {
+ super(httpSolrClient);
+ cloudSolClients.put(solrConnection, newCloudSolrClient(solrConnection,
httpSolrClient, true));
+ }
+
+ public InternalSolrClientCache(HttpSolrClientBase httpSolrClient) {
+ super(httpSolrClient);
+ }
+
+ @Override
+ public void setUseZookeeperACL(boolean useZookeeperACL) {
+ throw new UnsupportedOperationException(
+ "Changing ZooKeeper ACL usage is not allowed for
InternalSolrClientCache");
+ }
+
+ @Override
+ public synchronized CloudSolrClient getCloudSolrClient(
+ CloudSolrClient.CloudSolrClientConnection solrConnection) {
+ CloudSolrClient client = cloudSolClients.get(solrConnection);
+ if (client != null) {
+ return client;
+ }
+ if (Boolean.getBoolean(ALLOW_EXTERNAL_CLUSTERS_PROPERTY)) {
Review Comment:
we use EnvUtils
##########
solr/core/src/test/org/apache/solr/core/InternalSolrClientCacheTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.solr.core;
+
+import java.util.Map;
+import org.apache.solr.client.solrj.impl.CloudSolrClient;
+import org.apache.solr.client.solrj.io.SolrClientCache;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.DigestZkACLProvider;
+import org.apache.solr.common.cloud.DigestZkCredentialsProvider;
+import org.apache.solr.common.cloud.SolrZkClient;
+import org.apache.solr.common.cloud.VMParamsZkCredentialsInjector;
+import org.apache.solr.metrics.SolrMetricsContext;
+import org.apache.solr.security.MockSolrMetricsContextFactory;
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+public class InternalSolrClientCacheTest extends SolrCloudTestCase {
+ private static final Map<String, String> sysProps =
+ Map.of(
+ SolrZkClient.ZK_CREDENTIALS_INJECTOR_CLASS_NAME_VM_PARAM_NAME,
+ VMParamsZkCredentialsInjector.class.getName(),
+ SolrZkClient.ZK_CRED_PROVIDER_CLASS_NAME_VM_PARAM_NAME,
+ DigestZkCredentialsProvider.class.getName(),
+ SolrZkClient.ZK_ACL_PROVIDER_CLASS_NAME_VM_PARAM_NAME,
+ DigestZkACLProvider.class.getName(),
+ VMParamsZkCredentialsInjector.DEFAULT_DIGEST_USERNAME_VM_PARAM_NAME,
+ "admin-user",
+ VMParamsZkCredentialsInjector.DEFAULT_DIGEST_PASSWORD_VM_PARAM_NAME,
+ "pass",
+
VMParamsZkCredentialsInjector.DEFAULT_DIGEST_READONLY_USERNAME_VM_PARAM_NAME,
+ "read-user",
+
VMParamsZkCredentialsInjector.DEFAULT_DIGEST_READONLY_PASSWORD_VM_PARAM_NAME,
+ "pass");
+
+ @BeforeClass
+ public static void before() throws Exception {
+ sysProps.forEach(System::setProperty);
+ configureCluster(1)
+ .formatZkServer(true)
+ .addConfig(
+ "conf",
TEST_PATH().resolve("configsets").resolve("cloud-minimal").resolve("conf"))
+ .configure();
+ }
+
+ @AfterClass
+ public static void after() {
+ sysProps.keySet().forEach(System::clearProperty);
+ }
+
+ @After
+ public void afterEach() {
+ System.clearProperty("solr.allow-external-clusters");
+ }
+
+ @Test
+ public void testSelfClusterShouldBeAllowed() {
Review Comment:
no need to test this as other tests relying on this would break if we
inadvertently screwed this up. Like CrossCollectionJoinQueryTest.java
##########
solr/core/src/test/org/apache/solr/core/InternalSolrClientCacheTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.solr.core;
+
+import java.util.Map;
+import org.apache.solr.client.solrj.impl.CloudSolrClient;
+import org.apache.solr.client.solrj.io.SolrClientCache;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.DigestZkACLProvider;
+import org.apache.solr.common.cloud.DigestZkCredentialsProvider;
+import org.apache.solr.common.cloud.SolrZkClient;
+import org.apache.solr.common.cloud.VMParamsZkCredentialsInjector;
+import org.apache.solr.metrics.SolrMetricsContext;
+import org.apache.solr.security.MockSolrMetricsContextFactory;
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+public class InternalSolrClientCacheTest extends SolrCloudTestCase {
+ private static final Map<String, String> sysProps =
+ Map.of(
+ SolrZkClient.ZK_CREDENTIALS_INJECTOR_CLASS_NAME_VM_PARAM_NAME,
+ VMParamsZkCredentialsInjector.class.getName(),
+ SolrZkClient.ZK_CRED_PROVIDER_CLASS_NAME_VM_PARAM_NAME,
+ DigestZkCredentialsProvider.class.getName(),
+ SolrZkClient.ZK_ACL_PROVIDER_CLASS_NAME_VM_PARAM_NAME,
+ DigestZkACLProvider.class.getName(),
+ VMParamsZkCredentialsInjector.DEFAULT_DIGEST_USERNAME_VM_PARAM_NAME,
+ "admin-user",
+ VMParamsZkCredentialsInjector.DEFAULT_DIGEST_PASSWORD_VM_PARAM_NAME,
+ "pass",
+
VMParamsZkCredentialsInjector.DEFAULT_DIGEST_READONLY_USERNAME_VM_PARAM_NAME,
+ "read-user",
+
VMParamsZkCredentialsInjector.DEFAULT_DIGEST_READONLY_PASSWORD_VM_PARAM_NAME,
+ "pass");
+
+ @BeforeClass
+ public static void before() throws Exception {
+ sysProps.forEach(System::setProperty);
+ configureCluster(1)
+ .formatZkServer(true)
+ .addConfig(
+ "conf",
TEST_PATH().resolve("configsets").resolve("cloud-minimal").resolve("conf"))
+ .configure();
+ }
+
+ @AfterClass
+ public static void after() {
+ sysProps.keySet().forEach(System::clearProperty);
+ }
+
+ @After
+ public void afterEach() {
+ System.clearProperty("solr.allow-external-clusters");
+ }
+
+ @Test
+ public void testSelfClusterShouldBeAllowed() {
+ final SolrMetricsContext metricsContext =
MockSolrMetricsContextFactory.create();
+ try (HttpSolrClientProvider httpSolrClientProvider =
+ new HttpSolrClientProvider(null, metricsContext)) {
Review Comment:
I have never seen this pattern before. Getting an HttpSolrClient is a
simple matter of calling cluster.getRandomJetty().getSolrClient()
##########
solr/core/src/test/org/apache/solr/core/InternalSolrClientCacheTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.solr.core;
+
+import java.util.Map;
+import org.apache.solr.client.solrj.impl.CloudSolrClient;
+import org.apache.solr.client.solrj.io.SolrClientCache;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.DigestZkACLProvider;
+import org.apache.solr.common.cloud.DigestZkCredentialsProvider;
+import org.apache.solr.common.cloud.SolrZkClient;
+import org.apache.solr.common.cloud.VMParamsZkCredentialsInjector;
+import org.apache.solr.metrics.SolrMetricsContext;
+import org.apache.solr.security.MockSolrMetricsContextFactory;
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+public class InternalSolrClientCacheTest extends SolrCloudTestCase {
+ private static final Map<String, String> sysProps =
+ Map.of(
+ SolrZkClient.ZK_CREDENTIALS_INJECTOR_CLASS_NAME_VM_PARAM_NAME,
+ VMParamsZkCredentialsInjector.class.getName(),
+ SolrZkClient.ZK_CRED_PROVIDER_CLASS_NAME_VM_PARAM_NAME,
+ DigestZkCredentialsProvider.class.getName(),
+ SolrZkClient.ZK_ACL_PROVIDER_CLASS_NAME_VM_PARAM_NAME,
+ DigestZkACLProvider.class.getName(),
+ VMParamsZkCredentialsInjector.DEFAULT_DIGEST_USERNAME_VM_PARAM_NAME,
+ "admin-user",
+ VMParamsZkCredentialsInjector.DEFAULT_DIGEST_PASSWORD_VM_PARAM_NAME,
+ "pass",
+
VMParamsZkCredentialsInjector.DEFAULT_DIGEST_READONLY_USERNAME_VM_PARAM_NAME,
+ "read-user",
+
VMParamsZkCredentialsInjector.DEFAULT_DIGEST_READONLY_PASSWORD_VM_PARAM_NAME,
+ "pass");
+
+ @BeforeClass
+ public static void before() throws Exception {
+ sysProps.forEach(System::setProperty);
+ configureCluster(1)
+ .formatZkServer(true)
+ .addConfig(
+ "conf",
TEST_PATH().resolve("configsets").resolve("cloud-minimal").resolve("conf"))
+ .configure();
+ }
+
+ @AfterClass
+ public static void after() {
+ sysProps.keySet().forEach(System::clearProperty);
+ }
+
+ @After
+ public void afterEach() {
+ System.clearProperty("solr.allow-external-clusters");
+ }
Review Comment:
our test infra handles resetting sys props automatically
##########
solr/solrj-streaming/src/test/org/apache/solr/client/solrj/io/SolrClientCacheTest.java:
##########
@@ -64,10 +64,9 @@ public static void after() {
}
@Test
- public void testZkACLsNotUsedWithDifferentZkHost() {
+ public void testZkACLsShouldNotBeUsedByDefault() {
Review Comment:
I'm not sure how the expectThrows demonstrates with the testMethod says
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/sql/DriverImpl.java:
##########
@@ -89,4 +95,89 @@ public Logger getParentLogger() {
public DriverPropertyInfo[] getPropertyInfo(String url, Properties info) {
return null;
}
+
+ static class SolrJdbcUrlParser {
Review Comment:
It seems you are merely using SolrJdbcUrlParser as namespace / section, as
this won't be instantiated. That's highly unusual for an inner class; I don't
think I've ever seen that pattern. You could merely remove the namespace and
use a section-like comment and make the method(s) and maybe make some methods
clearer, especially the entrypoint.
Or move all these as static methods on JdbcConnectionMetadata.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
