Updates security group rule for latest sdk

- Update docs
- Change calls from cloud to proxy layer
- Make sure return value is a dict
- Improve test coverage

Change-Id: I857d7ba7b7ca1b23100ee7e85e90e98430d68462

ci/roles/security_group/defaults/main.yml

@@ -1 +1,20 @@
+expected_fields:
+  - created_at
+  - description
+  - direction
+  - ether_type
+  - id
+  - name
+  - port_range_max
+  - port_range_min
+  - project_id
+  - protocol
+  - remote_address_group_id
+  - remote_group_id
+  - remote_ip_prefix
+  - revision_number
+  - security_group_id
+  - tags
+  - tenant_id
+  - updated_at
 secgroup_name: shade_secgroup

ci/roles/security_group/tasks/main.yml

@@ -1,4 +1,11 @@
 ---
+- name: Ensure security group does not exist before tests
+  openstack.cloud.security_group:
+     cloud: "{{ cloud }}"
+     name: "{{ secgroup_name }}"
+     state: absent
+     description: Created from Ansible playbook
+
 - name: Create security group
   openstack.cloud.security_group:
      cloud: "{{ cloud }}"
@@ -13,6 +20,29 @@
      state: present
      protocol: icmp
      remote_ip_prefix: 0.0.0.0/0
+  register: rule
+
+- name: Assert return fields
+  assert:
+     that: item in rule.rule
+  loop: "{{ expected_fields }}"
+
+- name: Assert changed
+  assert:
+     that: rule is changed
+
+- name: Create empty ICMP rule again
+  openstack.cloud.security_group_rule:
+     cloud: "{{ cloud }}"
+     security_group: "{{ secgroup_name }}"
+     state: present
+     protocol: icmp
+     remote_ip_prefix: 0.0.0.0/0
+  register: rule
+
+- name: Assert not changed
+  assert:
+     that: rule is not changed
 
 - name: Create -1 ICMP rule
   openstack.cloud.security_group_rule:
@@ -23,6 +53,26 @@
      port_range_min: -1
      port_range_max: -1
      remote_ip_prefix: 0.0.0.0/0
+  register: rule
+
+- name: Assert not changed
+  assert:
+     that: rule is not changed
+
+- name: Create -1 ICMP rule again
+  openstack.cloud.security_group_rule:
+     cloud: "{{ cloud }}"
+     security_group: "{{ secgroup_name }}"
+     state: present
+     protocol: icmp
+     port_range_min: -1
+     port_range_max: -1
+     remote_ip_prefix: 0.0.0.0/0
+  register: rule
+
+- name: Assert not changed
+  assert:
+     that: rule is not changed
 
 - name: Create empty TCP rule
   openstack.cloud.security_group_rule:
@@ -31,6 +81,11 @@
      state: present
      protocol: tcp
      remote_ip_prefix: 0.0.0.0/0
+  register: rule
+
+- name: Assert changed
+  assert:
+     that: rule is changed
 
 - name: Create TCP rule again with port range (1, 65535)
   openstack.cloud.security_group_rule:
@@ -41,6 +96,11 @@
      port_range_min: 1
      port_range_max: 65535
      remote_ip_prefix: 0.0.0.0/0
+  register: rule
+
+- name: Assert changed
+  assert:
+     that: rule is not changed
 
 - name: Create TCP rule again with port range (-1, -1)
   openstack.cloud.security_group_rule:
@@ -51,6 +111,26 @@
      port_range_min: -1
      port_range_max: -1
      remote_ip_prefix: 0.0.0.0/0
+  register: rule
+
+- name: Assert changed
+  assert:
+     that: rule is not changed
+
+- name: Create TCP rule again with defined range
+  openstack.cloud.security_group_rule:
+     cloud: "{{ cloud }}"
+     security_group: "{{ secgroup_name }}"
+     state: present
+     protocol: tcp
+     port_range_min: 8000
+     port_range_max: 9000
+     remote_ip_prefix: 0.0.0.0/0
+  register: rule
+
+- name: Assert changed
+  assert:
+     that: rule is changed
 
 - name: Create empty UDP rule
   openstack.cloud.security_group_rule:

plugins/modules/security_group_rule.py

@@ -38,13 +38,14 @@ options:
         - Name or ID of the Security group to link (exclusive with
           remote_ip_prefix)
       type: str
-   ethertype:
+   ether_type:
       description:
         - Must be IPv4 or IPv6, and addresses represented in CIDR must
           match the ingress or egress rules. Not all providers support IPv6.
       choices: ['IPv4', 'IPv6']
       default: IPv4
       type: str
+      aliases: [ethertype]
    direction:
       description:
         - The direction in which the security group rule is applied. Not
@@ -145,42 +146,98 @@ id:
   description: Unique rule UUID.
   type: str
   returned: state == present
-direction:
+rule:
-  description: The direction in which the security group rule is applied.
+  description: Representation of the security group rule
-  type: str
+  type: dict
-  sample: 'egress'
+  returned: when I(state) is present
-  returned: state == present
+  contains:
-ethertype:
+    created_at:
-  description: One of IPv4 or IPv6.
+      description: Timestamp when the resource was created
-  type: str
+      type: str
-  sample: 'IPv4'
+      returned: always
-  returned: state == present
+    description:
-port_range_min:
+      description: Description of the resource
-  description: The minimum port number in the range that is matched by
+      type: str
-               the security group rule.
+      returned: always
-  type: int
+    direction:
-  sample: 8000
+      description: The direction in which the security group rule is applied.
-  returned: state == present
+      type: str
-port_range_max:
+      sample: 'egress'
-  description: The maximum port number in the range that is matched by
+      returned: always
-               the security group rule.
+    ether_type:
-  type: int
+      description: Either IPv4 or IPv6
-  sample: 8000
+      type: str
-  returned: state == present
+      returned: always
-protocol:
+    id:
-  description: The protocol that is matched by the security group rule.
+      description: Unique rule UUID.
-  type: str
+      type: str
-  sample: 'tcp'
+      returned: always
-  returned: state == present
+    name:
-remote_ip_prefix:
+      description: Name of the resource.
-  description: The remote IP prefix to be associated with this security group rule.
+      type: str
-  type: str
+      returned: always
-  sample: '0.0.0.0/0'
+    port_range_max:
-  returned: state == present
+      description: The maximum port number in the range that is matched by
-security_group_id:
+                   the security group rule.
-  description: The security group ID to associate with this security group rule.
+      type: int
-  type: str
+      sample: 8000
-  returned: state == present
+      returned: always
+    port_range_min:
+      description: The minimum port number in the range that is matched by
+                   the security group rule.
+      type: int
+      sample: 8000
+      returned: always
+    project_id:
+      description: ID of the project the resource belongs to.
+      type: str
+      returned: always
+    protocol:
+      description: The protocol that is matched by the security group rule.
+      type: str
+      sample: 'tcp'
+      returned: always
+    remote_address_group_id:
+      description: The remote address group ID to be associated with this
+                   security group rule.
+      type: str
+      sample: '0.0.0.0/0'
+      returned: always
+    remote_group_id:
+      description: The remote security group ID to be associated with this
+                   security group rule.
+      type: str
+      sample: '0.0.0.0/0'
+      returned: always
+    remote_ip_prefix:
+      description: The remote IP prefix to be associated with this security
+                   group rule.
+      type: str
+      sample: '0.0.0.0/0'
+      returned: always
+    revision_number:
+      description: Revision number
+      type: int
+      sample: 0
+      returned: always
+    security_group_id:
+      description: The security group ID to associate with this security group
+                   rule.
+      type: str
+      returned: always
+    tags:
+      description: Tags associated with resource.
+      type: list
+      elements: str
+      returned: always
+    tenant_id:
+      description: ID of the project the resource belongs to. Deprecated.
+      type: str
+      returned: always
+    updated_at:
+      description: Timestamp when the security group rule was last updated.
+      type: str
+      returned: always
 '''
 
 from ansible_collections.openstack.cloud.plugins.module_utils.openstack import (
@@ -255,8 +312,9 @@ class SecurityGroupRuleModule(OpenStackModule):
         port_range_max=dict(required=False, type='int'),
         remote_ip_prefix=dict(required=False),
         remote_group=dict(required=False),
-        ethertype=dict(default='IPv4',
+        ether_type=dict(default='IPv4',
-                       choices=['IPv4', 'IPv6']),
+                        choices=['IPv4', 'IPv6'],
+                        aliases=['ethertype']),
         direction=dict(default='ingress',
                        choices=['egress', 'ingress']),
         state=dict(default='present',
@@ -271,111 +329,112 @@ class SecurityGroupRuleModule(OpenStackModule):
         ]
     )
 
-    def _find_matching_rule(self, secgroup, remotegroup):
+    def _build_kwargs(self, secgroup, remote_group, project):
+        kwargs = dict(
+            security_group_id=secgroup.id,
+            description=self.params['description'],
+            port_range_max=self.params['port_range_max'],
+            port_range_min=self.params['port_range_min'],
+            protocol=self.params['protocol'],
+            remote_ip_prefix=self.params['remote_ip_prefix'],
+            direction=self.params['direction'],
+            ether_type=self.params['ether_type'],
+        )
+        if self.params['port_range_min'] != -1:
+            kwargs['port_range_min'] = self.params['port_range_min']
+        if self.params['port_range_max'] != -1:
+            kwargs['port_range_max'] = self.params['port_range_max']
+        if project:
+            kwargs['project_id'] = project.id
+        if remote_group:
+            kwargs['remote_group_id'] = remote_group.id
+        return {k: v for k, v in kwargs.items() if v is not None}
+
+    def _find_matching_rule(self, kwargs, secgroup):
         """
         Find a rule in the group that matches the module parameters.
         :returns: The matching rule dict, or None if no matches.
         """
-        protocol = self.params['protocol']
+        fields = ('protocol', 'remote_ip_prefix', 'direction',
-        remote_ip_prefix = self.params['remote_ip_prefix']
+                  'remote_group_id')
-        ethertype = self.params['ethertype']
-        direction = self.params['direction']
-        remote_group_id = remotegroup['id']
-
         for rule in secgroup['security_group_rules']:
-            if (
+            if ('ether_type' in kwargs
-                protocol == rule['protocol']
+               and rule['ethertype'] != kwargs['ether_type']):
-                and remote_ip_prefix == rule['remote_ip_prefix']
+                continue
-                and ethertype == rule['ethertype']
+            if any(field in kwargs and rule[field] != kwargs[field]
-                and direction == rule['direction']
+                   for field in fields):
-                and remote_group_id == rule['remote_group_id']
+                continue
-                and _ports_match(
+            if _ports_match(
-                    protocol,
+                    self.params['protocol'],
                     self.params['port_range_min'],
                     self.params['port_range_max'],
                     rule['port_range_min'],
-                    rule['port_range_max'])
+                    rule['port_range_max']
             ):
                 return rule
         return None
 
-    def _system_state_change(self, secgroup, remotegroup):
+    def _system_state_change(self, secgroup, rule):
         state = self.params['state']
-        if secgroup:
+        if not secgroup:
-            rule_exists = self._find_matching_rule(secgroup, remotegroup)
-        else:
             return False
 
-        if state == 'present' and not rule_exists:
+        if state == 'present' and not rule:
             return True
-        if state == 'absent' and rule_exists:
+        if state == 'absent' and rule:
             return True
         return False
 
     def run(self):
-
         state = self.params['state']
         security_group = self.params['security_group']
-        remote_group = self.params['remote_group']
+        remote_group_name_or_id = self.params['remote_group']
-        project = self.params['project']
+        project_name_or_id = self.params['project']
-        changed = False
 
-        if project is not None:
+        project = None
-            proj = self.conn.get_project(project)
+        if project_name_or_id:
-            if proj is None:
+            project = self.conn.identity.find_project(project_name_or_id,
-                self.fail_json(msg='Project %s could not be found' % project)
+                                                      ignore_missing=False)
-            project_id = proj['id']
-        else:
-            project_id = self.conn.current_project_id
 
-        if project_id and not remote_group:
+        filters = {}
-            filters = {'tenant_id': project_id}
+        if project and not remote_group_name_or_id:
-        else:
+            filters = {'project_id': project.id}
-            filters = None
 
-        secgroup = self.conn.get_security_group(security_group, filters=filters)
+        secgroup = self.conn.network.find_security_group(
+            security_group, ignore_missing=(state == 'absent'), **filters)
 
-        if remote_group:
+        remote_group = None
-            remotegroup = self.conn.get_security_group(remote_group, filters=filters)
+        if remote_group_name_or_id:
-        else:
+            remote_group = self.conn.network.find_security_group(
-            remotegroup = {'id': None}
+                remote_group_name_or_id, ignore_missing=False, filters=filters)
+
+        kwargs = self._build_kwargs(secgroup, remote_group, project)
+
+        rule = None
+        if secgroup:
+            # TODO: Replace with self.conn.network.find_security_group_rule()?
+            rule = self._find_matching_rule(kwargs, secgroup)
+            if rule:
+                rule = self.conn.network.get_security_group_rule(rule['id'])
 
         if self.ansible.check_mode:
-            self.exit_json(changed=self._system_state_change(secgroup, remotegroup))
+            self.exit_json(changed=self._system_state_change(secgroup, rule))
 
+        changed = False
         if state == 'present':
             if self.params['protocol'] == 'any':
                 self.params['protocol'] = None
 
-            if not secgroup:
-                self.fail_json(msg='Could not find security group %s' % security_group)
-
-            rule = self._find_matching_rule(secgroup, remotegroup)
             if not rule:
-                kwargs = {}
+                rule = self.conn.network.create_security_group_rule(**kwargs)
-                if project_id:
-                    kwargs['project_id'] = project_id
-                if self.params["description"] is not None:
-                    kwargs["description"] = self.params['description']
-                rule = self.conn.network.create_security_group_rule(
-                    security_group_id=secgroup['id'],
-                    port_range_min=None if self.params['port_range_min'] == -1 else self.params['port_range_min'],
-                    port_range_max=None if self.params['port_range_max'] == -1 else self.params['port_range_max'],
-                    protocol=self.params['protocol'],
-                    remote_ip_prefix=self.params['remote_ip_prefix'],
-                    remote_group_id=remotegroup['id'],
-                    direction=self.params['direction'],
-                    ethertype=self.params['ethertype'],
-                    **kwargs
-                )
                 changed = True
+
+            rule = rule.to_dict(computed=False)
             self.exit_json(changed=changed, rule=rule, id=rule['id'])
 
-        if state == 'absent' and secgroup:
+        if state == 'absent' and rule:
-            rule = self._find_matching_rule(secgroup, remotegroup)
+            self.conn.network.delete_security_group_rule(rule['id'])
-            if rule:
+            changed = True
-                self.conn.delete_security_group_rule(rule['id'])
-                changed = True
 
         self.exit_json(changed=changed)