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

ext_authz: add logging options #35698

Merged
merged 34 commits into from
Sep 12, 2024
Merged

Conversation

antoniovleonti
Copy link
Contributor

@antoniovleonti antoniovleonti commented Aug 13, 2024

Commit Message: ext_authz: add logging options / stats
Additional Description:

logging_options enables ext_authz to emit per-stream stats through filter state for access logging. The stats emitted depend on the client used.

If envoy GRPC is used, latency, bytes sent, bytes received, upstream host, and cluster info are emitted. Otherwise, only latency is emitted.

If logging_options.filter_metadata is present, ext_authz will also emit the filter metadata in its logging info.

ext authz will only emit stats & and filter metadata if a request is made.

Testing: unit & integration tests
Docs Changes: none
Release Notes: changelog updated
Platform Specific Features: n/a

Signed-off-by: antoniovleonti <leonti@google.com>
Signed-off-by: antoniovleonti <leonti@google.com>
Signed-off-by: antoniovleonti <leonti@google.com>
Signed-off-by: antoniovleonti <leonti@google.com>
Signed-off-by: antoniovleonti <leonti@google.com>
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #35698 was opened by antoniovleonti.

see: more, trace.

@antoniovleonti
Copy link
Contributor Author

/assign tyxia

@antoniovleonti
Copy link
Contributor Author

/retest

Signed-off-by: antoniovleonti <leonti@google.com>
@markdroth
Copy link
Contributor

/lgtm api

Signed-off-by: antoniovleonti <leonti@google.com>
Signed-off-by: antoniovleonti <leonti@google.com>
@adisuissa
Copy link
Contributor

@tyxia can you PTAL?

Signed-off-by: antoniovleonti <leonti@google.com>
@antoniovleonti
Copy link
Contributor Author

/retest

Signed-off-by: antoniovleonti <leonti@google.com>
Copy link
Member

@tyxia tyxia left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. Nice work!

Fleshing out the first pass of comments

Signed-off-by: antoniovleonti <leonti@google.com>
Signed-off-by: antoniovleonti <leonti@google.com>
Signed-off-by: antoniovleonti <leonti@google.com>
Signed-off-by: antoniovleonti <leonti@google.com>
Signed-off-by: antoniovleonti <leonti@google.com>
Signed-off-by: antoniovleonti <leonti@google.com>
@antoniovleonti antoniovleonti changed the title ext_authz: add emit_filter_state_stats option ext_authz: add logging options Aug 28, 2024
@phlax
Copy link
Member

phlax commented Sep 3, 2024

@markdroth @tyxia i think this may be waiting on further review

@antoniovleonti if you merge main the current arm fail should be resolved

Signed-off-by: antoniovleonti <leonti@google.com>
Signed-off-by: antoniovleonti <leonti@google.com>
Signed-off-by: antoniovleonti <leonti@google.com>
Signed-off-by: antoniovleonti <leonti@google.com>
@markdroth
Copy link
Contributor

/lgtm api

@repokitteh-read-only repokitteh-read-only bot removed the api label Sep 6, 2024
antoniovleonti and others added 3 commits September 9, 2024 08:09
Signed-off-by: Antonio V. Leonti <53806445+antoniovleonti@users.noreply.github.com>
Signed-off-by: antoniovleonti <leonti@google.com>
… into authzstatimpl

Signed-off-by: antoniovleonti <leonti@google.com>
Copy link
Member

@tyxia tyxia 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 nits. Nice work!

Please also merge main to resolve conflict. Thanks

source/extensions/filters/http/ext_authz/ext_authz.cc Outdated Show resolved Hide resolved
source/extensions/filters/http/ext_authz/ext_authz.h Outdated Show resolved Hide resolved
Signed-off-by: antoniovleonti <leonti@google.com>
Signed-off-by: antoniovleonti <leonti@google.com>
Signed-off-by: antoniovleonti <leonti@google.com>
@antoniovleonti
Copy link
Contributor Author

/retest

@tyxia
Copy link
Member

tyxia commented Sep 10, 2024

@antoniovleonti Looks like retest did not work, please merge main to re-trigger the CI. Thanks!

Signed-off-by: antoniovleonti <leonti@google.com>
@antoniovleonti
Copy link
Contributor Author

/retest

@antoniovleonti
Copy link
Contributor Author

/retest

Copy link
Member

@tyxia tyxia left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

Copy link
Member

@tyxia tyxia left a comment

Choose a reason for hiding this comment

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

The majority logic of this PR is under config knob. i.e. it should not impact the regular ext_authz logic. Thus, I am merging it.

@tyxia tyxia merged commit 1eedeee into envoyproxy:main Sep 12, 2024
41 checks passed
@antoniovleonti antoniovleonti deleted the authzstatimpl branch September 12, 2024 19:06
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.

5 participants