Merge lp://staging/~hduran-8/juju-core/apiworker_force_local_connection into lp://staging/~go-bot/juju-core/trunk

Reviewers: mp+221221_code.launchpad.net,

Message:
Please take a look.

Description:
APIWorker should only connect to API on localhost

In some cases, APIWorker might try to connect
to a different address than localhost in machines
with the ManageEnviron job, in those cases we
change the api hostname from whatever is set
to localhost and keep the port.

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

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/100810045/

Affected files (+83, -5 lines):
   A [revision details]
   M cmd/jujud/agent.go
   M cmd/jujud/agent_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140519143554-fd3fvpqfvhm5u642
+New revision: <email address hidden>

Index: cmd/jujud/agent.go
=== modified file 'cmd/jujud/agent.go'
--- cmd/jujud/agent.go 2014-05-14 21:13:17 +0000
+++ cmd/jujud/agent.go 2014-05-28 19:00:09 +0000
@@ -6,7 +6,9 @@
  import (
   "fmt"
   "io"
+ "net"
   "path/filepath"
+ "strconv"
   "sync"
   "time"

@@ -191,6 +193,25 @@

  type configChanger func(c *agent.Config)

+func pickBestHosts(apiInfo *api.Info, jobs []params.MachineJob) error {
+ for _, job := range jobs {
+ if job == params.JobManageEnviron {
+ firstAddr := apiInfo.Addrs[0]
+ _, port, err := net.SplitHostPort(firstAddr)
+ if err != nil {
+ return err
+ }
+ portNum, err := strconv.Atoi(port)
+ if err != nil {
+ return fmt.Errorf("bad port number %q", port)
+ }
+ apiInfo.Addrs = []string{fmt.Sprintf("localhost:%d", portNum)}
+ break
+ }
+ }
+ return nil
+}
+
  // openAPIState opens the API using the given information, and
  // returns the opened state and the api entity with
  // the given tag. The given changeConfig function is
@@ -205,6 +226,12 @@
   // then the worker that's calling this cannot
   // be interrupted.
   info := agentConfig.APIInfo()
+ // Ensure that we conect trough localhost
+ agentConfigJobs := agentConfig.Jobs()
+ if err := pickBestHosts(info, agentConfigJobs); err != nil {
+ return nil, nil, err
+ }
+
   st, err := apiOpen(info, api.DialOpts{})
   usedOldPassword := false
   if params.IsCodeUnauthorized(err) {

Index: cmd/jujud/agent_test.go
=== modified file 'cmd/jujud/agent_test.go'
--- cmd/jujud/agent_test.go 2014-04-30 23:18:40 +0000
+++ cmd/jujud/agent_test.go 2014-05-28 19:02:43 +0000
@@ -105,17 +105,23 @@
   return "old"
  }

+func (fakeAPIOpenConfig) Jobs() []params.MachineJob {
+ return []params.MachineJob{}
+}
+
  var _ = gc.Suite(&apiOpenSuite{})

+type replaceErrors struct {
+ openErr error
+ replaceErr error
+}
+
  func (s *apiOpenSuite) TestOpenAPIStateReplaceErrors(c *gc.C) {
   var apiError error
   s.PatchValue(&apiOpen, func(info *api.Info, opts api.DialOpts)
(*api.State, error) {
    return nil, apiError
   })
- for i, test := range []struct {
- openErr error
- replaceErr error
- }{{
+ errReplacePairs := []replaceErrors{{
    fmt.Errorf("blah"), nil,
   }, {
    openErr: &p...

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#newcode114
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#newcode362
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".

https://codereview.appspot.com/100810045/

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#newcode196
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#newcode233
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?

https://codereview.appspot.com/100810045/

Please take a look.

https://codereview.appspot.com/100810045/

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#newcode461
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#newcode112
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.go#newcode76
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.

https://codereview.appspot.com/100810045/

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#newcode111
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))

https://codereview.appspot.com/100810045/

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#newcode111
> 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?

https://codereview.appspot.com/100810045/