cloudflare / cloudflare/pingora
Support starting a `Server` with a `ServerConf`, and make `ServerConf` fields public
- Dominant language
- Rust
- Stars
- 27.4k
- Forks
- 1.7k
- Avg merge
- 6h 22m
- Merged PRs (30d)
- 3
Description
## 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.
Contributor guide
Assessment
This issue has not been assessed yet.