roryqi commented on code in PR #12489:
URL: https://github.com/apache/gravitino/pull/12489#discussion_r3803919031


##########
docs/gravitino-server-config.md:
##########
@@ -486,6 +488,53 @@ server, are documented with those services. See
 | `gravitino.job.stagingDirKeepTimeInMs` | How long in milliseconds a finished 
job's staging files are kept. Use at least 10 minutes outside testing. | 
`604800000` (7 days)          |
 | `gravitino.job.statusPullIntervalInMs` | Interval in milliseconds between 
job status polls. Use at least 1 minute outside testing.                  | 
`300000` (5 minutes)          |
 
+### Key Management
+
+The server talks to KMS instances you name in `gravitino.conf`. Each name is 
one configured
+instance. Each instance declares the KMS API it speaks. That API selects the 
`KmsClientFactory`
+implementation on the classpath. Two names may share one API, which is how you 
run more than one
+AWS or Azure vault.
+
+The list is empty by default, and then the server has no KMS clients. Naming a 
provider without a
+factory for its API, or without a valid API, fails startup. Client 
construction validates local
+configuration only; the first call to the provider is a later key inspection, 
not startup.
+
+| Configuration Item                      | Description                        
                                                                                
                                                         | Default Value |
+|-----------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------|
+| `gravitino.kms.providers`               | Comma-separated KMS instance 
names. Each name must match `[A-Za-z0-9][A-Za-z0-9_-]*` and cannot contain `.`. 
Duplicates fail startup.                                       | (empty)       |
+| `gravitino.kms.provider.<name>.api`     | Required KMS API identifier for 
that instance. Lowercase kebab-case with no surrounding whitespace, matched 
exactly, for example `aws-kms`. Values are never normalized.    | (none)        
|
+| `gravitino.kms.provider.<name>.<key>`   | Any other property under that name 
is passed to the factory for that API. Nested dots in `<key>` are allowed, as 
in `endpoint.region`.                                      | (none)        |
+
+Every name in `providers` needs a matching `.api`. A 
`gravitino.kms.provider.<name>.*` key for a
+name that is not in the list, or any other `gravitino.kms.*` key, fails 
startup.
+
+`api` is the protocol, not the instance. Set it to the identifier the factory 
reports from
+`KmsClientFactory.api()`. Identifiers already used by factories include 
`aws-kms`,
+`google-cloud-kms`, and `azure-key-vault`. A custom factory may use any other 
lowercase kebab-case
+value. The server does not ship a factory for every identifier; put the 
matching implementation on
+the classpath or startup fails with no factory for that API.
+
+Callers name the instance and the key. They do not send `api`. The server 
already bound
+`aws-prod` to `aws-kms` at startup.
+
+```text
+# conf/gravitino.conf
+gravitino.kms.providers = aws-prod, aws-dr, azure-eu
+
+gravitino.kms.provider.aws-prod.api = aws-kms
+gravitino.kms.provider.aws-prod.endpoint.region = us-west-2
+

Review Comment:
   The issue is that we should adopt the similar mechanism.
   SecretProvider uses SPI mode. `type` is similar to your `api`. It would 
better that @nevzheng and @lasdf1234 to keep consistent. 
   Any name is ok for me.
   But nevin adopts the class reflection mode, sai adopts SPI mode.
   Both modes are used in Gravitino repo.
   You can see class `Authenticator` and `CredentialProvider`.
   
   `CredentialProvider` used SPI mode.
   `Authenticator` used class reflection mode.
   
   It would better that secret providers and kms providers adopt the consistent 
mode.
   
   ```
   @DeveloperApi
   public interface SecretProvider {
   
     /**
      * Initializes this provider after construction.
      *
      * <p>Implementations must override this method and explicitly decide 
whether configuration is
      * required. An empty body is acceptable for providers that need no setup.
      *
      * @param name the configured provider instance name
      * @param config provider-specific configuration (without the {@code 
gravitino.secret.provider.
      *     <name>.} prefix)
      */
     void initialize(String name, Map<String, String> config);
   
     /**
      * Returns the provider type identifier.
      *
      * @return the provider type
      */
     String type();
   
     /**
      * Writes a plaintext secret and returns its URN.
      *
      * <p>Provider-specific write metadata is supplied as {@code attributes}. 
Required keys depend on
      * the provider implementation; for example the in-memory write-through 
provider expects {@link
      * SecretConstants#ATTR_ENTITY_TYPE}, {@link 
SecretConstants#ATTR_ENTITY_ID}, and {@link
      * SecretConstants#ATTR_PROPERTY_KEY}.
      *
      * @param plaintext the secret plaintext
      * @param attributes provider-specific write attributes
      * @return the secret URN
      */
     SecretUrn writeSecret(String plaintext, Map<String, String> attributes);
   
     /**
      * Reads a secret by URN.
      *
      * @param urn the secret URN
      * @return the secret plaintext
      */
     String readSecret(SecretUrn urn);
   
     /**
      * Deletes a secret by URN.
      *
      * @param urn the secret URN
      */
     void deleteSecret(SecretUrn urn);
   
     /**
      * Builds a URN for an external secret reference without writing secret 
material.
      *
      * <p>Providers that only support write-through must leave the default 
implementation, which
      * rejects external references.
      *
      * @param propertyKey the entity property key that will store the URN
      * @param attributes provider-specific locator attributes
      * @return the external-reference secret URN (must end with {@code 
propertyKey})
      * @throws UnsupportedOperationException if this provider does not support 
external references
      * @throws IllegalArgumentException if attributes are invalid for this 
provider
      */
     default SecretUrn buildReferenceUrn(String propertyKey, Map<String, 
String> attributes) {
       throw new UnsupportedOperationException(
           type() + " does not support external secret references");
     }
   
     /**
      * Releases resources owned by this provider.
      *
      * <p>Implementations must override this method and explicitly release any 
held resources. An
      * empty body is acceptable when there is nothing to clean up.
      */
     void close();
   }
   
   ```



-- 
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]

Reply via email to