New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide functions for IP address or port range validating (fixes #16256) #17731

Open
wants to merge 3 commits into
base: devel
from

Conversation

@xen0l
Contributor

xen0l commented Sep 23, 2016

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

module_utils/basic.py

ANSIBLE VERSION
ansible 2.1.0.0
  config file = 
  configured module search path = Default w/o overrides
SUMMARY

The change adds new types and validations for ports, L3/L4/L7 protocols, IPv4 and IPv6 addresses. Fixes issue #16256.


@xen0l

This comment has been minimized.

Contributor

xen0l commented Oct 5, 2016

Anyone?

@abadger

This comment has been minimized.

Member

abadger commented Oct 25, 2016

I hate to add this to basic.py since only a few things will need it but every module needs basic.py. At some point (sooner for some things like parameter parsing since network modules want to have access to those routines from ansible/ansible code) we want to pull basic.py apart into smaller pieces. Is there some way we could make parameter parsing more modular when we do this and then make this one optional piece of that?

@xen0l

This comment has been minimized.

Contributor

xen0l commented Oct 25, 2016

Perhaps, we could create a file, which would contain additional module parsing types on it's own. This file would be then imported in network or regular modules? I am thinking about this:

parameter_parsing_additional.py -> network_basic.py -> network_module

The first file could be also imported into regular modules.

@abadger

This comment has been minimized.

Member

abadger commented Oct 25, 2016

Yeah, something like that sounds good. Maybe using a subdirectory to hold all parameter related modules.

What I'm still thinking about is how we tell the argument_spec handling code about additional types.

Maybe something like:

# classes that implement new parameter validation types
from ansible.module_utils.parameters.ipaddress import IPValidator
from ansible.module_utils.parameters.basic import BasicValidator

AnsibleModule(parameter_validators=(BasicValidators, IPValidators), [...])

The Validators are really just mappings of type names to functions. So maybe it should look like:

# dictionaries mapping string type names to parameter validator/conversion functions
from ansible.module_utils.parameters.ipaddress import IPVALIDATORS
from ansible.module_utils.parameters.basic import BASICVALIDATORS

AnsibleModule(parameter_validators=(BASICVALIDATORS, IPVALIDATORS), [...])

AnsibleModule parameter_validators can default to [BASICVALIDATORS].

Does this start to sound like a good plan?

@xen0l

This comment has been minimized.

Contributor

xen0l commented Dec 3, 2016

Yeah, this seems reasonable. However, I think that we should do refactor in another PR.

@abadger

This comment has been minimized.

Member

abadger commented Dec 19, 2016

@xen0l Once it's added to basic.py it has to stay in basic.py for backwards compatibility. (look at the imports near the top of basic.py that simply bring old names into the basic namespace). This means that the size of downloads for modules which utilize basic.py just continues to grow and grow.

Since this is new functionality that is infrequently needed I think it is okay to gate acceptance of the feature on making it optional. We can bring it up with other core committers for a second opinion, though.

@nitzmahone

This comment has been minimized.

Member

nitzmahone commented Dec 19, 2016

I like where @abadger is going- a slightly simpler approach might be to extend the type value in the arg dicts to accept a validator type and/or function arg in addition to the string value it accepts today. Still meets the goal of not needing to predeclare what types we're using in the AnsibleModule ctor to extend the mapping while still allowing it be infinitely extended without further changes to basic.py.

eg:

from ansible.module_utils.parameters.ipaddress import subnet_mask_validator

...

module = AnsibleModule(arg_spec=dict(
  subnet=dict(type=subnet_mask_validator)
))
@gundalow

This comment has been minimized.

Contributor

gundalow commented Sep 13, 2017

Now that stable-2.4 has been branched this can be put into network_common.py and once fixed merged into devel

@abadger

This comment has been minimized.

Member

abadger commented Oct 16, 2017

@xen0l nitzmahone added the framework for this to go in here: #27183 (It's in 2.4.x) so if you want to rework this PR to use that framework it can get merged.

@gundalow

This comment has been minimized.

Contributor

gundalow commented Jan 17, 2018

@xen0l You able to look at this?

@gundalow

This comment has been minimized.

Contributor

gundalow commented Jan 18, 2018

@xen0l nitzmahone added the framework for this to go in here: #27183 (It's in 2.4.x) so if you want to rework this PR to use that framework it can get merged.

@ansibot ansibot added feature and removed feature_pull_request labels Mar 2, 2018

@gundalow

This comment has been minimized.

Contributor

gundalow commented May 22, 2018

@xen0l Gentle poke :)

@gundalow

This comment has been minimized.

Contributor

gundalow commented May 22, 2018

For the record I really like #17731 (comment)

@xen0l

This comment has been minimized.

Contributor

xen0l commented Aug 13, 2018

@gundalow is #17731 (comment) the way how to do it?

@gundalow

This comment has been minimized.

Contributor

gundalow commented Aug 14, 2018

@xen0l Correct

@xen0l xen0l force-pushed the xen0l:16256 branch from 9288e86 to 3f46496 Aug 18, 2018

@xen0l

This comment has been minimized.

Contributor

xen0l commented Aug 18, 2018

@gundalow @abadger so I updated the PR as requested, but there are few issues, on which I will need your guidance:

  1. I had to move boolean outside of AnsibleModule class (https://github.com/ansible/ansible/pull/17731/files#diff-90085fdcec6ed8b273ba885eaee60328R822) in order to be able to use it inside validators. However, I am not sure if this is the best approach how to tackle the problem. Given the poor naming choice of boolean from ansible.module_utils.parsing.convert_bools I can't create the function with same name, so I think this was the easiest option. If you happen to have a better idea, please let me know.
  2. I moved safe_eval outside of AnsibleModule as requested
  3. The code is currently not working, because in ansible.module_utils.validators.core we are trying to import jsonify (among other utils) from ansible.module_utils.basic, but it fails. I think this is because it results in circular imports. Would be the solution to move jsonify, safe_eval etc to different file?
  4. What is the best path for placing validator tests?

Feedback/reviews are welcome.

@ansibot

This comment has been minimized.

Contributor

ansibot commented Aug 18, 2018

The test ansible-test sanity --test ansible-doc --python 2.6 [explain] failed with the error:

Command "ansible-doc -t cache jsonfile memcached memory mongodb pickle redis yaml" returned exit status 250.
>>> Standard Error
/root/ansible/bin/ansible-doc:103: DeprecationWarning: BaseException.message has been deprecated as of Python 2.6
  msg = e.message
ERROR! Unexpected Exception, this is probably a bug: cannot import name jsonify

The test ansible-test sanity --test ansible-doc --python 2.7 [explain] failed with the error:

Command "ansible-doc -t cache jsonfile memcached memory mongodb pickle redis yaml" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: cannot import name jsonify

The test ansible-test sanity --test ansible-doc --python 3.5 [explain] failed with the error:

Command "ansible-doc -t cache jsonfile memcached memory mongodb pickle redis yaml" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: cannot import name 'jsonify'

The test ansible-test sanity --test ansible-doc --python 3.7 [explain] failed with the error:

Command "ansible-doc -t cache jsonfile memcached memory mongodb pickle redis yaml" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: cannot import name 'jsonify' from 'ansible.module_utils.basic' (/root/ansible/lib/ansible/module_utils/basic.py)

The test ansible-test sanity --test ansible-doc --python 3.6 [explain] failed with the error:

Command "ansible-doc -t cache jsonfile memcached memory mongodb pickle redis yaml" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: cannot import name 'jsonify'

The test ansible-test sanity --test docs-build [explain] failed with the error:

Command "/usr/bin/python test/sanity/code-smell/docs-build.py" returned exit status 1.
>>> Standard Error
Traceback (most recent call last):
  File "test/sanity/code-smell/docs-build.py", line 100, in <module>
    main()
  File "test/sanity/code-smell/docs-build.py", line 17, in main
    raise subprocess.CalledProcessError(sphinx.returncode, cmd, output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command '['make', 'singlehtmldocs']' returned non-zero exit status 2.

click here for bot help

@ansibot ansibot added the stale_ci label Aug 26, 2018

@xen0l

This comment has been minimized.

Contributor

xen0l commented Aug 28, 2018

@gundalow / @abadger kind reminder.

@mkrizek

This comment has been minimized.

Contributor

mkrizek commented Oct 25, 2018

bot_skip

@mkrizek

This comment has been minimized.

Contributor

mkrizek commented Oct 25, 2018

^^^ LabelWafflingError: "needs_rebase" label is waffling on https://github.com/ansible/ansible/pull/17731

@gundalow

This comment has been minimized.

Contributor

gundalow commented Nov 30, 2018

@xen0l Could you rebase, I'd like to get this in Ansible 2.8

@xen0l

This comment has been minimized.

Contributor

xen0l commented Dec 3, 2018

@gundalow yes, I will rebase. However, there are issues with circular imports, which need to be solved and I might need to get input from Ansible developers to know, which solution is acceptable.

@gundalow

This comment has been minimized.

Contributor

gundalow commented Dec 4, 2018

Command "ansible-doc -t cache jsonfile memcached memory mongodb pickle redis yaml" returned exit status 250.
>>> Standard Error
/root/ansible/bin/ansible-doc:103: DeprecationWarning: BaseException.message has been deprecated as of Python 2.6
  msg = e.message
ERROR! Unexpected Exception, this is probably a bug: cannot import name jsonify
Command "ansible-doc -t cache jsonfile memcached memory mongodb pickle redis yaml" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: cannot import name 'jsonify' from 'ansible.module_utils.basic' (/root/ansible/lib/ansible/module_utils/basic.py)
@gundalow

This comment has been minimized.

Contributor

gundalow commented Dec 4, 2018

@xen0l If you rebase we can then take a look at the circular imports

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment