Changing util's validation to raise on exceptions #117

insequent · 2016-06-08T03:59:35Z

These validation functions are used by clients such as calico-containers, and when they fail, users may want to know why such validations are failing. With this feature, pre-existing code will continue to work as is, however clients now have the option of returning the error (exception) for further diagnostics.

It feels a touch clunky with the way this is implemented, but I didn't want to break any existing code, and this seemed the simplest way. I'm definitely open to any ideas on alternatives.

CLAassistant · 2016-06-09T16:34:47Z

All committers have signed the CLA.

insequent · 2016-06-09T16:49:24Z

Added logic for version=None in validate_ip(). Seems more useful for it to be able to handle no version and just check if the IP is a valid.

Also, fixed my email to use the one I signed the CLA with.

insequent · 2016-06-09T17:52:02Z

Last update pending review (I promise). I noticed multiple functions were missing complete docstrings for the new return_error param, and one had no param or return at all. Added those in. This was a purely cosmetic update.

matthewdupre · 2016-06-09T23:19:16Z

Thanks for contributing @insequent! I'm sorry we got this wrong from the start. I'll make any detailed comments against the code, but first cover the general form of the validation functions.

I had a chat with some of the team, and we think the least clunky (but still back compatible) approach is probably to rename the functions to verify_, change them to return nothing and raise exceptions when needed, and then add the validate_ functions back in to point to them.

Maybe we could decorate the old validate_ functions to log a deprecation warning, call the new verify_ equivalent and return accordingly?

What do you think?

matthewdupre · 2016-06-09T23:35:01Z

calico_containers/pycalico/util.py


+    if not valid and return_error:
+        err = ValueError("ICMP type is invalid. Value must be between 0 and "
+                         "255 ('{0}' given).".format(icmp_type))


The range includes 0 (but not 255).

matthewdupre · 2016-06-10T00:08:37Z

Detail-wise, I found a couple of off-by-ones in the messages, but otherwise looks great - thanks :).

insequent · 2016-06-10T04:21:21Z

That makes sense and sounds much cleaner. I'll get right on it!

insequent · 2016-06-10T23:27:28Z

This commit should be closer in line with what you're looking for. However, I did take a few liberties this time:

I changed the order of the functions to be alphabetical (easier to find that way)
ICMP Types are now inclusively 0 to 255. I have no qualms changing this back, but it seems some people do use 255 as ICMP type (iptables in particular).
I changed everything I could to satisfy pylint/pep8 (cosmetic changes). The too long lines for MOCK_IP_ADDR and its friends in the test_util.py could not be changed without making the string look ugly, so I left those as is (you could do some trickiness with string concatenation, but the full example is much easier to read altogether).

insequent · 2016-06-10T23:36:03Z

Oh, as to deprecated decorators, I love the idea, but wasn't sure how you all would want them. As you can see in the code, I just put DEPRECATED into the docstrings. Preferably, you would make a custom decorator that logs a warning, however the logging in these modules are super limited (and uses a NullHandler?).

matthewdupre · 2016-06-13T17:48:13Z

calico_containers/pycalico/util.py

-                in_range = False
-        if not in_range:
-            break
+    return return_bool(validate_asn, asn)


Should call verify_, not validate_.

Oh! I didn't even notice there's no unit tests for validate_asn. I'll fix that as well.

insequent · 2016-06-13T20:19:09Z

Added unit tests for validate_asn and verify_asn. These will catch typos like the one I made, swapping the function names on accident. Note that I had to rework the try/except block for verify_asn, because the outer try block was catching all the raises inside the try block (another bug found by the unit tests).

matthewdupre · 2016-06-14T21:50:54Z

calico_containers/pycalico/util.py

+
+    :param cidr: IP addr and associated routing prefix
+    :type cidr: str
+    :return: True if valid IP, False if invalid


Should be returns None.

This comment is invalid now, although Github's page doesn't seem to think so. Thought it was my browser caching it, but it shows the same way in others. shrug

I guess the comment might have moved from verify_ to validate_ as the diff changed underneath it? Either that or it was just a bogus comment in the first place.

Weird, but you're right - it's definitely fixed.

matthewdupre · 2016-06-14T22:36:06Z

OK - I promise this is the last round of comments! Thanks for a very comprehensive PR!

insequent · 2016-06-16T02:24:50Z

Whew! Ok, so I took things even further, so definitely double check me.

Made specific custom exceptions
Changed (verify|validate)_hostname to allow IPv6 hostnames. This is kinda a big one, so definitely let me know if you want me to change that back.
Changed LOG to _log. I may make a PR for the whole module at some point, unless you all just really like the current name (technically a pep8 violation).
Changed return_bool to _return_bool.
Changed try blocks to be as specific as possible. This is what I originally thought you were talking about @matthewdupre, not just _return_bool (oops).

matthewdupre · 2016-06-17T00:02:03Z

calico_containers/pycalico/util.py

+        raise RangeValidationError(err_mess)
+
+    # NOTE: The real limit in DNS is 255 octets (253 chars) or 254 chars
+    #       if you include the root domain (i.e. a period on the end). RFC1035


matthewdupre · 2016-06-17T00:43:21Z

One remaining thing - I'd like a single custom base class, so people can just catch everything, regardless of what validation we add under the covers.
I'm OK with taking this, although I'm not 100% on how it would work in practice (1::2 vs 1::0:2 etc.),
You're right, but I'm not sure it's worth making the change. I'll ask the team about pointing a linter at PRs though (maybe https://github.com/markstory/lint-review or something?).
Sorry if I wasn't clear!

insequent · 2016-06-17T01:40:37Z

That makes perfect sense. I'll add single base class.

To retain compatability with legacy code, a new method was created with the name changed from 'validate_<object>' to 'verify_<object>'

insequent · 2016-06-17T14:32:05Z

The catch-all exception class is now ValidationError.

matthewdupre · 2016-06-23T02:06:47Z

Sorry it took me so long to get back to this: all looks great now.

Thanks for the contribution :)

insequent force-pushed the master branch from 7282d0a to 4c33449 Compare June 8, 2016 04:23

fasaxc assigned matthewdupre Jun 9, 2016

fasaxc added the kind/enhancement label Jun 9, 2016

insequent force-pushed the master branch from 4c33449 to 17ca5b2 Compare June 9, 2016 16:34

insequent force-pushed the master branch from 17ca5b2 to a776f6f Compare June 9, 2016 16:40

insequent force-pushed the master branch from a776f6f to 08d7434 Compare June 9, 2016 17:49

matthewdupre reviewed Jun 9, 2016
View reviewed changes

insequent force-pushed the master branch 2 times, most recently from 590c9a6 to c95cb34 Compare June 10, 2016 23:23

matthewdupre reviewed Jun 13, 2016
View reviewed changes

insequent force-pushed the master branch from c95cb34 to 33311f6 Compare June 13, 2016 20:15

matthewdupre reviewed Jun 14, 2016
View reviewed changes

insequent force-pushed the master branch 3 times, most recently from a6ed38f to b9f58a5 Compare June 16, 2016 02:18

insequent changed the title ~~Adding optional error return on validation functions~~ Changing util's validation to raise on exceptions Jun 16, 2016

matthewdupre reviewed Jun 17, 2016
View reviewed changes

Changing util's validation to raise on exceptions

9097240

To retain compatability with legacy code, a new method was created with the name changed from 'validate_<object>' to 'verify_<object>'

insequent force-pushed the master branch from b9f58a5 to 9097240 Compare June 17, 2016 14:27

matthewdupre merged commit 78092b3 into projectcalico:master Jun 23, 2016

ozdanborne mentioned this pull request Jun 27, 2016

Bumped ICMP validation test to use 256 projectcalico/calicoctl#1029

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changing util's validation to raise on exceptions #117

Changing util's validation to raise on exceptions #117

insequent commented Jun 8, 2016

CLAassistant commented Jun 9, 2016 •

edited

Loading

insequent commented Jun 9, 2016 •

edited

Loading

insequent commented Jun 9, 2016

matthewdupre commented Jun 9, 2016

matthewdupre Jun 9, 2016

matthewdupre commented Jun 10, 2016

insequent commented Jun 10, 2016

insequent commented Jun 10, 2016 •

edited

Loading

insequent commented Jun 10, 2016 •

edited

Loading

matthewdupre Jun 13, 2016

insequent Jun 13, 2016 •

edited

Loading

insequent commented Jun 13, 2016 •

edited

Loading

matthewdupre Jun 14, 2016

insequent Jun 16, 2016

matthewdupre Jun 16, 2016

matthewdupre commented Jun 14, 2016

insequent commented Jun 16, 2016 •

edited

Loading

matthewdupre Jun 17, 2016

matthewdupre commented Jun 17, 2016

insequent commented Jun 17, 2016

insequent commented Jun 17, 2016

matthewdupre commented Jun 23, 2016

Changing util's validation to raise on exceptions #117

Changing util's validation to raise on exceptions #117

Conversation

insequent commented Jun 8, 2016

CLAassistant commented Jun 9, 2016 • edited Loading

insequent commented Jun 9, 2016 • edited Loading

insequent commented Jun 9, 2016

matthewdupre commented Jun 9, 2016

matthewdupre Jun 9, 2016

Choose a reason for hiding this comment

matthewdupre commented Jun 10, 2016

insequent commented Jun 10, 2016

insequent commented Jun 10, 2016 • edited Loading

insequent commented Jun 10, 2016 • edited Loading

matthewdupre Jun 13, 2016

Choose a reason for hiding this comment

insequent Jun 13, 2016 • edited Loading

Choose a reason for hiding this comment

insequent commented Jun 13, 2016 • edited Loading

matthewdupre Jun 14, 2016

Choose a reason for hiding this comment

insequent Jun 16, 2016

Choose a reason for hiding this comment

matthewdupre Jun 16, 2016

Choose a reason for hiding this comment

matthewdupre commented Jun 14, 2016

insequent commented Jun 16, 2016 • edited Loading

matthewdupre Jun 17, 2016

Choose a reason for hiding this comment

matthewdupre commented Jun 17, 2016

insequent commented Jun 17, 2016

insequent commented Jun 17, 2016

matthewdupre commented Jun 23, 2016

CLAassistant commented Jun 9, 2016 •

edited

Loading

insequent commented Jun 9, 2016 •

edited

Loading

insequent commented Jun 10, 2016 •

edited

Loading

insequent commented Jun 10, 2016 •

edited

Loading

insequent Jun 13, 2016 •

edited

Loading

insequent commented Jun 13, 2016 •

edited

Loading

insequent commented Jun 16, 2016 •

edited

Loading