Adding support for svi enabled at network attachment level (supported on >= 4.1

[juarocha]

Fixes #690

Note:
This feature is only supported on ND v4.1 newer.

I didn't see any version knob on the repo for this type of features

[mikewiebe]

Hi @juarocha — you're right that there's no existing version knob for ND 4.1 specifically. The repo has two patterns for version gating; the one that fits dcnm_network.py without a larger refactor is the float-based approach already used in dcnm.py for multicluster proxy selection.

get_nd_version() returns a float derived from the NDFC version string (e.g. "12.4.1.245" → 12.4). ND 4.1 = NDFC 12.4.x, so the guard would be:

nd_version = get_nd_version(self._action_module, self._task_vars, self._tmp)
if nd_version >= 12.4:
    svi_enabled = bool(attach.pop("svi_enabled", True))
    inst_values = {"sviEnabled": "true" if svi_enabled else "false"}
    attach.update({"instanceValues": json.dumps(inst_values)})
else:
    attach.update({"instanceValues": ""})

The alternative is to add an is_controller_version_41 property to controller_version.py and wire ControllerVersion into dcnm_network.py, but that's a more involved refactor and out of scope for this change.

[juarocha]

The get_nd_version function is for action plugins, but i found a version var (self.dcnm_version) on the module and I used for the same knob logic

[mikewiebe]

High: Preserve disabled SVI state when svi_enabled is omitted in merged state

Issue

The new attachment option defaults to true even when the user omits it. With state: merged, that turns omission into a new desire to enable the SVI, and the diff treats an existing NDFC attachment whose sviEnabled value is "false" as changed. This violates merged-state preservation semantics.

replaced and overridden deliberately default omitted managed properties, so those states are not part of this finding.

Evidence

• plugins/modules/dcnm_network.py line 5275 assigns default=True during attachment validation.
• plugins/modules/dcnm_network.py line 1690 also defaults a missing value to True while constructing instanceValues.
• plugins/modules/dcnm_network.py line 1442 turns the resulting "false" to "true" mismatch into an attachment update.
• main() at head lines 5810–5817 sends merged through get_diff_merge() without replacement semantics, while replaced and overridden intentionally use the replacement path.
• The module’s state contract at head lines 511–534 says merged leaves unspecified configuration untouched, whereas replaced and overridden delete or default unspecified managed properties.
• A direct in-memory merged-state probe with unchanged ports, controller state {"sviEnabled": "false"}, and omission-generated desired state {"sviEnabled": "true"} returned one attachment diff and dep_net=True.

Existing PR overlap

Related: the existing version-gating discussion addresses how to identify ND 4.1, but it does not cover backward-compatible omission semantics.

Impact

After upgrading the collection, an otherwise unchanged state: merged task can silently enable an SVI that an operator disabled in NDFC. This is both a backward-compatibility break and an idempotency regression.

It applies to standalone and parent MSD/MCFG attachment processing. It does not apply to deleted or query; omission under replaced and overridden follows their documented defaulting contract.

Suggested fix

Keep omission distinct from an explicit boolean. For an existing attachment in state: merged, only override sviEnabled when the user supplied svi_enabled; otherwise preserve the value returned by NDFC.

Explicit true/false must continue to work in every applicable state. New attachments and replaced/overridden can continue using true as the default if that is the intended contract.

Add state-specific tests for:

• Omission with existing controller values of both true and false.
• A merged port update that retains the existing SVI value.
• Explicit true and false.
• An idempotent rerun.

[mikewiebe]

High: Complete the omission fix by sanitizing pre-12.4 attachment payloads

Issue

The first posted finding covers omission on a controller that supports sviEnabled: in state: merged, an omitted option must preserve the existing value rather than default it to true. This companion finding covers the older-controller side of the same input-handling problem.

svi_enabled is an Ansible input name, not an NDFC attachment payload field. The module should translate it into instanceValues.sviEnabled on NDFC 12.4+ and remove it on earlier controllers, where the feature is unsupported.

Instead, the code removes and translates the key only inside the 12.4+ branch. The pre-12.4 branch sets instanceValues to an empty string but leaves the raw snake-case key in the attachment dictionary.

Controller path	Required behavior	Current behavior
NDFC 12.4+	Remove svi_enabled and translate it to instanceValues.sviEnabled	Does so
Earlier than 12.4	Remove the unsupported Ansible key; reject explicit use or omit it according to the contract	Leaves svi_enabled in the REST payload

For example, this playbook never requests the new option:

state: merged
config:
  - net_name: Network-1
    attach:
      - ip_address: 10.1.1.10
        ports:
          - Ethernet1/1

Validation currently injects svi_enabled: true. If an attachment change must be sent to a pre-12.4 controller, the final request contains the equivalent of:

{
  "serialNumber": "ABC123",
  "switchPorts": "Ethernet1/1",
  "svi_enabled": true,
  "instanceValues": ""
}

Resolving the first finding only for merged-state preservation does not remove this payload leak. Removing the automatic default would protect omitted values, but an explicitly supplied value would still leak on pre-12.4.

This finding is fully resolved only if the implementation also removes the raw key from every outgoing payload and version-gates explicit use.

Evidence

• plugins/modules/dcnm_network.py line 1689 enters the only branch that removes svi_enabled; the else branch merely sets instanceValues to an empty string.
• plugins/modules/dcnm_network.py line 5275 causes every non-query attachment to contain the key even when the playbook omitted it.
• push_to_remote() at head lines 4879–4901 removes is_deploy and normalizes ToR ports, but performs no general attachment-key sanitization before posting self.diff_attach to /networks/attachments.
• A direct payload probe with dcnm_version=12.3 confirmed that the returned API attachment still contains svi_enabled alongside instanceValues.
• The code-level leak into the final request is definite. Whether a particular older controller rejects or ignores the unknown key requires a live test.

Existing PR overlap

• The first posted finding addresses merged-state omission on supported controllers. A comprehensive fix that also sanitizes and version-gates the field can resolve both findings; a merged-only preservation fix cannot.
• The existing version-gating discussion establishes that the feature needs a version guard, but it does not identify this leaked payload field.

Impact

Because validation adds the default even when the user omits the option, existing playbooks can acquire an unknown REST member without opting into this feature.

Any new or changed attachment sent under merged, replaced, or overridden can carry the key on a pre-12.4 controller. Depending on controller validation, the request can fail or rely on undocumented unknown-field handling.

An explicitly supplied false is also silently unsupported rather than producing an actionable compatibility error. deleted, query, and no-change runs do not send this attachment payload and are unaffected.

Suggested fix

Handle explicitness and sanitization together:

1. Remove svi_enabled from the attachment dictionary before branching on the controller version, so the Ansible-only key can never reach NDFC.
2. On NDFC 12.4+, translate an explicitly supplied value into instanceValues.sviEnabled and apply the state-aware omission behavior from the first finding.
3. On pre-12.4, reject an explicitly supplied value with an actionable compatibility error. If it was omitted, send neither the raw key nor a synthetic SVI value.
4. Add final-payload tests for omitted and explicit true/false on 12.3, nested translation on 12.4+, merged-state preservation, and absence of svi_enabled from every request.

[mikewiebe]

Requested changes

[mikewiebe]

Medium: Include svi_enabled in formatted and check-mode diffs

Issue

The internal attachment payload carries the new value inside
instanceValues, but format_diff() reconstructs user-facing attachments with
only IP address, ports, deploy, and optional ToR ports. An SVI-only operation
therefore reports a change without showing the property that changed.

Clear example — disable an existing SVI

Assume NDFC has Network-1 attached to Ethernet1/1 with its SVI enabled.
The user requests only an SVI-state change:

- cisco.dcnm.dcnm_network:
    fabric: Fabric-1
    state: merged
    config:
      - net_name: Network-1
        attach:
          - ip_address: 10.1.1.10
            ports:
              - Ethernet1/1
            svi_enabled: false
  check_mode: true

The internal comparison correctly detects true to false and constructs:

{
  "switchPorts": "Ethernet1/1",
  "instanceValues": "{\"sviEnabled\":\"false\"}"
}

Actual check-mode result

The module returns approximately:

changed: true
diff:
  - net_name: Network-1
    attach:
      - ip_address: 10.1.1.10
        ports: Ethernet1/1
        deploy: true

The unchanged ports are visible, but the reason for changed: true is not.

Expected result

The attachment diff must also contain:

svi_enabled: false

Evidence

• plugins/modules/dcnm_network.py line 1692
  stores sviEnabled only in the internal JSON payload.
• format_diff() at head lines 3806-3811
  constructs an attachment result from ip_address, switchPorts,
  deployment, and optional ToR ports without reading instanceValues.
• The second formatting loop at head lines 3825-3845
  repeats the omission for attachment-only diffs.
• main() at head lines 5842-5846
  assigns this formatted structure directly to result["diff"] and returns it
  immediately in check mode.
• The added tests exercise diff_for_attach_deploy() directly but do not
  assert that an action-plugin/module result exposes svi_enabled.

Existing PR overlap

No matching existing PR comment was found.

Impact

Normal output and check mode cannot tell an operator that the planned change is
an SVI enable/disable operation; the returned attachment can appear identical
to existing port intent. This weakens auditability and makes check-mode review
misleading. The controller request can still carry the correct nested value, so
this is an output-contract defect rather than evidence that NDFC applies the
wrong state. It remains relevant after the two posted input-handling findings
are fixed because explicit true/false operations still require an accurate
preview.

Suggested fix

Teach both attachment-formatting loops to deserialize the managed
sviEnabled value and emit svi_enabled as a public boolean whenever that
property participates in the change. Add module-level and check-mode tests for
the exact example above, both boolean values, combined port-and-SVI changes,
and the absence of svi_enabled when it is not part of the requested diff.

[mikewiebe]

Juan, I just realized that there are significant conflicts between your 3 PRs. It’s going to be easier if you can combine these 3 PRs into a single PR to sort out the conflicts. Following is a detailed plan to move the diffs into a single PR and coordinate the changes so we don’t lose functionality.

High: Coordinate the attachment-property integrations across PRs #687, #689, and #700

Issue

PRs #687, #689, and #700 each merge cleanly into develop individually, but every pair conflicts in the shared dcnm_network attachment pipeline.

These conflicts are substantive. Resolving them with a simple ours/theirs choice—or by only removing conflict markers—can silently discard one feature, leak an Ansible-only key into the NDFC REST request, reset another property during readback, or prevent property-only changes from triggering an update.

The coordinated implementation needs to support all three mappings simultaneously:

Ansible option	NDFC attachment field
vlan_id	vlan
freeform_config	freeformConfig
svi_enabled	instanceValues.sviEnabled

Required integration work

1. Use one coordinated integration branch for these three PRs. A reasonable starting point is the current develop branch plus Adding support for vlan_id override at network attachment level #687’s newer diff_for_attach_deploy(..., network_vlan=...) structure.

2. Retain all three options in both the query and non-query attachment schemas:

   • vlan_id
   • freeform_config
   • svi_enabled

3. Keep option presence separate from option value. This is necessary to distinguish omission from valid falsey values such as:

   • freeform_config: ""
   • svi_enabled: false
   • the VLAN reset sentinel

4. Normalize every managed option into its NDFC field and remove all Ansible-only aliases before constructing the REST request. No outgoing attachment may contain:

   • vlan_id
   • freeform_config
   • svi_enabled

5. Calculate independent change predicates instead of nesting one feature under another:

   ports_changed
   torports_changed
   vlan_changed
   freeform_changed
   svi_changed

   Append one complete attachment update whenever any applicable predicate is true.

6. Do not retain Adding support for freeform config at network attachment level #689’s current indentation around freeform preservation. The established port-change state machine must run independently; otherwise ordinary merged, replaced, and overridden port changes can silently produce no diff.

7. Apply state semantics independently to each property:

   • Under merged, omission must preserve the existing controller value.
   • An explicitly supplied value must update the property even when ports are unchanged.
   • Under replaced and overridden, omitted managed properties must follow their reset/default contract rather than inheriting stale controller values.
   • VLAN omission under replacement must reset the attachment to the network VLAN.
   • Explicit empty freeform configuration must remain distinguishable from omission and support the documented clear behavior.
   • Explicit svi_enabled use on unsupported controller versions should fail clearly, while the raw key must never reach those controllers.

8. Preserve vlan, freeformConfig, and instanceValues together during controller readback. Updating one field must not reset or discard either of the others.

9. Update both attachment-formatting paths in format_diff() so normal and check-mode output map the controller values back to:

   • vlan_id
   • freeform_config
   • svi_enabled

Required combined test

Add at least one attachment that manages all three properties together:

attach:
  - ip_address: 10.1.1.10
    ports:
      - Ethernet1/13
      - Ethernet1/14
    vlan_id: 301
    freeform_config: |
      interface Vlan301
        description Updated
    svi_enabled: false

The resulting REST attachment should contain the equivalent of:

{
  "vlan": 301,
  "freeformConfig": "interface Vlan301\n  description Updated",
  "instanceValues": "{\"sviEnabled\":\"false\"}"
}

The combined tests should verify:

• all three controller fields are present in the same request;
• none of the three Ansible-only aliases reaches NDFC;
• readback preserves all three values;
• changing each property by itself triggers an attachment update;
• existing port and ToR changes still work;
• merged omission preserves controller state;
• replaced and overridden omission/reset behavior is correct;
• explicit false and empty values are not mistaken for omission;
• check mode displays all three public options; and
• a second identical run is idempotent.

This coordinated coverage is needed before the three branches can be considered safe to integrate into the same 3.13.0 release.