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

fix: /webtransport breaks status page #2057

Merged

Conversation

SgtPooki
Copy link
Member

@SgtPooki SgtPooki commented Oct 26, 2022

Closes #2033

@SgtPooki SgtPooki requested a review from a team as a code owner October 26, 2022 11:26
@SgtPooki SgtPooki linked an issue Oct 26, 2022 that may be closed by this pull request
@SgtPooki SgtPooki temporarily deployed to Deploy October 26, 2022 11:34 Inactive
@whizzzkid
Copy link
Contributor

Next Action: Isolated test case that proves this fixes the issue.

@whizzzkid whizzzkid marked this pull request as draft October 31, 2022 17:00
@SgtPooki
Copy link
Member Author

SgtPooki commented Nov 2, 2022

Terminal 1

start the daemon that has webtransport enabled

Initializing daemon...
Kubo version: 0.16.0-rc1-d4ac65f43
Repo version: 12
System version: arm64/darwin
Golang version: go1.18.4
Swarm listening on /ip4/127.0.0.1/tcp/4001
Swarm listening on /ip4/127.0.0.1/udp/4001/quic
Swarm listening on /ip4/127.0.0.1/udp/4002/quic/webtransport/certhash/uEiByODVpMr51jsCufZWnEgyO-hlbedDFwsyLDncNa6Am0g/certhash/uEiDDllXXQMCn1nMuRGfqdyKXrBPeP3g4HJUL1di65zfVQg
Swarm listening on /ip4/192.168.5.237/tcp/4001
Swarm listening on /ip4/192.168.5.237/udp/4001/quic
Swarm listening on /ip4/192.168.5.237/udp/4002/quic/webtransport/certhash/uEiByODVpMr51jsCufZWnEgyO-hlbedDFwsyLDncNa6Am0g/certhash/uEiDDllXXQMCn1nMuRGfqdyKXrBPeP3g4HJUL1di65zfVQg
Swarm listening on /ip6/::1/tcp/4001
Swarm listening on /ip6/::1/udp/4001/quic
Swarm listening on /p2p-circuit
Swarm announcing /ip4/127.0.0.1/tcp/4001
Swarm announcing /ip4/127.0.0.1/udp/4001/quic
Swarm announcing /ip4/127.0.0.1/udp/4002/quic/webtransport/certhash/uEiByODVpMr51jsCufZWnEgyO-hlbedDFwsyLDncNa6Am0g/certhash/uEiDDllXXQMCn1nMuRGfqdyKXrBPeP3g4HJUL1di65zfVQg
Swarm announcing /ip4/192.168.5.237/tcp/4001
Swarm announcing /ip4/192.168.5.237/udp/4001/quic
Swarm announcing /ip4/192.168.5.237/udp/4002/quic/webtransport/certhash/uEiByODVpMr51jsCufZWnEgyO-hlbedDFwsyLDncNa6Am0g/certhash/uEiDDllXXQMCn1nMuRGfqdyKXrBPeP3g4HJUL1di65zfVQg
Swarm announcing /ip6/::1/tcp/4001
Swarm announcing /ip6/::1/udp/4001/quic
API server listening on /ip4/127.0.0.1/tcp/5002
WebUI: http://127.0.0.1:5002/webui
Gateway (readonly) server listening on /ip4/127.0.0.1/tcp/8081
Daemon is ready

You can see that webtransport is enabled via the Swarm announcing /ip4/127.0.0.1/udp/4002/quic/webtransport/certhash/uEiByODVpMr51jsCufZWnEgyO-hlbedDFwsyLDncNa6Am0g/certhash/uEiDDllXXQMCn1nMuRGfqdyKXrBPeP3g4HJUL1di65zfVQg line

Terminal 2

Run the tests. See 78d1634

╰─ ✔ ❯ KUBO_PORT_2033_TEST=5002 npm run test:unit -- --runTestsByPath "test/kubo-webtransport.test.js"          21.87   25G   24%   71%  ─╯

> ipfs-webui@2.19.0 test:unit
> react-app-rewired test --env=jsdom --runInBand --watchAll=false "--runTestsByPath" "test/kubo-webtransport.test.js"

 PASS  test/kubo-webtransport.test.js
  Kubo webtransport fix test
    ✓ should get the id (160 ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        0.692 s, estimated 1 s
Ran all test suites within paths "test/kubo-webtransport.test.js"

Now remove the patch that fixes this and show that it's broken

See 8b3e1f8

╰─ ✔ ❯ KUBO_PORT_2033_TEST=5002 npm run test:unit -- --runTestsByPath "test/kubo-webtransport.test.js"        29.57   24.4G   24%   82%  ─╯

> ipfs-webui@2.19.0 test:unit
> react-app-rewired test --env=jsdom --runInBand --watchAll=false "--runTestsByPath" "test/kubo-webtransport.test.js"

watchman warning:  Recrawled this watch 59 times, most recently because:
MustScanSubDirs UserDroppedTo resolve, please review the information on
https://facebook.github.io/watchman/docs/troubleshooting.html#recrawl
To clear this warning, run:
`watchman watch-del '/Users/sgtpooki/code/work/protocol.ai/ipfs/webui' ; watchman watch-project '/Users/sgtpooki/code/work/protocol.ai/ipfs/webui'`

 FAIL  test/kubo-webtransport.test.js
  Kubo webtransport fix test
    ✕ should get the id (515 ms)

  ● Kubo webtransport fix test › should get the id

    no protocol with name: webtransport

      21 |     const ipfs = ipfsHttpModule(`http://localhost:${KUBO_PORT}`)
      22 |
    > 23 |     expect(async () => await ipfs.id()).not.toThrow()
         |                        ^
      24 |     expect((await ipfs.id()).id).toEqual(expect.any(String))
      25 |     // expect the id to be equal to a string now
      26 |   })

      at Protocols (node_modules/multiaddr/src/protocols-table.js:15:11)
      at stringToStringTuples (node_modules/multiaddr/src/codec.js:45:19)
      at stringToBytes (node_modules/multiaddr/src/codec.js:182:13)
      at Object.fromString (node_modules/multiaddr/src/codec.js:190:10)
      at ClassIsWrapper.Object.<anonymous>.withIs.proto.className (node_modules/multiaddr/src/index.js:46:24)
      at new Multiaddr (node_modules/class-is/index.js:40:33)
      at Object.<anonymous>.withIs.proto.className (node_modules/multiaddr/src/index.js:29:12)
      at Multiaddr (node_modules/class-is/index.js:40:33)
      at node_modules/ipfs-http-client/src/id.js:24:53
          at Array.map (<anonymous>)
      at Object.id (node_modules/ipfs-http-client/src/id.js:24:43)
      at test/kubo-webtransport.test.js:23:24

  ● Kubo webtransport fix test › should get the id

    no protocol with name: webtransport

      22 |
      23 |     expect(async () => await ipfs.id()).not.toThrow()
    > 24 |     expect((await ipfs.id()).id).toEqual(expect.any(String))
         |             ^
      25 |     // expect the id to be equal to a string now
      26 |   })
      27 | })

      at Protocols (node_modules/multiaddr/src/protocols-table.js:15:11)
      at stringToStringTuples (node_modules/multiaddr/src/codec.js:45:19)
      at stringToBytes (node_modules/multiaddr/src/codec.js:182:13)
      at Object.fromString (node_modules/multiaddr/src/codec.js:190:10)
      at ClassIsWrapper.Object.<anonymous>.withIs.proto.className (node_modules/multiaddr/src/index.js:46:24)
      at new Multiaddr (node_modules/class-is/index.js:40:33)
      at Object.<anonymous>.withIs.proto.className (node_modules/multiaddr/src/index.js:29:12)
      at Multiaddr (node_modules/class-is/index.js:40:33)
      at node_modules/ipfs-http-client/src/id.js:24:53
          at Array.map (<anonymous>)
      at Object.id (node_modules/ipfs-http-client/src/id.js:24:43)
      at Object.<anonymous> (test/kubo-webtransport.test.js:24:13)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   0 total
Time:        1.255 s
Ran all test suites within paths "test/kubo-webtransport.test.js".

@SgtPooki SgtPooki self-assigned this Nov 2, 2022
@SgtPooki SgtPooki temporarily deployed to Deploy November 2, 2022 11:54 Inactive
@SgtPooki
Copy link
Member Author

SgtPooki commented Nov 2, 2022

the testcase now allows for specifying a port to directly target an already running kubo node (to prove it fixes #2033) while also allowing for testing against the current go-ipfs version in package.json.. this will allow it to prove, when we update the go-ipfs package to kubo 17 (webtransport enabled by default) that it continues to work.

@SgtPooki SgtPooki temporarily deployed to Deploy November 2, 2022 12:36 Inactive
@SgtPooki SgtPooki marked this pull request as ready for review November 2, 2022 12:41
@SgtPooki SgtPooki temporarily deployed to Deploy November 2, 2022 12:44 Inactive
@SgtPooki
Copy link
Member Author

SgtPooki commented Nov 2, 2022

@lidel @Jorropo I believe the issue should be fixed now.

I checked with @achingbrain if I could patch the multiaddr versions that webui is using (see #2033 (comment)) and he's adamant against that, so we do need to merge this, but there is a potential for fallout with pinning multiaddr@8.1.2 in package.json overrides. What is that fallout? I'm not sure offhand, but the latest version in our dep tree was 10.0.1 so any changes in multiformats/js-multiaddr@v8.1.2...v10.0.1.

cc @tinytb

@SgtPooki
Copy link
Member Author

SgtPooki commented Nov 2, 2022

FWIW, I don't see anything majorly concerning in multiformats/js-multiaddr@v8.1.2...v10.0.1

Copy link
Contributor

@whizzzkid whizzzkid left a comment

Choose a reason for hiding this comment

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

nits

src/bundles/identity.test.js Show resolved Hide resolved
Copy link
Contributor

@whizzzkid whizzzkid left a comment

Choose a reason for hiding this comment

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

LGTM, thanks, some non-blocking comments to ponder upon.

@lidel lidel changed the title fix: 2033 enabling webtransport in kubo v0160 rc1 breaks status page fix: /webtransport breaks status page Nov 7, 2022
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Thanks, works as expected on my end. 👍

We will have /webtransport enabled by default soon and will remove patch eventually as part of #1965, so quick patch here is acceptable workaround.

@lidel lidel merged commit ea89e7f into main Nov 7, 2022
@lidel lidel deleted the 2033-enabling-webtransport-in-kubo-v0160-rc1-breaks-status-page branch November 7, 2022 18:46
ipfs-gui-bot pushed a commit that referenced this pull request Nov 9, 2022
## [2.20.0](v2.19.0...v2.20.0) (2022-11-09)

 CID `bafybeibjbq3tmmy7wuihhhwvbladjsd3gx3kfjepxzkq6wylik6wc3whzy`

 ---

### Features

* add success notification on "set pinning" action ([#2038](#2038)) ([e410164](e410164))
* track remote pins in progress ([#1919](#1919)) ([d3a6524](d3a6524))
* update to ipfs-geoip v9 ([#2061](#2061)) ([546fb5a](546fb5a))

### Bug Fixes

*  /webtransport breaks status page ([#2057](#2057)) ([ea89e7f](ea89e7f)), closes [#2033](#2033)

### Trivial Changes

* pull new translations ([#2049](#2049)) ([f6062b2](f6062b2))
* pull new translations ([#2056](#2056)) ([e13ff17](e13ff17))
* pull new translations ([#2059](#2059)) ([0bb5bf3](0bb5bf3))
@ipfs-gui-bot
Copy link
Collaborator

🎉 This PR is included in version 2.20.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enabling WebTransport in Kubo v0.16.0-rc1 breaks Status page
5 participants