You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/05/11 19:40:59 UTC
[GitHub] [solr] madrob commented on a diff in pull request #857: SOLR-16192: Add ZK credentials injectors support
madrob commented on code in PR #857:
URL: https://github.com/apache/solr/pull/857#discussion_r870622412
##########
solr/core/src/test/org/apache/solr/cloud/OverriddenZkACLAndCredentialsProvidersTest.java:
##########
@@ -16,24 +16,21 @@
*/
package org.apache.solr.cloud;
+import static org.apache.solr.common.cloud.acl.VMParamsZkCredentialsInjector.DEFAULT_DIGEST_PASSWORD_VM_PARAM_NAME;
+import static org.apache.solr.common.cloud.acl.VMParamsZkCredentialsInjector.DEFAULT_DIGEST_READONLY_PASSWORD_VM_PARAM_NAME;
+import static org.apache.solr.common.cloud.acl.VMParamsZkCredentialsInjector.DEFAULT_DIGEST_READONLY_USERNAME_VM_PARAM_NAME;
+import static org.apache.solr.common.cloud.acl.VMParamsZkCredentialsInjector.DEFAULT_DIGEST_USERNAME_VM_PARAM_NAME;
+import static org.apache.solr.common.cloud.acl.ZkCredentialsInjector.ZkCredential.Perms;
+
import java.lang.invoke.MethodHandles;
import java.nio.charset.Charset;
-import java.nio.charset.StandardCharsets;
import java.nio.file.Path;
import java.util.ArrayList;
-import java.util.Collection;
import java.util.List;
import org.apache.solr.SolrTestCaseJ4;
-import org.apache.solr.common.StringUtils;
-import org.apache.solr.common.cloud.DefaultZkCredentialsProvider;
-import org.apache.solr.common.cloud.SecurityAwareZkACLProvider;
import org.apache.solr.common.cloud.SolrZkClient;
-import org.apache.solr.common.cloud.VMParamsAllAndReadonlyDigestZkACLProvider;
-import org.apache.solr.common.cloud.VMParamsSingleSetCredentialsDigestZkCredentialsProvider;
-import org.apache.solr.common.cloud.ZkACLProvider;
-import org.apache.solr.common.cloud.ZkCredentialsProvider;
+import org.apache.solr.common.cloud.acl.*;
Review Comment:
nit: We typically avoid wildcard imports.
##########
solr/solrj/src/java/org/apache/solr/common/cloud/acl/ZkCredentialsProvider.java:
##########
@@ -14,13 +14,13 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.solr.common.cloud;
+package org.apache.solr.common.cloud.acl;
Review Comment:
I think I missed the part of the discussion where we described why it is necessary to repackage? I suspect this is one of the major sources of incompatibility that would prevent a back port to the 9x release branch.
##########
solr/solrj/src/java/org/apache/solr/common/cloud/acl/ZkCredentialsInjector.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.common.cloud.acl;
+
+import java.util.List;
+
+public interface ZkCredentialsInjector {
+
+ List<ZkCredential> getZkCredentials();
+
+ class ZkCredential {
+
+ public enum Perms {
Review Comment:
Why do we need to use these instead of using ZooDefs.Perms directly?
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -323,22 +329,29 @@ public ZkController(
ZkClientConnectionStrategy strat =
ZkClientConnectionStrategy.forName(connectionStrategy, new DefaultConnectionStrategy());
+ String zkCredentialsInjectorClass = cloudConfig.getZkCredentialsInjectorClass();
+ ZkCredentialsInjector zkCredentialsInjector =
+ !StringUtils.isEmpty(zkCredentialsInjectorClass)
+ ? cc.getResourceLoader()
+ .newInstance(zkCredentialsInjectorClass, ZkCredentialsInjector.class)
+ : new DefaultZkCredentialsInjector();
+
String zkACLProviderClass = cloudConfig.getZkACLProviderClass();
- ZkACLProvider zkACLProvider = null;
- if (zkACLProviderClass != null && zkACLProviderClass.trim().length() > 0) {
- zkACLProvider = cc.getResourceLoader().newInstance(zkACLProviderClass, ZkACLProvider.class);
- } else {
- zkACLProvider = new DefaultZkACLProvider();
- }
+ ZkACLProvider zkACLProvider =
+ !StringUtils.isEmpty(zkACLProviderClass)
+ ? cc.getResourceLoader().newInstance(zkACLProviderClass, ZkACLProvider.class)
+ : new DefaultZkACLProvider();
Review Comment:
Does this change from if/else to ternary change the logic? If not, I don't think it's necessary.
##########
solr/solrj/src/java/org/apache/solr/common/cloud/acl/ZkCredentialsInjector.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.common.cloud.acl;
+
+import java.util.List;
+
+public interface ZkCredentialsInjector {
+
+ List<ZkCredential> getZkCredentials();
+
+ class ZkCredential {
+
+ public enum Perms {
+ ALL,
+ READ
+ }
+
+ private String username;
+ private String password;
+ private String perms;
+
+ public ZkCredential() {}
+
+ public ZkCredential(String username, String password, Perms perms) {
+ this(username, password, String.valueOf(perms));
+ }
+
+ public ZkCredential(String username, String password, String perms) {
+ this.username = username;
+ this.password = password;
+ this.perms = perms;
+ }
+
+ public String getUsername() {
+ return username;
+ }
+
+ public String getPassword() {
+ return password;
+ }
+
+ public void setPassword(String password) {
+ this.password = password;
+ }
+
+ public String getPerms() {
Review Comment:
I don't see where this is used.
##########
solr/solrj/src/java/org/apache/solr/common/cloud/acl/ZkACLProvider.java:
##########
@@ -14,12 +14,14 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.solr.common.cloud;
+package org.apache.solr.common.cloud.acl;
import java.util.List;
import org.apache.zookeeper.data.ACL;
public interface ZkACLProvider {
List<ACL> getACLsToAdd(String zNodePath);
+
+ void setZkCredentialsInjector(ZkCredentialsInjector zkCredentialsInjector);
Review Comment:
If this had a default no-op implementation, I think that would clarify some of the other use cases like DefaultZkACLProvider not using it.
##########
solr/solrj/src/java/org/apache/solr/common/cloud/SolrZkClient.java:
##########
@@ -256,18 +265,43 @@ protected ZkACLProvider createZkACLProvider() {
if (!StringUtils.isEmpty(zkACLProviderClassName)) {
try {
log.info("Using ZkACLProvider: {}", zkACLProviderClassName);
- return (ZkACLProvider) Class.forName(zkACLProviderClassName).getConstructor().newInstance();
+ ZkACLProvider zkACLProvider =
+ (ZkACLProvider) Class.forName(zkACLProviderClassName).getConstructor().newInstance();
+ zkACLProvider.setZkCredentialsInjector(zkCredentialsInjector);
+ return zkACLProvider;
} catch (Throwable t) {
// just ignore - go default
log.warn(
"VM param zkACLProvider does not point to a class implementing ZkACLProvider and with a non-arg constructor",
t);
}
}
- log.debug("Using default ZkACLProvider");
+ log.warn("Using default ZkACLProvider");
return new DefaultZkACLProvider();
}
+ public static final String ZK_CREDENTIALS_INJECTOR_CLASS_NAME_VM_PARAM_NAME =
Review Comment:
This is a weird place to declare such a constant.
##########
solr/solrj/src/java/org/apache/solr/common/cloud/SolrZkClient.java:
##########
@@ -236,16 +242,19 @@ protected ZkCredentialsProvider createZkCredentialsToAddAutomatically() {
if (!StringUtils.isEmpty(zkCredentialsProviderClassName)) {
try {
log.info("Using ZkCredentialsProvider: {}", zkCredentialsProviderClassName);
- return (ZkCredentialsProvider)
- Class.forName(zkCredentialsProviderClassName).getConstructor().newInstance();
+ ZkCredentialsProvider zkCredentialsProvider =
+ (ZkCredentialsProvider)
+ Class.forName(zkCredentialsProviderClassName).getConstructor().newInstance();
+ zkCredentialsProvider.setZkCredentialsInjector(zkCredentialsInjector);
+ return zkCredentialsProvider;
} catch (Throwable t) {
// just ignore - go default
log.warn(
"VM param zkCredentialsProvider does not point to a class implementing ZkCredentialsProvider and with a non-arg constructor",
t);
}
}
- log.debug("Using default ZkCredentialsProvider");
+ log.warn("Using default ZkCredentialsProvider");
Review Comment:
Why is using the default provider bad? Is that one insecure?
Making this a _warn_ level message should be accompanied by either a link to the docs or suggestions on what to do. Otherwise operators are going to see the message and promptly ignore it.
##########
solr/solrj/src/java/org/apache/solr/common/cloud/DefaultZkCredentialsProvider.java:
##########
@@ -18,10 +18,27 @@
import java.util.ArrayList;
import java.util.Collection;
+import org.apache.solr.common.cloud.acl.DefaultZkCredentialsInjector;
+import org.apache.solr.common.cloud.acl.ZkCredentialsInjector;
+import org.apache.solr.common.cloud.acl.ZkCredentialsProvider;
public class DefaultZkCredentialsProvider implements ZkCredentialsProvider {
private Collection<ZkCredentials> zkCredentials;
+ protected ZkCredentialsInjector zkCredentialsInjector;
Review Comment:
I think this is unused in this class? Can we push it down to where it would actually be needed?
##########
solr/solrj/src/java/org/apache/solr/common/cloud/SolrZkClient.java:
##########
@@ -256,18 +265,43 @@ protected ZkACLProvider createZkACLProvider() {
if (!StringUtils.isEmpty(zkACLProviderClassName)) {
try {
log.info("Using ZkACLProvider: {}", zkACLProviderClassName);
- return (ZkACLProvider) Class.forName(zkACLProviderClassName).getConstructor().newInstance();
+ ZkACLProvider zkACLProvider =
+ (ZkACLProvider) Class.forName(zkACLProviderClassName).getConstructor().newInstance();
+ zkACLProvider.setZkCredentialsInjector(zkCredentialsInjector);
+ return zkACLProvider;
} catch (Throwable t) {
// just ignore - go default
log.warn(
"VM param zkACLProvider does not point to a class implementing ZkACLProvider and with a non-arg constructor",
t);
}
}
- log.debug("Using default ZkACLProvider");
+ log.warn("Using default ZkACLProvider");
return new DefaultZkACLProvider();
}
+ public static final String ZK_CREDENTIALS_INJECTOR_CLASS_NAME_VM_PARAM_NAME =
+ "zkCredentialsInjector";
+
+ protected ZkCredentialsInjector createZkCredentialsInjector() {
Review Comment:
I wonder if it would make sense to encapsulate this pattern in a separate method and reuse for ZkCredentialsInjector, ZkACLProvider, and ZkCredentialsProvider
##########
solr/solrj/src/java/org/apache/solr/common/cloud/acl/ZkCredentialsInjector.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.common.cloud.acl;
+
+import java.util.List;
+
+public interface ZkCredentialsInjector {
Review Comment:
Please add javadoc to this class
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org