-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
router: add config to send date
header with requests
#11915
Closed
Closed
Changes from 13 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
c70f1e0
wip
rebello95 db280fb
updates
rebello95 37b3759
updates
rebello95 646fb5c
Merge remote-tracking branch 'origin/master' into date-request-header
rebello95 fde079b
Update date_provider_impl_test.cc
rebello95 7754eee
wip test
rebello95 89ed251
fixups
rebello95 cb03c2b
CR: use RequestOrResponseHeaderMap
rebello95 0dbfaa7
add to retries
rebello95 5b6309c
cleanup
rebello95 320d795
Update router_test.cc
rebello95 7b45763
CR: todo
rebello95 e446dc3
Merge remote-tracking branch 'origin/master' into date-request-header
rebello95 fe92c8e
Merge remote-tracking branch 'origin/master' into date-request-header
rebello95 755247f
add integration test
rebello95 ad3bd58
Merge remote-tracking branch 'origin/master' into date-request-header
rebello95 00ab63d
Merge remote-tracking branch 'origin/master' into date-request-header
rebello95 ec693b5
CR/CI
rebello95 5160d49
fixup
rebello95 6d4aa21
tests work locally now
rebello95 ca373a6
Merge remote-tracking branch 'origin/master' into date-request-header
rebello95 3eee495
fix windows
rebello95 a49c12a
Merge remote-tracking branch 'origin/master' into date-request-header
rebello95 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
16 changes: 10 additions & 6 deletions
16
api/envoy/extensions/filters/http/router/v4alpha/router.proto
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
16 changes: 10 additions & 6 deletions
16
generated_api_shadow/envoy/extensions/filters/http/router/v3/router.proto
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
16 changes: 10 additions & 6 deletions
16
generated_api_shadow/envoy/extensions/filters/http/router/v4alpha/router.proto
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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
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
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
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
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
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This config option is forever effectively. If we think this is not the right place for this ultimately, we shouldn't add this at all. Per @PiotrSikora I do wonder whether there is a way you could hack this somehow in envoy mobile if you think we want to move this. The hack would be to grab the created API listener and using dynamic_casts get at the underlying impl and toggle a bool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that the config option will become a part of the public API forever is a fair call out. I'm pausing on this for now while I sync up with our team internally to see if there are other options aside from upstreaming this change (including if your workaround suggestion could solve the issue). Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One option would be to keep the
add_request_date_header
option as part of the public API (since it's a valid behavior), which would only add theDate
request header to the original request (if absent) and retain the same value across all retries in Envoy Proxy, but which would keep updating theDate
request header in Envoy Mobile.