yuqi1129 commented on code in PR #14080:
URL: https://github.com/apache/gravitino/pull/14080#discussion_r4345501617
##########
core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java:
##########
@@ -242,6 +246,49 @@ public void validateOnCreate() {
ops();
}
+ /**
+ * Whether this catalog provides path-based credential contexts, which token
credential providers
+ * such as {@code s3-token} require to vend credentials.
+ *
+ * <p>A catalog whose operations implement {@link
SupportsPathBasedCredentials} must override this
+ * method to return {@code true}; otherwise creating or altering the catalog
with a token provider
+ * fails.
+ *
+ * @return {@code true} if path-based credentials are supported; {@code
false} (default) otherwise
+ */
+ public boolean supportsPathBasedCredentials() {
Review Comment:
[Important] This introduces a second source of truth for path credential
support. At request time, CredentialOperationDispatcher still checks `ops()
instanceof SupportsPathBasedCredentials`, whereas create/alter validation and
GCS provider inference now use this flag. An existing plugin that implements
the documented operations SPI inherits `false` after upgrading: its working
token configuration is rejected on create/alter. More subtly, an existing
catalog relying on `gcs-service-account-file` inference loses its inferred
`gcs-token` provider at line 723, so it can still load and produce path
contexts but credential vending skips the missing provider and returns no
credentials. The added Javadoc helps new implementations but does not preserve
existing plugins; the configurable `ops-impl` extension also makes requiring a
catalog-class override awkward. Could we keep compatibility with the existing
SPI and use a consistent capability source for validation, inference and
vending? Ic
eberg REST does need an explicit exception/capability because it builds the
context itself. Please avoid simply calling `ops()` here, since that eagerly
initializes the backend, and add regression coverage for a legacy operations
implementation with no new flag override.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -1815,6 +1834,19 @@ private CatalogWrapper initCatalogWrapper(
wrapper.classLoader.withClassLoader(
cl -> {
validatePropertyForCreate(catalog.catalogPropertiesMetadata(),
propsToValidate);
+ catalog.validateCredentialProviders(propsToValidate);
+ if (propsToValidate == null) {
+ // Loading stays lenient for catalogs persisted with unsupported
providers, so only
+ // warn about them.
+ try {
+ catalog.validateCredentialProviders(entity.getProperties());
Review Comment:
[Nit] This warning validates raw stored properties, so it misses legacy
secret-backed provider lists. For a catalog created before this change with
`credential-providers` stored as a secret containing `gcs-token`, this map
contains a secret URN; the URN does not match any path-based credential type,
and no warning is emitted. `createBaseCatalog()` already resolves those values
for connector initialization. Could the warning use the already-resolved
configuration, with a load-time regression test, while keeping loading lenient?
--
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]
