Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[nxapi] Add vrf option to nxos_nxapi module #877

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Miyoshi-Ryota
Copy link
Contributor

@Miyoshi-Ryota Miyoshi-Ryota commented Aug 2, 2024

SUMMARY

Add vrf option to nxos_nxapi module.

Comments on the design:

It's an old module with only state: present / absent. As far as I can see, when state: present is specified, it does not change parameters that are not mentioned. For example, if the nxapi sandbox command is already present on the device but the sandbox parameter is not specified in the playbook, it results that no deletion or other actions are taken.

Therefore, the vrf option follows this design.

Not applying the nxapi use-vrf command at all means enabling nxapi for all VRFs. However, in my implementation of this module, there is currently no way to remove an already applied nxapi use-vrf configuration at all. This is because while it is possible to specify another VRF, but there is no way to reflect the absence of a parameter. This aligns with the aforementioned behavior of state: present where nothing is done if a parameter is not specified.

This implementation method was chosen to maintain consistency within the module, avoid disrupting environments using the existing nxos_nxapi module, and keep the parameters simple.

Other possible approaches:

  • Implementing a behavior similar to the state: overriden in other modules, meaning if no VRF is specified, the existing nxapi use-vrf setting would be deleted.
  • Making the parameter more complex. For example, explicitly enabling or disabling as follows:
vrf:
  limit_to_vrf: bool
  vrf_name: xxx

If you prefer to select one of other two approaches I mentioned, or an entirely different approach, please let me know. I am willing to make adjustments accordingly.

Reference for the supported versions of use-vrf:
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

nxos_nxapi_module

ADDITIONAL INFORMATION

Comments on the design:
This is an old module with only state: present / absent.
As far as I can see, when state: present is specified,
it does not change parameters that are not mentioned.
For example, if the nxapi sandbox command is already present on the device
but the sandbox parameter is not specified in the playbook,
it appears that no deletion or other actions are taken.

Therefore, the use-vrf follows this design.

Not applying the `nxapi use-vrf` command at all means enabling nxapi for all VRFs.
However, in my implementation of this module, there is currently no way
to remove an already applied nxapi use-vrf <xxxxx> configuration at all.
This is because while it is possible to specify another VRF,
but there is no way to reflect the absence of a parameter.
This aligns with the aforementioned behavior of state: `present`
where nothing is done if a parameter is not specified.

This implementation method was chosen to maintain consistency within the module, provide clarity,
avoid disrupting environments using the existing nxos_nxapi module, and keep the parameters simple.

Other possible approaches:
- Implementing a behavior similar to the state: overriden in other modules,
  meaning if no VRF is specified, the existing nxapi use-vrf setting would be deleted.
- Making the parameters more complex. For example, explicitly enabling or disabling as follows:
```
vrf:
  limit_to_vrf: bool
  vrf_name: xxx
```

Reference for the supported versions of use-vrf:
- For N9K:
  At least, supported in version 7.0(3)I3(1)
  https://www.cisco.com/c/en/us/td/docs/switches/datacenter/nexus9000/sw/7-x/command_references/configuration_commands/b_Using_N9K_Config_Commands/b_N9K_Bookmap_chapter_01111.html#wp4568333650
  Also, it is not supported in version 6.1(2)I2(2)
  https://www.cisco.com/c/en/us/td/docs/switches/datacenter/nexus9000/sw/6-x/command_reference/config_612I22/b_n9k_command_ref/b_n9k_command_ref_chapter_010000.html#wp2897910537
  I am unsure of the exact version where it was released. I have reviewed all the release notes, but there is no mention of it.
- For N7K:
  > The nxapi use-vrf feature is introduced in Cisco NX-OS Release 8.2(3), which helps to secure NX-API by binding the NX-API to a particular VRF.
  https://www.cisco.com/c/en/us/td/docs/switches/datacenter/nexus7000/sw/programmability/guide/cisco_nexus7000_programmability_guide_8x/b-cisco-nexus7000-programmability-guide-8x_chapter_011.html
- For N3K:
  At least, supported in version 7.0(3)I4(1)
  https://www.cisco.com/c/en/us/td/docs/switches/datacenter/nexus3000/sw/command/reference/703i74m3k/config/b_N3K_Config_Commands_703i74/b_N3K_Config_Commands_703i74_chapter_01111.html#wp1351653258
Removed the mentions of N5K and N6K, as I had not specifically verified them.
@Miyoshi-Ryota
Copy link
Contributor Author

It's a WIP. I will test this change for real device.

Fix a bug where the version check for the vrf option
is applied even when the vrf option is not specified.
Fixed an issue where version comparisons failed for two-digit versions.
Previously, Version("10.1") > Version("9.1") was returning False.

This issue affected the vrf option and ssl_strong_ciphers option in nxos_nxapi.
@Miyoshi-Ryota Miyoshi-Ryota changed the title Add vrf option to nxos_nxapi module. [nxos_nxapi] Add vrf option to nxos_nxapi module Aug 2, 2024
@Miyoshi-Ryota Miyoshi-Ryota changed the title [nxos_nxapi] Add vrf option to nxos_nxapi module [nxapi] Add vrf option to nxos_nxapi module Aug 2, 2024
@Miyoshi-Ryota
Copy link
Contributor Author

Miyoshi-Ryota commented Aug 2, 2024

All my work is completed and it is ready for review.

Since Version class bug also affects the functionality I want to add this time, I have made the corrections within the same PR. However, if it needs to be split into two separate PRs, please let me know.
3cb4fef

@roverflow roverflow added the feature This issue/PR relates to a feature request. label Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue/PR relates to a feature request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants