- 
                Notifications
    You must be signed in to change notification settings 
- Fork 125
          [nxapi] Add vrf option to nxos_nxapi module
          #877
        
          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
          
     Open
      
      
            Miyoshi-Ryota
  wants to merge
  9
  commits into
  ansible-collections:main
  
    
      
        
          
  
    
      Choose a base branch
      
     
    
      
        
      
      
        
          
          
        
        
          
            
              
              
              
  
           
        
        
          
            
              
              
           
        
       
     
  
        
          
            
          
            
          
        
       
    
      
from
Miyoshi-Ryota:add-vrf-option-fo-nxapi-module
  
      
      
   
  
    
  
  
  
 
  
      
    base: main
Could not load branches
            
              
  
    Branch not found: {{ refName }}
  
            
                
      Loading
              
            Could not load tags
            
            
              Nothing to show
            
              
  
            
                
      Loading
              
            Are you sure you want to change the base?
            Some commits from the old base branch may be removed from the timeline,
            and old review comments may become outdated.
          
          
                
     Open
            
            
  
    [nxapi] Add vrf option to nxos_nxapi module
  
  #877
              
                    Miyoshi-Ryota
  wants to merge
  9
  commits into
  ansible-collections:main
from
Miyoshi-Ryota:add-vrf-option-fo-nxapi-module
  
      
      
   
              
            
      
        
          +147
        
        
          −3
        
        
          
        
      
    
  
Conversation
  
    
      This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
      Learn more about bidirectional Unicode characters
    
  
  
    
    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.
| 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.
    vrf option to nxos_nxapi module.vrf option to nxos_nxapi module
      vrf option to nxos_nxapi modulevrf option to nxos_nxapi module
      | 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. | 
  
    Sign up for free
    to join this conversation on GitHub.
    Already have an account?
    Sign in to comment
  
      
  Add this suggestion to a batch that can be applied as a single commit.
  This suggestion is invalid because no changes were made to the code.
  Suggestions cannot be applied while the pull request is closed.
  Suggestions cannot be applied while viewing a subset of changes.
  Only one suggestion per line can be applied in a batch.
  Add this suggestion to a batch that can be applied as a single commit.
  Applying suggestions on deleted lines is not supported.
  You must change the existing code in this line in order to create a valid suggestion.
  Outdated suggestions cannot be applied.
  This suggestion has been applied or marked resolved.
  Suggestions cannot be applied from pending reviews.
  Suggestions cannot be applied on multi-line comments.
  Suggestions cannot be applied while the pull request is queued to merge.
  Suggestion cannot be applied right now. Please check back later.
  
    
  
    
SUMMARY
Add
vrfoption tonxos_nxapimodule.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 sandboxcommand 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-vrfcommand 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:presentwhere 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:
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:
For N9K:
For N7K:
For N3K:
ISSUE TYPE
COMPONENT NAME
nxos_nxapi_module
ADDITIONAL INFORMATION