Code review comment for lp:~hduran-8/juju-core/apiworker_force_local_connection

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: &params.Error{Code: params.CodeNotProvisioned},
@@ -123,7 +129,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)
@@ -352,6 +359,48 @@
   c.Assert(err, gc.IsNil)
  }

+type providedExpectedAddresses struct {
+ provided string
+ expected string
+ jobs []params.MachineJob
+}
+
+func (s *agentSuite) TestProperAddressChosenForManageEnviron(c *gc.C) {
+ apiInfo := api.Info{}
+ addresses := []providedExpectedAddresses{{
+ "changeThisAddress:12345",
+ "localhost:12345",
+ []params.MachineJob{params.JobManageEnviron},
+ }, {
+ "dontChangeThisAddress:12345",
+ "dontChangeThisAddress:12345",
+ []params.MachineJob{},
+ },
+ }
+ for _, eachDataSet := range addresses {
+ apiInfo.Addrs = []string{eachDataSet.provided}
+ err := pickBestHosts(&apiInfo, eachDataSet.jobs)
+ c.Assert(err, gc.IsNil)
+ c.Assert(apiInfo.Addrs[0], gc.Equals, eachDataSet.expected)
+ }
+}
+
+func (s *agentSuite) TestBestHostPickerFailsWithIncompleteAddress(c *gc.C)
{
+ apiInfo := api.Info{}
+ apiInfo.Addrs = []string{"this is an invalid hostname"}
+ err := pickBestHosts(&apiInfo,
[]params.MachineJob{params.JobManageEnviron})
+ c.Check(apiInfo.Addrs[0], gc.Equals, "this is an invalid hostname")
+ c.Assert(err, gc.ErrorMatches, "missing port in address this is an
invalid hostname")
+}
+
+func (s *agentSuite) TestBestHostPickerFailsWithMalformedPort(c *gc.C) {
+ apiInfo := api.Info{}
+ apiInfo.Addrs = []string{"localhost:invalidport"}
+ err := pickBestHosts(&apiInfo,
[]params.MachineJob{params.JobManageEnviron})
+ c.Check(apiInfo.Addrs[0], gc.Equals, "localhost:invalidport")
+ c.Assert(err, gc.ErrorMatches, "bad port number \"invalidport\"")
+}
+
  func (s *agentSuite) testOpenAPIState(c *gc.C, ent state.AgentEntity,
agentCmd Agent, initialPassword string) {
   conf, err := agent.ReadConfig(agent.ConfigPath(s.DataDir(), ent.Tag()))
   c.Assert(err, gc.IsNil)