Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[#6050] http: add edge_accept_request_id field #7140

Merged
merged 22 commits into from
Jun 12, 2019
Merged

[#6050] http: add edge_accept_request_id field #7140

merged 22 commits into from
Jun 12, 2019

Conversation

atrifan
Copy link
Contributor

@atrifan atrifan commented Jun 3, 2019

Description

Add edge_accept_request_id property for the envoy.http_connection_manager filter. Field added to resolve #6050 and also maintain backward compatibility

Risk

Low - small feature disabled by default and maintaining backward compatibility

Testing

Added 2 additional integration tests in test/common/http/conn_manager_utility_test.c to validate behaviour for:

  1. edge request - activated edge_accept_request_id set to true but no x-request-id header sent - expected to generate a new one
  2. edge request - activated edge_accept_request_id set to true and sent x-request-id header sent - expected to keep the old one.
  3. all previous tests regarding edge requests resetting the x-request-id should still pass

Issues

Fixes #6050

@lizan lizan requested a review from jmarantz June 3, 2019 16:07
jmarantz
jmarantz reviewed Jun 3, 2019
View reviewed changes
Copy link
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically looks good; I admit I don't fully understanding the semantic ramifications of this. But I did offer a few minor improvements from just looking at the code.

source/common/http/conn_manager_utility.cc Outdated Show resolved Hide resolved
source/common/http/conn_manager_config.h Outdated Show resolved Hide resolved
test/common/http/conn_manager_utility_test.cc Outdated Show resolved Hide resolved
jmarantz
jmarantz reviewed Jun 4, 2019
View reviewed changes
Copy link
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm modulo testing nit.

source/common/http/conn_manager_utility.cc Outdated Show resolved Hide resolved
@jmarantz
Copy link
Contributor

jmarantz commented Jun 4, 2019

also you need to read about how to work DCO: https://github.com/envoyproxy/envoy/blob/master/CONTRIBUTING.md#dco-sign-your-work

Once you have your github hooks set up properly that will just work.

@atrifan atrifan force-pushed the 6050/x_req_id_sanitzation branch from 7faed7b to ee0b502 Compare June 5, 2019 09:34
@jmarantz
Copy link
Contributor

jmarantz commented Jun 5, 2019

something went awry in your github branch -- maybe an issue with the force-push? Looks like there are 479 files modified now.

@atrifan atrifan force-pushed the 6050/x_req_id_sanitzation branch 3 times, most recently from 7601e22 to 7faed7b Compare June 5, 2019 13:42
atrifan added 12 commits June 5, 2019 16:51
@atrifan
feature edge_accept_request_id flag enablement
d793ec5 
Signed-off-by: trifan <[email protected]>
@atrifan
autoformat
3c3fc3d 
Signed-off-by: trifan <[email protected]>
@atrifan
fixed missing comma
f0b83f0 
Signed-off-by: trifan <[email protected]>
@atrifan
added sanitization check for AcceptEdgeRequestId
8b87c4b 
Signed-off-by: trifan <[email protected]>
@atrifan
added tests
23235cd 
Signed-off-by: trifan <[email protected]>
@atrifan
format style
cb7ed16 
Signed-off-by: trifan <[email protected]>
@atrifan
updated release notes
fcd8791 
Signed-off-by: trifan <[email protected]>
@atrifan
removed unnecessary logs
db369b6 
Signed-off-by: trifan <[email protected]>
@atrifan
implemented @jmarantz code review feedback for source/common/http/con…
443a891 
…n_manager_utility.cc

Signed-off-by: trifan <[email protected]>
@atrifan
fixed formatting
ba5d821 
Signed-off-by: trifan <[email protected]>
@atrifan
revisteted @jmarantz code review changes for const
58b7520 
Signed-off-by: trifan <[email protected]>
@atrifan
implemented code review changes of @derekargueta
a6c629a 
Signed-off-by: trifan <[email protected]>
@atrifan atrifan force-pushed the 6050/x_req_id_sanitzation branch from 7faed7b to e1588f6 Compare June 5, 2019 13:52
@atrifan
Copy link
Contributor Author

atrifan commented Jun 5, 2019

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: clang_tidy (failed build)
🔨 rebuilding ci/circleci: release (failed build)
🔨 rebuilding ci/circleci: compile_time_options (failed build)
🔨 rebuilding ci/circleci: tsan (failed build)
🔨 rebuilding ci/circleci: asan (failed build)
🔨 rebuilding ci/circleci: coverage (failed build)
🔨 rebuilding ci/circleci: ipv6_tests (failed build)

🐱

Caused by: a #7140 (comment) was created by @atrifan.

see: more, trace.

@atrifan
Copy link
Contributor Author

atrifan commented Jun 5, 2019

/retest

@repokitteh-read-only
Copy link

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #7140 (comment) was created by @atrifan.

see: more, trace.

@atrifan
Copy link
Contributor Author

atrifan commented Jun 5, 2019

@jmarantz I have fixed the rebase. Can you check again? Thanks

@atrifan
Copy link
Contributor Author

atrifan commented Jun 7, 2019

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: ipv6_tests (failed build)

🐱

Caused by: a #7140 (comment) was created by @atrifan.

see: more, trace.

@atrifan
Copy link
Contributor Author

atrifan commented Jun 10, 2019

All tests are green can this be merged? Thanks

htuch
htuch suggested changes Jun 11, 2019
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just two nits and we can merge.

docs/root/intro/version_history.rst Outdated Show resolved Hide resolved
source/common/http/conn_manager_config.h Outdated Show resolved Hide resolved
@htuch htuch added the waiting label Jun 11, 2019
htuch
htuch approved these changes Jun 12, 2019
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@htuch htuch merged commit 0243ded into envoyproxy:master Jun 12, 2019
This was referenced Oct 21, 2019
Closed
Closed
@atrifan atrifan deleted the 6050/x_req_id_sanitzation branch March 23, 2020 14:21
@ardikabs ardikabs mentioned this pull request Apr 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 left review comments

@derekargueta derekargueta derekargueta left review comments

@jmarantz jmarantz jmarantz left review comments

@htuch htuch htuch approved these changes

@alyssawilk alyssawilk Awaiting requested review from alyssawilk

@lizan lizan Awaiting requested review from lizan

@snowp snowp Awaiting requested review from snowp

@zuercher zuercher Awaiting requested review from zuercher

Assignees

@jmarantz jmarantz

@htuch htuch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature request] Remove (make configurable) x-request-id request header sanitizing
5 participants
@atrifan @jmarantz @derekargueta @mattklein123 @htuch