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.

Sign up for GitHub

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

Jump to bottom

jwks: Add UA string to headers #35977

Merged
merged 2 commits into from
Sep 11, 2024
Merged

jwks: Add UA string to headers #35977

merged 2 commits into from
Sep 11, 2024

Conversation

Athishpranav2003
Copy link
Contributor

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 RepoKitteh - Read Only
Copy link

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 RepoKitteh - Read Only
Copy link

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
Copy link
Contributor Author

@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

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

KBaichoo commented Sep 6, 2024

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 Athishpranav2003 marked this pull request as draft September 8, 2024 08:33
@Athishpranav2003
Copy link
Contributor Author

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

@KBaichoo
Copy link
Contributor

KBaichoo commented Sep 9, 2024

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 KBaichoo marked this pull request as ready for review September 9, 2024 15:12
@KBaichoo
Copy link
Contributor

KBaichoo commented Sep 9, 2024

/wait

The CI failures are legitimate

@tyxia
Copy link
Member

tyxia commented Sep 9, 2024 •
edited
Loading

@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
Copy link
Contributor Author

Athishpranav2003 commented Sep 9, 2024 •
edited
Loading

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

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

Athishpranav2003 commented Sep 9, 2024 •
edited
Loading

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

tyxia
tyxia reviewed Sep 9, 2024
View reviewed changes
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 your contribution!

source/extensions/filters/http/common/jwks_fetcher.cc Outdated Show resolved Hide resolved
@tyxia
Copy link
Member

tyxia commented Sep 9, 2024

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

@Athishpranav2003
Copy link
Contributor Author

@tyxia resolved the comments and also CI tests are passing

@Athishpranav2003
Added UA string for JWKS fetcher
8f5e379 
Signed-off-by: Athish Pranav D <athishanna@gmail.com>
@Athishpranav2003
Added UTs
b1d63b3 
Signed-off-by: Athish Pranav D <athishanna@gmail.com>
tyxia
tyxia approved these changes Sep 10, 2024
View reviewed changes
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!

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

@yerkulees
Copy link

yerkulees commented Sep 10, 2024 via email

@yerkulees
Copy link

yerkulees commented Sep 11, 2024 via email

@tyxia tyxia merged commit 1ef5996 into envoyproxy:main Sep 11, 2024
37 checks passed
unicell added a commit to unicell/envoy that referenced this pull request Sep 11, 2024
@unicell
Merge remote-tracking branch 'upstream/main' into oid-lua-api
42a35a0 
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tyxia tyxia tyxia approved these changes

@KBaichoo KBaichoo Awaiting requested review from KBaichoo

Assignees

@tyxia tyxia

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add User-Agent to jwks http requests
4 participants
@Athishpranav2003 @KBaichoo @tyxia @yerkulees