Merge lp://staging/~gz/goose/nts_duplicate_secgroup_rules into lp://staging/goose

Reviewers: mp+147222_code.launchpad.net,

Message:
Please take a look.

Description:
Error on existing secgroup rules in test service

Juju has a test that relies on being able to tell the EC2 to reopen
the same port and ignore the error. To share that test we need some
of the existing rule checking behaviour from nova in the test service.

Most of the diff is just dedent, but this is none of it pretty.

Actually using this for anything useful is harder without stronger
handling of errors from the api.

https://code.launchpad.net/~gz/goose/nts_duplicate_secgroup_rules/+merge/147222

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M testservices/novaservice/service_http.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-20130207052913-e5vfzm0pg3786t9i
+New revision: <email address hidden>

Index: testservices/novaservice/service_http.go
=== modified file 'testservices/novaservice/service_http.go'
--- testservices/novaservice/service_http.go 2013-02-04 15:53:04 +0000
+++ testservices/novaservice/service_http.go 2013-02-07 20:32:29 +0000
@@ -879,23 +879,44 @@
    }
    if err = json.Unmarshal(body, &req); err != nil {
     return err
- } else {
- n.nextRuleId++
- nextId := n.nextRuleId
- err = n.addSecurityGroupRule(nextId, req.Rule)
- if err != nil {
- return err
- }
- rule, err := n.securityGroupRule(nextId)
- if err != nil {
- return err
- }
- var resp struct {
- Rule nova.SecurityGroupRule `json:"security_group_rule"`
- }
- resp.Rule = *rule
- return sendJSON(http.StatusOK, resp, w, r)
- }
+ }
+ inrule := req.Rule
+ group, err := n.securityGroup(inrule.ParentGroupId)
+ if err != nil {
+ return err // TODO: should be a 4XX error with details
+ }
+ for _, r := range group.Rules {
+ // TODO: this logic is actually wrong, not what nova does at all
+ // why are we reimplementing half of nova/api/openstack in go again?
+ if r.IPProtocol != nil && *r.IPProtocol == inrule.IPProtocol &&
+ r.FromPort != nil && *r.FromPort == inrule.FromPort &&
+ r.ToPort != nil && *r.ToPort == inrule.ToPort {
+ // TODO: Use a proper helper and sane error type
+ return &errorResponse{
+ http.StatusBadRequest,
+ fmt.Sprintf(`{"badRequest": {"message": "This rule already exists in
group %d", "code": 400}}`, group.Id),
+ "application/json; charset=UTF-8",
+ "rule already exists",
+ nil,
+ nil,
+ }
+ }
+ }
+ n.nextRuleId++
+ nextId := n.nextRuleId
+ err = n.addSecurityGroupRule(nextId, req.Rule)
+ if err != nil {
+ return err
+ }
+ rule, err := n.securityGroupRule(nextId)
+ if err != nil {
+ return err
+ }
+ var resp struct {
+ Rule nova.SecurityGroupRule `json:"security_group_rule"`
+ }
+ resp.Rule = *rule
+ return sendJSON(http.StatusOK, resp, w, r)
   case "PUT":
    if ruleId := path.Base(r.URL.Path); ruleId != "os-security-group-rules" {
     return errNotFoundJSON

...

LGTM with a couple of trivials.

https://codereview.appspot.com/7301061/diff/1/testservices/novaservice/service_http.go
File testservices/novaservice/service_http.go (right):

https://codereview.appspot.com/7301061/diff/1/testservices/novaservice/service_http.go#newcode886
testservices/novaservice/service_http.go:886: return err // TODO: should
be a 4XX error with details
Why not define an errorResponse for that?

https://codereview.appspot.com/7301061/diff/1/testservices/novaservice/service_http.go#newcode894
testservices/novaservice/service_http.go:894: // TODO: Use a proper
helper and sane error type
I think it's better to use a helper, like:

func ruleExistsError(groupId int) {
     return &errorResponse{...}
}

https://codereview.appspot.com/7301061/

LGTM, so long as my question is clarified.
Plus I agree with Dimiter's comments.

https://codereview.appspot.com/7301061/diff/1/testservices/novaservice/service_http.go
File testservices/novaservice/service_http.go (right):

https://codereview.appspot.com/7301061/diff/1/testservices/novaservice/service_http.go#newcode889
testservices/novaservice/service_http.go:889: // TODO: this logic is
actually wrong, not what nova does at all
I'm a little confused - the logic below is new to this branch, but the
code comment says it's wrong. Should it be done so that it is not wrong?
If it's wrong, are there not tests that should fail? If it needs to be
submitted as is, I'd love the comment to better clarify what's being
done here.

https://codereview.appspot.com/7301061/

LGTM

https://codereview.appspot.com/7301061/diff/1/testservices/novaservice/service_http.go
File testservices/novaservice/service_http.go (right):

https://codereview.appspot.com/7301061/diff/1/testservices/novaservice/service_http.go#newcode889
testservices/novaservice/service_http.go:889: // TODO: this logic is
actually wrong, not what nova does at all
AIUI, there is a test case we want to share with the EC2 code (live
test, probably), which asserts that it can make a request for a security
group, and get back a BadRequest, and then consider that to mean the
security group already exists as it wants it.
I'm not entirely sure about the "not what nova does at all" portion.

When chatting with Martin yesterday about it, it seemed to be that the
test we wanted to share was just overly long and was asserting too many
conditions in a row. So we couldn't just poke out the couple of
assertions that don't actually apply to Openstack.

So instead, we implement a bit that doesn't really match, and then work
to clean up the tests, and then the implementation. I agree that should
be documenting that thought by more than "this logic is wrong".

https://codereview.appspot.com/7301061/diff/1/testservices/novaservice/service_http.go#newcode890
testservices/novaservice/service_http.go:890: // why are we
reimplementing half of nova/api/openstack in go again?
Because we don't want to have to spin up devstack just to test that our
api calls are sane

https://codereview.appspot.com/7301061/

I'll either land this then some cleanup of the error handling or roll
this branch into that.

https://codereview.appspot.com/7301061/diff/1/testservices/novaservice/service_http.go
File testservices/novaservice/service_http.go (right):

https://codereview.appspot.com/7301061/diff/1/testservices/novaservice/service_http.go#newcode886
testservices/novaservice/service_http.go:886: return err // TODO: should
be a 4XX error with details
On 2013/02/08 00:02:16, dimitern wrote:
> Why not define an errorResponse for that?

We have several hundred lines of them at the top of the file already, it
seemed the wrong time to add to that.

https://codereview.appspot.com/7301061/diff/1/testservices/novaservice/service_http.go#newcode889
testservices/novaservice/service_http.go:889: // TODO: this logic is
actually wrong, not what nova does at all
The wrong is that is should really do the equality check for all five
parts, which is then a ten-comparison if statement. Moving to a more
realistic validation step would be nice but is not actually required for
juju tests.

https://codereview.appspot.com/7301061/