Hi David, Thanks for the PR. I left some comments in GH, but overall, I think it is a nice addition to Polaris.
Cheers, Dmitri. On Tue, Oct 27, 2026 at 8:19 AM David Chaava via dev <[email protected]> wrote: > Hi everyone, > > We’ve addressed and replied to all bot review comments on PR #5788: > https://github.com/apache/polaris/pull/5788 > > We would appreciate another review of the updated implementation, including > feedback on retaining the validator SPI as proposed in this discussion. > > The CI workflows are awaiting maintainer approval. Could someone approve > them so the checks can run? > > Thanks, > David > > On Fri, Oct 16, 2026 at 7:10 AM Jean-Baptiste Onofré <[email protected]> > wrote: > > > Hi David > > > > Thanks for the update! We will take a look on the PR. > > > > Regards > > JB > > > > On Thu, Oct 15, 2026 at 5:01 PM David Chaava via dev < > > [email protected]> wrote: > > > >> Hi Dmitri, JB, and Prithvi, > >> > >> We have opened a draft PR implementing the validator approach discussed > >> here: > >> > >> https://github.com/apache/polaris/pull/5788 > >> > >> It introduces a CDI-discovered CatalogConfigValidator SPI with a BigLake > >> implementation, keeping PolarisAdminService provider-neutral. Validation > >> runs on creation and update using the effective connection, catalog > >> properties, and storage configuration. > >> > >> The BigLake checks apply only to the canonical endpoint, treating a > >> trailing slash as equivalent, and cover the required configuration > >> discussed in this thread. The PR includes validator and > management-service > >> tests. > >> > >> Outbound header propagation is being addressed separately in #5613 > >> <https://github.com/apache/polaris/pull/5613>, which is still open. We > >> plan > >> to merge the validator after that fix. > >> > >> We would appreciate it if you could review this PR so we can move it > >> forward. > >> > >> Thanks, > >> David > >> > >> On Wed, Sep 30, 2026 at 11:06 AM David Chaava <[email protected]> > >> wrote: > >> > >> > Hi Dmitri, JB, and Prithvi, > >> > > >> > Thank you for the thoughtful feedback. The comments help clarify both > >> the > >> > architectural concern with the initial implementation and the > >> configuration > >> > gap we intended to address. > >> > > >> > Our proposed direction is a CDI-discovered validator SPI with a > >> > BigLake-specific implementation, keeping `PolarisAdminService` > >> > provider-neutral. The validator would receive the stored connection > >> > configuration and the catalog properties and storage configuration > being > >> > saved, so the same relevant checks can be applied during creation and > >> > update. > >> > > >> > We’ll scope the BigLake validator to the canonical BigLake Iceberg > REST > >> > endpoint, treating a trailing slash as equivalent. Other GCP-backed > >> Iceberg > >> > REST catalogs should continue to pass through unchanged. Within that > >> scope, > >> > validation would focus on required configuration: tthe remote catalog > >> name, > >> > the `x-goog-user-project` value, and GCS settings when Polaris-managed > >> > credential vending requires them. We would avoid broad URL or header > >> > allowlists. > >> > > >> > We agree that outbound custom-header propagation should be addressed > >> > separately first, with request-level test coverage in > >> > `IcebergRESTFederatedCatalogFactory`. The Python CLI work will also > >> remain > >> > in a separate PR. > >> > > >> > Once the header-propagation change is addressed, we’ll prepare the > >> > validator SPI and BigLake implementation around these boundaries. If > >> we’ve > >> > misunderstood any part of the suggested scope, please let us know > >> before we > >> > proceed. > >> > > >> > Thanks again, > >> > David > >> > > >> > On Sat, Sep 26, 2026 at 7:53 AM Prithvi S < > [email protected]> > >> > wrote: > >> > > >> >> Hi all, > >> >> > >> >> Thanks for writing this up David. I agree with Dmitri and JB. > >> >> > >> >> Fail-fast checks on create and update are worth having. The checks > are > >> >> just property lookups, and a CDI bean per provider keeps > >> >> PolarisAdminService free of BigLake details. Calling the beans there > is > >> >> fine. BigLake should stay ICEBERG_REST. HIVE and BIGQUERY have their > >> own > >> >> connection types because the remote API is different. > >> >> > >> >> One thing to fix before we require header.x-goog-user-project: the > >> >> factory never sends it. Iceberg's RESTSessionCatalog does > >> >> .withHeaders(RESTUtil.configHeaders(config)). > >> >> IcebergRESTFederatedCatalogFactory builds the same client and skips > >> that > >> >> call, so the key can be on the property map and still never leave as > a > >> >> header. The CLI already stores it with --property, which lands on > >> catalog > >> >> properties, and the factory merges those in. I would fix the factory > >> first, > >> >> on its own, with a test. IcebergRestConnectionConfigInfoDpo already > >> has an > >> >> allowlist for this one header. I would leave that list as it is. > >> Whether > >> >> BigLake requires the header belongs in the bean. > >> >> > >> >> For the bean, match > >> https://biglake.googleapis.com/iceberg/v1/restcatalog > >> >> exactly, ignoring a trailing slash. Any other GCP Iceberg REST > catalog > >> >> should pass through as it does today. updateCatalog changes > properties > >> and > >> >> storage config and leaves the connection as it was stored, so the > bean > >> >> needs the stored connection plus the properties and storage about to > be > >> >> saved. The header check has to accept catalog properties too, since > >> that is > >> >> where the CLI puts it. Remote catalog name, the quota-project header, > >> and, > >> >> when credential vending is on, a GCS location and a service account. > >> Simple > >> >> presence checks are enough. > >> >> > >> >> Python in its own PR, as Dmitri said. Once the factory forwards the > >> >> header, the CLI can keep storing it where it does today. > >> >> > >> >> wdyt? > >> >> > >> >> Regards, > >> >> Prithvi S > >> >> > >> >> On Sat, Sep 26, 2026 at 11:10 AM Jean-Baptiste Onofré < > [email protected] > >> > > >> >> wrote: > >> >> > >> >>> Hi guys, > >> >>> > >> >>> Thanks for bringing this on the dev mailing list David! > >> >>> > >> >>> I agree with Dmitri's points. Here's my perspective on the > questions: > >> >>> > >> >>> 1. Validation & abstraction: I think fail-fast is definitely good > for > >> >>> user experience, but keeping PolarisAdminService lean and > >> >>> vendor-neutral is paramount. Dmitri's suggestion to use a > >> >>> CDI-discovered SPI (e.g. ConnectionConfigValidator or > >> >>> CatalogConfigValidator) is the right architecture, similar to what > we > >> >>> do for Authorizer or S3 vending mechanisms (ok we talk about SPI > again > >> >>> in Polaris but that's appropriate here :)). > >> >>> 2. Ownership, dispatch, and validation scope: since catalog creation > >> >>> and updates are infrequent and validation logic is in-memory, > running > >> >>> matching validators during create/update won't impact performance. > >> >>> However, we should be cautious about over-validation: > >> >>> a. We should validate structural prerequisites (e.g. ensuring > >> >>> required credential-vending fields exist when credential vending is > >> >>> enabled for instance) and obvious misconfigurations. > >> >>> b. We should avoid overly rigid allowlists or fragile regexs on > >> >>> URIs/headers. If a validator rejects unknown headers or URL formats, > >> >>> any minor update could break users until a new Polaris release is > >> >>> available. > >> >>> 3. Generic ICEBERG_REST vs dedicated connection type: I'm strongly > in > >> >>> favor of keeping this under ICEBERG_REST. Introducing a dedicated > >> >>> ConnectionType for each provider REST catalog would lead to enum > >> >>> proliferation and defeat the purpose of having a standard > ICEBERG_REST > >> >>> federation layer. Dedicated connection types (like HIVE or BIGQUERY) > >> >>> should be reserved for distrinct protocols or SDKs, not variations > of > >> >>> standard Iceberg REST endpoints. > >> >>> 4. Transport & outbound headers (prerequisites): as noted during the > >> >>> initial PR review, before enforcing configuration properties like > >> >>> header.x-goog-user-project, we need to make sure > >> >>> IcebergRESTFederatedCatalogFactory actually propagates those headers > >> >>> property to the outbound HTTP client. > >> >>> > >> >>> I propose the following next steps: > >> >>> 1. We introduce a clean CDI-based validator SPI (with BigLake > >> >>> implemented as one bean) as proposed by Dmitri. > >> >>> 2. We ensure outbound custom header propagation works property in > >> >>> IcebergRESTFederatedCatalogFactory. > >> >>> 3. Follow Dmitri's advice on keeping the Python CLI changes in a > >> >>> dedicated PR. > >> >>> > >> >>> WDYT? > >> >>> > >> >>> Regards > >> >>> JB > >> >>> > >> >>> On Sat, Sep 26, 2026 at 2:03 AM Dmitri Bourlatchkov < > [email protected] > >> > > >> >>> wrote: > >> >>> > > >> >>> > Hi David, > >> >>> > > >> >>> > Thanks for stating this thread! > >> >>> > > >> >>> > > 1. Is provider-specific configuration validation desirable for > >> >>> generic > >> >>> > > `ICEBERG_REST` federation? > >> >>> > > >> >>> > From POV, having validation can help users detect mistakes early. > >> It is > >> >>> > nice to have. > >> >>> > > >> >>> > In this particular case, the validation is based purely on pattern > >> >>> matching > >> >>> > against existing catalog properties. It does not involve I/O or > >> >>> external > >> >>> > dependencies. > >> >>> > > >> >>> > I believe this kind of validation should be ok to keep in the main > >> >>> codebase > >> >>> > (runtime/service). I wonder what other people think about this. > >> >>> > > >> >>> > Side note: Please consider making Python changes in separate PRs > as > >> >>> they > >> >>> > usually need different reviewers ;) > >> >>> > > >> >>> > > 3. What ownership and dispatch boundary would keep the shared > >> admin > >> >>> > > workflow provider-neutral and avoid running provider-specific > >> logic > >> >>> for > >> >>> > > every catalog operation? > >> >>> > > >> >>> > Regarding abstraction, I would suggest using CDI to find multiple > >> >>> > implementations of a (new) common validation interface. Admin > >> Service > >> >>> code > >> >>> > will stay lean and only call the validation interface. That call > >> will > >> >>> be > >> >>> > dispatched to every matching CDI bean, each running logic specific > >> to > >> >>> a use > >> >>> > case (e.g. BigLake). > >> >>> > > >> >>> > Since validation code is involved only on updates (unfrequently), > >> and > >> >>> > valation logic likely runs in-memory (fast), I do not think we > need > >> to > >> >>> > worry about calling every validators on every change. > >> >>> > > >> >>> > The PR [5513] shows some nice (recent) examples of leveraging CDI > >> for > >> >>> > pluggable components. > >> >>> > > >> >>> > [5513] https://github.com/apache/polaris/pull/5513 > >> >>> > > >> >>> > Cheers, > >> >>> > Dmitri. > >> >>> > > >> >>> > On Wed, Sep 23, 2026 at 12:26 AM David Chaava via dev < > >> >>> > [email protected]> wrote: > >> >>> > > >> >>> > > Hi Polaris community, > >> >>> > > > >> >>> > > > >> >>> > > We would like guidance on whether Polaris should support > >> >>> provider-specific > >> >>> > > configuration validation for external Iceberg REST catalogs, > and, > >> if > >> >>> so, > >> >>> > > what the appropriate extension model should be. > >> >>> > > > >> >>> > > > >> >>> > > Context: > >> >>> > > - Polaris supports the generic `ICEBERG_REST` connection type. > >> >>> > > - The initial BigLake implementation added provider-specific > >> >>> validation in > >> >>> > > the shared `PolarisAdminService` create/update path. > >> >>> > > - Review feedback on PR #5196 raised concerns about putting > >> >>> > > provider-specific logic in the main admin workflow, introducing > a > >> >>> validator > >> >>> > > without a shared abstraction, and coupling provider > configuration > >> >>> rules to > >> >>> > > Polaris releases. > >> >>> > > > >> >>> > > > >> >>> > > We have closed that implementation and would like to agree on > the > >> >>> design > >> >>> > > before proposing a replacement. > >> >>> > > > >> >>> > > > >> >>> > > Questions: > >> >>> > > 1. Is provider-specific configuration validation desirable for > >> >>> generic > >> >>> > > `ICEBERG_REST` federation? > >> >>> > > 2. If so, should Polaris first define a shared abstraction or > >> >>> extension > >> >>> > > model for such validation? > >> >>> > > 3. What ownership and dispatch boundary would keep the shared > >> admin > >> >>> > > workflow provider-neutral and avoid running provider-specific > >> logic > >> >>> for > >> >>> > > every catalog operation? > >> >>> > > 4. Under what conditions, if any, would a separate connection > >> type be > >> >>> > > preferable to extending the generic `ICEBERG_REST` flow? > >> >>> > > > >> >>> > > > >> >>> > > Links: > >> >>> > > - Tracking issue: https://github.com/apache/polaris/issues/5195 > >> >>> > > - Closed initial PR: > https://github.com/apache/polaris/pull/5196 > >> >>> > > - Architecture feedback: > >> >>> > > > >> https://github.com/apache/polaris/pull/5196#discussion_r3846267047 > >> >>> > > > >> >>> > > > >> >>> > > Thanks, > >> >>> > > David > >> >>> > > > >> >>> > >> >> > >> > > >
