Skip to content

extension: enable tracing for the external authorization HTTP client implementation.#7267

Closed
gsagula wants to merge 21 commits intoenvoyproxy:masterfrom
datawire:http-auth-tracing
Closed

extension: enable tracing for the external authorization HTTP client implementation.#7267
gsagula wants to merge 21 commits intoenvoyproxy:masterfrom
datawire:http-auth-tracing

Conversation

@gsagula
Copy link
Copy Markdown
Member

@gsagula gsagula commented Jun 13, 2019 •
edited by dio
Loading

Description:
This PR adds tracing support to the HTTP client in the external authorization filter. So far, only gRPC client was able to trace requests from the filter to an authorization service.

Risk Level: Low
Testing: yes
Docs Changes: N/A
Release Notes: Added
Fixes #6520

Signed-off-by: Gabriel Linden Sagula gsagula@gmail.com

Gabriel added 9 commits June 11, 2019 22:30
added tracing support
c5e6f01 
Signed-off-by: Gabriel <gsagula@gmail.com>
added the tracing name string to the client config
3d09e35 
Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
fixed typo
f2bd0c8 
Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
added test for http client tracing ok
1e3cd85 
Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
more tests for http client tracing
612a5ab 
Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
fixed tests
6bbab73 
Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
added tests for http client tracing
5b82074 
Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
use fmt instead of string concatenation
7d9500a 
Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
merged upstream and fixed conflict
afb4a36 
Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
Gabriel Linden Sagula added 5 commits June 17, 2019 09:24
Merge branch 'master' of github.com:envoyproxy/envoy into http-auth-t…
885fbe0 
…racing

Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
injected resource time
259362e 
Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
Merge branch 'master' of github.com:envoyproxy/envoy into http-auth-t…
7d73546 
…racing

Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
fixed test
e5bfc7f 
Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
Merge remote-tracking branch 'upstream/master' into http-auth-tracing
b6ea882 
Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
@dio
Copy link
Copy Markdown
Member

dio commented Jun 24, 2019

@gsagula please let me know when this is ready/need hands for this 🙂

@stale
Copy link
Copy Markdown

stale bot commented Jul 1, 2019

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 1, 2019
@gsagula
Copy link
Copy Markdown
Member Author

gsagula commented Jul 2, 2019

Looking into this one.

@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 2, 2019
Gabriel Linden Sagula added 3 commits July 3, 2019 10:26
Merge remote-tracking branch 'upstream/master' into http-auth-tracing
9881be9 
Gabriel Linden Sagula <gsagula@gmail.com>
Merge remote-tracking branch 'upstream/master' into http-auth-tracing
a3ae616 
Gabriel Linden Sagula <gsagula@gmail.com>
fixed memory leak
4483242 
Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
@gsagula
Copy link
Copy Markdown
Member Author

gsagula commented Jul 6, 2019

I'm on the road now. I fix the config test later.

fixed config test
137fa48 
Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
@gsagula
Copy link
Copy Markdown
Member Author

gsagula commented Jul 8, 2019

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Jul 8, 2019

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #7267 (comment) was created by @gsagula.

see: more, trace.

@stale
Copy link
Copy Markdown

stale bot commented Jul 15, 2019

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 15, 2019
@stale
Copy link
Copy Markdown

stale bot commented Jul 22, 2019

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot closed this Jul 22, 2019
@dio dio reopened this Jul 24, 2019
@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 24, 2019
@dio
Copy link
Copy Markdown
Member

dio commented Jul 24, 2019

@gsagula could you resolve the conflict?

Gabriel Linden Sagula added 3 commits July 31, 2019 10:04
fixed conflict
36c841d 
Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
fixed format
eefdb94 
Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
fixed clang tidy
ed19859 
Signed-off-by: Gabriel Linden Sagula <gsagula@gmail.com>
@gsagula
Copy link
Copy Markdown
Member Author

gsagula commented Aug 1, 2019

@dio Done.

@lizan
Copy link
Copy Markdown
Member

lizan commented Aug 5, 2019

can you merge master again to pick up latest CI changes?

dio
dio reviewed Aug 6, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@dio dio left a comment

Choose a reason for hiding this comment

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

Looks good. But should we improve/enrich the trace metadata for ext_authz?

image

image

But for e.g. the upstream gives you 503,

image

Should we also add e.g. request_size, response_code from ext_authz's upstream cluster?

source/extensions/filters/common/ext_authz/ext_authz.h
/**
* Tracing statuses.
*/
struct ConstantValues {
Copy link
Copy Markdown
Member

@dio dio Aug 6, 2019

Choose a reason for hiding this comment

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

Should we rename this to express more about "trace"?

@stale
Copy link
Copy Markdown

stale bot commented Aug 13, 2019

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 13, 2019
@gsagula
Copy link
Copy Markdown
Member Author

gsagula commented Aug 13, 2019

@dio Thanks for looking into it. I addressed the issues but I will need to resubmit this PR since the Datawire repo is now private.

@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Aug 13, 2019
@stale
Copy link
Copy Markdown

stale bot commented Aug 20, 2019

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 20, 2019
@dio
Copy link
Copy Markdown
Member

dio commented Aug 21, 2019

@gsagula please let me know which path do you want to take on progressing this PR. Thanks!

@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Aug 21, 2019
@stale
Copy link
Copy Markdown

stale bot commented Aug 28, 2019

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 28, 2019
@gsagula
Copy link
Copy Markdown
Member Author

gsagula commented Aug 29, 2019 •
edited by dio
Loading

@gsagula please let me know which path do you want to take on progressing this PR. Thanks!

I addressed the comments above. I will open a new PR for this change.

@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Aug 29, 2019
@gsagula gsagula mentioned this pull request Sep 4, 2019
Merged
@dio
Copy link
Copy Markdown
Member

dio commented Sep 4, 2019

@gsagula should we close this one? In favor of #8142.

@dio dio closed this Sep 4, 2019
@gsagula
Copy link
Copy Markdown
Member Author

gsagula commented Sep 4, 2019 •
edited
Loading

@dio Yes, thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@dio dio dio left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@dio dio

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

B3 HTTP headers not propagated to ext_authz server in 1.10.0

3 participants

@gsagula @dio @lizan