temporalio / temporalio/temporal
config: strict mode for configuration parsing
@yiminc is already working on this.
Since Jan 6, 2022.
- Dominant language
- Go
- Stars
- 23.2k
- Forks
- 1.9k
- Avg merge
- 2d 8h
- Merged PRs (30d)
- 228
Description
Is your feature request related to a problem? Please describe.
My team is working on a new deployment of Temporal and we recently spent an entire day debugging an issue related to a missing key in configuration due to lack of error messaging and validation by the bootstrap code.
Example:
metrics:
hostPort: "127.0.0.1:8125"
prefix: "temporal"
needed to be
metrics:
statsd:
hostPort: "127.0.0.1:8125"
prefix: "temporal"
The server started up fine, the logic to configure the bootstrap code looked for any of the custom configured reporters, didn't find any, and everything booted as normal, only we were not getting any stats.
Describe the solution you'd like
I would like the option for strict config validation. If a key is unrecognized, the server should fail to start so it can be corrected.
I'm happy to put together a PR for option 1 below if someone can give their thoughts and approval on the approach.
Describe alternatives you've considered
-
Option 1
Change YAML unmarshaling to reject unknown fields (see yaml.Decoder.KnownFields), and ideally expand the use of thegopkg.in/validator.v2throughout the config structs.
Could we move to this as a default? If not add a flag which the server supports to unmarshal the config in strict mode. -
Option 2
Move to protobuf for configuration specifications. This would have the benefit of centralizing config specifications in a single place and definition language rather than scattering structs throughout the code base.
I previously made this suggestion in a `#development Slack thread:
Has the team considered moving to protobuf for the config schema? discovery and validation are subpar with how it's all done with structs currently. Example from our project lyft/clutch which uses proto for defining config and uses envoyproxy/protoc-gen-validate for validation:
- Config definition: api/config/gateway/v1/gateway.proto.
- Example YAML conforming to definition: clutch-config.yaml
- Example output from a bad config:
{"level":"fatal","ts":1641326218.3299987,"caller":"gateway/config.go:71","msg":"configuration proto validation failed","file":"clutch-config.yaml","error":"invalid Config.Gateway: embedded message failed validation | caused by: invalid GatewayOptions.Listener: value is required"}
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Assessment
This issue has not been assessed yet.