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

feat: make subdomain testing possible #9

Merged
merged 36 commits into from
Mar 23, 2023
Merged

feat: make subdomain testing possible #9

merged 36 commits into from
Mar 23, 2023

Conversation

laurentsenta
Copy link
Contributor

@laurentsenta laurentsenta commented Mar 7, 2023

This PR adds support for subdomain testing. We port the test from sharness 114 into the conformance test suite.

Tasks

  • Update the CI's ipfs daemon with gateway URLs (localhost and example.com should be enabled)
  • port missing test (50%)
  • resolve a few discussions tagged for lidel
  • implement TODOs
    • get the correct value from CID

@laurentsenta laurentsenta marked this pull request as draft March 7, 2023 14:12
@laurentsenta
Copy link
Contributor Author

laurentsenta commented Mar 7, 2023

cc @galargh this is the subdomain with custom resolver trick.
Behind a flag for now, it requires the user to configure their kubo instance with:

ipfs config --json Gateway.PublicGateways '{
  "example.com": {
    "UseSubdomains": true,
    "Paths": ["/ipfs", "/ipns", "/api"]
  }
}'

@laurentsenta laurentsenta changed the base branch from with-go-suite to main March 8, 2023 08:42
@laurentsenta laurentsenta mentioned this pull request Mar 8, 2023
29 tasks
@laurentsenta laurentsenta force-pushed the with-subdomain branch 2 times, most recently from 66ca0ba to 0818bfe Compare March 20, 2023 16:49
@laurentsenta laurentsenta requested a review from galargh March 21, 2023 16:07
@laurentsenta laurentsenta marked this pull request as ready for review March 21, 2023 16:07
Copy link
Contributor

@galargh galargh left a comment

Choose a reason for hiding this comment

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

Nicely done @laurentsenta 🏅 👏

I left some comments, but I don't want to block this. It might be worth agreeing on the param name before the merge, but even that isn't something that we can't tackle later.

Comment on lines +7 to +10
subdomain-url:
description: "The Subdomain URL of the IPFS Gateway implementation to be tested."
default: "http://example.com"
required: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we being clear enough with the name/description here? I'm afraid this creates an impression that I, as an implementer, should set up something accessible at http://example.com. I think I'd assume I have to set up another gateway instance that supports the subdomain gateway spec and expose it at subdomain-url.

Also, do we think we might have non-subdomain-related tests benefitting from the proxying in the future?

Wouldn't something like local-gateway-proxy-url or internal-gateway-proxy-url reflect the intention better?

I don't want to block this, but I wanted to raise my concerns with you.

Copy link
Contributor Author

@laurentsenta laurentsenta Mar 23, 2023

Choose a reason for hiding this comment

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

I'll add a comment in the README, there's no real proxying anymore, we use that value to overwrite the request sent to the gateway, it's really subdomain-related.

Copy link
Contributor

@galargh galargh Mar 24, 2023

Choose a reason for hiding this comment

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

I see. Thanks for adding the comment. It makes things clearer. Few more thoughts that it sparked with me.

Wouldn't it make sense to always use what we now call subdomain-url. As in, all the test should talk to gateway-url in a way that thinks it's called subdomain-url? Theoretically, wouldn't that be more correct? So really, every test case should be executed 3 times (with host header, proxy, proxy with tunneling) as long as subdomain-url and gateway-url are different.

Looking more closely at how we execute the test cases with host header, I think we're not using the protocol part of gateway-url. Is that correct?

If so, isn't what we really need from the user these two pieces of info:

  • What are you calling/want to call your gateway? Please give us the full URL with a domain name. That would be https://dweb.link for example or http://example.com. You could give us http://127.0.0.1:8080 here as well BUT there's no way for you to support sudomains on that URL so you'll have to disable subdomain-gateway spec.
  • Where is your gateway hosted? Please give us the host we should contact when we're making requests to the URL you provided in the previous step. If you don't, we're just going to use the host from the URL. It's fine if you give us a host with IP or a host with domain. This would be 127.0.0.1 for example or 127.0.0.1:8080 or foo.bar.example.com.

That would come together to something like this - main...gateway-host (note that I intentionally skipped modifying test/config code; I just wanted to get the idea across).

There's absolutely no rush with replying to this. And it's completely possible that I'm missing the point a bit - still trying to wrap my head fully around this. I just want to understand what might be most straight-forward way for those running the tests to provide us all the information we need.

.github/workflows/test.yml Outdated Show resolved Hide resolved
Makefile Outdated
@@ -8,6 +8,10 @@ test-cargateway: provision-cargateway
provision-kubo:
find ./fixtures -name '*.car' -exec ipfs dag import {} \;

test-kubo-subdomains: provision-kubo gateway-conformance
./ipfs-config.example.sh
./gateway-conformance test --json output.json --gateway-url http://127.0.0.1:8080 --subdomain-url http://example.com:8080 --specs +subdomain-gateway
Copy link
Contributor

Choose a reason for hiding this comment

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

We can skip the --specs arg.

Suggested change
./gateway-conformance test --json output.json --gateway-url http://127.0.0.1:8080 --subdomain-url http://example.com:8080 --specs +subdomain-gateway
./gateway-conformance test --json output.json --gateway-url http://127.0.0.1:8080 --subdomain-url http://example.com:8080

And I think this makes it obsolete since test-kubo would run those tests already anyway. Should we add the Kubo init to the test-kubo command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the makefile should be seen as a runnable quickstart / documentation.
I wouldn't mess with the users' ipfs config by default. I'll refactor the Makefile to make it more clear.

cmd/gateway-conformance/main.go Outdated Show resolved Hide resolved
ipfs-config.example.sh Outdated Show resolved Hide resolved
tooling/test/report.go Show resolved Hide resolved
tooling/test/report.go Show resolved Hide resolved
tooling/test/sugar.go Outdated Show resolved Hide resolved
tooling/test/sugar.go Outdated Show resolved Hide resolved
tooling/test/sugar.go Outdated Show resolved Hide resolved
@laurentsenta laurentsenta merged commit d38b8f9 into main Mar 23, 2023
@laurentsenta laurentsenta deleted the with-subdomain branch March 23, 2023 11:37
@laurentsenta laurentsenta mentioned this pull request Apr 7, 2023
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants