Added Ansible Tower Inventory Plugin.

[YunfanZhang42]

SUMMARY

This PR will allow the users to import an inventory from Ansible Tower using ansible inventory plugin. Example usage: ansible-inventory -i @tower_inventory --list. Note this plugin will read from environment variables instead of a config file, so tower_inventory is a keyword for triggering this plugin, not a path for the actual configuration file path.

ISSUE TYPE

• New Module Pull Request

COMPONENT NAME

lib/ansible/plugins/inventory/tower.py

ANSIBLE VERSION

ansible 2.7.0.dev0

ADDITIONAL INFORMATION

The reason that this plugin reads from environment variable instead of a configuration file is we used to have a ansible inventory script for tower that reads configuration from environment variables, and this plugin is a port of that inventory script, so we want to keep the consistency between the two.

Sample Usage:

# Set environment variables:
# Before you execute the following commands, you should make sure this file is in your plugin path, 
# and you enabled this plugin. 
# Set environment variables:
export TOWER_HOST=YOUR_TOWER_HOST_ADDRESS
export TOWER_USERNAME=YOUR_TOWER_USERNAME
export TOWER_PASSWORD=YOUR_TOWER_PASSWORD
export TOWER_INVENTORY=THE_ID_OF_TARGETED_INVENTORY
# Read the inventory specified in TOWER_INVENTORY from Ansible Tower, and list them. 
# The inventory path must always be tower_inventory
ansible-inventory -i tower_inventory --list

[ansibot]

The test ansible-test sanity --test pylint [explain] failed with 7 errors:

lib/ansible/plugins/inventory/tower.py:100:25: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/tower.py:101:44: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/tower.py:112:49: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/tower.py:115:45: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/tower.py:123:41: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/tower.py:129:41: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/tower.py:131:37: ansible-format-automatic-specification Format string contains automatic field numbering specification

The test ansible-test sanity --test no-dict-iterkeys [explain] failed with 1 error:

lib/ansible/plugins/inventory/tower.py:148:31: use `dict.keys` or `for key in dict:` instead of `dict.iterkeys`

The test ansible-test sanity --test use-compat-six [explain] failed with 1 error:

lib/ansible/plugins/inventory/tower.py:41:1: use `ansible.module_utils.six` instead of `six`

The test ansible-test sanity --test yamllint [explain] failed with 1 error:

lib/ansible/plugins/inventory/tower.py:36:1: error EXAMPLES: syntax error: expected '<document start>', but found '<scalar>'

click here for bot help

[s-hertel]

Rather than limiting this to environment variables you could utilize config to allow a more flexible use case. That would let you document each option individually and mark them as required so you don't have to manually check in the plugin that they were provided.
e.g.

• document options with their environment components:

options:
    tower_host:
        description: ...
        env:
            - name: TOWER_HOST
        required: true
    ...

• allow parsing of a file called tower_inventory by calling self._read_config_data(path)
• access those options with self.get_option('tower_host') after either calling self._read_config_data(path) (which will merge options provided in the tower_inventory file with the environment vars in a self._options dict)

[s-hertel]

you could keep the use case of no inventory config file if the path doesn't exist by calling self.set_options yourself (rather than happening in _read_config_data()) to have access to the environment variables with get_option()

[YunfanZhang42]

Thanks @s-hertel ! I will make these modification.

[s-hertel]

Rather than str, use to_text from ansible.module_utils._text to ensure compatibility with python 2 and 3: to_text(option, errors='surrogate_or_strict')

Looks good to me besides that.

[YunfanZhang42]

Thanks for reviewing this PR @s-hertel! Will do.

[s-hertel]

Do you want to add your github username? (@YunfanZhang42)

[YunfanZhang42]

No problem!

[YunfanZhang42]

@s-hertel Do you mind to take a look again?

[s-hertel]

Looks good to me. I'm not sure if there is a use case for users to create their own tower groups, but it would be easy to leverage constructed features if so. You can check out the virtualbox inventory for an example (in a nutshell, add the constructed documentation fragment, update InventoryModule to class InventoryModule(BaseInventoryPlugin, Constructable):, and then call self._set_composite_vars(), self._add_host_to_composed_groups(), and self._add_host_to_keyed_groups() where needed).

@bcoca Do you mind reviewing this?

[bcoca]

not really an 'import', 'uses tower as inventory source'

[YunfanZhang42]

Will change it, thanks for pointing out.

[bcoca]

why force either/or? you should be able to read some things from the config file, others from env vars, user might not wan't all eggs in one basket

[s-hertel]

It doesn't force either/or. Just indicates when the plugin needs to call set_options (or have self._read_config_data do it) if the inventory file doesn't exist.

[s-hertel]

direct only gets used for inventory plugins if there's a config file. Otherwise get_option suffices.

[YunfanZhang42]

@s-hertel Thanks for the clarification! That is my intention.

[bcoca]

we should fix the ensure_type function to do this, instead of working around it in every plugin

[YunfanZhang42]

👍

[bcoca]

this still has not changed, what is the actual issue you hit to require this 'type enforcement'?

[bcoca]

why is this ONLY an env var?

[AlanCoding]

I would really like to pull @matburt into the decision-making on this matter. It's not just the sourcing from the env var here, but also the use of the license_type parameter once it's received. What does this mean in practice? I think users would expect to be able to use the plugin as a part of the Ansible core CLI with any given type of target AWX or Tower instance.

[bcoca]

you can also just have a config file with plugin: tower as it's only entry

[YunfanZhang42]

Hey Brian thanks for your comments! The reason we did it this way is this plugin is adapted from an inventory script of Ansible Tower, so the we want to keep this "read environment variable only, do not require any config file" behavior to be backward compatible with the old way of reading inventory from Ansible Tower.

[AlanCoding]

Are other plugins supporting any kind of file-less use? It seems like that's more of where this was trying to go.

[YunfanZhang42]

No. They need to have a config file with plugin: plugin_name in it, and then read everything else from env var.

[bcoca]

my other issue is if user has an actual 'tower_inventory' file in cwd ... i would use something more like @tower_inventory or some other symbol to disambiguate from 'actual file'

[YunfanZhang42]

If they have an actual file, I think that file will end with .yml so this plugin will not mistakenly read it?

[bcoca]

not a blocker: i would use the internal url functions in lib/ansilbe/module_utils/urls.py as that is always present and has less issues with different versions than requests does

[bcoca]

wrap exceptions in to_native when stringifying them

[YunfanZhang42]

👍 Will do!

[AlanCoding]

It also might be good to compare these options to the options in

ansible/lib/ansible/utils/module_docs_fragments/tower.py

Lines 23 to 42 in cdd21e2

	DOCUMENTATION = '''
	options:
	tower_host:
	description:
	- URL to your Tower instance.
	tower_username:
	description:
	- Username for your Tower instance.
	tower_password:
	description:
	- Password for your Tower instance.
	tower_verify_ssl:
	description:
	- Dis/allow insecure connections to Tower. If C(no), SSL certificates will not be validated.
	This should only be used on personally controlled sites using self-signed certificates.
	type: bool
	default: 'yes'
	tower_config_file:
	description:
	- Path to the Tower config file. See notes.

[YunfanZhang42]

@AlanCoding Thanks for the dropping in! It seems we are using same names for items in yaml config file, while keeping the environment variable names the same as the they were in inventory script, so we should be fine.

I think we let the users to specify tower_config_file at run time, so this option is no longer needed right?

[AlanCoding]

I think we let the users to specify tower_config_file at run time, so this option is no longer needed right?

What do you mean by this?

I think the current state breaks congruence with the tower modules in that it won't accept the .tower_cli.cfg file. I'm not going to push for that right now, because I don't have some kind of grand unification plan that would allow both to take the same forms of credentials, although you could maybe just import the options processing code from the tower modules and have it work.

That may be too ambitious, and I'm braced for this being somewhat inconsistent of a specification.

If you look at my openstack usage example:

https://github.com/AlanCoding/Ansible-inventory-file-examples/blob/master/plugins/example_openstack/openstack.yml

You can see that the native configuration file format is clouds.yml, and the inventory file can do nothing more than point to the native configuration file.

Granted, I hate this, but it seems to be the canonical way of writing inventory plugins. If you extrapolated this pattern, then yes, I think you would allow the entry of tower_config_file inside of the tower_inventory.yml file. @bcoca please correct me if I have horribly misunderstood your standards here.

[bcoca]

its fine to have a 'separate pre-existing file', I would just use it as a fallback if other methods are not populated and i would not create one if it were not already standard.

[bcoca]

i actually don't think you should force having the tower_config_file entry, but it could be posed as an option and default to the 'normal location'

[bcoca]

the openstack plugin is not really the best example of standards to follow, you might be better off looking at aws_ec2 which also has 'pre existing configs' via boto

[AlanCoding]

yes, I agree that if tower_config_file is added as an option it should be optional. As a first-pass, maybe we don't need it, but have a plan for it in the future.

I still need to get synced up about the 2 methods of specification that this PR current does.

[bcoca]

So let me propose this, if just using the 'name' (no inventory source file) or the file contains a tower_config_file entry, use that first if they exist (in parse method):

super(InventoryModule, self).parse(inventory, loader, path)
if self.read_all_configs_from_env_var: # I would change name to 'no config file supplied' or similar
   if os.path.exists('~/.tower_cli.cfg'):
      # pick up options from the file
      self._set_options_from_tower_cli('~/.tower_cli.ccfg')
    self.set_options() # this will update with env vars to override file or pick up others       
else:
   self._read_config_data(path)
   if self.get_option('tower_cli_config'): # check for user defined
        self._set_options_from_tower_cli(self.get_option('tower_cli_config')
 ...

[YunfanZhang42]

Our current decision is to keep this plugin simple and do not let it read from tower_cli_config. In the future we might add this option.

[AlanCoding]

I agree with the other comments that this is hard to follow, this should look for vars from options first, and then check for the env var if the option is not provided.

[YunfanZhang42]

Yeah this is tricky. I think if the user want to read everything from environment variables, they still needs to provide a path in the command line to trigger the plugin, so I put tower_inventory as an option if they do not want to use any config file. I will switch to other implements if there is a better way.

[AlanCoding]

For the first-pass, I wouldn't balk at requiring a file with plugin: tower in it.

The question I'm more interested in is "do other plugins offer this in this type of format?"

[s-hertel]

I think all other inventory plugins currently use a file. Since most employ their own verify_file and don't always call super(InventoryModule, self).verify_file(path) there is kind of a dichotomy though.

[bcoca]

no, host_list and advance_host_list use an 'inventory string' , which is why they dont call super version of verify_file as 'its not a file'

[s-hertel]

Ah, okay.

Maybe if non-file inventory is to be supported, a more transparent way of doing this would be to remove self.read_all_configs_from_env_var, which does sound like it's indicating an either/or scenario and then put in parse() something like...

if os.path.isfile(path):
    try:
        config = self.loader.load_from_file(path)
    except Exception as e:
        raise AnsibleParserError(to_native(e))
    self.set_options(direct=config)

Oh, nevermind, I guess I see why you did that. The user could have a tower_inventory/@tower_inventory file for some reason. +1 to using @tower_inventory or something similar though, to make that more unlikely.

edit edit:
Hm. I guess just need a solution for that corner case. Or check verify_file that the file exists first, and move the path.endswith(... check to the elif.

[AlanCoding]

@YunfanZhang42 did you rebase against devel?

This is looking good, I have tested:

• dev server
• production server
• normal inventory
• smart inventory
• super user
• user with inventory read access

All of the issues are resolved with this testing so far. I am accumulating a list of non-blocking enhancements to table for later:

• add option to get cluster inventory
• add options to get from inventory & org names
• share code from the modules - allow use of OAuth2 token
• raise error if http:// given
• allow construct-able groups

I would still like to do a little bit more testing tomorrow morning.

[bcoca]

should this really be 'endswith' or more like equal? path.strip() == '@tower_inventory'

[YunfanZhang42]

On my computer it gives /Users/yunfzhan/ansible/@tower_inventory, so we probably should stick with endswith.

[bcoca]

ah, its seeing it as a file name ... weird since host_list 'works' .. i should look into that

[AlanCoding]

This is likely to be the use pattern of Tower / AWX, so I think allow this may be important. It sounds like this implementation may be subject to revision if another general fix is put in, but is there anything blocking this PR @bcoca for the time being?

[AlanCoding]

one thing that does bother me is that this isn't called tower_inventory_id, and likewise for the environment variable. If there is to be any sense of parity with the tower modules, then we would expect specification by inventory and organization names. Thus, we will add tower_inventory_name and probably tower_inventory_organization. In the syntax of the modules, either tower_inventory or tower_inventory_organization would implicitly refer to the name.

Also, it seems that there would be no problem for implementing this other env name as far as AWX server changes are involved.

[AlanCoding]

I'm sorry if I'm being a broken record about this, but I see lots of hazards for us in the future. Maintaining this could be a large undertaking if there's not a good deal of congruency. I now see three different forms of configuration files:

inventory plugin

plugin: tower
tower_host: https://localhost:8043/
tower_username: admin
tower_password: password1
tower_inventory: 6
tower_verify_ssl: False

tower modules

(docs actually call out key=value syntax, but docs are poorly thought out in several ways currently)

parameters match what are directly passed into the modules

host: https://localhost:8043/
username: admin
password: password1
verify_ssl: False

tower-cli config

[general]
host = http://localhost:8013/
verify_ssl = false
oauth_token = xxxxxxxxxxxxxxx

For the most part, I agree with the environment variable keys used here. However, I know this is painful, but it seems that the inventory config format is the odd-one out here. The tower-cli code can be adjusted to accept the YAML format. The modules can be free to ignore other variables like "plugin", but we can't go back once we've introduced the long names in the inventory plugin config.

[AlanCoding]

I withdraw suggestions I previously made regarding the module_utils code. I looked further into it, and even if we have parity with the parameter names, what it does it fairly trivial for our purposes, and would conflict with the plugin class get_option use, which looks like it works perfectly already.

[AlanCoding]

For general development reference, I want to call out the fact that the tower-cli methods will take the tower_ prefixed parameters as arguments, but will strip off the prefix for applying them to settings.

https://github.com/ansible/tower-cli/blob/2c115877a36238b94fb8af1ff32915ebaf868e60/tower_cli/conf.py#L266-L267

The tower modules apply the settings directly, so they deal exclusively with the un-prefixed namespace.

[bcoca]

ready to merge once you add missing version_added: "2.7"

[YunfanZhang42]

No problem, will add version in.

[AlanCoding]

Tested with the most recent version:

$ cat tower/tower_inventory.yml 
plugin: tower
host: https://localhost:8043/
username: admin
password: password1
inventory_id: 6
verify_ssl: False

ansible-inventory works.

Commenting out username and specifying via env var TOWER_USERNAME works.

The only thing that sticks out me me is that it appears that the env vars are lower precedence than the vars defined in the file. I didn't expect this, but it appears that this is consistent with the behavior of the modules, so this is actually desirable.

With this, the PR has my final 👍

[ansibot]

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/plugins/inventory/tower.py:131:29: ansible-format-automatic-specification Format string contains automatic field numbering specification

click here for bot help

[AlanCoding]

error with inventory with ungrouped host

 [WARNING]:  * Failed to parse /Users/alancoding/Documents/repos/ansible-inventory-file-examples/tower/tower_inventory.yml with auto plugin: 'vars'

  File "/Users/alancoding/.virtualenvs/ansible/lib/python2.7/site-packages/ansible-2.7.0.dev0-py2.7.egg/ansible/inventory/manager.py", line 270, in parse_source
    plugin.parse(self._inventory, self._loader, source, cache=cache)
  File "/Users/alancoding/.virtualenvs/ansible/lib/python2.7/site-packages/ansible-2.7.0.dev0-py2.7.egg/ansible/plugins/inventory/auto.py", line 55, in parse
    plugin.parse(inventory, loader, path, cache=cache)
  File "/Users/alancoding/.virtualenvs/ansible/lib/python2.7/site-packages/ansible-2.7.0.dev0-py2.7.egg/ansible/plugins/inventory/tower.py", line 184, in parse
    for var_name, var_value in six.iteritems(group_content['vars']):

[AlanCoding]

in all cases, I would default group_contents.get('hosts', [])

[AlanCoding]

I would also default children list to []

[AlanCoding]

I would default vars to {}

[AlanCoding]

I'm a little iffy about the ungrouped or all group here. I would skip ungrouped.

[YunfanZhang42]

This one is fine it seems. Our API never gives ungrouped, but inventory.add_host(host_name) in Ansible Inventory API will automatically add an ungroup to the hosts.

[AlanCoding]

ungrouped case fixed

$ ansible-inventory -i t1113/tower_inventory.yml --list --export
{
    "_meta": {
        "hostvars": {
            "lonley": {
                "remote_tower_enabled": "true", 
                "remote_tower_id": 3600
            }
        }
    }, 
    "all": {
        "children": [
            "ungrouped"
        ]
    }, 
    "ungrouped": {
        "hosts": [
            "lonley"
        ]
    }
}