Config precheck #966

ianballou · 2018-03-01T19:22:46Z

Addresses #45:

Config file is now checked when the HIL server is started
Sections that are not critical to HIL are optional
Option types are checked as well, such as strings vs bools
Tests that didn't include all of the required HIL.cfg sections/options have been updated

Note: I am going to start working on testing, but I wanted to get this out for review first since I'll be out next week.

coveralls · 2018-03-01T20:49:33Z

Pull Request Test Coverage Report for Build 1797

32 of 60 (53.33%) changed or added relevant lines in 8 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+55.6%) to 55.559%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
hil/ext/switches/nexus.py	0	4	0.0%
hil/ext/switches/brocade.py	0	4	0.0%
hil/ext/switches/dell.py	0	4	0.0%
hil/ext/switches/n3000.py	0	4	0.0%
hil/ext/switches/dellnos9.py	0	4	0.0%
hil/config.py	28	36	77.78%

Files with Coverage Reduction	New Missed Lines	%
hil/config.py	1	73.97%

Totals
Change from base Build 1796:	55.6%
Covered Lines:	2049
Relevant Lines:	3688

💛 - Coveralls

xuhang57 · 2018-03-01T21:02:57Z

Officially the first PR which increases the coverage 💟 🍸 🤙

zenhack

A few inlines comments.

Also, we've got a number of things where it might be nice to be more strict; we can probably check that URLs are syntactically valid URLs, and the check the format of the vlan pools, etc.

Down the line it might be nice to swap out some of the lambdas with Use schema, and start using the result of validating the schema, so we don't have to parse out the values again separately.

zenhack · 2018-03-01T21:25:49Z

hil/config.py

+    },
+    Optional('devel'): {
+        Optional('dry_run'):
+            lambda s: string_is_bool(s),


The lambda is superfluous here (and elsewhere); you can just do Optional('dry_run'): string_is_bool. You'll have to move the definition of that function up though, so it's defined before core_schema.

zenhack · 2018-03-01T21:27:15Z

hil/ext/switches/n3000.py

-                                     lambda v: 0 < v and v <= 4093,
-                                     schema.Use(str)),
+            'dummy_vlan': And(Use(int),
+                              lambda v: 0 < v and v <= 4093,


While you're at it, this python supports a shorthand, so this can be 0 < v <= 4093.

Also, is there a reason for 4093? The others are 4096, which makes more sense.

yeah, the Dell N3000 switch only showed that it's from 1 to 4093.

R5-PA-C01-U39#show vlan id ? <1-4093> Enter VLAN ID. R5-PA-C01-U39#show vlan id 4094 ^ Value is out of range. The valid range is 1 to 4093.

Weird. Would you mind adding a comment to that effect?

In network-drivers it's mentioned that we enable VLAN 2-4093, but it doesn't mention the reason. @Izhmash, can you put a comment in the code while you are at it and update the docs to make it more clear?

zenhack · 2018-03-01T21:29:35Z

hil/cli.py

@@ -853,5 +854,7 @@ def main():
            sys.exit('Error: %s\n' % e.message)
        except BadArgumentError as e:
            sys.exit('Error: %s\n' % e.message)
+        except SchemaError as e:
+            sys.exit('Config error: %s\n' % e.message)


I might be slighly more explicit here; Error in config file.

naved001 · 2018-03-02T19:38:04Z

hil/ext/switches/nexus.py

-                                     lambda v: 0 <= v and v <= 4096,
-                                     schema.Use(str)),
+            'dummy_vlan': And(Use(int),
+                              lambda v: 0 <= v and v <= 4096,


while we are talking about vlan ranges, I am sure 0 shouldn't be included in this range.

haas-loaner# show vlan id ? <1-3967,4048-4093> VLAN ID 1-4094 or range(s): 1-5, 10 or 2-5,7-19 > haas-loaner# show vlan id 0

ianballou · 2018-03-13T23:17:41Z

I believe I addressed the comments above. @zenhack for more in-depth config checks, I've added checks for malformed web/db urls, a check for the VLAN pool list, and a check for log_level. Naved and I discussed checking the headnode config options, and we decided they can be left as str for now since they will be removed in the future.

I considered checking log_dir, however a bad directory crashes HIL before before the config is even loaded. I'll add an issue about that.

ianballou · 2018-03-13T23:23:46Z

Tests will be on the way as soon as we lock in which config checks should be there.

naved001 · 2018-03-13T23:46:16Z

hil/config.py

@@ -89,3 +170,4 @@ def setup(filename='hil.cfg'):
    load(filename)
    configure_logging()
    load_extensions()
+    validate_config()


move this before configure_logging() and this PR itself can take care of #976
This line is probably what's causing trouble.

To be more clear, check for bad values of log_dir in validate_config() and call it before configure_logging().

We should probably check this by catching the IOError and reporting the issue; conceptually doing it here is weird, because the problem is an absent or non-writable directory, not a malformed confg file. Slightly more pragmatically, there's a race condition where if the directory is deleted/permissions changed between the check and the use, you could still get the error.

@zenhack I agree that conceptually it's strange to do it here as well. For the scope of this PR, I'm thinking about moving the config validation up and then only checking for a malformed path. The IO check can be done in another small PR.

Sounds good.

zenhack

A number of inline comments.

zenhack · 2018-03-14T21:30:28Z

hil/config.py

+    """Check if a string is a valid list of VLANs"""
+    for r in option.split(","):
+        r = r.strip().split("-")
+        if not all(s.isdigit() for s in r):


you can actually just call isdigit once:

>>> '423'.isdigit() True

Why not actually check the range?

s.isdigit() and 0 < int(s) <= 4096

Or, for that matter, just lift the relevant bit of the schema from the dummy_vlan field.

I'll check the range here, good for a first safety check

Also after I do the r = r.strip().split("-") line, I sometimes end up with a list of strings, so I think I still need to loop through them with all

Ah, you're right, I was thinking you were checking each character.

zenhack · 2018-03-14T21:34:14Z

hil/config.py


 cfg = ConfigParser.RawConfigParser()
 cfg.optionxform = str


+def string_is_bool(option):
+    """Check if a string matches ConfigParser's definition of a bool"""
+    return option.lower() in ['true', 'yes', 'on', '1',


You could be making better use of the library; Use, And, and Or are handy:

return And(Use(option.lower), Or('true', 'yes', 'on', ...))

This can be applied in a number of other places.

For some reason, having this function like this instead just places whatever is in option into my validation schema, causing any config string to pass.

It works however if I put the And(...) directly into the schema

@zenhack how partial are you to using more of these library functions? I've been hacking at it for a while, and returning in this style causes the validation to not happen. From debugging it looks like there's an issue with returning the validation from another function, since it does work when hard-coding into the Schema dictionary.

To use more of the library functions I could instead remove some of the functions (like string_is_bool) and write the validation code right into the schema.

Ah, you probably would need to do something like:

return And(Use(str.lower), Or(...)).validate(option)

I don't feel strongly; if it makes things awkward you can leave them as they are.

Oh, that's what I was missing. That definitely works, but it comes down now to how ugly the error messages look:

Current code error example:

Error in config file: string_is_bool('Frue') should evaluate to True

New suggestion error example:

Error in config file: Or('true', 'yes', 'on', '1', 'false', 'no', 'off', '0') did not validate 'Frue' '0' does not match 'Frue'

I'm not 100% sure why the errors look different now, perhaps because it's a separate validation happening before the main dictionary validation. I think the 2nd error option might expose a little too much code to the user.

I could go either way re: the error message. They're both exposing implementation detail a bit, but the later has the virtue that it actually gives the user a good hint as to what the correct settnigs are.

zenhack · 2018-03-14T21:37:24Z

hil/config.py

+def string_is_db_uri(option):
+    """Check if a string is a valid DB URI"""
+    url = urlparse(option)
+    if not ('postgres' in url.scheme or 'sqlite' in url.scheme):


I don't think we need to use in here; they should be exactly equal.

Alternatively, you could do url.scheme in ('postgres', 'sqlite').

zenhack · 2018-03-14T21:41:52Z

hil/config.py

@@ -89,3 +170,4 @@ def setup(filename='hil.cfg'):
    load(filename)
    configure_logging()
    load_extensions()
+    validate_config()


We should probably check this by catching the IOError and reporting the issue; conceptually doing it here is weird, because the problem is an absent or non-writable directory, not a malformed confg file. Slightly more pragmatically, there's a race condition where if the directory is deleted/permissions changed between the check and the use, you could still get the error.

zenhack · 2018-03-14T21:42:18Z

hil/ext/auth/keystone.py

 from hil.model import Project
 from hil import auth, rest
 import logging
 import sys

 logger = rest.ContextLogger(logging.getLogger(__name__), {})

+core_schema[__name__] = {
+    'auth_url': str,


Could probably check for a valid URL here.

Would be worth tightening the other fields here as well, where possible.

I believe the only field I can check here is the auth_url. I considered auth_protocol at least, but the authentication plugins available can differ depending on what's installed.

xuhang57 · 2018-03-15T17:26:02Z

hil/cli.py

@@ -853,5 +854,7 @@ def main():
            sys.exit('Error: %s\n' % e.message)
        except BadArgumentError as e:
            sys.exit('Error: %s\n' % e.message)
+        except SchemaError as e:
+            sys.exit('Error in config file: %s\n' % e.message)


As we discussed during the weekly meeting. Probably we should just return the e? Since e.message might be `None?

I'd like to do that, yeah. I'm not sure if it's within the scope of this PR though.

xuhang57 · 2018-03-15T17:35:40Z

hil/config.py

+    """Check if a string is a valid web URL"""
+    url = urlparse(option)
+    if url.scheme == '' or url.netloc == '':
+        return False


Curious whether we could just return url.scheme != '' and url.netloc != '' .

I know you may wanna return the False explicitly?

Sure, that works fine.

xuhang57

LGTM, just a note for the e.message may not exist.

ianballou · 2018-03-20T17:52:20Z

Tests are in progress now.

zenhack

A couple more things. Pretty close now.

zenhack · 2018-03-22T23:58:12Z

hil/config.py

+                log_file, when='D', interval=1))
+        except IOError:
+            sys.exit("Error: log directory does not exist or user "
+                     "has insufficient permissions")


Might be good to distinguish between diferent errors:

import errno ... except IOError as e: if e.errno == errno.ENOENT: # no such file or directory elif e.errno == errno.EPERM: ...

zenhack · 2018-03-23T00:53:19Z

hil/config.py


 cfg = ConfigParser.RawConfigParser()
 cfg.optionxform = str


+def string_is_bool(option):
+    """Check if a string matches ConfigParser's definition of a bool"""
+    return option.lower() in ['true', 'yes', 'on', '1',


I could go either way re: the error message. They're both exposing implementation detail a bit, but the later has the virtue that it actually gives the user a good hint as to what the correct settnigs are.

naved001

I'm happy. Just address @zenhack's comments.

ianballou · 2018-03-27T16:58:21Z

I've updated the functions to better use the schema library. I tried to not use lambdas, but I went with them since it seems to be a standard in the schema docs.

I also fixed up the IOError checking. In testing, EACCES was raised rather than EPERM for directory permissions. In case of an unexpected error, I re-raise it so the CLI can deal with it.

zenhack

One thing (caught by travis), but otherwise I'm happy.

zenhack · 2018-03-29T20:45:41Z

tests/unit/config.py

+    opts = ['frue', 'nes', 'yOn', '2', 'Salse', 'Go', 'bff', '3']
+    for s in opts:
+        with pytest.raises(SchemaError):
+            config.string_is_bool(opts)


Think this should config.string_is_bool(s). Yay for linters catching real bugs.

Ah so that's where it was! I was having a hard time figuring that out last week.

…oper VLAN pools

* Minor fixes

* Now catching bad directory IOErrors and reporting them

*Changed config validation funtions to better use schema library *Updated tests to match the updated functions

ianballou · 2018-04-03T16:52:18Z

Everything should be all set now.

zenhack · 2018-04-03T17:23:40Z

LGTM, merging.

ianballou force-pushed the cfg-precheck branch 3 times, most recently from c62ca76 to 696aad8 Compare March 1, 2018 20:30

xuhang57 assigned ianballou Mar 1, 2018

ianballou force-pushed the cfg-precheck branch from 696aad8 to 92d80fd Compare March 1, 2018 21:17

zenhack suggested changes Mar 1, 2018

View reviewed changes

naved001 reviewed Mar 2, 2018

View reviewed changes

xuhang57 added enhancement waiting on submitter labels Mar 7, 2018

ianballou force-pushed the cfg-precheck branch 2 times, most recently from 83db946 to f8ecfb0 Compare March 13, 2018 23:12

ianballou mentioned this pull request Mar 13, 2018

Bad log_dir in hil.cfg crashes server #976

Closed

naved001 reviewed Mar 13, 2018

View reviewed changes

zenhack suggested changes Mar 14, 2018

View reviewed changes

xuhang57 reviewed Mar 15, 2018

View reviewed changes

ianballou added waiting on reviewer and removed waiting on submitter labels Mar 15, 2018

ianballou force-pushed the cfg-precheck branch 2 times, most recently from 0181369 to 3be3cae Compare March 20, 2018 16:20

xuhang57 approved these changes Mar 20, 2018

View reviewed changes

ianballou force-pushed the cfg-precheck branch from 3be3cae to 9212599 Compare March 20, 2018 17:50

ianballou force-pushed the cfg-precheck branch 4 times, most recently from cce5da3 to 9f5d076 Compare March 21, 2018 15:40

zenhack suggested changes Mar 23, 2018

View reviewed changes

naved001 reviewed Mar 23, 2018

View reviewed changes

ianballou force-pushed the cfg-precheck branch 3 times, most recently from 2924671 to 028b23b Compare March 29, 2018 16:12

zenhack suggested changes Mar 29, 2018

View reviewed changes

ianballou force-pushed the cfg-precheck branch from 028b23b to e4321c2 Compare April 3, 2018 15:36

ianballou added 16 commits April 3, 2018 11:38

config checking in-progress

d43c7cf

added config checking for extensions

56da71c

edited test to fit new config requirements

10be644

now marking which config options are optional

a990a9b

headnode config options should not be optional

7185e53

Config errors now expected

ade2dbd

Added config checks for proper web URLs, db URI, proper log level, pr…

da72596

…oper VLAN pools

added note that Dell N3000 has a different VLAN range

d0a5a5a

config checking for proper VLAN pool strings

6d99c12

Removed lambdas from bool check, fixed N3000 VLANs

823a663

checking keystone auth url in config validation

e9046a1

* Now checking for malformed directories

31f0bfd

* Minor fixes

First pass at testing the new config file validation

be60c36

* Now only checking DB URIs for their schemas

4f8547d

* Now catching bad directory IOErrors and reporting them

*Added specific debug directory error checking

f4e2338

*Changed config validation funtions to better use schema library *Updated tests to match the updated functions

removed lambda from dir config checking

6a15b78

ianballou force-pushed the cfg-precheck branch from e4321c2 to 6a15b78 Compare April 3, 2018 15:57

zenhack approved these changes Apr 3, 2018

View reviewed changes

zenhack merged commit 39a8c13 into CCI-MOC:master Apr 3, 2018

zenhack mentioned this pull request Apr 3, 2018

Configuration should be checked on startup. #45

Closed

Config precheck #966

Config precheck #966

Conversation

ianballou commented Mar 1, 2018

coveralls commented Mar 1, 2018 • edited Loading

Pull Request Test Coverage Report for Build 1797

💛 - Coveralls

xuhang57 commented Mar 1, 2018

zenhack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianballou commented Mar 13, 2018

ianballou commented Mar 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zenhack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianballou Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuhang57 left a comment

Choose a reason for hiding this comment

ianballou commented Mar 20, 2018

zenhack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naved001 left a comment

Choose a reason for hiding this comment

ianballou commented Mar 27, 2018

zenhack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianballou Apr 3, 2018 • edited Loading

Choose a reason for hiding this comment

ianballou commented Apr 3, 2018

zenhack commented Apr 3, 2018

coveralls commented Mar 1, 2018 •

edited

Loading

ianballou Mar 15, 2018 •

edited

Loading

ianballou Apr 3, 2018 •

edited

Loading