cloudflare / pingora

A library for building fast, reliable and evolvable network services.
Apache License 2.0
20.3k stars 1.1k forks source link

Support starting a `Server` with a `ServerConf`, and make `ServerConf` fields public #159

Open jamesmunns opened 3 months ago

jamesmunns commented 3 months ago

What is the problem your feature solves, or the need it fulfills?

At the moment, there is no constructor for Server that allows you to fully set the contents of ServerConf. You can create a server with an Opt that has a None field for a conf path, and the configuration field of Server is public, but not all fields of ServerConf are public:

https://github.com/cloudflare/pingora/blob/0de54eb9071a9c4baccc6bad7acad11e9c54186f/pingora-core/src/server/configuration/mod.rs#L36-L69

This is a little silly though - ServerConf implements Deserialize, which means I could write my own contents in JSON or YAML and deserialize it myself, meaning that the non-public fields are not effective if the intent is to prevent user modification.

For the river application, we will likely have our own configuration file format, so it would be good to be able to disable (or at least skip) the built-in YAML configuration format used by the pingora crate. It may be good to split this functionality into an optional pingora-config crate, as "frontends" to pingora like river may want to use their own.

Describe the solution you'd like

  1. Create a new constructor to Server that takes ServerConf directly (and maybe not Opt at all?)
  2. Make all fields of ServerConf public, or provide some methods to be able to set all fields
  3. (optional) split out the configuration file handling into a separate crate, to reduce dependencies (such as StructOpt) and build time

Describe alternatives you've considered

If this is not implemented, I will need to either:

  1. be forced to use and extend the YAML file used by pingora, making that a stability issue for river to ensure compatibility with new items added by pingora in the future
  2. Use a somewhat hacky "deserialize to sneak in fields" workflow

Additional context

This is for the river application.

drcaramelsyrup commented 3 months ago

As discussed in #165, IMO at least I'm not opposed to separating out the yaml/config file handling itself into a separate crate and keeping ServerConf agnostic, perhaps included alongside other Conf per service type (e.g. proxy-specific config) in a base config file.

drcaramelsyrup commented 3 months ago

@jamesmunns I think separating the config handling out into its own crate deserves its own issue. Would you mind if I opened a separate issue for that and closed this one?

jamesmunns commented 3 months ago

@drcaramelsyrup Up to you! There's two parts:

I think having a specific issue for the latter is good, and happy to weigh in/answer any questions!