[node-patches] Change in ovirt-node[master]: Validate passed NTP servers and reject bad ones

[fabiand at fedoraproject.org]

Fabian Deutsch has posted comments on this change.

Change subject: Validate passed NTP servers and reject bad ones
......................................................................


Patch Set 3: Code-Review-1

(2 comments)

http://gerrit.ovirt.org/#/c/17888/3/scripts/ovirt-init-functions.sh.in
File scripts/ovirt-init-functions.sh.in:

Line 643:             dns=*)
Line 644:             dns=${i#dns=}
Line 645:             ;;
Line 646:             ntp=*)
Line 647:             ntp=$(printf $i|python -c "`cat << EOP
Could we push this functionality into some method, e.g.:

    ovirt.node.config.defaults.Nameservers.filter_valid_servers(list)

returns teh list of valid servers. The member can be static.
Line 648: import sys
Line 649: from ovirt.node import valid
Line 650: validservers = []
Line 651: for line in sys.stdin:


http://gerrit.ovirt.org/#/c/17888/3/src/ovirt/node/config/defaults.py
File src/ovirt/node/config/defaults.py:

Line 850:         validator = lambda v: valid.FQDNOrIPAddress()
Line 851:         map(validator, servers)
Line 852: 
Line 853:         validservers = [x for x in servers if
Line 854:                         valid.FQDNOrIPAddress().validate(x)]
I'd rather like to keep out the filtering out of this config module.
My suggestion would be that we _assume_ that only valid servers are passed and therefor validate all servres against the FQDNOrIPAddress() validator and raise an exception if necessary.

I would not silently filter out incrrect servers, that is hard to debug. At one point we wonder why some servers don't end up in the file.

So my suggestion:
Validate yes, filter no.

Filter in the higher level function ...
Line 855: 
Line 856:         # Mangling to match the conf file
Line 857:         return {"OVIRT_NTP": ",".join(validservers) or None
Line 858:                 }


-- 
To view, visit http://gerrit.ovirt.org/17888
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb135977b4359f5fe0161215cec2cdd1b22244b3
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-node
Gerrit-Branch: master
Gerrit-Owner: Ryan Barry <rbarry at redhat.com>
Gerrit-Reviewer: Fabian Deutsch <fabiand at fedoraproject.org>
Gerrit-Reviewer: Joey Boggs <jboggs at redhat.com>
Gerrit-Reviewer: Ryan Barry <rbarry at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes