Go OpenStack Exchange

Merge lp://staging/~jameinel/goose/security-group-rule-group-id-1226996 into lp://staging/goose

  1. security-group-rule-group-id-1226996
  2. Merge into trunk
Proposed by John A Meinel
Status: Merged
Approved by: John A Meinel
Approved revision: 119
Merged at revision: 118
Proposed branch: lp://staging/~jameinel/goose/security-group-rule-group-id-1226996
Merge into: lp://staging/goose
Diff against target: 62 lines (+12/-5)
2 files modified
testservices/novaservice/service.go (+7/-0)
testservices/novaservice/service_test.go (+5/-5)
To merge this branch: bzr merge lp://staging/~jameinel/goose/security-group-rule-group-id-1226996
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+186500@code.staging.launchpad.net

Commit message

panic if we get bad requests

It turns out that if you don't supply CIDR or GroupId Openstack just
sets CIDR to 0.0.0.0/0. We don't want to rely on that behavior, so
this just changes the goose test double to panic if it got that
situation.

This can't land until we land my updated Juju branch (or the test
suite will appropriately fail with panic().)

https://codereview.appspot.com/13781043/

Description of the change

panic if we get bad requests

It turns out that if you don't supply CIDR or GroupId Openstack just
sets CIDR to 0.0.0.0/0. We don't want to rely on that behavior, so
this just changes the goose test double to panic if it got that
situation.

This can't land until we land my updated Juju branch (or the test
suite will appropriately fail with panic().)

https://codereview.appspot.com/13781043/

To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :

Reviewers: mp+186500_code.launchpad.net,

Message:
Please take a look.

Description:
panic if we get bad requests

It turns out that if you don't supply CIDR or GroupId Openstack just
sets CIDR to 0.0.0.0/0. We don't want to rely on that behavior, so
this just changes the goose test double to panic if it got that
situation.

This can't land until we land my updated Juju branch (or the test
suite will appropriately fail with panic().)

https://code.launchpad.net/~jameinel/goose/security-group-rule-group-id-1226996/+merge/186500

(do not edit description out of merge proposal)

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

Affected files (+9, -0 lines):
   A [revision details]
   M testservices/novaservice/service.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-20130910084634-qu05mqby3hkd6jp1
+New revision: <email address hidden>

Index: testservices/novaservice/service.go
=== modified file 'testservices/novaservice/service.go'
--- testservices/novaservice/service.go 2013-09-04 09:50:54 +0000
+++ testservices/novaservice/service.go 2013-09-18 11:05:12 +0000
@@ -459,6 +459,13 @@
     TenantId: sourceGroup.TenantId,
     Name: sourceGroup.Name,
    }
+ } else if rule.Cidr == "" {
+ // http://pad.lv/1226996
+ // It seems that if you don't supply Cidr or GroupId then
+ // Openstack treats the Cidr as 0.0.0.0/0
+ // However, since that is not clearly specified we just panic()
+ // because we don't want to rely on that behavior
+ panic(fmt.Sprintf("Neither Cidr nor GroupId are set for this
SecurityGroup Rule: %v", rule))
   }
   if rule.FromPort != 0 {
    newrule.FromPort = &rule.FromPort

Revision history for this message
Martin Packman (gz) wrote :

LGTM.

https://codereview.appspot.com/13781043/

Revision history for this message
Go Bot (go-bot) wrote :
Download full text (9.4 KiB)

The attempt to merge lp:~jameinel/goose/security-group-rule-group-id-1226996 into lp:goose failed. Below is the output from the failed tests.

ok launchpad.net/goose 0.013s
ok launchpad.net/goose/client 0.108s
ok launchpad.net/goose/errors 0.012s
ok launchpad.net/goose/glance 0.019s
ok launchpad.net/goose/http 0.072s
ok launchpad.net/goose/identity 0.028s
ok launchpad.net/goose/nova 0.095s
ok launchpad.net/goose/swift 0.062s
ok launchpad.net/goose/sync 0.018s
ok launchpad.net/goose/testing/envsuite 0.011s
ok launchpad.net/goose/testing/httpsuite 0.069s
? launchpad.net/goose/testservices [no test files]
? launchpad.net/goose/testservices/cmd [no test files]
ok launchpad.net/goose/testservices/hook 0.011s
ok launchpad.net/goose/testservices/identityservice 0.023s
total: 58

----------------------------------------------------------------------
PANIC: service_test.go:642: NovaSuite.TestAddHasRemoveSecurityGroupRule

... Panic: Neither Cidr nor GroupId are set for this SecurityGroup Rule: { 0 0 <nil> 1} (PC=0x413B51)

/usr/lib/go/src/pkg/runtime/panic.c:229
  in panic
service.go:477
  in Nova.addSecurityGroupRule
service_test.go:651
  in NovaSuite.TestAddHasRemoveSecurityGroupRule

----------------------------------------------------------------------
FAIL: service_test.go:862: NovaSuite.TestAddHasRemoveServerSecurityGroup

service_test.go:866:
    s.ensureNoGroup(c, group)
service_test.go:40:
    c.Assert(err, ErrorMatches, fmt.Sprintf("no such security group %s", group.Id))
... value = nil
... regex string = "no such security group 1"
... Error value is nil

----------------------------------------------------------------------
FAIL: service_test.go:546: NovaSuite.TestAddRemoveSecurityGroup

service_test.go:548:
    s.createGroup(c, group)
service_test.go:40:
    c.Assert(err, ErrorMatches, fmt.Sprintf("no such security group %s", group.Id))
... value = nil
... regex string = "no such security group 1"
... Error value is nil

----------------------------------------------------------------------
FAIL: service_test.go:816: NovaSuite.TestAddSecurityGroupRuleKeepsNegativePort

service_test.go:822:
    s.createGroup(c, group)
service_test.go:40:
    c.Assert(err, ErrorMatches, fmt.Sprintf("no such security group %s", group.Id))
... value = nil
... regex string = "no such security group 1"
... Error value is nil

----------------------------------------------------------------------
FAIL: service_test.go:752: NovaSuite.TestAddSecurityGroupRuleToParentTwiceFails

service_test.go:759:
    s.createGroup(c, group)
service_test.go:40:
    c.Assert(err, ErrorMatches, fmt.Sprintf("no such security group %s", group.Id))
... value = nil
... regex string = "no such security group 1"
... Error value is nil

----------------------------------------------------------------------
FAIL: service_test.go:738: NovaSuite.TestAddSecurityGroupRuleTwiceFails

service_test.go:740:
    s.createGroup(c, group)
service_test.go:40:
    c.Assert(err, ErrorMatches, fmt.Sprintf("no such security group %s", group.Id))
... value = nil
... regex string = "no such security group 1"
... Error value is nil

----------------------------------------...

Read more...

Revision history for this message
Go Bot (go-bot) wrote :
Download full text (9.4 KiB)

The attempt to merge lp:~jameinel/goose/security-group-rule-group-id-1226996 into lp:goose failed. Below is the output from the failed tests.

ok launchpad.net/goose 0.011s
ok launchpad.net/goose/client 0.107s
ok launchpad.net/goose/errors 0.011s
ok launchpad.net/goose/glance 0.022s
ok launchpad.net/goose/http 0.060s
ok launchpad.net/goose/identity 0.029s
ok launchpad.net/goose/nova 0.104s
ok launchpad.net/goose/swift 0.057s
ok launchpad.net/goose/sync 0.010s
ok launchpad.net/goose/testing/envsuite 0.019s
ok launchpad.net/goose/testing/httpsuite 0.068s
? launchpad.net/goose/testservices [no test files]
? launchpad.net/goose/testservices/cmd [no test files]
ok launchpad.net/goose/testservices/hook 0.011s
ok launchpad.net/goose/testservices/identityservice 0.025s
total: 58

----------------------------------------------------------------------
PANIC: service_test.go:642: NovaSuite.TestAddHasRemoveSecurityGroupRule

... Panic: Neither Cidr nor GroupId are set for this SecurityGroup Rule: { 0 0 <nil> 1} (PC=0x413B51)

/usr/lib/go/src/pkg/runtime/panic.c:229
  in panic
service.go:477
  in Nova.addSecurityGroupRule
service_test.go:651
  in NovaSuite.TestAddHasRemoveSecurityGroupRule

----------------------------------------------------------------------
FAIL: service_test.go:862: NovaSuite.TestAddHasRemoveServerSecurityGroup

service_test.go:866:
    s.ensureNoGroup(c, group)
service_test.go:40:
    c.Assert(err, ErrorMatches, fmt.Sprintf("no such security group %s", group.Id))
... value = nil
... regex string = "no such security group 1"
... Error value is nil

----------------------------------------------------------------------
FAIL: service_test.go:546: NovaSuite.TestAddRemoveSecurityGroup

service_test.go:548:
    s.createGroup(c, group)
service_test.go:40:
    c.Assert(err, ErrorMatches, fmt.Sprintf("no such security group %s", group.Id))
... value = nil
... regex string = "no such security group 1"
... Error value is nil

----------------------------------------------------------------------
FAIL: service_test.go:816: NovaSuite.TestAddSecurityGroupRuleKeepsNegativePort

service_test.go:822:
    s.createGroup(c, group)
service_test.go:40:
    c.Assert(err, ErrorMatches, fmt.Sprintf("no such security group %s", group.Id))
... value = nil
... regex string = "no such security group 1"
... Error value is nil

----------------------------------------------------------------------
FAIL: service_test.go:752: NovaSuite.TestAddSecurityGroupRuleToParentTwiceFails

service_test.go:759:
    s.createGroup(c, group)
service_test.go:40:
    c.Assert(err, ErrorMatches, fmt.Sprintf("no such security group %s", group.Id))
... value = nil
... regex string = "no such security group 1"
... Error value is nil

----------------------------------------------------------------------
FAIL: service_test.go:738: NovaSuite.TestAddSecurityGroupRuleTwiceFails

service_test.go:740:
    s.createGroup(c, group)
service_test.go:40:
    c.Assert(err, ErrorMatches, fmt.Sprintf("no such security group %s", group.Id))
... value = nil
... regex string = "no such security group 1"
... Error value is nil

----------------------------------------...

Read more...

lp://staging/~jameinel/goose/security-group-rule-group-id-1226996 updated
119. By John A Meinel

Merge the original branch, since we missed it by accident

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches