dimas-b commented on code in PR #2680: URL: https://github.com/apache/polaris/pull/2680#discussion_r2453338636
########## extensions/auth/opa/impl/src/test/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizerFactoryTest.java: ########## @@ -0,0 +1,163 @@ +/* + * 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.extension.auth.opa; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.io.IOException; +import java.net.URI; +import java.nio.file.Files; +import java.nio.file.Path; +import java.time.Clock; +import java.time.Duration; +import java.util.Optional; +import org.apache.polaris.core.config.RealmConfig; +import org.apache.polaris.extension.auth.opa.token.FileBearerTokenProvider; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +public class OpaPolarisAuthorizerFactoryTest { + + @TempDir Path tempDir; + + @Test + public void testFactoryWithStaticTokenConfiguration() { + // Mock configuration for static token + OpaAuthorizationConfig.BearerTokenConfig.StaticTokenConfig staticTokenConfig = + mock(OpaAuthorizationConfig.BearerTokenConfig.StaticTokenConfig.class); Review Comment: nit: `OpaAuthorizationConfig` and related interfaces could be annotated with `@PolarisImmutable` to get auto-generated default impl. IMHO, using immutable builders is nicer than mocking... but this is totally optional. ########## extensions/auth/opa/impl/src/main/java/org/apache/polaris/extension/auth/opa/token/FileBearerTokenProvider.java: ########## @@ -0,0 +1,222 @@ +/* + * 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.extension.auth.opa.token; + +import static com.google.common.base.Preconditions.checkState; + +import com.auth0.jwt.JWT; +import com.auth0.jwt.exceptions.JWTDecodeException; +import com.auth0.jwt.interfaces.DecodedJWT; +import com.google.common.base.Strings; +import jakarta.annotation.Nullable; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.time.Clock; +import java.time.Duration; +import java.time.Instant; +import java.util.Date; +import java.util.Optional; +import java.util.concurrent.atomic.AtomicBoolean; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * A token provider that reads tokens from a file and automatically reloads them based on a + * configurable refresh interval or JWT expiration timing. + * + * <p>This is particularly useful in Kubernetes environments where tokens are mounted as files and + * refreshed by external systems (e.g., service account tokens, projected volumes, etc.). + * + * <p>The token file is expected to contain the bearer token as plain text. Leading and trailing + * whitespace will be trimmed. + * + * <p>If JWT expiration refresh is enabled and the token is a valid JWT with an 'exp' claim, the + * provider will automatically refresh the token based on the expiration time minus a configurable + * buffer, rather than using the fixed refresh interval. + */ +public class FileBearerTokenProvider implements BearerTokenProvider { + + private static final Logger logger = LoggerFactory.getLogger(FileBearerTokenProvider.class); + + private final Path tokenFilePath; + private final Duration refreshInterval; + private final boolean jwtExpirationRefresh; + private final Duration jwtExpirationBuffer; + private final Clock clock; + private final AtomicBoolean refreshLock = new AtomicBoolean(); + + private volatile String cachedToken; + private volatile Instant lastRefresh; + private volatile Instant nextRefresh; + private volatile boolean closed = false; + + /** + * Create a new file-based token provider with JWT expiration support. + * + * @param tokenFilePath path to the file containing the bearer token + * @param refreshInterval how often to check for token file changes (fallback for non-JWT tokens) + * @param jwtExpirationRefresh whether to use JWT expiration for refresh timing + * @param jwtExpirationBuffer buffer time before JWT expiration to refresh the token + * @param clock clock instance for time operations + */ + public FileBearerTokenProvider( + Path tokenFilePath, + Duration refreshInterval, + boolean jwtExpirationRefresh, + Duration jwtExpirationBuffer, + Clock clock) { + this.tokenFilePath = tokenFilePath; + this.refreshInterval = refreshInterval; + this.jwtExpirationRefresh = jwtExpirationRefresh; + this.jwtExpirationBuffer = jwtExpirationBuffer; + this.clock = clock; + this.lastRefresh = Instant.MIN; // Force initial load + this.nextRefresh = Instant.MIN; // Force initial refresh + + logger.debug( + "Created file token provider for path: {} with refresh interval: {}, JWT expiration refresh: {}, JWT buffer: {}", + tokenFilePath, + refreshInterval, + jwtExpirationRefresh, + jwtExpirationBuffer); + } + + @Override + public String getToken() { + checkState(!closed, "Token provider is closed"); + + // Check if we need to refresh + if (shouldRefresh()) { + refreshToken(); + } + + // If we couldn't load a token and have no cached token, this is a fatal error + if (Strings.isNullOrEmpty(cachedToken)) { + throw new RuntimeException( + "Unable to load bearer token from file: " + + tokenFilePath + + ". This is required for OPA authorization."); + } + return cachedToken; + } + + @Override + public void close() { + closed = true; + cachedToken = null; + } + + private boolean shouldRefresh() { + return clock.instant().isAfter(nextRefresh); + } + + private void refreshToken() { + if (!refreshLock.compareAndSet(false, true)) { + return; Review Comment: If two threads come here from `getToken()` on a freshly created object, and the loading of the token takes some time, one the threads will get `null` as the token... right? If this is not a concern, could you add a comment? ########## runtime/service/src/intTest/java/org/apache/polaris/service/it/ServiceProducersIT.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.polaris.service.it; + +import static org.junit.jupiter.api.Assertions.assertNotNull; Review Comment: Most of the (new) tests use AssertJ assertions... Would you mind switching to it? ########## runtime/service/src/intTest/java/org/apache/polaris/service/it/ServiceProducersIT.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.polaris.service.it; + +import static org.junit.jupiter.api.Assertions.assertNotNull; + +import io.quarkus.test.junit.QuarkusTest; +import io.quarkus.test.junit.QuarkusTestProfile; +import jakarta.inject.Inject; +import java.util.HashMap; +import java.util.Map; +import org.apache.polaris.core.auth.PolarisAuthorizer; +import org.junit.jupiter.api.Test; + +public class ServiceProducersIT { + + public static class InternalAuthorizationConfig implements QuarkusTestProfile { + @Override + public Map<String, String> getConfigOverrides() { + Map<String, String> config = new HashMap<>(); + config.put("polaris.authorization.type", "internal"); + return config; + } + } + + @QuarkusTest + @io.quarkus.test.junit.TestProfile(ServiceProducersIT.InternalAuthorizationConfig.class) Review Comment: nit: import? ########## extensions/auth/opa/build.gradle.kts: ########## Review Comment: `extensions/auth/opa/` seems to be an empty module... Does it need a build script? ########## extensions/auth/opa/impl/src/main/java/org/apache/polaris/extension/auth/opa/token/FileBearerTokenProvider.java: ########## @@ -0,0 +1,222 @@ +/* + * 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.extension.auth.opa.token; + +import static com.google.common.base.Preconditions.checkState; + +import com.auth0.jwt.JWT; +import com.auth0.jwt.exceptions.JWTDecodeException; +import com.auth0.jwt.interfaces.DecodedJWT; +import com.google.common.base.Strings; +import jakarta.annotation.Nullable; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.time.Clock; +import java.time.Duration; +import java.time.Instant; +import java.util.Date; +import java.util.Optional; +import java.util.concurrent.atomic.AtomicBoolean; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * A token provider that reads tokens from a file and automatically reloads them based on a + * configurable refresh interval or JWT expiration timing. + * + * <p>This is particularly useful in Kubernetes environments where tokens are mounted as files and + * refreshed by external systems (e.g., service account tokens, projected volumes, etc.). + * + * <p>The token file is expected to contain the bearer token as plain text. Leading and trailing + * whitespace will be trimmed. + * + * <p>If JWT expiration refresh is enabled and the token is a valid JWT with an 'exp' claim, the + * provider will automatically refresh the token based on the expiration time minus a configurable + * buffer, rather than using the fixed refresh interval. + */ +public class FileBearerTokenProvider implements BearerTokenProvider { + + private static final Logger logger = LoggerFactory.getLogger(FileBearerTokenProvider.class); + + private final Path tokenFilePath; + private final Duration refreshInterval; + private final boolean jwtExpirationRefresh; + private final Duration jwtExpirationBuffer; + private final Clock clock; + private final AtomicBoolean refreshLock = new AtomicBoolean(); + + private volatile String cachedToken; + private volatile Instant lastRefresh; + private volatile Instant nextRefresh; + private volatile boolean closed = false; + + /** + * Create a new file-based token provider with JWT expiration support. + * + * @param tokenFilePath path to the file containing the bearer token + * @param refreshInterval how often to check for token file changes (fallback for non-JWT tokens) + * @param jwtExpirationRefresh whether to use JWT expiration for refresh timing + * @param jwtExpirationBuffer buffer time before JWT expiration to refresh the token + * @param clock clock instance for time operations + */ + public FileBearerTokenProvider( + Path tokenFilePath, + Duration refreshInterval, + boolean jwtExpirationRefresh, + Duration jwtExpirationBuffer, + Clock clock) { + this.tokenFilePath = tokenFilePath; + this.refreshInterval = refreshInterval; + this.jwtExpirationRefresh = jwtExpirationRefresh; + this.jwtExpirationBuffer = jwtExpirationBuffer; + this.clock = clock; + this.lastRefresh = Instant.MIN; // Force initial load + this.nextRefresh = Instant.MIN; // Force initial refresh + + logger.debug( + "Created file token provider for path: {} with refresh interval: {}, JWT expiration refresh: {}, JWT buffer: {}", + tokenFilePath, + refreshInterval, + jwtExpirationRefresh, + jwtExpirationBuffer); + } + + @Override + public String getToken() { + checkState(!closed, "Token provider is closed"); Review Comment: Is it worth the trouble? It looks like we keep the `closed` flag only for this check. ########## runtime/defaults/src/main/resources/application-it.properties: ########## @@ -56,3 +56,4 @@ polaris.realm-context.realms=POLARIS,OTHER polaris.storage.gcp.token=token polaris.storage.gcp.lifespan=PT1H + Review Comment: spurious change? ########## extensions/auth/opa/impl/src/main/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizerFactory.java: ########## @@ -0,0 +1,176 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.extension.auth.opa; + +import com.fasterxml.jackson.databind.ObjectMapper; +import io.smallrye.common.annotation.Identifier; +import jakarta.annotation.PostConstruct; +import jakarta.annotation.PreDestroy; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.inject.Inject; +import java.io.IOException; +import java.net.URI; +import java.time.Clock; +import java.time.Duration; +import org.apache.hc.client5.http.impl.classic.CloseableHttpClient; +import org.apache.hc.client5.http.impl.classic.HttpClients; +import org.apache.polaris.core.auth.PolarisAuthorizer; +import org.apache.polaris.core.auth.PolarisAuthorizerFactory; +import org.apache.polaris.core.config.RealmConfig; +import org.apache.polaris.extension.auth.opa.token.BearerTokenProvider; +import org.apache.polaris.extension.auth.opa.token.FileBearerTokenProvider; +import org.apache.polaris.extension.auth.opa.token.StaticBearerTokenProvider; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** Factory for creating OPA-based Polaris authorizer implementations. */ +@ApplicationScoped +@Identifier("opa") +class OpaPolarisAuthorizerFactory implements PolarisAuthorizerFactory { + + private static final Logger logger = LoggerFactory.getLogger(OpaPolarisAuthorizerFactory.class); + + private final OpaAuthorizationConfig opaConfig; + private final Clock clock; + private CloseableHttpClient httpClient; + private BearerTokenProvider bearerTokenProvider; + private ObjectMapper objectMapper; + + @Inject + public OpaPolarisAuthorizerFactory(OpaAuthorizationConfig opaConfig, Clock clock) { + this.opaConfig = opaConfig; + this.clock = clock; + this.objectMapper = new ObjectMapper(); + } + + /** + * Gets the OPA authorization configuration. Used by OpaProductionReadinessCheck + * + * @return the OPA configuration + */ + OpaAuthorizationConfig getConfig() { + return opaConfig; + } + + @PostConstruct + public void initialize() { + // Validate configuration once during startup + opaConfig.validate(); + + // Create HTTP client once during startup + httpClient = createHttpClient(); + + // Setup authentication once during startup + setupAuthentication(opaConfig.auth()); Review Comment: maybe `bearerTokenProvider = setupAuthentication(opaConfig.auth())`?.. to follow the pattern of the `createHttpClient()` call above. ########## runtime/service/src/intTest/java/org/apache/polaris/service/it/ServiceProducersIT.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.polaris.service.it; + +import static org.junit.jupiter.api.Assertions.assertNotNull; + +import io.quarkus.test.junit.QuarkusTest; +import io.quarkus.test.junit.QuarkusTestProfile; +import jakarta.inject.Inject; +import java.util.HashMap; +import java.util.Map; +import org.apache.polaris.core.auth.PolarisAuthorizer; +import org.junit.jupiter.api.Test; + +public class ServiceProducersIT { + + public static class InternalAuthorizationConfig implements QuarkusTestProfile { + @Override + public Map<String, String> getConfigOverrides() { + Map<String, String> config = new HashMap<>(); + config.put("polaris.authorization.type", "internal"); + return config; + } + } + + @QuarkusTest + @io.quarkus.test.junit.TestProfile(ServiceProducersIT.InternalAuthorizationConfig.class) + public static class InternalAuthorizationTest { + + @Inject PolarisAuthorizer polarisAuthorizer; + + @Test + void testInternalPolarisAuthorizerProduced() { + assertNotNull(polarisAuthorizer, "PolarisAuthorizer should be produced"); + // Verify it's the correct implementation for internal config + assertNotNull(polarisAuthorizer, "Internal PolarisAuthorizer should not be null"); Review Comment: This assertion always holds if line 50 passes, right? ########## runtime/service/build.gradle.kts: ########## @@ -75,6 +75,7 @@ dependencies { implementation(libs.auth0.jwt) + implementation(libs.apache.httpclient5) Review Comment: Do we need this dep declaration? Is is not available transitively? ########## runtime/service/src/main/java/org/apache/polaris/service/config/ServiceProducers.java: ########## @@ -139,10 +139,23 @@ public RealmConfig realmConfig(CallContext callContext) { return callContext.getRealmConfig(); } + @Produces + @ApplicationScoped + public PolarisAuthorizerFactory polarisAuthorizerFactory( + AuthorizationConfiguration authorizationConfig, + @Any Instance<PolarisAuthorizerFactory> authorizerFactories) { + PolarisAuthorizerFactory factory = + authorizerFactories + .select(Identifier.Literal.of(authorizationConfig.type().getValue())) Review Comment: Is it worth having an enum for the type? `PolarisAuthorizerFactory` are pluggable at build time. There is no way for `runtime/service` code to be in sync will all possible downstream projects that want to customize `PolarisAuthorizerFactory` 🤔 ########## build.gradle.kts: ########## @@ -89,7 +89,7 @@ tasks.named<RatTask>("rat").configure { // Misc build artifacts excludes.add(".java-version") excludes.add("**/.keep") - excludes.add("logs/**") + excludes.add("extensions/auth/opa/tests/logs/**") Review Comment: Why would test logs be outside `build` or `tmp` directories? 🤔 -- 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]
