-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
udp_proxy: support proxy access logging #23242
Conversation
Signed-off-by: giantcroc <changran.wang@intel.com>
Signed-off-by: giantcroc <changran.wang@intel.com>
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
/retest |
Retrying Azure Pipelines: |
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.
API review
Signed-off-by: giantcroc <changran.wang@intel.com>
/retest |
Retrying Azure Pipelines: |
/retest |
Retrying Azure Pipelines: |
@phlax Hi, it seems wierd that the old code is still used to do test
Can you help with it? Thanks! |
@giantcroc CI runs on merge commit and you can see that part code here: https://github.com/envoyproxy/envoy/blob/e143e084f5/test/extensions/filters/udp/udp_proxy/udp_proxy_filter_test.cc#L629-L639 It's a merge race with #23121, you just need to merge main branch and fix that test as well. |
OK, fixed, thanks a lot! |
Signed-off-by: giantcroc <changran.wang@intel.com>
/retest |
Retrying Azure Pipelines: |
/retest |
Retrying Azure Pipelines: |
The coverage failure is a real one so you either need increase coverage or change threshold. |
Signed-off-by: giantcroc <changran.wang@intel.com>
Hi, lizan! Sorry for late reply. The coverage has been fixed after rebase. Thanks! |
Signed-off-by: giantcroc <changran.wang@intel.com>
/retest |
Retrying Azure Pipelines: |
Signed-off-by: giantcroc <changran.wang@intel.com>
Signed-off-by: giantcroc <changran.wang@intel.com>
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.
Should remove this line in this pr?
// TODO(mattklein123): UDP session access logging. |
Signed-off-by: giantcroc <changran.wang@intel.com>
OK, thanks! |
@lizan Hi, I have recovered the field name to address compatibility issue. Please help review, thanks! |
changelogs/current.yaml
Outdated
@@ -39,6 +39,56 @@ new_features: | |||
|
|||
- area: http | |||
change: | | |||
added the expected :ref:`receive <envoy_v3_api_field_config.core.v3.HealthCheck.HttpHealthCheck.receive>` payload check for HTTP health check. |
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.
there's a lot of unrelated changes in this version history which might comes from resolving conflicts, please merge main again and add only what you did in this PR.
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.
OK, fixed, thanks!
Signed-off-by: giantcroc <changran.wang@intel.com>
Signed-off-by: giantcroc <changran.wang@intel.com>
Signed-off-by: giantcroc changran.wang@intel.com
Commit Message:
Some stats like
no_route
andidle_timeout
can't be printed by session access log,so we need proxy-level access logging to log global stats.
Additional Description:
Risk Level: Low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue] #23241
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]