You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/02/25 19:56:31 UTC
[GitHub] [incubator-pinot] apucher opened a new pull request #6613: [WIP] Basic auth controller
apucher opened a new pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613
## Description
Adds support for token-based authentication in pinot-controller, pinot-broker, pinot-server, and any associated tooling. Furthermore, provides a sample implementation using basic auth.
Builds on #6507, may relate to #6435.
This is work in progress. Changes are invasive since pinot's existing implementation does not consider potential authentication needs in several places.
## Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
* [ ] Yes (Please label as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR fix a zero-downtime upgrade introduced earlier?
* [ ] Yes (Please label this as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR otherwise need attention when creating release notes? Things to consider:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
* [x] Yes (Please label this PR as **<code>release-notes</code>** and complete the section on Release Notes)
## Release Notes
TBD
## Documentation
TBD
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher merged pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher merged pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r601723887
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/fetcher/SegmentFetcherFactory.java
##########
@@ -34,29 +34,40 @@
public class SegmentFetcherFactory {
- private SegmentFetcherFactory() {
- }
+ private final static SegmentFetcherFactory instance = new SegmentFetcherFactory();
static final String SEGMENT_FETCHER_CLASS_KEY_SUFFIX = ".class";
private static final String PROTOCOLS_KEY = "protocols";
+ private static final String AUTH_TOKEN_KEY = "auth.token";
private static final String ENCODED_SUFFIX = ".enc";
private static final Logger LOGGER = LoggerFactory.getLogger(SegmentFetcherFactory.class);
- private static final Map<String, SegmentFetcher> SEGMENT_FETCHER_MAP = new HashMap<>();
- private static final SegmentFetcher DEFAULT_HTTP_SEGMENT_FETCHER = new HttpSegmentFetcher();
- private static final SegmentFetcher DEFAULT_PINOT_FS_SEGMENT_FETCHER = new PinotFSSegmentFetcher();
-
- static {
Review comment:
I examined the history dating back to PRs #1970 (caused a problem), #1973, #2171, #2200, #2210, and #2238. I am not able to spot anything that may cause problems. If the code structure will allow it, if you can commit this class independently that would be awesome. If not, we will take this.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r594764360
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java
##########
@@ -168,11 +185,19 @@ public static URI getRetrieveSchemaURI(String protocol, String host, int port, S
return getURI(protocol, host, port, SCHEMA_PATH + "/" + schemaName);
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
Review comment:
added
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java
##########
@@ -139,25 +140,41 @@ private static URI getURI(String protocol, String host, int port, String path)
return new URI(protocol, null, host, port, path, null, null);
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
+ */
+ @Deprecated
public static URI getRetrieveTableConfigHttpURI(String host, int port, String rawTableName)
throws URISyntaxException {
return getURI(HTTP, host, port, TABLES_PATH + "/" + rawTableName);
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
+ */
+ @Deprecated
public static URI getDeleteSegmentHttpUri(String host, int port, String rawTableName, String segmentName,
String tableType)
throws URISyntaxException {
return new URI(StringUtil.join("/", StringUtils.chomp(HTTP + "://" + host + ":" + port, "/"), OLD_SEGMENT_PATH,
rawTableName + "/" + URIUtils.encode(segmentName) + TYPE_DELIMITER + tableType));
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
+ */
+ @Deprecated
public static URI getRetrieveAllSegmentWithTableTypeHttpUri(String host, int port, String rawTableName,
String tableType)
throws URISyntaxException {
return new URI(StringUtil.join("/", StringUtils.chomp(HTTP + "://" + host + ":" + port, "/"), OLD_SEGMENT_PATH,
rawTableName + TYPE_DELIMITER + tableType));
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
Review comment:
added
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java
##########
@@ -139,25 +140,41 @@ private static URI getURI(String protocol, String host, int port, String path)
return new URI(protocol, null, host, port, path, null, null);
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
+ */
+ @Deprecated
public static URI getRetrieveTableConfigHttpURI(String host, int port, String rawTableName)
throws URISyntaxException {
return getURI(HTTP, host, port, TABLES_PATH + "/" + rawTableName);
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
+ */
+ @Deprecated
public static URI getDeleteSegmentHttpUri(String host, int port, String rawTableName, String segmentName,
String tableType)
throws URISyntaxException {
return new URI(StringUtil.join("/", StringUtils.chomp(HTTP + "://" + host + ":" + port, "/"), OLD_SEGMENT_PATH,
rawTableName + "/" + URIUtils.encode(segmentName) + TYPE_DELIMITER + tableType));
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
Review comment:
added
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java
##########
@@ -139,25 +140,41 @@ private static URI getURI(String protocol, String host, int port, String path)
return new URI(protocol, null, host, port, path, null, null);
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
+ */
+ @Deprecated
public static URI getRetrieveTableConfigHttpURI(String host, int port, String rawTableName)
throws URISyntaxException {
return getURI(HTTP, host, port, TABLES_PATH + "/" + rawTableName);
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
+ */
+ @Deprecated
Review comment:
added
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java
##########
@@ -139,25 +140,41 @@ private static URI getURI(String protocol, String host, int port, String path)
return new URI(protocol, null, host, port, path, null, null);
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
+ */
+ @Deprecated
public static URI getRetrieveTableConfigHttpURI(String host, int port, String rawTableName)
throws URISyntaxException {
return getURI(HTTP, host, port, TABLES_PATH + "/" + rawTableName);
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
Review comment:
added
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java
##########
@@ -139,25 +140,41 @@ private static URI getURI(String protocol, String host, int port, String path)
return new URI(protocol, null, host, port, path, null, null);
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
Review comment:
added
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on pull request #6613: [WIP] Basic auth controller
Posted by GitBox <gi...@apache.org>.
apucher commented on pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#issuecomment-786163735
@icefury71
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r599077583
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/fetcher/BaseSegmentFetcher.java
##########
@@ -36,6 +35,7 @@
public static final String RETRY_COUNT_CONFIG_KEY = "retry.count";
public static final String RETRY_WAIT_MS_CONFIG_KEY = "retry.wait.ms";
public static final String RETRY_DELAY_SCALE_FACTOR_CONFIG_KEY = "retry.delay.scale.factor";
+ public static final String AUTH_TOKEN = "auth.token";
Review comment:
done
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/fetcher/SegmentFetcherFactory.java
##########
@@ -34,29 +34,40 @@
public class SegmentFetcherFactory {
- private SegmentFetcherFactory() {
- }
+ private final static SegmentFetcherFactory instance = new SegmentFetcherFactory();
static final String SEGMENT_FETCHER_CLASS_KEY_SUFFIX = ".class";
private static final String PROTOCOLS_KEY = "protocols";
+ private static final String AUTH_TOKEN_KEY = "auth.token";
Review comment:
done
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r594721563
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -180,6 +180,16 @@ public BrokerResponse handleRequest(JsonNode request, @Nullable RequesterIdentit
requestStatistics.setRequestId(requestId);
requestStatistics.setRequestArrivalTimeMillis(System.currentTimeMillis());
+ // first-stage access control to prevent unauthenticated requests from using up resources
+ // secondary table-level check comes later
+ boolean hasAccess = _accessControlFactory.create().hasAccess(requesterIdentity);
+ if (!hasAccess) {
+ _brokerMetrics.addMeteredTableValue(null, BrokerMeter.REQUEST_DROPPED_DUE_TO_ACCESS_ERROR, 1);
Review comment:
You may want to consider a different metric here than in line 261
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java
##########
@@ -139,25 +140,41 @@ private static URI getURI(String protocol, String host, int port, String path)
return new URI(protocol, null, host, port, path, null, null);
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
Review comment:
Can you provide a pointer to the method to be used instead? Do we need to introduce a method that takes authToken as an additional argument ? Perhaps it should specify "null" (the string) in the calling sequence?
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java
##########
@@ -139,25 +140,41 @@ private static URI getURI(String protocol, String host, int port, String path)
return new URI(protocol, null, host, port, path, null, null);
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
+ */
+ @Deprecated
public static URI getRetrieveTableConfigHttpURI(String host, int port, String rawTableName)
throws URISyntaxException {
return getURI(HTTP, host, port, TABLES_PATH + "/" + rawTableName);
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
Review comment:
same. need an alternative so the user can migrate and stop using the deprecated methods. We can remove the deprecated ones in a subsequent release.
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/fetcher/BaseSegmentFetcher.java
##########
@@ -36,6 +35,7 @@
public static final String RETRY_COUNT_CONFIG_KEY = "retry.count";
public static final String RETRY_WAIT_MS_CONFIG_KEY = "retry.wait.ms";
public static final String RETRY_DELAY_SCALE_FACTOR_CONFIG_KEY = "retry.delay.scale.factor";
+ public static final String AUTH_TOKEN = "auth.token";
Review comment:
+1 As long as it has the same semantics as the auth token defined there, we should be able to re-use that.
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -305,6 +312,12 @@
public static final String CONFIG_OF_CONTROLLER_HTTPS_PORT = "controller.port";
public static final String CONFIG_OF_SEGMENT_UPLOAD_REQUEST_TIMEOUT_MS = "upload.request.timeout.ms";
+ /**
+ * Service token for accessing protected controller APIs.
Review comment:
Hmmm what is the difference between this config and the config in line 243 (and then in 371)? The comments all say the same thing :)
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java
##########
@@ -168,11 +185,19 @@ public static URI getRetrieveSchemaURI(String protocol, String host, int port, S
return getURI(protocol, host, port, SCHEMA_PATH + "/" + schemaName);
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
Review comment:
same
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java
##########
@@ -139,25 +140,41 @@ private static URI getURI(String protocol, String host, int port, String path)
return new URI(protocol, null, host, port, path, null, null);
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
+ */
+ @Deprecated
public static URI getRetrieveTableConfigHttpURI(String host, int port, String rawTableName)
throws URISyntaxException {
return getURI(HTTP, host, port, TABLES_PATH + "/" + rawTableName);
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
+ */
+ @Deprecated
public static URI getDeleteSegmentHttpUri(String host, int port, String rawTableName, String segmentName,
String tableType)
throws URISyntaxException {
return new URI(StringUtil.join("/", StringUtils.chomp(HTTP + "://" + host + ":" + port, "/"), OLD_SEGMENT_PATH,
rawTableName + "/" + URIUtils.encode(segmentName) + TYPE_DELIMITER + tableType));
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
Review comment:
same
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java
##########
@@ -139,25 +140,41 @@ private static URI getURI(String protocol, String host, int port, String path)
return new URI(protocol, null, host, port, path, null, null);
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
+ */
+ @Deprecated
public static URI getRetrieveTableConfigHttpURI(String host, int port, String rawTableName)
throws URISyntaxException {
return getURI(HTTP, host, port, TABLES_PATH + "/" + rawTableName);
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
+ */
+ @Deprecated
Review comment:
same
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java
##########
@@ -139,25 +140,41 @@ private static URI getURI(String protocol, String host, int port, String path)
return new URI(protocol, null, host, port, path, null, null);
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
+ */
+ @Deprecated
public static URI getRetrieveTableConfigHttpURI(String host, int port, String rawTableName)
throws URISyntaxException {
return getURI(HTTP, host, port, TABLES_PATH + "/" + rawTableName);
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
+ */
+ @Deprecated
public static URI getDeleteSegmentHttpUri(String host, int port, String rawTableName, String segmentName,
String tableType)
throws URISyntaxException {
return new URI(StringUtil.join("/", StringUtils.chomp(HTTP + "://" + host + ":" + port, "/"), OLD_SEGMENT_PATH,
rawTableName + "/" + URIUtils.encode(segmentName) + TYPE_DELIMITER + tableType));
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
+ */
+ @Deprecated
public static URI getRetrieveAllSegmentWithTableTypeHttpUri(String host, int port, String rawTableName,
String tableType)
throws URISyntaxException {
return new URI(StringUtil.join("/", StringUtils.chomp(HTTP + "://" + host + ":" + port, "/"), OLD_SEGMENT_PATH,
rawTableName + TYPE_DELIMITER + tableType));
}
+ /**
+ * Deprecated due to lack of protocol/scheme support. May break for deployments with TLS/SSL enabled
Review comment:
same
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r596212308
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BasicAuthBatchIntegrationTest.java
##########
@@ -0,0 +1,177 @@
+/**
+ * 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.integration.tests;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.google.common.base.Preconditions;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.util.Collections;
+import java.util.Map;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.IOUtils;
+import org.apache.pinot.controller.helix.core.minion.generator.PinotTaskGenerator;
+import org.apache.pinot.minion.executor.PinotTaskExecutor;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.JsonUtils;
+import org.apache.pinot.tools.BootstrapTableTool;
+import org.testng.Assert;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import static org.apache.pinot.integration.tests.BasicAuthTestUtils.AUTH_HEADER;
+import static org.apache.pinot.integration.tests.BasicAuthTestUtils.AUTH_HEADER_USER;
+import static org.apache.pinot.integration.tests.BasicAuthTestUtils.AUTH_TOKEN;
+
+
+/**
+ * Integration test that provides example of {@link PinotTaskGenerator} and {@link PinotTaskExecutor} and tests simple
+ * minion functionality.
+ */
+public class BasicAuthBatchIntegrationTest extends ClusterTest {
+ private static final String BOOTSTRAP_DATA_DIR = "/examples/batch/baseballStats";
+ private static final String SCHEMA_FILE = "baseballStats_schema.json";
+ private static final String CONFIG_FILE = "baseballStats_offline_table_config.json";
+ private static final String DATA_FILE = "baseballStats_data.csv";
+ private static final String JOB_FILE = "ingestionJobSpec.yaml";
+
+ @BeforeClass
+ public void setUp()
+ throws Exception {
+ startZk();
+ startController();
+ startBroker();
+ startServer();
+ startMinion(Collections.emptyList(), Collections.emptyList());
+ }
+
+ @AfterClass(alwaysRun = true)
+ public void tearDown()
+ throws Exception {
+ stopMinion();
+ stopServer();
+ stopBroker();
+ stopController();
+ stopZk();
+ }
+
+ @Override
+ public Map<String, Object> getDefaultControllerConfiguration() {
+ return BasicAuthTestUtils.addControllerConfiguration(super.getDefaultControllerConfiguration());
+ }
+
+ @Override
+ protected PinotConfiguration getDefaultBrokerConfiguration() {
+ return BasicAuthTestUtils.addBrokerConfiguration(super.getDefaultBrokerConfiguration().toMap());
+ }
+
+ @Override
+ protected PinotConfiguration getDefaultServerConfiguration() {
+ return BasicAuthTestUtils.addServerConfiguration(super.getDefaultServerConfiguration().toMap());
+ }
+
+ @Override
+ protected PinotConfiguration getDefaultMinionConfiguration() {
+ return BasicAuthTestUtils.addMinionConfiguration(super.getDefaultMinionConfiguration().toMap());
+ }
+
+ @Test
+ public void testBrokerNoAuth()
+ throws Exception {
+ JsonNode response =
+ JsonUtils.stringToJsonNode(sendPostRequest("http://localhost:18099/query/sql", "{\"sql\":\"SELECT now()\"}"));
+ Assert.assertFalse(response.has("resultTable"), "must not return result table");
+ Assert.assertTrue(response.get("exceptions").get(0).get("errorCode").asInt() != 0, "must return error code");
+ }
+
+ @Test
+ public void testBroker()
+ throws Exception {
+ JsonNode response = JsonUtils.stringToJsonNode(
+ sendPostRequest("http://localhost:18099/query/sql", "{\"sql\":\"SELECT now()\"}", AUTH_HEADER));
+ Assert.assertEquals(response.get("resultTable").get("dataSchema").get("columnDataTypes").get(0).asText(), "LONG",
+ "must return result with LONG value");
+ Assert.assertTrue(response.get("exceptions").isEmpty(), "must not return exception");
+ }
+
+ @Test
+ public void testControllerGetTables()
+ throws Exception {
+ JsonNode response = JsonUtils.stringToJsonNode(sendGetRequest("http://localhost:18998/tables", AUTH_HEADER));
+ Assert.assertTrue(response.get("tables").isArray(), "must return table array");
+ }
+
+ @Test
+ public void testIngestionBatch()
+ throws Exception {
+ File quickstartTmpDir = new File(FileUtils.getTempDirectory(), String.valueOf(System.currentTimeMillis()));
+ FileUtils.forceDeleteOnExit(quickstartTmpDir);
+
+ File baseDir = new File(quickstartTmpDir, "baseballStats");
+ File dataDir = new File(baseDir, "rawdata");
+ File schemaFile = new File(baseDir, SCHEMA_FILE);
+ File configFile = new File(baseDir, CONFIG_FILE);
+ File dataFile = new File(dataDir, DATA_FILE);
+ File jobFile = new File(baseDir, JOB_FILE);
+ Preconditions.checkState(dataDir.mkdirs());
+
+ FileUtils.copyURLToFile(getClass().getResource(BOOTSTRAP_DATA_DIR + "/" + SCHEMA_FILE), schemaFile);
+ FileUtils.copyURLToFile(getClass().getResource(BOOTSTRAP_DATA_DIR + "/" + CONFIG_FILE), configFile);
+ FileUtils.copyURLToFile(getClass().getResource(BOOTSTRAP_DATA_DIR + "/rawdata/" + DATA_FILE), dataFile);
+ FileUtils.copyURLToFile(getClass().getResource(BOOTSTRAP_DATA_DIR + "/" + JOB_FILE), jobFile);
+
+ // patch ingestion job
+ String jobFileContents = IOUtils.toString(new FileInputStream(jobFile));
+ IOUtils.write(jobFileContents.replaceAll("9000", String.valueOf(DEFAULT_CONTROLLER_PORT)),
+ new FileOutputStream(jobFile));
+
+ new BootstrapTableTool("http", "localhost", DEFAULT_CONTROLLER_PORT, baseDir.getAbsolutePath(), AUTH_TOKEN)
+ .execute();
+
+ Thread.sleep(5000);
+
+ // admin with full access
+ JsonNode response = JsonUtils.stringToJsonNode(
+ sendPostRequest("http://localhost:18099/query/sql", "{\"sql\":\"SELECT count(*) FROM baseballStats\"}",
+ AUTH_HEADER));
+ Assert.assertEquals(response.get("resultTable").get("dataSchema").get("columnDataTypes").get(0).asText(), "LONG",
+ "must return result with LONG value");
+ Assert.assertEquals(response.get("resultTable").get("dataSchema").get("columnNames").get(0).asText(), "count(*)",
+ "must return column name 'count(*)");
+ Assert.assertEquals(response.get("resultTable").get("rows").get(0).get(0).asInt(), 97889,
+ "must return row count 97889");
+ Assert.assertTrue(response.get("exceptions").isEmpty(), "must not return exception");
+
+ // user with valid auth but no table access
+ JsonNode responseUser = JsonUtils.stringToJsonNode(
+ sendPostRequest("http://localhost:18099/query/sql", "{\"sql\":\"SELECT count(*) FROM baseballStats\"}",
+ AUTH_HEADER_USER));
+ Assert.assertFalse(responseUser.has("resultTable"), "must not return result table");
+ Assert.assertTrue(responseUser.get("exceptions").get(0).get("errorCode").asInt() != 0, "must return error code");
+ }
+
+// // TODO this endpoint should be protected once UI supports auth
+// @Test
+// public void testControllerGetTablesNoAuth()
+// throws Exception {
+// System.out.println(sendGetRequest("http://localhost:18998/tables"));
+// }
Review comment:
we'll wait for the web UI to fully support auth, then protect all the remaining endpoints (including the tables endpoint referenced here)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#issuecomment-788191238
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=h1) Report
> Merging [#6613](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=desc) (d2bb5c7) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `0.70%`.
> The diff coverage is `61.85%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6613 +/- ##
==========================================
- Coverage 66.44% 65.74% -0.71%
==========================================
Files 1075 1394 +319
Lines 54773 67610 +12837
Branches 8168 9798 +1630
==========================================
+ Hits 36396 44452 +8056
- Misses 15700 19990 +4290
- Partials 2677 3168 +491
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `65.74% <61.85%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <0.00%> (-79.32%)` | :arrow_down: |
| [...pinot/broker/api/resources/PinotClientRequest.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdENsaWVudFJlcXVlc3QuamF2YQ==) | `0.00% <0.00%> (-27.28%)` | :arrow_down: |
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `85.71% <ø> (-14.29%)` | :arrow_down: |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `33.96% <0.00%> (-32.71%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `66.66% <ø> (+9.52%)` | :arrow_up: |
| [...n/java/org/apache/pinot/client/BrokerResponse.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Jyb2tlclJlc3BvbnNlLmphdmE=) | `100.00% <ø> (ø)` | |
| [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `35.55% <ø> (-13.29%)` | :arrow_down: |
| [...org/apache/pinot/client/DynamicBrokerSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0R5bmFtaWNCcm9rZXJTZWxlY3Rvci5qYXZh) | `82.85% <ø> (+10.12%)` | :arrow_up: |
| [...n/java/org/apache/pinot/client/ExecutionStats.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0V4ZWN1dGlvblN0YXRzLmphdmE=) | `68.88% <ø> (ø)` | |
| ... and [1283 more](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=footer). Last update [36d6db6...d2bb5c7](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#issuecomment-800634068
> Working the entire realtime flow inside an integration test is definitely useful. Where I am afraid things ay break is if servers are running older version and controllers newer, or vice versa. Realtime segment upload failure will block realtime consumption. If you can ascertain that as long as auth tokens are not configured anywhere the behavior is the same that would be awesome.
Fortunately, there aren't any logic changes here. Any auth tokens are passed via http headers and are therefore fully backwards-compatible.
The only way the segment upload can break is if controllers are configured to expect auth headers, while servers don't provide any. This would be an operations / configuration error, however. The inverse, i.e. servers providing auth headers without controllers expecting them, will work without issues (this also enables zero-downtime upgrades to authentication).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r592800726
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,106 @@
+/**
+ * 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.Collection;
+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.pinot.core.auth.BasicAuthPrincipal;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * 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>
+ */
+public class BasicAuthAccessControlFactory implements AccessControlFactory {
+ private static final String PREFIX = "controller.admin.access.control.principals";
+
+ private static final String HEADER_AUTHORIZATION = "Authorization";
+
+ private AccessControl _accessControl;
+
+ public void init(PinotConfiguration configuration) {
+ _accessControl = new BasicAuthAccessControl(BasicAuthUtils.extractBasicAuthPrincipals(configuration, PREFIX));
+ }
+
+ @Override
+ public AccessControl create() {
+ return _accessControl;
Review comment:
I have to maintain compatibility with the existing interfaces. pinot's config management isn't great, and this seems to be the only graceful way of getting access to pinot's config properties
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r596214769
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java
##########
@@ -26,6 +26,9 @@
@InterfaceAudience.Public
@InterfaceStability.Stable
public interface AccessControl {
+ String WORKFLOW_NONE = "NONE";
+ String WORKFLOW_BASIC = "BASIC";
+ String WORKFLOW_OAUTH2 = "OAUTH2";
Review comment:
Hardcoding this in an enum would eliminate any extensibility of `AuthWorkflowInfo` by third parties
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #6613: [WIP] Basic auth controller
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#issuecomment-788191238
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=h1) Report
> Merging [#6613](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=desc) (78663c2) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.56%`.
> The diff coverage is `43.90%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6613 +/- ##
===========================================
- Coverage 66.44% 43.88% -22.57%
===========================================
Files 1075 1366 +291
Lines 54773 66622 +11849
Branches 8168 9709 +1541
===========================================
- Hits 36396 29238 -7158
- Misses 15700 34934 +19234
+ Partials 2677 2450 -227
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `43.88% <43.90%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...t/broker/broker/BasicAuthAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0Jhc2ljQXV0aEFjY2Vzc0NvbnRyb2xGYWN0b3J5LmphdmE=) | `0.00% <0.00%> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...org/apache/pinot/broker/queryquota/HitCounter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IaXRDb3VudGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...che/pinot/broker/queryquota/MaxHitRateTracker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9NYXhIaXRSYXRlVHJhY2tlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...ache/pinot/broker/queryquota/QueryQuotaEntity.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9RdWVyeVF1b3RhRW50aXR5LmphdmE=) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1RpbWVTZWdtZW50UHJ1bmVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/interval/Interval.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsLmphdmE=) | `0.00% <0.00%> (ø)` | |
| ... and [1395 more](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=footer). Last update [318a4c8...78663c2](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#issuecomment-807929383
> > lgtm other than a very minor comment. I have resolved all the comments of mine. Can you also work with @sajjad-moradi to resolve his comments as well?
> > thanks for your patience.
>
> Awesome. Thanks, Subbu. Yeah, so many changes with all the glue and tooling code.
>
> Addressed your remaining comments, and waiting for @sajjad-moradi to circle back.
LGTM.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r592801658
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java
##########
@@ -0,0 +1,55 @@
+/**
+ * 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.core.auth;
+
+import java.util.Set;
+
+
+/**
+ * Container object for basic auth principal
+ */
+public class BasicAuthPrincipal {
+ private final String _name;
+ private final String _token;
+ private final Set<String> _tables;
+ private final Set<String> _permissions;
Review comment:
should be `String` to retain extensibility of AccessControl implementations
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] icefury71 commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
icefury71 commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r592742130
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/fetcher/BaseSegmentFetcher.java
##########
@@ -36,6 +35,7 @@
public static final String RETRY_COUNT_CONFIG_KEY = "retry.count";
public static final String RETRY_WAIT_MS_CONFIG_KEY = "retry.wait.ms";
public static final String RETRY_DELAY_SCALE_FACTOR_CONFIG_KEY = "retry.delay.scale.factor";
+ public static final String AUTH_TOKEN = "auth.token";
Review comment:
Looks like this is already defined in CommonConstants ?
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/segment/generation/SegmentGenerationUtils.java
##########
@@ -87,7 +94,12 @@ public static Schema getSchema(String schemaURIString) {
}
}
+ @Deprecated
Review comment:
Is it necessary to deprecate this ? For folks who don't care about security, they will have to keep passing null here ?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,106 @@
+/**
+ * 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.Collection;
+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.pinot.core.auth.BasicAuthPrincipal;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * 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>
+ */
+public class BasicAuthAccessControlFactory implements AccessControlFactory {
+ private static final String PREFIX = "controller.admin.access.control.principals";
+
+ private static final String HEADER_AUTHORIZATION = "Authorization";
+
+ private AccessControl _accessControl;
+
+ public void init(PinotConfiguration configuration) {
+ _accessControl = new BasicAuthAccessControl(BasicAuthUtils.extractBasicAuthPrincipals(configuration, PREFIX));
+ }
+
+ @Override
+ public AccessControl create() {
+ return _accessControl;
+ }
+
+ /**
+ * Access Control using header-based basic http authentication
+ */
+ private static class BasicAuthAccessControl implements AccessControl {
+ private final Map<String, BasicAuthPrincipal> _principals;
+
+ public BasicAuthAccessControl(Collection<BasicAuthPrincipal> principals) {
+ this._principals = principals.stream().collect(Collectors.toMap(BasicAuthPrincipal::getToken, p -> p));
+ }
+
+ @Override
+ public boolean hasDataAccess(HttpHeaders httpHeaders, String tableName) {
+ return getPrincipal(httpHeaders).filter(p -> p.hasTable(tableName)).isPresent();
+ }
+
+ @Override
+ public boolean hasAccess(String tableName, AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) {
Review comment:
Should all these methods be defined in some interface ? (hasDataAccess, has Access , blah)
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,106 @@
+/**
+ * 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.Collection;
+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.pinot.core.auth.BasicAuthPrincipal;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * 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>
+ */
+public class BasicAuthAccessControlFactory implements AccessControlFactory {
+ private static final String PREFIX = "controller.admin.access.control.principals";
+
+ private static final String HEADER_AUTHORIZATION = "Authorization";
+
+ private AccessControl _accessControl;
+
+ public void init(PinotConfiguration configuration) {
+ _accessControl = new BasicAuthAccessControl(BasicAuthUtils.extractBasicAuthPrincipals(configuration, PREFIX));
+ }
+
+ @Override
Review comment:
Not sure if we need Override here?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java
##########
@@ -0,0 +1,75 @@
+/**
+ * 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.resources;
+
+import io.swagger.annotations.Api;
+import io.swagger.annotations.ApiOperation;
+import io.swagger.annotations.ApiParam;
+import io.swagger.annotations.ApiResponse;
+import io.swagger.annotations.ApiResponses;
+import javax.inject.Inject;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.MediaType;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.controller.api.access.AccessControl;
+import org.apache.pinot.controller.api.access.AccessControlFactory;
+import org.apache.pinot.controller.api.access.AccessType;
+
+
+@Api(tags = "Auth")
+@Path("/")
+public class PinotControllerAuthResource {
+
+ @Inject
+ private AccessControlFactory _accessControlFactory;
+
+ @Context
+ HttpHeaders _httpHeaders;
+
+ @GET
+ @Path("auth/verify")
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Check whether authentication is enabled")
+ @ApiResponses(value = {@ApiResponse(code = 200, message = "Verification result provided"), @ApiResponse(code = 500, message = "Verification error")})
Review comment:
Should we use 401 instead of 500 ?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java
##########
@@ -64,4 +67,32 @@ default boolean hasAccess(String tableName, AccessType accessType, HttpHeaders h
default boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) {
return true;
}
+
+ /**
+ * Return workflow info for authenticating users. Not all workflows may be supported by the pinot UI implementation.
+ *
+ * @return workflow info for user authentication
+ */
+ default AuthWorkflowInfo getAuthWorkflowInfo() {
+ return new AuthWorkflowInfo(WORKFLOW_NONE);
+ }
+
+ /**
+ * Container for authentication workflow info. May be extended by implementations.
+ */
+ class AuthWorkflowInfo {
+ String workflow;
+
+ public AuthWorkflowInfo(String workflow) {
+ this.workflow = workflow;
+ }
+
+ public String getWorkflow() {
+ return workflow;
+ }
+
+ public void setWorkflow(String workflow) {
Review comment:
Do we need set here ? Once the auth workflow is created, would it ever change ?
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/fetcher/SegmentFetcherFactory.java
##########
@@ -34,29 +34,40 @@
public class SegmentFetcherFactory {
- private SegmentFetcherFactory() {
- }
+ private final static SegmentFetcherFactory instance = new SegmentFetcherFactory();
static final String SEGMENT_FETCHER_CLASS_KEY_SUFFIX = ".class";
private static final String PROTOCOLS_KEY = "protocols";
+ private static final String AUTH_TOKEN_KEY = "auth.token";
Review comment:
Same here
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java
##########
@@ -0,0 +1,55 @@
+/**
+ * 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.core.auth;
+
+import java.util.Set;
+
+
+/**
+ * Container object for basic auth principal
+ */
+public class BasicAuthPrincipal {
+ private final String _name;
+ private final String _token;
+ private final Set<String> _tables;
+ private final Set<String> _permissions;
Review comment:
Should this be of type String or better to have "AccessType" ?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,106 @@
+/**
+ * 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.Collection;
+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.pinot.core.auth.BasicAuthPrincipal;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * 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>
+ */
+public class BasicAuthAccessControlFactory implements AccessControlFactory {
+ private static final String PREFIX = "controller.admin.access.control.principals";
+
+ private static final String HEADER_AUTHORIZATION = "Authorization";
+
+ private AccessControl _accessControl;
+
+ public void init(PinotConfiguration configuration) {
+ _accessControl = new BasicAuthAccessControl(BasicAuthUtils.extractBasicAuthPrincipals(configuration, PREFIX));
+ }
+
+ @Override
+ public AccessControl create() {
+ return _accessControl;
+ }
+
+ /**
+ * Access Control using header-based basic http authentication
+ */
+ private static class BasicAuthAccessControl implements AccessControl {
+ private final Map<String, BasicAuthPrincipal> _principals;
+
+ public BasicAuthAccessControl(Collection<BasicAuthPrincipal> principals) {
+ this._principals = principals.stream().collect(Collectors.toMap(BasicAuthPrincipal::getToken, p -> p));
+ }
+
+ @Override
Review comment:
What are these Overrides for ?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,106 @@
+/**
+ * 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.Collection;
+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.pinot.core.auth.BasicAuthPrincipal;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * 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>
+ */
+public class BasicAuthAccessControlFactory implements AccessControlFactory {
+ private static final String PREFIX = "controller.admin.access.control.principals";
+
+ private static final String HEADER_AUTHORIZATION = "Authorization";
+
+ private AccessControl _accessControl;
+
+ public void init(PinotConfiguration configuration) {
+ _accessControl = new BasicAuthAccessControl(BasicAuthUtils.extractBasicAuthPrincipals(configuration, PREFIX));
+ }
+
+ @Override
+ public AccessControl create() {
+ return _accessControl;
Review comment:
So the caller has to invoke init before create ? Instead, should we check if _accessControl is initialized and take action accordingly ?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,106 @@
+/**
+ * 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.Collection;
+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.pinot.core.auth.BasicAuthPrincipal;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * 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>
+ */
+public class BasicAuthAccessControlFactory implements AccessControlFactory {
+ private static final String PREFIX = "controller.admin.access.control.principals";
+
+ private static final String HEADER_AUTHORIZATION = "Authorization";
+
+ private AccessControl _accessControl;
+
+ public void init(PinotConfiguration configuration) {
+ _accessControl = new BasicAuthAccessControl(BasicAuthUtils.extractBasicAuthPrincipals(configuration, PREFIX));
+ }
+
+ @Override
+ public AccessControl create() {
+ return _accessControl;
+ }
+
+ /**
+ * Access Control using header-based basic http authentication
+ */
+ private static class BasicAuthAccessControl implements AccessControl {
+ private final Map<String, BasicAuthPrincipal> _principals;
Review comment:
nit: rename to "_tokenToPrincipalsMap" just for ease of readability ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#issuecomment-788191238
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=h1) Report
> Merging [#6613](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=desc) (eb4f5ac) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.71%`.
> The diff coverage is `45.87%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6613 +/- ##
===========================================
- Coverage 66.44% 43.73% -22.72%
===========================================
Files 1075 1394 +319
Lines 54773 67615 +12842
Branches 8168 9799 +1631
===========================================
- Hits 36396 29570 -6826
- Misses 15700 35558 +19858
+ Partials 2677 2487 -190
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `43.73% <45.87%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...org/apache/pinot/broker/queryquota/HitCounter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IaXRDb3VudGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...che/pinot/broker/queryquota/MaxHitRateTracker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9NYXhIaXRSYXRlVHJhY2tlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...ache/pinot/broker/queryquota/QueryQuotaEntity.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9RdWVyeVF1b3RhRW50aXR5LmphdmE=) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1RpbWVTZWdtZW50UHJ1bmVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/interval/Interval.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...r/routing/segmentpruner/interval/IntervalTree.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsVHJlZS5qYXZh) | `0.00% <0.00%> (ø)` | |
| ... and [1423 more](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=footer). Last update [36d6db6...eb4f5ac](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r593523564
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java
##########
@@ -0,0 +1,75 @@
+/**
+ * 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.resources;
+
+import io.swagger.annotations.Api;
+import io.swagger.annotations.ApiOperation;
+import io.swagger.annotations.ApiParam;
+import io.swagger.annotations.ApiResponse;
+import io.swagger.annotations.ApiResponses;
+import javax.inject.Inject;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.MediaType;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.controller.api.access.AccessControl;
+import org.apache.pinot.controller.api.access.AccessControlFactory;
+import org.apache.pinot.controller.api.access.AccessType;
+
+
+@Api(tags = "Auth")
+@Path("/")
+public class PinotControllerAuthResource {
+
+ @Inject
+ private AccessControlFactory _accessControlFactory;
+
+ @Context
+ HttpHeaders _httpHeaders;
+
+ @GET
+ @Path("auth/verify")
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Check whether authentication is enabled")
+ @ApiResponses(value = {@ApiResponse(code = 200, message = "Verification result provided"), @ApiResponse(code = 500, message = "Verification error")})
+ public boolean verify(@ApiParam(value = "Table name without type") @QueryParam("tableName") String tableName,
+ @ApiParam(value = "API access type") @QueryParam("accessType") AccessType accessType,
+ @ApiParam(value = "Endpoint URL") @QueryParam("endpointUrl") String endpointUrl) {
+ AccessControl accessControl = _accessControlFactory.create();
+
+ if (StringUtils.isBlank(tableName)) {
+ return accessControl.hasAccess(accessType, _httpHeaders, endpointUrl);
+ }
+
+ return accessControl.hasAccess(tableName, accessType, _httpHeaders, endpointUrl);
+ }
+
+ @GET
+ @Path("auth/info")
Review comment:
added javadoc. auth info help the Pinot UI configure different auth workflows - no auth, basic auth, and oauth2
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r593503297
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/fetcher/SegmentFetcherFactory.java
##########
@@ -34,29 +34,40 @@
public class SegmentFetcherFactory {
- private SegmentFetcherFactory() {
- }
+ private final static SegmentFetcherFactory instance = new SegmentFetcherFactory();
static final String SEGMENT_FETCHER_CLASS_KEY_SUFFIX = ".class";
private static final String PROTOCOLS_KEY = "protocols";
+ private static final String AUTH_TOKEN_KEY = "auth.token";
private static final String ENCODED_SUFFIX = ".enc";
private static final Logger LOGGER = LoggerFactory.getLogger(SegmentFetcherFactory.class);
- private static final Map<String, SegmentFetcher> SEGMENT_FETCHER_MAP = new HashMap<>();
- private static final SegmentFetcher DEFAULT_HTTP_SEGMENT_FETCHER = new HttpSegmentFetcher();
- private static final SegmentFetcher DEFAULT_PINOT_FS_SEGMENT_FETCHER = new PinotFSSegmentFetcher();
-
- static {
Review comment:
The use of this class is pretty messy. The fetchers here were instantiated statically without configuration, but we now may need to inject authTokens. I've therefore refactored the class to maintain its public (static) interface, while setting properties in the starter before use.
With my manual testing, quickstart, and integration tests things seem to work, but I bet there;s a corner case lurking somewhere.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r599078262
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java
##########
@@ -64,4 +67,32 @@ default boolean hasAccess(String tableName, AccessType accessType, HttpHeaders h
default boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) {
return true;
}
+
+ /**
+ * Return workflow info for authenticating users. Not all workflows may be supported by the pinot UI implementation.
+ *
+ * @return workflow info for user authentication
+ */
+ default AuthWorkflowInfo getAuthWorkflowInfo() {
+ return new AuthWorkflowInfo(WORKFLOW_NONE);
+ }
+
+ /**
+ * Container for authentication workflow info. May be extended by implementations.
+ */
+ class AuthWorkflowInfo {
+ String workflow;
+
+ public AuthWorkflowInfo(String workflow) {
+ this.workflow = workflow;
+ }
+
+ public String getWorkflow() {
+ return workflow;
+ }
+
+ public void setWorkflow(String workflow) {
Review comment:
It's a POJO. While Pinot isn't consistent on this, most of them seem to prefer mutable state
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] icefury71 commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
icefury71 commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r592833328
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,106 @@
+/**
+ * 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.Collection;
+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.pinot.core.auth.BasicAuthPrincipal;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * 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>
+ */
+public class BasicAuthAccessControlFactory implements AccessControlFactory {
+ private static final String PREFIX = "controller.admin.access.control.principals";
+
+ private static final String HEADER_AUTHORIZATION = "Authorization";
+
+ private AccessControl _accessControl;
+
+ public void init(PinotConfiguration configuration) {
+ _accessControl = new BasicAuthAccessControl(BasicAuthUtils.extractBasicAuthPrincipals(configuration, PREFIX));
+ }
+
+ @Override
+ public AccessControl create() {
+ return _accessControl;
+ }
+
+ /**
+ * Access Control using header-based basic http authentication
+ */
+ private static class BasicAuthAccessControl implements AccessControl {
+ private final Map<String, BasicAuthPrincipal> _principals;
+
+ public BasicAuthAccessControl(Collection<BasicAuthPrincipal> principals) {
+ this._principals = principals.stream().collect(Collectors.toMap(BasicAuthPrincipal::getToken, p -> p));
+ }
+
+ @Override
+ public boolean hasDataAccess(HttpHeaders httpHeaders, String tableName) {
+ return getPrincipal(httpHeaders).filter(p -> p.hasTable(tableName)).isPresent();
+ }
+
+ @Override
+ public boolean hasAccess(String tableName, AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) {
Review comment:
My bad - I looked at the wrong interface. Sorry for the noise
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r592798280
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/segment/generation/SegmentGenerationUtils.java
##########
@@ -87,7 +94,12 @@ public static Schema getSchema(String schemaURIString) {
}
}
+ @Deprecated
Review comment:
users can keep *configuring* nulls, but any dev work should consider authentication from this point on. otherwise, we end up with a nightmare of a compatibility matrix for new features and auth support
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r594752354
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -305,6 +312,12 @@
public static final String CONFIG_OF_CONTROLLER_HTTPS_PORT = "controller.port";
public static final String CONFIG_OF_SEGMENT_UPLOAD_REQUEST_TIMEOUT_MS = "upload.request.timeout.ms";
+ /**
+ * Service token for accessing protected controller APIs.
Review comment:
all these do the same thing: authenticate with controller APIs. The only need for their replication is due to pinot's handling of configuration subsets. segment fetcher, uploader, etc. all manage their token separately.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r594771418
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -180,6 +180,16 @@ public BrokerResponse handleRequest(JsonNode request, @Nullable RequesterIdentit
requestStatistics.setRequestId(requestId);
requestStatistics.setRequestArrivalTimeMillis(System.currentTimeMillis());
+ // first-stage access control to prevent unauthenticated requests from using up resources
+ // secondary table-level check comes later
+ boolean hasAccess = _accessControlFactory.create().hasAccess(requesterIdentity);
+ if (!hasAccess) {
+ _brokerMetrics.addMeteredTableValue(null, BrokerMeter.REQUEST_DROPPED_DUE_TO_ACCESS_ERROR, 1);
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] icefury71 commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
icefury71 commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r592833378
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java
##########
@@ -0,0 +1,75 @@
+/**
+ * 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.resources;
+
+import io.swagger.annotations.Api;
+import io.swagger.annotations.ApiOperation;
+import io.swagger.annotations.ApiParam;
+import io.swagger.annotations.ApiResponse;
+import io.swagger.annotations.ApiResponses;
+import javax.inject.Inject;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.MediaType;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.controller.api.access.AccessControl;
+import org.apache.pinot.controller.api.access.AccessControlFactory;
+import org.apache.pinot.controller.api.access.AccessType;
+
+
+@Api(tags = "Auth")
+@Path("/")
+public class PinotControllerAuthResource {
+
+ @Inject
+ private AccessControlFactory _accessControlFactory;
+
+ @Context
+ HttpHeaders _httpHeaders;
+
+ @GET
+ @Path("auth/verify")
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Check whether authentication is enabled")
+ @ApiResponses(value = {@ApiResponse(code = 200, message = "Verification result provided"), @ApiResponse(code = 500, message = "Verification error")})
Review comment:
Makes sense
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r592815199
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,106 @@
+/**
+ * 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.Collection;
+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.pinot.core.auth.BasicAuthPrincipal;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * 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>
+ */
+public class BasicAuthAccessControlFactory implements AccessControlFactory {
+ private static final String PREFIX = "controller.admin.access.control.principals";
+
+ private static final String HEADER_AUTHORIZATION = "Authorization";
+
+ private AccessControl _accessControl;
+
+ public void init(PinotConfiguration configuration) {
+ _accessControl = new BasicAuthAccessControl(BasicAuthUtils.extractBasicAuthPrincipals(configuration, PREFIX));
+ }
+
+ @Override
+ public AccessControl create() {
+ return _accessControl;
+ }
+
+ /**
+ * Access Control using header-based basic http authentication
+ */
+ private static class BasicAuthAccessControl implements AccessControl {
+ private final Map<String, BasicAuthPrincipal> _principals;
+
+ public BasicAuthAccessControl(Collection<BasicAuthPrincipal> principals) {
+ this._principals = principals.stream().collect(Collectors.toMap(BasicAuthPrincipal::getToken, p -> p));
+ }
+
+ @Override
Review comment:
https://stackoverflow.com/questions/94361/when-do-you-use-javas-override-annotation-and-why
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r601726067
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java
##########
@@ -64,4 +67,36 @@ default boolean hasAccess(String tableName, AccessType accessType, HttpHeaders h
default boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) {
return true;
}
+
+ /**
+ * Return workflow info for authenticating users. Not all workflows may be supported by the pinot UI implementation.
+ *
+ * @return workflow info for user authentication
+ */
+ default AuthWorkflowInfo getAuthWorkflowInfo() {
+ return new AuthWorkflowInfo(WORKFLOW_NONE);
+ }
+
+ /**
+ * Container for authentication workflow info for the Pinot UI. May be extended by implementations.
+ *
+ * Auth workflow info hold any configuration necessary to execute a UI workflow. We currently foresee supporting NONE
+ * (auth disabled), BASIC (basic auth with username and password), and OAUTH2 (token-based workflow via external
+ * issuer)
+ */
+ class AuthWorkflowInfo {
Review comment:
@sajjad-moradi are you ok with this resolution?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#issuecomment-809592811
Thank you everyone for the reviews!
I've rebased the PR and I'll wait for one last CI run before merging.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] icefury71 commented on pull request #6613: [WIP] Basic auth controller
Posted by GitBox <gi...@apache.org>.
icefury71 commented on pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#issuecomment-790142216
Thanks so much for this PR. As part of this can we also add a new API to enable credential verification ? (eg: from the Cluster Manager UI). I'm thinking of something like this: "http://controller-host:port/verifyauth" where you pass in the token and verify whether its correct.
The other option is to annotate one of the existing APIs eg: GET /tables with "Authenticate" and reuse that. However, a new API might be cleaner. What do you think ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r592808133
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java
##########
@@ -0,0 +1,75 @@
+/**
+ * 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.resources;
+
+import io.swagger.annotations.Api;
+import io.swagger.annotations.ApiOperation;
+import io.swagger.annotations.ApiParam;
+import io.swagger.annotations.ApiResponse;
+import io.swagger.annotations.ApiResponses;
+import javax.inject.Inject;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.MediaType;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.controller.api.access.AccessControl;
+import org.apache.pinot.controller.api.access.AccessControlFactory;
+import org.apache.pinot.controller.api.access.AccessType;
+
+
+@Api(tags = "Auth")
+@Path("/")
+public class PinotControllerAuthResource {
+
+ @Inject
+ private AccessControlFactory _accessControlFactory;
+
+ @Context
+ HttpHeaders _httpHeaders;
+
+ @GET
+ @Path("auth/verify")
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Check whether authentication is enabled")
+ @ApiResponses(value = {@ApiResponse(code = 200, message = "Verification result provided"), @ApiResponse(code = 500, message = "Verification error")})
Review comment:
`/auth/verify` does not *require* authentication itself, i.e. it can't be `Unauthorized`/`Forbidden`. it does verify a provided token and return 200 with a body (true/false) or a 500 if there's an error processing the token
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r593485017
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -180,6 +180,16 @@ public BrokerResponse handleRequest(JsonNode request, @Nullable RequesterIdentit
requestStatistics.setRequestId(requestId);
requestStatistics.setRequestArrivalTimeMillis(System.currentTimeMillis());
+ // first-stage access control to prevent unauthenticated requests from using up resources
+ // secondary table-level check comes later
+ boolean hasAccess = _accessControlFactory.create().hasAccess(requesterIdentity, null);
Review comment:
added
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -235,6 +235,9 @@
"pinot.server.instance.realtime.alloc.offheap.direct";
public static final String PREFIX_OF_CONFIG_OF_PINOT_FS_FACTORY = "pinot.server.storage.factory";
public static final String PREFIX_OF_CONFIG_OF_PINOT_CRYPTER = "pinot.server.crypter";
+
+ public static final String CONFIG_OF_AUTH_TOKEN = "auth.token";
Review comment:
added
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -304,6 +307,7 @@
public static final String CONFIG_OF_CONTROLLER_HTTPS_ENABLED = "enabled";
public static final String CONFIG_OF_CONTROLLER_HTTPS_PORT = "controller.port";
public static final String CONFIG_OF_SEGMENT_UPLOAD_REQUEST_TIMEOUT_MS = "upload.request.timeout.ms";
+ public static final String CONFIG_OF_SEGMENT_UPLOAD_AUTH_TOKEN = "upload.auth.token";
Review comment:
added
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -353,6 +357,8 @@
public static final String PREFIX_OF_CONFIG_OF_SEGMENT_FETCHER_FACTORY = "segment.fetcher";
public static final String PREFIX_OF_CONFIG_OF_SEGMENT_UPLOADER = "segment.uploader";
public static final String PREFIX_OF_CONFIG_OF_PINOT_CRYPTER = "crypter";
+
+ public static final String CONFIG_OF_TASK_AUTH_TOKEN = "task.auth.token";
Review comment:
added
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#issuecomment-788191238
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=h1) Report
> Merging [#6613](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=desc) (b9b14a1) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `0.66%`.
> The diff coverage is `61.80%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6613 +/- ##
==========================================
- Coverage 66.44% 65.78% -0.67%
==========================================
Files 1075 1397 +322
Lines 54773 67731 +12958
Branches 8168 9809 +1641
==========================================
+ Hits 36396 44554 +8158
- Misses 15700 19989 +4289
- Partials 2677 3188 +511
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `65.78% <61.80%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <0.00%> (-79.32%)` | :arrow_down: |
| [...pinot/broker/api/resources/PinotClientRequest.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdENsaWVudFJlcXVlc3QuamF2YQ==) | `0.00% <0.00%> (-27.28%)` | :arrow_down: |
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `85.71% <ø> (-14.29%)` | :arrow_down: |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `33.96% <0.00%> (-32.71%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `66.66% <ø> (+9.52%)` | :arrow_up: |
| [...n/java/org/apache/pinot/client/BrokerResponse.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Jyb2tlclJlc3BvbnNlLmphdmE=) | `100.00% <ø> (ø)` | |
| [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `35.55% <ø> (-13.29%)` | :arrow_down: |
| [...ava/org/apache/pinot/client/ConnectionFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb25GYWN0b3J5LmphdmE=) | `23.07% <ø> (-22.38%)` | :arrow_down: |
| [...org/apache/pinot/client/DynamicBrokerSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0R5bmFtaWNCcm9rZXJTZWxlY3Rvci5qYXZh) | `82.85% <ø> (+10.12%)` | :arrow_up: |
| ... and [1286 more](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=footer). Last update [621ceb0...b9b14a1](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r594761496
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -180,6 +180,16 @@ public BrokerResponse handleRequest(JsonNode request, @Nullable RequesterIdentit
requestStatistics.setRequestId(requestId);
requestStatistics.setRequestArrivalTimeMillis(System.currentTimeMillis());
+ // first-stage access control to prevent unauthenticated requests from using up resources
+ // secondary table-level check comes later
+ boolean hasAccess = _accessControlFactory.create().hasAccess(requesterIdentity);
+ if (!hasAccess) {
+ _brokerMetrics.addMeteredTableValue(null, BrokerMeter.REQUEST_DROPPED_DUE_TO_ACCESS_ERROR, 1);
Review comment:
Ah, I know what I wanted to say:
```suggestion
_brokerMetrics.addMeteredGlobalValue(BrokerMeter.REQUEST_DROPPED_DUE_TO_ACCESS_ERROR, 1);
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#issuecomment-799883906
> > > Do you need to change LLCSegmentCompletionHandlers to pay attention to the auth token?
> >
> >
> > Probably. I'd appreciate your input on where this actually talks to controller APIs. I just noticed that this whole set of endpoints was excluded from my checklist (via swagger)
>
> These endpoints should not appear in Swagger because they are used by pinot-server to commit realtime segments using the segment commit protocol. They are not meant for administrative or user actions. These are completely pinot internal operations.
> The client side of this call is in ServerSegmentProtocolCompletionHandler.
>
> If you can make these changes in another PR, that will help reviewing a bit. If I miss something and it causes realtime segments not to complete segments, that will be really bad.
`ServerSegmentProtocolCompletionHandler` already has an `_authToken`. Imo the most important thing right now is getting out integration tests - batch is done, realtime is in the works.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r601802445
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/fetcher/SegmentFetcherFactory.java
##########
@@ -34,29 +34,40 @@
public class SegmentFetcherFactory {
- private SegmentFetcherFactory() {
- }
+ private final static SegmentFetcherFactory instance = new SegmentFetcherFactory();
Review comment:
done
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r592853188
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,106 @@
+/**
+ * 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.Collection;
+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.pinot.core.auth.BasicAuthPrincipal;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * 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>
+ */
+public class BasicAuthAccessControlFactory implements AccessControlFactory {
+ private static final String PREFIX = "controller.admin.access.control.principals";
+
+ private static final String HEADER_AUTHORIZATION = "Authorization";
+
+ private AccessControl _accessControl;
+
+ public void init(PinotConfiguration configuration) {
+ _accessControl = new BasicAuthAccessControl(BasicAuthUtils.extractBasicAuthPrincipals(configuration, PREFIX));
+ }
+
+ @Override
+ public AccessControl create() {
+ return _accessControl;
+ }
+
+ /**
+ * Access Control using header-based basic http authentication
+ */
+ private static class BasicAuthAccessControl implements AccessControl {
+ private final Map<String, BasicAuthPrincipal> _principals;
Review comment:
fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r592873439
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -304,6 +307,7 @@
public static final String CONFIG_OF_CONTROLLER_HTTPS_ENABLED = "enabled";
public static final String CONFIG_OF_CONTROLLER_HTTPS_PORT = "controller.port";
public static final String CONFIG_OF_SEGMENT_UPLOAD_REQUEST_TIMEOUT_MS = "upload.request.timeout.ms";
+ public static final String CONFIG_OF_SEGMENT_UPLOAD_AUTH_TOKEN = "upload.auth.token";
Review comment:
same doc comment
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -353,6 +357,8 @@
public static final String PREFIX_OF_CONFIG_OF_SEGMENT_FETCHER_FACTORY = "segment.fetcher";
public static final String PREFIX_OF_CONFIG_OF_SEGMENT_UPLOADER = "segment.uploader";
public static final String PREFIX_OF_CONFIG_OF_PINOT_CRYPTER = "crypter";
+
+ public static final String CONFIG_OF_TASK_AUTH_TOKEN = "task.auth.token";
Review comment:
same doc comment
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -180,6 +180,16 @@ public BrokerResponse handleRequest(JsonNode request, @Nullable RequesterIdentit
requestStatistics.setRequestId(requestId);
requestStatistics.setRequestArrivalTimeMillis(System.currentTimeMillis());
+ // first-stage access control to prevent unauthenticated requests from using up resources
+ // secondary table-level check comes later
+ boolean hasAccess = _accessControlFactory.create().hasAccess(requesterIdentity, null);
Review comment:
Better to introduce a new default method in AccessControl that takes only requesterIdentity as the argument. Default implementation can return true.
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/fetcher/SegmentFetcherFactory.java
##########
@@ -34,29 +34,40 @@
public class SegmentFetcherFactory {
- private SegmentFetcherFactory() {
- }
+ private final static SegmentFetcherFactory instance = new SegmentFetcherFactory();
static final String SEGMENT_FETCHER_CLASS_KEY_SUFFIX = ".class";
private static final String PROTOCOLS_KEY = "protocols";
+ private static final String AUTH_TOKEN_KEY = "auth.token";
private static final String ENCODED_SUFFIX = ".enc";
private static final Logger LOGGER = LoggerFactory.getLogger(SegmentFetcherFactory.class);
- private static final Map<String, SegmentFetcher> SEGMENT_FETCHER_MAP = new HashMap<>();
- private static final SegmentFetcher DEFAULT_HTTP_SEGMENT_FETCHER = new HttpSegmentFetcher();
- private static final SegmentFetcher DEFAULT_PINOT_FS_SEGMENT_FETCHER = new PinotFSSegmentFetcher();
-
- static {
- PinotConfiguration emptyConfig = new PinotConfiguration();
- DEFAULT_HTTP_SEGMENT_FETCHER.init(emptyConfig);
- DEFAULT_PINOT_FS_SEGMENT_FETCHER.init(emptyConfig);
+
+ private final Map<String, SegmentFetcher> _segmentFetcherMap = new HashMap<>();
+ private final SegmentFetcher _httpSegmentFetcher = new HttpSegmentFetcher();
+ private final SegmentFetcher _pinotFSSegmentFetcher = new PinotFSSegmentFetcher();
+
+ private SegmentFetcherFactory() {
+ // left blank
+ }
+
+ public static SegmentFetcherFactory getInstance() {
+ return instance;
}
/**
* Initializes the segment fetcher factory. This method should only be called once.
*/
public static void init(PinotConfiguration config)
throws Exception {
+ getInstance().initInternal(config);
+ }
+
+ private void initInternal(PinotConfiguration config)
+ throws Exception {
+ _httpSegmentFetcher.init(config); // directly, without sub-namespace
Review comment:
Am I missing this? Are these added to the map anywhere as default fetchers?
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java
##########
@@ -168,16 +172,6 @@ public static URI getRetrieveSchemaURI(String protocol, String host, int port, S
return getURI(protocol, host, port, SCHEMA_PATH + "/" + schemaName);
}
- public static URI getUploadSchemaHttpURI(String host, int port)
Review comment:
@jackjlli can you verify if any of these are used in our internal spark/hadoop jobs?
@apucher you should flag that this is a backward incompatible change for some cases such as this one above.
Another way to do this may be to move/copy this to spi with new methods, and deprecate the class altogether?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java
##########
@@ -0,0 +1,75 @@
+/**
+ * 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.resources;
+
+import io.swagger.annotations.Api;
+import io.swagger.annotations.ApiOperation;
+import io.swagger.annotations.ApiParam;
+import io.swagger.annotations.ApiResponse;
+import io.swagger.annotations.ApiResponses;
+import javax.inject.Inject;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.MediaType;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.controller.api.access.AccessControl;
+import org.apache.pinot.controller.api.access.AccessControlFactory;
+import org.apache.pinot.controller.api.access.AccessType;
+
+
+@Api(tags = "Auth")
+@Path("/")
+public class PinotControllerAuthResource {
+
+ @Inject
+ private AccessControlFactory _accessControlFactory;
+
+ @Context
+ HttpHeaders _httpHeaders;
+
+ @GET
+ @Path("auth/verify")
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Check whether authentication is enabled")
+ @ApiResponses(value = {@ApiResponse(code = 200, message = "Verification result provided"), @ApiResponse(code = 500, message = "Verification error")})
+ public boolean verify(@ApiParam(value = "Table name without type") @QueryParam("tableName") String tableName,
+ @ApiParam(value = "API access type") @QueryParam("accessType") AccessType accessType,
+ @ApiParam(value = "Endpoint URL") @QueryParam("endpointUrl") String endpointUrl) {
+ AccessControl accessControl = _accessControlFactory.create();
+
+ if (StringUtils.isBlank(tableName)) {
+ return accessControl.hasAccess(accessType, _httpHeaders, endpointUrl);
+ }
+
+ return accessControl.hasAccess(tableName, accessType, _httpHeaders, endpointUrl);
+ }
+
+ @GET
+ @Path("auth/info")
Review comment:
what is auth workflow?
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/fetcher/SegmentFetcherFactory.java
##########
@@ -34,29 +34,40 @@
public class SegmentFetcherFactory {
- private SegmentFetcherFactory() {
- }
+ private final static SegmentFetcherFactory instance = new SegmentFetcherFactory();
static final String SEGMENT_FETCHER_CLASS_KEY_SUFFIX = ".class";
private static final String PROTOCOLS_KEY = "protocols";
+ private static final String AUTH_TOKEN_KEY = "auth.token";
private static final String ENCODED_SUFFIX = ".enc";
private static final Logger LOGGER = LoggerFactory.getLogger(SegmentFetcherFactory.class);
- private static final Map<String, SegmentFetcher> SEGMENT_FETCHER_MAP = new HashMap<>();
- private static final SegmentFetcher DEFAULT_HTTP_SEGMENT_FETCHER = new HttpSegmentFetcher();
- private static final SegmentFetcher DEFAULT_PINOT_FS_SEGMENT_FETCHER = new PinotFSSegmentFetcher();
-
- static {
Review comment:
Need to look at the history of this class carefully. The last time we made some changes like this, we had issues where the controller (I think) had an exception, probably during startup. It was when this whole thing was in com.linkedin space, so we don't have the history readily available.
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -235,6 +235,9 @@
"pinot.server.instance.realtime.alloc.offheap.direct";
public static final String PREFIX_OF_CONFIG_OF_PINOT_FS_FACTORY = "pinot.server.storage.factory";
public static final String PREFIX_OF_CONFIG_OF_PINOT_CRYPTER = "pinot.server.crypter";
+
+ public static final String CONFIG_OF_AUTH_TOKEN = "auth.token";
Review comment:
What would be a good user doc of this config? You can add that in the comments here. It will then be a matter to translating it to user docs at some point in time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r592815559
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,106 @@
+/**
+ * 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.Collection;
+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.pinot.core.auth.BasicAuthPrincipal;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * 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>
+ */
+public class BasicAuthAccessControlFactory implements AccessControlFactory {
+ private static final String PREFIX = "controller.admin.access.control.principals";
+
+ private static final String HEADER_AUTHORIZATION = "Authorization";
+
+ private AccessControl _accessControl;
+
+ public void init(PinotConfiguration configuration) {
+ _accessControl = new BasicAuthAccessControl(BasicAuthUtils.extractBasicAuthPrincipals(configuration, PREFIX));
+ }
+
+ @Override
Review comment:
not required, but preferred
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r594751564
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -180,6 +180,16 @@ public BrokerResponse handleRequest(JsonNode request, @Nullable RequesterIdentit
requestStatistics.setRequestId(requestId);
requestStatistics.setRequestArrivalTimeMillis(System.currentTimeMillis());
+ // first-stage access control to prevent unauthenticated requests from using up resources
+ // secondary table-level check comes later
+ boolean hasAccess = _accessControlFactory.create().hasAccess(requesterIdentity);
+ if (!hasAccess) {
+ _brokerMetrics.addMeteredTableValue(null, BrokerMeter.REQUEST_DROPPED_DUE_TO_ACCESS_ERROR, 1);
Review comment:
imo this is the same concern. first-stage access control simply limits resource consumption (i.e. denial of service, leakage of non-table data) for unauthenticated requests. second-stage performs in-depth authorization per table. any rejections are are ACL-related.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#issuecomment-801255621
> I think it should be "AccessType" because AccessControl.hasAccess() takes an AccessType object as its argument. Implementations of AccessControl can't change AccessType.
I agree with this from the controller side, however, `AccessType` is a controller concept only - broker implementations may specify other types of access in an extensible way. One could imagine limiting access to PII data and similar
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on pull request #6613: [WIP] Basic auth controller
Posted by GitBox <gi...@apache.org>.
apucher commented on pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#issuecomment-793185590
> Thanks so much for this PR. As part of this can we also add a new API to enable credential verification ? (eg: from the Cluster Manager UI). I'm thinking of something like this: "http://controller-host:port/verifyauth" where you pass in the token and verify whether its correct.
>
> The other option is to annotate one of the existing APIs eg: GET /tables with "Authenticate" and reuse that. However, a new API might be cleaner. What do you think ?
added `/auth/info` and `/auth/verify` endpoints.
**info** provides a workflow name to be supported from the web UI, including `NONE`, `BASIC`, and `OAUTH2`
**verify** allows to verify access to API endpoints in a generic fashion, or simply to sanity-check auth headers without additional query params
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r595654824
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java
##########
@@ -64,4 +67,36 @@ default boolean hasAccess(String tableName, AccessType accessType, HttpHeaders h
default boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) {
return true;
}
+
+ /**
+ * Return workflow info for authenticating users. Not all workflows may be supported by the pinot UI implementation.
+ *
+ * @return workflow info for user authentication
+ */
+ default AuthWorkflowInfo getAuthWorkflowInfo() {
+ return new AuthWorkflowInfo(WORKFLOW_NONE);
+ }
+
+ /**
+ * Container for authentication workflow info for the Pinot UI. May be extended by implementations.
+ *
+ * Auth workflow info hold any configuration necessary to execute a UI workflow. We currently foresee supporting NONE
+ * (auth disabled), BASIC (basic auth with username and password), and OAUTH2 (token-based workflow via external
+ * issuer)
+ */
+ class AuthWorkflowInfo {
Review comment:
At LinkedIn, we're using both token-based & certificate-based authentication. We don't have a UI for the endpoints and I'm not sure what kind of information is needed to be returned to UI other than the type of authentication. What do you have in mind?
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BasicAuthBatchIntegrationTest.java
##########
@@ -0,0 +1,177 @@
+/**
+ * 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.integration.tests;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.google.common.base.Preconditions;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.util.Collections;
+import java.util.Map;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.IOUtils;
+import org.apache.pinot.controller.helix.core.minion.generator.PinotTaskGenerator;
+import org.apache.pinot.minion.executor.PinotTaskExecutor;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.JsonUtils;
+import org.apache.pinot.tools.BootstrapTableTool;
+import org.testng.Assert;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import static org.apache.pinot.integration.tests.BasicAuthTestUtils.AUTH_HEADER;
+import static org.apache.pinot.integration.tests.BasicAuthTestUtils.AUTH_HEADER_USER;
+import static org.apache.pinot.integration.tests.BasicAuthTestUtils.AUTH_TOKEN;
+
+
+/**
+ * Integration test that provides example of {@link PinotTaskGenerator} and {@link PinotTaskExecutor} and tests simple
+ * minion functionality.
+ */
+public class BasicAuthBatchIntegrationTest extends ClusterTest {
+ private static final String BOOTSTRAP_DATA_DIR = "/examples/batch/baseballStats";
+ private static final String SCHEMA_FILE = "baseballStats_schema.json";
+ private static final String CONFIG_FILE = "baseballStats_offline_table_config.json";
+ private static final String DATA_FILE = "baseballStats_data.csv";
+ private static final String JOB_FILE = "ingestionJobSpec.yaml";
+
+ @BeforeClass
+ public void setUp()
+ throws Exception {
+ startZk();
+ startController();
+ startBroker();
+ startServer();
+ startMinion(Collections.emptyList(), Collections.emptyList());
+ }
+
+ @AfterClass(alwaysRun = true)
+ public void tearDown()
+ throws Exception {
+ stopMinion();
+ stopServer();
+ stopBroker();
+ stopController();
+ stopZk();
+ }
+
+ @Override
+ public Map<String, Object> getDefaultControllerConfiguration() {
+ return BasicAuthTestUtils.addControllerConfiguration(super.getDefaultControllerConfiguration());
+ }
+
+ @Override
+ protected PinotConfiguration getDefaultBrokerConfiguration() {
+ return BasicAuthTestUtils.addBrokerConfiguration(super.getDefaultBrokerConfiguration().toMap());
+ }
+
+ @Override
+ protected PinotConfiguration getDefaultServerConfiguration() {
+ return BasicAuthTestUtils.addServerConfiguration(super.getDefaultServerConfiguration().toMap());
+ }
+
+ @Override
+ protected PinotConfiguration getDefaultMinionConfiguration() {
+ return BasicAuthTestUtils.addMinionConfiguration(super.getDefaultMinionConfiguration().toMap());
+ }
+
+ @Test
+ public void testBrokerNoAuth()
+ throws Exception {
+ JsonNode response =
+ JsonUtils.stringToJsonNode(sendPostRequest("http://localhost:18099/query/sql", "{\"sql\":\"SELECT now()\"}"));
+ Assert.assertFalse(response.has("resultTable"), "must not return result table");
+ Assert.assertTrue(response.get("exceptions").get(0).get("errorCode").asInt() != 0, "must return error code");
+ }
+
+ @Test
+ public void testBroker()
+ throws Exception {
+ JsonNode response = JsonUtils.stringToJsonNode(
+ sendPostRequest("http://localhost:18099/query/sql", "{\"sql\":\"SELECT now()\"}", AUTH_HEADER));
+ Assert.assertEquals(response.get("resultTable").get("dataSchema").get("columnDataTypes").get(0).asText(), "LONG",
+ "must return result with LONG value");
+ Assert.assertTrue(response.get("exceptions").isEmpty(), "must not return exception");
+ }
+
+ @Test
+ public void testControllerGetTables()
+ throws Exception {
+ JsonNode response = JsonUtils.stringToJsonNode(sendGetRequest("http://localhost:18998/tables", AUTH_HEADER));
+ Assert.assertTrue(response.get("tables").isArray(), "must return table array");
+ }
+
+ @Test
+ public void testIngestionBatch()
+ throws Exception {
+ File quickstartTmpDir = new File(FileUtils.getTempDirectory(), String.valueOf(System.currentTimeMillis()));
+ FileUtils.forceDeleteOnExit(quickstartTmpDir);
+
+ File baseDir = new File(quickstartTmpDir, "baseballStats");
+ File dataDir = new File(baseDir, "rawdata");
+ File schemaFile = new File(baseDir, SCHEMA_FILE);
+ File configFile = new File(baseDir, CONFIG_FILE);
+ File dataFile = new File(dataDir, DATA_FILE);
+ File jobFile = new File(baseDir, JOB_FILE);
+ Preconditions.checkState(dataDir.mkdirs());
+
+ FileUtils.copyURLToFile(getClass().getResource(BOOTSTRAP_DATA_DIR + "/" + SCHEMA_FILE), schemaFile);
+ FileUtils.copyURLToFile(getClass().getResource(BOOTSTRAP_DATA_DIR + "/" + CONFIG_FILE), configFile);
+ FileUtils.copyURLToFile(getClass().getResource(BOOTSTRAP_DATA_DIR + "/rawdata/" + DATA_FILE), dataFile);
+ FileUtils.copyURLToFile(getClass().getResource(BOOTSTRAP_DATA_DIR + "/" + JOB_FILE), jobFile);
+
+ // patch ingestion job
+ String jobFileContents = IOUtils.toString(new FileInputStream(jobFile));
+ IOUtils.write(jobFileContents.replaceAll("9000", String.valueOf(DEFAULT_CONTROLLER_PORT)),
+ new FileOutputStream(jobFile));
+
+ new BootstrapTableTool("http", "localhost", DEFAULT_CONTROLLER_PORT, baseDir.getAbsolutePath(), AUTH_TOKEN)
+ .execute();
+
+ Thread.sleep(5000);
+
+ // admin with full access
+ JsonNode response = JsonUtils.stringToJsonNode(
+ sendPostRequest("http://localhost:18099/query/sql", "{\"sql\":\"SELECT count(*) FROM baseballStats\"}",
+ AUTH_HEADER));
+ Assert.assertEquals(response.get("resultTable").get("dataSchema").get("columnDataTypes").get(0).asText(), "LONG",
+ "must return result with LONG value");
+ Assert.assertEquals(response.get("resultTable").get("dataSchema").get("columnNames").get(0).asText(), "count(*)",
+ "must return column name 'count(*)");
+ Assert.assertEquals(response.get("resultTable").get("rows").get(0).get(0).asInt(), 97889,
+ "must return row count 97889");
+ Assert.assertTrue(response.get("exceptions").isEmpty(), "must not return exception");
+
+ // user with valid auth but no table access
+ JsonNode responseUser = JsonUtils.stringToJsonNode(
+ sendPostRequest("http://localhost:18099/query/sql", "{\"sql\":\"SELECT count(*) FROM baseballStats\"}",
+ AUTH_HEADER_USER));
+ Assert.assertFalse(responseUser.has("resultTable"), "must not return result table");
+ Assert.assertTrue(responseUser.get("exceptions").get(0).get("errorCode").asInt() != 0, "must return error code");
+ }
+
+// // TODO this endpoint should be protected once UI supports auth
+// @Test
+// public void testControllerGetTablesNoAuth()
+// throws Exception {
+// System.out.println(sendGetRequest("http://localhost:18998/tables"));
+// }
Review comment:
I don't understand the TODO here!
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java
##########
@@ -0,0 +1,55 @@
+/**
+ * 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.core.auth;
+
+import java.util.Set;
+
+
+/**
+ * Container object for basic auth principal
+ */
+public class BasicAuthPrincipal {
+ private final String _name;
+ private final String _token;
+ private final Set<String> _tables;
+ private final Set<String> _permissions;
Review comment:
I think it should be "AccessType" because AccessControl.hasAccess() takes an AccessType object as its argument. Implementations of AccessControl can't change AccessType.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java
##########
@@ -0,0 +1,90 @@
+/**
+ * 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.resources;
+
+import io.swagger.annotations.Api;
+import io.swagger.annotations.ApiOperation;
+import io.swagger.annotations.ApiParam;
+import io.swagger.annotations.ApiResponse;
+import io.swagger.annotations.ApiResponses;
+import javax.inject.Inject;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.MediaType;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.controller.api.access.AccessControl;
+import org.apache.pinot.controller.api.access.AccessControlFactory;
+import org.apache.pinot.controller.api.access.AccessType;
+
+
+@Api(tags = "Auth")
+@Path("/")
+public class PinotControllerAuthResource {
+
+ @Inject
+ private AccessControlFactory _accessControlFactory;
+
+ @Context
+ HttpHeaders _httpHeaders;
+
+ /**
+ * Verify a token is both authenticated and authorized to perform an operation.
+ *
+ * @param tableName table name (optional)
+ * @param accessType access type (optional)
+ * @param endpointUrl endpoint url (optional)
+ *
+ * @return {@code true} if authenticated and authorized, {@code false} otherwise
+ */
+ @GET
+ @Path("auth/verify")
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Check whether authentication is enabled")
+ @ApiResponses(value = {@ApiResponse(code = 200, message = "Verification result provided"), @ApiResponse(code = 500, message = "Verification error")})
+ public boolean verify(@ApiParam(value = "Table name without type") @QueryParam("tableName") String tableName,
+ @ApiParam(value = "API access type") @QueryParam("accessType") AccessType accessType,
+ @ApiParam(value = "Endpoint URL") @QueryParam("endpointUrl") String endpointUrl) {
+ AccessControl accessControl = _accessControlFactory.create();
+
+ if (StringUtils.isBlank(tableName)) {
+ return accessControl.hasAccess(accessType, _httpHeaders, endpointUrl);
+ }
+
+ return accessControl.hasAccess(tableName, accessType, _httpHeaders, endpointUrl);
+ }
+
+ /**
+ * Provide the auth workflow configuration for the Pinot UI to perform user authentication. Currently supports NONE
+ * (no auth), BASIC (basic auth with username and password), and OAUTH2 (token-based via external issuer)
Review comment:
AFAIK currently there's no OAuth2 support. If that's correct, please update the comment.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java
##########
@@ -26,6 +26,9 @@
@InterfaceAudience.Public
@InterfaceStability.Stable
public interface AccessControl {
+ String WORKFLOW_NONE = "NONE";
+ String WORKFLOW_BASIC = "BASIC";
+ String WORKFLOW_OAUTH2 = "OAUTH2";
Review comment:
I guess we should stick to the ones that are already available in the code - ie none and basic. Whenever we add new implementation, we can add them here.
Also enum is a better choice than string.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r596213201
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java
##########
@@ -0,0 +1,90 @@
+/**
+ * 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.resources;
+
+import io.swagger.annotations.Api;
+import io.swagger.annotations.ApiOperation;
+import io.swagger.annotations.ApiParam;
+import io.swagger.annotations.ApiResponse;
+import io.swagger.annotations.ApiResponses;
+import javax.inject.Inject;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.MediaType;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.controller.api.access.AccessControl;
+import org.apache.pinot.controller.api.access.AccessControlFactory;
+import org.apache.pinot.controller.api.access.AccessType;
+
+
+@Api(tags = "Auth")
+@Path("/")
+public class PinotControllerAuthResource {
+
+ @Inject
+ private AccessControlFactory _accessControlFactory;
+
+ @Context
+ HttpHeaders _httpHeaders;
+
+ /**
+ * Verify a token is both authenticated and authorized to perform an operation.
+ *
+ * @param tableName table name (optional)
+ * @param accessType access type (optional)
+ * @param endpointUrl endpoint url (optional)
+ *
+ * @return {@code true} if authenticated and authorized, {@code false} otherwise
+ */
+ @GET
+ @Path("auth/verify")
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Check whether authentication is enabled")
+ @ApiResponses(value = {@ApiResponse(code = 200, message = "Verification result provided"), @ApiResponse(code = 500, message = "Verification error")})
+ public boolean verify(@ApiParam(value = "Table name without type") @QueryParam("tableName") String tableName,
+ @ApiParam(value = "API access type") @QueryParam("accessType") AccessType accessType,
+ @ApiParam(value = "Endpoint URL") @QueryParam("endpointUrl") String endpointUrl) {
+ AccessControl accessControl = _accessControlFactory.create();
+
+ if (StringUtils.isBlank(tableName)) {
+ return accessControl.hasAccess(accessType, _httpHeaders, endpointUrl);
+ }
+
+ return accessControl.hasAccess(tableName, accessType, _httpHeaders, endpointUrl);
+ }
+
+ /**
+ * Provide the auth workflow configuration for the Pinot UI to perform user authentication. Currently supports NONE
+ * (no auth), BASIC (basic auth with username and password), and OAUTH2 (token-based via external issuer)
Review comment:
updated
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r596217291
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java
##########
@@ -64,4 +67,36 @@ default boolean hasAccess(String tableName, AccessType accessType, HttpHeaders h
default boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) {
return true;
}
+
+ /**
+ * Return workflow info for authenticating users. Not all workflows may be supported by the pinot UI implementation.
+ *
+ * @return workflow info for user authentication
+ */
+ default AuthWorkflowInfo getAuthWorkflowInfo() {
+ return new AuthWorkflowInfo(WORKFLOW_NONE);
+ }
+
+ /**
+ * Container for authentication workflow info for the Pinot UI. May be extended by implementations.
+ *
+ * Auth workflow info hold any configuration necessary to execute a UI workflow. We currently foresee supporting NONE
+ * (auth disabled), BASIC (basic auth with username and password), and OAUTH2 (token-based workflow via external
+ * issuer)
+ */
+ class AuthWorkflowInfo {
Review comment:
afaik LinkedIn doesn't authenticate web UI calls at all (it's not yet supported by the controller UI). cert-based auth takes place in restli's tls layer, while broker queries are further authenticated with tokens at the request layer.
this endpoint enables us to configure different UI auth workflows depending on the scenario. at minimum we need to support NONE to maintain backwards compatibility and BASIC to support fully authenticated clusters with this PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r601714860
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/fetcher/SegmentFetcherFactory.java
##########
@@ -34,29 +34,40 @@
public class SegmentFetcherFactory {
- private SegmentFetcherFactory() {
- }
+ private final static SegmentFetcherFactory instance = new SegmentFetcherFactory();
Review comment:
```suggestion
private final static SegmentFetcherFactory INSTANCE = new SegmentFetcherFactory();
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r602010559
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java
##########
@@ -64,4 +67,36 @@ default boolean hasAccess(String tableName, AccessType accessType, HttpHeaders h
default boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) {
return true;
}
+
+ /**
+ * Return workflow info for authenticating users. Not all workflows may be supported by the pinot UI implementation.
+ *
+ * @return workflow info for user authentication
+ */
+ default AuthWorkflowInfo getAuthWorkflowInfo() {
+ return new AuthWorkflowInfo(WORKFLOW_NONE);
+ }
+
+ /**
+ * Container for authentication workflow info for the Pinot UI. May be extended by implementations.
+ *
+ * Auth workflow info hold any configuration necessary to execute a UI workflow. We currently foresee supporting NONE
+ * (auth disabled), BASIC (basic auth with username and password), and OAUTH2 (token-based workflow via external
+ * issuer)
+ */
+ class AuthWorkflowInfo {
Review comment:
Yes sounds good.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r593502204
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java
##########
@@ -168,16 +172,6 @@ public static URI getRetrieveSchemaURI(String protocol, String host, int port, S
return getURI(protocol, host, port, SCHEMA_PATH + "/" + schemaName);
}
- public static URI getUploadSchemaHttpURI(String host, int port)
Review comment:
I've returned the class to its original public interface and flagged any methods as deprecated instead. This should ease the transition and retain full backwards-compatibility
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r597144194
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java
##########
@@ -64,4 +67,36 @@ default boolean hasAccess(String tableName, AccessType accessType, HttpHeaders h
default boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) {
return true;
}
+
+ /**
+ * Return workflow info for authenticating users. Not all workflows may be supported by the pinot UI implementation.
+ *
+ * @return workflow info for user authentication
+ */
+ default AuthWorkflowInfo getAuthWorkflowInfo() {
+ return new AuthWorkflowInfo(WORKFLOW_NONE);
+ }
+
+ /**
+ * Container for authentication workflow info for the Pinot UI. May be extended by implementations.
+ *
+ * Auth workflow info hold any configuration necessary to execute a UI workflow. We currently foresee supporting NONE
+ * (auth disabled), BASIC (basic auth with username and password), and OAUTH2 (token-based workflow via external
+ * issuer)
+ */
+ class AuthWorkflowInfo {
Review comment:
certificate-based auth usually takes place on the lower layer (TLS/SSL).
This endpoint purely serves to configure UI auth workflows as we broaden support:
- disable auth workflows in the UI if pinot is set up without auth
- enable a user/password workflow for basic auth
- provide issuerUrl (with optional info) for oauth2 workflows
- provide secrets/redirects for Azure AD, kerberos, ... (as it comes up)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r597045061
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java
##########
@@ -64,4 +67,36 @@ default boolean hasAccess(String tableName, AccessType accessType, HttpHeaders h
default boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) {
return true;
}
+
+ /**
+ * Return workflow info for authenticating users. Not all workflows may be supported by the pinot UI implementation.
+ *
+ * @return workflow info for user authentication
+ */
+ default AuthWorkflowInfo getAuthWorkflowInfo() {
+ return new AuthWorkflowInfo(WORKFLOW_NONE);
+ }
+
+ /**
+ * Container for authentication workflow info for the Pinot UI. May be extended by implementations.
+ *
+ * Auth workflow info hold any configuration necessary to execute a UI workflow. We currently foresee supporting NONE
+ * (auth disabled), BASIC (basic auth with username and password), and OAUTH2 (token-based workflow via external
+ * issuer)
+ */
+ class AuthWorkflowInfo {
Review comment:
If there's only NONE and BASIC, AuthWorkflowInfo only returns the type of authentication. I still don't understand what information other than auth-type is going to be returned to UI for token or certificate based authentication. Could you elaborate on that?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r596221839
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java
##########
@@ -0,0 +1,55 @@
+/**
+ * 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.core.auth;
+
+import java.util.Set;
+
+
+/**
+ * Container object for basic auth principal
+ */
+public class BasicAuthPrincipal {
+ private final String _name;
+ private final String _token;
+ private final Set<String> _tables;
+ private final Set<String> _permissions;
Review comment:
(Double-posting here for context) I agree with this from the controller side, however, AccessType is a controller concept only - broker implementations may specify other types of access in an extensible way. One could imagine limiting access to PII data and similar
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#issuecomment-800623814
> > > > Do you need to change LLCSegmentCompletionHandlers to pay attention to the auth token?
> > >
> > >
> > > Probably. I'd appreciate your input on where this actually talks to controller APIs. I just noticed that this whole set of endpoints was excluded from my checklist (via swagger)
> >
> >
> > These endpoints should not appear in Swagger because they are used by pinot-server to commit realtime segments using the segment commit protocol. They are not meant for administrative or user actions. These are completely pinot internal operations.
> > The client side of this call is in ServerSegmentProtocolCompletionHandler.
> > If you can make these changes in another PR, that will help reviewing a bit. If I miss something and it causes realtime segments not to complete segments, that will be really bad.
>
> `ServerSegmentProtocolCompletionHandler` already has an `_authToken`. Imo the most important thing right now is getting out integration tests - batch is done, realtime is in the works.
Working the entire realtime flow inside an integration test is definitely useful. Where I am afraid things ay break is if servers are running older version and controllers newer, or vice versa. Realtime segment upload failure will block realtime consumption. If you can ascertain that as long as auth tokens are not configured anywhere the behavior is the same that would be awesome.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#issuecomment-788191238
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=h1) Report
> Merging [#6613](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=desc) (92f4a08) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.57%`.
> The diff coverage is `45.95%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6613 +/- ##
===========================================
- Coverage 66.44% 43.87% -22.58%
===========================================
Files 1075 1394 +319
Lines 54773 67628 +12855
Branches 8168 9799 +1631
===========================================
- Hits 36396 29673 -6723
- Misses 15700 35475 +19775
+ Partials 2677 2480 -197
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `43.87% <45.95%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...org/apache/pinot/broker/queryquota/HitCounter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IaXRDb3VudGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...che/pinot/broker/queryquota/MaxHitRateTracker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9NYXhIaXRSYXRlVHJhY2tlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...ache/pinot/broker/queryquota/QueryQuotaEntity.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9RdWVyeVF1b3RhRW50aXR5LmphdmE=) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1RpbWVTZWdtZW50UHJ1bmVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/interval/Interval.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...r/routing/segmentpruner/interval/IntervalTree.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsVHJlZS5qYXZh) | `0.00% <0.00%> (ø)` | |
| ... and [1425 more](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=footer). Last update [36d6db6...92f4a08](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #6613: [WIP] Basic auth controller
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#issuecomment-788191238
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=h1) Report
> Merging [#6613](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=desc) (cd60ca0) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `0.69%`.
> The diff coverage is `62.06%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6613 +/- ##
==========================================
- Coverage 66.44% 65.74% -0.70%
==========================================
Files 1075 1366 +291
Lines 54773 66627 +11854
Branches 8168 9711 +1543
==========================================
+ Hits 36396 43807 +7411
- Misses 15700 19692 +3992
- Partials 2677 3128 +451
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `65.74% <62.06%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <0.00%> (-79.32%)` | :arrow_down: |
| [...pinot/broker/api/resources/PinotClientRequest.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdENsaWVudFJlcXVlc3QuamF2YQ==) | `0.00% <0.00%> (-27.28%)` | :arrow_down: |
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `33.96% <0.00%> (-32.71%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `66.66% <ø> (+9.52%)` | :arrow_up: |
| [...n/java/org/apache/pinot/client/BrokerResponse.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Jyb2tlclJlc3BvbnNlLmphdmE=) | `100.00% <ø> (ø)` | |
| [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `35.55% <ø> (-13.29%)` | :arrow_down: |
| [...org/apache/pinot/client/DynamicBrokerSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0R5bmFtaWNCcm9rZXJTZWxlY3Rvci5qYXZh) | `82.85% <ø> (+10.12%)` | :arrow_up: |
| [...n/java/org/apache/pinot/client/ExecutionStats.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0V4ZWN1dGlvblN0YXRzLmphdmE=) | `68.88% <ø> (ø)` | |
| ... and [1253 more](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=footer). Last update [1dbdd67...cd60ca0](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#issuecomment-807387346
> lgtm other than a very minor comment. I have resolved all the comments of mine. Can you also work with @sajjad-moradi to resolve his comments as well?
>
> thanks for your patience.
Awesome. Thanks, Subbu. Yeah, so many changes with all the glue and tooling code.
Addressed your remaining comments, and waiting for @sajjad-moradi to circle back.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r601728182
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BasicAuthBatchIntegrationTest.java
##########
@@ -0,0 +1,177 @@
+/**
+ * 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.integration.tests;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.google.common.base.Preconditions;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.util.Collections;
+import java.util.Map;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.IOUtils;
+import org.apache.pinot.controller.helix.core.minion.generator.PinotTaskGenerator;
+import org.apache.pinot.minion.executor.PinotTaskExecutor;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.JsonUtils;
+import org.apache.pinot.tools.BootstrapTableTool;
+import org.testng.Assert;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import static org.apache.pinot.integration.tests.BasicAuthTestUtils.AUTH_HEADER;
+import static org.apache.pinot.integration.tests.BasicAuthTestUtils.AUTH_HEADER_USER;
+import static org.apache.pinot.integration.tests.BasicAuthTestUtils.AUTH_TOKEN;
+
+
+/**
+ * Integration test that provides example of {@link PinotTaskGenerator} and {@link PinotTaskExecutor} and tests simple
+ * minion functionality.
+ */
+public class BasicAuthBatchIntegrationTest extends ClusterTest {
+ private static final String BOOTSTRAP_DATA_DIR = "/examples/batch/baseballStats";
+ private static final String SCHEMA_FILE = "baseballStats_schema.json";
+ private static final String CONFIG_FILE = "baseballStats_offline_table_config.json";
+ private static final String DATA_FILE = "baseballStats_data.csv";
+ private static final String JOB_FILE = "ingestionJobSpec.yaml";
+
+ @BeforeClass
+ public void setUp()
+ throws Exception {
+ startZk();
+ startController();
+ startBroker();
+ startServer();
+ startMinion(Collections.emptyList(), Collections.emptyList());
+ }
+
+ @AfterClass(alwaysRun = true)
+ public void tearDown()
+ throws Exception {
+ stopMinion();
+ stopServer();
+ stopBroker();
+ stopController();
+ stopZk();
+ }
+
+ @Override
+ public Map<String, Object> getDefaultControllerConfiguration() {
+ return BasicAuthTestUtils.addControllerConfiguration(super.getDefaultControllerConfiguration());
+ }
+
+ @Override
+ protected PinotConfiguration getDefaultBrokerConfiguration() {
+ return BasicAuthTestUtils.addBrokerConfiguration(super.getDefaultBrokerConfiguration().toMap());
+ }
+
+ @Override
+ protected PinotConfiguration getDefaultServerConfiguration() {
+ return BasicAuthTestUtils.addServerConfiguration(super.getDefaultServerConfiguration().toMap());
+ }
+
+ @Override
+ protected PinotConfiguration getDefaultMinionConfiguration() {
+ return BasicAuthTestUtils.addMinionConfiguration(super.getDefaultMinionConfiguration().toMap());
+ }
+
+ @Test
+ public void testBrokerNoAuth()
+ throws Exception {
+ JsonNode response =
+ JsonUtils.stringToJsonNode(sendPostRequest("http://localhost:18099/query/sql", "{\"sql\":\"SELECT now()\"}"));
+ Assert.assertFalse(response.has("resultTable"), "must not return result table");
+ Assert.assertTrue(response.get("exceptions").get(0).get("errorCode").asInt() != 0, "must return error code");
+ }
+
+ @Test
+ public void testBroker()
+ throws Exception {
+ JsonNode response = JsonUtils.stringToJsonNode(
+ sendPostRequest("http://localhost:18099/query/sql", "{\"sql\":\"SELECT now()\"}", AUTH_HEADER));
+ Assert.assertEquals(response.get("resultTable").get("dataSchema").get("columnDataTypes").get(0).asText(), "LONG",
+ "must return result with LONG value");
+ Assert.assertTrue(response.get("exceptions").isEmpty(), "must not return exception");
+ }
+
+ @Test
+ public void testControllerGetTables()
+ throws Exception {
+ JsonNode response = JsonUtils.stringToJsonNode(sendGetRequest("http://localhost:18998/tables", AUTH_HEADER));
+ Assert.assertTrue(response.get("tables").isArray(), "must return table array");
+ }
+
+ @Test
+ public void testIngestionBatch()
+ throws Exception {
+ File quickstartTmpDir = new File(FileUtils.getTempDirectory(), String.valueOf(System.currentTimeMillis()));
+ FileUtils.forceDeleteOnExit(quickstartTmpDir);
+
+ File baseDir = new File(quickstartTmpDir, "baseballStats");
+ File dataDir = new File(baseDir, "rawdata");
+ File schemaFile = new File(baseDir, SCHEMA_FILE);
+ File configFile = new File(baseDir, CONFIG_FILE);
+ File dataFile = new File(dataDir, DATA_FILE);
+ File jobFile = new File(baseDir, JOB_FILE);
+ Preconditions.checkState(dataDir.mkdirs());
+
+ FileUtils.copyURLToFile(getClass().getResource(BOOTSTRAP_DATA_DIR + "/" + SCHEMA_FILE), schemaFile);
+ FileUtils.copyURLToFile(getClass().getResource(BOOTSTRAP_DATA_DIR + "/" + CONFIG_FILE), configFile);
+ FileUtils.copyURLToFile(getClass().getResource(BOOTSTRAP_DATA_DIR + "/rawdata/" + DATA_FILE), dataFile);
+ FileUtils.copyURLToFile(getClass().getResource(BOOTSTRAP_DATA_DIR + "/" + JOB_FILE), jobFile);
+
+ // patch ingestion job
+ String jobFileContents = IOUtils.toString(new FileInputStream(jobFile));
+ IOUtils.write(jobFileContents.replaceAll("9000", String.valueOf(DEFAULT_CONTROLLER_PORT)),
+ new FileOutputStream(jobFile));
+
+ new BootstrapTableTool("http", "localhost", DEFAULT_CONTROLLER_PORT, baseDir.getAbsolutePath(), AUTH_TOKEN)
+ .execute();
+
+ Thread.sleep(5000);
+
+ // admin with full access
+ JsonNode response = JsonUtils.stringToJsonNode(
+ sendPostRequest("http://localhost:18099/query/sql", "{\"sql\":\"SELECT count(*) FROM baseballStats\"}",
+ AUTH_HEADER));
+ Assert.assertEquals(response.get("resultTable").get("dataSchema").get("columnDataTypes").get(0).asText(), "LONG",
+ "must return result with LONG value");
+ Assert.assertEquals(response.get("resultTable").get("dataSchema").get("columnNames").get(0).asText(), "count(*)",
+ "must return column name 'count(*)");
+ Assert.assertEquals(response.get("resultTable").get("rows").get(0).get(0).asInt(), 97889,
+ "must return row count 97889");
+ Assert.assertTrue(response.get("exceptions").isEmpty(), "must not return exception");
+
+ // user with valid auth but no table access
+ JsonNode responseUser = JsonUtils.stringToJsonNode(
+ sendPostRequest("http://localhost:18099/query/sql", "{\"sql\":\"SELECT count(*) FROM baseballStats\"}",
+ AUTH_HEADER_USER));
+ Assert.assertFalse(responseUser.has("resultTable"), "must not return result table");
+ Assert.assertTrue(responseUser.get("exceptions").get(0).get("errorCode").asInt() != 0, "must return error code");
+ }
+
+// // TODO this endpoint should be protected once UI supports auth
+// @Test
+// public void testControllerGetTablesNoAuth()
+// throws Exception {
+// System.out.println(sendGetRequest("http://localhost:18998/tables"));
+// }
Review comment:
done
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#issuecomment-788191238
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=h1) Report
> Merging [#6613](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=desc) (618de28) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `0.64%`.
> The diff coverage is `61.80%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6613 +/- ##
==========================================
- Coverage 66.44% 65.80% -0.65%
==========================================
Files 1075 1396 +321
Lines 54773 67686 +12913
Branches 8168 9808 +1640
==========================================
+ Hits 36396 44539 +8143
- Misses 15700 19968 +4268
- Partials 2677 3179 +502
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `65.80% <61.80%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <0.00%> (-79.32%)` | :arrow_down: |
| [...pinot/broker/api/resources/PinotClientRequest.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdENsaWVudFJlcXVlc3QuamF2YQ==) | `0.00% <0.00%> (-27.28%)` | :arrow_down: |
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `85.71% <ø> (-14.29%)` | :arrow_down: |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `33.96% <0.00%> (-32.71%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `66.66% <ø> (+9.52%)` | :arrow_up: |
| [...n/java/org/apache/pinot/client/BrokerResponse.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Jyb2tlclJlc3BvbnNlLmphdmE=) | `100.00% <ø> (ø)` | |
| [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `35.55% <ø> (-13.29%)` | :arrow_down: |
| [...ava/org/apache/pinot/client/ConnectionFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb25GYWN0b3J5LmphdmE=) | `23.07% <ø> (-22.38%)` | :arrow_down: |
| [...org/apache/pinot/client/DynamicBrokerSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0R5bmFtaWNCcm9rZXJTZWxlY3Rvci5qYXZh) | `82.85% <ø> (+10.12%)` | :arrow_up: |
| ... and [1284 more](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=footer). Last update [621ceb0...618de28](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r592814764
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,106 @@
+/**
+ * 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.Collection;
+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.pinot.core.auth.BasicAuthPrincipal;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * 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>
+ */
+public class BasicAuthAccessControlFactory implements AccessControlFactory {
+ private static final String PREFIX = "controller.admin.access.control.principals";
+
+ private static final String HEADER_AUTHORIZATION = "Authorization";
+
+ private AccessControl _accessControl;
+
+ public void init(PinotConfiguration configuration) {
+ _accessControl = new BasicAuthAccessControl(BasicAuthUtils.extractBasicAuthPrincipals(configuration, PREFIX));
+ }
+
+ @Override
+ public AccessControl create() {
+ return _accessControl;
+ }
+
+ /**
+ * Access Control using header-based basic http authentication
+ */
+ private static class BasicAuthAccessControl implements AccessControl {
+ private final Map<String, BasicAuthPrincipal> _principals;
+
+ public BasicAuthAccessControl(Collection<BasicAuthPrincipal> principals) {
+ this._principals = principals.stream().collect(Collectors.toMap(BasicAuthPrincipal::getToken, p -> p));
+ }
+
+ @Override
+ public boolean hasDataAccess(HttpHeaders httpHeaders, String tableName) {
+ return getPrincipal(httpHeaders).filter(p -> p.hasTable(tableName)).isPresent();
+ }
+
+ @Override
+ public boolean hasAccess(String tableName, AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) {
Review comment:
err ... `hasAccess()`, etc. are defined in the `AccessControl` interface and `@Override` ensures a method signature match
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#issuecomment-799842929
> Do you need to change LLCSegmentCompletionHandlers to pay attention to the auth token?
Probably. I'd appreciate your input on where this actually talks to controller APIs. I just noticed that this whole set of endpoints was excluded from my checklist (via swagger)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] icefury71 commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
icefury71 commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r592832979
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/segment/generation/SegmentGenerationUtils.java
##########
@@ -87,7 +94,12 @@ public static Schema getSchema(String schemaURIString) {
}
}
+ @Deprecated
Review comment:
Sounds good.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#issuecomment-799867494
> > Do you need to change LLCSegmentCompletionHandlers to pay attention to the auth token?
>
> Probably. I'd appreciate your input on where this actually talks to controller APIs. I just noticed that this whole set of endpoints was excluded from my checklist (via swagger)
These endpoints should not appear in Swagger because they are used by pinot-server to commit realtime segments using the segment commit protocol. They are not meant for administrative or user actions. These are completely pinot internal operations.
The client side of this call is in ServerSegmentProtocolCompletionHandler.
If you can make these changes in another PR, that will help reviewing a bit. If I miss something and it causes realtime segments not to complete segments, that will be really bad.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r592808133
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java
##########
@@ -0,0 +1,75 @@
+/**
+ * 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.resources;
+
+import io.swagger.annotations.Api;
+import io.swagger.annotations.ApiOperation;
+import io.swagger.annotations.ApiParam;
+import io.swagger.annotations.ApiResponse;
+import io.swagger.annotations.ApiResponses;
+import javax.inject.Inject;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.MediaType;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.controller.api.access.AccessControl;
+import org.apache.pinot.controller.api.access.AccessControlFactory;
+import org.apache.pinot.controller.api.access.AccessType;
+
+
+@Api(tags = "Auth")
+@Path("/")
+public class PinotControllerAuthResource {
+
+ @Inject
+ private AccessControlFactory _accessControlFactory;
+
+ @Context
+ HttpHeaders _httpHeaders;
+
+ @GET
+ @Path("auth/verify")
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Check whether authentication is enabled")
+ @ApiResponses(value = {@ApiResponse(code = 200, message = "Verification result provided"), @ApiResponse(code = 500, message = "Verification error")})
Review comment:
`/auth/verify` does not *require* authentication itself, i.e. it can't be `Forbidden`. it does verify a provided token and return 200 with a body (true/false) or a 500 if there's an error processing the token
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6613: Basic Auth for pinot-controller
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r593503414
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/fetcher/SegmentFetcherFactory.java
##########
@@ -34,29 +34,40 @@
public class SegmentFetcherFactory {
- private SegmentFetcherFactory() {
- }
+ private final static SegmentFetcherFactory instance = new SegmentFetcherFactory();
static final String SEGMENT_FETCHER_CLASS_KEY_SUFFIX = ".class";
private static final String PROTOCOLS_KEY = "protocols";
+ private static final String AUTH_TOKEN_KEY = "auth.token";
private static final String ENCODED_SUFFIX = ".enc";
private static final Logger LOGGER = LoggerFactory.getLogger(SegmentFetcherFactory.class);
- private static final Map<String, SegmentFetcher> SEGMENT_FETCHER_MAP = new HashMap<>();
- private static final SegmentFetcher DEFAULT_HTTP_SEGMENT_FETCHER = new HttpSegmentFetcher();
- private static final SegmentFetcher DEFAULT_PINOT_FS_SEGMENT_FETCHER = new PinotFSSegmentFetcher();
-
- static {
- PinotConfiguration emptyConfig = new PinotConfiguration();
- DEFAULT_HTTP_SEGMENT_FETCHER.init(emptyConfig);
- DEFAULT_PINOT_FS_SEGMENT_FETCHER.init(emptyConfig);
+
+ private final Map<String, SegmentFetcher> _segmentFetcherMap = new HashMap<>();
+ private final SegmentFetcher _httpSegmentFetcher = new HttpSegmentFetcher();
+ private final SegmentFetcher _pinotFSSegmentFetcher = new PinotFSSegmentFetcher();
+
+ private SegmentFetcherFactory() {
+ // left blank
+ }
+
+ public static SegmentFetcherFactory getInstance() {
+ return instance;
}
/**
* Initializes the segment fetcher factory. This method should only be called once.
*/
public static void init(PinotConfiguration config)
throws Exception {
+ getInstance().initInternal(config);
+ }
+
+ private void initInternal(PinotConfiguration config)
+ throws Exception {
+ _httpSegmentFetcher.init(config); // directly, without sub-namespace
Review comment:
these replace the static fetcher instances from the comment above
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io commented on pull request #6613: [WIP] Basic auth controller
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#issuecomment-788191238
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=h1) Report
> Merging [#6613](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=desc) (58d047a) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `0.92%`.
> The diff coverage is `62.02%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6613 +/- ##
==========================================
- Coverage 66.44% 65.52% -0.93%
==========================================
Files 1075 1356 +281
Lines 54773 66685 +11912
Branches 8168 9720 +1552
==========================================
+ Hits 36396 43696 +7300
- Misses 15700 19869 +4169
- Partials 2677 3120 +443
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `65.52% <62.02%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <0.00%> (-79.32%)` | :arrow_down: |
| [...pinot/broker/api/resources/PinotClientRequest.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdENsaWVudFJlcXVlc3QuamF2YQ==) | `0.00% <0.00%> (-27.28%)` | :arrow_down: |
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `33.96% <0.00%> (-32.71%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `66.66% <ø> (+9.52%)` | :arrow_up: |
| [...n/java/org/apache/pinot/client/BrokerResponse.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Jyb2tlclJlc3BvbnNlLmphdmE=) | `100.00% <ø> (ø)` | |
| [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `35.55% <ø> (-13.29%)` | :arrow_down: |
| [...org/apache/pinot/client/DynamicBrokerSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0R5bmFtaWNCcm9rZXJTZWxlY3Rvci5qYXZh) | `82.85% <ø> (+10.12%)` | :arrow_up: |
| [...n/java/org/apache/pinot/client/ExecutionStats.java](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0V4ZWN1dGlvblN0YXRzLmphdmE=) | `68.88% <ø> (ø)` | |
| ... and [1220 more](https://codecov.io/gh/apache/incubator-pinot/pull/6613/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=footer). Last update [3adbbe4...58d047a](https://codecov.io/gh/apache/incubator-pinot/pull/6613?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org