DefaultListener::bindable() isn’t usable about rocket HOT 8 CLOSED

This should definitely be improved, though I'll say that the intention behind the bindable() method isn't quite this use-case. Of course you wouldn't know that as the entire listener module is yet to be documented 🙃.

For your use-case in particular however, it sounds like you'd be better served defining your own structure with an EndPoint (and any other config, like TlsConfig) in it and creating your own bindable from that. Those parts are designed to be composable and reusable, whereas default listener is really only exposed to document the config parameters that Rocket uses for its listener by default. Given you mention actually wanting access to base_listener instead, which is mostly a simple cfg-aware match-and-extract, using your own config struct in your implementation will probably result in at least as clean an implementation as it otherwise would. And you get to avoid this particular shortcoming.

from rocket.

I meant to replicate what the default TlsListener is doing – I don’t want to change the behavior, merely the configuration. So making assumptions about the default listener wasn’t my intention, I’ve already had to make way too many assumptions about Rocket internals. The real code is actually wrapping the default listener. But I guess this cannot be helped…

from rocket.

I meant to replicate what the default TlsListener is doing – I don’t want to change the behavior, merely the configuration.

I'm not sure how to reconcile this with your initial post. This would imply that your use-case would be achievable by:

• Extracting the DefaultListener, as you did.
• Tweaking the TlsConfig value.
• Calling and listening on bindable().

And if this is the case, what would you gain from having access to base_bindable()?

So making assumptions about the default listener wasn’t my intention, I’ve already had to make way too many assumptions about Rocket internals.

Can you clarify? What assumptions are you having to make about either the default listener or any of Rocket's other internals? The listener module isn't documented yet, but you still shouldn't need to make any assumptions. The entire module is extremely type-directed. And the rest of Rocket is extensively documented.

The real code is actually wrapping the default listener. But I guess this cannot be helped…

Again, I'm really not sure what this means. It reads like a critique, but I'm not sure of what.

from rocket.

Tweaking the TlsConfig value.

Nope, tweaking the ServerConfig from Rustls. It has SNI support whereas the default TlsConfig in Rocket doesn’t. So I have a custom config structure with per-host certificate configuration and TlsConfig as fallback.

What assumptions are you having to make about either the default listener or any of Rocket's other internals?

While I can deserialize TlsConfig as it is, I have to translate it into Rustls ServerConfig manually. So if this structure changes in future Rocket versions, my code might break.

And since I’m not reusing DefaultListener, I’ve also decided to limit my implementation to TCP sockets because supporting other socket types like UDS is considerable complexity and will likely break as Rocket evolves.

Quite frankly, all of this feels like I’m doing way too much work. It wouldn’t be necessary if Rocket allowed providing a custom rustls::server::ServerConfig instance like other frameworks do.

from rocket.

Tweaking the TlsConfig value.

Nope, tweaking the ServerConfig from Rustls. It has SNI support whereas the default TlsConfig in Rocket doesn’t. So I have a custom config structure with per-host certificate configuration and TlsConfig as fallback.

It wouldn't be very difficult to modify TlsConfig to add this support and upstream it into Rocket. That would make your work here much much easier while benefitting everyone else, too.

What assumptions are you having to make about either the default listener or any of Rocket's other internals?

While I can deserialize TlsConfig as it is, I have to translate it into Rustls ServerConfig manually. So if this structure changes in future Rocket versions, my code might break.

We internally have a function that converts the TlsConfig to a rustls config. If we exposed that, it sounds like this qualm would dissipate. Again, something you could choose to contribute that would benefit others while making your life much easier.

But to be clear, there's no assumption to be made here. The structure is well defined with a documented API. Obviously if there's a breaking change that impacts you, then you need to update your code. But that would be a choice you make. And that would happen with any library, in any language, and has nothing to do with Rocket's internals nor making assumptions about them.

And since I’m not reusing DefaultListener, I’ve also decided to limit my implementation to TCP sockets because supporting other socket types like UDS is considerable complexity and will likely break as Rocket evolves.

No, neither part of this statement is true. Listeners and connections compose, and since Rocket provides a UdsListener already, there's almost zero work you need to do to support them. If your custom TlsListener is designed generically, this would just work. There's no reason for you to assume that something will break as Rocket evolves.

Quite frankly, all of this feels like I’m doing way too much work. It wouldn’t be necessary if Rocket allowed providing a custom rustls::server::ServerConfig instance like other frameworks do.

I think you are doing too much work, but I don't think it's largely because Rocket isn't exposing the right primitives here, though obviously with my commentary above I concede that Rocket can make this easier.

We don't expose rustls directly because it's an implementation detail. We could easily switch to s2n tomorrow with 0 impact to users. That being said, as I've said before, I'm not opposed to exposing anything relevant here, as long as we do this in the right way.

Finally, I'd like to ask you to be more understanding about the feature-set you're using and criticizing: the entire thing landed just weeks ago and is still unreleased. To my knowledge, no other web framework in Rust offers the level of flexibility and composability that we've managed to get with the new listener/connection interfaces. Your commentary would be much better received if instead of only criticizing the project, you instead sought to improve it.

from rocket.

Your commentary would be much better received if instead of only criticizing the project, you instead sought to improve it.

I’m not sure where you saw me criticizing the project. Let me assure you that I did attempt to solve this issue on my own, and I would have contributed a fix had I been successful. Unfortunately, my knowledge of Rust turned out to be insufficient.

Finally, I'd like to ask you to be more understanding about the feature-set you're using and criticizing: the entire thing landed just weeks ago and is still unreleased.

I am aware of that. That’s why I used a development version, with version 0.5 what I want cannot be done at all.

I am also aware of the primary use case for the new API. Still, it’s my assumption that there is value in trying out a new API and testing its limitations before it is released to everyone. Particularly things that look like they should work but don’t.

It wouldn't be very difficult to modify TlsConfig to add this support and upstream it into Rocket.

Ok, I can create a pull request. I think the best approach would be adding a new field to_rustls: Option<Fn(&Self, &Rocket) -> ServerConfig> to TlsConfig. Presumably, I can make it deserialize as None while still allowing to set tls.to_rustls in the figment programmatically.

There should also be a TlsConfig.default_to_rustls(&self) -> ServerConfig method to be used as fallback if the new field is None. It can also be called by custom implementations to provide an initial config which can then be modified. This would contain the code currently in TlsConfig::acceptor().

Obviously, documentation should containing a warning that Rustls (and its particular version) is an implementation detail which can change in future Rocket versions.

from rocket.

Ok, I can create a pull request. I think the best approach would be adding a new field to_rustls: Option<Fn(&Self, &Rocket) -> ServerConfig> to TlsConfig. Presumably, I can make it deserialize as None while still allowing to set tls.to_rustls in the figment programmatically.

I think all we need here are two things:

1. A TryFrom<TlsConfig> for rustls::ServerConfig implementation that simply calls the existing method (it's server_config(), currently on the h3 branch https://github.com/rwf2/Rocket/blob/h3/core/lib/src/listener/tls.rs)

2. A new crate feature, rustls or tls-rustls that this method is gates on.

Obviously, documentation should containing a warning that Rustls (and its particular version) is an implementation detail which can change in future Rocket versions.

A warning is insufficient. This needs to be a static guarantee, and a feature gate is the correct way to approach this.

I don't believe we need any additional functionality than this.

from rocket.

How would a user provide a custom TlsConfig => ServerConfig conversion then? Even if it were possible to overwrite a trait implementation, Rust won’t let you write one (neither TlsConfig nor ServerConfig defined in current crate).

Note also that my suggestion takes the Rocket instance as input. That’s because a custom conversion might need additional configuration settings, so it needs access to Rocket::figment().

from rocket.