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

Enabling WebTransport in Kubo v0.16.0-rc1 breaks Status page #2033

Closed
Tracked by #1965 ...
lidel opened this issue Sep 27, 2022 · 13 comments · Fixed by #2057
Closed
Tracked by #1965 ...

Enabling WebTransport in Kubo v0.16.0-rc1 breaks Status page #2033

lidel opened this issue Sep 27, 2022 · 13 comments · Fixed by #2057
Assignees
Labels
kind/bug A bug in existing code (including security flaws) P0 Critical: Tackled by core team ASAP released topic/interop Interoperability

Comments

@lidel
Copy link
Member

lidel commented Sep 27, 2022

How to reproduce

  1. Run Kubo v0.16.0-rc1
  2. Enable WebTransport by
    1. adding "/ip4/0.0.0.0/udp/4002/quic/webtransport" to Addresses.Swarm
    2. enabling transport via ipfs config Swarm.Transports.Network.WebTransport --json true
  3. Restart node
  4. Open webui

See Release Notes for more info: https://github.com/ipfs/kubo/blob/master/docs/changelogs/v0.16.md

Broken state

2022-09-27_15-22

Suspected cause

Guess: ipfs.id output breaks the way we present ADDRESSES on the Status page.
I see ipfs-webui in infinite loop trying to read /api/v0/id over and over again.

New type of addresses is now returned as part of Addresses list, values look like this:

"/ip4/47.22.210.45/udp/55918/quic/webtransport/certhash/uEiAkH5a4DPGKUuOBjYw0CgwjvcJCJMD2K_1aluKR_tpevQ/certhash/uEiAfbgiymPP2_nX7Dgir8B4QkksjHp2lVuJZz0F79Be9JA/p2p/12D3KooWBdmLJjhpgJ9KZgLM3f894ff9xyBfPvPjFNn7MKJpyrC2"

cc ipfs/kubo#9237
@SgtPooki or @hacdias will you have time to take a look? Ideally we would fix this and ship new ipfs-webui and update Kubo 0.16 ships.

@lidel lidel added need/triage Needs initial labeling and prioritization kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up labels Sep 27, 2022
@SgtPooki SgtPooki self-assigned this Sep 27, 2022
@SgtPooki
Copy link
Member

I've got kubo 0.16 rc1 running locally against webui right now, working on repro
image

@SgtPooki
Copy link
Member

repro:
image

@SgtPooki
Copy link
Member

SgtPooki commented Sep 27, 2022

err:  Error: no protocol with name: webtransport
    at Protocols (protocols-table.js:15:1)
    at stringToStringTuples (codec.js:45:1)
    at stringToBytes (codec.js:182:1)
    at Object.fromString (codec.js:190:1)
    at __webpack_modules__../node_modules/multiaddr/src/index.js.withIs.proto.className (index.js:46:1)
    at new <anonymous> (index.js:40:1)
    at __webpack_modules__../node_modules/multiaddr/src/index.js.withIs.proto.className (index.js:29:1)
    at index.js:40:1
    at id.js:24:1
    at Array.map (<anonymous>)
> rg 'const res = await api.post\('\''id' node_modules

node_modules/ipfs-core/node_modules/ipfs-http-client/src/id.js
18:    const res = await api.post('id', {

node_modules/ipfs-cli/node_modules/ipfs-http-client/src/id.js
18:    const res = await api.post('id', {

node_modules/ipfs-http-client/src/id.js
13:    const res = await api.post('id', {

node_modules/ipfs-client/node_modules/ipfs-http-client/src/id.js
17:    const res = await api.post('id', {

@SgtPooki
Copy link
Member

SgtPooki commented Sep 27, 2022

Looks like the line throwing is https://github.com/multiformats/js-multiaddr/blob/aac01444747526149c8bff73e054597b61481007/src/protocols-table.js#L18

we're on multiaddr version 8.1.2 which doesn't support webtransport since it was just added 7 days ago multiformats/js-multiaddr@210f156 to version 11

╰─ ✔ ❯ npm ls multiaddr

ipfs-webui@2.18.1 /Users/sgtpooki/code/work/protocol.ai/ipfs/webui
├─┬ ipfs-http-client@49.0.2
│ ├─┬ ipfs-core-types@0.3.1
│ │ └── multiaddr@8.1.2 deduped
│ ├─┬ ipfs-core-utils@0.7.2
│ │ └── multiaddr@8.1.2 deduped
│ └── multiaddr@8.1.2 deduped
├─┬ ipfs@0.58.3
│ ├─┬ ipfs-cli@0.8.8
│ │ ├─┬ ipfs-core-types@0.7.3
│ │ │ └── multiaddr@10.0.1 deduped
│ │ ├─┬ ipfs-core-utils@0.10.5
│ │ │ ├─┬ ipfs-core-types@0.7.3
│ │ │ │ └── multiaddr@10.0.1 deduped
│ │ │ ├─┬ multiaddr-to-uri@8.0.0
│ │ │ │ └── multiaddr@10.0.1 deduped
│ │ │ └── multiaddr@10.0.1
│ │ ├─┬ ipfs-daemon@0.9.8
│ │ │ ├─┬ ipfs-core-types@0.7.3
│ │ │ │ └── multiaddr@10.0.1
│ │ │ ├─┬ ipfs-grpc-server@0.6.6
│ │ │ │ ├─┬ ipfs-core-types@0.7.3
│ │ │ │ │ └── multiaddr@10.0.1 deduped
│ │ │ │ └── multiaddr@10.0.1
│ │ │ ├─┬ ipfs-http-gateway@0.6.5
│ │ │ │ ├─┬ ipfs-core-types@0.7.3
│ │ │ │ │ └── multiaddr@10.0.1
│ │ │ │ ├─┬ is-ipfs@6.0.2
│ │ │ │ │ ├─┬ mafmt@10.0.0
│ │ │ │ │ │ └── multiaddr@10.0.1 deduped
│ │ │ │ │ └── multiaddr@10.0.1 deduped
│ │ │ │ └─┬ uri-to-multiaddr@6.0.0
│ │ │ │   └── multiaddr@10.0.1
│ │ │ └─┬ ipfs-http-server@0.7.6
│ │ │   ├─┬ ipfs-core-types@0.7.3
│ │ │   │ └── multiaddr@10.0.1 deduped
│ │ │   └── multiaddr@10.0.1
│ │ ├─┬ ipfs-http-client@52.0.5
│ │ │ └── multiaddr@10.0.1 deduped
│ │ ├─┬ mafmt@10.0.0
│ │ │ └── multiaddr@10.0.1 deduped
│ │ ├─┬ multiaddr-to-uri@8.0.0
│ │ │ └── multiaddr@10.0.1 deduped
│ │ └── multiaddr@10.0.1
│ └─┬ ipfs-core@0.10.8
│   ├─┬ ipfs-bitswap@6.0.3
│   │ ├─┬ libp2p-interfaces@1.3.1
│   │ │ └── multiaddr@10.0.1
│   │ └── multiaddr@10.0.1
│   ├─┬ ipfs-core-types@0.7.3
│   │ └── multiaddr@10.0.1 deduped
│   ├─┬ ipfs-http-client@52.0.5
│   │ └── multiaddr@10.0.1 deduped
│   ├─┬ is-ipfs@6.0.2
│   │ └── multiaddr@10.0.1 deduped
│   ├─┬ libp2p-bootstrap@0.13.0
│   │ ├─┬ mafmt@10.0.0
│   │ │ └── multiaddr@10.0.1 deduped
│   │ └── multiaddr@10.0.1
│   ├─┬ libp2p-delegated-content-routing@0.11.0
│   │ └── multiaddr@10.0.1
│   ├─┬ libp2p-kad-dht@0.24.2
│   │ └── multiaddr@10.0.1
│   ├─┬ libp2p-mdns@0.17.0
│   │ └── multiaddr@10.0.1
│   ├─┬ libp2p-tcp@0.17.2
│   │ ├─┬ libp2p-utils@0.4.1
│   │ │ └── multiaddr@10.0.1
│   │ ├─┬ mafmt@10.0.0
│   │ │ └── multiaddr@10.0.1 deduped
│   │ └── multiaddr@10.0.1
│   ├─┬ libp2p-webrtc-star@0.23.0
│   │ ├─┬ mafmt@10.0.0
│   │ │ └── multiaddr@10.0.1 deduped
│   │ └── multiaddr@10.0.1
│   ├─┬ libp2p-websockets@0.16.1
│   │ ├─┬ mafmt@10.0.0
│   │ │ └── multiaddr@10.0.1 deduped
│   │ ├─┬ multiaddr-to-uri@8.0.0
│   │ │ └── multiaddr@10.0.1 deduped
│   │ └── multiaddr@10.0.1
│   ├─┬ libp2p@0.32.5
│   │ ├─┬ mafmt@10.0.0
│   │ │ └── multiaddr@10.0.1 deduped
│   │ └── multiaddr@10.0.1
│   ├─┬ mafmt@10.0.0
│   │ └── multiaddr@10.0.1 deduped
│   ├─┬ multiaddr-to-uri@8.0.0
│   │ └── multiaddr@10.0.1 deduped
│   └── multiaddr@10.0.1
├─┬ ipfsd-ctl@7.2.0
│ └── multiaddr@8.1.2 deduped
├─┬ is-ipfs@3.0.0
│ ├─┬ mafmt@8.0.4
│ │ └── multiaddr@8.1.2 deduped
│ └── multiaddr@8.1.2 deduped
├─┬ multiaddr-to-uri@6.0.0
│ └── multiaddr@8.1.2 deduped
└── multiaddr@8.1.2

@SgtPooki
Copy link
Member

attempted a quick fix but was unsuccessful:

fc939b7 results in an error

98587cb results in another error:

Failed to get identity Error: invalid character 'u' in 'uEiD1qy_pwUXrusX6CufD-gipAzNSz52OVOhJUUOLpmmMNw'
    at Base.decode (base.js:43:1)
    at fromString (from-string.js:51:1)
    at Function.convertToBytes [as toBytes] (convert.js:84:1)
    at codec.js:97:1
    at Array.map (<anonymous>)
    at stringTuplesToTuples (codec.js:91:1)
    at stringToBytes (codec.js:183:1)
    at Object.fromString (codec.js:190:1)
    at __webpack_modules__../node_modules/multiaddr/src/index.js.withIs.proto.className (index.js:46:1)
    at new <anonymous> (index.js:40:1)

@lidel
Copy link
Member Author

lidel commented Sep 27, 2022

multiformats/js-multiaddr#271 was missing tests for /webtransport/certhash/../certhash/.. (addr format announced by Kubo 0.16.0-rc1)

I've added them in multiformats/js-multiaddr#276 but code from multiaddr@11 parses it correctly, so this looks like a problem on ipfs-webui's end (ancient dependencies).

Probably the only workaround here is reverse engineering the exception this and creating an ad-hoc patch against multiaddr@8.1.2 that does not throw when certhash is present.

@BigLep
Copy link

BigLep commented Oct 4, 2022

What's the verdict here? Are we going to do more hacks, or just this as more forcing function to get the dependencies updated... ?

@SgtPooki
Copy link
Member

SgtPooki commented Oct 7, 2022

What's the verdict here? Are we going to do more hacks, or just this as more forcing function to get the dependencies updated... ?

@BigLep This is going to require dependency updates.. hacks aren't really going to work without those hacks bleeding into older dependency versions that don't require those hacks in the newest versions. It's most likely less work to update webui than try to figure out a working hack

@BigLep
Copy link

BigLep commented Oct 8, 2022

@SgtPooki : ACK. One more thing dependent on the dependency update project! Let's add it to the issue to put further weight on the criticality of that work. Thanks.

@SgtPooki SgtPooki added topic/interop Interoperability P0 Critical: Tackled by core team ASAP and removed need/triage Needs initial labeling and prioritization P1 High: Likely tackled by core team if no one steps up labels Oct 10, 2022
@lidel
Copy link
Member Author

lidel commented Oct 13, 2022

Just flagging this is a hard blocker for having WebTransport enabled by default in Kubo.

@SgtPooki
Copy link
Member

@marten-seemann per our discussion, see #2033 (comment)

We can dive into this deeper at your convenience

@SgtPooki
Copy link
Member

SgtPooki commented Oct 25, 2022

Just flagging this is a hard blocker for having WebTransport enabled by default in Kubo.

@lidel is this status text not being visible really worth blocking enabling webtransport by default?

Would adding a UI notification with link to a tracking issue for this problem(this issue) be sufficient to unblock kubo? I don't feel like blocking the ecosystem because UI rendering a text field is broken is worth the delay.

I'm open to being corrected.

@SgtPooki SgtPooki linked a pull request Oct 26, 2022 that will close this issue
@whizzzkid whizzzkid added dependencies and removed status/blocked Unable to be worked further until needs are met labels Oct 31, 2022
lidel pushed a commit that referenced this issue Nov 7, 2022
ipfs-gui-bot pushed a commit that referenced this issue 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 issue has been resolved 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
kind/bug A bug in existing code (including security flaws) P0 Critical: Tackled by core team ASAP released topic/interop Interoperability
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants