search

cloudflare / foundations

Cloudflare's Rust service foundations library.
https://blog.cloudflare.com/introducing-foundations-our-open-source-rust-service-foundation-library
BSD 3-Clause "New" or "Revised" License
1.25k stars 51 forks source link

ZTC-1545: Log a warning when parsing settings if yaml contains unused keys #43

Closed OmegaJak closed 3 months ago

OmegaJak commented 4 months ago

This uses serde_ignored while deserializing settings from YAML to log a warning if keys are unused. This could help prevent accidental misconfigurations of services. For example, if the setting is reverted in code without also changing the name in the YAML, a warning will be logged for that key.

There were three things complicating this implementation:

  1. Settings deserialization generally occurs before the log is initialized. This is why I changed the logger used before initialization from a Discard logger to one using the default LoggingSettings. This way, warnings can be logged to stdout at least, following the usual format, while deserializing settings.
  2. Dependencies of foundations may be serializing and re-deserializing their Settings with omitted fields to remove data from copies of the settings. In this case, we do not want warnings logged for those omitted fields. Thus, an additional method was added to deserialize from a yaml string with an option controlling what action is taken when an ignored field is encountered.
  3. YAML fields that are only used as an anchor & merge key will appear as unused. To let applications avoid this false positive, there is a suffix that can be added to keys that will prevent the warning from being logged.
inikulin commented 4 months ago

Settings deserialization generally occurs before the log is initialized. This is why I changed the logger used before initialization from a Discard logger to one using the default LoggingSettings. This way, warnings can be logged to stdout at > least, following the usual format, while deserializing settings.

This is undesired behaviour. We don't want any implicit behavior or reporting unless settings for that are specified explicitly. This is one of the key principles of the library and we won't change it.

If something in settings can cause an incident, then that should be a hard error (i.e. reported to sentry and terminate the process), rather than a "warning" in operational logs.

OmegaJak commented 3 months ago

Closing in favor of #49 following internal discussion