nevzheng commented on code in PR #12489:
URL: https://github.com/apache/gravitino/pull/12489#discussion_r3803947761
##########
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:
Done in fbf7694c365146d7fd14b1503b189faad9a757ea
Nevin
Sent from my 🤖 (Cursor)
##########
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:
Done in fbf7694c365146d7fd14b1503b189faad9a757ea
Nevin
Sent from my 🤖 (Cursor)
##########
common/src/testFixtures/java/org/apache/gravitino/encryption/kms/FakeKmsClient.java:
##########
@@ -25,23 +25,19 @@
/** In-memory KMS client for contract and consumer tests. */
public final class FakeKmsClient implements KmsClient {
- private final String api;
- private final String source;
+ private final String provider;
private final Map<String, KeyState> keys = new HashMap<>();
/**
* Creates an empty fake client.
*
- * @param api exact KMS API identifier accepted by the client; lowercase
kebab-case with no
- * surrounding whitespace
- * @param source configured source accepted by the client
+ * @param provider configured provider accepted by the client
*/
- public FakeKmsClient(String api, String source) {
- this.api = KmsApiIdentifiers.requireValid(api);
- if (source == null || source.trim().isEmpty()) {
- throw new IllegalArgumentException("KMS source cannot be blank");
+ public FakeKmsClient(String provider) {
+ if (provider == null || provider.trim().isEmpty()) {
+ throw new IllegalArgumentException("KMS provider cannot be blank");
Review Comment:
Switched the blank check to `StringUtils.isBlank(provider)` in
`FakeKmsClient`.
fbf7694c365146d7fd14b1503b189faad9a757ea
Nevin
Sent from my 🤖 (Cursor)
##########
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:
Applied both of these.
Dropped the `.api=` lines. Each named provider now loads a factory the same
way listeners do:
```text
gravitino.kms.providers = aws-prod,aws-dr,azure-eu
gravitino.kms.provider.aws-prod.className =
org.apache.gravitino.encryption.kms.aws.AwsKmsClientFactory
gravitino.kms.provider.aws-dr.className =
org.apache.gravitino.encryption.kms.aws.AwsKmsClientFactory
gravitino.kms.provider.azure-eu.className =
org.apache.gravitino.encryption.kms.azure.AzureKmsClientFactory
```
The registry constructs that class with a no-arg constructor. Callers still
send `{provider, keyId}` only.
fbf7694c365146d7fd14b1503b189faad9a757ea
Nevin
Sent from my 🤖 (Cursor)
--
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]
