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

vmware: Only add configured network interfaces to facts #28552

Open
wants to merge 1 commit into
base: devel
from

Conversation

@nre-ableton
Copy link

commented Aug 23, 2017

SUMMARY

This change effectively filters out any network interfaces which were not explicitly configured for the guest. This fixes some unexpected behavior where a machine with multiple IP addresses (for example, when Docker is installed, an internal IPv4 interface is added to communicate with the container) would show one of the internal addresses in the 'ipv4' field, but then no other information about the corresponding hardware interface.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

vmware

ANSIBLE VERSION
2.4.0
vmware: Only add configured network interfaces to facts
This change effectively filters out any network interfaces which were
not explicitly configured for the guest. This fixes some unexpected
behavior where a machine with multiple IP addresses (for example, when
Docker is installed, an internal IPv4 interface is added to
communicate with the container) would show one of the internal
addresses in the 'ipv4' field, but then no other information about the
corresponding hardware interface.
@nre-ableton

This comment has been minimized.

Copy link
Author

commented Aug 23, 2017

ping @jctanner & @nerzhul, since both of you have touched the surrounding code somewhat recently. Please let me know if you have any thoughts on this patch. Thanks!

@samdoran samdoran removed the needs_triage label Aug 24, 2017

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2017

@Akasurde

This comment has been minimized.

Copy link
Member

commented Dec 3, 2017

@nre-ableton Thanks for your PR. Could you please write an integration test for this change and let me know ?

@nre-ableton

This comment has been minimized.

Copy link
Author

commented Feb 21, 2018

Hey @Akasurde, sorry for the delay in my reply, I've been busy with some other projects these days. Anyways, I spent some time poking around the Ansible repository and I can't find a good way to write such an acceptance test. I'm quite unfamiliar with the tooling here, so you'll have to forgive me.

At any rate, it seems that the vSphere simulator always returns null in the ipv4 field. If I'm not mistaken, there are not any integration tests which support the wait_for_ip_address attribute. I tried adding that to some of the tests but still saw ipv4: null in the integration test output even for the tests where a static IP address has been set.

If I'm not mistaken, this would need to be supported first before any integration test for this feature would be added. Also, it's quite likely that the vSphere simulator (which I understand is a different project written in golang?) would need to be modified to allow for a mock VM with multiple network interfaces, but only one of them explicitly configured. That would be the ideal case for testing this patch, but I think it exceeds the scope of this PR.

Can you please point me in the right direction? Or do you have any idea how this patch should be tested? Thanks!

@ansibot ansibot added bug and removed bugfix_pull_request labels Mar 2, 2018

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 5, 2018

@ansibot ansibot added the small_patch label Jun 22, 2018

@nre-ableton

This comment has been minimized.

Copy link
Author

commented Jul 18, 2018

re-ping @Akasurde, can you please have another look at this issue? Thanks!

@Akasurde

This comment has been minimized.

Copy link
Member

commented Jul 19, 2018

@nre-ableton Frankly speaking, I am not sure about this. I will add this to next Ansible VMware SIG
IRC meeting. Thanks.

@Akasurde

This comment has been minimized.

Copy link
Member

commented Jul 19, 2018

@jctanner @pdellaert @dericcrago What do you think about this ?

@jctanner

This comment has been minimized.

Copy link
Member

commented Jul 20, 2018

It's hard to comment without seeing an example of the problem.

@nre-ableton

This comment has been minimized.

Copy link
Author

commented Jul 20, 2018

@jctanner The easiest way to provoke this behavior is to try to use the vmware_guest module on a Linux VM which has Docker installed. So for example, in one of our playbooks we have a task file called set-connection-facts.yml, which looks more or less like this:

---
- name: Get facts for the VM
  delegate_to: localhost
  vmware_guest_facts:
    hostname: "{{ vcenter_hostname }}"
    username: "{{ vcenter_user }}"
    password: "{{ vcenter_password }}"
    validate_certs: false
    datacenter: "{{ vcenter_datacenter }}"
    name: "{{ inventory_hostname }}"
  register: vmware_guest_facts_result
  when: vm_guest_facts is not defined

- set_fact:
    vm_guest_facts: "{{ vmware_guest_facts_result }}"
  when: vmware_guest_facts_result.instance is defined

- debug:
    var: vm_guest_facts

- name: Set ansible_host fact with VM's IP address
  set_fact:
    ansible_host: "{{ vm_guest_facts.instance['ipv4'] }}"
  when: vm_guest_facts.instance['ipv4'] is defined

For our Docker-ized Linux VMs, that debug statement prints the following (I have cut all non-essential output to save space):

ok: [ubuntu16.04-staging] => {
    "vm_guest_facts": {
        "changed": false,
        "failed": false,
        "instance": {
            "hw_eth0": {
                "addresstype": "assigned",
                "ipaddresses": [
                    "10.50.0.15",
                    "xxxx::xxx:xxxx:xxxx:xxx"
                ],
                "label": "Netzwerkadapter 1",
                "macaddress": "00:50:56:8b:0e:c0",
                "macaddress_dash": "00-50-56-8b-0e-c0",
            },
            "hw_interfaces": [
                "eth0"
            ],
            "ipv4": "172.17.0.1",
            "ipv6": "xxxx::xxx:xxxx:xxxx:xxx"
        }
    }
}

So the problem here is the instance.ipv4 property, which is 172.17.0.1, which is the address of the docker0 interface on the host. This interface was not configured by VMWare, and hence it is not present in the hw_interfaces list because VMWare doesn't know anything about this interface (nor should it). However, it "steals" the ipv4 value by virtue of the fact that its interface name comes alphabetically before eth0.

I could imagine special firewall/router VMs that may add non-configured interfaces which would cause similar problems. Regardless, I think that only interfaces which have been configured by VMWare should show up in the list of facts, otherwise there are many possibilities for things to go wrong.

@jctanner

This comment has been minimized.

Copy link
Member

commented Jul 20, 2018

Alright, I see the point. I worry though that people are already relying on this behavior for better or worse. Therefore I'd suggest this be socialized to wider audience first or go through some sort of deprecation cycle.

@nre-ableton

This comment has been minimized.

Copy link
Author

commented Jul 20, 2018

@jctanner I agree that would be the prudent thing to do. I have no idea how to go about doing this, though. 😉 This is my first patch to Ansible; I'm mostly eager to abandon our fork of the project, since we rely on this patch for our playbooks to work correctly.

What work do you need from my end in order to merge this patch?

@dericcrago

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2018

In the short term, how bout an option that defaults to the old behavior?

@dagwieers

This comment has been minimized.

Copy link
Member

commented Jul 23, 2018

Wouldn't it be easier to select the IP address you need from the interface you need? I would much rather expose more information about all interfaces so it becomes easier to make a selection that fits better.

It may require improvements to the data structure (e.g. IPv4 and IPv6 addresses separated), better jinja-filters (e.g. to select IPv4 addresses from a list). From the example, this would work: vm_guest_facts.instance.hw_eth0.ipaddresses[0] but it may not give a reliable result.

There are more advanced techniques using Jinja to select information.

@nre-ableton

This comment has been minimized.

Copy link
Author

commented Jul 23, 2018

@dagwieers Yes, I can see where you're coming from. Just to be clear, the module currently does not sort addresses in the hw_eth0.ipaddresses list, so one cannot know whether vm_guest_facts.instance.hw_eth0.ipaddresses[0] is an IPv4 or IPv6 address (I've been bitten by this in the past). Maybe it would make sense to add lists for hw_eth0.ipv4addresses and hw_eth0.ipv6addresses?

At any rate, I think that getting the IP address of the host ought to be pretty straightforward and simple. I like the ipv4 and ipv6 properties exposed on the instance, but admittedly this patch doesn't solve the issue where a VM has multiple configured network interfaces.

Regardless, I still feel that this patch reflects the "correct" behavior. The alternative would be to also list non-configured IP addresses (like docker0 for my example) in the hw_interfaces list, though I have a feeling that will be more difficult to accomplish.

@dagwieers

This comment has been minimized.

Copy link
Member

commented Jul 23, 2018

It is not hard to create a Jinja filter that selects IPv4 or IPv6 addresses. Who knows, maybe it already exists?

Looking at plugins/filter/ipaddr.py, none of the filters have been documented.

@nre-ableton

This comment has been minimized.

Copy link
Author

commented Jul 23, 2018

@dagwieers it does exist, see: https://docs.ansible.com/ansible/2.5/user_guide/playbooks_filters_ipaddr.html

There are already filters for ipv4 and ipv6, so I will make a workaround in our playbooks along these lines:

- name: Set ansible_host fact with VM's IP address
  set_fact:
    ansible_host: "{{ item }}"
  when: item | ipv4
  with_items: "{{ vm_guest_facts.instance.hw_eth0.ipaddresses }}"

That seems to work fine, but again, I still wish that I could rely on the ipv4 field instead.

@dagwieers

This comment has been minimized.

Copy link
Member

commented Jul 23, 2018

@nre-ableton What I rather was looking for is this:

- set_fact:
    ansible_host: '{{ vm_guest_facts.instance.hw_eth0.ipaddresses|ipv4|first }}'

So yes, that's pretty close. Now imagine that you have 5 interfaces, and you would like to select the one that has the default gateway routed. Or the one that is in a specific range (or not in a specific range), etc... All of this is possible without having the ipv4 or ipv6 key, or any logic built-in.

So if anything, I would get rid of ipv4 or ipv6. But I realize that is a dream rather than practically possible.

@nre-ableton

This comment has been minimized.

Copy link
Author

commented Jul 23, 2018

@dagwieers Ah, ok, I misunderstood you. Yes, such a filter would be nice but I agree it's probably not worth the cost of implementing it.

Also, I agree that getting rid of the ipv4 and ipv6 properties would be the way to go, due not only to the issues with configured/non-configured interfaces, but also for VMs with multiple interfaces. However, there are probably way too many users who rely on these properties so deprecating them is likely to be a difficult task.

In the meantime, what should we do about this PR?

@dagwieers

This comment has been minimized.

Copy link
Member

commented Jul 23, 2018

@nre-ableton The example I showed works fine. Doing something more advanced is going to be harder in Jinja (with the current data-structure), uglier but not impossible. Enumerating interfaces and processing them without needing two different entries would make a real difference.

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2018

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

@dagwieers dagwieers added the cloud label Feb 28, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

@ansibot ansibot added the has_issue label Jul 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.