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

allow multiple tls features #237

Closed
wants to merge 1 commit into from

Conversation

esheppa
Copy link
Contributor

@esheppa esheppa commented Aug 23, 2022

Currently the main branch fails to compile if multiple tls features are selected. This is an attempt at one option to enable this by storing the TlsStreams in an enum and delegating to the current variant. This also allows a choice of tls crate in the config where multiple features are enabled.

@esheppa esheppa force-pushed the additive-tls-features branch 2 times, most recently from 0f79fad to 17575d2 Compare August 23, 2022 12:05
@pimeys
Copy link
Contributor

pimeys commented Aug 23, 2022

I'm currently on vacation, so I cannot spent too much time reviewing this until end of the week. Could you maybe explain a bit more what this fixes, what kind of a situation is broken right now?

@esheppa
Copy link
Contributor Author

esheppa commented Aug 23, 2022

No probs at all, I'm not in a rush to get this merged, please take your time 😄.

I'll leave the explanation here so you can read it when you get back.

The main issue is that cargo features should be additive but in this case vendored-openssl, native-tls and rustls are all mutually exclusive becuase each one currently imports a type with the same name: TlsStream. This should rarely be an issue as tiberius will commonly be a dependency of binaries rather than libraries, but in some rare cases a library will have a dependency on tiberius and in these cases, if the library chooses a TLS implementation, then that same choice is required by all binary users of that library, however native-tls is enabled by default .

There are a few different options here:

  • Leave it as-is, the current state is very unlikely to affect users.
  • Document that if using tiberius in a library, none of the TLS features should be selected (meaning they must specify default-features = false, and/or don't include any TLS implementations in the default features.
  • Allow the TLS implementation to be decided at runtime when there are more than one enabled (this PR)
  • Use this PR but investigate ways to make it more efficient in the case that it imposes a performance penalty (for example, only engage the new code in the case where more than one TLS implementation is enabled)
  • Probably other options that I've not thought through fully, for example having each TLS implementation as a seperate crate

fix imports

fix vendored-openssl

fmt
@khanage
Copy link

khanage commented Aug 31, 2023

Hi @esheppa, I ran into this issue as well and independently came up with a solution, only running into this after I went to open a PR 🤦. When @pimeys gets back perhaps we can pick one or the other?

@pimeys
Copy link
Contributor

pimeys commented Aug 31, 2023

Hey, sorry for no answering but I don't work in Prisma anymore and I gave up my admin rights to this repo. Please merge and continue working on it without me.

All the best!

@tomhoule
Copy link
Contributor

I like the simplicity of the solution in #308, and I agree features should be additive. Are there notable examples of crates where features are mutually exclusive? (nothing comes to my mind).

This, combined with #305, should make TLS implementation handling simpler.

@tomhoule
Copy link
Contributor

So I guess my point is now: what do you all think? I'd be in favour of adding a bit of documentation for the new behaviour and merging #308

@esheppa
Copy link
Contributor Author

esheppa commented Aug 31, 2023

I think #308 looks like a much better option, provided the selection order in the case of multiple features is documented as you suggested

@esheppa esheppa closed this Aug 31, 2023
@esheppa esheppa deleted the additive-tls-features branch September 8, 2023 00:39
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.

None yet

4 participants