From 8be3b3ef4d27e6484e3dc07c931714f5929b0387 Mon Sep 17 00:00:00 2001
From: Arnaud Patard <apatard@hupstream.com>
Date: Wed, 15 Feb 2023 17:02:47 +0100
Subject: [PATCH] vagrant: Improve network interfaces support

Current code doesn't ensure that the network name is valid. According
to current Vagrant documentation, it can only be:
- public_network
- private_network
- forwarded_port

Moreover, the same documentation is saying that public network configuration
may be reduced to:

config.vm.network "public_network"

So add needed code to support interfaces without any options.

On the test side:
- modify the network test to check that the generated Vagrantfile is correct
when no option is specified
- add a test to ensure invalid network name are caught.

Fixes: #99
Signed-off-by: Arnaud Patard <apatard@hupstream.com>
---
 src/molecule_plugins/vagrant/modules/vagrant.py  | 13 ++++++++++---
 test/vagrant/functional/test_func.py             | 16 ++++++++++++++++
 .../scenarios/molecule/invalid_net/converge.yml  | 10 ++++++++++
 .../scenarios/molecule/invalid_net/molecule.yml  | 11 +++++++++++
 .../scenarios/molecule/network/molecule.yml      |  1 +
 .../scenarios/molecule/network/verify.yml        |  4 ++--
 6 files changed, 50 insertions(+), 5 deletions(-)
 create mode 100644 test/vagrant/scenarios/molecule/invalid_net/converge.yml
 create mode 100644 test/vagrant/scenarios/molecule/invalid_net/molecule.yml

diff --git a/src/molecule_plugins/vagrant/modules/vagrant.py b/src/molecule_plugins/vagrant/modules/vagrant.py
index 1b9a3569..1762fb8c 100755
--- a/src/molecule_plugins/vagrant/modules/vagrant.py
+++ b/src/molecule_plugins/vagrant/modules/vagrant.py
@@ -176,6 +176,7 @@
 See doc/source/configuration.rst
 """
 
+VAGRANT_VALID_NETNAMES = ["private_network", "public_network", "forwarded_port"]
 VAGRANTFILE_TEMPLATE = """
 {%- macro ruby_format(value) -%}
   {%- if value is boolean -%}
@@ -244,7 +245,7 @@
     # Network
     ##
     {% for n in instance.networks %}
-    c.vm.network "{{ n.name }}", {{ dict2args(n.options) }}
+    c.vm.network "{{ n.name }}"{% if 'options' in n %}, {{ dict2args(n.options) }}{% endif %}
     {% endfor %}
 
     ##
@@ -617,10 +618,16 @@ def _get_instance_vagrant_config_dict(self, instance):
         networks = []
         if "interfaces" in instance:
             for iface in instance["interfaces"]:
+                net_name = iface["network_name"]
+                if net_name not in VAGRANT_VALID_NETNAMES:
+                    self._module.fail_json(
+                        msg=f"Invalid network_name value {net_name}."
+                    )
                 net = {}
-                net["name"] = iface["network_name"]
+                net["name"] = net_name
                 iface.pop("network_name")
-                net["options"] = iface
+                if len(iface) > 0:
+                    net["options"] = iface
                 networks.append(net)
 
         # compat
diff --git a/test/vagrant/functional/test_func.py b/test/vagrant/functional/test_func.py
index 62554e19..fb127c88 100644
--- a/test/vagrant/functional/test_func.py
+++ b/test/vagrant/functional/test_func.py
@@ -100,6 +100,22 @@ def test_invalid_settings(temp_dir):
     not is_vagrant_supported(),
     reason="vagrant not supported on this machine",
 )
+def test_invalid_network_name(temp_dir):
+    scenario_directory = os.path.join(
+        os.path.dirname(util.abs_path(__file__)), os.path.pardir, "scenarios"
+    )
+
+    with change_dir_to(scenario_directory):
+        cmd = ["molecule", "create", "--scenario-name", "invalid_net"]
+        result = run_command(cmd)
+        assert result.returncode == 2
+
+        assert "Invalid network_name value my_network." in result.stdout
+
+
+@pytest.mark.skipif(
+    not is_vagrant_supported(), reason="vagrant not supported on this machine"
+)
 @pytest.mark.parametrize(
     "scenario",
     [
diff --git a/test/vagrant/scenarios/molecule/invalid_net/converge.yml b/test/vagrant/scenarios/molecule/invalid_net/converge.yml
new file mode 100644
index 00000000..25a0e0d2
--- /dev/null
+++ b/test/vagrant/scenarios/molecule/invalid_net/converge.yml
@@ -0,0 +1,10 @@
+---
+- name: Converge
+  hosts: all
+  gather_facts: false
+  become: true
+  tasks:
+    - name: Sample task  # noqa command-instead-of-shell
+      ansible.builtin.shell:
+        cmd: uname
+      changed_when: false
diff --git a/test/vagrant/scenarios/molecule/invalid_net/molecule.yml b/test/vagrant/scenarios/molecule/invalid_net/molecule.yml
new file mode 100644
index 00000000..ca87d759
--- /dev/null
+++ b/test/vagrant/scenarios/molecule/invalid_net/molecule.yml
@@ -0,0 +1,11 @@
+---
+dependency:
+  name: galaxy
+driver:
+  name: vagrant
+platforms:
+  - name: instance/foo
+    interfaces:
+      - network_name: my_network
+provisioner:
+  name: ansible
diff --git a/test/vagrant/scenarios/molecule/network/molecule.yml b/test/vagrant/scenarios/molecule/network/molecule.yml
index beda0944..59593dc0 100644
--- a/test/vagrant/scenarios/molecule/network/molecule.yml
+++ b/test/vagrant/scenarios/molecule/network/molecule.yml
@@ -15,6 +15,7 @@ platforms:
       - network_name: private_network
         ip: 192.168.56.4
         auto_config: true
+      - network_name: public_network
 provisioner:
   name: ansible
 verifier:
diff --git a/test/vagrant/scenarios/molecule/network/verify.yml b/test/vagrant/scenarios/molecule/network/verify.yml
index 2d4e3530..01acae4a 100644
--- a/test/vagrant/scenarios/molecule/network/verify.yml
+++ b/test/vagrant/scenarios/molecule/network/verify.yml
@@ -5,7 +5,7 @@
   gather_subset:
     - network
   tasks:
-    - name: Check that there are 3 interfaces
+    - name: Check that there are 4 interfaces
       ansible.builtin.assert:
         that:
-          - "{{ ansible_interfaces | length == 3 }}"
+          - "{{ ansible_interfaces | length == 4 }}"