Only add or remove router interfaces when needed

Previously, when updating a router all its interfaces where removed and
readded by Ansible's os_router module. As a unwanted side effect all
active connections of the router and nat'ed devices where dropped,
closing e.g. all active tcp sessions. Now, only necessary changes
are applied.

Task: 40136
Story: 2007845
Change-Id: I172caf360e6e342dd54865da5a5b72b0dc0205c8

ci/roles/router/tasks/main.yml

@@ -15,6 +15,30 @@
      name: shade_subnet1
      cidr: 10.7.7.0/24
 
+- name: Create subnet2
+  openstack.cloud.subnet:
+     cloud: "{{ cloud }}"
+     state: present
+     network_name: "{{ network_name }}"
+     name: shade_subnet2
+     cidr: 10.8.8.0/24
+
+- name: Create subnet3
+  openstack.cloud.subnet:
+     cloud: "{{ cloud }}"
+     state: present
+     network_name: "{{ network_name }}"
+     name: shade_subnet3
+     cidr: 10.9.9.0/24
+
+- name: Create subnet4
+  openstack.cloud.subnet:
+     cloud: "{{ cloud }}"
+     state: present
+     network_name: "{{ network_name }}"
+     name: shade_subnet4
+     cidr: 10.10.10.0/24
+
 - name: Create router
   openstack.cloud.router:
      cloud: "{{ cloud }}"
@@ -29,6 +53,19 @@
      interfaces:
          - shade_subnet1
 
+- name: Update router (add interface) again
+  openstack.cloud.router:
+     cloud: "{{ cloud }}"
+     state: present
+     name: "{{ router_name }}"
+     interfaces:
+         - shade_subnet1
+  register: result
+
+- name: Assert idempotent module
+  assert:
+    that: result is not changed
+
 - name: Gather routers info
   openstack.cloud.routers_info:
      cloud: "{{ cloud }}"
@@ -43,6 +80,93 @@
       - "result.openstack_routers.0.name == router_name"
       - (result.openstack_routers.0.interfaces_info|length) == 1
 
+- name: Update router (change interfaces)
+  openstack.cloud.router:
+     cloud: "{{ cloud }}"
+     state: present
+     name: "{{ router_name }}"
+     interfaces:
+         - net: '{{ network_name }}'
+           subnet: shade_subnet2
+           portip: 10.8.8.1
+         - net: '{{ network_name }}'
+           subnet: shade_subnet3
+         - shade_subnet4
+
+- name: Update router (change interfaces) again
+  openstack.cloud.router:
+     cloud: "{{ cloud }}"
+     state: present
+     name: "{{ router_name }}"
+     interfaces:
+         - net: '{{ network_name }}'
+           subnet: shade_subnet2
+           portip: 10.8.8.1
+         - net: '{{ network_name }}'
+           subnet: shade_subnet3
+         - shade_subnet4
+  register: result
+
+- name: Assert idempotent module
+  assert:
+    that: result is not changed
+
+- name: Gather routers info
+  openstack.cloud.routers_info:
+     cloud: "{{ cloud }}"
+     name: "{{ router_name }}"
+     filters:
+       admin_state_up: true
+  register: result
+
+- name: Verify routers info
+  assert:
+    that:
+      - "result.openstack_routers.0.name == router_name"
+      - (result.openstack_routers.0.interfaces_info|length) == 3
+      - result.openstack_routers.0.interfaces_info|map(attribute='ip_address')|sort|list ==
+        ['10.10.10.1', '10.8.8.1', '10.9.9.1']
+
+- name: Update router (remove interface)
+  openstack.cloud.router:
+     cloud: "{{ cloud }}"
+     state: present
+     name: "{{ router_name }}"
+     interfaces:
+         - net: '{{ network_name }}'
+           subnet: shade_subnet1
+           portip: 10.7.7.1
+
+- name: Update router (remove interface) again
+  openstack.cloud.router:
+     cloud: "{{ cloud }}"
+     state: present
+     name: "{{ router_name }}"
+     interfaces:
+         - net: '{{ network_name }}'
+           subnet: shade_subnet1
+           portip: 10.7.7.1
+  register: result
+
+- name: Assert idempotent module
+  assert:
+    that: result is not changed
+
+- name: Gather routers info
+  openstack.cloud.routers_info:
+     cloud: "{{ cloud }}"
+     name: "{{ router_name }}"
+     filters:
+       admin_state_up: true
+  register: result
+
+- name: Verify routers info
+  assert:
+    that:
+      - "result.openstack_routers.0.name == router_name"
+      - (result.openstack_routers.0.interfaces_info|length) == 1
+      - result.openstack_routers.0.interfaces_info.0.ip_address == '10.7.7.1'
+
 # Admin operation
 - name: Create external network
   openstack.cloud.network:
@@ -53,12 +177,12 @@
   when:
     - network_external
 
-- name: Create subnet2
+- name: Create subnet5
   openstack.cloud.subnet:
      cloud: "{{ cloud }}"
      state: present
      network_name: "{{ external_network_name }}"
-     name: shade_subnet2
+     name: shade_subnet5
      cidr: 10.6.6.0/24
   when:
     - network_external
@@ -88,6 +212,142 @@
       - "result.openstack_routers.0.name == router_name"
       - (result.openstack_routers.0.interfaces_info|length) == 1
 
+- name: Update router (change external fixed ips)
+  openstack.cloud.router:
+     cloud: "{{ cloud }}"
+     state: present
+     name: "{{ router_name }}"
+     interfaces:
+         - shade_subnet1
+     network: "{{ external_network_name }}"
+     external_fixed_ips:
+        - subnet: shade_subnet5
+          ip: 10.6.6.100
+  when:
+    - network_external
+
+- name: Gather routers info
+  openstack.cloud.routers_info:
+     cloud: "{{ cloud }}"
+     name: "{{ router_name }}"
+     filters:
+       admin_state_up: true
+  register: result
+
+- name: Verify routers info
+  assert:
+    that:
+      - "result.openstack_routers.0.name == router_name"
+      - (result.openstack_routers.0.external_gateway_info.external_fixed_ips|length) == 1
+      - result.openstack_routers.0.external_gateway_info.external_fixed_ips.0.ip_address == "10.6.6.100"
+  when:
+    - network_external
+
+- name: Update router (add external fixed ips)
+  openstack.cloud.router:
+     cloud: "{{ cloud }}"
+     state: present
+     name: "{{ router_name }}"
+     interfaces:
+         - shade_subnet1
+     network: "{{ external_network_name }}"
+     external_fixed_ips:
+        - subnet: shade_subnet5
+          ip: 10.6.6.100
+        - subnet: shade_subnet5
+          ip: 10.6.6.101
+  when:
+    - network_external
+
+- name: Update router (add external fixed ips) again
+  openstack.cloud.router:
+     cloud: "{{ cloud }}"
+     state: present
+     name: "{{ router_name }}"
+     interfaces:
+         - shade_subnet1
+     network: "{{ external_network_name }}"
+     external_fixed_ips:
+        - subnet: shade_subnet5
+          ip: 10.6.6.100
+        - subnet: shade_subnet5
+          ip: 10.6.6.101
+  when:
+    - network_external
+  register: result
+
+- name: Assert idempotent module
+  assert:
+    that: result is not changed
+
+- name: Gather routers info
+  openstack.cloud.routers_info:
+     cloud: "{{ cloud }}"
+     name: "{{ router_name }}"
+     filters:
+       admin_state_up: true
+  register: result
+
+- name: Verify routers info
+  assert:
+    that:
+      - "result.openstack_routers.0.name == router_name"
+      - (result.openstack_routers.0.external_gateway_info.external_fixed_ips|length) == 2
+      - result.openstack_routers.0.external_gateway_info.external_fixed_ips|map(attribute='ip_address')|sort|list ==
+        ["10.6.6.100", "10.6.6.101"]
+  when:
+    - network_external
+
+- name: Update router (remove external fixed ips)
+  openstack.cloud.router:
+     cloud: "{{ cloud }}"
+     state: present
+     name: "{{ router_name }}"
+     interfaces:
+         - shade_subnet1
+     network: "{{ external_network_name }}"
+     external_fixed_ips:
+        - subnet: shade_subnet5
+          ip: 10.6.6.101
+  when:
+    - network_external
+
+- name: Update router (remove external fixed ips) again
+  openstack.cloud.router:
+     cloud: "{{ cloud }}"
+     state: present
+     name: "{{ router_name }}"
+     interfaces:
+         - shade_subnet1
+     network: "{{ external_network_name }}"
+     external_fixed_ips:
+        - subnet: shade_subnet5
+          ip: 10.6.6.101
+  when:
+    - network_external
+  register: result
+
+- name: Assert idempotent module
+  assert:
+    that: result is not changed
+
+- name: Gather routers info
+  openstack.cloud.routers_info:
+     cloud: "{{ cloud }}"
+     name: "{{ router_name }}"
+     filters:
+       admin_state_up: true
+  register: result
+
+- name: Verify routers info
+  assert:
+    that:
+      - "result.openstack_routers.0.name == router_name"
+      - (result.openstack_routers.0.external_gateway_info.external_fixed_ips|length) == 1
+      - result.openstack_routers.0.external_gateway_info.external_fixed_ips.0.ip_address == "10.6.6.101"
+  when:
+    - network_external
+
 - name: Delete router
   openstack.cloud.router:
      cloud: "{{ cloud }}"
@@ -105,6 +365,24 @@
      cloud: "{{ cloud }}"
      state: absent
      name: shade_subnet2
+
+- name: Delete subnet3
+  openstack.cloud.subnet:
+     cloud: "{{ cloud }}"
+     state: absent
+     name: shade_subnet3
+
+- name: Delete subnet4
+  openstack.cloud.subnet:
+     cloud: "{{ cloud }}"
+     state: absent
+     name: shade_subnet4
+
+- name: Delete subnet5
+  openstack.cloud.subnet:
+     cloud: "{{ cloud }}"
+     state: absent
+     name: shade_subnet5
   when:
     - network_external
 

plugins/modules/router.py

@@ -211,13 +211,7 @@ router:
 '''
 
 from ansible_collections.openstack.cloud.plugins.module_utils.openstack import OpenStackModule
-
+import itertools
-
-ROUTER_INTERFACE_OWNERS = set([
-    'network:router_interface',
-    'network:router_interface_distributed',
-    'network:ha_router_replicated_interface'
-])
 
 
 class RouterModule(OpenStackModule):
@@ -232,14 +226,17 @@ class RouterModule(OpenStackModule):
         project=dict(default=None)
     )
 
-    def _router_internal_interfaces(self, router):
+    def _get_subnet_ids_from_ports(self, ports):
-        for port in self.conn.list_router_interfaces(router, 'internal'):
+        return [fixed_ip['subnet_id'] for fixed_ip in
-            if port['device_owner'] in ROUTER_INTERFACE_OWNERS:
+                itertools.chain.from_iterable(port['fixed_ips'] for port in ports if 'fixed_ips' in port)]
-                yield port
 
-    def _needs_update(self, router, network, internal_subnet_ids, internal_port_ids, filters=None):
+    def _needs_update(self, router, net,
-        """Decide if the given router needs an update.
+                      missing_port_ids,
-        """
+                      requested_subnet_ids,
+                      existing_subnet_ids,
+                      router_ifs_cfg,
+                      filters=None):
+        """Decide if the given router needs an update."""
         if router['admin_state_up'] != self.params['admin_state_up']:
             return True
         if router['external_gateway_info']:
@@ -247,23 +244,21 @@ class RouterModule(OpenStackModule):
             if self.params['enable_snat'] is not None:
                 if router['external_gateway_info'].get('enable_snat', True) != self.params['enable_snat']:
                     return True
-        if network:
+        if net:
             if not router['external_gateway_info']:
                 return True
-            elif router['external_gateway_info']['network_id'] != network['id']:
+            elif router['external_gateway_info']['network_id'] != net['id']:
                 return True
 
-        # check external interfaces
+        # check if external_fixed_ip has to be added
-        if self.params['external_fixed_ips']:
+        for external_fixed_ip in router_ifs_cfg['external_fixed_ips']:
-            for new_iface in self.params['external_fixed_ips']:
-                subnet = self.conn.get_subnet(new_iface['subnet'], filters)
             exists = False
 
             # compare the requested interface with existing, looking for an existing match
-                for existing_iface in router['external_gateway_info']['external_fixed_ips']:
+            for existing_if in router['external_gateway_info']['external_fixed_ips']:
-                    if existing_iface['subnet_id'] == subnet['id']:
+                if existing_if['subnet_id'] == external_fixed_ip['subnet_id']:
-                        if 'ip' in new_iface:
+                    if 'ip' in external_fixed_ip:
-                            if existing_iface['ip_address'] == new_iface['ip']:
+                        if existing_if['ip_address'] == external_fixed_ip['ip']:
                             # both subnet id and ip address match
                             exists = True
                             break
@@ -276,39 +271,49 @@ class RouterModule(OpenStackModule):
             if not exists:
                 return True
 
-        # check internal interfaces
+        # check if external_fixed_ip has to be removed
-        if self.params['interfaces']:
+        if router_ifs_cfg['external_fixed_ips']:
-            existing_subnet_ids = []
+            for external_fixed_ip in router['external_gateway_info']['external_fixed_ips']:
-            for port in self._router_internal_interfaces(router):
+                obsolete = True
-                if 'fixed_ips' in port:
-                    for fixed_ip in port['fixed_ips']:
-                        existing_subnet_ids.append(fixed_ip['subnet_id'])
 
-            for iface in self.params['interfaces']:
+                # compare the existing interface with requested, looking for an requested match
-                if isinstance(iface, dict):
+                for requested_if in router_ifs_cfg['external_fixed_ips']:
-                    for p_id in internal_port_ids:
+                    if external_fixed_ip['subnet_id'] == requested_if['subnet_id']:
-                        p = self.conn.get_port(name_or_id=p_id)
+                        if 'ip' in requested_if:
-                        if 'fixed_ips' in p:
+                            if external_fixed_ip['ip_address'] == requested_if['ip']:
-                            for fip in p['fixed_ips']:
+                                # both subnet id and ip address match
-                                internal_subnet_ids.append(fip['subnet_id'])
+                                obsolete = False
+                                break
+                        else:
+                            # only the subnet was given, so ip doesn't matter
+                            obsolete = False
+                            break
 
-            if set(internal_subnet_ids) != set(existing_subnet_ids):
+                # this interface isn't present on the existing router
+                if obsolete:
+                    return True
+        else:
+            # no external fixed ips requested
+            if router['external_gateway_info'] \
+               and router['external_gateway_info']['external_fixed_ips'] \
+               and len(router['external_gateway_info']['external_fixed_ips']) > 1:
+                # but router has several external fixed ips
+                return True
+
+        # check if internal port has to be added
+        if router_ifs_cfg['internal_ports_missing']:
+            return True
+
+        if missing_port_ids:
+            return True
+
+        # check if internal subnet has to be added or removed
+        if set(requested_subnet_ids) != set(existing_subnet_ids):
             return True
 
         return False
 
-    def _system_state_change(self, router, network, internal_ids, internal_portids, filters=None):
+    def _build_kwargs(self, router, net):
-        """Check if the system state would be changed."""
-        state = self.params['state']
-        if state == 'absent' and router:
-            return True
-        if state == 'present':
-            if not router:
-                return True
-            return self._needs_update(router, network, internal_ids, internal_portids, filters)
-        return False
-
-    def _build_kwargs(self, router, network):
         kwargs = {
             'admin_state_up': self.params['admin_state_up'],
         }
@@ -318,8 +323,8 @@ class RouterModule(OpenStackModule):
         else:
             kwargs['name'] = self.params['name']
 
-        if network:
+        if net:
-            kwargs['ext_gateway_net_id'] = network['id']
+            kwargs['ext_gateway_net_id'] = net['id']
             # can't send enable_snat unless we have a network
             if self.params.get('enable_snat') is not None:
                 kwargs['enable_snat'] = self.params['enable_snat']
@@ -332,56 +337,83 @@ class RouterModule(OpenStackModule):
                 if 'ip' in iface:
                     d['ip_address'] = iface['ip']
                 kwargs['ext_fixed_ips'].append(d)
+        else:
+            # no external fixed ips requested
+            if router \
+               and router['external_gateway_info'] \
+               and router['external_gateway_info']['external_fixed_ips'] \
+               and len(router['external_gateway_info']['external_fixed_ips']) > 1:
+                # but router has several external fixed ips
+                # keep first external fixed ip only
+                fip = router['external_gateway_info']['external_fixed_ips'][0]
+                kwargs['ext_fixed_ips'] = [fip]
 
         return kwargs
 
-    def _validate_subnets(self, filters=None):
+    def _build_router_interface_config(self, filters=None):
-        external_subnet_ids = []
+        external_fixed_ips = []
-        internal_subnet_ids = []
+        internal_subnets = []
-        internal_port_ids = []
+        internal_ports = []
-        existing_port_ips = []
+        internal_ports_missing = []
+
+        # Build external interface configuration
         if self.params['external_fixed_ips']:
             for iface in self.params['external_fixed_ips']:
-                subnet = self.conn.get_subnet(iface['subnet'])
+                subnet = self.conn.get_subnet(iface['subnet'], filters)
                 if not subnet:
-                    self.fail_json(msg='subnet %s not found' % iface['subnet'])
+                    self.fail(msg='subnet %s not found' % iface['subnet'])
-                external_subnet_ids.append(subnet['id'])
+                new_external_fixed_ip = {'subnet_name': subnet.name, 'subnet_id': subnet.id}
+                if 'ip' in iface:
+                    new_external_fixed_ip['ip'] = iface['ip']
+                external_fixed_ips.append(new_external_fixed_ip)
 
+        # Build internal interface configuration
         if self.params['interfaces']:
+            internal_ips = []
             for iface in self.params['interfaces']:
                 if isinstance(iface, str):
                     subnet = self.conn.get_subnet(iface, filters)
                     if not subnet:
                         self.fail(msg='subnet %s not found' % iface)
-                    internal_subnet_ids.append(subnet['id'])
+                    internal_subnets.append(subnet)
+
                 elif isinstance(iface, dict):
                     subnet = self.conn.get_subnet(iface['subnet'], filters)
                     if not subnet:
                         self.fail(msg='subnet %s not found' % iface['subnet'])
+
                     net = self.conn.get_network(iface['net'])
                     if not net:
                         self.fail(msg='net %s not found' % iface['net'])
+
                     if "portip" not in iface:
-                        internal_subnet_ids.append(subnet['id'])
+                        # portip not set, add any ip from subnet
+                        internal_subnets.append(subnet)
                     elif not iface['portip']:
+                        # portip is set but has invalid value
                         self.fail(msg='put an ip in portip or remove it from list to assign default port to router')
                     else:
+                        # portip has valid value
+                        # look for ports whose fixed_ips.ip_address matchs portip
                         for existing_port in self.conn.list_ports(filters={'network_id': net.id}):
                             for fixed_ip in existing_port['fixed_ips']:
                                 if iface['portip'] == fixed_ip['ip_address']:
-                                    internal_port_ids.append(existing_port.id)
+                                    # portip exists in net already
-                                    existing_port_ips.append(fixed_ip['ip_address'])
+                                    internal_ports.append(existing_port)
-                        if iface['portip'] not in existing_port_ips:
+                                    internal_ips.append(fixed_ip['ip_address'])
-                            p = self.conn.create_port(network_id=net.id, fixed_ips=[
+                        if iface['portip'] not in internal_ips:
-                                {
+                            # no port with portip exists hence create a new port
-                                    'ip_address': iface['portip'],
+                            internal_ports_missing.append({
-                                    'subnet_id': subnet.id
+                                'network_id': net.id,
-                                }
+                                'fixed_ips': [{'ip_address': iface['portip'], 'subnet_id': subnet.id}]
-                            ])
+                            })
-                            if p:
-                                internal_port_ids.append(p.id)
 
-        return external_subnet_ids, internal_subnet_ids, internal_port_ids
+        return {
+            'external_fixed_ips': external_fixed_ips,
+            'internal_subnets': internal_subnets,
+            'internal_ports': internal_ports,
+            'internal_ports_missing': internal_ports_missing
+        }
 
     def run(self):
 
@@ -391,7 +423,7 @@ class RouterModule(OpenStackModule):
         project = self.params['project']
 
         if self.params['external_fixed_ips'] and not network:
-            self.fail_json(msg='network is required when supplying external_fixed_ips')
+            self.fail(msg='network is required when supplying external_fixed_ips')
 
         if project is not None:
             proj = self.conn.get_project(project)
@@ -412,67 +444,125 @@ class RouterModule(OpenStackModule):
 
         # Validate and cache the subnet IDs so we can avoid duplicate checks
         # and expensive API calls.
-        external_ids, subnet_internal_ids, internal_portids = self._validate_subnets(filters)
+        router_ifs_cfg = self._build_router_interface_config(filters)
+        requested_subnet_ids = [subnet.id for subnet in router_ifs_cfg['internal_subnets']] + \
+            self._get_subnet_ids_from_ports(router_ifs_cfg['internal_ports'])
+        requested_port_ids = [i['id'] for i in router_ifs_cfg['internal_ports']]
+
+        if router:
+            router_ifs_internal = self.conn.list_router_interfaces(router, 'internal')
+            existing_subnet_ids = self._get_subnet_ids_from_ports(router_ifs_internal)
+            obsolete_subnet_ids = set(existing_subnet_ids) - set(requested_subnet_ids)
+            existing_port_ids = [i['id'] for i in router_ifs_internal]
+
+        else:
+            router_ifs_internal = []
+            existing_subnet_ids = []
+            obsolete_subnet_ids = []
+            existing_port_ids = []
+
+        missing_port_ids = set(requested_port_ids) - set(existing_port_ids)
+
         if self.ansible.check_mode:
-            self.exit_json(
+            # Check if the system state would be changed
-                changed=self._system_state_change(router, net, subnet_internal_ids, internal_portids, filters)
+            if state == 'absent' and router:
-            )
+                changed = True
+            elif state == 'absent' and not router:
+                changed = False
+            elif state == 'present' and not router:
+                changed = True
+            else:  # if state == 'present' and router
+                changed = self._needs_update(router, net,
+                                             missing_port_ids,
+                                             requested_subnet_ids,
+                                             existing_subnet_ids,
+                                             router_ifs_cfg,
+                                             filters)
+            self.exit_json(changed=changed)
 
         if state == 'present':
             changed = False
 
             if not router:
+                changed = True
+
                 kwargs = self._build_kwargs(router, net)
                 if project_id:
                     kwargs['project_id'] = project_id
                 router = self.conn.create_router(**kwargs)
-                for int_s_id in subnet_internal_ids:
+
-                    self.conn.add_router_interface(router, subnet_id=int_s_id)
+                # add interface by subnet id, because user did not specify a port id
-                # add interface by port id as well
+                for subnet in router_ifs_cfg['internal_subnets']:
-                for int_p_id in internal_portids:
+                    self.conn.add_router_interface(router, subnet_id=subnet.id)
-                    self.conn.add_router_interface(router, port_id=int_p_id)
+
-                changed = True
+                # add interface by port id if user did specify a valid port id
+                for port in router_ifs_cfg['internal_ports']:
+                    self.conn.add_router_interface(router, port_id=port.id)
+
+                # add port and interface if user did specify an ip address but port is missing yet
+                for missing_internal_port in router_ifs_cfg['internal_ports_missing']:
+                    p = self.conn.create_port(**missing_internal_port)
+                    if p:
+                        self.conn.add_router_interface(router, port_id=p.id)
+
             else:
-                if self._needs_update(router, net, subnet_internal_ids, internal_portids, filters):
+                if self._needs_update(router, net,
+                                      missing_port_ids,
+                                      requested_subnet_ids,
+                                      existing_subnet_ids,
+                                      router_ifs_cfg,
+                                      filters):
+                    changed = True
                     kwargs = self._build_kwargs(router, net)
                     updated_router = self.conn.update_router(**kwargs)
 
-                    # Protect against update_router() not actually
+                    # Protect against update_router() not actually updating the router.
-                    # updating the router.
                     if not updated_router:
                         changed = False
-
+                    else:
-                    # On a router update, if any internal interfaces were supplied,
-                    # just detach all existing internal interfaces and attach the new.
-                    if internal_portids or subnet_internal_ids:
                         router = updated_router
-                        ports = self._router_internal_interfaces(router)
+
-                        for port in ports:
+                    # delete internal subnets i.e. ports
+                    if obsolete_subnet_ids:
+                        for port in router_ifs_internal:
+                            if 'fixed_ips' in port:
+                                for fip in port['fixed_ips']:
+                                    if fip['subnet_id'] in obsolete_subnet_ids:
                                         self.conn.remove_router_interface(router, port_id=port['id'])
-                    if internal_portids:
-                        external_ids, subnet_internal_ids, internal_portids = self._validate_subnets(filters)
-                        for int_p_id in internal_portids:
-                            self.conn.add_router_interface(router, port_id=int_p_id)
-                        changed = True
-                    if subnet_internal_ids:
-                        for s_id in subnet_internal_ids:
-                            self.conn.add_router_interface(router, subnet_id=s_id)
                                         changed = True
 
-            self.exit(changed=changed, router=router, id=router['id'])
+                    # add new internal interface by subnet id, because user did not specify a port id
+                    for subnet in router_ifs_cfg['internal_subnets']:
+                        if subnet.id not in existing_subnet_ids:
+                            self.conn.add_router_interface(router, subnet_id=subnet.id)
+                            changed = True
+
+                    # add new internal interface by port id if user did specify a valid port id
+                    for port_id in missing_port_ids:
+                        self.conn.add_router_interface(router, port_id=port_id)
+                        changed = True
+
+                    # add new port and new internal interface if user did specify an ip address but port is missing yet
+                    for missing_internal_port in router_ifs_cfg['internal_ports_missing']:
+                        p = self.conn.create_port(**missing_internal_port)
+                        if p:
+                            self.conn.add_router_interface(router, port_id=p.id)
+                            changed = True
+
+            self.exit_json(changed=changed, router=router)
 
         elif state == 'absent':
             if not router:
-                self.exit(changed=False)
+                self.exit_json(changed=False)
             else:
-                # We need to detach all internal interfaces on a router before
+                # We need to detach all internal interfaces on a router
-                # we will be allowed to delete it.
+                # before we will be allowed to delete it. Deletion can
-                ports = self._router_internal_interfaces(router)
+                # still fail if e.g. floating ips are attached to the
-                router_id = router['id']
+                # router.
-                for port in ports:
+                for port in router_ifs_internal:
                     self.conn.remove_router_interface(router, port_id=port['id'])
-                self.conn.delete_router(router_id)
+                self.conn.delete_router(router['id'])
-                self.exit_json(changed=True)
+                self.exit_json(changed=True, router=router)
 
 
 def main():