Environ.OpenPorts() for the Azure provider.

Environ.OpenPorts() for the Azure provider.

After discussion with jtv, this method is tested by providing a patch
point through which azureInstance.openEndpoints() is called. This
avoids having to set up a lengthy HTTP conversation only to
demonstrate that it is being called successfully.

https://code.launchpad.net/~allenap/juju-core/azure-open-ports/+merge/177908

(do not edit description out of merge proposal)

[allenap, 2013-07-31 17:10:14]

Please take a look.

[jtv.canonical, 2013-08-02 10:49:17]

Yes, setting up a huge test for this does seem disproportionate.  But you could
start by unit-testing azureEnviron.openEndpoints for zero instances.

Once you've done that, and unit-tested azureInstance.openEndpoints, then I
suspect you'll have done most of the work needed to unit-test
azureEnviron.openEndpoints.  The rest may be a matter of factoring out the right
helpers.

[allenap, 2013-08-02 12:50:28]

Please take a look.

[dimitern, 2013-08-02 13:04:31]

LGTM modulo a few suggestions below

https://codereview.appspot.com/12176044/diff/5001/environs/azure/environ_test.go
File environs/azure/environ_test.go (right):

https://codereview.appspot.com/12176044/diff/5001/environs/azure/environ_test...
environs/azure/environ_test.go:1046: openInstanceEndpoints = func(i
*azureInstance, _ *azureManagementContext, p []instance.Port) error {

Since you're only using one argument, how about:
openInstanceEndpoints = func(i *azureInstance, _ *azureManagementContext, _
[]instance.Port) error { ...

https://codereview.appspot.com/12176044/diff/5001/environs/azure/environ_test...
environs/azure/environ_test.go:1086: c.Assert(err, ErrorMatches, "GET request
failed [(]500: Internal Server Error[)]")

"GET request failed \(500: Internal Server Error\)" ?

https://codereview.appspot.com/12176044/diff/5001/environs/azure/environ_test...
environs/azure/environ_test.go:1096: openInstanceEndpoints = func(i
*azureInstance, _ *azureManagementContext, p []instance.Port) error {

How about:
openInstanceEndpoints = func(*azureInstance, *azureManagementContext,
[]instance.Port) error { ...

[allenap, 2013-08-02 13:10:10]

Thanks for the review Dimiter!

https://codereview.appspot.com/12176044/diff/5001/environs/azure/environ_test.go
File environs/azure/environ_test.go (right):

https://codereview.appspot.com/12176044/diff/5001/environs/azure/environ_test...
environs/azure/environ_test.go:1046: openInstanceEndpoints = func(i
*azureInstance, _ *azureManagementContext, p []instance.Port) error {
On 2013/08/02 13:04:32, dimitern wrote:
> 
> Since you're only using one argument, how about:
> openInstanceEndpoints = func(i *azureInstance, _ *azureManagementContext, _
> []instance.Port) error { ...

I'm using the last argument (the ports) too.

https://codereview.appspot.com/12176044/diff/5001/environs/azure/environ_test...
environs/azure/environ_test.go:1086: c.Assert(err, ErrorMatches, "GET request
failed [(]500: Internal Server Error[)]")
On 2013/08/02 13:04:32, dimitern wrote:
> 
> "GET request failed \(500: Internal Server Error\)" ?

I avoid back-slashes here; it's not clear whether they're escaping the next
character in the string or in the regular expression without remembering a pile
of rules. Some regular expression syntaxes use \(...\) for capture groups too.
Using square braces is unambiguous, and only one more character.

https://codereview.appspot.com/12176044/diff/5001/environs/azure/environ_test...
environs/azure/environ_test.go:1096: openInstanceEndpoints = func(i
*azureInstance, _ *azureManagementContext, p []instance.Port) error {
On 2013/08/02 13:04:32, dimitern wrote:
> 
> How about:
> openInstanceEndpoints = func(*azureInstance, *azureManagementContext,
> []instance.Port) error { ...

Yeah, I'll do that.

[gz, 2013-08-02 16:34:28]

No real comments on the code but...

* Do we actually want to support global firewaller mode with azure? It's mostly
a hack around security group limits with ec2 and openstack, rather than
something people should actually use.

* What happens when new deloyments are created after a port is opened? Unless
I'm missing something, they don't get the port open applied. With a global
security group, once port N is opened, all machines from there on can be
accessed on that port.

[allenap, 2013-08-02 17:22:30]

On 2013/08/02 16:34:28, gz wrote:
> No real comments on the code but...
> 
> * Do we actually want to support global firewaller mode with azure? It's
mostly
> a hack around security group limits with ec2 and openstack, rather than
> something people should actually use.

How do we indicate that it's not supported? I'm happy with that.

> 
> * What happens when new deloyments are created after a port is opened? Unless
> I'm missing something, they don't get the port open applied. With a global
> security group, once port N is opened, all machines from there on can be
> accessed on that port.

You're not missing anything; the port will not be open on a new machine. I
assumed that Juju would ensure convergence. Sounds like not supporting the
global firewall mode is the best approach.