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

Syntax error in "aws_kms_info" example for filtering by tag #276

Closed
daurrutia opened this issue Oct 22, 2020 · 8 comments
Closed

Syntax error in "aws_kms_info" example for filtering by tag #276

daurrutia opened this issue Oct 22, 2020 · 8 comments
Assignees

Comments

@daurrutia
Copy link

SUMMARY

"HINT: Did you know the documentation has an "Edit on GitHub" link on every page ?"
I did not see this feature.

The example for tag filtering below:

# Gather information about all keys with a specific name
- community.aws.aws_kms_info:
    filters:
      "tag:Name": Example

Ought to read:

# Gather information about all keys with a specific name
- community.aws.aws_kms_info:
    filters:
      "tags:Name": Example

https://docs.ansible.com/ansible/latest/collections/community/aws/aws_kms_info_module.html

ISSUE TYPE
  • Documentation Report
COMPONENT NAME

community.aws.aws_kms_info

ANSIBLE VERSION
2.10.2
@markuman
Copy link
Member

What the exactly error message you got?
I cannot reproduce it.

---
- hosts: localhost
  connection: local
  gather_facts: False

  tasks:
    - name: test
      community.aws.aws_kms_info:
        filters:
          "tags:Name": Example

works fine. With communit.aws 1.0.0 and also with current version 1.2.1

@tremble
Copy link
Contributor

tremble commented Nov 12, 2020

I'm about 90% sure that "tag:Name" is correct (looking at the integration tests for other modules), we're currently waiting on some CI policy changes so that we can run the KMS tests in CI. Once that's in I'll go through the KMS examples and add some integration tests to make sure they work.

@tremble
Copy link
Contributor

tremble commented Nov 27, 2020

We got the test policies in place for #200 and I've added a test.

I've identified a bug if you search for a tag that not all keys have:

TASK [aws_kms : Perform a search for key by tag] *******************************
task path: /root/ansible_collections/community/aws/tests/output/.tmp/integration/aws_kms-cs_2o198-ÅÑŚÌβŁÈ/tests/integration/targets/aws_kms/tasks/main.yml:363
An exception occurred during task execution. To see the full traceback, use -vvv. The error was: KeyError: 'Tag Three'
fatal: [testhost]: FAILED! => {"changed": false, "module_stderr": "Traceback (most recent call last):\n  File \"/root/.ansible/tmp/ansible-tmp-1606458494.7582881-995-102501913335682/AnsiballZ_aws_kms_info.py\", line 113, in <module>\n    _ansiballz_main()\n  File \"/root/.ansible/tmp/ansible-tmp-1606458494.7582881-995-102501913335682/AnsiballZ_aws_kms_info.py\", line 105, in _ansiballz_main\n    invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)\n  File \"/root/.ansible/tmp/ansible-tmp-1606458494.7582881-995-102501913335682/AnsiballZ_aws_kms_info.py\", line 54, in invoke_module\n    runpy.run_module(mod_name='ansible_collections.community.aws.plugins.modules.aws_kms_info', init_globals=None, run_name='__main__', alter_sys=True)\n  File \"/usr/lib/python3.6/runpy.py\", line 205, in run_module\n    return _run_module_code(code, init_globals, run_name, mod_spec)\n  File \"/usr/lib/python3.6/runpy.py\", line 96, in _run_module_code\n    mod_name, mod_spec, pkg_name, script_name)\n  File \"/usr/lib/python3.6/runpy.py\", line 85, in _run_code\n    exec(code, run_globals)\n  File \"/tmp/ansible_aws_kms_info_payload_35rp5m58/ansible_aws_kms_info_payload.zip/ansible_collections/community/aws/plugins/modules/aws_kms_info.py\", line 450, in <module>\n  File \"/tmp/ansible_aws_kms_info_payload_35rp5m58/ansible_aws_kms_info_payload.zip/ansible_collections/community/aws/plugins/modules/aws_kms_info.py\", line 446, in main\n  File \"/tmp/ansible_aws_kms_info_payload_35rp5m58/ansible_aws_kms_info_payload.zip/ansible_collections/community/aws/plugins/modules/aws_kms_info.py\", line 446, in <listcomp>\n  File \"/tmp/ansible_aws_kms_info_payload_35rp5m58/ansible_aws_kms_info_payload.zip/ansible_collections/community/aws/plugins/modules/aws_kms_info.py\", line 363, in key_matches_filters\n  File \"/tmp/ansible_aws_kms_info_payload_35rp5m58/ansible_aws_kms_info_payload.zip/ansible_collections/community/aws/plugins/modules/aws_kms_info.py\", line 363, in <listcomp>\n  File \"/tmp/ansible_aws_kms_info_payload_35rp5m58/ansible_aws_kms_info_payload.zip/ansible_collections/community/aws/plugins/modules/aws_kms_info.py\", line 356, in key_matches_filter\nKeyError: 'Tag Three'\n", "module_stdout": "", "msg": "MODULE FAILURE\nSee stdout/stderr for the exact error", "rc": 1}

I expect to fix this with #200

@daurrutia
Copy link
Author

What the exactly error message you got?
I cannot reproduce it.

---
- hosts: localhost
  connection: local
  gather_facts: False

  tasks:
    - name: test
      community.aws.aws_kms_info:
        filters:
          "tags:Name": Example

works fine. With communit.aws 1.0.0 and also with current version 1.2.1

Thanks for looking into it.

The example on the docs page reads "tag:Name": Example.

I only had success with "tags:Name": Example with the plural 's' in the word tags. the example shows the singular form, tag:.

I may be mistaken, but from a brief look I took into the module, it looks like it's written to utilize tags.

It is unknown to me what should be the correct syntax (tag or tags), I just wanted to raise awareness of the discrepancy in what worked for me and the docs example.

@tremble
Copy link
Contributor

tremble commented Nov 27, 2020

The relevant code is

if filtr[0].startswith('tag:'):
    return key['tags'][filtr[0][4:]] == filtr[1]

What's really confusing is that the filter is named 'tag:...' but the data from the 'describe_key' is stored in 'keys'

If you used 'tags:Name' it should actually ignore the filter entirely... I'm not a big fan of the way this is implemented, unfortunately the AWS APIs don't allow for filtering lists of CMKs in the way thay (for example) EC2 instance lists can be filtered.

@tremble
Copy link
Contributor

tremble commented Nov 27, 2020

Thanks for raising the issue, adding an integration test to double check the behaviour did expose that the code's fragile and will break if the tag doesn't exist on all CMKs. I've updated the integration test to include a tag based filter:
https://github.com/ansible-collections/community.aws/pull/200/files#diff-ad6c7386b5fec521ce8b86821c7e2fad0a0713ab5a3375b0c0053a61a13688a1R374-R420

    - name: Perform a search for key by tag
      aws_kms_info:
        filters:
          "tag:Tag Three": "{{ resource_prefix }}"
      register: info_tag_filtered

Hopefully this can give you some level of confidence that the code works. Note: there are 6 copies of this test running in parallel, info_tag_filtered["keys"] | length == 1 means that it really is only

@jatorcasso
Copy link
Contributor

can this issue be closed?

@markuman
Copy link
Member

can this issue be closed?

Imo yes. tags: is/was wrong, and tag: was fixed by @tremble and released with 1.4.0 https://github.com/ansible-collections/community.aws/blob/main/CHANGELOG.rst#id16

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

No branches or pull requests

6 participants