apucher commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r836093058
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/broker/AllowAllAccessControlFactory.java
##########
@@ -34,6 +36,10 @@ public AllowAllAccessControlFactory() {
public void init(PinotConfiguration configuration) {
}
+ public void init(ZkHelixPropertyStore<ZNRecord> propertyStore) {
+ }
+
+ @Override
Review comment:
see above. if there's any change in this file, it's not
backwards-compatible
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/broker/AccessControlFactory.java
##########
@@ -29,6 +31,7 @@
public static final String ACCESS_CONTROL_CLASS_CONFIG = "class";
public abstract void init(PinotConfiguration confguration);
+ public abstract void init(ZkHelixPropertyStore<ZNRecord> propertyStore);
Review comment:
See suggestion below. I'd make this a non-abstract method that takes
both `PinotConfiguration` and `ZkHelixPropertyStore` and calls the legacy
`init(PinotConfiguration)`. Then, add override this method in your new code
while old code remains unaffected. You'll also want to document this intended
use in the javadoc
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/broker/AccessControlFactory.java
##########
@@ -29,10 +31,13 @@
public static final String ACCESS_CONTROL_CLASS_CONFIG = "class";
public abstract void init(PinotConfiguration confguration);
+ public abstract void init(ZkHelixPropertyStore<ZNRecord> propertyStore);
+
public abstract AccessControl create();
- public static AccessControlFactory loadFactory(PinotConfiguration
configuration) {
+ public static AccessControlFactory loadFactory(PinotConfiguration
configuration,
+ ZkHelixPropertyStore<ZNRecord> propertyStore) {
Review comment:
Great. IMO you can change the signature of the static `loadFactory` to
include both configuration and property store. Since it's static it's not
really part of the interface and there's no need to duplicate the code.
Rather that using `instanceof` I'd suggest to introduce an
`init(PinotConfiguration, ZkHelixPropertyStore)` that defaults to invoking the
legacy `init(PinotConfiguration)` method. That way, you achieve backwards
compatibility and your new code can override the expanded method and take
advantage of ZK access
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/utils/BcryptUtils.java
##########
@@ -0,0 +1,50 @@
+/**
+ * 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.pinot.common.utils;
+
+import org.mindrot.jbcrypt.BCrypt;
+
+public class BcryptUtils {
Review comment:
awesome.
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/config/provider/UserCache.java
##########
@@ -0,0 +1,176 @@
+/**
+ * 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.pinot.common.config.provider;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.stream.Collectors;
+import javax.annotation.Nullable;
+import org.I0Itec.zkclient.IZkChildListener;
+import org.I0Itec.zkclient.IZkDataListener;
+import org.apache.commons.collections.CollectionUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.pinot.common.utils.config.UserConfigUtils;
+import org.apache.pinot.spi.config.user.ComponentType;
+import org.apache.pinot.spi.config.user.UserConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class UserCache {
+
+ private static final Logger LOGGER =
LoggerFactory.getLogger(UserCache.class);
+ private static final String USER_CONFIG_PARENT_PATH = "/CONFIGS/USER";
+ private static final String USER_CONFIG_PATH_PREFIX = "/CONFIGS/USER/";
+
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
+
+ private final UserConfigChangeListener _userConfigChangeListener = new
UserConfigChangeListener();
+
+ private final Map<String, UserConfig> _userConfigMap = new
ConcurrentHashMap<>();
+ private final Map<String, UserConfig> _userControllerConfigMap = new
ConcurrentHashMap<>();
+ private final Map<String, UserConfig> _userBrokerConfigMap = new
ConcurrentHashMap<>();
+ private final Map<String, UserConfig> _userServerConfigMap = new
ConcurrentHashMap<>();
Review comment:
it's fine. we'll extend this whenever we need any properties for minion
or proxy
##########
File path: pinot-common/pom.xml
##########
@@ -301,6 +301,11 @@
<groupId>org.apache.yetus</groupId>
<artifactId>audience-annotations</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.mindrot</groupId>
+ <artifactId>jbcrypt</artifactId>
+ <version>0.4</version>
+ </dependency>
Review comment:
@Jackie-Jiang do we have bcrypt available somewhere in the existing
dependencies?
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,130 @@
+/**
+ * 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.pinot.broker.broker;
+
+import com.google.common.base.Preconditions;
+import java.util.Collection;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.pinot.broker.api.AccessControl;
+import org.apache.pinot.broker.api.HttpRequesterIdentity;
+import org.apache.pinot.broker.api.RequesterIdentity;
+import org.apache.pinot.common.config.provider.UserCache;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.utils.BcryptUtils;
+import org.apache.pinot.core.auth.BasicAuthPrincipal;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.core.auth.ZkBasicAuthPrincipal;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the
"pinot.broker.access.control" family of properties.
+ *
+ * <pre>
+ * Example:
+ * pinot.broker.access.control.principals=admin123,user456
+ * pinot.broker.access.control.principals.admin123.password=verysecret
+ * pinot.broker.access.control.principals.user456.password=kindasecret
+ *
pinot.broker.access.control.principals.user456.tables=stuff,lessImportantStuff
+ * </pre>
+ */
Review comment:
would you mind updating the javadoc?
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -239,7 +241,10 @@ private static long getRandomInitialDelayInSeconds() {
private static final boolean DEFAULT_ENABLE_SPLIT_COMMIT = true;
private static final int DEFAULT_JERSEY_ADMIN_PORT = 21000;
private static final String DEFAULT_ACCESS_CONTROL_FACTORY_CLASS =
- "org.apache.pinot.controller.api.access.AllowAllAccessFactory";
+ "org.apache.pinot.controller.api.access.ZkBasicAuthAccessControlFactory";
+// "org.apache.pinot.controller.api.access.AllowAllAccessFactory";
Review comment:
change to legacy behavior?
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
##########
@@ -55,10 +57,16 @@ public BasicAuthAccessControlFactory() {
// left blank
}
+ @Override
public void init(PinotConfiguration configuration) {
_accessControl = new
BasicAuthAccessControl(BasicAuthUtils.extractBasicAuthPrincipals(configuration,
PREFIX));
}
+ @Override
+ public void init(ZkHelixPropertyStore<ZNRecord> propertyStore) {
+ }
+
+ @Override
Review comment:
same here.
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/ZkBasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,132 @@
+/**
+ * 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.pinot.controller.api.access;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import javax.ws.rs.core.HttpHeaders;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.pinot.common.config.provider.UserCache;
+import org.apache.pinot.common.utils.BcryptUtils;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.core.auth.ZkBasicAuthPrincipal;
+import org.apache.pinot.spi.config.user.ComponentType;
+import org.apache.pinot.spi.config.user.RoleType;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the
"controller.admin.access.control" family of
+ * properties.
+ *
+ * <pre>
+ * Example:
+ * controller.admin.access.control.principals=admin123,user456
+ * controller.admin.access.control.principals.admin123.password=verysecret
+ * controller.admin.access.control.principals.user456.password=kindasecret
+ *
controller.admin.access.control.principals.user456.tables=stuff,lessImportantStuff
+ *
controller.admin.access.control.principals.user456.permissions=read,update
+ * </pre>
+ */
Review comment:
javadoc
--
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]
