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

Preserve hostname specified with --api in http request headers #10233

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

softwareplumber
Copy link

@softwareplumber softwareplumber commented Nov 25, 2023

System resolves hostname to ip address which is no use for a reverse proxy (or kubernetes ingress) which may want to route the request to a service using the hostname.

Update still checks api addr is resolvable, but doesn't resolve it, thus enabling the cli to use a remote api behind a reverse proxy.

Closes #10232

@softwareplumber softwareplumber requested a review from a team as a code owner November 25, 2023 16:49
Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

I'm surprised this works, I would guess that the problem is how we intercept the HTTP request to send it to manet.

If this works and does not have any unintended consequences you can remove the resolveAddr call completely.

@Jorropo
Copy link
Contributor

Jorropo commented Nov 25, 2023

Thx, could you please add a test ? (avoid regressions)

@softwareplumber
Copy link
Author

softwareplumber commented Nov 25, 2023 via email

@Jorropo
Copy link
Contributor

Jorropo commented Nov 25, 2023

Screenshot_2023-11-25-20-06-09-015_com.github.android.jpg

I'm busy right now, you can try to view the generated html report.

@softwareplumber
Copy link
Author

softwareplumber commented Nov 26, 2023 via email

@hacdias
Copy link
Member

hacdias commented Nov 27, 2023

@softwareplumber sorry for the problem here. We have some flaky tests that haven't been solved - a panic somewhere causing a lot of tests to fail. I restarted the tests and everything passes.

Regarding make test: without knowing what error you got, it is hard to help debugging what went wrong. Remember that the sharness tests specifically are designed to run on Linux, so they may not work on other platforms. We are moving away from this, but it takes time.

@softwareplumber
Copy link
Author

@hacdias many thanks for the help, FWIW I'm running locally under WSL2/Alma9 and I'm almost sure it is the fuse stuff that is causing problems. I will investigate further. @Jorropo if I add a test that shows connectivity between client and server with --api=/dns4/localhost/tcp/ will that be sufficient?

@Jorropo
Copy link
Contributor

Jorropo commented Nov 27, 2023

You want to add a test that would fail before but work now.
As far as I can tell --api=/dns4/localhost/tcp/ works before already because it's able to translate it to /ip4/127.0.0.1/tcp/.

Ideally the test I want to see is something using the new harness test suite:
https://github.com/ipfs/kubo/tree/master/test/cli

You would use net/http to setup an http server listening, then you try --api=/dns4/localhost/tcp/ and you check that in your server you indeed get Host: localhost.

I get this is a lot more involved but I can't promise you this will continue to work if we don't have a test, idk what other maintainers thinks here.

@softwareplumber
Copy link
Author

softwareplumber commented Nov 27, 2023 via email

@lidel lidel requested a review from Jorropo December 11, 2023 14:26
@lidel lidel requested a review from a team April 16, 2024 13:26
@lidel lidel added kind/test Testing work need/maintainer-input Needs input from the current maintainer(s) labels Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/test Testing work need/maintainer-input Needs input from the current maintainer(s)
Projects
Status: 🔎 In Review
4 participants