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

Fix X-Forwarded-* headers in openvsx-proxy #12071

Merged
merged 1 commit into from
Aug 12, 2022
Merged

Fix X-Forwarded-* headers in openvsx-proxy #12071

merged 1 commit into from
Aug 12, 2022
+82 −135

Conversation

jeanp413
Copy link
Member

Description

Fix X-Forwarded-* headers in openvsx-proxy, openvsx will read these values to properly create links to extension resources

How to test

  1. Open workspace
  2. Do some queries in vscode extension view and check response in dev tools links should point to openvsx-proxy
  3. Check extensions install sucessfully

Release Notes

NONE

Werft options:

  • /werft with-preview

@jeanp413 jeanp413 requested a review from a team August 11, 2022 15:40
@jeanp413
Copy link
Member Author

jeanp413 commented Aug 11, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-jp-openvsx-remove-rewrite.2
(with .werft/ from main)

@github-actions github-actions bot added team: IDE and removed size/L labels Aug 11, 2022
@jeanp413 jeanp413 requested a review from akosyakov August 11, 2022 16:14
andreafalzetti
andreafalzetti approved these changes Aug 11, 2022
View reviewed changes
Copy link
Contributor

@andreafalzetti andreafalzetti left a comment

Choose a reason for hiding this comment

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

Tested and works as advertised.

/hold in case of anyone with more context on this has major comments

@akosyakov
Copy link
Member

akosyakov commented Aug 12, 2022 •
edited
Loading

@jeanp413 I noticed that now if i navigate to Marketplace link it takes out to our proxy, not open-vsx.org as on main:
Screenshot 2022-08-12 at 07 58 19

The link on main as well points to proxy but it resolves to upstrea:
Screenshot 2022-08-12 at 07 59 33

Did we change anything that could break it? We should fix up the host header to upstream, no?

akosyakov
akosyakov reviewed Aug 12, 2022
View reviewed changes
components/openvsx-proxy/pkg/handler.go
@@ -49,7 +52,6 @@ func (o *OpenVSXProxy) Handler(p *httputil.ReverseProxy) func(http.ResponseWrite
key, err := o.key(r)
if err != nil {
log.WithFields(logFields).WithError(err).Error("cannot create cache key")
r.Host = o.upstreamURL.Host
Copy link
Member

Choose a reason for hiding this comment

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

it seems to break: #12071 (comment)

Do we really need to change it? OpenVSX should care about X-Forwarded* to create links or it also looks at Host header for it?

Copy link
Member

@akosyakov akosyakov Aug 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It is interesting though in context of Self Hosted in air gapped scenario where maybe a user machine does not have access to open-vsx.org directly but only via our proxy. In this case it would be even desirable to navigate user to our proxy for access of website. Not sure. cc @corneliusludmann @mrsimonemms

akosyakov
akosyakov reviewed Aug 12, 2022
View reviewed changes
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

great work, but we need to agree on #12071 (comment) before landing

also cleaning up commit history :)

@jeanp413
Copy link
Member Author

@jeanp413 I noticed that now if i navigate to Marketplace link it takes out to our proxy, not open-vsx.org as on main:

@akosyakov That's expected behavior, as the openvsx api will use the x-forwarded-* headers, I can add a hardcoded check in the reverse-proxy that if the endpoint is /vscode/item then delete the x-forwarded-* headers so openvsx use the prod url in this case open-vsx.org

@akosyakov
Copy link
Member

akosyakov commented Aug 12, 2022 •
edited by werft-gitpod-dev-com bot
Loading

@akosyakov That's expected behavior, as the openvsx api will use the x-forwarded-* headers, I can add a hardcoded check in the reverse-proxy that if the endpoint is /vscode/item then delete the x-forwarded-* headers so openvsx use the prod url in this case open-vsx.org

actually let's go with it, I think it is minor, and can be rather beneficial for self hosted installation.

Feel free to clean up commits and unhold 🙏

@jeanp413 jeanp413 force-pushed the jp/openvsx-remove-rewrite branch from d1b9988 to 3c4ef8d Compare August 12, 2022 15:21
@jeanp413
Copy link
Member Author

/unhold

@roboquat roboquat merged commit 1fe7486 into main Aug 12, 2022
@roboquat roboquat deleted the jp/openvsx-remove-rewrite branch August 12, 2022 15:33
@roboquat roboquat added deployed: IDE IDE change is running in production deployed Change is completely running in production labels Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akosyakov akosyakov akosyakov left review comments

@andreafalzetti andreafalzetti andreafalzetti approved these changes

@felladrin felladrin felladrin approved these changes

Assignees
No one assigned
Labels
deployed: IDE IDE change is running in production deployed Change is completely running in production release-note-none size/L team: IDE
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jeanp413 @akosyakov @felladrin @andreafalzetti @roboquat