Merge lp:~allenap/juju-core/azure-open-machine-ports into lp:~go-bot/juju-core/trunk

[Gavin Panella]

Implement azureInstance.OpenPorts()

Implement azureInstance.OpenPorts()

https://codereview.appspot.com/12102045/

[Gavin Panella (allenap)]

Reviewers: mp+177717_code.launchpad.net,

Message:
Please take a look.

Description:
Implement azureInstance.OpenPorts()

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

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M environs/azure/environ.go
   M environs/azure/environ_test.go
   M environs/azure/instance.go
   M environs/azure/instance_test.go

[Julian Edwards (julian-edwards)]

LGTM

Given that we always map one role to one deployment to one service, is
it strictly necessary to have that loop around deployments in
OpenPorts()?

Also does juju's identifier for a protocol match that of Azure's? The
doc for instance.Port does not say what "Protocol" is.

https://codereview.appspot.com/12102045/

[Raphaël Badin (rvb)]

On 2013/07/30 23:16:12, allenap wrote:
> Please take a look.

[0]

Julian has a point, only one deployment should be present in the
service, and it has the same name as the service
('azInstance.ServiceName'). That will save one API call.

[1]

We've used a "tag" to indicate all the places in the code which rely on
the assumption that one juju instance = one service = one deployment so
that if people change that model, they will have a marker indicating all
the places where things must be changed. Would you mind adding this
"tag" in a comment somwhere: "(instance==service)"?

https://codereview.appspot.com/12102045/

[Raphaël Badin (rvb)]

LGTM

https://codereview.appspot.com/12102045/

[Raphaël Badin (rvb)]

On 2013/07/31 09:00:46, rvb wrote:
> On 2013/07/30 23:16:12, allenap wrote:
> > Please take a look.

> [0]

> Julian has a point, only one deployment should be present in the
service, and it
> has the same name as the service ('azInstance.ServiceName'). That
will save one
> API call.

Actually, since you need to get all the roles for the deployment, I
don't think relying on the fact that we know there is only one
deployment named 'serviceName' will save us an API call…

https://codereview.appspot.com/12102045/

[Gavin Panella (allenap)]

On 2013/07/31 09:04:02, rvb wrote:
> Actually, since you need to get all the roles for the deployment, I
don't think
> relying on the fact that we know there is only one deployment named
> 'serviceName' will save us an API call…

The code as I've written it will work now and if the
service:deployment:role ratio changes so I think it's better to leave
it.

Thank you both for the reviews!

https://codereview.appspot.com/12102045/

[Gavin Panella (allenap)]

On 2013/07/30 23:53:07, julian.edwards wrote:
> Also does juju's identifier for a protocol match that of Azure's? The
doc for
> instance.Port does not say what "Protocol" is.

It seems to be either "tcp" or "udp". Azure's endpoint name seems to be
mainly for human consumption right now, albeit with undocumented
limitations on what form it may take.

https://codereview.appspot.com/12102045/

[Raphaël Badin (rvb)]

> The code as I've written it will work now and if the
service:deployment:role
> ratio changes so I think it's better to leave it.

I agree this is the way to go.

https://codereview.appspot.com/12102045/

=== modified file 'environs/azure/environ.go'
--- environs/azure/environ.go	2013-07-30 09:30:57 +0000
+++ environs/azure/environ.go	2013-07-30 23:14:25 +0000
@@ -483,7 +483,7 @@
 	if err != nil {
 		return nil, fmt.Errorf("could not get instance %q: %v", instanceName, err)
 	}
-	instance := &azureInstance{service.HostedServiceDescriptor}
+	instance := &azureInstance{service.HostedServiceDescriptor, env}
 	return instance, nil
 }
 
@@ -627,7 +627,7 @@
 		return nil, environs.ErrNoInstances
 	}
 
-	instances := convertToInstances(services)
+	instances := convertToInstances(services, env)
 
 	// Check if we got a partial result.
 	if len(ids) != len(instances) {
@@ -652,7 +652,7 @@
 	if err != nil {
 		return nil, err
 	}
-	return convertToInstances(services), nil
+	return convertToInstances(services, env), nil
 }
 
 // getEnvPrefix returns the prefix used to name the objects specific to this
@@ -663,10 +663,10 @@
 
 // convertToInstances converts a slice of gwacl.HostedServiceDescriptor objects
 // into a slice of instance.Instance objects.
-func convertToInstances(services []gwacl.HostedServiceDescriptor) []instance.Instance {
+func convertToInstances(services []gwacl.HostedServiceDescriptor, env *azureEnviron) []instance.Instance {
 	instances := make([]instance.Instance, len(services))
 	for i, service := range services {
-		instances[i] = &azureInstance{service}
+		instances[i] = &azureInstance{service, env}
 	}
 	return instances
 }
 
=== modified file 'environs/azure/environ_test.go'
--- environs/azure/environ_test.go	2013-07-30 09:20:19 +0000
+++ environs/azure/environ_test.go	2013-07-30 23:14:25 +0000
@@ -586,7 +586,9 @@
 	responses := buildDestroyAzureServiceResponses(c, services)
 	requests := gwacl.PatchManagementAPIResponses(responses)
 	env := makeEnviron(c)
-	instances := convertToInstances([]gwacl.HostedServiceDescriptor{*service1Desc, *service2Desc})
+	instances := convertToInstances(
+		[]gwacl.HostedServiceDescriptor{*service1Desc, *service2Desc},
+		env)
 
 	err := env.StopInstances(instances)
 	c.Check(err, IsNil)
@@ -630,7 +632,7 @@
 	cleanupResponses := getVnetAndAffinityGroupCleanupResponses(c)
 	responses = append(responses, cleanupResponses...)
 	gwacl.PatchManagementAPIResponses(responses)
-	instances := convertToInstances([]gwacl.HostedServiceDescriptor{})
+	instances := convertToInstances([]gwacl.HostedServiceDescriptor{}, env)
 
 	err := env.Destroy(instances)
 	c.Check(err, IsNil)
@@ -665,7 +667,7 @@
 	}
 	responses = append(responses, cleanupResponses...)
 	requests := gwacl.PatchManagementAPIResponses(responses)
-	instances := convertToInstances([]gwacl.HostedServiceDescriptor{})
+	instances := convertToInstances([]gwacl.HostedServiceDescriptor{}, env)
 
 	err = env.Destroy(instances)
 	c.Check(err, IsNil)
@@ -718,7 +720,9 @@
 	requests := gwacl.PatchManagementAPIResponses(responses)
 
 	// Call Destroy with service1 and service2.
-	instances := convertToInstances([]gwacl.HostedServiceDescriptor{*service1Desc, *service2Desc})
+	instances := convertToInstances(
+		[]gwacl.HostedServiceDescriptor{*service1Desc, *service2Desc},
+		env)
 	err := env.Destroy(instances)
 	c.Check(err, IsNil)
 
@@ -751,6 +755,9 @@
 	c.Check(err, IsNil)
 
 	c.Check(string(instance.Id()), Equals, serviceName)
+	c.Check(instance, FitsTypeOf, &azureInstance{})
+	azInstance := instance.(*azureInstance)
+	c.Check(azInstance.environ, Equals, env)
 }
 
 func (*EnvironSuite) TestNewOSVirtualDisk(c *C) {
@@ -1008,3 +1015,15 @@
 	// may have to become configurable.
 	c.Check(env.getImageMetadataSigningRequired(), Equals, true)
 }
+
+func (*EnvironSuite) TestConvertToInstances(c *C) {
+	services := []gwacl.HostedServiceDescriptor{
+		{ServiceName: "foo"}, {ServiceName: "bar"},
+	}
+	env := makeEnviron(c)
+	instances := convertToInstances(services, env)
+	c.Check(instances, DeepEquals, []instance.Instance{
+		&azureInstance{services[0], env},
+		&azureInstance{services[1], env},
+	})
+}
 
=== modified file 'environs/azure/instance.go'
--- environs/azure/instance.go	2013-07-19 01:18:26 +0000
+++ environs/azure/instance.go	2013-07-30 23:14:25 +0000
@@ -15,6 +15,7 @@
 type azureInstance struct {
 	// An instance contains an Azure Service (instance==service).
 	gwacl.HostedServiceDescriptor
+	environ *azureEnviron
 }
 
 // azureInstance implements Instance.
@@ -50,7 +51,46 @@
 
 // OpenPorts is specified in the Instance interface.
 func (azInstance *azureInstance) OpenPorts(machineId string, ports []instance.Port) error {
-	// TODO: implement this.
+	env := azInstance.environ
+	context, err := env.getManagementAPI()
+	if err != nil {
+		return err
+	}
+	defer env.releaseManagementAPI(context)
+
+	env.Lock()
+	defer env.Unlock()
+
+	deployments, err := context.ListAllDeployments(&gwacl.ListAllDeploymentsRequest{
+		ServiceName: azInstance.ServiceName,
+	})
+	if err != nil {
+		return err
+	}
+
+	for _, deployment := range deployments {
+		for _, role := range deployment.RoleList {
+			request := &gwacl.AddRoleEndpointsRequest{
+				ServiceName:    azInstance.ServiceName,
+				DeploymentName: deployment.Name,
+				RoleName:       role.RoleName,
+			}
+			for _, port := range ports {
+				request.InputEndpoints = append(
+					request.InputEndpoints, gwacl.InputEndpoint{
+						LocalPort: port.Number,
+						Name:      fmt.Sprintf("%s%d", port.Protocol, port.Number),
+						Port:      port.Number,
+						Protocol:  port.Protocol,
+					})
+			}
+			err := context.AddRoleEndpoints(request)
+			if err != nil {
+				return err
+			}
+		}
+	}
+
 	return nil
 }
 
 
=== modified file 'environs/azure/instance_test.go'
--- environs/azure/instance_test.go	2013-07-16 03:10:34 +0000
+++ environs/azure/instance_test.go	2013-07-30 23:14:25 +0000
@@ -9,7 +9,9 @@
 	. "launchpad.net/gocheck"
 	"launchpad.net/gwacl"
 
+	"fmt"
 	"launchpad.net/juju-core/instance"
+	"net/http"
 )
 
 type InstanceSuite struct{}
@@ -26,7 +28,7 @@
 func (*StorageSuite) TestId(c *C) {
 	serviceName := "test-name"
 	testService := makeHostedServiceDescriptor(serviceName)
-	azInstance := azureInstance{*testService}
+	azInstance := azureInstance{*testService, nil}
 	c.Check(azInstance.Id(), Equals, instance.Id(serviceName))
 }
 
@@ -34,7 +36,7 @@
 	// An instance's DNS name is computed from its hosted-service name.
 	host := "hostname"
 	testService := makeHostedServiceDescriptor(host)
-	azInstance := azureInstance{*testService}
+	azInstance := azureInstance{*testService, nil}
 	dnsName, err := azInstance.DNSName()
 	c.Assert(err, IsNil)
 	c.Check(dnsName, Equals, host+"."+AZURE_DOMAIN_NAME)
@@ -45,8 +47,196 @@
 	// waiting involved.
 	host := "hostname"
 	testService := makeHostedServiceDescriptor(host)
-	azInstance := azureInstance{*testService}
+	azInstance := azureInstance{*testService, nil}
 	dnsName, err := azInstance.WaitDNSName()
 	c.Assert(err, IsNil)
 	c.Check(dnsName, Equals, host+"."+AZURE_DOMAIN_NAME)
 }
+
+func makeRole(name string) gwacl.Role {
+	return gwacl.Role{
+		RoleName: name,
+		ConfigurationSets: []gwacl.ConfigurationSet{
+			{ConfigurationSetType: gwacl.CONFIG_SET_NETWORK},
+		},
+	}
+}
+
+func makeDeployment(name string, roles ...gwacl.Role) gwacl.Deployment {
+	return gwacl.Deployment{
+		Name:     name,
+		RoleList: roles,
+	}
+}
+
+func makeInputEndpoint(port int, protocol string) gwacl.InputEndpoint {
+	return gwacl.InputEndpoint{
+		LocalPort: port,
+		Name:      fmt.Sprintf("%s%d", protocol, port),
+		Port:      port,
+		Protocol:  protocol,
+	}
+}
+
+func serialize(c *C, object gwacl.AzureObject) []byte {
+	xml, err := object.Serialize()
+	c.Assert(err, IsNil)
+	return []byte(xml)
+}
+
+func (*StorageSuite) TestOpenPorts(c *C) {
+	service := makeHostedServiceDescriptor("service-name")
+	deployments := []gwacl.Deployment{
+		makeDeployment("deployment-one", makeRole("role-one"), makeRole("role-two")),
+		makeDeployment("deployment-two", makeRole("role-three")),
+	}
+	responses := []gwacl.DispatcherResponse{
+		// First, GetHostedServiceProperties
+		gwacl.NewDispatcherResponse(
+			serialize(c, &gwacl.HostedService{
+				Deployments:             deployments,
+				HostedServiceDescriptor: *service,
+				XMLNS: gwacl.XMLNS,
+			}),
+			http.StatusOK, nil),
+	}
+	for _, deployment := range deployments {
+		for _, role := range deployment.RoleList {
+			// GetRole returns a PersistentVMRole.
+			persistentRole := &gwacl.PersistentVMRole{
+				XMLNS:             gwacl.XMLNS,
+				RoleName:          role.RoleName,
+				ConfigurationSets: role.ConfigurationSets,
+			}
+			responses = append(responses, gwacl.NewDispatcherResponse(
+				serialize(c, persistentRole), http.StatusOK, nil))
+			// UpdateRole expects a 200 response, that's all.
+			responses = append(responses,
+				gwacl.NewDispatcherResponse(nil, http.StatusOK, nil))
+		}
+	}
+	record := gwacl.PatchManagementAPIResponses(responses)
+	env := makeEnviron(c)
+	azInstance := azureInstance{*service, env}
+
+	err := azInstance.OpenPorts("machine-id", []instance.Port{
+		{"tcp", 79}, {"tcp", 587}, {"udp", 9},
+	})
+
+	c.Assert(err, IsNil)
+	expected := []struct {
+		method     string
+		urlpattern string
+	}{
+		{"GET", ".*/services/hostedservices/service-name[?].*"},   // GetHostedServiceProperties
+		{"GET", ".*/deployments/deployment-one/roles/role-one"},   // GetRole
+		{"PUT", ".*/deployments/deployment-one/roles/role-one"},   // UpdateRole
+		{"GET", ".*/deployments/deployment-one/roles/role-two"},   // GetRole
+		{"PUT", ".*/deployments/deployment-one/roles/role-two"},   // UpdateRole
+		{"GET", ".*/deployments/deployment-two/roles/role-three"}, // GetRole
+		{"PUT", ".*/deployments/deployment-two/roles/role-three"}, // UpdateRole
+	}
+	c.Assert(*record, HasLen, len(expected))
+	for index, request := range *record {
+		c.Check(request.Method, Equals, expected[index].method)
+		c.Check(request.URL, Matches, expected[index].urlpattern)
+	}
+
+	// A representative UpdateRole payload includes configuration for the
+	// ports requested.
+	role := &gwacl.PersistentVMRole{}
+	err = role.Deserialize((*record)[2].Payload)
+	c.Assert(err, IsNil)
+	c.Check(
+		*(role.ConfigurationSets[0].InputEndpoints),
+		DeepEquals, []gwacl.InputEndpoint{
+			makeInputEndpoint(79, "tcp"),
+			makeInputEndpoint(587, "tcp"),
+			makeInputEndpoint(9, "udp"),
+		})
+}
+
+func (*StorageSuite) TestOpenPortsFailsWhenUnableToGetServiceProperties(c *C) {
+	service := makeHostedServiceDescriptor("service-name")
+	responses := []gwacl.DispatcherResponse{
+		// GetHostedServiceProperties breaks.
+		gwacl.NewDispatcherResponse(nil, http.StatusInternalServerError, nil),
+	}
+	record := gwacl.PatchManagementAPIResponses(responses)
+	env := makeEnviron(c)
+	azInstance := azureInstance{*service, env}
+
+	err := azInstance.OpenPorts("machine-id", []instance.Port{
+		{"tcp", 79}, {"tcp", 587}, {"udp", 9},
+	})
+
+	c.Check(err, ErrorMatches, "GET request failed [(]500: Internal Server Error[)]")
+	c.Check(*record, HasLen, 1)
+}
+
+func (*StorageSuite) TestOpenPortsFailsWhenUnableToGetRole(c *C) {
+	service := makeHostedServiceDescriptor("service-name")
+	deployments := []gwacl.Deployment{
+		makeDeployment("deployment-one", makeRole("role-one")),
+	}
+	responses := []gwacl.DispatcherResponse{
+		// First, GetHostedServiceProperties
+		gwacl.NewDispatcherResponse(
+			serialize(c, &gwacl.HostedService{
+				Deployments:             deployments,
+				HostedServiceDescriptor: *service,
+				XMLNS: gwacl.XMLNS,
+			}),
+			http.StatusOK, nil),
+		// Second, GetRole fails
+		gwacl.NewDispatcherResponse(
+			nil, http.StatusInternalServerError, nil),
+	}
+	record := gwacl.PatchManagementAPIResponses(responses)
+	env := makeEnviron(c)
+	azInstance := azureInstance{*service, env}
+
+	err := azInstance.OpenPorts("machine-id", []instance.Port{
+		{"tcp", 79}, {"tcp", 587}, {"udp", 9},
+	})
+
+	c.Check(err, ErrorMatches, "GET request failed [(]500: Internal Server Error[)]")
+	c.Check(*record, HasLen, 2)
+}
+
+func (*StorageSuite) TestOpenPortsFailsWhenUnableToUpdateRole(c *C) {
+	service := makeHostedServiceDescriptor("service-name")
+	deployments := []gwacl.Deployment{
+		makeDeployment("deployment-one", makeRole("role-one")),
+	}
+	responses := []gwacl.DispatcherResponse{
+		// First, GetHostedServiceProperties
+		gwacl.NewDispatcherResponse(
+			serialize(c, &gwacl.HostedService{
+				Deployments:             deployments,
+				HostedServiceDescriptor: *service,
+				XMLNS: gwacl.XMLNS,
+			}),
+			http.StatusOK, nil),
+		// Seconds, GetRole
+		gwacl.NewDispatcherResponse(
+			serialize(c, &gwacl.PersistentVMRole{
+				XMLNS:    gwacl.XMLNS,
+				RoleName: "role-one",
+			}),
+			http.StatusOK, nil),
+		// Third, UpdateRole fails
+		gwacl.NewDispatcherResponse(
+			nil, http.StatusInternalServerError, nil),
+	}
+	record := gwacl.PatchManagementAPIResponses(responses)
+	env := makeEnviron(c)
+	azInstance := azureInstance{*service, env}
+
+	err := azInstance.OpenPorts("machine-id", []instance.Port{
+		{"tcp", 79}, {"tcp", 587}, {"udp", 9},
+	})
+
+	c.Check(err, ErrorMatches, "PUT request failed [(]500: Internal Server Error[)]")
+	c.Check(*record, HasLen, 3)
+}