Add warning for TLS #609
Add warning for TLS #609
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
chrisstaite-menlo
force-pushed
the
tonic_tls_hint
branch
from
4bdd77b
to
54bf387
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained
nativelink-util/src/tls_utils.rs line 75 at r1 (raw file):
}; if endpoint.scheme_str() != Some("https") { tracing::warn!("You have set TLS configuration on {endpoint}, these will be ignored because the scheme is insecure. Set to https or grpcs.");
I'm thinking maybe we should not allow TLS to be configured if "https" is not used. What do you think about returning an error here instead?
It looks like all places that use this function are all on construction, so in theory the program would just not start.
|
Previously, allada (Nathan (Blaise) Bruer) wrote…
I considered an error, but couldn't decide. Happy to go either way. |
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained
nativelink-util/src/tls_utils.rs line 75 at r1 (raw file):
Previously, chrisstaite-menlo (Chris Staite) wrote…
I considered an error, but couldn't decide. Happy to go either way.
Yeah lets do an error. Devs will have a higher chance of reading stderr/out if it crashes than at runtime.
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 0 of 1 LGTMs obtained
chrisstaite-menlo
force-pushed
the
tonic_tls_hint
branch
from
54bf387
to
d774b62
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Vercel, pre-commit-checks
nativelink-util/src/tls_utils.rs line 75 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
Yeah lets do an error. Devs will have a higher chance of reading stderr/out if it crashes than at runtime.
Done.
Tonic has changed its behaviour such that the TLS configuration is ignored within transport/service/connector.rs if the scheme isn't https. Firstly, add support for grpcs scheme by swapping it out in the endpoint Uri if set. Secondly, add a warning to the unsuspecting user that sets the TLS configuration, but not the correct scheme.
chrisstaite-menlo
force-pushed
the
tonic_tls_hint
branch
from
d774b62
to
d9c34f9
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:complete! 2 of 1 LGTMs obtained
Description
Tonic has changed its behaviour such that the TLS configuration is ignored within transport/service/connector.rs if the scheme isn't https. Firstly, add support for grpcs scheme by swapping it out in the endpoint Uri if set. Secondly, add a warning to the unsuspecting user that sets the TLS configuration, but not the correct scheme.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
I spent a long time trying to figure out why my TLS configuration wasn't working with the Tonic upgrade to 0.10.
Checklist
bazel test //...passes locallygit amendsee some docsThis change is