You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "sririshindra (via GitHub)" <gi...@apache.org> on 2026/10/06 23:36:12 UTC
[PR] Spec: Add named storage configurations to the management API [polaris]
sririshindra opened a new pull request, #5721:
URL: https://github.com/apache/polaris/pull/5721
## Why
This is the API part of #5556, split out so it can be reviewed first, as a reviewer suggested.
#5556 stays open and shows the whole change. Once the API shape here is agreed, the
implementation follows in smaller PRs on top of it.
Background: a Polaris catalog can hold only one storage configuration today. The design discussed
on dev@ ([thread](https://lists.apache.org/thread/v7ygzp5syph4lcwblodjjc16xzcpntq9)) lets a catalog
hold extra **named** storage configurations next to its default one. This PR only adds the
API definition. It does not store the configurations and does not change credential vending.
Related: #5556, #4023, #3409.
## What changes
**`spec/polaris-management-service.yml`**
- New optional `storageConfigInfos: [StorageConfigInfo]` on `Catalog` (the create input and every
response) and on `UpdateCatalogRequest`. The existing singular `storageConfigInfo` stays the
catalog's default configuration. Each entry must have a non-empty `storageName`.
- Update semantics: if the array is omitted, the existing set stays as it is. If it is present, it
replaces the whole set. An empty array removes all named configurations.
- New `StorageConfigInfos` schema (the list response).
- New endpoints for managing one configuration at a time:
- `GET /catalogs/{catalogName}/storage-configs`: list the named configurations. The default
is not included.
- `GET /catalogs/{catalogName}/storage-configs/{storageConfigName}`
- `PUT /catalogs/{catalogName}/storage-configs/{storageConfigName}`: create or replace one
entry. If the body sets `storageName`, it must equal the path name.
- `DELETE /catalogs/{catalogName}/storage-configs/{storageConfigName}`
The documented status codes are 400 (invalid configuration or disallowed change), 403, 404
(unknown catalog or name) and 409 (concurrent catalog change, retry).
**`api/management-model/build.gradle.kts`**: `openApiNullable=false`. `storageConfigInfos` is
`nullable: true`, which keeps an omitted array different from an empty one: the generated
default is `null` instead of `new ArrayList<>()`. With `openApiNullable=false`, the field is a
plain `List<StorageConfigInfo>` and does not need `JsonNullable`. No other schema in this spec
uses `nullable`, so no other generated type changes.
**Constructor call sites**: the new property adds a trailing argument to the all-args
constructors of `Catalog` and `UpdateCatalogRequest`. Existing call sites pass `null`; this
accounts for all the one-line test changes.
## Behavior with this PR alone
- The four new endpoints return **501 Not Implemented**. That is the generated default in
`PolarisCatalogsApiService`, and `PolarisServiceImpl` does not override it yet.
- `storageConfigInfos` is accepted in a request body but ignored by the server. Responses never
contain it: it stays `null`, and the model is annotated `JsonInclude.NON_NULL`.
- Nothing else changes. The catalog JSON for existing catalogs is byte-identical.
## Planned follow-ups (each a separate PR, from #5556)
1. Store and validate named configurations through `storageConfigInfos` on create, update and
get, plus the feature flag `ENABLE_NAMED_STORAGE_CONFIGURATIONS` (default off).
2. Implement the `/storage-configs` endpoints on top of that.
3. Later: resolving a configuration by name during credential vending, and the binding API for
namespaces and tables. These are not part of this spec.
## Testing
- No new tests: this PR adds no server behavior. Existing tests that build `Catalog` and
`UpdateCatalogRequest` were updated for the new constructor argument.
- Ran locally: `:polaris-api-management-model:check`, `:polaris-api-management-service:check`,
and `ManagementServiceTest`, `PolarisOverlappingCatalogTest`,
`S3CredentialVendingMechanismRoutesTest`, `PolarisS3InteroperabilityTest`. Compiled the
test sources of `polaris-runtime-service` (including `cloudTest`), `polaris-tests` and
`polaris-extensions-semantic-models`. CI runs the full suite.
## Checklist
- [x] ๐ก๏ธ Don't disclose security issues! (contact security@apache.org)
- [x] ๐ Clearly explained why the changes are needed, or linked related issues: Related to #5556
- [x] ๐งช Added/updated tests with good coverage, or manually tested (and explained how)
- [x] ๐ก Added comments for complex logic
- [ ] ๐งพ Updated `CHANGELOG.md` (if needed): not needed yet, there is no server behavior; the entry comes with the implementation PR
- [ ] ๐ Updated documentation in `site/content/in-dev/unreleased` (if needed): deferred until named configurations can be resolved
๐ค Generated with [Claude Code](https://claude.com/claude-code)
--
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] Spec: Add named storage configurations to the management API [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5721:
URL: https://github.com/apache/polaris/pull/5721#discussion_r4350205237
##########
spec/polaris-management-service.yml:
##########
@@ -1111,7 +1116,7 @@ components:
example: "For AWS [s3://bucketname/prefix/], for AZURE [abfss://container@storageaccount.blob.core.windows.net/prefix/], for GCP [gs://bucketname/prefix/]"
storageName:
type: string
- description: An optional name referencing a server-side storage configuration
+ description: An optional name referencing a server-side storage configuration. This is independent of the configuration name under which a StorageConfigInfo is held in Catalog.storageConfigurations, so several entries may share a storageName or leave it unset.
Review Comment:
+1 to mentioning that `storageName` can be used for base credential lookup in the default Polaris Server.
However, I'd prefer to avoid referencing specific configuration properties in this API spec. Different server builds may choose to use different mechanisms for finding base credentials.
Also, `storageName` is not limited to base credential lookup. It is exposed to pluggable `PolarisStorageIntegration` components. Let's keep the usage options open for customization.
Essentially, `storageName` is a plugin point for connecting `StorageConfigInfo` to build-specific configuration.
My proposal:
```
This name is meant for associating StorageConfigInfo objects to deployment-specific
configuration. The interpretation of this name depends on specific runtime components
that are included in a server to support interacting with storage systems. Standard Apache
Polaris servers use this name for finding base S3 and STS credentials.
Note: this name is optional and unrelated to the configuration name under which a StorageConfigInfo
is stored in Catalog.storageConfigurations.
```
WDYT?
--
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] Spec: Add named storage configurations to the management API [polaris]
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on code in PR #5721:
URL: https://github.com/apache/polaris/pull/5721#discussion_r4307132598
##########
spec/polaris-management-service.yml:
##########
@@ -879,6 +989,12 @@ components:
description: The version of the catalog object used to determine if the catalog metadata has changed
storageConfigInfo:
$ref: "#/components/schemas/StorageConfigInfo"
+ storageConfigInfos:
Review Comment:
I changed `storageConfigInfos` to a map from configuration name to `StorageConfigInfo`. The key is the name, so that's the identity now. `storageName` inside an entry still just picks the credential set, and it's optional, so entries can share one or leave it unset. The default config still has no name.
Keys are meant to match `^[a-zA-Z0-9_-]{1,128}$`. OpenAPI 3.0 can't put a pattern on map keys, so that's in the description and the server will enforce it.
##########
spec/polaris-management-service.yml:
##########
@@ -589,6 +589,105 @@ paths:
404:
description: "The principal role does not exist"
+ /catalogs/{catalogName}/storage-configs:
Review Comment:
Agreed, removed. Create, get and update catalog already cover all of it, and `updateCatalog` checks `currentEntityVersion`. The endpoints can come back in Phase 2 when configs become entities.
##########
spec/polaris-management-service.yml:
##########
@@ -879,6 +989,12 @@ components:
description: The version of the catalog object used to determine if the catalog metadata has changed
storageConfigInfo:
$ref: "#/components/schemas/StorageConfigInfo"
+ storageConfigInfos:
Review Comment:
Makes sense. I dropped the endpoints, so this PR now only adds the field on the catalog.
##########
spec/polaris-management-service.yml:
##########
@@ -589,6 +589,105 @@ paths:
404:
description: "The principal role does not exist"
+ /catalogs/{catalogName}/storage-configs:
+ parameters:
Review Comment:
The list endpoint is gone from this PR. When it comes back with Phase 2 it will be paginated from the start, like the catalog API lists (`pageToken` / `next-page-token`).
##########
spec/polaris-management-service.yml:
##########
@@ -589,6 +589,105 @@ paths:
404:
description: "The principal role does not exist"
+ /catalogs/{catalogName}/storage-configs:
+ parameters:
+ - name: catalogName
+ in: path
+ description: The catalog that holds the named storage configurations
+ required: true
+ schema:
+ type: string
+ minLength: 1
+ maxLength: 256
+ pattern: '^(?!\s*[sS][yY][sS][tT][eE][mM]\$).*$'
+ get:
+ operationId: listStorageConfigs
+ description: List the named storage configurations held by a catalog. The catalog's default storageConfigInfo is not included.
+ responses:
+ 200:
+ description: The named storage configurations of the catalog
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/StorageConfigInfos"
+ 403:
+ description: "The caller does not have permission to read catalog details"
+ 404:
+ description: "The catalog does not exist"
+
+ /catalogs/{catalogName}/storage-configs/{storageConfigName}:
+ parameters:
+ - name: catalogName
+ in: path
+ description: The catalog that holds the named storage configuration
+ required: true
+ schema:
+ type: string
+ minLength: 1
+ maxLength: 256
+ pattern: '^(?!\s*[sS][yY][sS][tT][eE][mM]\$).*$'
+ - name: storageConfigName
+ in: path
+ description: The name of the named storage configuration. Surrounding whitespace is trimmed; the result must match ^[a-zA-Z0-9_-]{1,128}$.
+ required: true
+ schema:
+ type: string
+ minLength: 1
+ maxLength: 128
+ get:
+ operationId: getStorageConfig
+ description: Get one named storage configuration of a catalog
+ responses:
+ 200:
+ description: The named storage configuration
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/StorageConfigInfo"
+ 403:
+ description: "The caller does not have permission to read catalog details"
+ 404:
+ description: "The catalog or the named storage configuration does not exist"
+
+ put:
+ operationId: putStorageConfig
+ description: Create or replace one named storage configuration of a catalog, leaving the catalog's other storage configurations unchanged. If the payload sets storageName, it must equal the storageConfigName path parameter; if it does not, the path parameter is used as the name.
+ requestBody:
+ description: The storage configuration to store under this name
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/StorageConfigInfo"
Review Comment:
Agreed. The `PUT` took no version, so last write would have won. With the routes gone, changes go through `updateCatalog`, which already checks `currentEntityVersion`. In Phase 2 each config will have its own version.
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java:
##########
@@ -450,7 +450,10 @@ public void testUpdateCatalogWithoutDefaultBaseLocationInUpdate() {
// default-base-location.
UpdateCatalogRequest updateRequest =
new UpdateCatalogRequest(
- fetchedCatalog.getEntityVersion(), Map.of("foo", "bar"), null /* storageConfigIno */);
+ fetchedCatalog.getEntityVersion(),
+ Map.of("foo", "bar"),
+ null /* storageConfigIno */,
Review Comment:
Fixed.
--
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] Spec: Add named storage configurations to the management API [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5721:
URL: https://github.com/apache/polaris/pull/5721#discussion_r4350205237
##########
spec/polaris-management-service.yml:
##########
@@ -1111,7 +1116,7 @@ components:
example: "For AWS [s3://bucketname/prefix/], for AZURE [abfss://container@storageaccount.blob.core.windows.net/prefix/], for GCP [gs://bucketname/prefix/]"
storageName:
type: string
- description: An optional name referencing a server-side storage configuration
+ description: An optional name referencing a server-side storage configuration. This is independent of the configuration name under which a StorageConfigInfo is held in Catalog.storageConfigurations, so several entries may share a storageName or leave it unset.
Review Comment:
+1 to mentioning that `storageName` can be used for base credential lookup in the default Polaris Server.
However, I'd prefer to avoid referencing specific configuration properties in this API spec. Different server builds may choose to use different mechanisms for finding base credentials.
Also, `storageName` is not limited to base credential lookup. It is exposed to pluggable `PolarisStorageIntegration` components. Let's keep the usage options open for customization.
Essentially, `storageName` is a plugin point for connecting `StorageConfigInfo` to build-specific configuration.
My proposal:
```
This name is meant for associating StorageConfigInfo objects to deployment-specific
configuration. The interpretation of this name depends on specific runtime components that
are included in a server to support interacting with storage systems. Standard Apache Polaris
servers use this name for finding base S3 and STS credentials.
Note: this name is optional and unrelated to the configuration name under which a StorageConfigInfo
is stored in Catalog.storageConfigurations.
```
WDYT?
--
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] Spec: Add named storage configurations to the management API [polaris]
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on PR #5721:
URL: https://github.com/apache/polaris/pull/5721#issuecomment-6123997296
> Is this ready for review? ๐
Yes, sorry for the delay.
--
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] Spec: Add named storage configurations to the management API [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5721:
URL: https://github.com/apache/polaris/pull/5721#discussion_r4300357237
##########
spec/polaris-management-service.yml:
##########
@@ -879,6 +989,12 @@ components:
description: The version of the catalog object used to determine if the catalog metadata has changed
storageConfigInfo:
$ref: "#/components/schemas/StorageConfigInfo"
+ storageConfigInfos:
Review Comment:
I think this field contradicts the new endpoints (above).
If the storage config is part of the catalog object, we do not need the new endpoints.
If we use the new endpoints, the configs should be independent entities (referenced by name).
##########
spec/polaris-management-service.yml:
##########
@@ -589,6 +589,105 @@ paths:
404:
description: "The principal role does not exist"
+ /catalogs/{catalogName}/storage-configs:
+ parameters:
+ - name: catalogName
+ in: path
+ description: The catalog that holds the named storage configurations
+ required: true
+ schema:
+ type: string
+ minLength: 1
+ maxLength: 256
+ pattern: '^(?!\s*[sS][yY][sS][tT][eE][mM]\$).*$'
+ get:
+ operationId: listStorageConfigs
+ description: List the named storage configurations held by a catalog. The catalog's default storageConfigInfo is not included.
+ responses:
+ 200:
+ description: The named storage configurations of the catalog
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/StorageConfigInfos"
+ 403:
+ description: "The caller does not have permission to read catalog details"
+ 404:
+ description: "The catalog does not exist"
+
+ /catalogs/{catalogName}/storage-configs/{storageConfigName}:
+ parameters:
+ - name: catalogName
+ in: path
+ description: The catalog that holds the named storage configuration
+ required: true
+ schema:
+ type: string
+ minLength: 1
+ maxLength: 256
+ pattern: '^(?!\s*[sS][yY][sS][tT][eE][mM]\$).*$'
+ - name: storageConfigName
+ in: path
+ description: The name of the named storage configuration. Surrounding whitespace is trimmed; the result must match ^[a-zA-Z0-9_-]{1,128}$.
+ required: true
+ schema:
+ type: string
+ minLength: 1
+ maxLength: 128
+ get:
+ operationId: getStorageConfig
+ description: Get one named storage configuration of a catalog
+ responses:
+ 200:
+ description: The named storage configuration
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/StorageConfigInfo"
+ 403:
+ description: "The caller does not have permission to read catalog details"
+ 404:
+ description: "The catalog or the named storage configuration does not exist"
+
+ put:
+ operationId: putStorageConfig
+ description: Create or replace one named storage configuration of a catalog, leaving the catalog's other storage configurations unchanged. If the payload sets storageName, it must equal the storageConfigName path parameter; if it does not, the path parameter is used as the name.
+ requestBody:
+ description: The storage configuration to store under this name
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/StorageConfigInfo"
Review Comment:
We probably need entity version for concurrency control.
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java:
##########
@@ -450,7 +450,10 @@ public void testUpdateCatalogWithoutDefaultBaseLocationInUpdate() {
// default-base-location.
UpdateCatalogRequest updateRequest =
new UpdateCatalogRequest(
- fetchedCatalog.getEntityVersion(), Map.of("foo", "bar"), null /* storageConfigIno */);
+ fetchedCatalog.getEntityVersion(),
+ Map.of("foo", "bar"),
+ null /* storageConfigIno */,
Review Comment:
"Ino" -> "Info"?
##########
spec/polaris-management-service.yml:
##########
@@ -879,6 +989,12 @@ components:
description: The version of the catalog object used to determine if the catalog metadata has changed
storageConfigInfo:
$ref: "#/components/schemas/StorageConfigInfo"
+ storageConfigInfos:
Review Comment:
Yes, we need to distinguish the config name from the "base" storage credential name.
##########
spec/polaris-management-service.yml:
##########
@@ -589,6 +589,105 @@ paths:
404:
description: "The principal role does not exist"
+ /catalogs/{catalogName}/storage-configs:
+ parameters:
Review Comment:
Please add pagination upfront. We do not know how many configs users may have, so we should make sure clients do not assume all of them can be obtained in one response.
##########
spec/polaris-management-service.yml:
##########
@@ -589,6 +589,105 @@ paths:
404:
description: "The principal role does not exist"
+ /catalogs/{catalogName}/storage-configs:
Review Comment:
IIRC our `dev` discussions, this was "phase 2", where we move storage configs to top-level entities under a catalog.
--
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] Spec: Add named storage configurations to the management API [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5721:
URL: https://github.com/apache/polaris/pull/5721#discussion_r4330506654
##########
spec/polaris-management-service.yml:
##########
@@ -879,6 +879,12 @@ components:
description: The version of the catalog object used to determine if the catalog metadata has changed
storageConfigInfo:
$ref: "#/components/schemas/StorageConfigInfo"
+ storageConfigInfos:
Review Comment:
nit: I'd call this `storageConfigurations`. It would be easier to distinguish from the singular `storageConfigInfo` property.
##########
spec/polaris-management-service.yml:
##########
@@ -879,6 +879,12 @@ components:
description: The version of the catalog object used to determine if the catalog metadata has changed
storageConfigInfo:
$ref: "#/components/schemas/StorageConfigInfo"
+ storageConfigInfos:
+ type: object
+ nullable: true
Review Comment:
We should probably use type arrays after #5819
--
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] Spec: Add named storage configurations to the management API [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5721:
URL: https://github.com/apache/polaris/pull/5721#discussion_r4349735319
##########
spec/polaris-management-service.yml:
##########
@@ -1111,7 +1116,7 @@ components:
example: "For AWS [s3://bucketname/prefix/], for AZURE [abfss://container@storageaccount.blob.core.windows.net/prefix/], for GCP [gs://bucketname/prefix/]"
storageName:
type: string
- description: An optional name referencing a server-side storage configuration
+ description: An optional name referencing a server-side storage configuration. This is independent of the configuration name under which a StorageConfigInfo is held in Catalog.storageConfigurations, so several entries may share a storageName or leave it unset.
Review Comment:
Thanks for accepting to separate this field from the storage configuration name. I also appreciate that you added a sentence to disambiguate the two concepts.
I would go even further and rephrase the description more deeply, as follows:
> An optional name referencing a storage credential configuration section in the Polaris server configuration. If present, Polaris will use the credentials declared under `polaris.storage.aws.<name>.*` instead of the default ones, declared under `polaris.storage.aws.*`, when contacting STS. This is currently only supported for AWS.
> Note: this name is unrelated to the configuration name under which a `StorageConfigInfo` is stored in `Catalog.storageConfigurations`.
--
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] Spec: Add named storage configurations to the management API [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #5721:
URL: https://github.com/apache/polaris/pull/5721#issuecomment-6028103782
Is this ready for review? :sweat_smile:
--
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] Spec: Add named storage configurations to the management API [polaris]
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on code in PR #5721:
URL: https://github.com/apache/polaris/pull/5721#discussion_r4201605791
##########
spec/polaris-management-service.yml:
##########
@@ -879,6 +989,12 @@ components:
description: The version of the catalog object used to determine if the catalog metadata has changed
storageConfigInfo:
$ref: "#/components/schemas/StorageConfigInfo"
+ storageConfigInfos:
+ type: array
+ nullable: true
+ description: Optional list of additional named storage configurations held by the catalog, alongside its default storageConfigInfo. Each entry is identified by its storageName, which is required. Names are trimmed, must match ^[a-zA-Z0-9_-]{1,128}$, are case-sensitive, must be unique within the list, and must differ from the storageName of the default storageConfigInfo. Unlike the default storageConfigInfo, each entry must list at least one allowedLocations entry.
Review Comment:
A note on why named entries must list at least one allowed location, while the default configuration doesn't have to:
- When the default `storageConfigInfo` has no `allowedLocations`, Polaris uses the catalog's `default-base-location` instead (`CatalogEntity.Builder#processStorageConfigurationInfo`). A named entry has no equivalent to fall back on. It usually points at different storage than the catalog default (another bucket, another account, sometimes another storage type), so borrowing the catalog's base location would either allow the wrong place or fail the storage-type prefix check.
- An empty list doesn't mean "anywhere". `StorageLocationValidator.validateAllowedLocations` accepts a location only if it is under one of the allowed locations. With an empty list, every table create, commit and credential request that uses the entry would fail with 403. It's better to reject the entry with a 400 when it is defined than to have it fail later on every table that uses it.
- The cross-catalog overlap check also works from `allowedLocations`, so an entry without any would never be checked against other catalogs.
Happy to change this if people would prefer a different rule.
--
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] Spec: Add named storage configurations to the management API [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5721:
URL: https://github.com/apache/polaris/pull/5721#discussion_r4350205237
##########
spec/polaris-management-service.yml:
##########
@@ -1111,7 +1116,7 @@ components:
example: "For AWS [s3://bucketname/prefix/], for AZURE [abfss://container@storageaccount.blob.core.windows.net/prefix/], for GCP [gs://bucketname/prefix/]"
storageName:
type: string
- description: An optional name referencing a server-side storage configuration
+ description: An optional name referencing a server-side storage configuration. This is independent of the configuration name under which a StorageConfigInfo is held in Catalog.storageConfigurations, so several entries may share a storageName or leave it unset.
Review Comment:
+1 to mentioning that `storageName` can be used for base credential lookup in the default Polaris Server.
However, I'd prefer to avoid referencing specific configuration properties in this API spec. Different server builds may choose to use different mechanisms for finding base credentials.
Also, `storageName` is not limited to base credential lookup. It is exposed to pluggable `PolarisStorageIntegration` components. Let's keep the usage options open for customization.
Essentially, `storageName` is a plugin point for connecting `StorageConfigInfo` to build-specific configuration.
My proposal:
```
This name is meant for associating StorageConfigInfo objects to deployment-specific
configuration. The interpretation of this name depends on specific runtime components
that are included in a server to support interacting with storage systems. Standard Apache
Polaris servers use this name for finding base S3 and STS credentials.
Note: this name is optional and unrelated to the configuration name under which a
StorageConfigInfo is stored in Catalog.storageConfigurations.
```
WDYT?
--
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] Spec: Add named storage configurations to the management API [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5721:
URL: https://github.com/apache/polaris/pull/5721#discussion_r4340796688
##########
spec/polaris-management-service.yml:
##########
@@ -879,6 +879,11 @@ components:
description: The version of the catalog object used to determine if the catalog metadata has changed
storageConfigInfo:
$ref: "#/components/schemas/StorageConfigInfo"
+ storageConfigurations:
+ type: [ object, "null" ]
+ description: Optional additional named storage configurations held by the catalog, alongside its default storageConfigInfo, keyed by configuration name. Each key must match ^[a-zA-Z0-9_-]{1,128}$ and is case-sensitive. The default storageConfigInfo is not part of this map and has no configuration name. The storageName of an entry keeps its meaning of selecting a server-side storage configuration and is independent of the key, so entries may share a storageName or leave it unset. Unlike the default storageConfigInfo, each entry must list at least one allowedLocations entry.
Review Comment:
Leaving `storageName` out of this description make sense :+1:
I'd still keep `names` in the first sentence because it indicates how the configurations are distinguished from each other.
`A map from configuration name to StorageConfigInfo` - I'm not sure what this means :thinking:
--
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] Spec: Add named storage configurations to the management API [polaris]
Posted by "vigneshio (via GitHub)" <gi...@apache.org>.
vigneshio commented on code in PR #5721:
URL: https://github.com/apache/polaris/pull/5721#discussion_r4276276650
##########
spec/polaris-management-service.yml:
##########
@@ -879,6 +989,12 @@ components:
description: The version of the catalog object used to determine if the catalog metadata has changed
storageConfigInfo:
$ref: "#/components/schemas/StorageConfigInfo"
+ storageConfigInfos:
Review Comment:
`storageName` already picks the deployment time credential set (`polaris.storage.aws.<name>` / `RESOLVE_CREDENTIALS_BY_STORAGE_NAME`). here its also the identity for each named entry, and it can't reuse the default config's name..
Can we settle the identity part before this spec lands? if one field keeps doing both jobs, multiple configs can't share one credential set. And changing it later would be a breaking management API change.
WDYT?? @dimas-b @flyrain @adutra
##########
spec/polaris-management-service.yml:
##########
@@ -589,6 +589,105 @@ paths:
404:
description: "The principal role does not exist"
+ /catalogs/{catalogName}/storage-configs:
Review Comment:
Do we really need these item routes in the same spec as `Catalog.storageConfigInfos`? create/get/update catalog already carry the list. Same thing Dimitri asked on #5556
--
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] Spec: Add named storage configurations to the management API [polaris]
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on code in PR #5721:
URL: https://github.com/apache/polaris/pull/5721#discussion_r4344301566
##########
spec/polaris-management-service.yml:
##########
@@ -879,6 +879,11 @@ components:
description: The version of the catalog object used to determine if the catalog metadata has changed
storageConfigInfo:
$ref: "#/components/schemas/StorageConfigInfo"
+ storageConfigurations:
+ type: [ object, "null" ]
+ description: Optional additional named storage configurations held by the catalog, alongside its default storageConfigInfo, keyed by configuration name. Each key must match ^[a-zA-Z0-9_-]{1,128}$ and is case-sensitive. The default storageConfigInfo is not part of this map and has no configuration name. The storageName of an entry keeps its meaning of selecting a server-side storage configuration and is independent of the key, so entries may share a storageName or leave it unset. Unlike the default storageConfigInfo, each entry must list at least one allowedLocations entry.
Review Comment:
Updated in b0a89e4ca9. I dropped the `storageName` mention from the `Catalog.storageConfigurations` description and kept "names" in the first sentence. I spelled out the map as "keys are configuration names, values are StorageConfigInfo objects", which should answer @dimas-b's question. The independence of `storageName` from the configuration name is now documented on `StorageConfigInfo.storageName`. Happy to adjust the wording if it's still unclear.
--
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] Spec: Add named storage configurations to the management API [polaris]
Posted by "sririshindra (via GitHub) via issues" <is...@polaris.apache.org>.
sririshindra commented on PR #5721:
URL: https://github.com/apache/polaris/pull/5721#issuecomment-6561427471
Closing this in favor of #5943. As discussed on dev@ (https://lists.apache.org/thread/ytdc13npxq4m0dz54vm1g7n3ygpywf5q), we're skipping the approach of keeping several storage configurations inside the catalog and making them dedicated, catalog-scoped entities instead. Thanks to everyone who reviewed this.
--
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] Spec: Add named storage configurations to the management API [polaris]
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on code in PR #5721:
URL: https://github.com/apache/polaris/pull/5721#discussion_r4330629219
##########
spec/polaris-management-service.yml:
##########
@@ -879,6 +879,12 @@ components:
description: The version of the catalog object used to determine if the catalog metadata has changed
storageConfigInfo:
$ref: "#/components/schemas/StorageConfigInfo"
+ storageConfigInfos:
+ type: object
+ nullable: true
Review Comment:
done!
##########
spec/polaris-management-service.yml:
##########
@@ -879,6 +879,12 @@ components:
description: The version of the catalog object used to determine if the catalog metadata has changed
storageConfigInfo:
$ref: "#/components/schemas/StorageConfigInfo"
+ storageConfigInfos:
Review Comment:
done!
--
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] Spec: Add named storage configurations to the management API [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5721:
URL: https://github.com/apache/polaris/pull/5721#discussion_r4331923729
##########
spec/polaris-management-service.yml:
##########
@@ -879,6 +879,11 @@ components:
description: The version of the catalog object used to determine if the catalog metadata has changed
storageConfigInfo:
$ref: "#/components/schemas/StorageConfigInfo"
+ storageConfigurations:
+ type: [ object, "null" ]
+ description: Optional additional named storage configurations held by the catalog, alongside its default storageConfigInfo, keyed by configuration name. Each key must match ^[a-zA-Z0-9_-]{1,128}$ and is case-sensitive. The default storageConfigInfo is not part of this map and has no configuration name. The storageName of an entry keeps its meaning of selecting a server-side storage configuration and is independent of the key, so entries may share a storageName or leave it unset. Unlike the default storageConfigInfo, each entry must list at least one allowedLocations entry.
Review Comment:
I'm OK with the key pattern written in the description.
--
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] Spec: Add named storage configurations to the management API [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5721:
URL: https://github.com/apache/polaris/pull/5721#discussion_r4340867514
##########
spec/polaris-management-service.yml:
##########
@@ -879,6 +879,11 @@ components:
description: The version of the catalog object used to determine if the catalog metadata has changed
storageConfigInfo:
$ref: "#/components/schemas/StorageConfigInfo"
+ storageConfigurations:
+ type: [ object, "null" ]
+ description: Optional additional named storage configurations held by the catalog, alongside its default storageConfigInfo, keyed by configuration name. Each key must match ^[a-zA-Z0-9_-]{1,128}$ and is case-sensitive. The default storageConfigInfo is not part of this map and has no configuration name. The storageName of an entry keeps its meaning of selecting a server-side storage configuration and is independent of the key, so entries may share a storageName or leave it unset. Unlike the default storageConfigInfo, each entry must list at least one allowedLocations entry.
Review Comment:
That's what storageConfigurations is. It is a map with key(configuraiton name) and value(a StorageConfigInfo). Open to any wording suggestion.
--
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] Spec: Add named storage configurations to the management API [polaris]
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on code in PR #5721:
URL: https://github.com/apache/polaris/pull/5721#discussion_r4344301566
##########
spec/polaris-management-service.yml:
##########
@@ -879,6 +879,11 @@ components:
description: The version of the catalog object used to determine if the catalog metadata has changed
storageConfigInfo:
$ref: "#/components/schemas/StorageConfigInfo"
+ storageConfigurations:
+ type: [ object, "null" ]
+ description: Optional additional named storage configurations held by the catalog, alongside its default storageConfigInfo, keyed by configuration name. Each key must match ^[a-zA-Z0-9_-]{1,128}$ and is case-sensitive. The default storageConfigInfo is not part of this map and has no configuration name. The storageName of an entry keeps its meaning of selecting a server-side storage configuration and is independent of the key, so entries may share a storageName or leave it unset. Unlike the default storageConfigInfo, each entry must list at least one allowedLocations entry.
Review Comment:
I dropped the `storageName` mention from the `Catalog.storageConfigurations` description and kept "names" in the first sentence. I spelled out the map as "keys are configuration names, values are StorageConfigInfo objects", which should answer @dimas-b's question. The independence of `storageName` from the configuration name is now documented on `StorageConfigInfo.storageName`. Happy to adjust the wording if it's still unclear.
--
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] Spec: Add named storage configurations to the management API [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5721:
URL: https://github.com/apache/polaris/pull/5721#discussion_r4340645623
##########
spec/polaris-management-service.yml:
##########
@@ -1303,6 +1308,11 @@ components:
type: string
storageConfigInfo:
$ref: "#/components/schemas/StorageConfigInfo"
+ storageConfigurations:
+ type: [ object, "null" ]
+ description: Optional replacement set of named storage configurations, keyed by configuration name. When present, replaces the catalog's entire set of named storage configurations (an empty object removes them all); when absent, the existing set is left unaltered. Keys and entries follow the same rules as Catalog.storageConfigurations.
Review Comment:
We cannot rely on the build config(`penApiNullable=false`) to ensure the correctness. Downstreams or clients may have different build configs, which causes the behavior changes. It is fragile to use the absent/null/emptyMap to control whether deleting or preserving the map. We may need to either introduce a special update type for the storageConfig map, or let clients to always send the whole map(this may have compatible issue, given the old clients don't know this field at all).
--
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] Spec: Add named storage configurations to the management API [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5721:
URL: https://github.com/apache/polaris/pull/5721#discussion_r4300388528
##########
spec/polaris-management-service.yml:
##########
@@ -879,6 +989,12 @@ components:
description: The version of the catalog object used to determine if the catalog metadata has changed
storageConfigInfo:
$ref: "#/components/schemas/StorageConfigInfo"
+ storageConfigInfos:
Review Comment:
My reading of the proposal is that in Phase 1 we add storage configs as sub-elements in the Catalog. So this change makes sense.
I'd have expected the dedicated config endpoints to be added later... WDYT?
https://lists.apache.org/thread/ytdc13npxq4m0dz54vm1g7n3ygpywf5q
--
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] Spec: Add named storage configurations to the management API [polaris]
Posted by "sririshindra (via GitHub) via issues" <is...@polaris.apache.org>.
sririshindra closed pull request #5721: Spec: Add named storage configurations to the management API
URL: https://github.com/apache/polaris/pull/5721
--
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] Spec: Add named storage configurations to the management API [polaris]
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on PR #5721:
URL: https://github.com/apache/polaris/pull/5721#issuecomment-6250394892
The test-site-guides failure is unrelated and would be addressed by https://github.com/apache/polaris/pull/5812 . A rebase should fix it.
--
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] Spec: Add named storage configurations to the management API [polaris]
Posted by "orca-review-bot (via GitHub)" <gi...@apache.org>.
orca-review-bot commented on code in PR #5721:
URL: https://github.com/apache/polaris/pull/5721#discussion_r4331217870
##########
spec/polaris-management-service.yml:
##########
@@ -879,6 +879,11 @@ components:
description: The version of the catalog object used to determine if the catalog metadata has changed
storageConfigInfo:
$ref: "#/components/schemas/StorageConfigInfo"
+ storageConfigurations:
+ type: [ object, "null" ]
+ description: Optional additional named storage configurations held by the catalog, alongside its default storageConfigInfo, keyed by configuration name. Each key must match ^[a-zA-Z0-9_-]{1,128}$ and is case-sensitive. The default storageConfigInfo is not part of this map and has no configuration name. The storageName of an entry keeps its meaning of selecting a server-side storage configuration and is independent of the key, so entries may share a storageName or leave it unset. Unlike the default storageConfigInfo, each entry must list at least one allowedLocations entry.
Review Comment:
The key rule only lives in the description, so generated clients and schema validators won't see it. Since the spec is OpenAPI 3.1, can we add `propertyNames: { pattern: '^[a-zA-Z0-9_-]{1,128}$' }` here (and on the UpdateCatalogRequest field)?
<sub>๐ค AI-generated review comment โ agents make mistakes, so please verify before acting on it.</sub>
##########
spec/polaris-management-service.yml:
##########
@@ -1303,6 +1308,11 @@ components:
type: string
storageConfigInfo:
$ref: "#/components/schemas/StorageConfigInfo"
+ storageConfigurations:
+ type: [ object, "null" ]
+ description: Optional replacement set of named storage configurations, keyed by configuration name. When present, replaces the catalog's entire set of named storage configurations (an empty object removes them all); when absent, the existing set is left unaltered. Keys and entries follow the same rules as Catalog.storageConfigurations.
Review Comment:
This defines 'absent' vs `{}`, but not an explicit `"storageConfigurations": null`. With `openApiNullable=false`, an explicit null deserializes the same as an omitted field, so it would mean 'leave the set unchanged'. Is that the intent? If so, can we say it here so clients don't send `null` expecting a clear?
<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] Spec: Add named storage configurations to the management API [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5721:
URL: https://github.com/apache/polaris/pull/5721#discussion_r4331923729
##########
spec/polaris-management-service.yml:
##########
@@ -879,6 +879,11 @@ components:
description: The version of the catalog object used to determine if the catalog metadata has changed
storageConfigInfo:
$ref: "#/components/schemas/StorageConfigInfo"
+ storageConfigurations:
+ type: [ object, "null" ]
+ description: Optional additional named storage configurations held by the catalog, alongside its default storageConfigInfo, keyed by configuration name. Each key must match ^[a-zA-Z0-9_-]{1,128}$ and is case-sensitive. The default storageConfigInfo is not part of this map and has no configuration name. The storageName of an entry keeps its meaning of selecting a server-side storage configuration and is independent of the key, so entries may share a storageName or leave it unset. Unlike the default storageConfigInfo, each entry must list at least one allowedLocations entry.
Review Comment:
I'm OK with pattern written in the description.
--
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] Spec: Add named storage configurations to the management API [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5721:
URL: https://github.com/apache/polaris/pull/5721#discussion_r4332137565
##########
spec/polaris-management-service.yml:
##########
@@ -879,6 +879,11 @@ components:
description: The version of the catalog object used to determine if the catalog metadata has changed
storageConfigInfo:
$ref: "#/components/schemas/StorageConfigInfo"
+ storageConfigurations:
+ type: [ object, "null" ]
+ description: Optional additional named storage configurations held by the catalog, alongside its default storageConfigInfo, keyed by configuration name. Each key must match ^[a-zA-Z0-9_-]{1,128}$ and is case-sensitive. The default storageConfigInfo is not part of this map and has no configuration name. The storageName of an entry keeps its meaning of selecting a server-side storage configuration and is independent of the key, so entries may share a storageName or leave it unset. Unlike the default storageConfigInfo, each entry must list at least one allowedLocations entry.
Review Comment:
```suggestion
description: Optional additional storage configurations held by the catalog, alongside its default storageConfigInfo. A map from configuration name to StorageConfigInfo. Configuration names are case sensitive and must match ^[a-zA-Z0-9_-]{1,128}$. The default storageConfigInfo is not included in this map and has no configuration name. Unlike the default storageConfigInfo, each entry must list at least one allowedLocations entry.
```
I'd prefer to not mention storageName here. Maybe just me, it seems more confusing with explanation. We can definitely add more description for `storageName` though. Giving an example also helps, but I'm OK without an example.
--
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