Copilot commented on code in PR #5119:
URL: https://github.com/apache/polaris/pull/5119#discussion_r3622586949
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java:
##########
@@ -90,21 +91,22 @@ public class DefaultAuthenticator implements Authenticator {
@Inject PolarisMetaStoreManager metaStoreManager;
@Inject CallContext callContext;
@Inject PolarisDiagnostics diagnostics;
+ @Inject AuthenticationRealmConfiguration authConfig;
@Override
public PolarisPrincipal authenticate(SecurityIdentity identity) {
PolarisCredential credentials = extractPolarisCredential(identity);
LOGGER.debug("Resolving principal for credentials: {}", credentials);
- PrincipalEntity principalEntity = resolvePrincipalEntity(credentials);
- PrincipalRoleSelection principalRoles = resolvePrincipalRoles(credentials,
principalEntity);
- Map<String, Object> principalAttributes =
- resolvePrincipalAttributes(identity, principalEntity,
principalRoles.allRolesRequested());
- PolarisPrincipal polarisPrincipal =
- PolarisPrincipal.of(principalEntity.getName(), principalAttributes,
principalRoles.roles());
+ var entity = resolvePrincipalEntity(credentials);
+ var roleSelection = resolvePrincipalRoles(credentials, entity);
+ var attributes = resolvePrincipalAttributes(identity, entity,
roleSelection);
- LOGGER.debug("Resolved principal: {}", polarisPrincipal);
+ var principalName = entity != null ? entity.getName() :
credentials.getPrincipalName();
+ var polarisPrincipal = PolarisPrincipal.of(principalName, attributes,
roleSelection.roles());
+
+ LOGGER.debug("Resolved internal principal: {}", polarisPrincipal);
return polarisPrincipal;
Review Comment:
The debug log message always says "Resolved internal principal", but this
authenticator can now create external principals (no metastore-backed entity).
This log line will be misleading for external principal mode.
##########
tools/testcontainers/opa/src/main/java/org/apache/polaris/test/opa/OpaContainer.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.polaris.test.opa;
+
+import java.io.OutputStream;
+import java.net.HttpURLConnection;
+import java.net.URI;
+import java.net.URL;
+import java.nio.charset.StandardCharsets;
+import java.time.Duration;
+import org.apache.polaris.containerspec.ContainerSpecHelper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testcontainers.containers.GenericContainer;
+import org.testcontainers.containers.output.Slf4jLogConsumer;
+import org.testcontainers.containers.wait.strategy.Wait;
+
+public class OpaContainer extends GenericContainer<OpaContainer> {
+
+ private static final Logger LOGGER =
LoggerFactory.getLogger(OpaContainer.class);
+
+ private static final int OPA_PORT = 8181;
+
+ private URI externalUrl;
+
+ @SuppressWarnings("resource")
+ public OpaContainer() {
+ super(
+ ContainerSpecHelper.containerSpecHelper("opa", OpaContainer.class)
+ .dockerImageName(null)
+ .asCanonicalNameString());
+ withExposedPorts(OPA_PORT);
+ withCommand("run", "--server", "--addr=0.0.0.0:" + OPA_PORT);
+ waitingFor(
+ Wait.forHttp("/health")
+ .forPort(OPA_PORT)
+ .forStatusCode(200)
+ .withStartupTimeout(Duration.ofSeconds(120)));
+ withLogConsumer(new Slf4jLogConsumer(LOGGER));
+ }
+
+ @Override
+ public void start() {
+ super.start();
+ externalUrl = URI.create("http://"; + getHost() + ":" +
getMappedPort(OPA_PORT) + "/");
+ }
+
+ public URI getExternalUrl() {
+ return externalUrl;
+ }
+
+ public void createRegoPolicy(String name, String rego) {
+ try {
+ URL url = getExternalUrl().resolve("v1/policies/" + name).toURL();
+ HttpURLConnection conn = (HttpURLConnection) url.openConnection();
+ conn.setRequestMethod("PUT");
+ conn.setDoOutput(true);
+ conn.setRequestProperty("Content-Type", "text/plain");
+ try (OutputStream os = conn.getOutputStream()) {
+ os.write(rego.getBytes(StandardCharsets.UTF_8));
+ }
+ int code = conn.getResponseCode();
+ if (code < 200 || code >= 300) {
+ throw new IllegalStateException("OPA policy upload failed, HTTP " +
code);
+ }
Review Comment:
createRegoPolicy() does not set connection timeouts and never disconnects
the HttpURLConnection. In CI or local runs, a stalled OPA endpoint can hang the
test indefinitely and the connection can hold resources longer than needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
