-
Notifications
You must be signed in to change notification settings - Fork 14
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
bug: gateway-conformance subdomains are inflexible and setup is not clear #185
Comments
I also get the following error quite frequently:
but this is very odd because the request works perfectly fine in a browser: |
Running the test command with ENV vars instead seems to work...?
|
but not when run via docker with
|
Looks like a bug, we haven't tested gateways behind subdomains with ports. We'd need to update the subdomain tests to keep the port as well (cc @lidel). Example of a test that loses the port. The localhost tests might require a light refactor to force the port somehow (code)
The Could you double-check your
So the What do you mean by disabling the proxy entirely? My recommendation would be to NOT test the subdomain specs, maybe get started with the simpler ones. For example, use More examples in the docs |
good call:
and thanks for the rest of the links, super helpful. figuring out how to filter out tests right now is a lot of the trouble, but running tests from vscode is making determining those much easier |
I see how having two URLs options here becomes confusing:
Perhaps an useful insight is that subdomain gateway is not related to request's URL. I took a stab at improving docs in #193 Here, it should be something like: @SgtPooki try following prior art used for testing kubo, such as gateway-conformance/.github/workflows/test-kubo-e2e.yml Lines 65 to 66 in 4760ba7
With this, all you need to do is to make sure your gateway implementation has subdomain gateway enabled on And I agree with @laurentsenta, will be easier to disable Digression on why we see "localhost without port in tests"Gateway feature could be enabled per origin (DNS name+port+protocol), but in real world it is easier to implement and reason about enabling this feature globally per DNS name. For example, in Kubo, subdomain gateway is implicitly enabled on This is also why we have tests that seem to ignore port in the Yes, this could be fixed, but it also is cosmetic, and only impacts |
@lidel lack of port support is making gateway-conformance support in verified-fetch a pita. "real world" usecase is developers testing their servers in CI, which will likely not be on implicit ports. |
@SgtPooki let's reopen and discuss it here instead of slack. It has been a while, could you describe what exactly gets painful in your setup what would be the preferred fix? I looked at https://github.com/ipfs/helia-verified-fetch/pull/85/files#diff-175730b97b315afceee1b708f9bd876588ef883662dc52e139a7961ac7804c56R100 |
this would fix my concerns for sure. and remove the need for the header munging with @lidel if you have a recommendation on where to look for fixing this i'd be happy to put out a PR.
.Hostname or similar. https://pkg.go.dev/net/url#URL.Hostname implies that .Host is supposed to return port number, and that Hostname removes it.. but navigating go docs & code is not simple for me yet. If you could point me in the right direction I could take this on
|
diving in a bit more, |
it seems like this is the problem:
This seems like an implicit dependence on kubo's "accept all things on localhost" requirement, which will break in any gateway testing where that's not true/possible. Is the fix just removing |
Thanks for digging into this. I think we should fix the conformance tests to run test against explicit endpoint, and yes, localhost tests should not be hardcoded. Your pointers are very useful, I'll see if I can open a PR later today/tomorrow. |
Spent some time spelunking and thinking how to clean this up. Just like you noted, the underlying problem is not really Kubo specific, but the way gateway-conformance has hardcoded subdomain tests to always run against both It was likely a quick&dirty way to ensure we keep testing localhost ipfs-desktop/companion and brave use cases, but now it should be explicit rather implicit. I have local PR that cleans this up, but I've noticed additional problems, so will take a bit longer than expected to finish all loose ends. For now I will be tracking progress here, and open PR once ready. We will likely have to release it as Cleanup progress
|
I'm trying to improve on conformance of helia-http-gateway, and I am continuously getting failures from the gateway-conformance tool (via docker and directly with
go run ./cmd/gateway-conformance/main.go test # ...
)I believe these may be related to whatever proxying thing is happening under the hood, because i've seen this error more than a few times:
Some questions:
--gateway-url 'http://localhost:8090' --subdomain-url 'http://localhost:8090'
, why is the gateway-conformance tool queryinglocalhost
with no port?The text was updated successfully, but these errors were encountered: