search

ansible / awx

AWX provides a web-based user interface, REST API, and task engine built on top of Ansible. It is one of the upstream projects for Red Hat Ansible Automation Platform.
Other
14.06k stars 3.42k forks source link

Allow disabling var plugins during inventory sync #12829

Open apollo13 opened 2 years ago

apollo13 commented 2 years ago

Please confirm the following

Feature type

Enhancement to Existing Feature

Feature Summary

I'd like to see an option to disable var plugins during inventory syncing. For some users (like me) there is no need that the AWX inventory sync process copies variables from group & hostvars into the AWX datamodel to be supplied back to job during execution.

When a job is run it has access to those variables anyways and storing them in the first place causes issues like #4089 and #10200. I would image a checkbox ala "Disable var syncing from plugins" which in turn would set ANSIBLE_VARS_ENABLED='' when executing ansible-inventory and would result in only inventory (ie from inventory.ini) variables to get synced. If wanted the checkbox could be named Disable var syncing and it would then also ignore inventory variables.

If AWX is runable without syncing variables (I think it should be) then this feature request would site nicely between the current way of doing things and the additional complexity #4089 would have to introduce.

I am volunteering to write the code for this feature if accepted.

Select the relevant components

Steps to reproduce

Current results

All variables are synced into AWX

Sugested feature result

No or only inventory variables are synced into AWX

Additional information

No response

AlanCoding commented 2 years ago

The setting you mentioned could be set via the vars_plugins_enabled key in the [defaults] section of ansible.cfg.

https://github.com/ansible/ansible/blob/0de44804679461b8d898129068183d6da416e3a7/lib/ansible/config/base.yml#L1980

That could (in-principal) be checked into the project where the inventory lives (I think top-level). And I would like to get your thoughts on how well this would or wouldn't solve your problem.

apollo13 commented 2 years ago

@AlanCoding we already do check in ansible.cfg :) The problem with your suggestion though is that not only ansible-inventory would ignore vars plugins but also the actual ansible-playbook run. I want/need to disable it only during inventory parsing otherwise the playbooks would basically have no variables at all. Does that make sense?

AlanCoding commented 2 years ago

It absolutely makes sense. If this could be specifically disabled for inventory syncs, that sounds like a fairly desirable outcome. We have this statically-defined dictionary which are environment variables applied to all inventory updates but not other job types.

https://github.com/ansible/awx/blob/e7c97923a305ffeb2bd06de84fcc8e69e164d589/awx/main/constants.py#L34-L41

This is not a setting which is editable by the user (either in the UI or in file settings). So applying your suggestion of ANSIBLE_VARS_ENABLED='' would still be a code change here... although it is completely static. I don't like the checkbox idea:

I would image a checkbox ala "Disable var syncing from plugins"

But absent any supporting user-facing changes, I would at least entertain the notion that we may want to set this (or something like it) as the default behavior. Talking it out... problem we hit is that host_vars/foobar may contain completely unencrypted variables, and the setting you mention would wipe those out, and that would break a lot of current usage.

I'm very interested in the idea that we could add a custom vars plugin to awx/playbooks and then enable that instead. This custom vars plugin would do the standard behavior (hopefully via inheritance) and then catch the errors from vault decryption. That could also allow writing warning messages in these cases, which would be desirable.

apollo13 commented 2 years ago

I would at least entertain the notion that we may want to set this (or something like it) as the default behavior.

May I ask why you'd want this as default behavior?

problem we hit is that host_vars/foobar may contain completely unencrypted variables, and the setting you mention would wipe those out, and that would break a lot of current usage.

Unless I misunderstood how AWX parses inventories and then run jobs I do not see how that would break something. What I understood so far (or at least thought that would be how AWX operates):

Assuming my assumptions above are correct then the only "visible" change is that the inventory in AWX would no longer show variables that are usually supplied via the vars plugins.

apollo13 commented 2 years ago

As a sidenote:

I'm very interested in the idea that we could add a custom vars plugin to awx/playbooks and then enable that instead.

Please don't, we are using our own variable plugins (via the commited ansible.cfg) in AWX that fetches data from elsewhere and overriding the vars plugins would break that flow.

AlanCoding commented 2 years ago

But even with that supplied inventory host_vars/foobar & group_vars/bar would still get evaluated at this stage as well -- or am I mistaken here?

Only if the inventory and playbook are in the same directory. It's possible to put host_vars/ and group_vars/ next to the inventory. These get included in the output from ansible-inventory which was designed to include hostvars from vars plugins. So those get merged with general hostvars, and are part of the JSON data in the job's temporary inventory file you mention.

AlanCoding commented 2 years ago

Here's the stock Ansible vars plugin:

https://github.com/ansible/ansible/blob/devel/lib/ansible/plugins/vars/host_group_vars.py

Unfortunately, it has a monolithic get_vars method, so a subclass can't easily override the behavior of loading variables for a single host or group (entity). We might still be able to monkey-patch the part that loads and parses the file. This would probably be loader.load_from_file. Again, the approach I'm interested in is to override that method and wrap the original in a try-except, catching (and doing nothing, aside from warning) the specific Ansible vault exception type that there are no secrets to decrypt.

apollo13 commented 2 years ago

Only if the inventory and playbook are in the same directory.

I do not think this is correct. Using this minimal example repo: https://github.com/apollo13/ansible-test

and executing: ansible-playbook -i inventory/inv.ini plays/test.yml (play and inv directory are in different dirs, group_vars next to play) I get a successful run:


PLAY [localhost] *****************************************************************************************************************************************************************************************************************************

TASK [Gathering Facts] ***********************************************************************************************************************************************************************************************************************
ok: [localhost]

TASK [debug] *********************************************************************************************************************************************************************************************************************************
ok: [localhost] => {
    "msg": "Hello"
}

PLAY RECAP ***********************************************************************************************************************************************************************************************************************************
localhost                  : ok=2    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0

The second last paragraph in https://docs.ansible.com/ansible/latest/user_guide/intro_inventory.html#organizing-host-and-group-variables also seems to confirm that:

For ansible-playbook you can also add group_vars/ and host_vars/ directories to your playbook directory. Other Ansible commands (for example, ansible, ansible-console, and so on) will only look for group_vars/ and host_vars/ in the inventory directory.

apollo13 commented 2 years ago

I just realized the following: We are using a playbook structure like this:

inv1.ini
inv2.ini
group_vars/
host_vars/
play1.yml

This way AWX inventory syncing will read the group_vars & host_vars.

If we instead were to use:

inventory/inv1.ini
inventory/inv2.ini
group_vars/
host_vars/
play1.yml

then AWX inventory syncing would have no vars to sync but the plays would still continue to work :)

AlanCoding commented 2 years ago

You're second case is instructive, let's clarify the expectation where inventory/host_vars/ are present...

s-hertel commented 2 years ago

Re: https://github.com/ansible/awx/issues/12829#issuecomment-1239769592

I'm not sure if this would be helpful, but ansible-core has a config option for determining whether vars plugins run after inventory or before tasks (or both). ansible-inventory ignores the option (removed in https://github.com/ansible/ansible/commit/c1f280ba6e4a1e5867720e8c8426bc451ad32126) since the default is to run vars plugins per task. Maybe ansible-inventory could re implement support but I'm not sure how how to do that in a backwards compatible way - maybe a new option at this point.

AlanCoding commented 2 years ago

I have a CLI demo of my preferred approach here:

https://github.com/AlanCoding/Ansible-inventory-file-examples/tree/master/vault/file_vars

I need to further polish the new option being added to the vars plugin here. With that done, then I would add the plugin to the awx/playbooks folder here and enable it for inventory plugins. That can solve the problem immediately for AWX, but for long-term, I would take the additions to the host_group_vars plugin and contribute it back up to ansible-core, so we could eventually delete the custom plugin.

apollo13 commented 2 years ago

With that done, then I would add the plugin to the awx/playbooks folder here and enable it for inventory plugins.

How would that be done without disabling other user defined vars plugins from ansible.cfg. Or is using custom vars plugins simply not supported?

Are you thinking about enabling this plugin by default? If yes I think it would need careful documentation around it because it introduces another set of issues. Assuming the alternative layout from https://docs.ansible.com/ansible/2.8/user_guide/playbooks_best_practices.html#alternative-directory-layout ignoring vaulted files would result in the being ignored completely since the actual playbook run wouldn't pick up files from the inventory subdirs anymore (since AWX passes a custom inventory file to ansible-runner). This can have two results which are probably both hard to debug:

Personally I'd still love to see an option to disable all var plugins during inventory sync as optimization for users who are using the "standard" layout (https://docs.ansible.com/ansible/2.8/user_guide/playbooks_best_practices.html#directory-layout) and simply do not need the variables to be visible inside of AWX.

apollo13 commented 2 years ago

I see you wrote:

A key point here is that many users co-locate their inventory with their playbook (same folder), so they really just want ansible-inventory to ignore vault variables.

in https://github.com/ansible/ansible/pull/78765. I would like to ask what makes you think that those users want ansible-inventory to ignore vault variables (Not trying to attack here, I'd like to understand your thought process so we can reach a good solution)? Personally I think those users would probably just be as fine if variable gathering can be disabled completely. The end result is roughly the same (if not better because you can't get confused by missing synced variables from encrypted files).

To be fair I am probably not the standard AWX user. I never bothered about inventories in AWX aside from the fact that I need them to run a playbook. If I could just tell AWX to not sync an inventory at all and just use an inventory file from the project (like a manual ansible-playbook run would do) I'd be happy as well (I guess plenty of users would). Note that I am not recommending to get rid of that stuff, I am sure users have their usecases for it, but for people migrating from manual ansible runs to AWX it is usually not the first thing they care about -- but that might just be my opinion.

AlanCoding commented 2 years ago

How would that be done without disabling other user defined vars plugins from ansible.cfg.

In the demo/proposal I have, I have example CLI commands that use environment variables to set the vars plugin list. This would be impossible for the user to change, as it takes highest precedence, yes. However, this would only be enabled for inventory updates, not for job runs.

You mentioned the user scenario where inventory / playbook are co-located in the same folder. Yes, for those users, it would be fine to disable vars plugins for inventory updates, but I think this is a flawed one-side-fits-all approach, because someone could be using the typical folder layout that you link where the inventory variables are not encrypted, or even encrypted at the var level and there is no reason to make that stop working. The pain that most users are reporting are that an encrypted vars file will totally error an SCM inventory update. In the co-location scenario, that error is a false-positive, because it would just be decrypted by a vault credential in a subsequent job run. Nonetheless, you have articulated well the hazards with turning that error off entirely.

If I could just tell AWX to not sync an inventory at all and just use an inventory file from the project (like a manual ansible-playbook run would do) I'd be happy as well

Well there's https://github.com/ansible/awx/issues/5692, which has a lot of question marks, but at least has overlap with what you're saying. I picture passing the -i for the SCM inventory in the ansible-playbook CLI for the job, but then add another -i which runs a custom inventory plugin that actually reads the inventory state and pipes (or writes) it to another location. Then that inventory data is used to update the database inventory.

mateuszdrab commented 1 year ago

Just stumbled upon the issue with my relatively simple AWX deployment. After getting some encrypted group_vars in place to deduplicate usage of some variables across my playbooks, I realized my inventory fails to update. In my case, I'd rather not have the variables read into the groups, I want them to be read at runtime and that's why they're in a group_vars directory and not in the hosts file. Is there an easy workaround to disable loading any variables other than the ones in the hosts inventory file?

AlanCoding commented 1 year ago

In my last comment, I gave a proposed solution, which has a corresponding patch in Ansible core https://github.com/ansible/ansible/pull/78765

If Ansible core were to accept that patch (even in principal) then AWX could temporarily vendor the plugin and solve the problem... until all supported Ansible versions have the patch.

@bcoca replied that prior work exists at https://github.com/ansible/ansible/pull/37019, which I didn't respond positively to in 2018 because the problem of concern at the time was group vars from the inventory source, not adjacent group_vars/ folders. The final form of that patch would solve this problem with ANSIBLE_ERROR_ON_VAULT_SECRET=false set for all AWX inventory imports. (again, I've argued that this should be reasonable default behavior)

If you want a workaround

You can borrow the vars plugin from my proposed solution and put it in your source tree, but the stated goal of this issue is to ignore in inventory imports, but not ignore in job runs. To do this, you can create a custom credential type that sets it to ignore decryption errors. However, the env var naming I used, ANSIBLE_HOST_GROUP_VARS_VAULT_ERROR_BEHAVIOR would not work, because env vars starting with ANSIBLE_ are forbidden in credential types. So you would have to name it something else.

bcoca commented 1 year ago

I'm not sure the ask is matching the proposed solution, the ask is for avoiding all imports, the solution proposed only avoids vaulted imports. For the former, its already been pointed out above that a solution exists, just use the env var to turn on/off the vars plugins as needed.

For the vault issue I still do not think the vars plugin is the correct place for this, but a global value to allow for error/warn/ignore of the unvault failure.

AlanCoding commented 1 year ago

Turning off vars plugins will drop unrelated variables. That would work for some users.

The user pain here is that they vault a file in group_vars/ or host_vars/ and this makes imports unusable, because AWX does not provide vault secrets to ansible-inventory. There is a conflict of expectations because it otherwise passes through vault content like this:

{
    "_meta": {
        "hostvars": {
            "green.example.com": {
                "anyvariable": "value",
                "firstvariable": {
                    "__ansible_vault": "$ANSIBLE_VAULT;1.1;AES256\n32633234373965356537366531333238363166393039306462633137353462396335653531633637\n6335366138623264303862306535303366646630643262650a386433376130383562343037613764\n62653330643635623033323764383461613331336339336435633864303032646233393336643366\n6163383166626565660a623836303939326564346338343338346366373561623862633065613266\n6132\n"
                }
            }
        }
    },

That's independent of vars plugins, because encrypted variables can come from the inventory source itself (as well as vars plugins).

Turning off vars plugins, or ignoring all encrypted variables (ignoring decryption fails), both fail to fit the shape of the problem. Encrypted variables in inventory imports is a solved problem, but encrypted files are not because you do not know what keys are in them, so Ansible's behavior is to always error.

bcoca commented 1 year ago

@AlanCoding I acknowledge there are several problems, but this ticket expresses one, you are solving another. I'm not saying all should not be solved. Speaking from a 'core' perspective, the ones expressed in this ticket (avoid importing vars plugin data) is already solvable via using the configuration.

The one you are expressing (deal with file vault errors on import) can be solved in several ways (inline vaults already work as you show above):

Each of the solutions works for a diff subset of users/contexts.

AlanCoding commented 1 year ago

import with vault secrets, but do not unvault the data, basically convert the vaulted files into individual inline vaults (also requires changes)

It sounds like that would be the "perfect" solution, and may happen completely in core. I'm unclear if it can be done, so I haven't suggested it.

If you have a group "foo" with a "bar" variable with an encrypted value, then the JSON encoder builds the ... dict if the data is {"foo": {"vars": {"bar": ..., "other": "stuff"}}}. But we're talking about top-level group variables that need to co-exist with other variables... thinking something like {"foo": {"vars": {..., "other": "stuff"}}}. I can see how that might work on the encoder side - where it currently detects an encrypted object (if getattr(o, '__ENCRYPTED__', False):), it would have to be redirected at a different kind of point, like when it goes into elif isinstance(o, Mapping):, you need to check if the dictionary has a marker key to indicate that there's an extra "blob" of variables that have to be merged. But what would be the internal object in the first place? Probably not AnsibleVaultEncryptedUnicode, because it's not a string-like thing, it's a dict-like thing (if decrypted content is valid YAML to begin with, which it may not be). I can easily see the internal parts of this being a showstopper.

on the JSON decoder side I think it might work too, because we're fed in a mapping pairs, so we would do a check outside of the loop, as opposed to the in-loop check for "__ansible_vault". So from a purely JSON encode/decode perspective it might work, but I'm expecting the entire thing to blow up when representing it in YAML.

bcoca commented 1 year ago

that option while possible, requires major updates to many subsystems.

hagaram commented 3 months ago

Any updates ? This issue, and some linked ones are still relevant.