Flexible / sane header forwarding #5103
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Blindly copying all inbound headers to outbound is unamibguously wrong, but the exact list that //is// appropriate is complex and not necessarily the same everywhere. So this adds a semi-flexible config-driven rule for controlling this. It is in dynamic config currently because it's easier for us to change internally, while trying to build a smarter list of headers - it belongs in static config, and possibly dynamic config *after refreshing is added*.
davidporter-id-au
approved these changes
Feb 17, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Blindly forwarding all inbound headers to outbound requests is unambiguously wrong, and causing problems.
This adds a semi-flexible system to allow controlling what headers are forwarded, via static and dynamic config.
Dynamic config is currently only loaded at startup, but at least internally it's still quicker than changing static config. We should consider making this truly dynamic.
As this has historically been copying ?? to outbounds, leading to ?? total headers: the current version does not change anything. We'll likely experiment internally to make sure our new sets of headers are safe with our setup, and will make a separate PR to tighten down the list somewhat when we've confirmed that it works + is likely to work in other clusters.
Why this kind of add/remove/regex list structure? No strong reason, I've just used it in previous projects pretty happily. Only-inclusive or only-exclusive has been insufficient every time, but I've had this in a couple services for years without needing more.
Longer-term we probably want this to be truly dynamically updatable and/or fully pluggable. If we're lucky we might not need more than this though, and driving it via config is pretty convenient and easily understood.