APIWorker should only connect to API on localhost

APIWorker should only connect to API on localhost

In some cases, APIWorker might try to connect
to a different address than localhost in State 
Servers, for this APIInfo was modified to add
localhost with the configured StateServerPort
to the Addrs list if not present. To make sure
localhost option will be picked first Open now
sorts the Addrs list before trying to connect
to make sure local ones will be tried first.

https://code.launchpad.net/~hduran-8/juju-core/apiworker_force_local_connection/+merge/221221

(do not edit description out of merge proposal)

[hduran, 2014-05-28 19:06:50]

Please take a look.

[natefinch, 2014-05-28 20:19:18]

I think we need to test that we're actually passing the results of the address
picker into apiOpen.

https://codereview.appspot.com/100810045/diff/20001/cmd/jujud/agent_test.go
File cmd/jujud/agent_test.go (right):

https://codereview.appspot.com/100810045/diff/20001/cmd/jujud/agent_test.go#n...
cmd/jujud/agent_test.go:114: type replaceErrors struct {
You can put this type definition right in the test function, and I think you
should :)

https://codereview.appspot.com/100810045/diff/20001/cmd/jujud/agent_test.go#n...
cmd/jujud/agent_test.go:362: type providedExpectedAddresses struct {
move this into the test, and you can give it a less long name.

Also, please give each test a unique description and log that description inside
the test's loop, that way it's clear what test fails (otherwise there's no way
to know which of the tests in the loop failed).  It can be something like "State
machine should always use localhost" and "non-state machine should use the
address given".

[fwereade, 2014-05-28 20:33:11]

Forgive the pontification, but I think this demands a bit more thought. As it is
it feel like just slapping another layer on top to cover the deficiencies below,
and that rarely ends well...

https://codereview.appspot.com/100810045/diff/20001/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/100810045/diff/20001/cmd/jujud/agent.go#newcod...
cmd/jujud/agent.go:196: func pickBestHosts(apiInfo *api.Info, jobs
[]params.MachineJob) error {
On general principles, I think I'd prefer it if we copied and returned a new
*api.Info, rather than mutating the one we passed in. It might be the case that
nobody else has a reference to this one, but then it also might not ;).

https://codereview.appspot.com/100810045/diff/20001/cmd/jujud/agent.go#newcod...
cmd/jujud/agent.go:233: }
Hmm. This feels like a band-aid-style solution... I'd prefer to record the
addresses we mean to connect to in the first place.

But then that involves throwing away information we'd need if we were ever
demoted, so it's probably not viable. And we can't have state servers reject API
connections from other state servers, or how will we ever promote a machine to a
state server? Bah.

However... apiOpen *does* try different servers if it can't connect, right? What
would you think of the combination of:

1) APIInfo() uses the presence of a StateServingInfo to *also* return
localhost:port-this-machine-is-configured-to-serve-the-api-on (so we don't ever
actually lie, but we do return what's likely to be the best address to use.)

...and...

2) apiOpen (or the layer below) prioritises localhost: addresses (which STM like
a good idea anyway..?)

..?

The other option would be to have the API call that returns state server
addresses figure out the correct addresses from the POV of the affected entity,
and return localhost where appropriate in the first place. I think that'd be
best, but I'm not sure it's best enough to be worth the effort. I *am* really
bothered about encoding the one-api-port-only assumption in this snippet of
code, though -- picking an (effectively) random address from which to take the
port feels all kinds of wrong.

Thoughts?

[hduran, 2014-05-30 02:03:01]

Please take a look.

[natefinch, 2014-05-30 14:12:29]

LGTM with a couple minor modifications.

https://codereview.appspot.com/100810045/diff/40001/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/100810045/diff/40001/agent/agent.go#newcode615
agent/agent.go:615: if eachAddress == localApiAddr {
I don't really like this naming convention.  I get that you're making it "for
eachAddress in addresses"... but then the if statement looks like it's saying
"if each address equal local"  which in English means "if *all* addresses equal
local".  Plus, the variable is used directly under where it is declared, so it's
not like you need a long variable name to remember that this is one item in the
list.

I'd prefer

for _, addr := range addrs {
    if addr == localApiAddr {

each here doesn't really add anything, except give me more words to look at and
try to understand.

https://codereview.appspot.com/100810045/diff/40001/agent/agent_test.go
File agent/agent_test.go (right):

https://codereview.appspot.com/100810045/diff/40001/agent/agent_test.go#newco...
agent/agent_test.go:461: c.Assert(localhostAddressFound, gc.Equals, true)
There's a jc.IsTrue, by the way.

https://codereview.appspot.com/100810045/diff/40001/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/100810045/diff/40001/state/api/apiclient.go#ne...
state/api/apiclient.go:112: func SortLocalHostFirst(addrs []string) []string {
you should just implement a sort type:

type localFirst []string
func (l localFirst) Len() int {
    return len(l)
}
func (l LocalFirst) Swap(i, j int) {
    l[i], l[j] = l[j], l[i]
}
func Less(i, j int) bool {
    return strings.HasPrefix(l[i])
}

then you can just do

addrs := sort.Sort(localFirst(info.Addrs))

And that way I don't have to debug your sorting algorithm ;)

https://codereview.appspot.com/100810045/diff/40001/state/api/apiclient_test.go
File state/api/apiclient_test.go (right):

https://codereview.appspot.com/100810045/diff/40001/state/api/apiclient_test....
state/api/apiclient_test.go:76: listener, err := net.Listen("tcp",
"localhost:27017")
Don't use the same port as built in mongo port, since that may conflict with
mongo if it's running.

[wwitzel3, 2014-05-30 14:12:32]

https://codereview.appspot.com/100810045/diff/40001/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/100810045/diff/40001/state/api/apiclient.go#ne...
state/api/apiclient.go:111: 
We should use the built-in sort here.

http://golang.org/pkg/sort/

type LocalhostFirst []string

func (a LocalhostFirst) Len() int { return len(a) }
func (a LocalhostFirst) Swap(i int, j int) { a[i], a[j] = a[j], a[i] }
func (a LocalhostFirst) Less(i int, j int) bool { return strings.HasPrefix(a[i],
"localhost") }

return sort.Sort(LocalhostFirst(addrs))

[jameinel, 2014-06-01 10:00:53]

On 2014/05/30 14:12:32, wwitzel3 wrote:
> https://codereview.appspot.com/100810045/diff/40001/state/api/apiclient.go
> File state/api/apiclient.go (right):
> 
>
https://codereview.appspot.com/100810045/diff/40001/state/api/apiclient.go#ne...
> state/api/apiclient.go:111: 
> We should use the built-in sort here.
> 
> http://golang.org/pkg/sort/
> 
> type LocalhostFirst []string
> 
> func (a LocalhostFirst) Len() int { return len(a) }
> func (a LocalhostFirst) Swap(i int, j int) { a[i], a[j] = a[j], a[i] }
> func (a LocalhostFirst) Less(i int, j int) bool { return
strings.HasPrefix(a[i],
> "localhost") }
> 
> return sort.Sort(LocalhostFirst(addrs))

So I think the sort.Sort is actually incorrect, having worked in this code a
bit.

Specifically, this breaks the order for *everything else*. And we intentionally
move the last-successful-connection to the beginning of the list, so the order
isn't random.

So for things like EC2, where you have public and private (etc) addresses, this
now essentially randomizes which one we are going to try to connect to first.
(Actually, it probably sorts the private addresses first because 10. comes
before 172.).

I can see the point of wanting to try localhost first, and in HA mode we really
do want localhost. However, I think what we *really* want is to check "if
JobManageEnviron" then *only* connect to localhost, right?