docs(bindings): add example for kms pkey offload #4980
Conversation
maddeleine
left a comment
maddeleine
left a comment
There was a problem hiding this comment.
I really just have nits. I don't know if we can make the example better without fixing our underlying issues.
bindings/rust-examples/async-pkey-offload/tests/client_server.rs
Outdated
Show resolved
Hide resolved
| // > The public key that AWS KMS returns is a DER-encoded X.509 public key, | ||
| // > also known as SubjectPublicKeyInfo (SPKI), as defined in RFC 5280. | ||
| // > When you use the HTTP API or the AWS CLI, the value is Base64-encoded. | ||
| // > Otherwise, it is not Base64-encoded. | ||
| // https://docs.aws.amazon.com/kms/latest/developerguide/download-public-key.html |
There was a problem hiding this comment.
Nit: I prefer the link first so that I know what I'm reading
There was a problem hiding this comment.
Hmm, I feel like that's a bit non-standard w.r.t block quotes, e.g. apa.
* refactor to list-keys method
* make error message parameterized
|
Offline discussion: what this PR is missing is an explanation of where the tricky things come from. Everyone feels like this feature should be easier to use, but it's difficult to explain why or why not. I need to explain why it can't be any simpler. |
bindings/rust-examples/async-pkey-offload/tests/client_server.rs
Outdated
Show resolved
Hide resolved
|
|
||
| The solution is to wrap the anonymous future in some other type, something like the following: | ||
| ```rust | ||
| pub struct SimplePrivateKeyFuture<F> { |
There was a problem hiding this comment.
Could you instead spawn the task in handle_operation and then store the outputted JoinHandle in SimplePrivateKeyFuture? JoinHandle is Sync, so it would clean up the Future a little.
There was a problem hiding this comment.
Hmm 🤔 . So we could do that, but it's not really the way we want customers to use this? Like generally they shouldn't be tokio::spawning tasks just to get a join handle. They are only having to do that as a workaround for our send/sync stuff.
There was a problem hiding this comment.
I took a whack at implementing it this way. I still think it's cleaner, but I did run into all of the same things you ran into. I'm interested if you think this is better, worse, or exactly equivalent to your implementation:
maddeleine@5ae2652
It does compile, although I haven't actually run the test for this diff.
There was a problem hiding this comment.
I do expect that it would pass tests, but I do not think it's nicer.
Customers are used to using the AWS-SDK, and hate working with manual futures. The AWS-SDK provides convenient async functions, which allows them to easily use the async functionality.
For a customers starting from the mental model, I think the current example structure is the clearest/most accurate. Bascially "lots of business logic lives in the async method, and that all sort of works".
While your implementation does minimize the amount of code in the async block, I don't think that's actually cleaner, and it relies on an "accident" of implementation w.r.t. the the tokio JoinHandle, which in an ideal world wouldn't exist.
There was a problem hiding this comment.
Yeah then I don't really have any ideas for how to improve this example 🤷♀️
- clarify where "block_on" is used
* reference the KMS developer guide
* update diagram
* fix name leftover from development explanation
|
|
||
| // We need multiple threads, because the block_on (used in cert generation) can | ||
| // only be used in multi-threaded runtimes. | ||
| #[tokio::test(flavor = "multi_thread", worker_threads = 2)] |
There was a problem hiding this comment.
I am planning to talk a crack at getting these tests actually running in CI. If I can't do that simply, how do people feel about just mocking out the KMS dependency and using a "fake" client?
There was a problem hiding this comment.
I have no problem with a "fake" client.
3 participants
Description of changes:
This example demonstrate the pkey offload feature in combination with KMS APIs.
Call-outs:
I structured this example as a test, because it was the only way to keep my sanity while I was writing it. Furthermore, I think this is an excellent item to add to our CI.
While there might be a slight readability hit by structuring it as a "test", I actually think that keeping everything in the same process is a bit of a readability win. There isn't an CLI parsing, etc. So very happy to make the tradeoff.
Testing:
Verified that the test passes locally.
I also opened #4979 to track this.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.