-
Notifications
You must be signed in to change notification settings - Fork 375
Implement OpaPolarisAuthorizer #2680
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
adutra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sungwy for this draft PR, I couldn't resist and took a look 😄
This is really interesting imho and a very nice addition to Polaris. We need to start thinking about ways to make Polaris RBAC fully pluggable.
.../auth/opa/impl/src/main/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizer.java
Show resolved
Hide resolved
...sions/auth/opa/src/main/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizer.java
Outdated
Show resolved
Hide resolved
runtime/service/src/main/java/org/apache/polaris/service/config/AuthorizationConfiguration.java
Outdated
Show resolved
Hide resolved
runtime/service/src/main/java/org/apache/polaris/service/config/ServiceProducers.java
Outdated
Show resolved
Hide resolved
runtime/service/src/main/java/org/apache/polaris/service/config/ServiceProducers.java
Outdated
Show resolved
Hide resolved
dimas-b
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very interesting idea! Just some preliminary comments below :)
|
Thanks for working on it @sungwy ! Looking forward to the RFC/design doc! |
snazy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit, everything else beside int-tests LGTM.
For integration-tests, I'm thinking of having non-Quarkus integration tests against an OPA testcontainer in the "main" OPA module.
I've prepared some stuff for tests in this commit (here as well):
- Add a starter for an integration test in the "main" module
- Updated the existing int tests
Idea is to have the majority of integration-tests in the "main" module and only a smoke-test in runtime-service.
I think we can then get rid of the opa-tests module and can move the "main" module one directory level up.
BTW: Please add an entry for the "main" module to :polaris-bom.
WDYT?
.../impl/src/main/java/org/apache/polaris/extension/auth/opa/token/FileBearerTokenProvider.java
Outdated
Show resolved
Hide resolved
| if (Strings.isNullOrEmpty(cachedToken)) { | ||
| throw new RuntimeException( | ||
| "Bearer token is unexpectedly empty. This should not happen after successful construction."); | ||
| } | ||
| return cachedToken; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is outdated now with the new commits.
reduce volatile reads from 2 -> 1 Co-authored-by: Robert Stupp <[email protected]>
Thanks again @snazy :) Let me take a look at your draft PR and see if I can get rid of the Just so I'm clear: is your suggestion to move the QuarkusIntegrationTest annotated tests back into |
Yes and no ;) Have non-Quarkus integration tests (like |
| } | ||
|
|
||
| private void scheduleRefreshAttempt(Duration delay) { | ||
| this.refreshTask = asyncExec.schedule(this::refreshTokenAttempt, delay); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice reuse of existing functionality 🚀
| // In this case we wait for the configured amount of time | ||
| // (5 seconds in production, much lower in tests). | ||
| try { | ||
| return initialTokenFuture.get(initialTokenWaitMillis, TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional: I do not want to disturb the impl. at this stage as it looks pretty solid to me... but I suppose it could be simplified this way:
- keep a
volatilereference to aCompletionStagewith token data getTokenalways calls.get()with a timeout- the initial ref. to the
CompletionStageis make in the constructor by scheduling a refresh task - when refresh completes it will update the
CompletionStageand reschedule - the
CompletionStagewill be "complete" in all cases but the initial refresh, so.get()calls will not wait except possibly on the first call. - we have less fields and less
ifs (hopefully) in this class (onlyrefreshTaskand theCompletionStage)
WDYT?
polaris-core/build.gradle.kts
Outdated
| testFixturesApi(libs.jakarta.ws.rs.api) | ||
|
|
||
| compileOnly(libs.jakarta.annotation.api) | ||
| compileOnly(libs.jakarta.enterprise.cdi.api) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, we had some prior discussions about avoiding CDI annotations in core as a convention...
If it's required only for @Identifier("internal") on DefaultPolarisAuthorizerFactory, I'd propose remove the annotation from the class and make a producer method for it in ServiceProducers.
Alternatively, we could remove DefaultPolarisAuthorizerFactory completely and update ServiceProducers to "manually" use PolarisAuthorizerImpl if Instance<PolarisAuthorizerFactory> is not resolvable in CDI.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dimas-b these are great suggestions. Thank you for giving me the background context on the package discussions.
I've added a Producer method in ServiceProducers and removed the cdi dependency from polaris-core
I actually found it very helpful to have the Quarkus integration tests because it helped verify the @dimas-b - I would love to get your thoughts on this suggestion as well |
I've added the |
Yes, we will need them in BOM as well. A separate PR sounds good to me. |
dimas-b
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about the delay. This PR LGTM in its current state, except the dep. on CDI annotations in polaris-core.
The current tests layout LGTM. So, if @snazy is ok with that, I think we're pretty close to merging.
Re: CDI in core, we can probably discuss that on the dev ML if you prefer to keep the annotation. The reason I flagged it is not my personal objection, but the fact that IIRC it was discussed before and the agreement in the community was to avoid CDI-specific annotations in core.
|
@dimas-b - I've addressed the concern regarding the |
dimas-b
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 @sungwy : Many thanks for the contribution and for bearing with the long review process 😉
|
Woohoo! Thank you @dimas-b ! And @snazy @flyrain and @adutra for the reviews as well. This was a great way for me to get up to speed with the project, so thanks for your interest and patience in reviewing this PR as well. I'll be following this up with some more PRs (docs, schema, etc) and we'll start integrating this into our platform. |
|
|
||
| implementation(libs.jakarta.servlet.api) | ||
|
|
||
| runtimeOnly(project(":polaris-async-vertx")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this dependency? Is this only for test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this actually is the CDI implementation of polaris-async-api AsyncExec that's optimized for Quarkus applications. Hence, we need it as a runtimeOnly dependency in polaris-runtime-service
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m good with using the Vert.x async executor since we’re already on Quarkus. The polaris-async-vertx module currently lives under /persistence/nosql, but it’s really part of the async infrastructure rather than a NoSQL implementation detail. We could consider moving it out of that directory, though it doesn’t need to be done in this PR.
* Fix Jandex Maven coordinates (apache#2888) The entry `jandex = { module = "io.smallrye.jandex:jandex", version ="3.5.0" }` is wrong (coordinates are `io.smallrye:jandex`), and Jandex is defined elsewhere as `smallrye-jandex`. Interestingly, these (broken) coordinates seem to cause the consistent re-creation of the Quarkus 3.29.0 PR (the cause is a mystery). * Update plugin com.gradle.develocity to v4.2.2 (apache#2597) * Site: Hugo docs relative links (apache#2892) * Update dependency software.amazon.awssdk:bom to v2.36.2 (apache#2901) * Update GitHub Artifact Actions (apache#2895) * Formatting: apply Spotless to :polaris-distribution (apache#2900) * Build: Capture jcstress output in a log file (apache#2890) The jcstress output is pretty verbose and prints a lot to the console. This change captures the output in a log file. In case of a test failure, the output is logged to the console, but only in case of a failure. * Prep: Site for 1.2 release (apache#2877) * Adding 1.2.0 as one of active releases (apache#2916) Co-authored-by: Yufei Gu <yufei.apache.org> * Use official spark image (apache#2899) * Update dependency ipykernel to v7.1.0 (apache#2918) * Added missing features doc (apache#2898) * Added missing features doc * Added missing features doc * Site: Add a blog for StarRocks and Apache Polaris Integration (apache#2851) * NoSQL: Node IDs - API, SPI + general implementation (apache#2728) * NoSQL: Node IDs - API, SPI + general implementation This PR provides a mechanism to assign a Polaris-cluster-wide unique node-ID to each Polaris instance, which is then used when generating Polaris-cluster-wide unique Snowflake-IDs. The change is fundamental for the NoSQL work, but also demanded for the existing relational JDBC persistence. Does not include any persistence specific implementation. * NoSQL: Fail node-management-impl init after timeout Also move the expensive part to a `@PostConstruct` to not block CDI entirely from initializing. * Update dependency io.prometheus:prometheus-metrics-exporter-servlet-jakarta to v1.4.2 (apache#2929) * Build-logic: `GitInfo` refactor (apache#2908) Allows use of `GitInfo` for other use cases than just Jar manifest attributes. SBOM generation will be another use case for Git information. * Memoize ASF project information (apache#2909) Information included in Polaris publications pulls some information about the project from ASF project metadata sources (Whimsey). This information is currently only used when generating Maven poms, but will also be needed in SBOMs. This change adds a new, memoized `AsfProject` information object, which holds the project infromation from Whimsey. * Build: Simplify signing + fix execution in polaris-distribution (apache#2906) This change simplifies generation of non-publication artifacts by adding a function taking the task which outputs shall be signed. That function takes care of setting up the correct task dependencies and task execution. Also fixes an issue that signing does not always happen when running `./gradlew :polaris-distribution:assemble`, because the task dependency graph for the archive tasks and the corresponding signing tasks isn't properly set up. * Proposed Test Fix (apache#2936) Co-authored-by: Travis Michael Bowen <[email protected]> * Update docker.io/prom/prometheus Docker tag to v3.7.3 (apache#2944) * Update Quarkus Platform and Group to v3.29.0 (apache#2934) * Update Gradle to v9.2.0 (apache#2938) Co-authored-by: Robert Stupp <[email protected]> * Update dependency openapi-generator-cli to v7.17.0 (apache#2940) * Implement OpaPolarisAuthorizer (apache#2680) * Update dependency com.github.ben-manes.caffeine:caffeine to v3.2.3 (apache#2923) * Prefer PolarisPrincipal.getRoles in Resolver (apache#2925) it should be sufficient to rely on `SecurityContext.getUserPrincipal` alone, we dont need to call `isUserInRole` explicitly. note due to the `ResolverTest` testing with non-existent roles we have to add null-filtering to the `Resolver`. * Move `nodeids` to `nosql` package parent (apache#2931) Following up on apache#2728 this change moves "nodeids" code to the `org.apache.polaris.persistence.nosql.nodeids` package. * Update actions/stale digest to 39bea7d (apache#2950) * Update dependency org.junit:junit-bom to v5.14.1 (apache#2951) * docs(2843): Add documentation around Polaris-Tools (apache#2946) * Add documentation around Polaris-Tools * Related to apache#2843 * Add getting started with Apache Ozone (apache#2853) * Add getting started with Apache Ozone Use Apache Ozone as an example S3 impl. that does not have STS. * fix typo in MinIO readme * Update dependency com.azure:azure-sdk-bom to v1.3.0 (apache#2754) * docs: add feature configuration section to Hive federation guide (apache#2952) Add documentation for required feature flags when enabling Hive Metastore federation. Users must configure three properties in `application.properties` before Hive federation will work: - `SUPPORTED_CATALOG_CONNECTION_TYPES` - `SUPPORTED_EXTERNAL_CATALOG_AUTHENTICATION_TYPES` - `ENABLE_CATALOG_FEDERATION` Inspired from [this](https://apache-polaris.slack.com/archives/C084XDM50CB/p1761851426511259) Slack thread. Co-authored-by: Prathyush Shankar <[email protected]> * Change getting-start docker file to use official spark image from outdated jupyter image (apache#2943) * Use official spark image * Use official spark image * Use official spark image * Use official spark image * Use official spark image * Use Iterable for realms in BootstrapCommand (apache#2956) * Simplify digest generation (apache#2907) Similarly to the change to simplify artifact signing, this change simplifies digest generation by introducing a function to digest the output files of any task. That function takes care of setting up the correct task dependencies and task execution. Also removes an unnecessary double buffering during digest generation. * Build: `GitInfo` function to build a raw github content URL (apache#2910) * NoSQL: nodeids renames * NoSQL: Update test for Caffeine 3.2.3 The read of `Eviction` properties is "just" a volatile read since Caffeine 3.2.3 and trigger cleanups asynchronously. Before 3.2.3, cleanups happened synchronously. This change breaks the initially present assertions of this test, but not the functionality of the production code. See ben-manes/caffeine#1897 * Last merged commit cec41c4 --------- Co-authored-by: Mend Renovate <[email protected]> Co-authored-by: olsoloviov <[email protected]> Co-authored-by: Prashant Singh <[email protected]> Co-authored-by: Yufei Gu <[email protected]> Co-authored-by: Yong Zheng <[email protected]> Co-authored-by: Youngwb <[email protected]> Co-authored-by: Travis Bowen <[email protected]> Co-authored-by: Travis Michael Bowen <[email protected]> Co-authored-by: Sung Yun <[email protected]> Co-authored-by: Christopher Lambert <[email protected]> Co-authored-by: Dmitri Bourlatchkov <[email protected]> Co-authored-by: Adam Christian <[email protected]> Co-authored-by: carc-prathyush-shankar <[email protected]> Co-authored-by: Prathyush Shankar <[email protected]>
* Add loadEntities batch call and rename listFullEntities (apache#2508) * Add loadEntities batch call and rename listFullEntities * Changed batch call to implement loadResolvedEntities instead * Add loadResolvedEntities by id and entity cache support * Add additional test for loadResolvedEntities by id * Added additional test and updated comments in EntityCache interface * Add additional constructor to ResolvedEntitiesResult * Fixed unused method reference * Removed loadResolvedEntities method with lookup record param * Pulled out toResolvedPolarisEntity method per PR comment * Core: made the ARN role regex more generic (apache#3005) * fix(docs): Generify S3 index page (apache#2997) * Remove the mention of "cloud" since not all possible storage options are provided in "cloud". * Avoid listing specific child pages in the doc test. Rely on Hugo-general index (on the left-hand pane). --------- Co-authored-by: Alexandre Dutra <[email protected]> * fix(deps): update dependency io.prometheus:prometheus-metrics-exporter-servlet-jakarta to v1.4.3 (apache#3009) * fix(deps): update dependency com.google.cloud:google-cloud-storage-bom to v2.60.0 (apache#3011) * fix(deps): update dependency io.opentelemetry:opentelemetry-bom to v1.56.0 (apache#3012) * fix(deps): update dependency com.adobe.testing:s3mock-testcontainers to v4.10.0 (apache#3010) * fix(deps): update dependency org.agrona:agrona to v2.3.2 (apache#3014) * fix(deps): update quarkus platform and group to v3.29.2 (apache#3013) * chore(deps): update dependency pre-commit to v4.4.0 (apache#3015) * fix(deps): update dependency software.amazon.awssdk:bom to v2.38.2 (apache#3019) * Add test for TracingFilter (apache#2847) * NoSQL: Add (micro-ish) benchmarks (apache#3006) A project for JMH based benchmarks against NoSQL persistence. * Helm chart: include configmap checksum in deployment annotations (apache#3023) * fix(deps): update dependency ch.qos.logback:logback-classic to v1.5.21 (apache#3025) * NoSQL: Realms handling (apache#3007) Introduces handling for realms including realm-state management/transition. The `RealmStore` implementation for NoSQL depends on CDI components, coming in a follo-up PR. * Rename AccessConfig and AccessConfigProvider for clarity (apache#2883) * rename AccessConfig for clarity * rename getStorageAccessConfig() and add javadoc * Refactor: improve and clean up Dockerfiles (apache#2957) * Refactor: improve and clean up Dockerfiles * Refactor: improve and clean up Dockerfiles * Refactor: improve and clean up Dockerfiles * Refactor: improve and clean up Dockerfiles * Refactor: improve and clean up Dockerfiles * Refactor: improve and clean up Dockerfiles * Make StorageAccessConfigProvider request-scoped (apache#2974) - add `StorageCredentialsVendor` as request-scoped wrapper around `PolarisCredentialVendor` - make `FileIOFactory` request-scoped - make `TaskFileIOSupplier` request-scoped * Increase javadoc visibility in `nosql/realms` (apache#3029) This is to fix javadoc error: `No public or protected classes found to document` * NoSQL: Add correctness tests (apache#3027) Verifies the correctness of concurrent commits, and big index handling. These tests are intentionally _not_ part of the base-backend test suite for two reasons: 1. These tests do not run against the `Backend` interface but the `Persistence` interface, including commit and index logic. 2. These tests are intended to be runnable against a custom provisioned database cluster, not just tiny-ish test containers. * NoSQL: Add maintenance API, SPI (apache#3028) Maintenance operations include a bunch of tasks that are regularly executed against a backend database. Types of maintenance operations include: * Purging unreferenced objects and references within a catalog * Purging whole catalogs that are marked to be purged * Purging whole realms that are marked to be purged Implementation added in a follow-up PR. * Embrace request-scoped TokenBroker (apache#3024) * Embrace request-scoped TokenBroker `TokenBroker` and `CallContext` are both request-scoped, so instead of passing the former into the latter, we can do this via the `TokenBrokerFactory` and thus simplify the `TokenBroker` interface. * fix(deps): update dependency io.smallrye:jandex to v3.5.2 (apache#3032) * Fix monkey patching (apache#3016) * chore(deps): update quay.io/keycloak/keycloak docker tag to v26.4.5 (apache#3034) * chore(deps): update registry.access.redhat.com/ubi9/openjdk-21-runtime docker tag to v1.23-6.1762870925 (apache#3053) * fix(deps): update dependency org.testcontainers:testcontainers-bom to v2.0.2 (apache#3054) * chore(deps): update postgres docker tag to v18.1 (apache#3055) * Add Polaris Community Meeting 2025-11-13 (apache#3060) * Site: Rename menu "downloads" to "releases" (apache#2928) * Update dependency software.amazon.awssdk:bom to v2.38.7 (apache#3065) * Test-fix: Cleanup OPA test container on stop (apache#3041) Quarkus takes care of reusing a test-resource across tests. The current behavior leaves the container around. Plus some nit-fixes (deprecation + local var) * Update dependency org.apache.commons:commons-lang3 to v3.20.0 (apache#3063) * Build: ensure LICENSE/NOTICE is in all jars, always add pom-files to all jars (apache#3057) There are a some inconsistencies between the different kinds of jars and the included information: * LICENSE/NOTICE files are present in the "main" jar and in the sources jar, but not in the javadoc jar. * The Maven pom.xml and pom.properties files are only present for release builds or when explicitly requested. * "Additional" jar-manifest attributes that are only present in release builds. This change fixes the three mentioned issues: * Always include pom.xml and pom.properties in the built jar files. * Always include the additional jar-manifest attributes, except the Git information, which would otherwise render the Gradle build cache ineffective. * Include pom.xml + pom.properties + license/notice in literally all jar files. The Gradle logic to include the license+notice+pom files has been simplified as well. * Remove unused polarisEventListener field from IcebergCatalogHandler (apache#3045) it was added in c3f5001 but then its only usage was removed in d03c717 * fix(deps): update quarkus platform and group to v3.29.3 (apache#3052) * Site: Add Open Policy Agent (OPA) as External Policy Decision Point (apache#3030) Doc PR following up the introduction of OpaPolarisAuthorizer: apache#2680 * OPA: Tackle deprecation warnings (apache#3042) Instead of suppressing the deprecations, this change updates the code a little bit to remove the mocks (except to create a non-nullable parameter). * Use POJOs for OPA JSON schema construction and publish schema (apache#3031) Co-authored-by: Robert Stupp <[email protected]> * Use CDI for more test setups (apache#3040) this avoids a bunch of redundant manual setup. the important parts are establishing a `RealmContext` by calling `QuarkusMock.installMockForType` and then populating `polarisContext` from the injected `CallContext`. * fix(deps): update dependency com.github.dasniko:testcontainers-keycloak to v4 (apache#3070) * chore(deps): update actions/checkout digest to 93cb6ef (apache#3068) * OPA: Fail fast when OPA bearer token file is unreadable (apache#3062) * fix(deps): update immutables to v2.11.7 (apache#3072) * Skip Hugo Site workflow on forks (apache#3056) Forks usually don't have the "versioned-docs" tag and thus PRs against forks or rebasing the main branch on a fork currently always causes workflow failures. * Fix warnings around TransactionWorkspaceMetaStoreManager (apache#3044) - dont return `null` for interface methods that are `@Nonnull` - fix wrong method name parameters - dont annotate void methods as `@Nonnull` * NoSQL: Add CDI/common+testing + necessary nosql-store implementations (apache#3035) Adds common and test-specific CDI functionality. Requires the NoSQL store implementations `:polaris-persistence-nosql-realms-store-nosql` and `:polaris-nodes-store-nosql`. Those modules have cross-project dependencies for test purposes, hence those are all contained in this PR. CDI for Quarkus will be added in a follow-up. * Automate the release guide - Take 2 - Github workflows (apache#2383) The release automation is simplified to four GitHub workflows that just require the really mandatory user input: the version number. 1. workflow: Trigger the creation of the release branch 2. workflow: Upgrade the release branch with the version and build the the final change-log for that version 3. workflow: Build the RC artifacts from the release branch and push those to the various staging repositories 4. workflow: Eventually release the artifacts. See also the [email announcement](https://lists.apache.org/thread/d0smz07gnr509yj5dc6omo3cvkf1pnh7). --------- Co-authored-by: Robert Stupp <[email protected]> * Update actions/checkout digest to 93cb6ef (apache#3082) * NoSQL: adapt to conflicting changes in main * Last merged commit 8ccddc5 --------- Co-authored-by: Michael Collado <[email protected]> Co-authored-by: cccs-cat001 <[email protected]> Co-authored-by: Dmitri Bourlatchkov <[email protected]> Co-authored-by: Alexandre Dutra <[email protected]> Co-authored-by: Mend Renovate <[email protected]> Co-authored-by: Nuoya Jiang <[email protected]> Co-authored-by: Yong Zheng <[email protected]> Co-authored-by: Christopher Lambert <[email protected]> Co-authored-by: JB Onofré <[email protected]> Co-authored-by: Yufei Gu <[email protected]> Co-authored-by: Sung Yun <[email protected]> Co-authored-by: Pierre Laporte <[email protected]>
Resolves #138
RFC: https://docs.google.com/document/d/1HadMFygjbuZathZZPanO6cFVorx0Ju0FopkICxX1tCE/edit?tab=t.0
Mailing list discussion: https://lists.apache.org/thread/54qdbsxs3j7wwhv3tsccqj6qng5lqgmz
Some followup items highlighted as a part of this PR review:
OpaHttpClientFactorythat utilizesPoolingHttpClientConnectionManagerto get opa-http-client to be more production ready