jwks: Add UA string to headers #35977

Athishpranav2003 · 2024-09-05T04:59:25Z

Commit Message: Add the "Go-browser" UA string to the headers to make request for JWKS Fetcher
Additional Description: Since none of the requests are having a UA string it makes sense to add it as part of utility lib itself
Risk Level: N/A
Testing: N/A
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
Fixes #35785

repokitteh-read-only · 2024-09-05T04:59:30Z

Hi @Athishpranav2003, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #35977 was opened by Athishpranav2003.

see: more, trace.

repokitteh-read-only · 2024-09-05T04:59:30Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #35977 was opened by Athishpranav2003.

see: more, trace.

Athishpranav2003 · 2024-09-05T05:08:06Z

@KBaichoo @tyxia could you guys please check this. I am not sure how i can test it.
As far as i have read the code it seems fine(went through different libs defined inside).

I will go through the existing JWKS tests but meanwhile want some comment on this

source/common/http/utility.cc

KBaichoo · 2024-09-06T14:39:33Z

Hi @Athishpranav2003,

You can add tests in test/extensions/filters/http/common/jwks_fetcher_test.cc

/assign @tyxia

as codeowner.

/wait

for tests, addressing pending comments

Athishpranav2003 · 2024-09-09T12:20:18Z

@tyxia I would need help in setting up the testing env

KBaichoo · 2024-09-09T15:12:54Z

Hey @Athishpranav2003

In order for the reviewers to be reminded about a PR it needs to be in "ready for review mode" otherwise reviewers won't get pinged.

KBaichoo · 2024-09-09T15:13:58Z

/wait

The CI failures are legitimate

tyxia · 2024-09-09T15:20:10Z

@tyxia I would need help in setting up the testing env

@Athishpranav2003 Could you clarify what specific assistance you need? Have you checked this https://github.com/envoyproxy/envoy/blob/1da76eb9a1695e35b7097481624e7ffc3e9aeab8/bazel/README.md suggested by Kevin?

btw, for format error you can try command "bazel run //tools/code_format:check_format -- fix"

Athishpranav2003 · 2024-09-09T16:54:01Z

@tyxia I guess i am able to use the docker scripts for format and testing, so its fine only(even tho its slow)

Athishpranav2003 · 2024-09-09T19:25:04Z

@tyxia can you check the PR now?
i guess i figured the UTs now.
And also checks passed

tyxia

Thanks for your contribution!

source/extensions/filters/http/common/jwks_fetcher.cc

tyxia · 2024-09-09T23:47:24Z

Also add @yerkulees (original reporter of #35785) for review to see if it meets the need.

Athishpranav2003 · 2024-09-10T05:54:07Z

@tyxia resolved the comments and also CI tests are passing

Signed-off-by: Athish Pranav D <athishanna@gmail.com>

tyxia

LGTM, Thanks!

I would like @yerkulees (original reporter of #35785) to take a look before merging the PR

yerkulees · 2024-09-10T14:19:52Z

Thank you, I’ll be able to look at it this morning.

…

On Tue, Sep 10, 2024 at 7:15 AM Tianyu ***@***.***> wrote: ***@***.**** approved this pull request. LGTM, Thanks! I would like @yerkulees <https://github.com/yerkulees> (original reporter of #35785 <#35785>) to take a look before merging the PR — Reply to this email directly, view it on GitHub <#35977 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABYZ2BQEVHM4E7LKSOVKP33ZV35G7AVCNFSM6AAAAABNVUWFTGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEOJSGY3TAOJVGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

yerkulees · 2024-09-11T18:43:22Z

This works for my situation, thank you very much!

…

On Tue, Sep 10, 2024 at 7:19 AM Jordan Yerkes ***@***.***> wrote: Thank you, I’ll be able to look at it this morning. On Tue, Sep 10, 2024 at 7:15 AM Tianyu ***@***.***> wrote: > ***@***.**** approved this pull request. > > LGTM, Thanks! > > I would like @yerkulees <https://github.com/yerkulees> (original > reporter of #35785 <#35785>) > to take a look before merging the PR > > — > Reply to this email directly, view it on GitHub > <#35977 (review)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ABYZ2BQEVHM4E7LKSOVKP33ZV35G7AVCNFSM6AAAAABNVUWFTGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEOJSGY3TAOJVGM> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

-- Jordan K Yerkes San Diego, CA e. ***@***.***

* upstream/main: (21 commits) Add a CPU utilization resource monitor for overload manager (envoyproxy#34713) jwks: Add UA string to headers (envoyproxy#35977) exceptions: cleaning up macros (envoyproxy#35694) coverage: ratcheting (envoyproxy#36058) runtime: load rtds bool correctly as true/false instead of 1/0 (envoyproxy#36044) Typo in documentation of http original_src filter (envoyproxy#36060) docs: updating meeting info (envoyproxy#36052) quic: removes more references to spdy::Http2HeaderBlock. (envoyproxy#36057) json: add null support to the streamer (envoyproxy#36051) json: make the streamer a template class (envoyproxy#36001) docs: Add `apt.envoyproxy.io` install information (envoyproxy#36050) ext_proc: elide redundant copy in ext_proc filter factory callback (envoyproxy#36015) build(deps): bump yarl from 1.11.0 to 1.11.1 in /tools/base (envoyproxy#36049) build(deps): bump multidict from 6.0.5 to 6.1.0 in /tools/base (envoyproxy#36048) quic: enable certificate compression/decompression (envoyproxy#35999) Geoip fix asan failure (envoyproxy#36043) mobile: Fix missing logging output in Swift integration tests (envoyproxy#36040) http: minor code clean up to the http filter manager (envoyproxy#36027) ci/example: Dont build/test the filter example in Envoy CI (envoyproxy#36038) ci/codeql: Fix build setup (envoyproxy#36021) ... Signed-off-by: Qiu Yu <qiuyu@apple.com>

Athishpranav2003 force-pushed the add_ua_string branch from 04e6f1d to 342977c Compare September 5, 2024 05:17

KBaichoo marked this pull request as ready for review September 6, 2024 14:34

KBaichoo reviewed Sep 6, 2024

View reviewed changes

source/common/http/utility.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot assigned tyxia Sep 6, 2024

repokitteh-read-only bot added the waiting label Sep 6, 2024

Athishpranav2003 force-pushed the add_ua_string branch from 342977c to a491288 Compare September 8, 2024 06:07

repokitteh-read-only bot removed the waiting label Sep 8, 2024

Athishpranav2003 force-pushed the add_ua_string branch from a491288 to f4cbb87 Compare September 8, 2024 08:20

Athishpranav2003 marked this pull request as draft September 8, 2024 08:33

KBaichoo marked this pull request as ready for review September 9, 2024 15:12

repokitteh-read-only bot added the waiting label Sep 9, 2024

Athishpranav2003 force-pushed the add_ua_string branch from f4cbb87 to f0e941e Compare September 9, 2024 17:52

repokitteh-read-only bot removed the waiting label Sep 9, 2024

Athishpranav2003 force-pushed the add_ua_string branch 2 times, most recently from ebdfcc6 to de55178 Compare September 9, 2024 19:02

Athishpranav2003 requested a review from KBaichoo September 9, 2024 19:55

tyxia reviewed Sep 9, 2024

View reviewed changes

source/extensions/filters/http/common/jwks_fetcher.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Sep 9, 2024

Athishpranav2003 force-pushed the add_ua_string branch from de55178 to b1d63b3 Compare September 10, 2024 04:33

repokitteh-read-only bot removed the waiting label Sep 10, 2024

Athishpranav2003 requested a review from tyxia September 10, 2024 04:34

Athishpranav2003 added 2 commits September 10, 2024 15:22

Added UA string for JWKS fetcher

8f5e379

Signed-off-by: Athish Pranav D <athishanna@gmail.com>

Added UTs

b1d63b3

Signed-off-by: Athish Pranav D <athishanna@gmail.com>

tyxia approved these changes Sep 10, 2024

View reviewed changes

tyxia merged commit 1ef5996 into envoyproxy:main Sep 11, 2024
37 checks passed

jwks: Add UA string to headers #35977

jwks: Add UA string to headers #35977

Uh oh!

Conversation

Athishpranav2003 commented Sep 5, 2024

Uh oh!

repokitteh-read-only bot commented Sep 5, 2024

Uh oh!

repokitteh-read-only bot commented Sep 5, 2024

Uh oh!

Athishpranav2003 commented Sep 5, 2024

Uh oh!

Uh oh!

KBaichoo commented Sep 6, 2024

Uh oh!

Athishpranav2003 commented Sep 9, 2024

Uh oh!

KBaichoo commented Sep 9, 2024

Uh oh!

KBaichoo commented Sep 9, 2024

Uh oh!

tyxia commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Athishpranav2003 commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Athishpranav2003 commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tyxia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tyxia commented Sep 9, 2024

Uh oh!

Athishpranav2003 commented Sep 10, 2024

Uh oh!

tyxia left a comment

Choose a reason for hiding this comment

Uh oh!

yerkulees commented Sep 10, 2024 via email

Uh oh!

yerkulees commented Sep 11, 2024 via email

Uh oh!

Uh oh!

Uh oh!

tyxia commented Sep 9, 2024 •

edited

Loading

Athishpranav2003 commented Sep 9, 2024 •

edited

Loading

Athishpranav2003 commented Sep 9, 2024 •

edited

Loading