You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "davidchaava (via GitHub)" <gi...@apache.org> on 2026/10/14 12:53:03 UTC
[PR] Add CDI-based validation for BigLake catalog configuration [polaris]
davidchaava opened a new pull request, #5788:
URL: https://github.com/apache/polaris/pull/5788
## Summary
- Add a CDI-discovered `CatalogConfigValidator` SPI, with a BigLake-specific validator, keeping `PolarisAdminService` provider-neutral.
- Run validation during catalog creation and updates using the effective connection, catalog properties, and storage configuration.
- Limit BigLake checks to the canonical BigLake Iceberg REST endpoint, treating a trailing slash as equivalent. Validate required configuration while avoiding broad URL and header allowlists.
- Leave other GCP-backed Iceberg REST catalogs unchanged.
## Related Work
Related to [apache/polaris#5613](https://github.com/apache/polaris/pull/5613), which is expected to fix outbound custom-header propagation in `IcebergRESTFederatedCatalogFactory`. This PR does not include that factory change; BigLake quota-project validation assumes the header propagation fix is addressed there. Python CLI changes remain separate.
## Testing
- `./gradlew format` passed.
- Focused BigLake validator and catalog create/update tests passed.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add CDI-based validation for BigLake catalog configuration [polaris]
Posted by "davidchaava (via GitHub)" <gi...@apache.org>.
davidchaava commented on code in PR #5788:
URL: https://github.com/apache/polaris/pull/5788#discussion_r4314621960
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -188,7 +191,55 @@ public PolarisAdminService(
@NonNull PolarisPrincipal principal,
@NonNull PolarisAuthorizer authorizer,
@NonNull ReservedProperties reservedProperties,
- @NonNull S3CredentialVendingMechanisms vendingMechanisms) {
+ @NonNull S3CredentialVendingMechanisms vendingMechanisms,
+ @Any Instance<CatalogConfigValidator> catalogConfigValidators) {
+ this(
+ callContext,
+ resolutionManifestFactory,
+ metaStoreManager,
+ userSecretsManager,
+ serviceIdentityProvider,
+ principal,
+ authorizer,
+ reservedProperties,
+ vendingMechanisms,
+ (Iterable<CatalogConfigValidator>) catalogConfigValidators);
+ }
+
+ public PolarisAdminService(
+ @NonNull CallContext callContext,
+ @NonNull ResolutionManifestFactory resolutionManifestFactory,
+ @NonNull PolarisMetaStoreManager metaStoreManager,
+ @NonNull UserSecretsManager userSecretsManager,
+ @NonNull ServiceIdentityProvider serviceIdentityProvider,
+ @NonNull PolarisPrincipal principal,
+ @NonNull PolarisAuthorizer authorizer,
+ @NonNull S3CredentialVendingMechanisms vendingMechanisms,
Review Comment:
Thanks for catching this. I restored the parameter order to reservedProperties followed by vendingMechanisms, matching the @Inject constructor, and updated all affected test call sites.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add CDI-based validation for BigLake catalog configuration [polaris]
Posted by "orca-review-bot (via GitHub)" <gi...@apache.org>.
orca-review-bot commented on code in PR #5788:
URL: https://github.com/apache/polaris/pull/5788#discussion_r4342286912
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/BigLakeCatalogValidator.java:
##########
@@ -0,0 +1,158 @@
+/*
+ * 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.admin;
+
+import com.google.common.base.Strings;
+import jakarta.enterprise.context.ApplicationScoped;
+import java.net.URI;
+import java.util.Map;
+import org.apache.polaris.core.admin.model.AuthenticationParameters;
+import org.apache.polaris.core.admin.model.Catalog;
+import org.apache.polaris.core.admin.model.ExternalCatalog;
+import org.apache.polaris.core.admin.model.GcpStorageConfigInfo;
+import org.apache.polaris.core.admin.model.IcebergRestConnectionConfigInfo;
+import org.apache.polaris.core.admin.model.StorageConfigInfo;
+import org.apache.polaris.core.config.FeatureConfiguration;
+import org.apache.polaris.core.config.RealmConfig;
+
+@ApplicationScoped
+public class BigLakeCatalogValidator implements CatalogConfigValidator {
+ private static final String BIGLAKE_HOST = "biglake.googleapis.com";
+ private static final String BIGLAKE_PATH = "/iceberg/v1/restcatalog";
+ private static final String DEFAULT_BASE_LOCATION_KEY = "default-base-location";
+ private static final String QUOTA_PROJECT_HEADER = "header.x-goog-user-project";
+
+ @Override
+ public void validate(RealmConfig realmConfig, Catalog catalog) {
+ if (!(catalog instanceof ExternalCatalog externalCatalog)) {
+ return;
+ }
+
+ if (!(externalCatalog.getConnectionConfigInfo()
+ instanceof IcebergRestConnectionConfigInfo connectionConfig)) {
+ return;
+ }
+
+ // BigLake uses ICEBERG_REST with GCP authentication. This is distinct from the BIGQUERY
+ // connection type used by the optional BigQuery Metastore federation extension.
+ if (connectionConfig.getAuthenticationParameters() == null
+ || connectionConfig.getAuthenticationParameters().getAuthenticationType()
+ != AuthenticationParameters.AuthenticationTypeEnum.GCP) {
+ return;
+ }
+
+ if (!isBigLakeEndpoint(connectionConfig.getUri())) {
+ return;
+ }
+
+ validateBigLakeRemoteCatalogName(connectionConfig.getRemoteCatalogName());
+ validateBigLakeHeaders(
+ connectionConfig.getProperties(), externalCatalog.getProperties().toMap());
+ validateBigLakeStorageConfiguration(realmConfig, externalCatalog);
+ }
+
+ private static void validateBigLakeRemoteCatalogName(String remoteCatalogName) {
+ if (Strings.isNullOrEmpty(remoteCatalogName) || remoteCatalogName.trim().isEmpty()) {
+ throw new IllegalArgumentException(
+ "Invalid BigLake connectionConfigInfo.remoteCatalogName: a remote catalog or warehouse identifier is required.");
+ }
+ }
+
+ private static void validateBigLakeHeaders(
+ Map<String, String> connectionProperties, Map<String, String> catalogProperties) {
+ // Iceberg's property prefix is case-sensitive, but HTTP header names are not.
+ String quotaProject =
+ catalogProperties.entrySet().stream()
+ .filter(
+ entry ->
+ entry.getKey().startsWith("header.")
+ && QUOTA_PROJECT_HEADER.equalsIgnoreCase(entry.getKey()))
+ .findFirst()
Review Comment:
If catalog props contain both `header.x-goog-user-project` and `header.X-Goog-User-Project`, which one we validate depends on map iteration order. That matters when one is blank and the other isn't. Can we check the exact canonical key first and only fall back to the case-insensitive match?
<sub>🤖 AI-generated review comment — agents make mistakes, so please verify before acting on it.</sub>
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -189,6 +216,30 @@ public PolarisAdminService(
@NonNull PolarisAuthorizer authorizer,
@NonNull ReservedProperties reservedProperties,
@NonNull S3CredentialVendingMechanisms vendingMechanisms) {
+ this(
+ callContext,
+ resolutionManifestFactory,
+ metaStoreManager,
+ userSecretsManager,
+ serviceIdentityProvider,
+ principal,
+ authorizer,
+ reservedProperties,
+ vendingMechanisms,
+ List.of());
Review Comment:
This keeps the old constructor but quietly runs no validators. Anyone still calling it (e.g. PolarisAdminServiceTest) gets no BigLake checks, and nothing at the call site shows that. Do we need to keep it, or can those callers pass `List.of(...)` explicitly?
<sub>🤖 AI-generated review comment — agents make mistakes, so please verify before acting on it.</sub>
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -189,6 +216,30 @@ public PolarisAdminService(
@NonNull PolarisAuthorizer authorizer,
@NonNull ReservedProperties reservedProperties,
@NonNull S3CredentialVendingMechanisms vendingMechanisms) {
+ this(
+ callContext,
+ resolutionManifestFactory,
+ metaStoreManager,
+ userSecretsManager,
+ serviceIdentityProvider,
+ principal,
+ authorizer,
+ reservedProperties,
+ vendingMechanisms,
+ List.of());
+ }
+
+ public PolarisAdminService(
+ @NonNull CallContext callContext,
+ @NonNull ResolutionManifestFactory resolutionManifestFactory,
+ @NonNull PolarisMetaStoreManager metaStoreManager,
+ @NonNull UserSecretsManager userSecretsManager,
+ @NonNull ServiceIdentityProvider serviceIdentityProvider,
+ @NonNull PolarisPrincipal principal,
+ @NonNull PolarisAuthorizer authorizer,
+ @NonNull ReservedProperties reservedProperties,
+ @NonNull S3CredentialVendingMechanisms vendingMechanism,
Review Comment:
The rename `vendingMechanisms` → `vendingMechanism` doesn't seem related to this change. Can we keep the original name?
<sub>🤖 AI-generated review comment — agents make mistakes, so please verify before acting on it.</sub>
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add CDI-based validation for BigLake catalog configuration [polaris]
Posted by "davidchaava (via GitHub)" <gi...@apache.org>.
davidchaava commented on PR #5788:
URL: https://github.com/apache/polaris/pull/5788#issuecomment-6313208779
Hi @vigneshio, I’ve addressed the constructor parameter order and added the upgrade note to the changelog. I also replied to your question about the validator interface with context from the dev mailing-list discussion.
Could you take another look when you have a chance? Thanks!
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add CDI-based validation for BigLake catalog configuration [polaris]
Posted by "davidchaava (via GitHub)" <gi...@apache.org>.
davidchaava commented on code in PR #5788:
URL: https://github.com/apache/polaris/pull/5788#discussion_r4314614109
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/CatalogConfigValidator.java:
##########
@@ -0,0 +1,27 @@
+/*
+ * 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.admin;
+
+import org.apache.polaris.core.admin.model.Catalog;
+import org.apache.polaris.core.config.RealmConfig;
+
+/** Validates catalog configuration before it is created or updated. */
+public interface CatalogConfigValidator {
Review Comment:
Thanks for the suggestion. We introduced the interface following the dev mailing-list discussion, which recommended CDI-discovered validators to keep PolarisAdminService provider-neutral:
https://lists.apache.org/thread/jf36m3dd0g9qwothxlm2twjr8x87gpk3
Injecting BigLakeCatalogValidator directly would introduce a provider-specific dependency into the shared admin service. Would you be comfortable keeping the interface to preserve that separation, even with a single implementation initially?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add CDI-based validation for BigLake catalog configuration [polaris]
Posted by "vigneshio (via GitHub)" <gi...@apache.org>.
vigneshio commented on code in PR #5788:
URL: https://github.com/apache/polaris/pull/5788#discussion_r4295007261
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/CatalogConfigValidator.java:
##########
@@ -0,0 +1,27 @@
+/*
+ * 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.admin;
+
+import org.apache.polaris.core.admin.model.Catalog;
+import org.apache.polaris.core.config.RealmConfig;
+
+/** Validates catalog configuration before it is created or updated. */
+public interface CatalogConfigValidator {
Review Comment:
This adds a new extension point with just one impl, and `BigLakeCatalogValidator` is always on in runtime/service anyway. can we just inject that class from `PolarisAdminService` for now, until we actually have a second validator?
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -188,7 +191,55 @@ public PolarisAdminService(
@NonNull PolarisPrincipal principal,
@NonNull PolarisAuthorizer authorizer,
@NonNull ReservedProperties reservedProperties,
- @NonNull S3CredentialVendingMechanisms vendingMechanisms) {
+ @NonNull S3CredentialVendingMechanisms vendingMechanisms,
+ @Any Instance<CatalogConfigValidator> catalogConfigValidators) {
+ this(
+ callContext,
+ resolutionManifestFactory,
+ metaStoreManager,
+ userSecretsManager,
+ serviceIdentityProvider,
+ principal,
+ authorizer,
+ reservedProperties,
+ vendingMechanisms,
+ (Iterable<CatalogConfigValidator>) catalogConfigValidators);
+ }
+
+ public PolarisAdminService(
+ @NonNull CallContext callContext,
+ @NonNull ResolutionManifestFactory resolutionManifestFactory,
+ @NonNull PolarisMetaStoreManager metaStoreManager,
+ @NonNull UserSecretsManager userSecretsManager,
+ @NonNull ServiceIdentityProvider serviceIdentityProvider,
+ @NonNull PolarisPrincipal principal,
+ @NonNull PolarisAuthorizer authorizer,
+ @NonNull S3CredentialVendingMechanisms vendingMechanisms,
Review Comment:
The 9-arg constructor takes `vendingMechanisms` then `reservedProperties`, but the `@Inject` one has them the other way round. the extra arg on the 10-arg constructor already tells them apart, so no need for the swap. can we keep `reservedProperties` then `vendingMechanisms`, same as the inject constructor?
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -1091,6 +1150,7 @@ private void validateUpdateCatalogDiffOrThrow(
}
CatalogEntity updatedEntity = updateBuilder.build();
+ validateCatalogConfiguration(updatedEntity.asCatalog(getServiceIdentityProvider()));
Review Comment:
This also kicks in for catalogs created before this change. so if an older BigLake catalog is missing quota project or `gcsServiceAccount`, any later properties or storage update on it will 400. can we add a line under Unreleased in the CHANGELOG for 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.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add CDI-based validation for BigLake catalog configuration [polaris]
Posted by "gkalashyan-akv (via GitHub)" <gi...@apache.org>.
gkalashyan-akv commented on code in PR #5788:
URL: https://github.com/apache/polaris/pull/5788#discussion_r4348980495
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -189,6 +216,30 @@ public PolarisAdminService(
@NonNull PolarisAuthorizer authorizer,
@NonNull ReservedProperties reservedProperties,
@NonNull S3CredentialVendingMechanisms vendingMechanisms) {
+ this(
+ callContext,
+ resolutionManifestFactory,
+ metaStoreManager,
+ userSecretsManager,
+ serviceIdentityProvider,
+ principal,
+ authorizer,
+ reservedProperties,
+ vendingMechanisms,
+ List.of());
Review Comment:
Agreed. The empty collection should be explicit at manual construction sites. The convenience overload was originally retained to avoid changing existing test setup. I've prepared its removal and updated the test callers to pass their validator collection explicitly (`List.of()` where these validators are intentionally out of scope).
The production `@Inject` constructor and the BigLake-enabled fixtures remain unchanged. This cleanup is prepared locally and will be included in the next code update.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add CDI-based validation for BigLake catalog configuration [polaris]
Posted by "gkalashyan-akv (via GitHub)" <gi...@apache.org>.
gkalashyan-akv commented on code in PR #5788:
URL: https://github.com/apache/polaris/pull/5788#discussion_r4348980495
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -189,6 +216,30 @@ public PolarisAdminService(
@NonNull PolarisAuthorizer authorizer,
@NonNull ReservedProperties reservedProperties,
@NonNull S3CredentialVendingMechanisms vendingMechanisms) {
+ this(
+ callContext,
+ resolutionManifestFactory,
+ metaStoreManager,
+ userSecretsManager,
+ serviceIdentityProvider,
+ principal,
+ authorizer,
+ reservedProperties,
+ vendingMechanisms,
+ List.of());
Review Comment:
Agreed—the empty collection should be explicit at manual construction sites. The convenience overload was originally retained to avoid changing existing test setup. I've prepared its removal and updated the test callers to pass their validator collection explicitly (`List.of()` where these validators are intentionally out of scope).
The production `@Inject` constructor and the BigLake-enabled fixtures remain unchanged. This cleanup is prepared locally and will be included in the next code update.
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -189,6 +216,30 @@ public PolarisAdminService(
@NonNull PolarisAuthorizer authorizer,
@NonNull ReservedProperties reservedProperties,
@NonNull S3CredentialVendingMechanisms vendingMechanisms) {
+ this(
+ callContext,
+ resolutionManifestFactory,
+ metaStoreManager,
+ userSecretsManager,
+ serviceIdentityProvider,
+ principal,
+ authorizer,
+ reservedProperties,
+ vendingMechanisms,
+ List.of());
+ }
+
+ public PolarisAdminService(
+ @NonNull CallContext callContext,
+ @NonNull ResolutionManifestFactory resolutionManifestFactory,
+ @NonNull PolarisMetaStoreManager metaStoreManager,
+ @NonNull UserSecretsManager userSecretsManager,
+ @NonNull ServiceIdentityProvider serviceIdentityProvider,
+ @NonNull PolarisPrincipal principal,
+ @NonNull PolarisAuthorizer authorizer,
+ @NonNull ReservedProperties reservedProperties,
+ @NonNull S3CredentialVendingMechanisms vendingMechanism,
Review Comment:
Agreed. I've restored the plural `vendingMechanisms` name for both the constructor parameter and its field assignment in the prepared cleanup. This will be included with the constructor update; no behavior change is intended.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add CDI-based validation for BigLake catalog configuration [polaris]
Posted by "dimas-b (via GitHub) via issues" <is...@polaris.apache.org>.
dimas-b commented on code in PR #5788:
URL: https://github.com/apache/polaris/pull/5788#discussion_r4429304858
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/CatalogConfigValidator.java:
##########
@@ -0,0 +1,27 @@
+/*
+ * 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.admin;
+
+import org.apache.polaris.core.admin.model.Catalog;
+import org.apache.polaris.core.config.RealmConfig;
+
+/** Validates catalog configuration before it is created or updated. */
+public interface CatalogConfigValidator {
+ void validate(RealmConfig realmConfig, Catalog catalog);
Review Comment:
`RealmConfig` is contextual. If a validator needs it, it should be possible to inject via CDI as opposed to forcing callers to provide it.
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -838,6 +873,7 @@ public PolarisEntity createCatalog(CreateCatalogRequest catalogRequest) {
validateS3CredentialVendingMechanism(catalog.getStorageConfigInfo());
CatalogEntity entity = CatalogEntity.fromCatalog(realmConfig, catalog);
+ validateCatalogConfiguration(catalog);
Review Comment:
Why not use `entity` for validation? It represents the internal, source-of-truth data as opposed to API-level data of `Catalog` 🤔
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -188,7 +191,32 @@ public PolarisAdminService(
@NonNull PolarisPrincipal principal,
@NonNull PolarisAuthorizer authorizer,
@NonNull ReservedProperties reservedProperties,
- @NonNull S3CredentialVendingMechanisms vendingMechanisms) {
+ @NonNull S3CredentialVendingMechanisms vendingMechanisms,
+ @Any Instance<CatalogConfigValidator> catalogConfigValidators) {
Review Comment:
This is a request-scoped bean, so it is used frequently, but the list of validators is static.
Could we resolve them once per startup? Cf. `S3CredentialVendingMechanisms`
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add CDI-based validation for BigLake catalog configuration [polaris]
Posted by "davidchaava (via GitHub)" <gi...@apache.org>.
davidchaava commented on code in PR #5788:
URL: https://github.com/apache/polaris/pull/5788#discussion_r4314629579
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -1091,6 +1150,7 @@ private void validateUpdateCatalogDiffOrThrow(
}
CatalogEntity updatedEntity = updateBuilder.build();
+ validateCatalogConfiguration(updatedEntity.asCatalog(getServiceIdentityProvider()));
Review Comment:
Thanks for pointing this out. I added an upgrade note under Unreleased in CHANGELOG.md explaining that validation also applies to existing BigLake catalogs on update. It lists the required settings and clarifies that updates are rejected until any missing configuration is supplied.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add CDI-based validation for BigLake catalog configuration [polaris]
Posted by "davidchaava (via GitHub)" <gi...@apache.org>.
davidchaava commented on code in PR #5788:
URL: https://github.com/apache/polaris/pull/5788#discussion_r4376021436
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/BigLakeCatalogValidator.java:
##########
@@ -0,0 +1,158 @@
+/*
+ * 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.admin;
+
+import com.google.common.base.Strings;
+import jakarta.enterprise.context.ApplicationScoped;
+import java.net.URI;
+import java.util.Map;
+import org.apache.polaris.core.admin.model.AuthenticationParameters;
+import org.apache.polaris.core.admin.model.Catalog;
+import org.apache.polaris.core.admin.model.ExternalCatalog;
+import org.apache.polaris.core.admin.model.GcpStorageConfigInfo;
+import org.apache.polaris.core.admin.model.IcebergRestConnectionConfigInfo;
+import org.apache.polaris.core.admin.model.StorageConfigInfo;
+import org.apache.polaris.core.config.FeatureConfiguration;
+import org.apache.polaris.core.config.RealmConfig;
+
+@ApplicationScoped
+public class BigLakeCatalogValidator implements CatalogConfigValidator {
+ private static final String BIGLAKE_HOST = "biglake.googleapis.com";
+ private static final String BIGLAKE_PATH = "/iceberg/v1/restcatalog";
+ private static final String DEFAULT_BASE_LOCATION_KEY = "default-base-location";
+ private static final String QUOTA_PROJECT_HEADER = "header.x-goog-user-project";
+
+ @Override
+ public void validate(RealmConfig realmConfig, Catalog catalog) {
+ if (!(catalog instanceof ExternalCatalog externalCatalog)) {
+ return;
+ }
+
+ if (!(externalCatalog.getConnectionConfigInfo()
+ instanceof IcebergRestConnectionConfigInfo connectionConfig)) {
+ return;
+ }
+
+ // BigLake uses ICEBERG_REST with GCP authentication. This is distinct from the BIGQUERY
+ // connection type used by the optional BigQuery Metastore federation extension.
+ if (connectionConfig.getAuthenticationParameters() == null
+ || connectionConfig.getAuthenticationParameters().getAuthenticationType()
+ != AuthenticationParameters.AuthenticationTypeEnum.GCP) {
+ return;
+ }
+
+ if (!isBigLakeEndpoint(connectionConfig.getUri())) {
+ return;
+ }
+
+ validateBigLakeRemoteCatalogName(connectionConfig.getRemoteCatalogName());
+ validateBigLakeHeaders(
+ connectionConfig.getProperties(), externalCatalog.getProperties().toMap());
+ validateBigLakeStorageConfiguration(realmConfig, externalCatalog);
+ }
+
+ private static void validateBigLakeRemoteCatalogName(String remoteCatalogName) {
+ if (Strings.isNullOrEmpty(remoteCatalogName) || remoteCatalogName.trim().isEmpty()) {
+ throw new IllegalArgumentException(
+ "Invalid BigLake connectionConfigInfo.remoteCatalogName: a remote catalog or warehouse identifier is required.");
+ }
+ }
+
+ private static void validateBigLakeHeaders(
+ Map<String, String> connectionProperties, Map<String, String> catalogProperties) {
+ // Iceberg's property prefix is case-sensitive, but HTTP header names are not.
+ String quotaProject =
+ catalogProperties.entrySet().stream()
+ .filter(
+ entry ->
+ entry.getKey().startsWith("header.")
+ && QUOTA_PROJECT_HEADER.equalsIgnoreCase(entry.getKey()))
+ .findFirst()
Review Comment:
The canonical key now takes precedence, including when its value is blank. Case-insensitive lookup is used only when the canonical key is absent. Added regression tests covering conflicting case variants.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org