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

Rewrite Host Header by default in ReverseProxy #580

Merged
merged 1 commit into from
Jan 11, 2025

Conversation

rahulbabu95
Copy link
Contributor

Description

The default implementation of ReverseProxy provided by NewSingleHostReverseProxy does not rewrite the host header of the outbound request. Instantiate ReverseProxy with an implementation that rewrites the host header of the outbound request by default.

Why is this needed

When the host header is not being rewritten, it would still have the host value that the incoming request had which in this case would be the smee host IP itself. If the upstream URL is hosted on a public service such as S3 which does some validations on the request header, the current implementation would fail as S3 would deny any such request where it's not able to validate the host header.

Fixes: #
Tested this change with isoURL in the smee deployment set to an S3 link and was able to boot up a Hardware properly.

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

Copy link

codecov bot commented Jan 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56%. Comparing base (5f59d3e) to head (d369677).
Report is 2 commits behind head on main.

Additional details and impacted files
@@         Coverage Diff         @@
##           main   #580   +/-   ##
===================================
- Coverage    56%    56%   -1%     
===================================
  Files        32     32           
  Lines      3621   3624    +3     
===================================
+ Hits       2052   2053    +1     
+ Misses     1499   1497    -2     
- Partials     70     74    +4     

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

@jacobweinstock jacobweinstock added the ready-to-merge Signal to Mergify to merge the PR. label Jan 11, 2025
@mergify mergify bot merged commit 15033c0 into tinkerbell:main Jan 11, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants