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

Attach authentication header to proxy agent #2382

Merged
merged 13 commits into from
Dec 17, 2024
Merged

Conversation

rudyflores
Copy link
Contributor

@rudyflores rudyflores commented Dec 10, 2024

What It Does

Change the current way the authorization header is attached for the proxy request to instead be attached in the agent itself to avoid 403 error.

How to Test

In order to test this, use a proxy available that requires authentication and follow these setup steps:

  1. In the proxy settings, define the url that requires authentication
  2. In the proxy authorization setting click Edit in settings.json and replace the null property with the authentication type followed by space and the credentials in base64. Example: Basic ==MyBase64UserPassword. Note the credentials should use this pattern before encoding user:password

Review Checklist
I certify that I have:

Additional Comments

Copy link

codecov bot commented Dec 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.28%. Comparing base (a0b45a0) to head (92aa324).
Report is 15 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2382      +/-   ##
==========================================
+ Coverage   91.26%   91.28%   +0.01%     
==========================================
  Files         638      638              
  Lines       18190    18207      +17     
  Branches     3923     3930       +7     
==========================================
+ Hits        16602    16620      +18     
+ Misses       1587     1586       -1     
  Partials        1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Rudy Flores <[email protected]>
Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

Hey @rudyflores,
The changes make sense to me 😋

Could you provide some steps to test it?
Also, could you update the changelog to capture this fix?

Copy link
Member

@traeok traeok left a comment

Choose a reason for hiding this comment

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

Changes LGTM, but looks like existing tests are failing in AbstractRestClient, so I'll approve once resolved. I also agree with Fernando's suggestion to have tests for this case - looks like they might just need moved and refactored. Thanks Rudy!

@rudyflores
Copy link
Contributor Author

@zFernand0 @traeok instructions for testing this is on the PR readme now, currently working on the unit tests!

@rudyflores rudyflores linked an issue Dec 16, 2024 that may be closed by this pull request
Signed-off-by: Rudy Flores <[email protected]>
@rudyflores
Copy link
Contributor Author

@traeok @zFernand0 FYI not sure why the CHANGELOG check is failing even after updating it...

Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

Changes makes sense to me 🙏

left a suggestion to help fix the changelog check

packages/imperative/CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Rudy Flores <[email protected]>
@rudyflores rudyflores requested a review from zFernand0 December 16, 2024 19:34
Copy link

sonarcloud bot commented Dec 16, 2024

Copy link
Member

@traeok traeok 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 Rudy!

Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

Overall, LGTM! 😋

I'm very curious about your prettier settings 😋
I don't mind the reformatting of the ProxySettings files, but I'm curious to know if people think we should enforce this sort of style going forward. 😋

@zFernand0 zFernand0 merged commit fff3f23 into master Dec 17, 2024
18 checks passed
@zFernand0 zFernand0 deleted the attach-proxy-options branch December 17, 2024 21:10
@zFernand0 zFernand0 added the release-minor Indicates a minor feature has been added label Dec 17, 2024
@zFernand0
Copy link
Member

labeled release-minor to combine the following PRs:
image

Copy link

Release succeeded for the master branch. 🎉

The following packages have been published:

Powered by Octorelease 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-minor Indicates a minor feature has been added released
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

Proxy updates missing auth headers in agent of request
3 participants