feat(encryption): add KMS catalog-property registry#1493
feat(encryption): add KMS catalog-property registry#1493hectar-glitches wants to merge 2 commits into
Conversation
Signed-off-by: hectar-glitches <hectar@uni.minerva.edu>
tanmayrauth
left a comment
tanmayrauth
left a comment
There was a problem hiding this comment.
Registry mechanics look right and mirror io.Register faithfully. One test-gap plus a couple of design calls worth settling now while nothing consumes the API yet. Details inline.
| return nil, fmt.Errorf("%w: %q", ErrKMSTypeNotFound, name) | ||
| } | ||
|
|
||
| return factory(props) |
There was a problem hiding this comment.
Nothing verifies props actually reaches the factory here — every test registers a factory that ignores its argument. The whole point of KMSFactory(props) is that real KMS factories read key material/region/endpoints out of props, so a future cleanup that filtered the map or passed nil before this call would compile, pass the whole suite, and silently hand an AWS/GCP factory an empty map — a factory reading encryption.kms.region would build a wrong-region client or fail at construction, uncaught. Please add a test that registers a factory recording its received props and assert it equals the input (include a sentinel key like encryption.kms.region).
There was a problem hiding this comment.
added some tests to register a factory that records the props map it receives and asserts it's identical to what was passed to LoadKeyManagementClient
| // [KMSTypeKey] for vendors that ship their own KMS integration under a | ||
| // distinct name; the two properties share the same registry and | ||
| // resolution mechanism. | ||
| KMSImplKey = "encryption.kms-impl" |
There was a problem hiding this comment.
kms-type and kms-impl resolve the same registry by the same short-name lookup, so they're functionally identical — and that's novel here: nothing else in the repo uses a -impl property, and the precedent (catalog.Load reads a single "type"; the io scheme registry) is one short key. In Iceberg generally -impl means a fully-qualified implementation, distinct from the short -type alias, so someone migrating a config that sets encryption.kms-impl=com.acme.AcmeKmsClient gets ErrKMSTypeNotFound because Go looks it up as a registered short name. Since nothing consumes this yet, either drop kms-impl and keep only kms-type (matching the repo's single-key precedent), or keep both and make the doc explicit that kms-impl is a registered short name, not an FQCN — the "a custom implementation" wording at 34-35 currently reads like the Java FQCN meaning.
There was a problem hiding this comment.
dropped kms-imply entirely leaving just KMSTypeKey
| // [ErrKMSTypeNotFound] if neither property is set or the named KMS has not | ||
| // been registered via [RegisterKMS]. | ||
| func LoadKeyManagementClient(props map[string]string) (KeyManagementClient, error) { | ||
| name := props[KMSTypeKey] |
There was a problem hiding this comment.
The doc says it falls back to KMSImplKey "if present," but the code only falls back when kms-type is empty, not when it's set to an unregistered name — so kms-type=typo + kms-impl=memory errors instead of loading memory. The behavior's defensible (an explicit type shouldn't silently swallow a typo), but the doc reads otherwise. Tighten it to "falls back only when KMSTypeKey is unset," and consider adding that set-but-unregistered-type + valid-impl case to the precedence test at kms_registry_test.go:44.
There was a problem hiding this comment.
will tighten
Signed-off-by: hectar-glitches <hectar@uni.minerva.edu>
| // [KeyManagementClient] implementation registered via [RegisterKMS] (e.g. | ||
| // "memory"). It is a registered short name, not a fully-qualified class | ||
| // name. | ||
| const KMSTypeKey = "encryption.kms-type" |
There was a problem hiding this comment.
is this already part of the iceberg spec?
There was a problem hiding this comment.
no, this isn't part of the Iceberg spec. I checked the spec and configuration docs: the only standardized encryption properties are "encryption.key-id" and "encryption.data-key-length". "encryption.kms-type" is an iceberg-go-specific mechanism for selecting a registered "KeyManagementClient" implementation at runtime so there's no cross-implementation compatibility concern here since it only affects how this Go library wires up its KMS client, not anything written to table metadata.
Happy to rename it to something less spec-adjacent (e.g. a Go-specific prefix) if that's clearer
3 participants
What
Adds a named-factory registry for
KeyManagementClientimplementations, selected via theencryption.kms-type/encryption.kms-implcatalog properties. This completes the second half of the "KMS client interface"checklist item from #1289 (the interface + in-memory impl landed in #1447).
Mirrors the existing
io.Register/io.SchemeFactoryscheme registry pattern (io/registry.go) rather than introducing a new convention.Details
RegisterKMS(name, factory)/UnregisterKMS(name)/GetRegisteredKMSNames(): a mutex-guarded named registry forKMSFactoryimplementations.LoadKeyManagementClient(props): resolvesencryption.kms-typefirst,falling back to
encryption.kms-impl: returning an error wrapping the newErrKMSTypeNotFoundif neither is set or the name isn't registered."memory"toInMemoryKeyManagementClientas the built-in option ininit().Not in scope
StandardEncryptionManager(envelope KEK/DEK logic).Testing
encryption/kms_registry_test.gocovering: resolution via each property,kms-typetaking precedence when both are set, missing/unregistered names, duplicate/nil-factory registration panics, and factory-error propagation.