Skip to content

Endpoint - allowing override server configuration - #1191

Merged
djc merged 1 commit into
quinn-rs:mainfrom
BiagioFesta:bfesta/override-config-server
Sep 20, 2021
Merged

Endpoint - allowing override server configuration#1191
djc merged 1 commit into
quinn-rs:mainfrom
BiagioFesta:bfesta/override-config-server

Conversation

@BiagioFesta

@BiagioFesta BiagioFesta commented Sep 18, 2021

Copy link
Copy Markdown
Contributor

The idea here is to reload the server configuration allowing a new setup for incoming connections without dropping the endpoint and closing existing ones.

The main rationale is the fact that, in some circumstances, on long server sessions, it is needed to reload its TLS configuration. In particular, to have the possibility to renew an expiring TLS certificate keeping the server/service operative.


Pseudo Code Usage Idea
let (endpoint, mut incoming) = endpoint.bind(&listen_addr).unwrap();

loop {
  select! {
      Some(incoming) = incoming.next() => {
          // Handle new incoming connection
       }
       new_config = config_reload.next() => {
           let new_server_config = build_new_server_config(&config);
           endpoint.override_server_config(new_server_config);
        }
    }
}

@Ralith Ralith left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the PR! This will be great to have; I honestly didn't realize how easy it would be.

Comment thread quinn-proto/src/endpoint.rs Outdated
Comment thread quinn/src/endpoint.rs Outdated
Comment thread quinn/src/endpoint.rs Outdated
@BiagioFesta
BiagioFesta force-pushed the bfesta/override-config-server branch 2 times, most recently from 48c08a2 to b87ef98 Compare September 18, 2021 22:36
@BiagioFesta
BiagioFesta requested a review from Ralith September 18, 2021 23:10
Comment thread quinn/src/endpoint.rs Outdated
@BiagioFesta
BiagioFesta force-pushed the bfesta/override-config-server branch from b87ef98 to f8723bc Compare September 18, 2021 23:13
@BiagioFesta
BiagioFesta requested a review from Ralith September 18, 2021 23:39

@Ralith Ralith left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks!

Comment thread quinn/src/endpoint.rs

/// Replace the server configuration, affecting new incoming connections only
///
/// Useful for e.g. refreshing TLS certificates without disrupting existing connections.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You should already be able to do this with the current config, since the cert is not necessarily static but a lookup callback. Same applies for other functions for which "providers" can be hooked in (like the CidGenerator).

I guess it won't hurt to swap the complete config instead of just adding more callbacks. But the question is whether swapping this config dynamically might break anything that doesn't expect the values to change over time.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You should already be able to do this with the current config

Good point. Setters are a lot more convenient than setting up a sidechannel, though, and it might be useful to adjust some of the other options on the fly non-disruptively as well.

But the question is whether swapping this config dynamically might break anything

I've audited this and I'm satisfied that there aren't any issues with the current set of config options. We actually don't read the server config in very many places; the overwhelming bulk is in Endpoint::handle_first_packet. At worst, swapping out token_key might cause spurious failure for in-flight handshakes, but that's trivially recoverable and far less disruptive than restarting the endpoint outright. It's also nice for rotating token_key to be enabled in its own right.

What could break things would be changing the config for an existing connection, but existing connections hold onto the Arc of the config they were created with, so that's not a risk.

}

/// Replace the server configuration, affecting new incoming connections only
pub fn set_server_config(&mut self, server_config: Option<Arc<ServerConfig<S>>>) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why allow Option? For servers, this must always be Some.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewing the code, it looks to me like this will allow enabling/disabling server operation at will, which seems somewhere between harmless and useful, and less surprising than silently doing nothing. I guess we could omit the Option and let it be possible to enable server mode but not disable it, but why not be symmetrical?

@djc
djc merged commit 9bb4971 into quinn-rs:main Sep 20, 2021
@BiagioFesta
BiagioFesta deleted the bfesta/override-config-server branch November 6, 2021 09:52
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.

4 participants