roryqi commented on code in PR #12366:
URL: https://github.com/apache/gravitino/pull/12366#discussion_r3718580676
##########
common/src/main/java/org/apache/gravitino/dto/requests/CatalogCreateRequest.java:
##########
@@ -54,6 +54,14 @@ public class CatalogCreateRequest implements RESTRequest {
@JsonProperty("properties")
private final Map<String, String> properties;
+ @Nullable
+ @JsonProperty("secretBindings")
+ private final Map<String, Map<String, String>> secretBindings;
Review Comment:
We should introduce a typed `SecretBinding` instead of representing the
fixed `provider` + plaintext `value` structure as `Map<String, String>`. The
request can then use `Map<String, SecretBinding>`, where the outer map key is
the entity property name. Please also ensure that `SecretBinding.value` is
redacted/excluded from `toString()`.
##########
common/src/main/java/org/apache/gravitino/dto/requests/CatalogCreateRequest.java:
##########
@@ -54,6 +54,14 @@ public class CatalogCreateRequest implements RESTRequest {
@JsonProperty("properties")
private final Map<String, String> properties;
+ @Nullable
+ @JsonProperty("secretBindings")
+ private final Map<String, Map<String, String>> secretBindings;
+
+ @Nullable
+ @JsonProperty("secretReferences")
+ private final Map<String, Map<String, String>> secretReferences;
Review Comment:
The same concern applies to `secretReferences`. Please model each entry as a
typed `SecretReference` with an explicit `provider` and a `Map<String, String>
attributes` field, instead of mixing the reserved provider key with
provider-specific attributes in one string map.
##########
docs/open-api/catalogs.yaml:
##########
@@ -390,6 +390,27 @@ components:
default: { }
additionalProperties:
type: string
+ secretBindings:
+ type: object
+ description: >
+ Optional map of property key to write-through binding. Each
binding must
+ include provider and value (plaintext). Persisted value becomes a
URN.
+ nullable: true
+ additionalProperties:
Review Comment:
Please define reusable `SecretBinding` and `SecretReference` component
schemas and reference them here and from the schema/fileset create requests.
The current nested `additionalProperties` shape cannot express that `provider`
and `value` are required, and generated clients will only expose untyped
dictionaries.
##########
clients/client-python/gravitino/dto/requests/catalog_create_request.py:
##########
@@ -41,6 +41,12 @@ class CatalogCreateRequest(RESTRequest):
_properties: Optional[Dict[str, str]] = field(
metadata=config(field_name="properties")
)
+ _secret_bindings: Optional[Dict[str, Dict[str, str]]] = field(
Review Comment:
The Python API should expose the same typed models rather than `Dict[str,
Dict[str, str]]`, for example `Dict[str, SecretBinding]` and `Dict[str,
SecretReference]`. Since a binding contains plaintext, its `value` should also
be excluded from/redacted in `repr`.
##########
api/src/main/java/org/apache/gravitino/secret/SecretConstants.java:
##########
@@ -37,5 +37,21 @@ public final class SecretConstants {
/** Write attribute: entity property key that holds the secret. */
public static final String ATTR_PROPERTY_KEY = "propertyKey";
+ /**
+ * Required key in each {@code secretBindings} / {@code secretReferences}
map: registered provider
+ * instance name.
+ */
+ public static final String PROVIDER_KEY = "provider";
Review Comment:
If the request structures are modeled with `SecretBinding` and
`SecretReference`, these request-shape magic keys should no longer be needed.
In particular, `REFERENCE_PROVIDER_KEY` is only an alias and is currently
unused. Provider-specific attribute maps can remain dynamic; only the fixed
REST/API structure needs typed fields.
--
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]
