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.

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

access log: add CONNECTION_TERMINATION_DETAILS operator #13305

Merged
merged 6 commits into from
Oct 6, 2020

Conversation

yangminzhu
Copy link
Contributor

@yangminzhu yangminzhu commented Sep 29, 2020

Follow up of #12912 (comment) for adding a new operator for network filter instead of using RESPONSE_CODE_DETAILS.

I actually wonder would it be better to also use RESPONSE_DETAILS for HTTP to deprecate the RESPONSE_CODE_DETAILS so that the user can use the same operator for both HTTP and TCP? cc @alyssawilk

cc @mandarjog

Signed-off-by: Yangmin Zhu ymzhu@google.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message:
Additional Description:
Risk Level: Low
Testing: Unit tests and integration tests
Docs Changes: Updated
Release Notes: Updated
Issue: #12873

Signed-off-by: Yangmin Zhu <ymzhu@google.com>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #13305 was opened by yangminzhu.

see: more, trace.

Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

Cool, I think this will be useful for L4 debug.

I think keeping both L4 and L7 makes sense to me - given ordering I wouldn't want say an HTTP invalid header response (which closes the connection) get overwritten by L4 details (which would include the local close, and maybe if all the data was flushed or not)

api/envoy/data/accesslog/v3/accesslog.proto Outdated Show resolved Hide resolved
@@ -10,6 +10,10 @@ block-list (DENY) set of policies based on properties of the connection (IPs, po
This filter also supports policy in both enforcement and shadow modes. Shadow mode won't effect real
users, it is used to test that a new set of policies work before rolling out to production.

When a request is denied, the :ref:`RESPONSE_DETAILS<config_access_log_format_response_details>`
Copy link
Member

Choose a reason for hiding this comment

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

How do we know it's the RBAC filter setting this? I.e. don't we both need to know the response details and which network filter sets this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The RBAC filter will set it in the format of rbac_access_denied_matched_policy[policy_name], this should tell which filter sets it. Also updated the doc to make this more clear.

Signed-off-by: Yangmin Zhu <ymzhu@google.com>
@yangminzhu yangminzhu changed the title access log: add RESPONSE_DETAILS for TCP access log: add TERMINATION_DETAILS for TCP Sep 29, 2020
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

Looks great - just 2 minor nits and you're good to go!

api/envoy/data/accesslog/v3/accesslog.proto Outdated Show resolved Hide resolved
docs/root/configuration/observability/access_log/usage.rst Outdated Show resolved Hide resolved
Signed-off-by: Yangmin Zhu <ymzhu@google.com>
alyssawilk
alyssawilk previously approved these changes Oct 1, 2020
@mattklein123
Copy link
Member

Can you merge main and take a look at CI? Not sure what is going on there, but seems related to a recent commit.

/wait

@akonradi
Copy link
Contributor

akonradi commented Oct 1, 2020

//test/common/http:conn_manager_impl_test is currently failing at head

@mattklein123
Copy link
Member

Yup sorry just realized that. Let's wait until that is fixed and then I can take a final pass.

Signed-off-by: Yangmin Zhu <ymzhu@google.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

A few questions/comments. Thank you!

/wait

include/envoy/stream_info/stream_info.h Outdated Show resolved Hide resolved
api/envoy/data/accesslog/v3/accesslog.proto Outdated Show resolved Hide resolved
Signed-off-by: Yangmin Zhu <ymzhu@google.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks LGTM with one nit.

/wait

include/envoy/stream_info/stream_info.h Outdated Show resolved Hide resolved
Signed-off-by: Yangmin Zhu <ymzhu@google.com>
@yangminzhu yangminzhu changed the title access log: add TERMINATION_DETAILS for TCP access log: add CONNECTION_TERMINATION_DETAILS operator Oct 5, 2020
@mattklein123 mattklein123 merged commit 105e3f1 into envoyproxy:master Oct 6, 2020
@yangminzhu yangminzhu deleted the rbac-log branch October 6, 2020 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants