Tahoe invite.3 #418
Tahoe invite.3 #418
Conversation
|
The only remaining tweak here (besides some squashing and depending on a wormhole release with the new API) would be I think deciding on the APPID and relay-URI to burn into |
|
For what it's worth, Gridsync currently expects an Also, +1 to |
|
Would it be more appropriate to make the |
I think making wormhole.tahoe-lafs.org a CNAME for wormhole.leastauthority.com (which is already set up and points at a wormhole server) would do it, right? That should be the work of a couple minutes at most. |
Codecov Report
@@ Coverage Diff @@
## master #418 +/- ##
==========================================
+ Coverage 87.98% 88.09% +0.11%
==========================================
Files 147 148 +1
Lines 27835 27951 +116
Branches 3973 3986 +13
==========================================
+ Hits 24491 24624 +133
+ Misses 2626 2615 -11
+ Partials 718 712 -6
Continue to review full report at Codecov.
|
|
Hmm. My coverage tool says 100% of the diff is covered...Does codecov not update its comment when more commits are added? |
|
Ah, it does update. but still seems confused? or there's some Heisen-coverage? |
|
CNAME sounds good to me: tahoe source code will use Maybe we can factor things to make the |
This is a good idea in general, so I'll do this. However, I don't think it will help GridSync specifically, as it's written in Python3 and tahoe doesn't support that. There's some slim chance we could make "just this one API" able to be imported in py3 which seems fragile. Also, the API which would help gridsync would be the "client" side (i.e. The use-cases I expect for Is it worth making a "more machine-readable" version of the output? Perhaps; that could be a future |
meejah
force-pushed
the
tahoe-invite.3
branch
2 times, most recently
from
9f462e9
to
8f6ad97
Compare
|
p.s. I think this is worth having in Tahoe before doing any "grid manager" or "accounting" things for at least two reasons:
|
There was a problem hiding this comment.
Thanks! This looks really good. I left some comments inline.
One other thing that's on my mind is how to structure the code so that the versioning scheme can actually be used easily. I think it would be better for future changes if we didn't assume server-v1 and client-v1 in very many places - or if those places were explicitly about v1. I don't have any specific suggestions for how to re-arrange the code to accommodate that idea, though. Perhaps just something to think about.
Lastly, what about long-form/prose documentation for this new feature? The split nature of the feature, between invite and join, suggests something that it would be nice to explain in a single cohesive document. The usage docs included with each command are nice references but I don't think they'll help users who don't already have a decent idea of what the feature is.
src/allmydata/_auto_deps.py
Outdated
| @@ -91,6 +91,9 @@ | |||
| "PyYAML >= 3.11", | |||
|
|
|||
| "six >= 1.10.0", | |||
|
|
|||
| # for 'tahoe invite' and 'tahoe join' | |||
| "magic-wormhole", # XXX for now, we need master! | |||
There was a problem hiding this comment.
Are you going to hold off merging this until the next magic-wormhole release?
There was a problem hiding this comment.
Yes I was hoping we'd have a "real" magic-wormhole release before this gets merged. But, I could just depend on master and make sure we don't do a tahoe-lafs release before magic-wormhole?
src/allmydata/scripts/create_node.py
Outdated
|
|
||
| wh = wormhole.create( | ||
| appid=u"tahoe-lafs.org/lafs", | ||
| relay_url=u"ws://wormhole.leastauthority.com:4000/v1", |
There was a problem hiding this comment.
I suppose this is still meant to change - but not until wormhole.tahoe-lafs.org gets configured? Though I don't see the harm in merging this as-is if setting up that alternate name takes too long. It would be a simple matter to update it in the future after DNS configuration is done.
There was a problem hiding this comment.
Also, these two strings would probably make good global constants somewhere.
src/allmydata/scripts/create_node.py
Outdated
|
|
||
| print >>out, " received server introduction" | ||
| assert u'abilities' in server_intro | ||
| assert u'server-v1' in server_intro['abilities'] |
There was a problem hiding this comment.
Rather than assert, what do you think about doing a regular if and presenting a more presentable failure to the user? The traceback is likely to be meaningless to 99% of potential users.
Also, Tahoe-LAFS should include some documentation explaining what the server-v1 ability corresponds to. This will be pretty boring, I think, but it will set the stage for someone to write something interesting for server-v2.
src/allmydata/scripts/create_node.py
Outdated
| for k, v in remote_config.items(): | ||
| if k not in ['happy', 'needed', 'total', 'introducer']: | ||
| print >>out, " --{}={}".format(k, v) | ||
| config[k] = v |
There was a problem hiding this comment.
This is a bit confusing. The config doesn't actually map to tahoe create command line arguments, right? This is more of a metaphor to try to explain things to the user?
I don't see an obvious problem with letting the server any supported configuration option... but part of me still wants to err on the side of a whitelist. Perhaps because I don't have a deep understanding of every single current Tahoe-LAFS option so I'm not confident it's safe to expose them all to the server. Or perhaps just out of fear of what new behaviors might arise as folks add new configuration options.
Or, from another angle, if the client lets any option be set, the server won't actually know which options it can set on the client. If a new config option is added to Tahoe-LAFS but client-v1 is still in use, a server will have no way to know whether it's talking to a client that supports that option or not. It might be harmless to set it for everyone or the server might actually care about doing something different for older clients. So ... I'd go with a whitelist and bump the client version somehow (either the number or add an item to the corresponding object that tells the server what the client's whitelist is).
Also, the handling of introducer points in the direction of another reason for a whitelist. Perhaps a new sensitive value will be added to the configuration at some point. Without knowing about each key being set, the client has no way to elide that new sensitive value from the logs.
There was a problem hiding this comment.
It does map to options, but it turns out the only thing not in that list is --nickname so yeah probably just whitelisting it (er, well explicitly pulling out the ones that are ok) is best.
| from allmydata.util import configutil | ||
|
|
||
|
|
||
| INVITE_SEPARATOR = "+" |
| @@ -80,7 +83,7 @@ def validate_where_options(o): | |||
| else: | |||
| # no --location and --port? expect --listen= (maybe the default), and | |||
| # --listen=tcp requires --hostname. But --listen=none is special. | |||
| if o['listen'] != "none": | |||
| if o['listen'] != "none" and o.get('join', None) is None: | |||
There was a problem hiding this comment.
I think this implies that "a dict with some keys, you know what they are, right?" as a data-type is failing validate_where_options. It would be pretty cool to introduce something a little bit better defined here.
Not necessarily part of (or blocking) this PR, since the problem is clearly pre-existing...
src/allmydata/scripts/create_node.py
Outdated
| @@ -221,7 +225,9 @@ def write_node_config(c, config): | |||
| c.write("\n") | |||
|
|
|||
| c.write("[node]\n") | |||
| nickname = argv_to_unicode(config.get("nickname") or "") | |||
| nickname = config.get("nickname") or u"" | |||
| if not isinstance(nickname, unicode): | |||
There was a problem hiding this comment.
When is it not? Is it only for the u"" default above? If so, I suggest b"" as a default and drop the conditional (ie, unconditionally decode).
|
|
||
| wh = wormhole.create( | ||
| appid=u"tahoe-lafs.org/lafs", | ||
| relay_url=u"ws://wormhole.tahoe-lafs.org:4000/v1", |
There was a problem hiding this comment.
Oh. And here is the wormhole.tahoe-lafs.org name. :) That's a good argument in favor of making this a constant somewhere and sharing it with the other application code that needs it.
There was a problem hiding this comment.
yeah :) I now made it a global option. maybe overkill? but might be useful when testing etc...
| "--total", "1", | ||
| "foo", | ||
| ) | ||
| self.assertTrue(rc != 0) |
| "foo", | ||
| ) | ||
| self.assertTrue(rc != 0) | ||
| self.assertTrue( |
|
per @warner's comment about API: you could currently import the Since it makes sense to re-factor/re-work the whole "runner" and dispatch code anyway (in a different PR!) perhaps we could supply a slightly easier way to run any particular command "from python" (which would also help the unit-tests). |
|
The protocol for client-v1, server-v1 etc was taken from discussion here: gridsync/gridsync#42 |
meejah
force-pushed
the
tahoe-invite.3
branch
2 times, most recently
from
0e73198
to
e717a8a
Compare
|
Okay, I've squashed and re-based to current master (and changed the URI to This still leaves the "what to depend on" part out though! |
meejah
force-pushed
the
tahoe-invite.3
branch
3 times, most recently
from
ebb0ab7
to
66c0921
Compare
| synopsis = "[options] <nickname>" | ||
| description = "Create a client-only Tahoe-LAFS node (no storage server)." | ||
|
|
||
| optParameters = [ |
There was a problem hiding this comment.
For the sake of being consistent with the standard tahoe create-client/tahoe create-node options, these should probably be changed throughout to --shares-needed, --shares-happy, and --shares-total (instead of just --needed, --happy, and --total).
|
While testing this, I noticed that Running |
src/allmydata/scripts/create_node.py
Outdated
| print >>out, "Connecting to '{}'".format(relay_url) | ||
|
|
||
| wh = wormhole.create( | ||
| appid=u"tahoe-lafs.org/lafs", |
There was a problem hiding this comment.
This should probably be changed to tahoe-lafs.org/tahoe-lafs/v1 (so as to be compatible with this and others).
On a related note, we might want to consider now whether this should be bumped up to "v2", or even be made more specific (perhaps so as to distinguish the "grid invites" of now from the "magic-folder invites" of the future -- maybe something like tahoe-lafs.org/tahoe-lafs/invite vs. tahoe-lafs.org/tahoe-lafs/magic-folder/invite?).
There was a problem hiding this comment.
Did we release anything with the "v1" appid in it?
Also, is the appid the right place to do versioning like this, or in the capabilities exchanged as the first messages? I guess the "pro" of doing it in the appid is no possibilities of "crossing the streams" and accidentally receiving a grid-invite as a magic-folder-invite..? (but still, in such a case the capabilities wouldn't match anyway...)
There was a problem hiding this comment.
(But yes the main point about the appid being wrong/not-matching above needs to be fixed)
There was a problem hiding this comment.
Did we release anything with the "v1" appid in it?
Nope.. I remember @exarkun suggesting earlier, however, that a version-bump might make for good practice (but don't recall us reaching any sort of consensus on moving forward with that idea -- and may even be mistaken in thinking that he was referring to the appid specifically now that I think about it..). Personally, I have no qualms either way, but wanted to raise the the question here in the event that anyone felt strongly about what the appid should be more generally (since it isn't something we can easily change if/when this PR lands); like you implied, what really matters here is that everything is compatible.
Also, is the appid the right place to do versioning like this, or in the capabilities exchanged as the first messages?
Hmm.. That's a good question. Reducing the possibility of "crossing the streams" by using separate appids where possible is desirable for both security and usability reasons (since an attacker would need to flood channels on both appids instead of just one -- while fewer nameplates in use means potentially fewer characters to type!) and I was already operating under the assumption that we would be doing this later on to isolate magic-folder-invites from grid-invites (or at least I can't really think of any good reasons right now for keeping the appids the same for both types). On that note, I was under the impression -- and please correct me if I'm wrong -- that the capabilities-exchange was intended more so as a means of handling the situation in which we might want/need to change the data-format of a specific invite-type in a way that is not backwards-compatible with the previous one (i.e., as a means of signaling to users more broadly that they need to update their clients) -- in other words, a means of migrating from grid-invite "version 1" to grid-invite "version 2" independently of the appid (and not so much as a means of packing magic-folder-invites in with grid-invites under the same appid).
All that being said, I suppose it doesn't really matter at this point, since we're only handling one "type" of invite right now and the capabilities-exchange gives us a good way -- better than changing the appid, certainly -- of updating its data-format in the unlikely event that we ever need to (and in retrospect I probably should have framed my original comment to more clearly distinguish between a) the question of isolating future invite-"types" and b) the question of versioning since they can be addressed independently of one another).
Maybe, then, we should just omit the "v1" suffix from the current appid altogether (i.e., use tahoe-lafs.org/tahoe-lafs everywhere) and use the capabilities-exchange to handle any/all "versioning"/update-messaging stuff moving forward? Using the appid for versioning seems like a pretty bad idea to me overall (since, in practice, a sender would always have to open at least two wormholes -- one using the "old" appid(s) and one using the "new" -- in order to tell clients on the old one to update).. What do you think?
|
After further testing, it looks like To demonstrate, I pointed Because my (The |
meejah
force-pushed
the
tahoe-invite.3
branch
4 times, most recently
from
db429ed
to
43580a9
Compare
meejah
dismissed
exarkun’s stale review
I believe I have covered all the things; thanks for the review!
meejah
force-pushed
the
tahoe-invite.3
branch
2 times, most recently
from
d8ca7a1
to
4a65fb9
Compare
|
re-based this to current master as of today |
docs/magic-wormhole-invites.rst
Outdated
| "total": 10, | ||
| "happy": 7, | ||
| "nickname": "bob", | ||
| "introducer": "pb://xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx@example.com:41505" |
There was a problem hiding this comment.
pb://xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx@example.com:41505/yyyyyyyyyyyyyyyyyyyyyyy perhaps
docs/running.rst
Outdated
| Being Introduced to a Grid | ||
| -------------------------- | ||
|
|
||
| A collection of Tahoe servers is called a Grid and usually has 1 Introducer (but sometimes more, and it's possible to run with zero). The Introducer announces which storage servers constitute the Grid and how to contact them. There is a secret "fURL" you need to know to talk to the Introducer. |
There was a problem hiding this comment.
Other docs are hard-wrapped at around 80 cols but these docs seem to be paragraph-per-line.
There was a problem hiding this comment.
hmm, yup. re-wrapped.
| print("waiting 5 seconds unil we're maybe ready") | ||
| yield task.deferLater(reactor, 5, lambda: None) | ||
| print("waiting 10 seconds unil we're maybe ready") | ||
| yield task.deferLater(reactor, 10, lambda: None) |
There was a problem hiding this comment.
Ha ha 😢
#430 has landed, perhaps it's possible to do better than this for now.
Did this get bumped from 5 to 10 because of changes in this branch or just because time-based wait is inherently unreliable and in your development environment 10 provides a better facsimile of correctness than 5?
There was a problem hiding this comment.
...and yes teaching the tests to properly figure out when "the grid is ready" would be the right answer here. Hopefully #430 will help, I'll try (in a new branch).
src/allmydata/scripts/create_node.py
Outdated
| relay_url = config.parent['wormhole-server'] | ||
| print >>out, "Connecting to '{}'".format(relay_url) | ||
|
|
||
| wh = wormhole.create( |
There was a problem hiding this comment.
Probably would be better if the wormhole exchange were factored into a separate function and just called from here.
| try: | ||
| introducer_furl = config.get('client', 'introducer.furl') | ||
| except NoSectionError: | ||
| # we're not a client; maybe this is running *on* the introducer? |
There was a problem hiding this comment.
I have no clue if this is the right heuristic to use to detect the kind of node. It's too bad a heuristic is required, I guess.
If one is required, maybe it could at least be formalized and centralized? A determine_node_type API that this code could use?
There was a problem hiding this comment.
Yeah, maybe that would be better. There is a helper for that. I don't really care what type of node this is, it's more that there isn't just one way to get "the introducer fURL" -- maybe I really want to add a helper for that ...
| client_intro = yield wh.get_message() | ||
| print >>out, " received client introduction" | ||
| client_intro = json.loads(client_intro) | ||
| if not u'abilities' in client_intro: |
There was a problem hiding this comment.
u'abilities' not in client_intro
|
|
||
| @defer.inlineCallbacks | ||
| def test_create_node_join(self): | ||
| node_dir = self.mktemp() |
There was a problem hiding this comment.
Having recently tried to maintain some unit tests with no explanatory documentation regarding the case being covered, I think it would be nice to see some docs for that here.
| "foo", | ||
| ) | ||
| self.assertNotEqual(rc, 0) | ||
| self.assertIn(u"Can't find introducer fURL", out + err) |
There was a problem hiding this comment.
It seems like the run_cli args and following assertions are the only parts which vary between many of these tests. Perhaps it would be nice to factor out the rest into a helper to reduce future maintenance burden and make the tests themselves easier to read.
src/allmydata/test/test_node.py
Outdated
| def setUp(self): | ||
| testutil.SignalMixin.setUp(self) | ||
| self.parent = LoggingMultiService() | ||
| self.parent.startService() | ||
| self._available_port = yield iputil.allocate_tcp_port() |
There was a problem hiding this comment.
Changes in this module look unrelated, just general reliability improvements for the test suite by avoiding random port collisions? If so it might be nice to split this off into a separate pr.
| her command running until Bob has done his step as it is waiting until | ||
| a secure channel is established before sending the data. | ||
|
|
||
| Bob then runs ``tahoe create-client --join <secret code>`` with any |
There was a problem hiding this comment.
I forget if there was a discussion about the verb "join" here. I think @warner hinted at preferring something like "accept" (but I can't channel his reasoning). They both make equal sense to me, fwiw. Either you are "joining" a grid to which you were "invited" or you are "accepting" an "invitation" to a grid...
There was a problem hiding this comment.
I don't have a good reason for preferring one over the other; happy to paint the bikeshed whichever colour @warner wants :)
meejah
force-pushed
the
tahoe-invite.3
branch
5 times, most recently
from
347d38f
to
f79c500
Compare
|
Pulled out the port 1234 stuff: #435 |
meejah
force-pushed
the
tahoe-invite.3
branch
2 times, most recently
from
4561af5
to
18c1e78
Compare
|
BTW, the CNAME step is done. |
This opens a wormhole and sends appropriate JSON down it to a tahoe-gui using a wormhole server running on tahoe-lafs.org The other end uses the 'tahoe create-node' command (with new --join option) to read the configuration JSON from a 'tahoe invite' command
|
Looks good. I had to restart the travis build for some reason (it completed but then didn't update the ticket). Landing now. Way to go! |
As per gridsync/gridsync#42 this branch updates
tahoe-invite.2to use a "bare"wormholeobject and do the client and server "abilities" messages before exchanging the actual configuration.Note that this actually depends on wormhole "master" as that includes the new
wormhole.wormhole.create()method among others.Before merging:
wormhole.tahoe-lafs.orga CNAME towormhole.leastauthority.com.get_welcome()etc)