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

Add support for ipv6 addresses #430

Merged

Conversation

abhattacharyaNS1
Copy link
Contributor

@abhattacharyaNS1 abhattacharyaNS1 commented Aug 2, 2021

SUMMARY

Allow amazon.aws.aws_service_ip_ranges to return only IPv6 addresses with setting ipv6_prefixes=True

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

amazon.aws.aws_service_ip_ranges

ADDITIONAL INFORMATION
vars:
  rt53_ranges: "{{ lookup('aws_service_ip_ranges', region='us-west-2', service='ROUTE53_HEALTHCHECKS', ipv6_prefix=True, wantlist=True) }}"
tasks:

- name: "use list return option and iterate as a loop"
  debug: msg="{% for x in rt53_ranges %}{{ x }} {% endfor %}"
# "2600:1f14:7ff:f800::/56,2600:1f14:fff:f800::/56"

Closes #438

@ansibullbot ansibullbot added community_review feature This issue/PR relates to a feature request lookup lookup plugin needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) labels Aug 2, 2021
Copy link
Contributor Author

@abhattacharyaNS1 abhattacharyaNS1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed formatting changes

@ansibullbot

This comment has been minimized.

@ansibullbot ansibullbot added merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed community_review labels Aug 9, 2021
@ansibullbot ansibullbot added community_review and removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Aug 9, 2021
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to raise this PR.

The kwargs logic currently looks a little broken. I've added some suggestions as to how to fix this.

If possible please also add some integration tests. There are currently two examples of integration tests for lookups:

It would be really good if these tests could test

  • unlimited lookup
  • limiting by region
  • limiting by service
  • limiting by region and service
  • setting ipv6_prefix(es) to True
  • setting ipv6_prefix(es) to False

I'd recommend just checking that the returned values are valid ipv4/ipv6 CIDRs, since we have no guarantees what IP addresses Amazon would use.

plugins/lookup/aws_service_ip_ranges.py Outdated Show resolved Hide resolved
plugins/lookup/aws_service_ip_ranges.py Outdated Show resolved Hide resolved
@tremble
Copy link
Contributor

tremble commented Aug 10, 2021

Integration tests are simply an Ansible role in the tests/integration/targets directory named after the module being tested (lookups are prefixed with lookup_.

@tremble
Copy link
Contributor

tremble commented Aug 10, 2021

@gravesm
Copy link
Member

gravesm commented Aug 10, 2021

recheck

@tremble
Copy link
Contributor

tremble commented Aug 25, 2021

recheck

@ansibullbot
Copy link

@abhattacharyaNS1 this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibullbot ansibullbot added merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed community_review labels Aug 25, 2021
@ansibullbot
Copy link

@abhattacharyaNS1 this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibullbot ansibullbot added module module new_plugin New plugin labels Aug 25, 2021
@ansibullbot ansibullbot added community_review integration tests/integration tests tests and removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Sep 10, 2021
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor tweaks

@tremble tremble force-pushed the allow-ipv6-ipranges branch from 60df222 to e810e8e Compare September 11, 2021 06:46
@abhattacharyaNS1
Copy link
Contributor Author

@tremble Thanks for the fix! Please let me know if there is anything else i need to add to this PR.

Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly niggles, I'll merge these, if they pass CI we can get it into main.

@tremble tremble added gate and removed gate labels Sep 16, 2021
Copy link
Contributor

@ansible-zuul ansible-zuul bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ansible-zuul ansible-zuul bot merged commit 71de845 into ansible-collections:main Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community_review feature This issue/PR relates to a feature request has_issue integration tests/integration lookup lookup plugin new_contributor Help guide this first time contributor plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws_service_ip_ranges suppport for ipv6
5 participants