Bug Fixes for ontap_net_routes.py (#44208)

lib/ansible/modules/storage/netapp/na_ontap_net_routes.py

@@ -13,36 +13,37 @@ ANSIBLE_METADATA = {'metadata_version': '1.1',
 
 DOCUMENTATION = '''
 module: na_ontap_net_routes
-short_description: Manage NetApp Ontap network routes
+short_description: Manage NetApp ONTAP network routes
 extends_documentation_fragment:
     - netapp.na_ontap
 version_added: '2.6'
-author:
+author: NetApp Ansible Team (ng-ansibleteam@netapp.com)
-- Chris Archibald (carchi@netapp.com), Kevin Hutton (khutton@netapp.com)
 description:
-- Modify a Ontap network routes
+- Modify ONTAP network routes.
 options:
   state:
     description:
-    - Whether you want to create or delete a network route
+    - Whether you want to create or delete a network route.
     choices: ['present', 'absent']
     default: present
   vserver:
     description:
-    - The name of the vserver
+    - The name of the vserver.
     required: true
   destination:
     description:
-    - Specify the route destination. Example 10.7.125.5/20, fd20:13::/64
+    - Specify the route destination.
+    - Example 10.7.125.5/20, fd20:13::/64.
     required: true
   gateway:
     description:
-    - Specify the route gateway. Example 10.7.125.1, fd20:13::1
+    - Specify the route gateway.
+    - Example 10.7.125.1, fd20:13::1.
     required: true
   metric:
     description:
     - Specify the route metric.
-    - If this field is not provided the default will be set to 30
+    - If this field is not provided the default will be set to 20.
   new_destination:
     description:
     - Specify the new route destination.
@@ -76,18 +77,19 @@ import traceback
 from ansible.module_utils.basic import AnsibleModule
 from ansible.module_utils._text import to_native
 import ansible.module_utils.netapp as netapp_utils
+from ansible.module_utils.netapp_module import NetAppModule
 
 HAS_NETAPP_LIB = netapp_utils.has_netapp_lib()
 
 
 class NetAppOntapNetRoutes(object):
     """
-        Create, Modifies and Destroys a Net Route
+    Create, Modifies and Destroys a Net Route
     """
 
     def __init__(self):
         """
-            Initialize the Ontap Net Route class
+        Initialize the Ontap Net Route class
         """
         self.argument_spec = netapp_utils.na_ontap_host_argument_spec()
         self.argument_spec.update(dict(
@@ -96,7 +98,7 @@ class NetAppOntapNetRoutes(object):
             vserver=dict(required=True, type='str'),
             destination=dict(required=True, type='str'),
             gateway=dict(required=True, type='str'),
-            metric=dict(required=False, type='str', default=None),
+            metric=dict(required=False, type='str'),
             new_destination=dict(required=False, type='str', default=None),
             new_gateway=dict(required=False, type='str', default=None),
             new_metric=dict(required=False, type='str', default=None),
@@ -107,35 +109,27 @@ class NetAppOntapNetRoutes(object):
             supports_check_mode=True
         )
 
-        parameters = self.module.params
+        self.na_helper = NetAppModule()
-
+        self.parameters = self.na_helper.set_parameters(self.module.params)
-        # set up state variables
-        self.state = parameters['state']
-        self.vserver = parameters['vserver']
-        self.destination = parameters['destination']
-        self.gateway = parameters['gateway']
-        self.metric = parameters['metric']
-        self.new_destination = parameters['new_destination']
-        self.new_gateway = parameters['new_destination']
-        self.new_metric = parameters['new_metric']
 
         if HAS_NETAPP_LIB is False:
-            self.module.fail_json(
+            self.module.fail_json(msg="the python NetApp-Lib module is required")
-                msg="the python NetApp-Lib module is required")
         else:
-            self.server = netapp_utils.setup_na_ontap_zapi(
+            self.server = netapp_utils.setup_na_ontap_zapi(module=self.module, vserver=self.parameters['vserver'])
-                module=self.module, vserver=self.vserver)
         return
 
-    def create_net_route(self):
+    def create_net_route(self, current_metric=None):
         """
         Creates a new Route
         """
         route_obj = netapp_utils.zapi.NaElement('net-routes-create')
-        route_obj.add_new_child("destination", self.destination)
+        route_obj.add_new_child("destination", self.parameters['destination'])
-        route_obj.add_new_child("gateway", self.gateway)
+        route_obj.add_new_child("gateway", self.parameters['gateway'])
-        if self.metric:
+        if current_metric is None and self.parameters.get('metric') is not None:
-            route_obj.add_new_child("metric", self.metric)
+            metric = self.parameters['metric']
+        else:
+            metric = current_metric
+        route_obj.add_new_child("metric", metric)
         try:
             self.server.invoke_successfully(route_obj, True)
         except netapp_utils.zapi.NaApiError as error:
@@ -148,8 +142,8 @@ class NetAppOntapNetRoutes(object):
         Deletes a given Route
         """
         route_obj = netapp_utils.zapi.NaElement('net-routes-destroy')
-        route_obj.add_new_child("destination", self.destination)
+        route_obj.add_new_child("destination", self.parameters['destination'])
-        route_obj.add_new_child("gateway", self.gateway)
+        route_obj.add_new_child("gateway", self.parameters['gateway'])
         try:
             self.server.invoke_successfully(route_obj, True)
         except netapp_utils.zapi.NaApiError as error:
@@ -157,80 +151,141 @@ class NetAppOntapNetRoutes(object):
                                   % (to_native(error)),
                                   exception=traceback.format_exc())
 
-    def modify_net_route(self):
+    def modify_net_route(self, current, desired):
         """
         Modify a net route
         """
+        # return if there is nothing to change
+        for key, val in desired.items():
+            if val == current[key]:
+                self.na_helper.changed = False
+                return
+        # delete and re-create with new params
         self.delete_net_route()
         route_obj = netapp_utils.zapi.NaElement('net-routes-create')
-        if self.new_destination:
+        for attribute in ['metric', 'destination', 'gateway']:
-            route_obj.add_new_child("destination", self.new_destination)
+            if desired.get(attribute) is not None:
-        else:
+                value = desired[attribute]
-            route_obj.add_new_child("destination", self.destination)
+            else:
-        if self.new_gateway:
+                value = current[attribute]
-            route_obj.add_new_child("gateway", self.new_gateway)
+            route_obj.add_new_child(attribute, value)
-        else:
-            route_obj.add_new_child("gateway", self.gateway)
-        if self.new_metric:
-            route_obj.add_new_child("metric", self.new_metric)
-        else:
-            route_obj.add_new_child("metric", self.metric)
-        # if anything goes wrong creating the new net route,
-        # restore the old one
         try:
-            self.server.invoke_successfully(route_obj, True)
+            result = self.server.invoke_successfully(route_obj, True)
-        except netapp_utils.zapi.NaApiError:
+        except netapp_utils.zapi.NaApiError as error:
-            self.create_net_route()
+            # restore the old route, create the route with the existing metric
+            self.create_net_route(current['metric'])
+            # return if desired route already exists
+            if to_native(error.code) == '13001':
+                return
+            # Invalid value specified for any of the attributes
+            self.module.fail_json(msg='Error modifying net route: %s'
+                                  % (to_native(error)),
+                                  exception=traceback.format_exc())
 
-    def does_route_exists(self):
+    def get_net_route(self, params=None):
         """
         Checks to see if a route exist or not
-        :return: True if a route exists, False if it does not
+        :return: NaElement object if a route exists, None otherwise
         """
+        if params is not None:
+            # we need at least on of the new_destination or new_gateway to fetch desired route
+            if params.get('destination') is None and params.get('gateway') is None:
+                return None
+        current = dict()
         route_obj = netapp_utils.zapi.NaElement('net-routes-get')
-        route_obj.add_new_child("destination", self.destination)
+        for attr in ['destination', 'gateway']:
-        route_obj.add_new_child("gateway", self.gateway)
+            if params and params.get(attr) is not None:
+                value = params[attr]
+            else:
+                value = self.parameters[attr]
+            route_obj.add_new_child(attr, value)
         try:
-            self.server.invoke_successfully(route_obj, True)
+            result = self.server.invoke_successfully(route_obj, True)
-        except netapp_utils.zapi.NaApiError:
+            route_info = result.get_child_by_name('attributes').get_child_by_name('net-vs-routes-info')
+            current['destination'] = route_info.get_child_content('destination')
+            current['gateway'] = route_info.get_child_content('gateway')
+            current['metric'] = route_info.get_child_content('metric')
+        except netapp_utils.zapi.NaApiError as error:
+            # Error 13040 denotes a route doesn't exist.
+            if to_native(error.code) == "15661":
+                return None
+            self.module.fail_json(msg='Error fetching net route: %s'
+                                  % (to_native(error)),
+                                  exception=traceback.format_exc())
+        return current
+
+    def is_modify_action(self, current, desired):
+        """
+        Get desired action to be applied for net routes
+        Destination and gateway are unique params for a route and cannot be duplicated
+        So if a route with desired destination or gateway exists already, we don't try to modify
+        :param current: current details
+        :param desired: desired details
+        :return: create / delete / modify / None
+        """
+        if current is None and desired is None:
+            # this is invalid
+            # cannot modify a non existent resource
+            return None
+        if current is None and desired is not None:
+            # idempotency or duplication
+            # we need not create
+            return False
+        if current is not None and desired is not None:
+            # we can't modify an ambiguous route (idempotency/duplication)
             return False
         return True
 
+    def get_params_to_be_modified(self, current):
+        """
+        Get parameters and values that need to be modified
+        :param current: current details
+        :return: dict(), None
+        """
+        if current is None:
+            return None
+        desired = dict()
+        if self.parameters.get('new_destination') is not None and \
+                self.parameters['new_destination'] != current['destination']:
+            desired['destination'] = self.parameters['new_destination']
+        if self.parameters.get('new_gateway') is not None and \
+                self.parameters['new_gateway'] != current['gateway']:
+            desired['gateway'] = self.parameters['new_gateway']
+        if self.parameters.get('new_metric') is not None and \
+                self.parameters['new_metric'] != current['metric']:
+            desired['metric'] = self.parameters['new_metric']
+        return desired
+
     def apply(self):
         """
         Run Module based on play book
         """
-        changed = False
+        # changed = False
         netapp_utils.ems_log_event("na_ontap_net_routes", self.server)
-        route_exists = False
+        # route_exists = False
-        existing_route = self.does_route_exists()
+        current = self.get_net_route()
-
+        modify, cd_action = None, None
-        if existing_route:
+        modify_params = {'destination': self.parameters.get('new_destination'),
-            route_exists = True
+                         'gateway': self.parameters.get('new_gateway'),
-            if self.state == 'absent':
+                         'metric': self.parameters.get('new_metric')}
-                changed = True
+        # if any new_* param is present in playbook, check for modify action
-            elif self.state == 'present':
+        if any(modify_params.values()):
-                if self.new_destination or \
+            # get parameters that are eligible for modify
-                        self.new_gateway or self.new_metric:
+            d = self.get_net_route(modify_params)
-                    changed = True
+            modify = self.na_helper.is_rename_action(current, d)
+            if modify is None:
+                self.module.fail_json(msg="Error modifying: route %s does not exist" % self.parameters['destination'])
         else:
-            if self.state == 'present':
+            cd_action = self.na_helper.get_cd_action(current, self.parameters)
-                changed = True
 
-        if changed:
+        if cd_action == 'create':
-            if self.module.check_mode:
+            self.create_net_route()
-                pass
+        elif cd_action == 'delete':
-            else:
+            self.delete_net_route()
-                if not route_exists:
+        elif modify:
-                    if self.state == 'present':
+            self.modify_net_route(current, modify_params)
-                        self.create_net_route()
-                else:
-                    if self.state == 'present':
-                        self.modify_net_route()
-                    elif self.state == 'absent':
-                        self.delete_net_route()
 
-        self.module.exit_json(changed=changed)
+        self.module.exit_json(changed=self.na_helper.changed)
 
 
 def main():