Skip to content

Commit

Permalink
VerifyPlaybookParams(): parameter validation fixes (#335)
Browse files Browse the repository at this point in the history
* WIP: VerifyPlaybookParams(): fix eval_string

1. eval_string was using literal strings instead of f-strings.

2. VerifyPlaybookParams().update_decision_set() conditional for playbook_is_valid should compare to False explicitely.

* dcnm_fabric: cleanup rule_value and user_value processing

Prior to passing eval_string to eval() we need to ensure that rule_value and user_value are not null (None, "", or "null), else, we'll get a syntax error in eval().

Change the handling here such that:

if rule_value is in [None, "", "null"], return True (since, in this case, there is no rule for the parameter).

if user_value is in [None, "", "null"] set the user_value such that the eval() will pass (given current value of operator).

Also, modifications based on pylint recommendations.

* Improve KeyError message content.

verify_playbook_params.py
    VerifyPlaybookParams().eval_parameter_rule() - Add class_name and method_name to all raise KeyError() cases.

* Raise ValueError if playbook value is None or "" or "null"

verify_playbook_params.py - VerifyPlaybookParams().eval_parameter_rule()

I can't think of a case where a playbook value should be any of None, "", or "null".  These are causing eval() to generate a syntax error, so let's raise a ValueError to avoid this.

* dcnm_fabric: update_decision_set() fix, more...

1. verify_playbook_params.py - VerifyPlaybookParams().eval_parameter_rule()

- Revert eval_string change from earlier commit.  The original eval_string was correct.

2. verify_playbook_params.py - VerifyPlaybookParams().update_decision_set()

- Update method docstring
- If all called *_is_valid() methods return None, set decision_set to {True}.

update_decision_set() calls the following methods:
           - controller_param_is_valid()
           - playbook_param_is_valid()
           - default_param_is_valid()

If all of these return None, then decision_set will be empty.
If decision_set is empty we now set it to  {True}.

Below is a summary of when None is returned by these three methods.

- controller_param_is_valid() returns None in the following cases:
    - The fabric does not exist on the controller.
    - The controller's value for the dependent parameter is None
       (more accurately, "")
    - The controller fabric config does not contain the dependent
       parameter (this is not likely)

- default_param_is_valid() returns None in the following cases:
    - The controller config contains the dependent parameter.
    - The dependent parameter has no default value.

- playbook_param_is_valid() returns None in the following cases:
    - The dependent parameter is not in the playbook.

3. Add debug statements throughout the above two methods for future troubleshooting.

* Update unit tests to work with this PR's fixes.

ValueError() is (correctly) not being raised now for the parameter combinations in the following unit tests:

- test_verify_playbook_params_00070
- test_verify_playbook_params_00080

Modified the tested parameters and expected regex so that these are now passing.

* fix for network_crash

* Update unit tests to work with ND 321e

ND 321e requires ENABLE_PVLAN and ENABLE_SGT to be false if UNDERLAY_IS_V6 is false.

Updating two integration tests to set these correctly.

* dcnm_fabric IT: Remove UNDERLAY_IS_V6: false

WIth ND 321e, UNDERLAY_IS_V6=false requires ENABLE_PVLAN==false and ENABLE_SGT==false.  Since ENABLE_PVLAN is not backwards compatible with earlier versions of NDFC, we're removing UNDERLAY_IS_V6 from integration tests.

* Update main.yml

* Update main.yml

Include matrix name in collection

* Add 'skip_validation' knob

Add playbook knob to skip parameter validation.

* Add documention for skip_validation

* Update documentation to include skip_validation.

1. Add an example with skip_validation set to true.

2. Add an example that generates a misleading error message and explain how to interpret the message.

* Fix sanity errors

Remove markdown backticks from documentation.

* Fix sanity errors (continued)

Comment out error message since it generates YAML error.

* Fix sanity errors (continued)

* Run collection_prep to update .rst file.

---------

Co-authored-by: prabahal <[email protected]>
Co-authored-by: Mike Wiebe <[email protected]>
  • Loading branch information
3 people authored Oct 30, 2024
1 parent dff68aa commit 743ea87
Show file tree
Hide file tree
Showing 6 changed files with 313 additions and 34 deletions.
66 changes: 66 additions & 0 deletions docs/cisco.dcnm.dcnm_fabric_module.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1940,6 +1940,7 @@ Parameters
<a class="ansibleOptionLink" href="#parameter-" title="Permalink to this option"></a>
<div style="font-size: small">
<span style="color: purple">list</span>
/ <span style="color: purple">elements=string</span>
</div>
</td>
<td>
Expand All @@ -1958,6 +1959,7 @@ Parameters
<a class="ansibleOptionLink" href="#parameter-" title="Permalink to this option"></a>
<div style="font-size: small">
<span style="color: purple">list</span>
/ <span style="color: purple">elements=string</span>
</div>
</td>
<td>
Expand All @@ -1976,6 +1978,7 @@ Parameters
<a class="ansibleOptionLink" href="#parameter-" title="Permalink to this option"></a>
<div style="font-size: small">
<span style="color: purple">list</span>
/ <span style="color: purple">elements=string</span>
</div>
</td>
<td>
Expand All @@ -1994,6 +1997,7 @@ Parameters
<a class="ansibleOptionLink" href="#parameter-" title="Permalink to this option"></a>
<div style="font-size: small">
<span style="color: purple">list</span>
/ <span style="color: purple">elements=string</span>
</div>
</td>
<td>
Expand Down Expand Up @@ -7325,6 +7329,25 @@ Parameters
</tr>


<tr>
<td colspan="3">
<div class="ansibleOptionAnchor" id="parameter-"></div>
<b>skip_validation</b>
<a class="ansibleOptionLink" href="#parameter-" title="Permalink to this option"></a>
<div style="font-size: small">
<span style="color: purple">boolean</span>
</div>
</td>
<td>
<ul style="margin: 0; padding: 0"><b>Choices:</b>
<li><div style="color: blue"><b>no</b>&nbsp;&larr;</div></li>
<li>yes</li>
</ul>
</td>
<td>
<div>Skip playbook parameter validation. Useful for debugging.</div>
</td>
</tr>
<tr>
<td colspan="3">
<div class="ansibleOptionAnchor" id="parameter-"></div>
Expand Down Expand Up @@ -7407,6 +7430,27 @@ Examples
- debug:
var: result
# Setting skip_validation to True to bypass parameter validation in the module.
# Note, this does not bypass parameter validation in NDFC. skip_validation
# can be useful to verify that the dcnm_fabric module's parameter validation
# is disallowing parameter combinations that would also be disallowed by
# NDFC.
- name: Update fabrics
cisco.dcnm.dcnm_fabric:
state: merged
skip_validation: True
config:
- FABRIC_NAME: VXLAN_Fabric
FABRIC_TYPE: VXLAN_EVPN
BGP_AS: 65000
ANYCAST_GW_MAC: 0001.aabb.ccdd
UNDERLAY_IS_V6: false
EXTRA_CONF_LEAF: |
interface Ethernet1/1-16
description managed by NDFC
DEPLOY: false
# Use replaced state to return the fabrics to their default configurations.
- name: Return fabrics to default configuration.
Expand Down Expand Up @@ -7453,6 +7497,28 @@ Examples
- debug:
var: result
# When skip_validation is False (the default), some error messages might be
# misleading. For example, with the playbook below, the error message
# that follows should be interpreted as "ENABLE_PVLAN is mutually-exclusive
# to ENABLE_SGT and should be removed from the playbook if ENABLE_SGT is set
# to True." In the NDFC GUI, if Security Groups is enabled, NDFC disables
# the ability to modify the PVLAN option. Hence, even a valid value for
# ENABLE_PVLAN in the playbook will generate an error.
- name: merge fabric MyFabric
cisco.dcnm.dcnm_fabric:
state: merged
skip_validation: false
config:
- FABRIC_NAME: MyFabric
FABRIC_TYPE: VXLAN_EVPN
BGP_AS: 65001
ENABLE_SGT: true
ENABLE_PVLAN: false
# Resulting error message (edited for brevity)
# "The following parameter(value) combination(s) are invalid and need to be reviewed: Fabric: f3, ENABLE_PVLAN(False) requires ENABLE_SGT != True."
Expand Down
Loading

0 comments on commit 743ea87

Please sign in to comment.