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

Keep TLS transport items together in a single module #1778

Closed
wants to merge 6 commits into from
Closed

Conversation

djc
Copy link
Contributor

@djc djc commented Jul 8, 2024

Counterproposal to #1763.

@djc djc requested a review from tottoto July 8, 2024 14:19
@djc djc changed the title Tls transport Keep TLS transport items together in a single module Jul 8, 2024
@tottoto
Copy link
Collaborator

tottoto commented Jul 8, 2024

The modules are already nicely separated correspoinding to their roles, this change feels like it breaks that.

@djc
Copy link
Contributor Author

djc commented Jul 8, 2024

The modules are already nicely separated correspoinding to their roles, this change feels like it breaks that.

What's the difference in role between transport::service::tls and transport::tls? Having those separate doesn't really make sense to me either conceptually or in terms of complexity (both are very small modules, so having them in separate files just makes you have to bounce between modules to make sense of what's going on).

@tottoto
Copy link
Collaborator

tottoto commented Jul 8, 2024

transport::service::tls module is the utilities used by the channel and server service modules and transport::tls is used by the transport wide implementations related to tls.

@LucioFranco
Copy link
Member

@djc I am not totally following the motivation for this? Could you expand more and if its a counter proposal to the other PR it would be good to get a summary of why its a counter proposal.

@djc
Copy link
Contributor Author

djc commented Aug 20, 2024

@djc I am not totally following the motivation for this? Could you expand more and if its a counter proposal to the other PR it would be good to get a summary of why its a counter proposal.

The main difference IMO is that this PR merges more of the TLS infrastructure (which is currently kind of spread over 4 different tls.rs modules) to live in a single module, and to expose a smaller interface between that module (transport::tls) and the modules that depend on it.

This PR:

  • Certificate::to_der_certificates(&self) -> Result<Vec<CertificateDer<'static>>, TlsError>
  • Identity::to_der_parts(&self) -> Result<(Vec<CertificateDer<'static>>, PrivateKeyDer<'static>), TlsError>

Other PR:

  • pub(crate) fn convert_certificate_to_pki_types(certificate: &Certificate) -> Result<Vec<CertificateDer<'static>>, TlsError>
  • pub(crate) fn convert_identity_to_pki_types(identity: &Identity) -> Result<(Vec<CertificateDer<'static>>, PrivateKeyDer<'static>), TlsError>

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

I think I am more infavor of #1763 due to two things 1) its a smaller change that does less and 2) I kinda agree with @tottoto that the way things are laid out is done on purpose. To me the main transport::tls file is designed to only contain the public tls infrastructure while the rest is designed to contain the internal transport things like the tls error for example.


pub(crate) use self::grpc_timeout::GrpcTimeout;
#[derive(Debug, Clone)]
pub(crate) struct GrpcTimeout<S> {
Copy link
Member

Choose a reason for hiding this comment

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

what is the benefit of having this in the mod file compared to what we had before? And why is this relevant to this specific PR around tls?

@djc djc closed this Sep 12, 2024
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