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

Fix IPv6 zone handling #66

Merged
merged 4 commits into from
Jan 25, 2021

Conversation

felixfontein
Copy link
Collaborator

SUMMARY

Fixes #65. The same problem also appears in docker_network's CIDR validation. I don't know whether Docker supports zones here, but I don't see why we should restrict to non-zone CIDRs.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

docker_container
docker_network

@WojciechowskiPiotr
Copy link
Collaborator

Is there any way we can test IPv6 zones in integration tests?

@felixfontein
Copy link
Collaborator Author

@WojciechowskiPiotr probably not in a sane way. I'm not sure which values are actually valid for it, and it probably depends on the system running the docker daemon, which at least in the dockerized tests we have no access to.

@WojciechowskiPiotr
Copy link
Collaborator

@WojciechowskiPiotr probably not in a sane way. I'm not sure which values are actually valid for it, and it probably depends on the system running the docker daemon, which at least in the dockerized tests we have no access to.

Did you manage to manually test the patch?

@felixfontein
Copy link
Collaborator Author

No. I only tested the regex itself. I'm hoping that @kristof-mattei could try it out.

@WojciechowskiPiotr
Copy link
Collaborator

No. I only tested the regex itself. I'm hoping that @kristof-mattei could try it out.

Patch looks fine, but let's test properly it before merging

@kristof-mattei
Copy link
Contributor

@WojciechowskiPiotr I'd love to test this, but any pointers how?

@felixfontein
Copy link
Collaborator Author

@kristof-mattei you can use the way described here: https://docs.ansible.com/ansible/devel/user_guide/collections_using.html#installing-a-collection-from-a-git-repository Here, it would beansible-galaxy collection install git+https://github.com/felixfontein/community.docker.git,ipv6-zones. Just make sure to eventually delete the manually installed collection, because otherwise it will hide updated versions of the collections installed with Ansible (if you are using Ansible 2.10).

Alternatively (if you don't mind working with git) you could use the way described here (https://docs.ansible.com/ansible/devel/dev_guide/developing_collections.html#contributing-to-collections, third paragraph+). You need to check out the ipv6-zones branch of https://github.com/felixfontein/community.docker/.

@felixfontein
Copy link
Collaborator Author

@kristof-mattei did you had a chance to test this yet?

@felixfontein felixfontein force-pushed the ipv6-zones branch 2 times, most recently from 47f0daa to f4cbebd Compare January 25, 2021 06:33
@felixfontein
Copy link
Collaborator Author

I managed to test it for docker_container, it seems to work fine. I couldn't test it for docker_network (resp. Docker daemon always complained about an invalid CIDR), so I removed the change for that one.

It looks like an arbitrary zone name works. If Docker daemon ever starts
validating it (against what?) we either have to try to fix this test by
a valid value, or remove it again.
@felixfontein
Copy link
Collaborator Author

As Docker daemon seems to allow a random zone, we're able to (somewhat) test this in CI.

Copy link
Collaborator

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

LGTM

@felixfontein felixfontein merged commit c1e6c2e into ansible-collections:main Jan 25, 2021
@felixfontein felixfontein deleted the ipv6-zones branch January 25, 2021 14:16
@felixfontein
Copy link
Collaborator Author

@WojciechowskiPiotr @Andersson007 thanks for reviewing this!

felixfontein added a commit to felixfontein/community.general that referenced this pull request Jan 25, 2021
felixfontein added a commit to ansible-collections/community.general that referenced this pull request Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docker_container: IPv6 port validation doesn't allow for interface specification.
4 participants