danhuawang commented on issue #11601:
URL: https://github.com/apache/gravitino/issues/11601#issuecomment-4718685105
Re-opening visibility: the fix from #11628 does **not** take effect on the
most common path — loading an already-persisted catalog through the Gravitino
catalog store (`USE CATALOG ...`). It still fails with the same
`NotAuthorizedException` at `RESTSessionCatalog.fetchConfig` (`GET /v1/config`).
### Root cause: the REST auth-propagation branch is gated on a key that has
already been renamed
In `GravitinoIcebergCatalogFactory.toIcebergCatalogOptions`, the auth
propagation is gated on `catalog-backend`:
```java
String catalogBackend =
catalogOptions.get(IcebergPropertiesConstants.GRAVITINO_ICEBERG_CATALOG_BACKEND);
// "catalog-backend"
...
if
(IcebergPropertiesConstants.ICEBERG_CATALOG_BACKEND_REST.equalsIgnoreCase(catalogBackend)
&& !icebergCatalogOptions.containsKey(AuthProperties.AUTH_TYPE)) {
icebergCatalogOptions.putAll(
IcebergPropertiesConverter.INSTANCE.toRestAuthProperties(
GravitinoCatalogManager.get().getGravitinoClientConfig()));
}
```
But when a catalog is loaded via the catalog store,
`GravitinoCatalogStore.getCatalog` runs
`IcebergPropertiesConverter.toFlinkCatalogProperties(...)` first, which renames
`catalog-backend` → `catalog-type`:
```java
private static final Map<String, String> GRAVITINO_CONFIG_TO_FLINK_ICEBERG =
ImmutableMap.of(IcebergConstants.CATALOG_BACKEND, //
"catalog-backend"
IcebergPropertiesConstants.ICEBERG_CATALOG_TYPE); //
"catalog-type"
```
So by the time the factory runs, the descriptor carries `catalog-type=rest`
and **no** `catalog-backend` key. As a result:
- `catalogBackend = options.get("catalog-backend")` → `null`
- `"rest".equalsIgnoreCase(null)` → `false` → **auth propagation is skipped**
- the catalog is still created as REST (the later branch reads
`catalog-type`), but with no `rest.auth.*` → REST client hits `/v1/config`
unauthenticated → 401.
The two backend checks in the same method are inconsistent: the JDBC branch
reads `catalog-type`, while the REST-auth branch reads `catalog-backend`.
### Why it slipped through
- `TestGravitinoIcebergCatalogFactory.testRestBackendKeepsCatalogType` feeds
the option as `catalog-type` and does **not** assert any auth property, so the
propagation gap isn't exercised.
- `CREATE CATALOG ... WITH ('catalog-backend'='rest', ...)` does carry
`catalog-backend` in `context.getOptions()`, so that path works. The failure is
specific to loading a persisted catalog through the catalog store — which is
what the OAuth2 REST e2e test (and real users) do.
### Suggested fix
Gate the REST auth propagation on the normalized `catalog-type` (after the
`catalog-backend` → `catalog-type` normalization), so both the CREATE path and
the catalog-store load path behave the same:
```java
String catalogType =
icebergCatalogOptions.get(IcebergPropertiesConstants.ICEBERG_CATALOG_TYPE);
if
(IcebergPropertiesConstants.ICEBERG_CATALOG_BACKEND_REST.equalsIgnoreCase(catalogType)
&& !icebergCatalogOptions.containsKey(AuthProperties.AUTH_TYPE)) {
icebergCatalogOptions.putAll(
IcebergPropertiesConverter.INSTANCE.toRestAuthProperties(
GravitinoCatalogManager.get().getGravitinoClientConfig()));
}
```
A regression test should cover the catalog-store load path
(`catalog-type=rest` with `gravitino.client.auth.type=oauth2`) and assert that
`rest.auth.type` / OAuth2 properties are injected into the resulting Iceberg
options.
(Found while running an OAuth2 REST-backend Flink view e2e test on a branch
that already includes #11628.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
