[01:56] https://bugs.launchpad.net/juju-core/+bug/1059921 [01:56] --format=json :( [05:03] davecheney: I've been explicitly using --format=json to avoid formatting incompatibilities between go/python [05:07] SpamapS: yeah, i'm fixing the command line parser to handle this case [05:11] lucky(~/src/launchpad.net/juju-core) % juju bootstrap --upload-tools [05:11] lucky(~/src/launchpad.net/juju-core) % juju deploy mysql [05:11] error: no reachable servers [05:11] outstanding ... bootstrap didn't [05:11] aws said yes [05:11] but the console says that instance is stillborne [06:26] https://code.launchpad.net/~dave-cheney/gnuflag/001-parse-more-gnu-args/+merge/127419 [06:26] ^ gnuflags doesn't have lbox setup [06:39] fwereade, davechen1y: mornin' [06:39] monring [06:39] rogpeppe, davecheney, heyhey [06:39] rogpeppe: i have a non CL patch for you to review [06:40] rogpeppe, davecheney: I'm thinking I might actually take my swap day, I fell asleep really early last night and would prefer not to waste the sudden urge to rest [06:40] rogpeppe, davecheney, *but* ofc I have reviews to respond to first :p [06:40] fwereade: sgtm - don't forget meeting this aternoon [06:40] fwereade: ok, have fun if so! [06:40] davecheney, good point, ty [06:40] davecheney: i was about to ask if you had any reviews you wanted me to look at [06:41] one to gnuflags [06:41] rogpeppe: https://code.launchpad.net/~dave-cheney/gnuflag/001-parse-more-gnu-args/+merge/127419 [06:42] needed, basically, because all the charms expect it [06:42] davecheney: no codereview link? [06:43] rogpeppe: it doesn't make one [06:43] hang on [06:43] davecheney: what doesn't make one? [06:43] lbox [06:43] just found the option [06:43] davecheney: -cr [06:43] https://codereview.appspot.com/6588060 [06:44] rogpeppe: why don't i need to do that on juju-core ? [06:44] davecheney: there's a .lbox file in its root [06:44] davecheney: with flags in [06:45] fairy nuf [06:57] davecheney: i'm on the review BTW [06:59] davecheney: i'm not sure it's as simple as your CL tries to make it (which is why i didn't do it before) [07:10] davecheney: you've got a review [08:30] good morning [08:54] hi fwereade [09:03] TheMue, heyhey, I'm not really here :) [09:03] oh, where you are? [09:05] TheMue, swap-daying [09:05] TheMue, I'm tired and it's my birthday :) [09:05] fwereade: ah, ic. so what do you do here? ;) [09:05] oh, happy birthday [09:06] TheMue, well, the most entertaining thing I can do for the next 10 mins is poke at how we implement relations and try to come up with something cleaner than what we have for the current use cases [09:06] TheMue, after that I'll go out for a while, but see you at the meeting [09:07] fwereade: so it's a half swap-day [09:09] TheMue, whatever :) [09:09] fwereade: :D [10:54] fwereade: hippy barday to you! [10:55] fwereade: BTW AgentName isn't happening, but we're renaming PathKey to... something else. [10:55] happy birthday fwereade. [10:56] fwereade: my suggestion was Name, but that doesn't work because Service.Name already has implications. niemeyer suggested Key but that doesn't work because all kinds of things have keys. [10:56] fwereade: currently going with an earlier of niemeyer's suggestions: EntityName [10:57] fwereade: but i'm not greatly happy with that either - any suggestions welcome. [11:00] meeting in 1 min or 61 min? [11:00] ow [11:00] now [11:01] Morning! [11:01] team meeting hangout invite sent [11:01] https://plus.google.com/hangouts/_/557e6f4f5f9de98be4769ec91fc82db48a75d356?authuser=0&hl=en-GB [11:02] mramm: Thanks [11:03] fwereade: meeting... [11:17] rogpeppe: https://bugs.launchpad.net/mgo/+bug/1045678 [11:17] ^ may be of interest [11:19] niemeyer: --format=json [11:19] gnuargs only supports --format json [11:20] niemeyer: all: https://bugs.launchpad.net/juju-core/+bug/1059921 [11:36] davecheney: i'm happy to do the gnuflag change if you like [11:36] davecheney: i do bear responsibility for that package, after all [11:36] rogpeppe: if you want to take a crack over night, i would be most grateful [11:36] davecheney: ok [11:37] i also tried splitting on the = and pushing the remainder back into f.procArgs, which worked, but the testFlagsetParse tests were not happy [11:37] and I couldn't figure out why [11:37] davecheney, rogpeppe: What's the problem with storing the empty flag in procFlag? [11:37] davecheney, rogpeppe: Sorry, I think I meant procArgs [11:38] niemeyer: i'm not sure i understand the question [11:38] niemeyer: that would probably work, but TestFlagSetParse always failed for reasons I didn't understand [11:38] rogpeppe: What's the problem with doing the straightforward and just splitting --foo=bar and storing "bar" as usual? [11:38] niemeyer: that is what I tried first [11:39] pushing the remainder back into procArgs [11:39] but I couldn't make the test pass [11:39] niemeyer: what if bar is empty? [11:39] rogpeppe: What's the problem with that/ [11:39] ? [11:39] rogpeppe: --foo "" is fine [11:39] rogpeppe: and so is --foo= [11:39] dammit -- did I completely miss it? [11:39] fwereade: Happy birthday! [11:40] niemeyer, haha, cheers :) [11:40] :-) [11:40] fwereade: happy annual celebration! [11:40] niemeyer, went out for a nice casual brunch with cath and then got screwed by the busses on the way back :/ [11:40] niemeyer, the intent was there though :( [11:40] davecheney, cheers :) [11:41] fwereade: No worries really, it was a nice and quick sync up [11:41] niemeyer, excellent [11:41] niemeyer: i have to remember the code properly, gimme a few moments [11:42] rogpeppe: And btw, given your suggestion in the review, note that "--foo=bar" and "--foo =bar" mean different things [11:43] niemeyer: indeed, but i think that wouldn't be a problem if we did it the way i suggested [11:43] niemeyer: because we wouldn't put the latter argument into procFlag [11:45] rogpeppe: Cool [11:46] niemeyer: the main immediate problem with storing an empty procFlag is that i want to be able to do --foo=false for boolean flags. [11:47] niemeyer: while still allowing --foo to mean the same as --foo=true [11:50] rogpeppe: Sounds good, although I'm not familiar with the algorithm there to have an opinion [11:50] niemeyer: i'm nearly there. we'll see if the tests work :-) [11:51] rogpeppe: Super :) [11:51] * niemeyer gets some coffee [12:00] niemeyer, Aram: can we talk a little about relations and their creation, retrieval, and watchery? [12:00] sure [12:01] Aram, teh relation key is now a string with N service:relation pairs, in sort order, right? [12:01] yes [12:03] Aram, this is interesting from my POV because we identify relations to hooks in a not-quite-neatly-fitting way [12:04] it's also used in some watchers to identify that the relation belongs to a service. [12:04] Aram, as I see it we can either have an API that gets a relation by pre-existing ID number, rather than by key [12:05] Aram, which will be convenient for the hook tools [12:05] Aram, or we can try to do more interesting things with the key and do everything that way [12:06] Aram, niemeyer: and I'm rather hoping someone else has some thoughts on this because I can't tell which is better [12:06] so, the hook cares about the id of the relation? [12:07] niemeyer, eg, do we think that anyone is depending on any way on the format of the JUJU_RELATION env vars? [12:07] Aram, the hook needs to differentiate between different relations on the same endpoint [12:07] Aram, there's a JUJU_RELATION which is, say, "db", and a JUJU_RELATION_ID which is, say "db:7" [12:09] Aram, niemeyer: if we were to specify, instead, a JUJU_RELATION_ID of "db:otherservice:otherrelation" [12:09] Aram, niemeyer: I think it should be pretty simple to transform back and forth into real relation keys [12:10] Aram, niemeyer: which would resolve the icky two-Relation-getter-methods-on-State thing [12:12] niemeyer, Aram: I am then *very* torn on the issue of AddRelation and what params it should take [12:12] niemeyer, Aram: on the one hand, it's very convenient to be able to make up any old relations we like [12:13] niemeyer, Aram: and we make wanton use of this ability in the tests [12:14] niemeyer, Aram: (thought, derail atm: there's some always-on relation we should always be able to handle, right? for subordinates?) [12:15] niemeyer, Aram: on the other, it's pretty clearly a Bad Thing to be able to create any nonsensical relation we like, we ought to be validating them against the charms, just like we should be on upgrade [12:17] niemeyer, Aram, so my instinct is drawn towards a fuzzy-name interface for both AddRelation and Relation: ie it accepts things like "wordpress mysql" and figures out the right answer without ambiguity [12:17] niemeyer, Aram: but which mostly wants and expects real keys for normal usage [12:17] * niemeyer catches up [12:17] hmm? [12:17] how would it resolve the fuzzy naming? [12:17] niemeyer, Aram: do I sound like I'm on crack, and just making needless work? I *think* there's something important buried in there somewhere [12:18] Aram, not very fuzzy -- you have to specify the service names, but can leave the :relation bit off if there's no ambiguity [12:19] fwereade: No, I think you're on a good track [12:20] Aram, the point is just that it allows for a single clean API that includes appropriate validation for all uses [12:20] fwereade: I dislike a bit the way relations are referred to right now [12:20] Aram, it's definitely not a polished proposal just a feel that *something* is wrong [12:20] niemeyer, in what context? [12:20] fwereade: In general [12:21] niemeyer, they're represented too many ways already, I think ;) [12:21] fwereade: Oh, sory [12:21] sorry [12:21] fwereade: I mean the way we refer to them by a string that is a concatenation of the endpoints [12:22] fwereade: It ended up as a convenient way, though [12:22] niemeyer, ha, I've been coming to rather like that -- if it is (or is trivially transformable to/from) the relation "names" that the user types/sees, I think it is a good thing [12:22] fwereade: So, let me try to help on a few of your questions above [12:23] * fwereade sits attentively [12:23] fwereade: I don't think we should change the environment variable [12:23] fwereade: It's breakage that sounds easy to avoid [12:23] niemeyer, ok, wholeheartedly accept that argument [12:24] fwereade: It sounds fine to have a method that grabs the relation by its numeric id [12:25] fwereade: I'm not sure I get the problem you describe referring to AddRelation, though [12:26] niemeyer, a client of the API is perfectly able to AddRelation(, ) [12:26] niemeyer, and we will consider that a perfectly good relation, and join it, and just not run any hooks [12:27] niemeyer, IMO this is bad because if that relation *becomes* valid in future charms, to which we upgrade, we'll start running hooks in a weird weird state -- not installed, for a start [12:27] davecheney, niemeyer: https://codereview.appspot.com/6598052/ [12:27] now for some lunch [12:28] niemeyer, so, we should really just not be allowed to create them, just as we don't create services referring to charms that cdon't exist [12:28] fwereade: yeah, we should validate the relation, we didn't do so because we didn't have the emans when I've written that/ [12:28] s/emans/means/ [12:29] Aram, did we not? I thought charm.Meta was all we needed? [12:29] Aram, ok, but it's not just a matter of validating the endpoints [12:29] fwereade: Curious.. :-) [12:29] Aram, it's a matter of specifying them in the first place, in add-relation, which requires that we do the validation [12:29] fwereade: A bit of history.. https://codereview.appspot.com/6305067/ ;-) [12:30] niemeyer, ok, that is referring (I think) to a somewhat lower standard of validity [12:31] niemeyer, I think that in apprehending the charm-upgrade issue, my thoughts on this matter have taken a harder line [12:32] fwereade: Yeah, your thoughts on it are well appreciated [12:32] fwereade: So, hmm.. what prevents a relation from being added while an upgrade is in progress? [12:33] niemeyer, nothing yet [12:33] niemeyer, things are still changing under my feet enough that I don't have a clear picture of the final shape of the solution [12:33] niemeyer, I just know it's something that should be borne in mind [12:34] fwereade: We should certainly validate the relation.. just trying to imagine how to do it properly [12:37] niemeyer, I don't see any way around getting the charms' Metas [12:37] fwereade: We can add an assertion against the revision numbers of the involved services, I suppose [12:37] niemeyer, I think it's easier that that if we are careful with the lifetimes and with what upgrades we allow [12:37] niemeyer, block upgrades which would change non-Dead relations [12:37] fwereade: Yeah, this is the basic need.. the next question is which charm [12:38] fwereade: That's already an agreement I think, although we didn't put it in place yet [12:38] niemeyer, the current one of the service -- by blocking upgrades until incompatible relations are gone we can always use the service's current charm [12:38] fwereade: But besides that, we must also prevent adding a relation after the decision that it was ok to upgrade happens [12:40] niemeyer, it becomes a matter of figuring out what races we have to deal with between client 1 adding relations and client 2 upgrading charms [12:40] fwereade: Yeah, that's what I was referring to above [12:40] fwereade: We can add an assertion against the revision numbers of the involved services, I suppose [12:40] niemeyer, I am hoping that we will be able to express suitable asserts in the transactions [12:41] fwereade: To prevent adding a relation to a service that holds an unknown charm [12:41] niemeyer, yeah, exactly [12:41] fwereade: Cool, nothing sounds too bad so far [12:42] niemeyer, as long as we express the changes correctly on the client, we can find some way to keep not-yet-ok charms off to one side until the deployed charm can handle them [12:42] niemeyer, not really sure how to do that yet [12:43] niemeyer, doesn't feel likely to be a real dealbreaker [12:43] fwereade: You mean not-yet-ok relations? [12:43] * fwereade slaps self [12:43] niemeyer, yes, thank you [12:43] fwereade: Cool, np [12:44] fwereade: Yeah, agreed.. we have easy access to the current charm from the uniter [12:44] niemeyer, yeah, it feels quite neat [12:44] niemeyer, you had doubts about the appropriateness of the current "foo:bar baz:qux" key, though? [12:45] niemeyer, actually, that's in backward form :p [12:45] fwereade: I have a tiny dislike for it.. but it's not a big deal I think [12:46] niemeyer, I *think* that the ease of transformation to/from user-speak is very nice [12:46] fwereade: The only change I'll likely propose in the short term is to order the keys by role [12:46] niemeyer, +1 [12:47] fwereade: Otherwise we have "wordpress:db mysql:server" and "mysql:server blogger:foo" [12:47] Which is not great [12:48] niemeyer, requirer then provider? [12:49] fwereade: Yeah, that sounds sensible [13:03] ok then -- since the ids-in-watchers change, I have no way to progress on relations without working on some of this stuff [13:03] niemeyer, ^^ [13:05] niemeyer, I really don't think it is sensible for me to get sidetracked by the CLI-related bit when the uniter is still to be done, so I think I will just add a minimal RelationById(int) (*Relation, error) and change Relation() to take a key instead of endpoints [13:06] Aram, how are the watchers looking? [13:06] Aram, specifically ServiceRelationsWatcher :) [13:06] nothing done on that front, there's one in review you can look at, it will definetly look very similar. [13:07] Aram, great, I will * try* to check that out this evening [13:08] fwereade: Hmm [13:08] * fwereade listens [13:09] fwereade: Sorry, still pondering on the scheme you mention there [13:10] fwereade: Having the relation taking a key instead of endpoints doesn't feel great [13:11] fwereade: The way we have the current interface pretty consistent on that front feels like a nice win we have [13:13] niemeyer, hmm; I need to get relations both by key (because I presume that is how the watcher will deliver them) and by id (because that is how I express them to the hooks, which might pass them back to me) [13:14] niemeyer, getting relations, and watching relations, by key, seems natural to me [13:14] * rogpeppe had never realised the subtle difference in Gnu flags between -u=foo and --u=foo [13:15] niemeyer, the intent is that at some stage we can change over AddRelation to take a string, just as GetRelation does, that does the appropriate inference of unambiguously underspecified endpoints [13:15] niemeyer, and sorts appropriately, I guess [13:15] fwereade: :-) [13:15] fwereade: Effectively manipulating the key onto a different key [13:16] niemeyer, and thereby as a second step get back a unified API, that is backward-compatible with the stuff I've been doing in the uniter [13:16] niemeyer, ok, the heart of it is this [13:17] niemeyer, I need to look them up in 2 different ways, neither of which is the same as the current way to get a relation [13:17] fwereade: What is the real underlying needs you have? [13:17] niemeyer, if I just add two trivial RelationByKey and RelationById methods, I'm golden [13:18] fwereade: -1 on relation by key [13:18] fwereade: key is internal, and is a compilation of the current endpoint semantics [13:18] fwereade: I'm pretty sure we'll regret if we change it [13:18] niemeyer, ah -- I had thought we were loving away from internal keys that differed from external names [13:19] niemeyer, but, ok, if it's not the thing that we use to look things up by, and to send in watchers, it's not much of a key, is it? [13:19] fwereade: That's one of the reasons why I dislike the key mechanism. That said, I'm pretty sure exposing it as suggested will lead to many other issues that don't feel great to get into. [13:20] fwereade: You just mentioned one of them.. we cannot add a relation by its key, because we have to manipulate the key first [13:20] fwereade: It's a primary key used in the database.. there's no single place in our API that exposes this key [13:20] fwereade: It's an implementation detail [13:21] fwereade: To enable the API that we've agreed on so far [13:21] fwereade: We can kill the idea of using it as a key if we change the API [13:21] niemeyer, ok, then ID remains the primary way of identifying relations? [13:21] fwereade: It depends on the way you look at it [13:21] fwereade: how does our API look like? Are we using id as a primary way to identify relations? [13:21] niemeyer, ok: SRW will send IDs, and I will call a State method that takes an ID when I want to see what relation it refers to [13:22] niemeyer, I have assumed ID to be the main way, yes, but little committed code depends on this [13:23] fwereade: We can make the concept more first class [13:24] fwereade: For instance, state.Relation(id) sounds fine.. [13:25] fwereade: Feels a lot more sensible than obtaining all the endpoints to grab the relation [13:26] fwereade: We can then have e.g. state.EndpointsRelation(endpoints ...RelationEndpoint) (*Relation, error) [13:27] niemeyer, great, +1 [13:28] Aram, in the absence of other feedback, then, I will probably move on to a SRW that generates IDs at some point tomorrow; please let me know if you suspect collisions [13:28] Aram, I imagine I will implement it along the lines of your proposal [13:28] fwereade: This kills the inconsistency, I think, and makes state.Relation(relation.Id()) work, as usual for everything else [13:28] niemeyer, yeah, that is fine by me [13:29] fwereade: I don't anticipate any collisions, everythung should be smooth [13:29] niemeyer, and then we can have a state.InferEndpoints(eps ...string) ([]RelationEndpoint, error), that can be used by the CLI tools [13:29] fwereade: Yeah, that sounds great [13:30] niemeyer, and as a bonus we can keep the AddRelation logic as it is for now and do that properly in concert with the upgrady bits [13:30] fwereade: Cool [13:32] fwereade: Btw, there's a great benefit of having the DB _id being the key as it is today [13:32] fwereade: The service relations watcher doesn't have to load the document to tell what service it targets [13:33] niemeyer, +100 === TheMue_ is now known as TheMue [13:33] niemeyer, I *knew* there *was* something really good about it [13:34] fwereade: Yeah, that was kind of an unanticipated benefit.. the real reason we changed was the transaction mechanism [13:35] fwereade: Without this, we can't add a unique relation [13:35] fwereade: Because we don't have anything to assert on [13:35] niemeyer, very neat :) [13:35] fwereade: We can't assert on non-existence of something else that contains a given field [13:44] fwereade: Do you have a moment to talk about https://codereview.appspot.com/6588053/diff/2001/worker/uniter/modes.go#newcode171 [13:44] fwereade: happy to postpone if you're in flow on something else [13:47] :( [13:47] fwereade: Do you have a moment to talk about https://codereview.appspot.com/6588053/diff/2001/worker/uniter/modes.go#newcode171 [13:47] fwereade: happy to postpone if you're in flow on something else === niemeyer_ is now known as niemeyer [14:12] hmm, the current test in environs/ec2 in trunk has one fail and one panic. anybody working on that? [14:17] TheMue: What's the failure/panic? [14:18] Hmm.. something in the last two revisions broke it [14:18] niemeyer: Fail is NoSuchBucket in TearDownTest [14:18] Ah [14:18] That's Dave's change [14:19] niemeyer: Panic is in localServerSuite.TestBootstrapInstanceUserDataAndState [14:19] Let's see in the previous revision [14:26] TheMue: Proposing [14:33] niemeyer: Pardon? [14:36] TheMue: I've fixed it.. CL up in a moment [14:37] TheMue: https://codereview.appspot.com/6596056 [14:41] niemeyer: LGTM [14:41] niemeyer: PathKey->EntityName https://codereview.appspot.com/6593061/ [14:44] rogpeppe: LGTM [14:44] niemeyer: thanks! [14:47] niemeyer, I think I can for a mo :) [14:47] rogpeppe: np, and thanks [14:47] fwereade: Sorry, I just read your mail and didn't realize you were out before [14:48] niemeyer, nah, I was explicitly doing it because I realised I needed to and, meh, cath was out :) [14:48] niemeyer, while C&L are distracted together I'm more than happy to talk if it helps us progress [14:48] niemeyer, it just means I may disappear abruptly for hours at a time :) [14:48] fwereade: I was just going to mention that the issue there was simpler than it sounded [14:49] fwereade: I was questioning why we do the wantEvent+waitForConfig dance at all [14:49] fwereade: Rather than just running the hook directly [14:49] niemeyer, yeah -- as it turned out I really did have a reason ;) [14:49] niemeyer, not well expressed in the comments though ;) [14:50] niemeyer, oh yeah, I meant to ask about the resolved business [14:51] fwereade: Cool, I'm curious about why that is, but I can wait for the comment easily :) [14:52] niemeyer, in suggesting that no ResolvedNone event needs to be sent, I imagine that, to go with this, a wantResolved would just do nothing if the state was ResolvedNone [14:52] niemeyer, which makes "want" not quite right, maybe [14:52] niemeyer, or maybe "want" is just right :) [14:53] niemeyer, regardless, it would necessarily I think break the want-always-causes-a-send-as-soon-as-possible feature [14:53] niemeyer, the immediate consequences are generally good [14:53] niemeyer, but that property did seem like quite a nice one [14:54] fwereade: Yeah, a slight change in perspective.. it does send an event immediately, as long as the current state is sensible [14:54] niemeyer, ha, actually, the way I expressed it above, it *doesn't* break it [14:54] niemeyer, ok, if you've already considered it from that perspective I'm very happy indeed to do it that way [14:55] fwereade: aren't you supposed to be doing celebratory chores? [14:55] fwereade: Yeah, I wish we could do the same for upgrades actually [14:55] SpamapS, did some, now I can relax again ;p [14:56] niemeyer, upgrades aren't so simple, the upgradiness of a given service charm differs depending on mode [14:57] fwereade: You mean force vs. non-force? [14:59] niemeyer, and what it's to [14:59] niemeyer, X is not an interesting request when we're already upgrading to X even if it's forced [14:59] nie sorry gtg for a while again [14:59] fwereade: Please do go :) [15:32] * niemeyer => lunch [15:35] niemeyer: a final propose of https://codereview.appspot.com/6596051/ for the error message of illegal firewall modes === Aram2 is now known as Aram [15:59] niemeyer: any idea why this doesn't work? http://paste.ubuntu.com/1256219/ [16:00] rogpeppe: Because you're authenticating against the wrong database [16:00] niemeyer: ah, i wondered if it might be something like that [16:00] rogpeppe: The "/dbname" at the end of the URL does the trick [16:00] rogpeppe: That said, I don't think we want to use it like that, FWIW [16:00] rogpeppe: Login is a lot easier [16:00] niemeyer: i don't think so either [16:01] niemeyer: but i wanted something to verify that AddUser was actually doing something [16:01] niemeyer: and in fact, for non-SSL connections, perhaps we do want to do it this way [16:01] niemeyer: Login... hmm, i started doing it that way, then thought this was easier! [16:32] niemeyer: SetPassword in state: https://codereview.appspot.com/6587060/ [17:02] rogpeppe: Reviewed [17:02] niemeyer: thanks [17:03] rogpeppe: np [17:04] niemeyer: i'm not entirely sure how i can check that the presence database is authenticated. currently there's no difference between an authenticated and an unauthenticated connection AFAIK [17:05] rogpeppe: Perhaps we should fix that first then? [17:05] niemeyer: i'm not sure i can do that without running mongo in auth mode, which comes in the next CL [17:05] rogpeppe: Perhaps we should do that first? [17:06] niemeyer: hmm, yeah, we could do that [17:06] rogpeppe: As it is the CL has a bug that went unperceived because we had no tests.. feels like a good reason to step back [17:06] niemeyer: i'm not entirely sure i understand how the auth mode thing works - the mongodb docs seem pretty sketchy [17:07] niemeyer: i don't think the bug comment was published [17:07] rogpeppe: ? [17:07] niemeyer: i don't see any comment in your review that points out a bug [17:08] niemeyer: (apart from "Just needs a test to ensure the bug is fixed") [17:08] rogpeppe: Try to add the test then :-) [17:08] niemeyer: ha [17:08] niemeyer: i just looked at the comment in context :-) [17:09] rogpeppe: In terms of --auth, just running with --auth enables secure mode.. it allows connecting locally to the database before a user is added [17:09] rogpeppe: Which is quite handy for what we want [17:10] niemeyer: ok, i'll make another CL that runs the testing mongodb server in auth mode. [17:10] niemeyer: once a user is added, does it stop any further unauthenticated local connections? [17:10] rogpeppe: I think all we'd need is running the test mgo in auth mode, and use SetAdminPassword on it [17:10] rogpeppe: Right [17:11] niemeyer: i don't see a SetAdminPassword method in mgo [17:12] rogpeppe: That's about juju :-) [17:12] niemeyer: but we can add a user in state.Initialize [17:12] niemeyer: i'd thought we might add a password argument to state.Initialize [17:12] rogpeppe: and then how do we connect? [17:13] rogpeppe: Well, sorry, that's irrelevant [17:13] niemeyer: with Open as usual, no?: [17:13] rogpeppe: Yeah [17:13] rogpeppe: Either way, Initialize can be made in terms of SetAdminPassword.. [17:13] niemeyer: seems reasonable [17:14] niemeyer: BTW in environs/cloudinit, i'm planning to write the entity name and password to $dataDir/admin/mongodb-auth [17:14] niemeyer: then the agents will read that and put the info into the state.Info before connecting [17:15] niemeyer: i've gotta go and cook up some wild mushrooms for tea :-) [17:16] rogpeppe: Sounds fine, except a "/auth" file sounds enough. We need to consider how to handle the multiple agents too. $dataDir is shared, IIRC [17:17] niemeyer: ok. i wondered if there might be a problem with people reading the length of the file, given that $dataDir is globally readable. [17:18] niemeyer: for multiple agents, i'd thought we'd use agents/$entityname/auth [17:19] rogpeppe: Not sure.. agents/ was going away, I think.. we'll have containers/ instead IIRC [17:19] niemeyer: i think i'll fold in the Initialize changes into the current CL otherwise we've got a small chicken/egg problem . [17:19] rogpeppe: We must also think about how to avoid having the admin password exposed.. [17:20] niemeyer: well, the same directory that's used for the agent anyway [17:20] rogpeppe: I don't see the chicken/egg problem [17:20] niemeyer: i think it's got to sit in a file. as long as it's in a 700 file in a 700 directory, i don't think there's a problem [17:21] rogpeppe: I mean how to get the admin password to be set in a way that doesn't leak it [17:21] rogpeppe: and at the same time is known by the client [17:21] niemeyer: Initialize relies on SetPassword (well SetAdminPassword which is almost exactly the same thing), and that's what's in the current CL [17:21] rogpeppe: That's how we end up with big unfocused branches [17:22] rogpeppe: It doesn't really on unit.SetPassword or machine.SetPassword [17:22] rely [17:22] niemeyer: yes, but the testing will be exactly the same (they both share tests) [17:22] niemeyer: as will the implementation [17:23] niemeyer: and we'll need the changes from the current CL that do the login too [17:23] rogpeppe: What happens if you login with an invalid user? [17:23] niemeyer: you get an "auth failed" error [17:24] rogpeppe: s.Session.Login("admin", "foo") [17:24] rogpeppe: This means one can trivially implement SetAdminPassword, and test it [17:25] niemeyer: ok, so that's stage 1. the next stage needs to have all the other login stuff in. and that's the bulk of it. SetAdminPassword is about two lines of code. [17:25]