Merge lp://staging/~wallyworld/goose/quantum-security-group-uuid into lp://staging/goose

Reviewers: mp+171187_code.launchpad.net,

Message:
Please take a look.

Description:
Support string id for SecurityGroup et al

Openstack with Quantum/Grizzly uses string ids for SecurityGroup
and other similar data structures. Goose was written for deployments
that use int ids. This branch fixes that issue using the pattern
already in palce to deal with HP Cloud vs Canonistack for
ServerDetail et al ids.

https://code.launchpad.net/~wallyworld/goose/quantum-security-group-uuid/+merge/171187

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M nova/json.go
   M nova/json_test.go
   M nova/nova.go
   M nova/nova_test.go
   M testservices/novaservice/service.go
   M testservices/novaservice/service_http.go
   M testservices/novaservice/service_http_test.go
   M testservices/novaservice/service_test.go
   M tools/secgroup-delete-all/main_test.go

Some thoughts about how we can avoid all of the extra types just to
handle a different named Id attribute.

https://codereview.appspot.com/10527043/diff/1/nova/json.go
File nova/json.go (right):

https://codereview.appspot.com/10527043/diff/1/nova/json.go#newcode254
nova/json.go:254: func (securityGroupRule SecurityGroupRule)
MarshalJSON() ([]byte, error) {
I thought that in MarshalJSON we would need to preserve the type of Id
that we got. It didn't matter for InstanceId because that was in a URL
part, so it would always be 'string', but for identifiers that end up in
the JSON content we have to match types or Openstack is uphappy.

Here you are just using 'convertId' which is just using the global
'treat it as an int or a string'. Which works for test suites, but it
seems like it would accidentally work sometimes. (we get a numeric id
from Rackspace, but end up sending it back as a string).

So it feels like your patch perfectly addresses the Unmarshall case (get
something from remote that we can internally handle), but it doesn't
seem to save whether we read an int or a string for us to handle round
tripping the Id back to the service.

https://codereview.appspot.com/10527043/diff/1/nova/json.go#newcode274
nova/json.go:274: }
I'm a little unhappy about the proliferation of types with identical
String functions.

Can we either unmarshal into a map and then pull the right attribute
out? Or maybe just have a base:

type commonId interface {
   rawId() interface{}
}

func ToString(id commonId) string {
   rawId := commonId.rawId()
   if rawId == nil {
     return ""
   }

   if fid, ok := rawId.(float64); ok {
     return fmt.Sprint(int(fid))
   }
   return fmt.Sprint(rawId)
}

type genericGroupId struct {
   GroupId interface{} `json:"group_id"`
}

func (id genericGroupId) RawId() interface { return id.GroupId }

And then in the UnmarshalJSON code, we can do:

var gid genericGroupId
if err := json.Unmarshal(b, &gid); err != nil {
     return err
}
jri.GroupId = ToString(gid)

I'm thinking, though, that a common helper that used maps internally
would let you do:

func GetIdAsString(b []byte, name string) (string, error) {
     var out map[string]interface{}
     if err := json.Unmarshal(b, &out); err != nil {
        return "", err
     }
     if val, ok := out[name]; !ok {
       return ????
     } else {
        if floatVal, ok := val.(float64); ok {
           return fmt.Sprint(int(floatVal))
        } else {
           return fmt.Sprint(floatVal)
        }
     }
}

jri.GroupId, err = GetIdAsString(b, "group_id")

https://codereview.appspot.com/10527043/

Please take a look.

https://codereview.appspot.com/10527043/diff/1/nova/json.go
File nova/json.go (right):

https://codereview.appspot.com/10527043/diff/1/nova/json.go#newcode254
nova/json.go:254: func (securityGroupRule SecurityGroupRule)
MarshalJSON() ([]byte, error) {
On 2013/06/25 13:41:38, jameinel wrote:
> I thought that in MarshalJSON we would need to preserve the type of Id
that we
> got. It didn't matter for InstanceId because that was in a URL part,
so it would
> always be 'string', but for identifiers that end up in the JSON
content we have
> to match types or Openstack is uphappy.

> Here you are just using 'convertId' which is just using the global
'treat it as
> an int or a string'. Which works for test suites, but it seems like it
would
> accidentally work sometimes. (we get a numeric id from Rackspace, but
end up
> sending it back as a string).

> So it feels like your patch perfectly addresses the Unmarshall case
(get
> something from remote that we can internally handle), but it doesn't
seem to
> save whether we read an int or a string for us to handle round
tripping the Id
> back to the service.

tl;dr; The implementation seems to work. Live tests pass against
Canonistack as well as using juju to deploy and expose a charm. The
Landscape guys confirmed the fix works for them also.

The compute API docs indicate that ids are received as ints but can be
sent as strings. See http://api.openstack.org/api-ref.html#compute-ext.
The create security group rule operation is documented as expecting the
following json:

Request: JSON
{
     "security_group_rule": {
         "ip_protocol": "tcp",
         "from_port": "80",
         "to_port": "8080",
         "cidr": "0.0.0.0/0",
         "group_id": "",
         "parent_group_id": "161"
     }
}

Response: JSON
{
     "security_group_rule": {
         "from_port": 80,
         "group": {},
         "ip_protocol": "tcp",
         "to_port": 8080,
         "parent_group_id": 161,
         "ip_range": {
             "cidr": "0.0.0.0/0"
         },
         "id": 218
     }
}

See how parent_group_id is sent as "161" but received as 161. The
implementation is consistent with this behaviour. ie we accept strings
or ints when reading but write strings. The docs also say that versions
1.1 and 2.0 of the API are identical.

The convertId() method is just for tests, to control whether the test
services send ints or strings.

I haven't checked the Openstack source; I can only assume it is also
tolerant of what is read in and accepts either int/string for those
versions where ids are int.

https://codereview.appspot.com/10527043/diff/1/nova/json.go#newcode274
nova/json.go:274: }
On 2013/06/25 13:41:38, jameinel wrote:
> I'm a little unhappy about the proliferation of types with identical
String
> functions.

Fair enough. It was 2 but with this branch is now 4 which seems like the
right time to refactor.

<snip>

I implemented the generic map based solution.

https://codereview.appspot.com/10527043/

LGTM

https://codereview.appspot.com/10527043/