ansible-collections / cisco.ios

Ansible Network Collection for Cisco IOS
GNU General Public License v3.0
262 stars 161 forks source link

L2_InterfacesFacts not gathering all allowed VLANs from trunk #77

Closed mihudec closed 3 years ago

mihudec commented 3 years ago
SUMMARY

The Facts module for L2 properties does not include allowed VLANs on trunk interfaces when there are multiple allowed ranges. The module only recognizes VLAN ranges in switchport trunk allowed vlan X, but not in switchport trunk allowed vlan add Y. This result in change every time.

ISSUE TYPE
COMPONENT NAME

ios_l2_interfaces L2_InterfacesFacts

ANSIBLE VERSION
ansible 2.10.0.dev0
  config file = /home/username/Develop/ansible.cfg
  configured module search path = ['/home/username/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /home/username/ansible/lib/ansible
  executable location = /home/username/ansible/bin/ansible
  python version = 3.8.3 (default, May 15 2020, 00:00:00) [GCC 10.1.1 20200507 (Red Hat 10.1.1-1)]
CONFIGURATION
DEFAULT_CALLBACK_WHITELIST(/home/username/Develop/ansible.cfg) = ['timer', 'profile_tasks']
DEFAULT_FORKS(/home/username/Develop/ansible.cfg) = 10
DEFAULT_GATHERING(/home/username/Develop/ansible.cfg) = explicit
DEFAULT_VAULT_PASSWORD_FILE(/home/username/Develop/ansible.cfg) = XXXXXXXX
HOST_KEY_CHECKING(/home/username/Develop/ansible.cfg) = False
INTERPRETER_PYTHON(/home/username/Develop/ansible.cfg) = auto_silent
OS / ENVIRONMENT

Fedora 32

Cisco: Any IOS/IOS-XE Switch

STEPS TO REPRODUCE

Playbook:

---
- name: Test Interfaces Trunk
  hosts: all
  collections:
    - cisco.ios

  vars:
    vlans: [1,2,4,6,8,10,12,14,16,18,20,22,24,26,28,30,32,1500,1502,1504]  

  tasks:
    - name: Configure Trunk Interfaces
      ios_l2_interfaces:
        config:
          - name: Ethernet0/2
            trunk:
              allowed_vlans: "{{ vlans | join(',') }}"
      tags: test
      register: output

Device config before running the playbook:

SW-1#show running-config interface Ethernet 0/2
Building configuration...

Current configuration : 156 bytes
!
interface Ethernet0/2
 switchport trunk allowed vlan 1,2,4,6,8,10,12,14,16,18,20,22,24,26,28,30,32
 switchport trunk allowed vlan add 1500,1502,1504
end
EXPECTED RESULTS

Module parses the current device config correctly, including the line switchport trunk allowed vlan add 1500,1502,1504. The module then compares the currently allowed VLANs with the VLANs specified in task and pushes only the difference, preferably using switchport trunk allowed vlan add <diff>. In this exact case, the VLANs configured on the device are identical to the ones specified in task, therefore the task should return ok instead of changed.

ACTUAL RESULTS

The module only recognizes the switchport trunk allowed vlan 1,2,4,6,8,10,12,14,16,18,20,22,24,26,28,30,32 and tries to update the allowed VLANs to contain 1500,1502,1504. However, it does it in a strange way, putting together already present VLANs with ALL specified VLANs, producing command switchport trunk allowed vlan 1,2,4,6,8,10,12,14,16,18,20,22,24,26,28,30,32,1500,1502,1504,1,2,4,6,8,10,12,14,16,18,20,22,24,26,28,30,32 . A proper way would be to get want.difference(have) and send switchport trunk allowed vlan add 1500,1502,1504. The difference should be done per individual VLANs, not per ranges.

PLAYBOOK: interfaces_trunk.yml ***************************************************************************************************************************************************************************************************************
1 plays in interfaces_trunk.yml

PLAY [Test Interfaces Trunk] *****************************************************************************************************************************************************************************************************************
META: ran handlers

TASK [Configure Trunk Interfaces] ************************************************************************************************************************************************************************************************************
changed: [SW-01] => {
    "after": [
        {
            "mode": "trunk",
            "name": "Ethernet0/0",
            "trunk": {
                "allowed_vlans": [
                    "10",
                    "20"
                ],
                "encapsulation": "dot1q"
            }
        },
        {
            "name": "Ethernet0/1"
        },
        {
            "name": "Ethernet0/2",
            "trunk": {
                "allowed_vlans": [
                    "1",
                    "2",
                    "4",
                    "6",
                    "8",
                    "10",
                    "12",
                    "14",
                    "16",
                    "18",
                    "20",
                    "22",
                    "24",
                    "26",
                    "28",
                    "30",
                    "32"
                ],
                "native_vlan": 11
            }
        },
        {
            "name": "Ethernet0/3"
        }
    ],
    "ansible_facts": {
        "discovered_interpreter_python": "/usr/bin/python3"
    },
    "before": [
        {
            "mode": "trunk",
            "name": "Ethernet0/0",
            "trunk": {
                "allowed_vlans": [
                    "10",
                    "20"
                ],
                "encapsulation": "dot1q"
            }
        },
        {
            "name": "Ethernet0/1"
        },
        {
            "name": "Ethernet0/2",
            "trunk": {
                "allowed_vlans": [
                    "1",
                    "2",
                    "4",
                    "6",
                    "8",
                    "10",
                    "12",
                    "14",
                    "16",
                    "18",
                    "20",
                    "22",
                    "24",
                    "26",
                    "28",
                    "30",
                    "32"
                ],
                "native_vlan": 11
            }
        },
        {
            "name": "Ethernet0/3"
        }
    ],
    "changed": true,
    "commands": [
        "interface Ethernet0/2",
        "switchport trunk allowed vlan 1,2,4,6,8,10,12,14,16,18,20,22,24,26,28,30,32,1500,1502,1504,1,2,4,6,8,10,12,14,16,18,20,22,24,26,28,30,32"
    ],
    "invocation": {
        "module_args": {
            "config": [
                {
                    "access": null,
                    "mode": null,
                    "name": "Ethernet0/2",
                    "trunk": {
                        "allowed_vlans": [
                            "1",
                            "2",
                            "4",
                            "6",
                            "8",
                            "10",
                            "12",
                            "14",
                            "16",
                            "18",
                            "20",
                            "22",
                            "24",
                            "26",
                            "28",
                            "30",
                            "32",
                            "1500",
                            "1502",
                            "1504"
                        ],
                        "encapsulation": null,
                        "native_vlan": null,
                        "pruning_vlans": null
                    },
                    "voice": null
                }
            ],
            "running_config": null,
            "state": "merged"
        }
    }
}

PLAY RECAP ***********************************************************************************************************************************************************************************************************************************
SW-01                      : ok=1    changed=1    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   

Suggestions

I suggest creating new function in utils for parsing allowed VLANs, maybe something like:

def get_trunk_allowed_vlans(conf):
    allowed_vlans = None
    for line in filter(lambda x: "switchport trunk allowed vlan" in x, conf.split("\n")):
        if allowed_vlans is None:
            allowed_vlans = []
        if "vlan all" in line: 
            return ["1-4094"]
        if "vlan none" in line:
            return []
        allowed_vlans.extend(parse_conf_arg(line, "allowed vlan(?: add)?").split(","))
    return allowed_vlans

It should be noted that the config switchport trunk allowed vlan all will never be present in show running-config only in "show running-config all". Maybe the facts module should return ["1-4094"] for all trunk interfaces with default configuration. Another possibility is switchport trunk allowed vlan none, which could make the module return an empty list. In my opinion, the allowed_vlan = None should be returned only for non-trunking interfaces.

Then another function for expanding the VLAN ranges to the list (or set) of integers:

def expand_vlan_range(vlans):
    expanded = set()
    for vlan_range in vlans:
        if isinstance(vlan_range, int):
            expanded.add(vlan_range)
        elif isinstance(vlan_range, str):
            vlan_range = vlan_range.split("-")
            if len(vlan_range) > 1:
                # Maybe add validation that stop > start
                start = int(vlan_range[0].strip())
                stop = int(vlan_range[1].strip())
                expanded.update(range(start, stop+1))
            else:
                expanded.add(int(vlan_range))
    return list(expanded)
justjais commented 3 years ago

@mihudec Thanks for raising the issue, can u share the out of sh running-config | section ^interface which is used to generate the out for parsing l2_interface params. The l2 device (ref version: cisco vios_l2 15.2) I have tried the scenario with doesn’t differentiate the output for allowed vlan/pruning vlan based on add param. I can add the logic for parsing the add param once confirmed.

mihudec commented 3 years ago

@justjais Sorry for the late response, the config from the interface is:

interface Ethernet0/2
 switchport mode trunk
 switchport trunk allowed vlan 1,2,4,6,8,10,12,14,16,18,20,22,24,26,28,30,32
 switchport trunk allowed vlan add 1500,1502,1504

To my knowledge, this happens on all IOS versions and platforms regardless the terminal width parameter, when the switchport trunk allowed vlan line becomes too long. You can spot this behaviour by adding more VLANs, where from certain point the lines start to fold using the add keyword. This can result in multiple add lines.

Thanks.

el00ruobuob commented 3 years ago

Hi all,

Facing the same issue today for a customer, i came up with a quick fix in module_utils/network/ios/facts/l2_interfaces/l2_interfaces.py:

   127              if allowed_vlan:
   128                  trunk["allowed_vlans"] = allowed_vlan.split(",")
   129              allowed_vlan_add = utils.parse_conf_arg(conf, "allowed vlan add")
   130              if allowed_vlan_add:
   131                  trunk["allowed_vlans"] += allowed_vlan_add.split(",")

Would you like a proper merge request?

mihudec commented 3 years ago

Hi @el00ruobuob , glad to see I'm not the only one facing this issue. I've done similar patch for myself, however, I believe this issue goes beyond this single module. The other part of the problem is the way VLAN ranges are compared in ios_l2_interfaces and the way new VLANs are added/removed from configuration. I'm afraid that simple pushing switchport trunk allowed vlan <all vlans go here> is not the best way to do it, as it may cause a temporary service disruptions on already allowed VLANs (the VLAN might flap when being re-added). I would prefer if the fix addressed this as well and used proper add and remove logic. And the same thing should be incorporated in NXOS modules. I've tried to come up with a PR, but frankly I couldn't tell where exactly it should be fixed. Maybe @justjais has some ideas?

Thanks.

el00ruobuob commented 3 years ago

as it may cause a temporary service disruptions on already allowed VLANs (the VLAN might flap when being re-added)

@justjais, Could you confirm or infirm such possibility of service disruption? For something like adding vlans to production trunk interfaces, that's quite a concern.

justjais commented 3 years ago

@el00ruobuob @mihudec apologies for the late reply as I was on PTO and I'll verify the issue again from my end and raise a PR for fixing the same asap.

el00ruobuob commented 3 years ago

@justjais Any update on this?

justjais commented 3 years ago

@el00ruobuob excuse me for the delay it's been a busy month, you can expect the update sometime next week.

phpipam commented 3 years ago

+1 , any update ?

justjais commented 3 years ago

@phpipam u can expect the fix this week, as supposedly the PR against the issue is not the intended fix.

phpipam commented 3 years ago

Hi @justjais , thanks, let me know if I can assist if some testing is needed.

justjais commented 3 years ago

@phpipam @mihudec I've raised the PR to fix the particular issue, plz verify the PR changes if it works as expected and also fixes the issue faced. Excuse me for the delayed fix, it had slipped under the crack.

phpipam commented 3 years ago

Hi @justjais, thanks.

I did some test and it seems it works only for first line of switchport trunk allowed vlans add, if there are more all following lines are ignored.

Interface cfg example:

interface GigabitEthernet1/0/1
 description UPLINK
 switchport trunk allowed vlan 11,59,67,75,77,81,100,400-408,411-413,415,418
 switchport trunk allowed vlan add 461,674,675,696,931,935,951,952,973,974,979
 switchport trunk allowed vlan add 982,986,988,993
 switchport mode trunk
...
end

As mentioned second add line is excluded:

switchport trunk allowed vlan add 982,986,988,993

This are facts gathered for interface:

  - mode: trunk
    name: GigabitEthernet1/0/1
    trunk:
      allowed_vlans:
      - '11'
      - '59'
      - '67'
      - '75'
      - '77'
      - '81'
      - '100'
      - '400'
      - '401'
      - '402'
      - '403'
      - '404'
      - '405'
      - '406'
      - '407'
      - '408'
      - '411'
      - '412'
      - '413'
      - '415'
      - '418'
      - '461'
      - '674'
      - '675'
      - '696'
      - '931'
      - '935'
      - '951'
      - '952'
      - '973'
      - '974'
      - '979'
justjais commented 3 years ago

@phpipam perfect, thanks for testing out the PR and letting me know of the issue with the fix. I'll update the fix and let you know.

justjais commented 3 years ago

@phpipam I've updated the fix, let me know if that works for you as expected. Thanks again for testing the fix.

phpipam commented 3 years ago

HI, seems to be ok 👍 , thanks !

  - mode: trunk
    name: GigabitEthernet1/0/1
    trunk:
      allowed_vlans:
      - '11'
      - '59'
      - '67'
      - '75'
      - '77'
      - '81'
      - '100'
      - '400'
      - '401'
      - '402'
      - '403'
      - '404'
      - '405'
      - '406'
      - '407'
      - '408'
      - '411'
      - '412'
      - '413'
      - '415'
      - '418'
      - '461'
      - '674'
      - '675'
      - '696'
      - '931'
      - '935'
      - '951'
      - '952'
      - '973'
      - '974'
      - '979'
      - '982'
      - '986'
      - '988'
      - '993'