Fix `stringify()` in net and host to respect host #48

christianbundy · 2019-05-11T21:34:18Z

Previously we were disregarding the host setting for all configs
except in cases where scope === 'public' && opts.external != null. This resolves the error by first checking whether opts.host` is defined
before falling back to automatic detection methods.

Resolves #47

Before

net:192.168.0.109:8008~shs:+oaWWDs8g73EZFUMfW37R/ULtFEjwKN/DczvdYihjbU=
net:192.168.0.109:8008~shs:+oaWWDs8g73EZFUMfW37R/ULtFEjwKN/DczvdYihjbU=
net:192.168.0.109:8008~shs:+oaWWDs8g73EZFUMfW37R/ULtFEjwKN/DczvdYihjbU=
ws://192.168.0.109:8989~shs:+oaWWDs8g73EZFUMfW37R/ULtFEjwKN/DczvdYihjbU=
ws://192.168.0.109:8989~shs:+oaWWDs8g73EZFUMfW37R/ULtFEjwKN/DczvdYihjbU=
ws://192.168.0.109:8989~shs:+oaWWDs8g73EZFUMfW37R/ULtFEjwKN/DczvdYihjbU=

After

net:192.168.0.109:8008~shs:+oaWWDs8g73EZFUMfW37R/ULtFEjwKN/DczvdYihjbU=
net:172.18.0.1:8008~shs:+oaWWDs8g73EZFUMfW37R/ULtFEjwKN/DczvdYihjbU=
net:fce2:9811:4862:81a7:bb08:91d6:2e41:d220:8008~shs:+oaWWDs8g73EZFUMfW37R/ULtFEjwKN/DczvdYihjbU=
ws://192.168.0.109:8989~shs:+oaWWDs8g73EZFUMfW37R/ULtFEjwKN/DczvdYihjbU=
ws://172.18.0.1:8989~shs:+oaWWDs8g73EZFUMfW37R/ULtFEjwKN/DczvdYihjbU=
ws://[fce2:9811:4862:81a7:bb08:91d6:2e41:d220]:8989~shs:+oaWWDs8g73EZFUMfW37R/ULtFEjwKN/DczvdYihjbU=

cc: @dominictarr @arj03 @regular @cryptix @the-kenny

Previously we were disregarding the `host` setting for all configs except in cases where `scope === 'public' && `opts.external != null`. This resolves the error by first checking whether `opts.host` is defined before falling back to automatic detection methods.

arj03 · 2019-05-12T08:32:41Z

The code is a bit confusing, like host is defined here but is only used in the server code it seems. Also stringify has a close but different interpretation of host.

Would be nice with some tests of this stuff. There are so many configurations to take into account.

arj03 · 2019-05-12T10:00:01Z

A bit more context for other people reading this. Seems to be this commit where multiserver scopes is always picking the default for the scope. So this is only a problem if you have multiple interfaces.

christianbundy · 2019-05-13T21:57:35Z

Yeah, I think I've cleaned it up about as well as I'm able to without making any breaking changes, but it's still a bit hairy. Hopefully the comments are useful for future code spelunkers and the bugfix is useful for folks with multiple network interfaces (apparently common on macOS (see ssbc/patchwork#1035)).

plugins/ws.js

arj03 · 2019-05-14T06:36:43Z

Nice refactor @christianbundy. Still thinking about writing a test, maybe I can have a go at that.

arj03 · 2019-05-14T08:49:21Z

Pushed up two tests

the-kenny · 2019-05-14T09:32:06Z

Hey there! I'm the guy who originally discovered the issue on my machines.

I just rebuilt Patchwork using the latest multiserver from this branch (package-lock.json shows commit e9376b5 and I can report there is at least some progress :-)

Right now tcpdump -s 0 -u port 8008 -A (plus some formatting) displays the following announcement:

net:fe80::aede:48ff:fe00:1122:8008~shs:mucTrTjExFklGdAFobgY4zypBAZMVi7q0m6Ya55gLVo=
net:fe80::aede:48ff:fe00:1122:8008~shs:mucTrTjExFklGdAFobgY4zypBAZMVi7q0m6Ya55gLVo=
net:fe80::aede:48ff:fe00:1122:8008~shs:mucTrTjExFklGdAFobgY4zypBAZMVi7q0m6Ya55gLVo=
ws://10.10.20.73:8989~shs:mucTrTjExFklGdAFobgY4zypBAZMVi7q0m6Ya55gLVo=
ws://[2a0a:a545:2448:0:189a:8e5d:ef83:8eff]:8989~shs:mucTrTjExFklGdAFobgY4zypBAZMVi7q0m6Ya55gLVo=
ws://[2a0a:a545:2448:0:e1a4:d7c8:5d23:5d54]:8989~shs:mucTrTjExFklGdAFobgY4zypBAZMVi7q0m6Ya55gLVo=

So there's definitely progress for the ws:// side of things. However, the net: announcements are still using the IP of a single (disconnected) interface.

related to ssbc/patchwork#1035

christianbundy · 2019-05-14T16:53:32Z

@the-kenny

Hey, could you run npm ls multiserver and ensure that it looks like this:

$ npm ls multiserver
ssb-patchwork@3.12.0-beta.2 /home/christianbundy/src/ssbc/patchwork
├─┬ ssb-client@4.7.4
│ └── UNMET DEPENDENCY multiserver@3.3.2 
├─┬ ssb-server@14.1.12 (github:ssbc/ssb-server#982a053a608a19de59638c0056832bce34b8436b)
│ ├── UNMET DEPENDENCY multiserver@3.3.2 
│ ├─┬ secret-stack@6.1.2
│ │ └── UNMET DEPENDENCY multiserver@3.3.2 
│ └─┬ ssb-ws@5.1.1
│   └── UNMET DEPENDENCY multiserver@3.3.2 
└─┬ ssb-ws@6.0.0
  └── UNMET DEPENDENCY multiserver@3.3.2 

npm ERR! missing: multiserver@3.3.2, required by ssb-client@4.7.4
npm ERR! missing: multiserver@3.3.2, required by ssb-server@14.1.12
npm ERR! missing: multiserver@3.3.2, required by secret-stack@6.1.2
npm ERR! missing: multiserver@3.3.2, required by ssb-ws@5.1.1
npm ERR! missing: multiserver@3.3.2, required by ssb-ws@6.0.0

Unfortunately ssb-server has a shrinkwrap file, which means that a normal npm link doesn't affect those dependencies. If you're using npm install to test this commit it won't get those dependencies either, which makes it hard. I can confirm that on this branch I'm still seeing the fixed values:

net:192.168.3.55:8008~shs:+oaWWDs8g73EZFUMfW37R/ULtFEjwKN/DczvdYihjbU=
net:172.18.0.1:8008~shs:+oaWWDs8g73EZFUMfW37R/ULtFEjwKN/DczvdYihjbU=
net:fce2:9811:4862:81a7:bb08:91d6:2e41:d220:8008~shs:+oaWWDs8g73EZFUMfW37R/ULtFEjwKN/DczvdYihjbU=
ws://192.168.3.55:8989~shs:+oaWWDs8g73EZFUMfW37R/ULtFEjwKN/DczvdYihjbU=
ws://172.18.0.1:8989~shs:+oaWWDs8g73EZFUMfW37R/ULtFEjwKN/DczvdYihjbU=
ws://[fce2:9811:4862:81a7:bb08:91d6:2e41:d220]:8989~shs:+oaWWDs8g73EZFUMfW37R/ULtFEjwKN/DczvdYihjbU=

I don't think there's an easy way to test this in Patchwork right now.

christianbundy · 2019-05-14T16:54:34Z

Also: thanks a bunch for adding those tests!

the-kenny · 2019-05-14T20:22:05Z

@christianbundy sure, here you are!

$ npm ls multiserver
ssb-patchwork@3.12.0-beta.2 /Users/moritz/dev/patchwork
├── multiserver@3.3.2  (github:ssbc/multiserver#a6285b2282ca3d3ce78a6b47ced28324d64e4b3f)
├─┬ ssb-client@4.7.5
│ └── multiserver@3.3.2  deduped (github:ssbc/multiserver#a6285b2282ca3d3ce78a6b47ced28324d64e4b3f)
├─┬ ssb-server@14.1.12
│ ├── multiserver@3.3.2
│ ├─┬ secret-stack@6.1.0
│ │ └── multiserver@3.3.2  deduped
│ ├─┬ ssb-client@4.7.2
│ │ └── multiserver@3.3.2  deduped
│ └─┬ ssb-ws@5.1.1
│   └── multiserver@3.3.2  deduped
└─┬ ssb-ws@6.0.0
  └── multiserver@3.3.2  deduped (github:ssbc/multiserver#a6285b2282ca3d3ce78a6b47ced28324d64e4b3f)

Note that I added multiserver as a dependency pointing to this branch directly to patchwork to override whatever version its dependencies use.

arj03 · 2019-05-15T06:40:04Z

@the-kenny looks as if the depedency in ssb-server is still pointing to 3.3.2 and not this branch, that is probably why its not working is my guess.

the-kenny · 2019-05-15T07:40:27Z

@arj03 Oh, are you sure? I thought the git-ref to the latest commit in this branch and all the deduped markers tell me it's using the correct one.

christianbundy · 2019-05-15T13:14:23Z

That's my understanding as well. The ssb-server shrinkwrap makes it so that you can't easily change versions. You can confirm with `find node_modules -name multiserver` I think, that should output all of the different instances of the multiserver dependency. Maybe you can delete the `ssb-server/node_modules/multiserver` and replace it with the one from Git? No easy way to do this with npm though.

…

On Wed, May 15, 2019, at 00:40, Moritz Ulrich wrote: @arj03 <https://github.com/arj03> Oh, are you sure? I thought the git-ref to the latest commit in this branch and all the `deduped` markers tell me it's using the correct one. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#48?email_source=notifications&email_token=AAEDIZHS75OO3KRDOGHZUQLPVO46ZA5CNFSM4HMJA5B2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVNZNAQ#issuecomment-492541570>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEDIZCROTADJJKHKGHUPBDPVO46ZANCNFSM4HMJA5BQ>.

the-kenny · 2019-05-15T21:59:15Z

@christianbundy Wow, NPM really is a mess. The hack to replace ssb-server/node_modules/multiserver did it. I'm annoucing the correct IPs now :-) Thanks for taking your time with this.

christianbundy · 2019-05-15T22:53:05Z

😨 Shrinkwraps... 😱

arj03 · 2019-05-17T22:18:33Z

@christianbundy everyone is at dtn, so don't expect much reply to this for a few days, but for me this looks ready to merge.

christianbundy · 2019-05-17T22:58:48Z

Oh, I'd totally forgotten about that. I'll merge and release as a patch, then I'll push my other multiserver branch. 🙃

christianbundy added the bug label May 11, 2019

christianbundy self-assigned this May 11, 2019

christianbundy force-pushed the fix-host branch from c36115a to ad146a3 Compare May 11, 2019 22:30

christianbundy added 3 commits May 13, 2019 14:22

Clean up net plugin

54b4635

More cleanup, use explicit type coercion

e0a5deb

Ensure that we still fallback to multiserver-scopes

9510b11

arj03 reviewed May 14, 2019

View reviewed changes

plugins/ws.js Outdated Show resolved Hide resolved

Add tests for host fix

e9376b5

Add small refactor for ws plugin

a6285b2

christianbundy mentioned this pull request May 16, 2019

How can I change to ip address ssb server runs on? ssbc/ssb-server#665

Open

christianbundy merged commit 9aff759 into master May 17, 2019

This was referenced May 21, 2019

Patchwork listens on IPv6 but advertises IPv4 address ssbc/patchwork#979

Closed

Local discovery still announcing wrong IPs on 3.12.0-beta2 ssbc/patchwork#1035

Closed

christianbundy deleted the fix-host branch August 25, 2020 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `stringify()` in net and host to respect host #48

Fix `stringify()` in net and host to respect host #48

christianbundy commented May 11, 2019

arj03 commented May 12, 2019

arj03 commented May 12, 2019

christianbundy commented May 13, 2019

arj03 commented May 14, 2019

arj03 commented May 14, 2019

the-kenny commented May 14, 2019

christianbundy commented May 14, 2019

christianbundy commented May 14, 2019

the-kenny commented May 14, 2019

arj03 commented May 15, 2019

the-kenny commented May 15, 2019

christianbundy commented May 15, 2019 via email •

edited

the-kenny commented May 15, 2019 •

edited

christianbundy commented May 15, 2019

arj03 commented May 17, 2019

christianbundy commented May 17, 2019

Fix stringify() in net and host to respect host #48

Fix stringify() in net and host to respect host #48

Conversation

christianbundy commented May 11, 2019

Before

After

arj03 commented May 12, 2019

arj03 commented May 12, 2019

christianbundy commented May 13, 2019

arj03 commented May 14, 2019

arj03 commented May 14, 2019

the-kenny commented May 14, 2019

christianbundy commented May 14, 2019

christianbundy commented May 14, 2019

the-kenny commented May 14, 2019

arj03 commented May 15, 2019

the-kenny commented May 15, 2019

christianbundy commented May 15, 2019 via email • edited

the-kenny commented May 15, 2019 • edited

christianbundy commented May 15, 2019

arj03 commented May 17, 2019

christianbundy commented May 17, 2019

Fix `stringify()` in net and host to respect host #48

Fix `stringify()` in net and host to respect host #48

christianbundy commented May 15, 2019 via email •

edited

the-kenny commented May 15, 2019 •

edited