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

add: example showing how to use native-tls #946

Merged
merged 8 commits into from
Dec 9, 2021

Conversation

Velfi
Copy link
Contributor

@Velfi Velfi commented Dec 8, 2021

Motivation and Context

It's not currently possible to avoid using rustls

Description

This PR makes it possible to use native-tls and disable rustls and includes an example for users demonstrating how to do it.

Testing

I included a test that ensures rustls is not in use

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.

update: aws-config to correctly separate the native-tls and rustls features for its dependencies
fix: native-tls feature gate typo
update: prefix unused id field with an underscore
@Velfi Velfi requested a review from a team as a code owner December 8, 2021 20:03
@github-actions
Copy link

github-actions bot commented Dec 8, 2021

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Dec 8, 2021

A new generated diff is ready to view.

id: &'static str,
_id: &'static str,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clippy was complaining about this locally and the field seemed unused so I added the underscore

Comment on lines +10 to +16
# aws-config pulls in rustls and several other things by default. We have to disable defaults in order to use native-tls
# and then manually bring the other defaults back
aws-config = { path = "../../build/aws-sdk/sdk/aws-config", default-features = false, features = ["default-provider", "native-tls", "rt-tokio", "dns", "tcp-connector"] }
# aws-sdk-s3 brings in rustls by default so we disable that in order to use native-tls only
aws-sdk-s3 = { package = "aws-sdk-s3", path = "../../build/aws-sdk/sdk/s3", default-features = false, features = ["native-tls"] }
# aws-sdk-sts is the same as aws-sdk-s3
aws-sdk-sts = { package = "aws-sdk-sts", path = "../../build/aws-sdk/sdk/sts", default-features = false, features = ["native-tls"] }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks to John's suggestion of removing default features for runtime crates, we don't even need to include them here!

aws/rust-runtime/aws-config/Cargo.toml Outdated Show resolved Hide resolved
aws/rust-runtime/aws-config/Cargo.toml Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Dec 8, 2021

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Dec 8, 2021

A new generated diff is ready to view.

@github-actions
Copy link

github-actions bot commented Dec 8, 2021

A new generated diff is ready to view.

@github-actions
Copy link

github-actions bot commented Dec 8, 2021

A new generated diff is ready to view.

@github-actions
Copy link

github-actions bot commented Dec 8, 2021

A new doc preview is ready to view.

@@ -32,7 +32,7 @@ dns = ["tokio/rt"]
default = ["default-provider", "rustls", "rt-tokio", "dns", "tcp-connector"]

[dependencies]
aws-sdk-sts = { path = "../../sdk/build/aws-sdk/sdk/sts", optional = true }
aws-sdk-sts = { path = "../../sdk/build/aws-sdk/sdk/sts", default-features = false, optional = true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the rustls and native-tls features in aws-config also enable the equivalent features on the STS crate? Or to reframe the question, is it working by coincidence right now without explicitly enabling those?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had that previously and removed it because it didn't seem to matter. The test runs and doesn't detect rustls even though aws-config is a dependency so I believe we're good. Personally, my gut tells me to include the features but I can't articulate why so I'm ignoring the feeling.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jdisanti they should not: this is because the only purpose of those in the STS crate are to support from_conf (constructing the client without a connection). But this crate will always provide a connection when constructing a client.

Comment on lines +34 to +39
let cargo_location = std::env::var("CARGO").unwrap();
let cargo_command = std::process::Command::new(&cargo_location)
.arg("tree")
.arg("--invert")
.arg("rustls")
.output()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Clever!

Does this differentiate between something being wrong with the cargo binary (not able to execute or something) vs. exiting with a non-zero status?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only checks that Cargo writes a specific message to stderr when the command is run. The possible paths I see are:

  1. rustls is not in tree, message is logged to stderr, we panic with that message and match on it with should_panic, it's a match so that's a test pass
  2. some other failure occurs, message is logged to stderr, we panic with that message and match on it with should_panic, It's not a match so that's a test fail
  3. rustls is in the tree, print tree and exit, test fails cause no panic

So, to answer your question: yes, it does differentiate

@github-actions
Copy link

github-actions bot commented Dec 9, 2021

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Dec 9, 2021

A new generated diff is ready to view.

@github-actions
Copy link

github-actions bot commented Dec 9, 2021

A new generated diff is ready to view.

@github-actions
Copy link

github-actions bot commented Dec 9, 2021

A new doc preview is ready to view.

@Velfi Velfi enabled auto-merge (squash) December 9, 2021 21:09
@github-actions
Copy link

github-actions bot commented Dec 9, 2021

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Dec 9, 2021

A new generated diff is ready to view.

@Velfi Velfi merged commit 979c703 into main Dec 9, 2021
@Velfi Velfi deleted the add/example_for_using_native_tls_attempt_2 branch December 9, 2021 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants