GODRIVER-3289 Add option to configure DEK cache lifetime. #1922
Conversation
API Change Report./v2/mongo/optionscompatible changes(*AutoEncryptionOptions).SetKeyExpiration: added ./v2/x/mongo/driver/mongocrypt/optionscompatible changes(*MongoCryptOptions).SetKeyExpiration: added |
qingyang-hu
force-pushed
the
godriver3289
branch
7 times, most recently
from
50eee42 to
6319ce0
Compare
qingyang-hu
force-pushed
the
godriver3289
branch
from
6319ce0 to
32e7246
Compare
| @@ -164,3 +166,10 @@ func (a *AutoEncryptionOptions) SetBypassQueryAnalysis(bypass bool) *AutoEncrypt | |||
|
|
|||
| return a | |||
| } | |||
|
|
|||
| // SetKeyExpiration specifies duration for the key expiration. 0 or negative value means "never expire". | |||
There was a problem hiding this comment.
Are negative values interpreted by libmongocrypt as "never expire" or are we enforcing that behavior in the Go Driver? I can't find documentation on the negative case. The C and Rust implementations use uint64.
There was a problem hiding this comment.
Negative values are handled in x/mongo/driver/mongocrypt/mongocrypt.go by passing 0 to libmongocrypt. I'm open to using uint64 to align the API with other drivers.
| @@ -193,7 +193,7 @@ func newClient(opts ...*options.ClientOptions) (*Client, error) { | |||
| } | |||
| // AutoEncryptionOptions | |||
| if clientOpts.AutoEncryptionOptions != nil { | |||
| if err := client.configureAutoEncryption(clientOpts); err != nil { | |||
| if err = client.configureAutoEncryption(clientOpts); err != nil { | |||
There was a problem hiding this comment.
Was there a particular scoping or style reason for migrating from the variable declaration to assignment?
| kr := keyRetriever{coll: c.keyVaultCollFLE} | ||
| var cir collInfoRetriever | ||
| bypass := aeOpts.BypassAutoEncryption != nil && *aeOpts.BypassAutoEncryption | ||
| if !bypass { | ||
| if args.MaxPoolSize != nil && *args.MaxPoolSize == 0 { | ||
| c.metadataClientFLE = c | ||
| } else { | ||
| c.metadataClientFLE, err = c.getOrCreateInternalClient(args) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| cir.client = c.metadataClientFLE | ||
| } | ||
|
|
||
| c.cryptFLE = driver.NewCrypt(&driver.CryptOptions{ | ||
| MongoCrypt: mc, | ||
| CollInfoFn: cir.cryptCollInfo, | ||
| KeyFn: kr.cryptKeys, | ||
| MarkFn: c.mongocryptdFLE.markCommand, | ||
| TLSConfig: aeOpts.TLSConfig, | ||
| BypassAutoEncryption: bypass, | ||
| }) |
There was a problem hiding this comment.
This refactor seems unrelated to DEK cache lifetime. I think this should be reverted to avoid confusing the scope of this PR.
| keyExpirationMs = uint64(expirationMs) | ||
| } | ||
| } | ||
| C.mongocrypt_setopt_key_expiration(crypt.wrapped, C.uint64_t(keyExpirationMs)) |
There was a problem hiding this comment.
Should we expose this type as a uint64 instead of a time.Duration in the options API? That is the convention for both C and Rust:
Rust: https://github.com/mongodb/libmongocrypt-rust/pull/38/files
C: mongodb/mongo-c-driver@b0edf30#diff-782e2ab0e9ed19e1fc3e8e7bb5587e57805d5c816b85ab4b606da41de292bc77R137
There was a problem hiding this comment.
IMO, time.Duration is more expressive. We also use it for duration values in other option structs. However, I'm open to using uint64 if it simplifies the API and improves consistency with other drivers.
There was a problem hiding this comment.
It's conventional in the Go Driver to use the time.Duration type to specify duration values (e.g. FindOptions.SetMaxTime). There is a difference in precision (nanoseconds vs milliseconds), but it's unlikely any users want to set sub-millisecond key expiration values. Using time.Duration avoids the more likely arithmetic bugs that can happen when manually converting minutes, hours, or days to milliseconds (e.g. you get to use 2 * time.Hour vs 2 * 60 * 60 * 1_000).
For those reasons, I think we should use time.Duration.
| // SetKeyExpiration sets the key expiration duration. 0 means "never expire". | ||
| func (mo *MongoCryptOptions) SetKeyExpiration(expiration *time.Duration) *MongoCryptOptions { |
There was a problem hiding this comment.
Maintaining these setters adds a lot of unnecessary code, especially because this is expected to be an internal-only API. Instead of continuing to add these setter methods, can we remove all of them and define the MongoCryptOptions struct literal instead?
| expirationMs := opts.KeyExpiration.Milliseconds() | ||
| if expirationMs < 0 { | ||
| keyExpirationMs = 0 | ||
| } else { | ||
| keyExpirationMs = uint64(expirationMs) | ||
| } |
There was a problem hiding this comment.
The Milliseconds method will truncate values that are less than 1ms to 0, which is interpreted as "never expire". Users might be confused when they put a non-zero expiry value that is interpreted as "never expire".
E.g.
d := 10 * time.Microsecond
fmt.Println(d.Milliseconds())
// Prints "0"We should check for that case and set expirationMs to 1.
E.g.
expirationMs := opts.KeyExpiration.Milliseconds()
if expirationMs < 0 {
keyExpirationMs = 0
} else if expirationMs == 0 && opts.KeyExpiration > 0 {
// If we get 0ms, but the KeyExpiration duration is non-zero,
// set keyExpirationMs to 1 so we don't interpret sub-millisecond
// durations as "never expire".
keyExpirationMs = 1
} else {
keyExpirationMs = uint64(expirationMs)
}
GODRIVER-3289
Summary
keyExpirationMSanddecryptin the spec tests.Background & Motivation
Add an option to configure DEK cache lifetime.
Specs updates: