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

softwareplumber · 2023-11-25T16:49:27Z

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

Jorropo

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 · 2023-11-25T17:00:57Z

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

softwareplumber · 2023-11-25T18:53:19Z

Is there a testing HOWTO someplace? Make test fails for me on a clean checkout of master, even after I installed fuse and zsh. I confess one reason I raised the PR in the first place was that I hoped to see a clean test run in CI.

…

------ Original Message ------ From "Jorropo" ***@***.***> To "ipfs/kubo" ***@***.***> Cc "softwareplumber" ***@***.***>; "Author" ***@***.***> Date 11/25/2023 12:01:08 PM Subject Re: [ipfs/kubo] Preserve hostname specified with --api in http request headers (PR #10233)

Could you please add a test ? — Reply to this email directly, view it on GitHub <#10233 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGAPFY2GKL4OT6Y3MWZQY4DYGIP5JAVCNFSM6AAAAAA72GT5JCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRWGM3TONBRGA>. You are receiving this because you authored the thread.Message ID: ***@***.***>

Jorropo · 2023-11-25T19:07:03Z

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

softwareplumber · 2023-11-26T04:25:21Z

Yeah, struggling to make sense of that. Looking at the summary at the top of the html report, it would appear (for example) that package kubo/test/cli has no errors and one skipped test out of 791. If I navigate down through the package link in the report to the 'TestGateway' test in this package, it shows a bunch of errors. Are these errors expected? If I run 'go test' in test/cli it appears to run the relevant tests without a problem. I try to duplicate the failing github workflow locally (setting some environment and then running make test/unit/gotest.junit.xml) the resulting test/unit/gotest.json doesn't seem to have any failed actions. So I'm not sure where to go from here.

…

------ Original Message ------ From "Jorropo" ***@***.***> To "ipfs/kubo" ***@***.***> Cc "softwareplumber" ***@***.***>; "Author" ***@***.***> Date 11/25/2023 2:07:14 PM Subject Re: [ipfs/kubo] Preserve hostname specified with --api in http request headers (PR #10233)

Screenshot_2023-11-25-20-06-09-015_com.github.android.jpg (view on web) <https://github.com/ipfs/kubo/assets/24391983/7a8d7341-b623-437c-b0a0-89a8680c8b9c> I'm busy right now, you can try to view the generated html report. — Reply to this email directly, view it on GitHub <#10233 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGAPFY2562HRMQXH4WCJKVDYGI6WFAVCNFSM6AAAAAA72GT5JCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRWGQYDAMJSGY>. You are receiving this because you authored the thread.Message ID: ***@***.***>

hacdias · 2023-11-27T09:12:38Z

@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 · 2023-11-27T14:22:18Z

@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 · 2023-11-27T18:13:21Z

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 · 2023-11-27T19:01:41Z

:-) That's exactly what I was afraid you'd say. I was trying to dodge actually doing because of the effort involved in setting up a dummy server to inspect the headers, in a test framework I'm not particularly familiar with. It wouldn't happen quickly. Unless there is a similar test somewhere already that I can clone and modify?

…

------ Original Message ------ From "Jorropo" ***@***.***> To "ipfs/kubo" ***@***.***> Cc "softwareplumber" ***@***.***>; "Mention" ***@***.***> Date 11/27/2023 1:13:34 PM Subject Re: [ipfs/kubo] Preserve hostname specified with --api in http request headers (PR #10233)

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/localhost/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 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. — Reply to this email directly, view it on GitHub <#10233 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGAPFY6D7SEW4RMTNMOIOOLYGTJ45AVCNFSM6AAAAAA72GT5JCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRYGM3TAMBXGY>. You are receiving this because you were mentioned.Message ID: ***@***.***>

- Replaces PR #10233 - Add test to check for hostname in HTTP header

gammazero · 2024-08-28T16:17:16Z

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

It is necessary to keep the call to resolveAddr to check for a resolvable address, otherwise the host is reported as offline when the address should be reported as not resolvable.

gammazero · 2024-08-28T16:17:37Z

Replacing this PR with #10479 because the changed code has move to a different file.

…ers (#10497) Preserve hostname specified with --api in http request headers - Replaces PR #10233 - Add test to check for hostname in HTTP header - Update docs/changelogs/v0.30.md

…ers (#10497) Preserve hostname specified with --api in http request headers - Replaces PR #10233 - Add test to check for hostname in HTTP header - Update docs/changelogs/v0.30.md (cherry picked from commit 5fe9604)

check api addr is resolvable, but don't resolve it

e6d71ed

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

update changelog

b711721

Jorropo requested changes Nov 25, 2023

View reviewed changes

softwareplumber mentioned this pull request Nov 27, 2023

Don't convert the host name specified with --api=/dns4/<domain>/ into an IP address (thus making life easy for reverse proxy) #10232

Closed

3 tasks

lidel requested a review from Jorropo December 11, 2023 14:26

lidel requested a review from a team April 16, 2024 13:26

lidel added kind/test Testing work need/maintainer-input Needs input from the current maintainer(s) labels Apr 16, 2024

gammazero self-assigned this Aug 28, 2024

gammazero added a commit that referenced this pull request Aug 28, 2024

Preserve hostname specified with --api in http request headers

f826093

- Replaces PR #10233 - Add test to check for hostname in HTTP header

gammazero mentioned this pull request Aug 28, 2024

fix(cli): preserve hostname specified with --api in http request headers #10497

Merged

gammazero closed this Aug 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

softwareplumber commented Nov 25, 2023 •

edited by lidel

Loading

Jorropo left a comment

Jorropo commented Nov 25, 2023 •

edited

Loading

softwareplumber commented Nov 25, 2023 via email

Jorropo commented Nov 25, 2023

softwareplumber commented Nov 26, 2023 via email

hacdias commented Nov 27, 2023

softwareplumber commented Nov 27, 2023

Jorropo commented Nov 27, 2023 •

edited

Loading

softwareplumber commented Nov 27, 2023 via email

gammazero commented Aug 28, 2024

gammazero commented Aug 28, 2024

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

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

Conversation

softwareplumber commented Nov 25, 2023 • edited by lidel Loading

Jorropo left a comment

Choose a reason for hiding this comment

Jorropo commented Nov 25, 2023 • edited Loading

softwareplumber commented Nov 25, 2023 via email

Jorropo commented Nov 25, 2023

softwareplumber commented Nov 26, 2023 via email

hacdias commented Nov 27, 2023

softwareplumber commented Nov 27, 2023

Jorropo commented Nov 27, 2023 • edited Loading

softwareplumber commented Nov 27, 2023 via email

gammazero commented Aug 28, 2024

gammazero commented Aug 28, 2024

softwareplumber commented Nov 25, 2023 •

edited by lidel

Loading

Jorropo commented Nov 25, 2023 •

edited

Loading

Jorropo commented Nov 27, 2023 •

edited

Loading