Skip to content
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

Switch to base64 crate #1294

Closed
wants to merge 4 commits into from
Closed

Switch to base64 crate #1294

wants to merge 4 commits into from

Conversation

david-perez
Copy link
Contributor

@david-perez david-perez commented Apr 1, 2022

This removes our implementation of base64 encoding/decoding from
aws-smithy-types, switching to the base64 Rust crate.

Our implementation was written at a time when we wanted to keep
dependencies at an absolute minimum. However, a performant base64
implementation is now needed for the server.

Our implementation was also accepting invalid base64 (see #1277).

Fixes #1277.

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

This removes our implementation of base64 encoding/decoding from
`aws-smithy-types`, switching to the `base64` Rust crate.

Our implementation was written at a time when we wanted to keep
dependencies at an absolute minimum. However, a performant base64
implementation is now needed for the server.

Our implementation was also accepting invalid base64 (see #1277).

Fixes #1277.
@david-perez david-perez requested review from a team as code owners April 1, 2022 12:23
[[smithy-rs]]
message = "Switch to `base64` crate"
references = ["smithy-rs#1294"]
meta = { "breaking" = true, "tada" = false, "bug" = false }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do I add this message?

The base64 implementation from aws-smithy-types has been removed. Please switch to another implementation (e.g. the base64 crate).

I wanted to add it to aws/SDK_CHANGELOG.md, but that file is autogenerated from update-changelog from tools/sdk-lints. How are the migration guides from that file added if the file is autogenerated? Do they get added manually just before preparing a release?

Copy link
Collaborator

@rcoh rcoh Apr 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an example above—[[aws-sdk-rust]] instead of smithy-rs.

Anything you want in the message goes in the message field—TOML allows multiline strings. However: I think we should keep the same function signatures and just delegate to the base64 crate behind the scenes. Having a "blessed" base64 implementation inside of aws-smithy-types is probably generally a good thing to have.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an example above—[[aws-sdk-rust]] instead of smithy-rs.

But how do I add a longer migration guide e.g. https://github.com/awslabs/smithy-rs/blob/775d227c5fbc46c2e3c2878b270b8e8226ab7ef1/aws/SDK_CHANGELOG.md?plain=1#L5-L33 ? Do I plop all of that Markdown into message?


However: I think we should keep the same function signatures and just delegate to the base64 crate behind the scenes. Having a "blessed" base64 implementation inside of aws-smithy-types is probably generally a good thing to have.

I thought a bit about this before making the change and concluded that there's no benefit. base64 is standard and I can't foresee us or Smithy wanting to customize anything that is not already customizable via the base64 crate. I also don't think that SDK users should have public access to a base64-encoding routine from a runtime crate; they should have no need for it after all, since they should just be using aws_smithy_types::Blob. I think having a "blessed" base64 implementation is unnecessarily increasing public API surface.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe blob should add a from_base64 method...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, just use triple quote strings for long messages

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've added it here f1c00cb

@github-actions
Copy link

github-actions bot commented Apr 1, 2022

A new generated diff is ready to view.

@github-actions
Copy link

github-actions bot commented Apr 1, 2022

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement Deviation Current Old
Requests/sec 3.67% 81398 78518.69
Total requests 3.66% 7331117 7072328
Total errors NaN% 0 0
Total successes 3.66% 7331117 7072328
Average latency ms 6.09% 1.22 1.15
Minimum latency ms 0.00% 0.02 0.02
Maximum latency ms -0.58% 23.81 23.95
Stdev latency ms 5.37% 2.16 2.05
Transfer Mb 3.66% 762.07 735.17
Connect errors NaN% 0 0
Read errors NaN% 0 0
Write errors NaN% 0 0
Status errors (not 2xx/3xx) NaN% 0 0
Timeout errors NaN% 0 0

[[smithy-rs]]
message = "Switch to `base64` crate"
references = ["smithy-rs#1294"]
meta = { "breaking" = true, "tada" = false, "bug" = false }
Copy link
Collaborator

@rcoh rcoh Apr 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an example above—[[aws-sdk-rust]] instead of smithy-rs.

Anything you want in the message goes in the message field—TOML allows multiline strings. However: I think we should keep the same function signatures and just delegate to the base64 crate behind the scenes. Having a "blessed" base64 implementation inside of aws-smithy-types is probably generally a good thing to have.

@david-perez
Copy link
Contributor Author

Let's hold off merging this until we know Smithy's firm stance on extant base64-encoded data.

@rcoh
Copy link
Collaborator

rcoh commented Apr 25, 2022

@david-perez smithy-lang/smithy#1168 was merged—what's the plan here?

@david-perez
Copy link
Contributor Author

@rcoh It was reverted, and for good reason; see smithy-lang/smithy#1168 (comment).

To move forward; we either wait for/implement marshallpierce/rust-base64#181, or we switch to another crate that rejects unpadded inputs, like base64ct.

@rcoh
Copy link
Collaborator

rcoh commented Apr 25, 2022

We also just fix our impl for now

@Nugine
Copy link
Contributor

Nugine commented Jun 26, 2022

Hi. The base64-simd crate may be what you want.

@david-perez
Copy link
Contributor Author

Superseded by #1938.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect base64 implementation?
5 participants