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

[Horacio Durán]

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, to achieve 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 api.Open now
sorts the Addrs list before trying to connect
to make sure local ones will be tried first.

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

R=natefinch

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://codereview.appspot.com/100810045/

[Horacio Durán (hduran-8)]

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...

[Nate Finch (natefinch)]

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/

[William Reade (fwereade)]

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/

[Horacio Durán (hduran-8)]

Please take a look.

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

[Nate Finch (natefinch)]

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/

[Wayne Witzel III (wwitzel3)]

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/

[John A Meinel (jameinel)]

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/

=== modified file 'agent/agent.go'
--- agent/agent.go	2014-05-13 04:50:10 +0000
+++ agent/agent.go	2014-05-30 16:13:04 +0000
@@ -605,8 +605,24 @@
 }
 
 func (c *configInternal) APIInfo() *api.Info {
+	servingInfo, isStateServer := c.StateServingInfo()
+	addrs := c.apiDetails.addresses
+	if isStateServer {
+		port := servingInfo.APIPort
+		localApiAddr := fmt.Sprintf("localhost:%d", port)
+		addrInAddrs := false
+		for _, addr := range addrs {
+			if addr == localApiAddr {
+				addrInAddrs = true
+				break
+			}
+		}
+		if !addrInAddrs {
+			addrs = append(addrs, localApiAddr)
+		}
+	}
 	return &api.Info{
-		Addrs:    c.apiDetails.addresses,
+		Addrs:    addrs,
 		Password: c.apiDetails.password,
 		CACert:   c.caCert,
 		Tag:      c.tag,

=== modified file 'agent/agent_test.go'
--- agent/agent_test.go	2014-05-20 04:27:02 +0000
+++ agent/agent_test.go	2014-05-30 16:13:04 +0000
@@ -437,13 +437,45 @@
 	c.Assert(reread, jc.DeepEquals, conf)
 }
 
+func (*suite) TestAPIInfoAddsLocalhostWhenServingInfoPesent(c *gc.C) {
+	attrParams := attributeParams
+	servingInfo := params.StateServingInfo{
+		Cert:           "old cert",
+		PrivateKey:     "old key",
+		StatePort:      69,
+		APIPort:        1492,
+		SharedSecret:   "shared",
+		SystemIdentity: "identity",
+	}
+	conf, err := agent.NewStateMachineConfig(attrParams, servingInfo)
+	c.Assert(err, gc.IsNil)
+	apiinfo := conf.APIInfo()
+	c.Check(apiinfo.Addrs, gc.HasLen, len(attrParams.APIAddresses)+1)
+	localhostAddressFound := false
+	for _, eachApiAddress := range apiinfo.Addrs {
+		if eachApiAddress == "localhost:1492" {
+			localhostAddressFound = true
+			break
+		}
+	}
+	c.Assert(localhostAddressFound, jc.IsTrue)
+}
+
+func (*suite) TestAPIInfoDoesntAddLocalhostWhenNoServingInfo(c *gc.C) {
+	attrParams := attributeParams
+	conf, err := agent.NewAgentConfig(attrParams)
+	c.Assert(err, gc.IsNil)
+	apiinfo := conf.APIInfo()
+	c.Assert(apiinfo.Addrs, gc.DeepEquals, attrParams.APIAddresses)
+}
+
 func (*suite) TestSetPassword(c *gc.C) {
 	attrParams := attributeParams
 	servingInfo := params.StateServingInfo{
 		Cert:           "old cert",
 		PrivateKey:     "old key",
-		StatePort:      69,
-		APIPort:        47,
+		StatePort:      1234,
+		APIPort:        1235,
 		SharedSecret:   "shared",
 		SystemIdentity: "identity",
 	}

=== modified file 'cmd/jujud/agent_test.go'
--- cmd/jujud/agent_test.go	2014-05-28 10:56:55 +0000
+++ cmd/jujud/agent_test.go	2014-05-30 16:13:04 +0000
@@ -104,17 +104,22 @@
 	return "old"
 }
 
+func (fakeAPIOpenConfig) Jobs() []params.MachineJob {
+	return []params.MachineJob{}
+}
+
 var _ = gc.Suite(&apiOpenSuite{})
 
 func (s *apiOpenSuite) TestOpenAPIStateReplaceErrors(c *gc.C) {
+	type replaceErrors struct {
+		openErr    error
+		replaceErr error
+	}
 	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:    &params.Error{Code: params.CodeNotProvisioned},
@@ -122,7 +127,8 @@
 	}, {
 		openErr:    &params.Error{Code: params.CodeUnauthorized},
 		replaceErr: worker.ErrTerminateAgent,
-	}} {
+	}}
+	for i, test := range errReplacePairs {
 		c.Logf("test %d", i)
 		apiError = test.openErr
 		_, _, err := openAPIState(fakeAPIOpenConfig{}, nil)

=== modified file 'state/api/apiclient.go'
--- state/api/apiclient.go	2014-04-30 23:18:40 +0000
+++ state/api/apiclient.go	2014-05-30 16:13:04 +0000
@@ -8,6 +8,8 @@
 	"crypto/x509"
 	"fmt"
 	"io"
+	"sort"
+	"strings"
 	"time"
 
 	"code.google.com/p/go.net/websocket"
@@ -108,6 +110,18 @@
 	}
 }
 
+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 (l LocalFirst) Less(i, j int) bool {
+	return strings.HasPrefix(l[i], "localhost") && !strings.HasPrefix(l[j], "localhost")
+}
+
 func Open(info *Info, opts DialOpts) (*State, error) {
 	if len(info.Addrs) == 0 {
 		return nil, fmt.Errorf("no API addresses to connect to")
@@ -122,7 +136,10 @@
 	// Dial all addresses at reasonable intervals.
 	try := parallel.NewTry(0, nil)
 	defer try.Kill()
-	for _, addr := range info.Addrs {
+	var addrs []string
+	addrs = append(addrs, info.Addrs...)
+	sort.Sort(LocalFirst(addrs))
+	for _, addr := range addrs {
 		err := dialWebsocket(addr, opts, pool, try)
 		if err == parallel.ErrStopped {
 			break

=== modified file 'state/api/apiclient_test.go'
--- state/api/apiclient_test.go	2014-04-02 09:43:39 +0000
+++ state/api/apiclient_test.go	2014-05-30 16:13:04 +0000
@@ -6,6 +6,7 @@
 import (
 	"io"
 	"net"
+	"sort"
 
 	gc "launchpad.net/gocheck"
 
@@ -20,6 +21,85 @@
 
 var _ = gc.Suite(&apiclientSuite{})
 
+func (s *apiclientSuite) TestSortLocalhost(c *gc.C) {
+	addrs := []string{
+		"notlocalhost1",
+		"notlocalhost2",
+		"notlocalhost3",
+		"localhost1",
+		"localhost2",
+		"localhost3",
+	}
+	expectedAddrs := []string{
+		"localhost1",
+		"localhost2",
+		"localhost3",
+		"notlocalhost1",
+		"notlocalhost2",
+		"notlocalhost3",
+	}
+	var sortedAddrs []string
+	sortedAddrs = append(sortedAddrs, addrs...)
+	sort.Sort(api.LocalFirst(sortedAddrs))
+	c.Assert(addrs, gc.Not(gc.DeepEquals), sortedAddrs)
+	c.Assert(sortedAddrs, gc.HasLen, 6)
+	c.Assert(sortedAddrs, gc.DeepEquals, expectedAddrs)
+
+}
+
+func (s *apiclientSuite) TestSortLocalhostIdempotent(c *gc.C) {
+	addrs := []string{
+		"localhost1",
+		"localhost2",
+		"localhost3",
+		"notlocalhost1",
+		"notlocalhost2",
+		"notlocalhost3",
+	}
+	expectedAddrs := []string{
+		"localhost1",
+		"localhost2",
+		"localhost3",
+		"notlocalhost1",
+		"notlocalhost2",
+		"notlocalhost3",
+	}
+	var sortedAddrs []string
+	sortedAddrs = append(sortedAddrs, addrs...)
+	sort.Sort(api.LocalFirst(sortedAddrs))
+	c.Assert(sortedAddrs, gc.DeepEquals, expectedAddrs)
+
+}
+
+func (s *apiclientSuite) TestOpenPrefersLocalhostIfPresent(c *gc.C) {
+	// Create a socket that proxies to the API server though our localhost address.
+	info := s.APIInfo(c)
+	serverAddr := info.Addrs[0]
+	server, err := net.Dial("tcp", serverAddr)
+	c.Assert(err, gc.IsNil)
+	defer server.Close()
+	listener, err := net.Listen("tcp", "localhost:26104")
+	c.Assert(err, gc.IsNil)
+	defer listener.Close()
+	go func() {
+		for {
+			client, err := listener.Accept()
+			if err != nil {
+				return
+			}
+			go io.Copy(client, server)
+			go io.Copy(server, client)
+		}
+	}()
+
+	// Check that we are using our working address to connect
+	info.Addrs = []string{"fakeAddress:1", "fakeAddress:1", "localhost:26104"}
+	st, err := api.Open(info, api.DialOpts{})
+	c.Assert(err, gc.IsNil)
+	defer st.Close()
+	c.Assert(st.Addr(), gc.Equals, "localhost:26104")
+}
+
 func (s *apiclientSuite) TestOpenMultiple(c *gc.C) {
 	// Create a socket that proxies to the API server.
 	info := s.APIInfo(c)