Merge lp:~thumper/juju-core/make-syslog-port-configurable into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Make the syslog udp port configurable.

Instead of having the syslog port hard-coded to 514 it makes
sense to make this configurable.

Any existing config that doesn't specify one will get the default,
which is 514.

This config port is passed through the cloud init machine config
structure so the syslog config that is written out forwards to the
correct port. This syslog port is needed, so a number of tests had
to be updated to make this work.

The (rather generic) 'suite' test in environs/config_test was updated
to use the LoggingSuite base to silence the logging that was leaked
out when running just the package tests.

The port number is now a template param in the rsyslog config.

The only other place where this was needed was with the deployer.
It was making multiple calls to get the state and api addresses.
Instead of adding another call, we now make one call to the api
to get the address information we need.

https://codereview.appspot.com/29990045/

Make the syslog udp port configurable.

Instead of having the syslog port hard-coded to 514 it makes
sense to make this configurable.

Any existing config that doesn't specify one will get the default,
which is 514.

This config port is passed through the cloud init machine config
structure so the syslog config that is written out forwards to the
correct port. This syslog port is needed, so a number of tests had
to be updated to make this work.

The (rather generic) 'suite' test in environs/config_test was updated
to use the LoggingSuite base to silence the logging that was leaked
out when running just the package tests.

The port number is now a template param in the rsyslog config.

The only other place where this was needed was with the deployer.
It was making multiple calls to get the state and api addresses.
Instead of adding another call, we now make one call to the api
to get the address information we need.

https://codereview.appspot.com/29990045/

[Tim Penhey (thumper)]

Reviewers: mp+196046_code.launchpad.net,

Message:
Please take a look.

Description:
Make the syslog udp port configurable.

Instead of having the syslog port hard-coded to 514 it makes
sense to make this configurable.

Any existing config that doesn't specify one will get the default,
which is 514.

This config port is passed through the cloud init machine config
structure so the syslog config that is written out forwards to the
correct port. This syslog port is needed, so a number of tests had
to be updated to make this work.

The (rather generic) 'suite' test in environs/config_test was updated
to use the LoggingSuite base to silence the logging that was leaked
out when running just the package tests.

The port number is now a template param in the rsyslog config.

The only other place where this was needed was with the deployer.
It was making multiple calls to get the state and api addresses.
Instead of adding another call, we now make one call to the api
to get the address information we need.

https://code.launchpad.net/~thumper/juju-core/make-syslog-port-configurable/+merge/196046

(do not edit description out of merge proposal)

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

[Andrew Wilkins (axwalk)]

I see the value of a single round-trip to the API, but it seems a bit
ugly. I think someone with a better idea about the API design will have
to weigh in.

https://codereview.appspot.com/29990045/diff/1/log/syslog/config.go
File log/syslog/config.go (right):

https://codereview.appspot.com/29990045/diff/1/log/syslog/config.go#newcode135
log/syslog/config.go:135:
fmt.Sprint(slConfig.Port)

https://codereview.appspot.com/29990045/diff/1/state/address.go
File state/address.go (right):

https://codereview.appspot.com/29990045/diff/1/state/address.go#newcode125
state/address.go:125:
Difficult to parse, IMHO. I'd prefer
     stateAddresses = appendPort(addrs, config.StatePort())
     apiAddresses = appendPort(addrs, config.APIPort())
     return stateAddresses, apiAddresses, config.SyslogPort(), nil

https://codereview.appspot.com/29990045/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/29990045/diff/1/state/api/params/params.go#newcode526
state/api/params/params.go:526:
I know I'm being pedantic, but a port isn't exactly an address. Multiple
API calls isn't great, though. It would be nice if we could make bulk
calls of an arbitrary sequence of API methods.

https://codereview.appspot.com/29990045/diff/1/worker/deployer/addresser.go
File worker/deployer/addresser.go (right):

https://codereview.appspot.com/29990045/diff/1/worker/deployer/addresser.go#newcode15
worker/deployer/addresser.go:15:
This seems like the wrong place to be talking about the API. This should
just be about domain objects, I think.

https://codereview.appspot.com/29990045/

[Ian Booth (wallyworld)]

Big +1 to using bulk api calls which reflect the distributed workflow.
Out current api design is way too chatty since it seems to simply add a
remoting layer directly on the top of a domain api. So even though there
are quibbles about naming and packaging of results, this is great. The
main issue I have is that the api params structs should not be used
external to the api.

https://codereview.appspot.com/29990045/diff/1/state/address.go
File state/address.go (right):

https://codereview.appspot.com/29990045/diff/1/state/address.go#newcode116
state/address.go:116:
I think it's worth making this function return a struct.
Since we are now returning a hodge podge of values, perhaps a better
method name is required
eg DeployerAddressInfo or DeployerConnectionInfo

I also think it may be worth naming this method to be the same as the
one on state, which is currently ServerAddresses().

https://codereview.appspot.com/29990045/diff/1/worker/deployer/addresser.go
File worker/deployer/addresser.go (right):

https://codereview.appspot.com/29990045/diff/1/worker/deployer/addresser.go#newcode15
worker/deployer/addresser.go:15:
On 2013/11/21 05:17:42, axw wrote:
> This seems like the wrong place to be talking about the API. This
should just be
> about domain objects, I think.

+1
params.ServerAddressesResults does not belong here. No api params
structs should escape the api infrastructure

https://codereview.appspot.com/29990045/

[Tim Penhey (thumper)]

I'm not yet pushing up changes because I don't want to write more code
that is going to get argued over until we have a consensus on api/params
return types and the interface name.

https://codereview.appspot.com/29990045/diff/1/log/syslog/config.go
File log/syslog/config.go (right):

https://codereview.appspot.com/29990045/diff/1/log/syslog/config.go#newcode135
log/syslog/config.go:135:
On 2013/11/21 05:17:42, axw wrote:
> fmt.Sprint(slConfig.Port)

Done.

https://codereview.appspot.com/29990045/diff/1/state/address.go
File state/address.go (right):

https://codereview.appspot.com/29990045/diff/1/state/address.go#newcode116
state/address.go:116:
On 2013/11/22 00:46:52, wallyworld wrote:
> I think it's worth making this function return a struct.
> Since we are now returning a hodge podge of values, perhaps a better
method name
> is required
> eg DeployerAddressInfo or DeployerConnectionInfo

> I also think it may be worth naming this method to be the same as the
one on
> state, which is currently ServerAddresses().

Done.

https://codereview.appspot.com/29990045/diff/1/state/address.go#newcode125
state/address.go:125:
On 2013/11/21 05:17:42, axw wrote:
> Difficult to parse, IMHO. I'd prefer
> stateAddresses = appendPort(addrs, config.StatePort())
> apiAddresses = appendPort(addrs, config.APIPort())
> return stateAddresses, apiAddresses, config.SyslogPort(), nil

Now this returns a struct, it is more obvious.

https://codereview.appspot.com/29990045/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/29990045/diff/1/state/api/params/params.go#newcode526
state/api/params/params.go:526:
On 2013/11/21 05:17:42, axw wrote:
> I know I'm being pedantic, but a port isn't exactly an address.
Multiple API
> calls isn't great, though. It would be nice if we could make bulk
calls of an
> arbitrary sequence of API methods.

Changed struct name.

https://codereview.appspot.com/29990045/diff/1/worker/deployer/addresser.go
File worker/deployer/addresser.go (right):

https://codereview.appspot.com/29990045/diff/1/worker/deployer/addresser.go#newcode15
worker/deployer/addresser.go:15:
On 2013/11/22 00:46:52, wallyworld wrote:
> On 2013/11/21 05:17:42, axw wrote:
> > This seems like the wrong place to be talking about the API. This
should just
> be
> > about domain objects, I think.

> +1
> params.ServerAddressesResults does not belong here. No api params
structs should
> escape the api infrastructure

OK, I disagree here on two fronts.

Firstly this interface should be renamed to have something to do with
the API. It defines the API methods that it calls.

Secondly there is already precedence for exporting api/params structures
as return parameters for client api method calls.
I don't see anything wrong with that as the api/params package is
defined to have no other dependencies, and is just a holding package for
structures. It seems stupid to redefine it just because.

This interface should be both renamed, and have the first two methods
removed, as they are no longer needed.

https://codereview.appspot.com/29990045/

[Ian Booth (wallyworld)]

https://codereview.appspot.com/29990045/diff/1/worker/deployer/addresser.go
> File worker/deployer/addresser.go (right):

https://codereview.appspot.com/29990045/diff/1/worker/deployer/addresser.go#newcode15
> worker/deployer/addresser.go:15:
> On 2013/11/22 00:46:52, wallyworld wrote:
> > On 2013/11/21 05:17:42, axw wrote:
> > > This seems like the wrong place to be talking about the API. This
should
> just
> > be
> > > about domain objects, I think.
> >
> > +1
> > params.ServerAddressesResults does not belong here. No api params
structs
> should
> > escape the api infrastructure

> OK, I disagree here on two fronts.

> Firstly this interface should be renamed to have something to do with
the API.
> It defines the API methods that it calls.

> Secondly there is already precedence for exporting api/params
structures as
> return parameters for client api method calls.
> I don't see anything wrong with that as the api/params package is
defined to
> have no other dependencies, and is just a holding package for
structures. It
> seems stupid to redefine it just because.

> This interface should be both renamed, and have the first two methods
removed,
> as they are no longer needed.

Might be easier to discuss via a hangout. I have been tending to view
the params.* structs as internal, on-the-wire, representations of the
data to pass from client to server. And use of these outside of the api
becomes a layering violation. Perhaps I'm wrong

https://codereview.appspot.com/29990045/

[Ian Booth (wallyworld)]

https://codereview.appspot.com/29990045/diff/1/worker/deployer/addresser.go
> File worker/deployer/addresser.go (right):

https://codereview.appspot.com/29990045/diff/1/worker/deployer/addresser.go#newcode15
> worker/deployer/addresser.go:15:
> On 2013/11/22 00:46:52, wallyworld wrote:
> > On 2013/11/21 05:17:42, axw wrote:
> > > This seems like the wrong place to be talking about the API. This
should
> just
> > be
> > > about domain objects, I think.
> >
> > +1
> > params.ServerAddressesResults does not belong here. No api params
structs
> should
> > escape the api infrastructure

> OK, I disagree here on two fronts.

> Firstly this interface should be renamed to have something to do with
the API.
> It defines the API methods that it calls.

> Secondly there is already precedence for exporting api/params
structures as
> return parameters for client api method calls.
> I don't see anything wrong with that as the api/params package is
defined to
> have no other dependencies, and is just a holding package for
structures. It
> seems stupid to redefine it just because.

> This interface should be both renamed, and have the first two methods
removed,
> as they are no longer needed.

Might be easier to discuss via a hangout. I have been tending to view
the params.* structs as internal, on-the-wire, representations of the
data to pass from client to server. And use of these outside of the api
becomes a layering violation. Perhaps I'm wrong

https://codereview.appspot.com/29990045/

[Tim Penhey (thumper)]

Please take a look.

https://codereview.appspot.com/29990045/

[Ian Booth (wallyworld)]

LGTM. As discussed, we can bikeshed later various issues with current
api implementation. The end result of this mp is few api round trips
which is a very good thing. So let's get this sucker landed.

https://codereview.appspot.com/29990045/

=== modified file 'container/lxc/lxc_test.go'
--- container/lxc/lxc_test.go	2013-11-11 21:38:30 +0000
+++ container/lxc/lxc_test.go	2013-11-25 03:52:41 +0000
@@ -70,6 +70,7 @@
 	stateInfo := jujutesting.FakeStateInfo(machineId)
 	apiInfo := jujutesting.FakeAPIInfo(machineId)
 	machineConfig := environs.NewMachineConfig(machineId, "fake-nonce", stateInfo, apiInfo)
+	machineConfig.SyslogPort = 2345
 	machineConfig.Tools = &tools.Tools{
 		Version: version.MustParseBinary("2.3.4-foo-bar"),
 		URL:     "http://tools.testing.invalid/2.3.4-foo-bar.tgz",

=== modified file 'environs/cloudinit.go'
--- environs/cloudinit.go	2013-11-15 10:08:11 +0000
+++ environs/cloudinit.go	2013-11-25 03:52:41 +0000
@@ -53,8 +53,9 @@
 
 func PopulateMachineConfig(mcfg *cloudinit.MachineConfig,
 	providerType, authorizedKeys string,
-	sslHostnameVerification bool) error {
-
+	sslHostnameVerification bool,
+	syslogPort int,
+) error {
 	if authorizedKeys == "" {
 		return fmt.Errorf("environment configuration has no authorized-keys")
 	}
@@ -65,6 +66,7 @@
 	mcfg.AgentEnvironment[agent.ProviderType] = providerType
 	mcfg.AgentEnvironment[agent.ContainerType] = string(mcfg.MachineContainerType)
 	mcfg.DisableSSLHostnameVerification = !sslHostnameVerification
+	mcfg.SyslogPort = syslogPort
 	return nil
 }
 
@@ -81,7 +83,7 @@
 func FinishMachineConfig(mcfg *cloudinit.MachineConfig, cfg *config.Config, cons constraints.Value) (err error) {
 	defer utils.ErrorContextf(&err, "cannot complete machine configuration")
 
-	if err := PopulateMachineConfig(mcfg, cfg.Type(), cfg.AuthorizedKeys(), cfg.SSLHostnameVerification()); err != nil {
+	if err := PopulateMachineConfig(mcfg, cfg.Type(), cfg.AuthorizedKeys(), cfg.SSLHostnameVerification(), cfg.SyslogPort()); err != nil {
 		return err
 	}
 

=== modified file 'environs/cloudinit/cloudinit.go'
--- environs/cloudinit/cloudinit.go	2013-11-20 02:14:38 +0000
+++ environs/cloudinit/cloudinit.go	2013-11-25 03:52:41 +0000
@@ -58,6 +58,10 @@
 	// if StateServer is true.
 	APIPort int
 
+	// SyslogPort specifies the port number that will be used when
+	// sending the log messages using rsyslog.
+	SyslogPort int
+
 	// StateInfo holds the means for the new instance to communicate with the
 	// juju state. Unless the new machine is running a state server (StateServer is
 	// set), there must be at least one state server address supplied.
@@ -246,10 +250,10 @@
 	var configRenderer syslog.SyslogConfigRenderer
 	if cfg.StateServer {
 		configRenderer = syslog.NewAccumulateConfig(
-			names.MachineTag(cfg.MachineId))
+			names.MachineTag(cfg.MachineId), cfg.SyslogPort)
 	} else {
 		configRenderer = syslog.NewForwardConfig(
-			names.MachineTag(cfg.MachineId), cfg.stateHostAddrs())
+			names.MachineTag(cfg.MachineId), cfg.SyslogPort, cfg.stateHostAddrs())
 	}
 	content, err := configRenderer.Render()
 	if err != nil {
@@ -500,6 +504,9 @@
 	if cfg.APIInfo == nil {
 		return fmt.Errorf("missing API info")
 	}
+	if cfg.SyslogPort == 0 {
+		return fmt.Errorf("missing syslog port")
+	}
 	if len(cfg.APIInfo.CACert) == 0 {
 		return fmt.Errorf("missing API CA certificate")
 	}

=== modified file 'environs/cloudinit/cloudinit_test.go'
--- environs/cloudinit/cloudinit_test.go	2013-11-20 02:14:38 +0000
+++ environs/cloudinit/cloudinit_test.go	2013-11-25 03:52:41 +0000
@@ -71,6 +71,7 @@
 			StateServerKey:  serverKey,
 			StatePort:       37017,
 			APIPort:         17070,
+			SyslogPort:      514,
 			MachineNonce:    "FAKE_NONCE",
 			StateInfo: &state.Info{
 				Password: "arble",
@@ -140,6 +141,7 @@
 			StateServerKey:  serverKey,
 			StatePort:       37017,
 			APIPort:         17070,
+			SyslogPort:      514,
 			MachineNonce:    "FAKE_NONCE",
 			StateInfo: &state.Info{
 				Password: "arble",
@@ -188,6 +190,7 @@
 				Password: "bletch",
 				CACert:   []byte("CA CERT\n" + testing.CACert),
 			},
+			SyslogPort: 514,
 		},
 		expectScripts: `
 set -xe
@@ -236,6 +239,7 @@
 				Password: "bletch",
 				CACert:   []byte("CA CERT\n" + testing.CACert),
 			},
+			SyslogPort: 514,
 		},
 		inexactMatch: true,
 		expectScripts: `
@@ -272,6 +276,7 @@
 				Password: "bletch",
 				CACert:   []byte("CA CERT\n" + testing.CACert),
 			},
+			SyslogPort:                     514,
 			DisableSSLHostnameVerification: true,
 		},
 		inexactMatch: true,
@@ -291,6 +296,7 @@
 			StateServerKey:  serverKey,
 			StatePort:       37017,
 			APIPort:         17070,
+			SyslogPort:      514,
 			MachineNonce:    "FAKE_NONCE",
 			StateInfo: &state.Info{
 				Password: "arble",
@@ -581,6 +587,9 @@
 	{"missing API info", func(cfg *cloudinit.MachineConfig) {
 		cfg.APIInfo = nil
 	}},
+	{"missing syslog port", func(cfg *cloudinit.MachineConfig) {
+		cfg.SyslogPort = 0
+	}},
 	{"missing state hosts", func(cfg *cloudinit.MachineConfig) {
 		cfg.StateServer = false
 		cfg.StateInfo = &state.Info{
@@ -684,6 +693,7 @@
 		StateServerKey:   serverKey,
 		StatePort:        1234,
 		APIPort:          1235,
+		SyslogPort:       2345,
 		MachineId:        "99",
 		Tools:            newSimpleTools("9.9.9-linux-arble"),
 		AuthorizedKeys:   "sshkey1",

=== modified file 'environs/cloudinit_test.go'
--- environs/cloudinit_test.go	2013-11-06 13:48:29 +0000
+++ environs/cloudinit_test.go	2013-11-25 03:52:41 +0000
@@ -62,13 +62,15 @@
 		StateInfo: &state.Info{Tag: "not touched"},
 		APIInfo:   &api.Info{Tag: "not touched"},
 		DisableSSLHostnameVerification: false,
+		SyslogPort:                     2345,
 	})
 }
 
-func (s *CloudInitSuite) TestFinishMachineConfigNoSSLVerification(c *gc.C) {
+func (s *CloudInitSuite) TestFinishMachineConfigNonDefault(c *gc.C) {
 	attrs := dummySampleConfig().Merge(testing.Attrs{
 		"authorized-keys":           "we-are-the-keys",
 		"ssl-hostname-verification": false,
+		"syslog-port":               8888,
 	})
 	cfg, err := config.New(config.NoDefaults, attrs)
 	c.Assert(err, gc.IsNil)
@@ -87,6 +89,7 @@
 		StateInfo: &state.Info{Tag: "not touched"},
 		APIInfo:   &api.Info{Tag: "not touched"},
 		DisableSSLHostnameVerification: true,
+		SyslogPort:                     8888,
 	})
 }
 
@@ -164,6 +167,7 @@
 		Config:           envConfig,
 		StatePort:        envConfig.StatePort(),
 		APIPort:          envConfig.APIPort(),
+		SyslogPort:       envConfig.SyslogPort(),
 		StateServer:      true,
 		AgentEnvironment: map[string]string{agent.ProviderType: "dummy"},
 	}

=== modified file 'environs/config/config.go'
--- environs/config/config.go	2013-11-14 10:59:22 +0000
+++ environs/config/config.go	2013-11-25 03:52:41 +0000
@@ -38,6 +38,10 @@
 
 	// DefaultApiPort is the default port the API server is listening on.
 	DefaultAPIPort int = 17070
+
+	// DefaultSyslogPort is the default port that the syslog UDP listener is
+	// listening on.
+	DefaultSyslogPort int = 514
 )
 
 // Config holds an immutable environment configuration.
@@ -382,6 +386,11 @@
 	return c.mustInt("api-port")
 }
 
+// SyslogPort returns the syslog port for the environment.
+func (c *Config) SyslogPort() int {
+	return c.mustInt("syslog-port")
+}
+
 // AuthorizedKeys returns the content for ssh's authorized_keys file.
 func (c *Config) AuthorizedKeys() string {
 	return c.mustString("authorized-keys")
@@ -518,6 +527,7 @@
 	"ssl-hostname-verification": schema.Bool(),
 	"state-port":                schema.ForceInt(),
 	"api-port":                  schema.ForceInt(),
+	"syslog-port":               schema.ForceInt(),
 	"logging-config":            schema.String(),
 
 	// Deprecated fields, retain for backwards compatibility.
@@ -556,8 +566,9 @@
 
 	// For backward compatibility only - default ports were
 	// not filled out in previous versions of the configuration.
-	"state-port": DefaultStatePort,
-	"api-port":   DefaultAPIPort,
+	"state-port":  DefaultStatePort,
+	"api-port":    DefaultAPIPort,
+	"syslog-port": DefaultSyslogPort,
 }
 
 func allowEmpty(attr string) bool {
@@ -574,6 +585,7 @@
 		"ssl-hostname-verification": true,
 		"state-port":                DefaultStatePort,
 		"api-port":                  DefaultAPIPort,
+		"syslog-port":               DefaultSyslogPort,
 	}
 	for attr, val := range alwaysOptional {
 		d[attr] = val
@@ -606,6 +618,7 @@
 	"firewall-mode",
 	"state-port",
 	"api-port",
+	"syslog-port",
 }
 
 var (

=== modified file 'environs/config/config_test.go'
--- environs/config/config_test.go	2013-11-14 10:59:22 +0000
+++ environs/config/config_test.go	2013-11-25 03:52:41 +0000
@@ -52,6 +52,7 @@
 	"development":               false,
 	"state-port":                1234,
 	"api-port":                  4321,
+	"syslog-port":               2345,
 	"default-series":            "precise",
 }
 
@@ -453,6 +454,23 @@
 		},
 		err: `api-port: expected number, got string\("illegal"\)`,
 	}, {
+		about:       "Explicit syslog port",
+		useDefaults: config.UseDefaults,
+		attrs: testing.Attrs{
+			"type":        "my-type",
+			"name":        "my-name",
+			"syslog-port": 3456,
+		},
+	}, {
+		about:       "Invalid syslog port",
+		useDefaults: config.UseDefaults,
+		attrs: testing.Attrs{
+			"type":        "my-type",
+			"name":        "my-name",
+			"syslog-port": "illegal",
+		},
+		err: `syslog-port: expected number, got string\("illegal"\)`,
+	}, {
 		about:       "Invalid logging configuration",
 		useDefaults: config.UseDefaults,
 		attrs: testing.Attrs{
@@ -684,6 +702,9 @@
 	if apiPort, ok := test.attrs["api-port"]; ok {
 		c.Assert(cfg.APIPort(), gc.Equals, apiPort)
 	}
+	if syslogPort, ok := test.attrs["syslog-port"]; ok {
+		c.Assert(cfg.SyslogPort(), gc.Equals, syslogPort)
+	}
 
 	dev, _ := test.attrs["development"].(bool)
 	c.Assert(cfg.Development(), gc.Equals, dev)
@@ -798,6 +819,7 @@
 		"development":               false,
 		"state-port":                1234,
 		"api-port":                  4321,
+		"syslog-port":               2345,
 		"default-series":            "precise",
 	}
 	cfg, err := config.New(config.NoDefaults, attrs)
@@ -869,6 +891,11 @@
 	new:   testing.Attrs{"api-port": 42},
 	err:   `cannot change api-port from 17070 to 42`,
 }, {
+	about: "Cannot change the syslog-port",
+	old:   testing.Attrs{"syslog-port": 345},
+	new:   testing.Attrs{"syslog-port": 42},
+	err:   `cannot change syslog-port from 345 to 42`,
+}, {
 	about: "Can change the state-port from explicit-default to implicit-default",
 	old:   testing.Attrs{"state-port": config.DefaultStatePort},
 }, {
@@ -888,6 +915,10 @@
 	about: "Cannot change the api-port from implicit-default to different value",
 	new:   testing.Attrs{"api-port": 42},
 	err:   `cannot change api-port from 17070 to 42`,
+}, {
+	about: "Cannot change the syslog-port from implicit-default to different value",
+	new:   testing.Attrs{"syslog-port": 42},
+	err:   `cannot change syslog-port from 514 to 42`,
 }}
 
 func (*ConfigSuite) TestValidateChange(c *gc.C) {

=== modified file 'environs/config_test.go'
--- environs/config_test.go	2013-11-13 08:15:10 +0000
+++ environs/config_test.go	2013-11-25 03:52:41 +0000
@@ -16,14 +16,18 @@
 	"launchpad.net/juju-core/provider/dummy"
 	"launchpad.net/juju-core/testing"
 	jc "launchpad.net/juju-core/testing/checkers"
+	"launchpad.net/juju-core/testing/testbase"
 )
 
-type suite struct{}
-
-var _ = gc.Suite(suite{})
-
-func (suite) TearDownTest(c *gc.C) {
+type suite struct {
+	testbase.LoggingSuite
+}
+
+var _ = gc.Suite(&suite{})
+
+func (s *suite) TearDownTest(c *gc.C) {
 	dummy.Reset()
+	s.LoggingSuite.TearDownTest(c)
 }
 
 var invalidConfigTests = []struct {
@@ -40,7 +44,7 @@
 	},
 }
 
-func (suite) TestInvalidConfig(c *gc.C) {
+func (*suite) TestInvalidConfig(c *gc.C) {
 	for i, t := range invalidConfigTests {
 		c.Logf("running test %v", i)
 		_, err := environs.ReadEnvironsBytes([]byte(t.env))
@@ -72,7 +76,7 @@
 	},
 }
 
-func (suite) TestInvalidEnv(c *gc.C) {
+func (*suite) TestInvalidEnv(c *gc.C) {
 	defer testing.MakeFakeHomeNoEnvironments(c, "only").Restore()
 	for i, t := range invalidEnvTests {
 		c.Logf("running test %v", i)
@@ -84,7 +88,7 @@
 	}
 }
 
-func (suite) TestNoWarningForDeprecatedButUnusedEnv(c *gc.C) {
+func (*suite) TestNoWarningForDeprecatedButUnusedEnv(c *gc.C) {
 	// This tests that a config that has a deprecated field doesn't
 	// generate a Warning if we don't actually ask for that environment.
 	// However, we can only really trigger that when we have a deprecated
@@ -124,7 +128,7 @@
 	c.Check(tw.Log, gc.HasLen, 1)
 }
 
-func (suite) TestNoHomeBeforeConfig(c *gc.C) {
+func (*suite) TestNoHomeBeforeConfig(c *gc.C) {
 	// Test that we don't actually need HOME set until we call envs.Config()
 	// Because of this, we intentionally do *not* call testing.MakeFakeHomeNoEnvironments()
 	content := `
@@ -138,7 +142,7 @@
 	c.Check(err, gc.IsNil)
 }
 
-func (suite) TestNoEnv(c *gc.C) {
+func (*suite) TestNoEnv(c *gc.C) {
 	defer testing.MakeFakeHomeNoEnvironments(c).Restore()
 	es, err := environs.ReadEnvirons("")
 	c.Assert(es, gc.IsNil)
@@ -189,7 +193,7 @@
 	}},
 }
 
-func (suite) TestConfig(c *gc.C) {
+func (*suite) TestConfig(c *gc.C) {
 	defer testing.MakeFakeHomeNoEnvironments(c, "only", "valid", "one", "two").Restore()
 	for i, t := range configTests {
 		c.Logf("running test %v", i)
@@ -199,7 +203,7 @@
 	}
 }
 
-func (suite) TestDefaultConfigFile(c *gc.C) {
+func (*suite) TestDefaultConfigFile(c *gc.C) {
 	defer testing.MakeEmptyFakeHome(c).Restore()
 
 	env := `
@@ -221,7 +225,7 @@
 	c.Assert(cfg.Name(), gc.Equals, "only")
 }
 
-func (suite) TestConfigPerm(c *gc.C) {
+func (*suite) TestConfigPerm(c *gc.C) {
 	defer testing.MakeSampleHome(c).Restore()
 
 	path := testing.HomePath(".juju")
@@ -248,7 +252,7 @@
 
 }
 
-func (suite) TestNamedConfigFile(c *gc.C) {
+func (*suite) TestNamedConfigFile(c *gc.C) {
 	defer testing.MakeFakeHomeNoEnvironments(c, "only").Restore()
 
 	env := `
@@ -275,7 +279,7 @@
 	return ok
 }
 
-func (suite) TestBootstrapConfig(c *gc.C) {
+func (*suite) TestBootstrapConfig(c *gc.C) {
 	defer testing.MakeFakeHomeNoEnvironments(c, "bladaam").Restore()
 	attrs := dummySampleConfig().Merge(testing.Attrs{
 		"agent-version": "1.2.3",

=== modified file 'log/syslog/config.go'
--- log/syslog/config.go	2013-08-19 03:03:46 +0000
+++ log/syslog/config.go	2013-11-25 03:52:41 +0000
@@ -26,7 +26,7 @@
 $InputRunFileMonitor
 
 $ModLoad imudp
-$UDPServerRun 514
+$UDPServerRun {{portNumber}}
 
 # Messages received from remote rsyslog machines contain a leading space so we
 # need to account for that.
@@ -52,7 +52,7 @@
 $InputFileStateFile {{logfileName}}
 $InputRunFileMonitor
 
-:syslogtag, startswith, "juju-" @{{bootstrapIP}}:514
+:syslogtag, startswith, "juju-" @{{bootstrapIP}}:{{portNumber}}
 & ~
 `
 
@@ -77,24 +77,28 @@
 	LogFileName string
 	// the addresses of the state server to which messages should be forwarded.
 	StateServerAddresses []string
+	// the port number for the udp listener
+	Port int
 }
 
 // NewForwardConfig creates a SyslogConfig instance used on unit nodes to forward log entries
 // to the state server nodes.
-func NewForwardConfig(logFile string, stateServerAddresses []string) *SyslogConfig {
+func NewForwardConfig(logFile string, port int, stateServerAddresses []string) *SyslogConfig {
 	return &SyslogConfig{
 		configTemplate:       nodeRsyslogTemplate,
 		StateServerAddresses: stateServerAddresses,
 		LogFileName:          logFile,
+		Port:                 port,
 	}
 }
 
 // NewAccumulateConfig creates a SyslogConfig instance used to accumulate log entries from the
 // various unit nodes.
-func NewAccumulateConfig(logFile string) *SyslogConfig {
+func NewAccumulateConfig(logFile string, port int) *SyslogConfig {
 	return &SyslogConfig{
 		configTemplate: stateServerRsyslogTemplate,
 		LogFileName:    logFile,
+		Port:           port,
 	}
 }
 
@@ -127,11 +131,18 @@
 		return fmt.Sprintf("/var/spool/rsyslog/juju-%s-state", slConfig.LogFileName)
 	}
 
+	var portNumber = func() string {
+		return fmt.Sprint(slConfig.Port)
+	}
+
 	t := template.New("")
-	t.Funcs(template.FuncMap{"logfileName": logFileName})
-	t.Funcs(template.FuncMap{"bootstrapIP": bootstrapIP})
-	t.Funcs(template.FuncMap{"logfilePath": logFilePath})
-	t.Funcs(template.FuncMap{"statefilePath": stateFilePath})
+	t.Funcs(template.FuncMap{
+		"logfileName":   logFileName,
+		"bootstrapIP":   bootstrapIP,
+		"logfilePath":   logFilePath,
+		"statefilePath": stateFilePath,
+		"portNumber":    portNumber,
+	})
 
 	// Process the rsyslog config template and echo to the conf file.
 	p, err := t.Parse(slConfig.configTemplate)

=== modified file 'log/syslog/config_test.go'
--- log/syslog/config_test.go	2013-09-13 14:48:13 +0000
+++ log/syslog/config_test.go	2013-11-25 03:52:41 +0000
@@ -52,7 +52,7 @@
 $InputRunFileMonitor
 
 $ModLoad imudp
-$UDPServerRun 514
+$UDPServerRun 8888
 
 # Messages received from remote rsyslog machines contain a leading space so we
 # need to account for that.
@@ -66,12 +66,12 @@
 `
 
 func (s *SyslogConfigSuite) TestAccumulateConfigRender(c *gc.C) {
-	syslogConfigRenderer := syslog.NewAccumulateConfig("some-machine")
+	syslogConfigRenderer := syslog.NewAccumulateConfig("some-machine", 8888)
 	s.assertRsyslogConfigContents(c, syslogConfigRenderer, expectedAccumulateSyslogConf)
 }
 
 func (s *SyslogConfigSuite) TestAccumulateConfigWrite(c *gc.C) {
-	syslogConfigRenderer := syslog.NewAccumulateConfig("some-machine")
+	syslogConfigRenderer := syslog.NewAccumulateConfig("some-machine", 8888)
 	syslogConfigRenderer.ConfigDir = s.configDir
 	syslogConfigRenderer.ConfigFileName = "rsyslog.conf"
 	s.assertRsyslogConfigPath(c, syslogConfigRenderer)
@@ -93,17 +93,17 @@
 $InputFileStateFile some-machine
 $InputRunFileMonitor
 
-:syslogtag, startswith, "juju-" @server:514
+:syslogtag, startswith, "juju-" @server:999
 & ~
 `
 
 func (s *SyslogConfigSuite) TestForwardConfigRender(c *gc.C) {
-	syslogConfigRenderer := syslog.NewForwardConfig("some-machine", []string{"server"})
+	syslogConfigRenderer := syslog.NewForwardConfig("some-machine", 999, []string{"server"})
 	s.assertRsyslogConfigContents(c, syslogConfigRenderer, expectedForwardSyslogConf)
 }
 
 func (s *SyslogConfigSuite) TestForwardConfigWrite(c *gc.C) {
-	syslogConfigRenderer := syslog.NewForwardConfig("some-machine", []string{"server"})
+	syslogConfigRenderer := syslog.NewForwardConfig("some-machine", 999, []string{"server"})
 	syslogConfigRenderer.ConfigDir = s.configDir
 	syslogConfigRenderer.ConfigFileName = "rsyslog.conf"
 	s.assertRsyslogConfigPath(c, syslogConfigRenderer)

=== modified file 'provider/azure/customdata_test.go'
--- provider/azure/customdata_test.go	2013-09-20 02:33:04 +0000
+++ provider/azure/customdata_test.go	2013-11-25 03:52:41 +0000
@@ -44,6 +44,7 @@
 			Addrs:  []string{"127.0.0.1:123"},
 			Tag:    names.MachineTag(machineID),
 		},
+		SyslogPort: 2345,
 	}
 }
 

=== modified file 'provider/dummy/environs.go'
--- provider/dummy/environs.go	2013-11-18 05:41:36 +0000
+++ provider/dummy/environs.go	2013-11-25 03:52:41 +0000
@@ -74,6 +74,7 @@
 		"development":               false,
 		"state-port":                1234,
 		"api-port":                  4321,
+		"syslog-port":               2345,
 		"default-series":            "precise",
 
 		"secret":       "pork",

=== modified file 'state/address.go'
--- state/address.go	2013-10-14 17:44:23 +0000
+++ state/address.go	2013-11-25 03:52:41 +0000
@@ -110,3 +110,27 @@
 	}
 	return appendPort(addrs, config.APIPort()), nil
 }
+
+type DeployerConnectionValues struct {
+	StateAddresses []string
+	APIAddresses   []string
+	SyslogPort     int
+}
+
+// DeployerConnectionInfo returns the address information necessary for the deployer.
+// The function does the expensive operations (getting stuff from mongo) just once.
+func (st *State) DeployerConnectionInfo() (*DeployerConnectionValues, error) {
+	addrs, err := st.stateServerAddresses()
+	if err != nil {
+		return nil, err
+	}
+	config, err := st.EnvironConfig()
+	if err != nil {
+		return nil, err
+	}
+	return &DeployerConnectionValues{
+		StateAddresses: appendPort(addrs, config.StatePort()),
+		APIAddresses:   appendPort(addrs, config.APIPort()),
+		SyslogPort:     config.SyslogPort(),
+	}, nil
+}

=== modified file 'state/api/deployer/deployer.go'
--- state/api/deployer/deployer.go	2013-07-24 13:15:52 +0000
+++ state/api/deployer/deployer.go	2013-11-25 03:52:41 +0000
@@ -90,3 +90,10 @@
 	}
 	return result.Result, nil
 }
+
+// ConnectionInfo returns all the address information that the deployer task
+// needs in one call.
+func (st *State) ConnectionInfo() (result params.DeployerConnectionValues, err error) {
+	err = st.caller.Call("Deployer", "", "ConnectionInfo", nil, &result)
+	return result, err
+}

=== modified file 'state/api/params/params.go'
--- state/api/params/params.go	2013-11-21 14:43:12 +0000
+++ state/api/params/params.go	2013-11-25 03:52:41 +0000
@@ -479,6 +479,7 @@
 	ProviderType            string
 	AuthorizedKeys          string
 	SSLHostnameVerification bool
+	SyslogPort              int
 }
 
 type MachineConfigParams struct {
@@ -517,3 +518,11 @@
 type SetEnvironAgentVersion struct {
 	Version version.Number
 }
+
+// DeployerConnectionValues containers the result of deployer.ConnectionInfo
+// API call.
+type DeployerConnectionValues struct {
+	StateAddresses []string
+	APIAddresses   []string
+	SyslogPort     int
+}

=== modified file 'state/api/provisioner/provisioner_test.go'
--- state/api/provisioner/provisioner_test.go	2013-11-19 02:14:38 +0000
+++ state/api/provisioner/provisioner_test.go	2013-11-25 03:52:41 +0000
@@ -377,6 +377,7 @@
 	c.Assert(result.ProviderType, gc.Equals, "dummy")
 	c.Assert(result.AuthorizedKeys, gc.Equals, "my-keys")
 	c.Assert(result.SSLHostnameVerification, jc.IsTrue)
+	c.Assert(result.SyslogPort, gc.Equals, 2345)
 }
 
 func (s *provisionerSuite) TestCACert(c *gc.C) {

=== modified file 'state/apiserver/deployer/deployer.go'
--- state/apiserver/deployer/deployer.go	2013-11-01 21:06:12 +0000
+++ state/apiserver/deployer/deployer.go	2013-11-25 03:52:41 +0000
@@ -122,3 +122,17 @@
 	}
 	return result, nil
 }
+
+// ServerAddresses returns all the address information that the deployer task
+// needs in one call.
+func (d *DeployerAPI) ConnectionInfo() (result params.DeployerConnectionValues, err error) {
+	info, err := d.st.DeployerConnectionInfo()
+	if info != nil {
+		result = params.DeployerConnectionValues{
+			StateAddresses: info.StateAddresses,
+			APIAddresses:   info.APIAddresses,
+			SyslogPort:     info.SyslogPort,
+		}
+	}
+	return result, err
+}

=== modified file 'state/apiserver/provisioner/provisioner.go'
--- state/apiserver/provisioner/provisioner.go	2013-11-22 03:08:44 +0000
+++ state/apiserver/provisioner/provisioner.go	2013-11-25 03:52:41 +0000
@@ -224,6 +224,7 @@
 	result.ProviderType = config.Type()
 	result.AuthorizedKeys = config.AuthorizedKeys()
 	result.SSLHostnameVerification = config.SSLHostnameVerification()
+	result.SyslogPort = config.SyslogPort()
 	return result, nil
 }
 

=== modified file 'state/apiserver/provisioner/provisioner_test.go'
--- state/apiserver/provisioner/provisioner_test.go	2013-11-22 03:08:44 +0000
+++ state/apiserver/provisioner/provisioner_test.go	2013-11-25 03:52:41 +0000
@@ -698,6 +698,7 @@
 	c.Check(results.ProviderType, gc.Equals, "dummy")
 	c.Check(results.AuthorizedKeys, gc.Equals, "my-keys")
 	c.Check(results.SSLHostnameVerification, jc.IsTrue)
+	c.Check(results.SyslogPort, gc.Equals, 2345)
 }
 
 func (s *provisionerSuite) TestToolsRefusesWrongAgent(c *gc.C) {

=== removed file 'worker/deployer/addresser.go'
--- worker/deployer/addresser.go	2013-07-25 11:03:16 +0000
+++ worker/deployer/addresser.go	1970-01-01 00:00:00 +0000
@@ -1,11 +0,0 @@
-// Copyright 2013 Canonical Ltd.
-// Licensed under the AGPLv3, see LICENCE file for details.
-
-package deployer
-
-// Addresser implementations provide the capability to lookup a list
-// of server addresses.
-type Addresser interface {
-	StateAddresses() ([]string, error)
-	APIAddresses() ([]string, error)
-}

=== modified file 'worker/deployer/export_test.go'
--- worker/deployer/export_test.go	2013-09-02 06:03:14 +0000
+++ worker/deployer/export_test.go	2013-11-25 03:52:41 +0000
@@ -5,21 +5,22 @@
 
 import (
 	"launchpad.net/juju-core/agent"
+	"launchpad.net/juju-core/state/api/params"
 )
 
-type fakeAddresser struct{}
-
-func (*fakeAddresser) StateAddresses() ([]string, error) {
-	return []string{"s1:123", "s2:123"}, nil
-}
-
-func (*fakeAddresser) APIAddresses() ([]string, error) {
-	return []string{"a1:123", "a2:123"}, nil
+type fakeAPI struct{}
+
+func (*fakeAPI) ConnectionInfo() (params.DeployerConnectionValues, error) {
+	return params.DeployerConnectionValues{
+		[]string{"s1:123", "s2:123"},
+		[]string{"a1:123", "a2:123"},
+		2345,
+	}, nil
 }
 
 func NewTestSimpleContext(agentConfig agent.Config, initDir, logDir, syslogConfigDir string) *SimpleContext {
 	return &SimpleContext{
-		addresser:       &fakeAddresser{},
+		api:             &fakeAPI{},
 		agentConfig:     agentConfig,
 		initDir:         initDir,
 		logDir:          logDir,

=== modified file 'worker/deployer/simple.go'
--- worker/deployer/simple.go	2013-09-02 22:15:48 +0000
+++ worker/deployer/simple.go	2013-11-25 03:52:41 +0000
@@ -16,17 +16,23 @@
 	"launchpad.net/juju-core/juju/osenv"
 	"launchpad.net/juju-core/log/syslog"
 	"launchpad.net/juju-core/names"
+	"launchpad.net/juju-core/state/api/params"
 	"launchpad.net/juju-core/upstart"
 	"launchpad.net/juju-core/version"
 )
 
+// APICalls defines the interface to the API that the simple context needs.
+type APICalls interface {
+	ConnectionInfo() (params.DeployerConnectionValues, error)
+}
+
 // SimpleContext is a Context that manages unit deployments via upstart
 // jobs on the local system.
 type SimpleContext struct {
 
-	// addresser is used to get the current state server addresses at the time
-	// the given unit is deployed.
-	addresser Addresser
+	// api is used to get the current state server addresses at the time the
+	// given unit is deployed.
+	api APICalls
 
 	// agentConfig returns the agent config for the machine agent that is
 	// running the deployer.
@@ -55,9 +61,9 @@
 // specified deployer, that deploys unit agents as upstart jobs in
 // "/etc/init" logging to "/var/log/juju". Paths to which agents and tools
 // are installed are relative to dataDir.
-func NewSimpleContext(agentConfig agent.Config, addresser Addresser) *SimpleContext {
+func NewSimpleContext(agentConfig agent.Config, api APICalls) *SimpleContext {
 	return &SimpleContext{
-		addresser:   addresser,
+		api:         api,
 		agentConfig: agentConfig,
 		initDir:     "/etc/init",
 		logDir:      "/var/log/juju",
@@ -82,27 +88,22 @@
 	toolsDir := tools.ToolsDir(dataDir, tag)
 	defer removeOnErr(&err, toolsDir)
 
-	// Retrieve the state addresses.
-	// TODO: remove the state addresses when unit agent is API only.
-	stateAddrs, err := ctx.addresser.StateAddresses()
-	if err != nil {
-		return err
-	}
-	logger.Debugf("state addresses: %q", stateAddrs)
-	apiAddrs, err := ctx.addresser.APIAddresses()
-	if err != nil {
-		return err
-	}
-	logger.Debugf("API addresses: %q", apiAddrs)
+	result, err := ctx.api.ConnectionInfo()
+	if err != nil {
+		return err
+	}
+	logger.Debugf("state addresses: %q", result.StateAddresses)
+	logger.Debugf("API addresses: %q", result.APIAddresses)
 	containerType := ctx.agentConfig.Value(agent.ContainerType)
 	conf, err := agent.NewAgentConfig(
 		agent.AgentConfigParams{
-			DataDir:        dataDir,
-			Tag:            tag,
-			Password:       initialPassword,
-			Nonce:          "unused",
-			StateAddresses: stateAddrs,
-			APIAddresses:   apiAddrs,
+			DataDir:  dataDir,
+			Tag:      tag,
+			Password: initialPassword,
+			Nonce:    "unused",
+			// TODO: remove the state addresses here and test when api only.
+			StateAddresses: result.StateAddresses,
+			APIAddresses:   result.APIAddresses,
 			CACert:         ctx.agentConfig.CACert(),
 			Values: map[string]string{
 				agent.ContainerType: containerType,
@@ -118,7 +119,7 @@
 
 	// Install an upstart job that runs the unit agent.
 	logPath := path.Join(ctx.logDir, tag+".log")
-	syslogConfigRenderer := syslog.NewForwardConfig(tag, stateAddrs)
+	syslogConfigRenderer := syslog.NewForwardConfig(tag, result.SyslogPort, result.StateAddresses)
 	syslogConfigRenderer.ConfigDir = ctx.syslogConfigDir
 	syslogConfigRenderer.ConfigFileName = fmt.Sprintf("26-juju-%s.conf", tag)
 	if err := syslogConfigRenderer.Write(); err != nil {

=== modified file 'worker/deployer/simple_test.go'
--- worker/deployer/simple_test.go	2013-10-21 21:49:04 +0000
+++ worker/deployer/simple_test.go	2013-11-25 03:52:41 +0000
@@ -218,7 +218,7 @@
 $InputFileStateFile %s
 $InputRunFileMonitor
 
-:syslogtag, startswith, "juju-" @s1:514
+:syslogtag, startswith, "juju-" @s1:2345
 & ~
 `
 

=== modified file 'worker/provisioner/lxc-broker.go'
--- worker/provisioner/lxc-broker.go	2013-11-11 21:38:30 +0000
+++ worker/provisioner/lxc-broker.go	2013-11-25 03:52:41 +0000
@@ -74,6 +74,7 @@
 		config.ProviderType,
 		config.AuthorizedKeys,
 		config.SSLHostnameVerification,
+		config.SyslogPort,
 	); err != nil {
 		lxcLogger.Errorf("failed to populate machine config: %v", err)
 		return nil, nil, err

=== modified file 'worker/provisioner/lxc-broker_test.go'
--- worker/provisioner/lxc-broker_test.go	2013-11-20 04:49:01 +0000
+++ worker/provisioner/lxc-broker_test.go	2013-11-25 03:52:41 +0000
@@ -284,5 +284,5 @@
 type fakeAPI struct{}
 
 func (*fakeAPI) ContainerConfig() (params.ContainerConfig, error) {
-	return params.ContainerConfig{"fake", "my-keys", true}, nil
+	return params.ContainerConfig{"fake", "my-keys", true, 2345}, nil
 }