Skip to content

router: set upstream connection filter state in downstream request stream info#15543

Merged
snowp merged 6 commits intoenvoyproxy:mainfrom
eziskind:upstreamfilterstate
Mar 24, 2021
Merged

router: set upstream connection filter state in downstream request stream info#15543
snowp merged 6 commits intoenvoyproxy:mainfrom
eziskind:upstreamfilterstate

Conversation

@eziskind
Copy link
Copy Markdown
Contributor

This makes it possible to lookup upstream connection filter state from a downstream HTTP access log, similar to #10458 which does the same for upstream HTTP access logs.

Risk Level: low
Testing: unit tests
Fixes #13166

Signed-off-by: Elisha Ziskind eziskind@google.com

…ream info

Signed-off-by: Elisha Ziskind <eziskind@google.com>
@eziskind
Copy link
Copy Markdown
Contributor Author

@snowp @ggreenway Could you take a look at this since it's related to #10457?

Copy link
Copy Markdown
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

Thanks! Just one small nit

Comment thread test/common/router/router_test.cc
Signed-off-by: Elisha Ziskind <eziskind@google.com>
snowp
snowp previously approved these changes Mar 19, 2021
Copy link
Copy Markdown
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

Thanks!

@snowp
Copy link
Copy Markdown
Contributor

snowp commented Mar 22, 2021

Seems like there is an unrelated failure in clang tidy, can you try merging main?

Signed-off-by: Elisha Ziskind <eziskind@google.com>
Signed-off-by: Elisha Ziskind <eziskind@google.com>
@eziskind
Copy link
Copy Markdown
Contributor Author

Seems like there is an unrelated failure in clang tidy, can you try merging main?

Done. Also fixed an unrelated format issue in test/test_common/environment.cc

Signed-off-by: Elisha Ziskind <eziskind@google.com>
@eziskind
Copy link
Copy Markdown
Contributor Author

eziskind commented Mar 22, 2021

Seems like there is an unrelated failure in clang tidy, can you try merging main?

Done. Also fixed an unrelated format issue in test/test_common/environment.cc

Hmm, looks like running "tools/code_format/check_format.py fix test/test_common/environment.cc" generated an unacceptable change, so I'm reverting that.

@snowp
Copy link
Copy Markdown
Contributor

snowp commented Mar 23, 2021

Seems like the clang tidy errors are being picked up since this changes code close to the code that fails clang tidy. Do you think you can take a look to see if they are easy to fix? I think this PR will be blocked on them being resolved

Signed-off-by: Elisha Ziskind <eziskind@google.com>
@eziskind
Copy link
Copy Markdown
Contributor Author

Seems like the clang tidy errors are being picked up since this changes code close to the code that fails clang tidy. Do you think you can take a look to see if they are easy to fix? I think this PR will be blocked on them being resolved

Ok, trying again. It seems like a clang-tidy false positive where it incorrectly thinks that there is a null pointer access. I added a NOLINT comment to suppress this.

@eziskind
Copy link
Copy Markdown
Contributor Author

Seems like the clang tidy errors are being picked up since this changes code close to the code that fails clang tidy. Do you think you can take a look to see if they are easy to fix? I think this PR will be blocked on them being resolved

Ok, trying again. It seems like a clang-tidy false positive where it incorrectly thinks that there is a null pointer access. I added a NOLINT comment to suppress this.

@lizan fyi about this false-positive clang-tidy error.

Copy link
Copy Markdown
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

Thanks!

@snowp snowp merged commit 6936ea4 into envoyproxy:main Mar 24, 2021
@eziskind eziskind deleted the upstreamfilterstate branch March 24, 2021 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Retrieving socket options in access loggers

3 participants