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

Flexible hostname for Scaleway Dynamic Inventory #41658

Merged
merged 1 commit into from
Jul 17, 2018

Conversation

remyleone
Copy link
Contributor

SUMMARY

Using IP address is more common to index servers. This PR changes the key to index machines from scaleway id to its public IP.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME
  • Scaleway dynamic inventory
ANSIBLE VERSION
$ ansible --version
ansible 2.7.0.dev0 (key_ip 251266f80b) last updated 2018/06/18 18:30:36 (GMT +200)
  config file = None
  configured module search path = [u'/Users/sieben/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/sieben/workspace/ansible/lib/ansible
  executable location = /Users/sieben/workspace/ansible/bin/ansible
  python version = 2.7.15 (default, Jun 17 2018, 12:46:58) [GCC 4.2.1 Compatible Apple LLVM 9.1.0 (clang-902.0.39.2)]

@remyleone
Copy link
Contributor Author

remyleone commented Jun 18, 2018

Could you tell me how could we handle machines that got only private IP? What about IPv6 addresses? What are other plugins doing typically?

@ansibot ansibot added affects_2.7 This issue/PR affects Ansible v2.7 bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jun 18, 2018
@s-hertel
Copy link
Contributor

s-hertel commented Jun 18, 2018

In the aws_ec2 inventory plugin this is a configurable option.

plugin: aws_ec2
regions:
  - us-east-1
hostnames:
  - tag:CustomDNSName
  - dns-name
  - private-ip-address
  - ...

hostnames is a list in order of precedence, it checks each, until it finds an attribute that exists for the new host. If none of the possible hostnames exist for the new host then it doesn't add it to inventory. You could implement something similar for scaleway, defaulting your hostnames to a list of what you think is reasonable precedence.

I'm assuming this is causing a traceback right now because the key doesn't exist. But if it isn't causing a traceback, a workaround could be using the compose features to change the ansible_host. In the aws_ec2 inventory plugin this would look like:

plugin: aws_ec2
regions:
  - us-east-1
compose:
  # if the new host has a 'private_ip_address' variable set ansible_host with it
  ansible_host: private_ip_address

Compose won't change the hostname listed with ansible-inventory --graph (inventory_hostname), but it will change the IP ansible connects to.

@s-hertel s-hertel removed the needs_triage Needs a first human triage before being processed. label Jun 18, 2018
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jun 18, 2018
@ansibot ansibot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jun 20, 2018
@remyleone remyleone force-pushed the key_ip branch 2 times, most recently from 8a5d863 to 5039500 Compare June 26, 2018 15:26
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jun 26, 2018
@remyleone
Copy link
Contributor Author

@s-hertel Thanks a lot for the explanation. I've started building a similar feature.

@remyleone remyleone force-pushed the key_ip branch 3 times, most recently from ad6868f to 275998e Compare July 2, 2018 12:18
@remyleone
Copy link
Contributor Author

@pilou- @maxamillion Could you take a look? I think the dynamic inventory in scaleway starts to be way more useful with this :)

@remyleone
Copy link
Contributor Author

@remyleone remyleone changed the title Use IP address instead of server id to index servers Flexible hostname for Scaleway Dynamic Inventory Jul 2, 2018
@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jul 3, 2018
Copy link
Contributor

@s-hertel s-hertel left a comment

Choose a reason for hiding this comment

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

This looks great to me. It would be good if someone using scaleway could test it - @pilou- are you able to?

@remyleone
Copy link
Contributor Author

Could it be backported to 2.6 ?

@mhalano
Copy link
Contributor

mhalano commented Jul 9, 2018

I tested using "ansible-inventory" command and worked great. The only question is why I can use 'ping' module to a IPv6 address, but this question doesn't seems related to this pull request. The only problem is in this part of code:

...
                - public_ipv4
            choices:
                choices:
                  - public_ipv4
                  - private_ipv6
...

You repeated "choices:" two times, which avoid the possible values to field to appear on help.

@ansibot ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jul 9, 2018
@remyleone
Copy link
Contributor Author

@mhalano fixed

@ansibot ansibot removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jul 9, 2018
@maxamillion
Copy link
Contributor

rebuild_merge

@remyleone
Copy link
Contributor Author

@maxamillion What could we could do to get it merge. I don't think the build fails because of my MR.

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jul 13, 2018
@maxamillion
Copy link
Contributor

rebuild_merge

@remyleone
Copy link
Contributor Author

@maxamillion I've rebased and pushed again. I hope I will get lucky with the CI this time.

@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jul 16, 2018
@maxamillion
Copy link
Contributor

rebuild_merge

@remyleone
Copy link
Contributor Author

The CI is happy :) could this get merged and backported?

@ansibot ansibot merged commit 2177d37 into ansible:devel Jul 17, 2018
@remyleone remyleone deleted the key_ip branch July 17, 2018 08:52
@mhalano
Copy link
Contributor

mhalano commented Jul 17, 2018

@sieben I think this change is big and not security related, so I wouldn't a good idea to backport that. My $0.02.

@maxamillion
Copy link
Contributor

I think this would be a feature and not so much "bugfix" or "security" so I'm not sure it's a candidate for backport, but if there's a reason this falls under one of those categories and I'm simply lacking perspective, I'm open to discussion on the topic.

@sklemmer
Copy link

@maxamillion Why do you think that this is not a bugfix? Currently the inventory is unusable in version 2.6 as ansible tries to connect using the server id.

@s-hertel
Copy link
Contributor

s-hertel commented Sep 17, 2018

@sklemmer This added a new option. Sometimes features and bugfixes overlap. If you need this on an older version you can add the corrected plugin code in your plugin path and make sure it is enabled if you need the functionality now (the same idea as putting custom/modified modules in your library directory for Ansible to use). Or else as a workaround I'd suggest either using an additional config file for the constructed plugin to be paired with your 2.6 inventory plugin to correct the hostname (see my suggestion here #41658 (comment)) where your constructed config would contain something along the lines of:

plugin: constructed
compose:
  # if the new host has a 'private_ip_address' variable set ansible_host with it
  ansible_host: private_ip_address

You can use ansible-inventory -i yourscalewayconfig.yml -i yourconstructedconfig.yml --list to check that your ansible_host host vars are set the the correct value. (Note you'll need to make sure either the constructed inventory plugin or the auto inventory plugin is also enabled).

@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.7 This issue/PR affects Ansible v2.7 bug This issue/PR relates to a bug. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants