Merge lp://qastaging/~u-matt-h/nova/aws-api-validation into lp://qastaging/~hudson-openstack/nova/trunk

This looks great. Don't see any pressure to get it in before d4 as it is definitely cleanup-related.

Is there a reason that you are validating only string values, and not unicode with the validate_str method? Are non-ASCII values prohibited in urls?

Also, it would be better to use:

if not isinstance(val, str):
(or even: if not isinstance(val, basestring): if unicode should be allowed)

... instead of:
if type(val) != str

> Is there a reason that you are validating only string values, and not unicode
> with the validate_str method? Are non-ASCII values prohibited in urls?
URLs may only contain a subset of ascii characters

>
> Also, it would be better to use:
>
> if not isinstance(val, str):
> (or even: if not isinstance(val, basestring): if unicode should be allowed)
>
> ... instead of:
> if type(val) != str

good feedback. I've incorporated this into the patch.

Hi matt. Have you merged this with the latest nova trunk (with the new NoAuth changes)?

==> /var/log/nova/nova-api.log <==
(nova): TRACE: File "/usr/lib/pymodules/python2.6/paste/deploy/loadwsgi.py", line 503, in _pipeline_app_context
(nova): TRACE: for name in pipeline[:-1]]
(nova): TRACE: File "/usr/lib/pymodules/python2.6/paste/deploy/loadwsgi.py", line 413, in get_context
(nova): TRACE: section)
(nova): TRACE: File "/usr/lib/pymodules/python2.6/paste/deploy/loadwsgi.py", line 458, in _context_from_explicit
(nova): TRACE: value = import_string(found_expr)
(nova): TRACE: File "/usr/lib/pymodules/python2.6/paste/deploy/loadwsgi.py", line 18, in import_string
(nova): TRACE: return pkg_resources.EntryPoint.parse("x="+s).load(False)
(nova): TRACE: File "/usr/lib/python2.6/dist-packages/pkg_resources.py", line 1959, in load
(nova): TRACE: raise ImportError("%r has no %r attribute" % (entry,attr))
(nova): TRACE: ImportError: <module 'nova.api.ec2' from '/usr/lib/pymodules/python2.6/nova/api/ec2/__init__.pyc'> has no 'NoAuth' attribute

> Hi matt. Have you merged this with the latest nova trunk (with the new NoAuth
> changes)?
>
> ==> /var/log/nova/nova-api.log <==
> (nova): TRACE: File "/usr/lib/pymodules/python2.6/paste/deploy/loadwsgi.py",
> line 503, in _pipeline_app_context
> (nova): TRACE: for name in pipeline[:-1]]
> (nova): TRACE: File "/usr/lib/pymodules/python2.6/paste/deploy/loadwsgi.py",
> line 413, in get_context
> (nova): TRACE: section)
> (nova): TRACE: File "/usr/lib/pymodules/python2.6/paste/deploy/loadwsgi.py",
> line 458, in _context_from_explicit
> (nova): TRACE: value = import_string(found_expr)
> (nova): TRACE: File "/usr/lib/pymodules/python2.6/paste/deploy/loadwsgi.py",
> line 18, in import_string
> (nova): TRACE: return pkg_resources.EntryPoint.parse("x="+s).load(False)
> (nova): TRACE: File "/usr/lib/python2.6/dist-packages/pkg_resources.py",
> line 1959, in load
> (nova): TRACE: raise ImportError("%r has no %r attribute" % (entry,attr))
> (nova): TRACE: ImportError: <module 'nova.api.ec2' from
> '/usr/lib/pymodules/python2.6/nova/api/ec2/__init__.pyc'> has no 'NoAuth'
> attribute

Hi Dan,

Thanks for catching this. I did merge from trunk (and run tests), but new entries were added to nova/api-paste.ini which needed to be updated to reflect the new nova/api/ec2/web.py path

This is going to be very long, I apologize.

Why did you add an inner function to your validate_X() functions instead of doing something like the following:
    validate_str(val, max_length=None):
       …
and
    validate_int(val, max_size=None):
       …

In your validate_url_path function, why aren't you using urlparse to parse the url instead of doing it manually?

Also, we should not be assuming that the strings being passed around will be ASCII strings instead of unicode strings, as most of the stack already presupposes unicode strings. The WSGI server will be passing in strings as python unicode strings (u'foo' versus 'foo'), so your string validation functions will fail[1]. To fix that compare against basestring with isinstance(val, basestring). You'll also want to update your tests to reflect that.

The previous issue highlights the need for a working functional test of what you've written, because that would have been caught by trying use the api. The smokestack app on github has a lot of work being done on it to build functional tests for nova, but in the absence of something like that I'd suggest writing a script that just calls the euca tools or one that uses the boto library directly to talk to the api.

If you want to ensure they're only in the ASCII range contact novas0x2a, he has a way of doing it that he can't recall right now.

Also in your tests, you're using ec2_web to refer to ec2.web. I think it's more clear to import ec2.web in addition to ec2 and use it directly instead of adding another identifier.

from nova.api import ec2
from nova.api.ec2 import web

instead of:

from nova.api.ec2 import web as ec2_web

[1]

>>> isinstance(u'foo', str)
False
>>> u'foo' == 'foo'
True
>>>

> This is going to be very long, I apologize.

No problem. I aprreciate you taking the time to review it

> Why did you add an inner function to your validate_X() functions instead of
> doing something like the following:
> validate_str(val, max_length=None):
> …
> and
> validate_int(val, max_size=None):
> …

The validate function takes a list of callables. validate_str/int act as factories, returning validators which use an earlier specified max_lenght/size. If I were to put that value in the outer function, I would need to use functools.partial, which I think is the wrong abstraction.

> In your validate_url_path function, why aren't you using urlparse to parse the
> url instead of doing it manually?
urlparse is good for parsing URLs, but we've got a path. urlparse would, in effect, perform absolutely no validation. It was difficult to decide which logic to use to validate the path, and so I settled on replicating how the url_paths are actually used.

>
> Also, we should not be assuming that the strings being passed around will be
> ASCII strings instead of unicode strings, as most of the stack already
> presupposes unicode strings. The WSGI server will be passing in strings as
> python unicode strings (u'foo' versus 'foo'), so your string validation
> functions will fail[1]. To fix that compare against basestring with
> isinstance(val, basestring). You'll also want to update your tests to reflect
> that.

That's fair. I will modify the test to accept strings or unicode, and to validate that the input only contains ascii characters.

> The previous issue highlights the need for a working functional test of what
> you've written, because that would have been caught by trying use the api. The
> smokestack app on github has a lot of work being done on it to build
> functional tests for nova, but in the absence of something like that I'd
> suggest writing a script that just calls the euca tools or one that uses the
> boto library directly to talk to the api.

I agree. without a fully working, installed environment, however, this seems unlikely. I will do more research.

> If you want to ensure they're only in the ASCII range contact novas0x2a, he
> has a way of doing it that he can't recall right now.

> Also in your tests, you're using ec2_web to refer to ec2.web. I think it's
> more clear to import ec2.web in addition to ec2 and use it directly instead of
> adding another identifier.

fair enough. My feeling was that 'web' is too ambiguous, but maybe now.

> from nova.api import ec2
> from nova.api.ec2 import web
>
> instead of:
>
> from nova.api.ec2 import web as ec2_web
>
>
>
> [1]
>
> >>> isinstance(u'foo', str)
> False
> >>> u'foo' == 'foo'
> True
> >>>

Thanks again for the feedback. I'll make a couple of changes and push it back.

Hi Matthew,

Your way of validating ascii strings actually includes characters like é from the extended-ascii set (128-255), which I'm pretty sure aren't valid in EC2 names.

You should probably do something like this instead:

import string

def validate_ascii(val):
    # ... your stuff here.

    for c in val:
        if not c in string.printable:
            return False

It's either that or something like:

def validate_ascii(val):
    # ... your stuff here.

    for c in val:
        ord_c = ord(c)
        # Only chars in the ASCII printable range
        if ord_c < 32 or ord_c > 127:
            return False

As for the tests, I suggested that you use ec2.web instead of just web or ec2_web doing:

from nova.api import ec2
from nova.api.ec2 import web

Then you can use it with:

ec2.web.Authorizer(...)

Also, your DEFAULT_VALIDATORS should really be a dict instead of a list of tuples. You can iterate across them in the same way you're using it with DEFAULT_VALIDATORS.items().

Hi Matt,

Just one more fix to make your code more pythonix, then it should be good:
1174 +DEFAULT_VALIDATOR = {
1175 + 'instance_id': (validate_ec2_id,),
           ...
1186 +}

Shouldn't have a tuple value and should be:

DEFAULT_VALIDATOR = {
    'instance_id': validate_ec2_id,
    ...
}

Which would make your validate function look like this:

for key, value in validator.items():
    if not key in args:
        continue

    assert callable(f)
    if not f(args[key]):
        ...

instead of:

1210 + for key in validator:
1211 + if key not in args:
1212 + continue
1213 +
1214 + for f in validator[key]:
1215 + assert callable(f)
1216 +
1217 + if not f(args[key]):
1218 + msg = "%s with value %s failed validator %s" % (
1219 + key, args[key], f.__name__)
1220 + LOG.debug(_(msg))
1221 + return False
1222 + return True

> Hi Matt,
>
> Just one more fix to make your code more pythonix, then it should be good:

Hi Christopher,

Thanks again for the feedback. Although my patch doesn't exhibit any instances of it, the idea is that validators could be composable. You could conceivably want to chain, for example, a url path validator and a string validator with a max length value.