Merge lp:~gz/goose/novaservice_preserve_negative_ports into lp:goose

[Martin Packman]

Preserve negative ports on novaservice

Preserve negative ports on novaservice

Creating a security group rule with the value -1 for the ports has a special
meaning in some cases, the test service needs to preserve these values rather
than treating it as the ports not having been set.

https://codereview.appspot.com/8796045/

[Martin Packman (gz)]

Reviewers: mp+159519_code.launchpad.net,

Message:
Please take a look.

Description:
Preserve negative ports on novaservice

Creating a security group rule with the value -1 for the ports has a
special
meaning in some cases, the test service needs to preserve these values
rather
than treating it as the ports not having been set.

https://code.launchpad.net/~gz/goose/novaservice_preserve_negative_ports/+merge/159519

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M testservices/novaservice/service.go
   M testservices/novaservice/service_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-20130328125516-y1zpoibtfvl873gg
+New revision: <email address hidden>

Index: testservices/novaservice/service.go
=== modified file 'testservices/novaservice/service.go'
--- testservices/novaservice/service.go 2013-02-25 07:16:32 +0000
+++ testservices/novaservice/service.go 2013-04-17 22:27:24 +0000
@@ -458,10 +458,10 @@
     Name: sourceGroup.Name,
    }
   }
- if rule.FromPort > 0 {
+ if rule.FromPort != 0 {
    newrule.FromPort = &rule.FromPort
   }
- if rule.ToPort > 0 {
+ if rule.ToPort != 0 {
    newrule.ToPort = &rule.ToPort
   }
   if rule.IPProtocol != "" {

Index: testservices/novaservice/service_test.go
=== modified file 'testservices/novaservice/service_test.go'
--- testservices/novaservice/service_test.go 2013-02-08 17:21:44 +0000
+++ testservices/novaservice/service_test.go 2013-04-17 22:27:24 +0000
@@ -789,6 +789,38 @@
   c.Assert(*gr, DeepEquals, group)
  }

+func (s *NovaSuite) TestAddSecurityGroupRuleKeepsNegativePort(c *C) {
+ group := nova.SecurityGroup{
+ Id: 1,
+ Name: "test",
+ TenantId: s.service.TenantId,
+ }
+ s.createGroup(c, group)
+ defer s.deleteGroup(c, group)
+ ri := nova.RuleInfo{
+ IPProtocol: "icmp",
+ FromPort: -1,
+ ToPort: -1,
+ Cidr: "0.0.0.0/0",
+ ParentGroupId: group.Id,
+ }
+ rule := nova.SecurityGroupRule{
+ Id: 10,
+ ParentGroupId: group.Id,
+ FromPort: &ri.FromPort,
+ ToPort: &ri.ToPort,
+ IPProtocol: &ri.IPProtocol,
+ IPRange: map[string]string{"cidr": "0.0.0.0/0"},
+ }
+ s.ensureNoRule(c, rule)
+ err := s.service.addSecurityGroupRule(rule.Id, ri)
+ c.Assert(err, IsNil)
+ defer s.deleteRule(c, rule)
+ returnedGroup, err := s.service.securityGroup(group.Id)
+ c.Assert(err, IsNil)
+ c.Assert(returnedGroup.Rules, DeepEquals, []nova.SecurityGroupRule{rule})
+}
+
  func (s *NovaSuite) TestRemoveSecurityGroupRuleTwiceFails(c *C) {
   group := nova.SecurityGroup{Id: 1}
   s.createGroup(c, group)

[Ian Booth (wallyworld)]

LGTM

https://codereview.appspot.com/8796045/

[Dimiter Naydenov (dimitern)]

LGTM, ah I knew that, but forgot to bring it up when the original code
landed. Thanks!

https://codereview.appspot.com/8796045/

=== modified file 'testservices/novaservice/service.go'
--- testservices/novaservice/service.go	2013-02-25 07:16:32 +0000
+++ testservices/novaservice/service.go	2013-04-17 22:35:29 +0000
@@ -458,10 +458,10 @@
 			Name:     sourceGroup.Name,
 		}
 	}
-	if rule.FromPort > 0 {
+	if rule.FromPort != 0 {
 		newrule.FromPort = &rule.FromPort
 	}
-	if rule.ToPort > 0 {
+	if rule.ToPort != 0 {
 		newrule.ToPort = &rule.ToPort
 	}
 	if rule.IPProtocol != "" {

=== modified file 'testservices/novaservice/service_test.go'
--- testservices/novaservice/service_test.go	2013-02-08 17:21:44 +0000
+++ testservices/novaservice/service_test.go	2013-04-17 22:35:29 +0000
@@ -789,6 +789,38 @@
 	c.Assert(*gr, DeepEquals, group)
 }
 
+func (s *NovaSuite) TestAddSecurityGroupRuleKeepsNegativePort(c *C) {
+	group := nova.SecurityGroup{
+		Id:       1,
+		Name:     "test",
+		TenantId: s.service.TenantId,
+	}
+	s.createGroup(c, group)
+	defer s.deleteGroup(c, group)
+	ri := nova.RuleInfo{
+		IPProtocol:    "icmp",
+		FromPort:      -1,
+		ToPort:        -1,
+		Cidr:          "0.0.0.0/0",
+		ParentGroupId: group.Id,
+	}
+	rule := nova.SecurityGroupRule{
+		Id:            10,
+		ParentGroupId: group.Id,
+		FromPort:      &ri.FromPort,
+		ToPort:        &ri.ToPort,
+		IPProtocol:    &ri.IPProtocol,
+		IPRange:       map[string]string{"cidr": "0.0.0.0/0"},
+	}
+	s.ensureNoRule(c, rule)
+	err := s.service.addSecurityGroupRule(rule.Id, ri)
+	c.Assert(err, IsNil)
+	defer s.deleteRule(c, rule)
+	returnedGroup, err := s.service.securityGroup(group.Id)
+	c.Assert(err, IsNil)
+	c.Assert(returnedGroup.Rules, DeepEquals, []nova.SecurityGroupRule{rule})
+}
+
 func (s *NovaSuite) TestRemoveSecurityGroupRuleTwiceFails(c *C) {
 	group := nova.SecurityGroup{Id: 1}
 	s.createGroup(c, group)