Skip to content
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

ansible-inventory --list JSON output shows unsafe values as dictionaries with __ansible_unsafe key #82999

Closed
1 task done
felixfontein opened this issue Apr 7, 2024 · 13 comments
Labels
affects_2.18 bug This issue/PR relates to a bug. data_tagging has_pr This issue has an associated PR.

Comments

@felixfontein
Copy link
Contributor

Summary

When running ansible-inventory --list (JSON output, i.e. not --yaml or --toml) for an inventory plugin which marks variables as unsafe, these variables' values are shown as dictionaries with a single __ansible_unsafe key and the actual value as a value.

Issue Type

Bug Report

Component Name

ansible-inventory --list

Ansible Version

devel, probably also older ones

Configuration

-

OS / Environment

Steps to Reproduce

Use an inventory plugin which marks variables such as ansible_host as unsafe.

Expected Results

{
    "_meta": {
        "hostvars": {
            "my_hostname": {
                "ansible_host": "my_ip_address"
            },
...

Actual Results

{
    "_meta": {
        "hostvars": {
            "my_hostname": {
                "ansible_host": {
                    "__ansible_unsafe": "my_ip_address"
                }
            },
...

Code of Conduct

  • I agree to follow the Ansible Code of Conduct
@felixfontein felixfontein changed the title ansible-inventory --list JSON output shows unsafe values as dictionaries with __ansible_unsafe key ansible-inventory --list JSON output shows unsafe values as dictionaries with __ansible_unsafe key Apr 7, 2024
@ansibot ansibot added bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. labels Apr 7, 2024
@ansibot
Copy link
Contributor

ansibot commented Apr 7, 2024

Files identified in the description:

If these files are incorrect, please update the component name section of the description or use the component bot command.

@flowerysong
Copy link
Contributor

This is not a bug. That is the correct JSON representation of an unsafe value. See 5941e4c

@felixfontein
Copy link
Contributor Author

Hmm, interesting. As a user I find the output somewhat confusing, but I now see why it's there.

But according to #47295, YAML and TOML output should then also indicate unsafe values, shouldn't they? Right now they do not. (I'm not sure how this should work in TOML, but in YAML !unsafe could be used.)

@flowerysong
Copy link
Contributor

Hmm, yes. I suspect this hasn't been noticed because people mainly use the YAML from this tool for visual inspection (not round-tripping data through) and hardly anyone uses TOML for anything.

TOML would require a convention similar to the JSON one, which I don't think is currently implemented for either dumping or loading.

YAML should be straightforward; for some reason AnsibleDumper currently represents unsafe values as strings instead of preserving that information, but something like this (plus a representer for AnsibleUnsafeBytes) could change that:

diff --git a/lib/ansible/parsing/yaml/dumper.py b/lib/ansible/parsing/yaml/dumper.py
index 11a1431b94..a9c990f1e6 100644
--- a/lib/ansible/parsing/yaml/dumper.py
+++ b/lib/ansible/parsing/yaml/dumper.py
@@ -44,6 +44,10 @@ def represent_vault_encrypted_unicode(self, data):
     return self.represent_scalar(u'!vault', data._ciphertext.decode(), style='|')


+def represent_unsafe(self, data):
+    return self.represent_scalar(u'!unsafe', text_type(data))
+
+
 def represent_unicode(self, data):
     return yaml.representer.SafeRepresenter.represent_str(self, text_type(data))

@@ -66,7 +70,7 @@ AnsibleDumper.add_representer(

 AnsibleDumper.add_representer(
     AnsibleUnsafeText,
-    represent_unicode,
+    represent_unsafe,
 )

 AnsibleDumper.add_representer(
@@ -111,7 +115,7 @@ AnsibleDumper.add_representer(

 AnsibleDumper.add_representer(
     NativeJinjaUnsafeText,
-    represent_unicode,
+    represent_unsafe,
 )

 AnsibleDumper.add_representer(
$ cat testinv.yml
all:
  hosts:
    testhost:
      ansible_host: !unsafe "'my{unsafe}'value"

$ ansible-inventory --list -i testinv.yml -y
all:
  children:
    ungrouped:
      hosts:
        testhost:
          ansible_host: !unsafe '''my{unsafe}''value'

@bcoca
Copy link
Member

bcoca commented Apr 8, 2024

vault is the other thing that you'll see 'strangely represented'. Sadly JSON and TOML do not have a way to create custom types like !yaml does, so this is what we did for JSON and could extend for TOML .. though that might not be worth the effort.

@felixfontein
Copy link
Contributor Author

I'm wondering whether the change above is a good idea - AnsibleDumper is also used in the to_yaml and to_nice_yaml filters, where it can have unwanted side-effects. And potentially other plugins use it.

@bcoca
Copy link
Member

bcoca commented Apr 8, 2024

I might need to add a toggle --strip-ansible-objects .. or the opposite

@ansibot ansibot added the has_pr This issue has an associated PR. label Apr 8, 2024
@felixfontein
Copy link
Contributor Author

I played around a bit and created a AnsibleUnsafeDumper, which differs from AnsibleDumper by representing unsafe strings with !unsafe, see #83003.

I also added another commit to add a new !unsafe-binary tag and use it to round-trip unsafe byte strings.

What do you think of the AnsibleUnsafeDumper approach? And of the !unsafe-binary tag? And could this (or at least parts of it) count as a bugfix, or is this definitely a new feature?

@bcoca
Copy link
Member

bcoca commented Apr 8, 2024

That it might all be redundant with data tagging, also i would not create a new dumper but use the existing one and just have 'feature parity'.

@felixfontein
Copy link
Contributor Author

@bcoca just modifying the existing one is a huge breaking change. Every time someone templates some data structure to YAML that contains unsafe strings - for example strings returned by a module -, these strings will now be marked as !unsafe. If this YAML is passed to any other tool than Ansible, say it's templated into a config file of some other program, this can cause a lot of problems, depending on how the other program reacts.

@bcoca
Copy link
Member

bcoca commented Apr 9, 2024

why i think that if we implement this at all, it has to have switches

@s-hertel s-hertel added data_tagging and removed needs_triage Needs a first human triage before being processed. labels Apr 9, 2024
@felixfontein
Copy link
Contributor Author

But how do you make the behavior switchable without having two dumpers? Or do you want to make this a global setting (so either all parts of ansible-core dump unsafe as !unsafe in YAML, or all parts don't)?

@felixfontein
Copy link
Contributor Author

According to #83003 (comment) the current state (JSON has this information, YAML and TOML not) is intentional. Closing this.

@ansible ansible locked and limited conversation to collaborators May 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.18 bug This issue/PR relates to a bug. data_tagging has_pr This issue has an associated PR.
Projects
None yet
Development

No branches or pull requests

6 participants