-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CSHARP-5205: Add option to configure DEK cache lifetime #1614
base: main
Are you sure you want to change the base?
Conversation
@sanych-sun there are two things I'm not super convinced:
(I also need to add input validation to be sure that the number of milliseconds isn't negative) |
/// <summary> | ||
/// Gets the value of the expiration time for the DEK cache in ms. | ||
/// </summary> | ||
public long? DekCacheLifetimeMs { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a compelling reason to use a long here instead of a TimeSpan? .NET typically prefers TimeSpans for durations as they're easier to configure in terms of code and you don't need suffixes then like "Ms" on the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the issue here would be that TimeSpan
has a greater granularity (microseconds) than the corresponding input for libmongocrypt (milliseconds).
We could also approximate the value if microseconds are used, but I'm not super convinced. I agree that maybe it would be easier to read with TimeSpan
, even though more verbose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we should use TimeSpan. And pass TotalMilliseconds portion of it to libmongocrypt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, or we could also throw an exception if there is microseconds.
/// <summary> | ||
/// Gets the value of the expiration time for the DEK cache in ms. | ||
/// </summary> | ||
public long? DekCacheLifetimeMs { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we should use TimeSpan. And pass TotalMilliseconds portion of it to libmongocrypt.
/// <summary> | ||
/// Gets the value of the expiration time for the DEK cache in ms. | ||
/// </summary> | ||
public long? DekCacheLifetimeMs { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What Dek means? Is property name prescribed by spec? I saw we call mongocrypt_setopt_key_expiration method after all. Should we name the property as simple as KeyExpiration
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DEK is the data encryption key. We can also call it KeyExpiration
, I was just worried it could be too generic given the number of keys involved in the encryption, but I suppose it could be only this one if set up locally
@damieng @sanych-sun I've changed the name to |
No description provided.