Skip to content

manage edge bgp_password - #803

Open
ccoueffe wants to merge 5 commits into
netascode:developfrom
ccoueffe:update_defaults_bgp_password_enable
Open

manage edge bgp_password#803
ccoueffe wants to merge 5 commits into
netascode:developfrom
ccoueffe:update_defaults_bgp_password_enable

Conversation

@ccoueffe

Copy link
Copy Markdown
Collaborator

update bgp_password_enable for edge connection. Change to false. By default it was enabled but if you don't provide password it failed, because password can't be empty.

Related Issue(s)

Related Collection Role

  • cisco.nac_dc_vxlan.validate
  • cisco.nac_dc_vxlan.dtc.create
  • cisco.nac_dc_vxlan.dtc.deploy
  • cisco.nac_dc_vxlan.dtc.remove
  • other

Related Data Model Element

  • vxlan.fabric
  • vxlan.global
  • vxlan.topology
  • vxlan.underlay
  • vxlan.overlay
  • vxlan.overlay_extensions
  • vxlan.policy
  • vxlan.multisite
  • defaults.vxlan
  • other

Proposed Changes

change default value for edge bgp password enable.

Test Notes

Cisco Nexus Dashboard Version

Checklist

  • Latest commit is rebased from develop with merge conflicts resolved
  • New or updates to documentation has been made accordingly
  • Assigned the proper reviewers

update bgp_password_enable for edge connection. Change to false.
By default it was enabled but if you don't provide password it failed, because password can't be empty.
@ccoueffe ccoueffe self-assigned this May 29, 2026
@ccoueffe
ccoueffe requested a review from a team as a code owner May 29, 2026 14:38
@ccoueffe ccoueffe added the ready for review PR Ready for Review label May 29, 2026
@juburnet juburnet added the 0.9.0 Release 0.9.0 label Aug 5, 2026
@juburnet
juburnet requested a review from mthurstocisco August 5, 2026 17:05
@mthurstocisco

Copy link
Copy Markdown
Collaborator

Hey Charlie,
So, had a bit of a think, I wonder if something like the below makes more sense and we just dont use a default here? basically, if you provide a password, the jinja template sets password to true, if you dont provide a password (or its blank), set password to false?

  policies:
    - create_additional_policy: False
      description: {{ 'nace_bgp_peer_template_dci_underlay_jython_' + link.source_device + '_' + link.source_interface + '_' + link.dest_device }}
      name: bgp_peer_template_dci_underlay_jython
      policy_vars:
        BGP_PASSWORD: "{{ link.bgp_section.bgp_password | default('') }}"

{% if link.bgp_section is defined and link.bgp_section.bgp_password is defined and link.bgp_section.bgp_password is not none %}
BGP_PASSWORD_ENABLE: true
{% else %}
BGP_PASSWORD_ENABLE: false
{% endif %}

@ccoueffe ccoueffe changed the title Update defaults.yml edge bgp_password_enable manage edge bgp_password Aug 6, 2026

@peter8498 peter8498 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@juburnet
juburnet requested a review from mikewiebe August 10, 2026 16:06
policy_vars:
BGP_PASSWORD: "{{ link.bgp_section.bgp_password | default('') }}"
BGP_PASSWORD_ENABLE: {{ link.bgp_section.bgp_password_enable | default(defaults.vxlan.topology.edge_connections.bgp_section.bgp_password_enable) }}
{% if link.bgp_section is defined and link.bgp_section.bgp_password is defined and link.bgp_section.bgp_password is not none and link.bgp_section.bgp_password | length > 0 %}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium: Preserve the public bgp_password_enable contract

Issue

The changed template derives BGP_PASSWORD_ENABLE only from whether bgp_password is non-empty. It never reads the schema-valid bgp_password_enable input. That silently changes the public contract from an explicit enable control, defaulting to true, to password-presence inference.

Evidence

Practical example

An operator can pre-stage a BGP password in the model while deliberately keeping authentication disabled until the remote peer is ready:

vxlan:
  topology:
    edge_connections:
      - source_device: border-1
        source_interface: Ethernet1/1
        dest_device: wan-router-1
        dest_fabric: WAN
        local_ip: 192.0.2.1/30
        neighbor_ip: 192.0.2.2
        bgp_section:
          neighbor_asn: 65100

          # The password is staged, but authentication must remain disabled.
          bgp_password_enable: false
          bgp_password: "example-staged-secret"

The explicit intent is:

BGP_PASSWORD_ENABLE: false
BGP_PASSWORD: "example-staged-secret"

PR 803 ignores the Boolean and sees only that the password is non-empty, so it renders:

BGP_PASSWORD_ENABLE: true
BGP_PASSWORD: "example-staged-secret"

If the remote router has not enabled the same authentication yet, the NaC run unexpectedly enables it on border-1; the peers no longer agree and the established BGP session can drop and remain down. The inverse is also silent: bgp_password_enable: true with an accidentally omitted password renders BGP_PASSWORD_ENABLE: false instead of failing validation for incomplete input.

Existing PR overlap

No matching existing PR comment found. The August 6 suggestion from mthurstocisco proposes password-presence gating, but it does not reconcile, remove, or deprecate the existing public enable property.

Impact

A user who explicitly disables password authentication while retaining a password gets authentication enabled. That can reset an established BGP session and prevent it from returning if the peer is not using the same password. Conversely, explicit enable intent without a password is silently disabled instead of being rejected as incomplete input. In both cases, a schema-valid setting is accepted and then contradicted by the rendered intent.

Suggested fix

Choose one authoritative contract and enforce it end to end. If the Boolean remains public, honor its effective value; when true, require a non-empty password through semantic validation, and when false keep enable false even if a password is supplied. If passwordless edge BGP should be the normal omission behavior, coordinate a source-model default change from true to false rather than overriding the source default in this template. In either case, emit the desired password key deterministically as described in Finding 2. Add schema/semantic/render tests for omitted, empty, null, present, explicit false plus present, and explicit true plus absent.

If password presence is intentionally replacing the Boolean, coordinate a source-model change that deprecates or removes bgp_password_enable, updates its default and generated docs, rejects ambiguous legacy combinations with a migration message, and lands before this behavior is released.

@mikewiebe

Copy link
Copy Markdown
Collaborator

Medium: Emit an empty password so removal is observable to dcnm_policy

Issue

The false branch omits BGP_PASSWORD entirely. dcnm_policy compares only keys present in desired nvPairs; it ignores additional keys already stored by NDFC. An existing disabled policy containing a password can therefore match the disabled/no-password payload, preventing the password from being cleared.

Evidence

  • The PR emits only BGP_PASSWORD_ENABLE: false when no password is supplied. The previous template always emitted BGP_PASSWORD, using an empty string when absent.
  • In DCNM 3.12.1, dcnm_policy_compare_nvpairs() iterates only desired keys.
  • An exact method test returned DCNM_POLICY_MATCH for desired disabled/no-password state versus existing disabled/non-empty-password state.
  • This remains unchanged on current DCNM develop commit 939c75bb667498631e9047ff9f62f424f00b98c2, which is planned for the next release.

Practical example

Assume an earlier run left this policy in NDFC:

existing_in_ndfc:
  BGP_PASSWORD_ENABLE: false
  BGP_PASSWORD: "<redacted-existing-secret>"

The operator removes bgp_password from the NaC model because the secret must be cleared. PR #803 renders:

desired_from_nac:
  BGP_PASSWORD_ENABLE: false
  # BGP_PASSWORD is absent

The comparator checks BGP_PASSWORD_ENABLE, sees false == false, never examines the extra stored password, and returns:

DCNM_POLICY_MATCH

No update is sent, so NDFC retains the secret. Publishing current DCNM develop does not change this result.

The always-present assignment proposed by mthurstocisco is therefore important for reset semantics:

desired_from_nac:
  BGP_PASSWORD_ENABLE: false
  BGP_PASSWORD: ""

Now the comparison sees empty versus non-empty and detects an update.

Existing PR overlap

This strengthens the unanswered August 6 suggestion from mthurstocisco. The current implementation adopts its enable gating but drops its always-present BGP_PASSWORD assignment.

Impact

Removing a password from the source model can leave the secret stored in NDFC while the run reports no policy change. It could later reappear if authentication is re-enabled.

Suggested fix

Always emit BGP_PASSWORD, resolving absent or null values to "", and compute BGP_PASSWORD_ENABLE separately according to the public model contract. Add a transition test asserting that existing disabled/non-empty state changes to desired disabled/empty state and that NDFC clears the stored value.

ccoueffe and others added 2 commits August 13, 2026 10:27
…hen enabled

- Template: if bgp_password_enable is true, send password; if false, send empty password
- Validation: rule 321 now rejects bgp_password_enable=true without a non-empty bgp_password
- Always emit BGP_PASSWORD key for dcnm_policy diff detection
@ccoueffe

Copy link
Copy Markdown
Collaborator Author

Hi Mike,

Thanks for the thorough review. I went back to my initial approach — keeping the boolean — but with a hybrid logic:

  • We honor bgp_password_enable as the public contract
  • If only the password is defined (without explicit boolean), it works because the default is true — so it just pushes
  • We now catch the case where enable is true but password is empty/missing with a validation error

This covers the points raised by Matt and Peter during the meeting, your review comments, and my original concern about the empty password failure.

Template changes (ndfc_edge_connections.j2):

  • When bgp_password_enable is true: sends the password as-is
  • When bgp_password_enable is false: sends BGP_PASSWORD: "" — aligned with ND behavior where disabling eBGP password authentication clears the password
  • BGP_PASSWORD is always emitted so dcnm_policy can detect password removal (addresses your finding about dcnm_policy_compare_nvpairs only comparing desired keys)

Validation rule (321):

  • If bgp_password_enable is true (explicit or default) and bgp_password is empty/missing → HIGH severity validation error
  • This catches the original bug (default true + no password = NDFC deploy failure) at validation time instead of deploy time

Regarding the staged password scenario (bgp_password_enable: false + password provided):

  • In ND, disabling eBGP password removes the password — there is no concept of "staged but disabled"
  • Our template mirrors this: false = empty password, regardless of what is in the model
  • Since NaC is declarative (full state pushed every run), re-enabling just requires setting bgp_password_enable: true + password in the model and re-running

Behavior matrix:

bgp_password_enable bgp_password Result
true (explicit or default) provided ENABLE: true, PW: "secret"
true (explicit or default) missing/empty ❌ Validation error
false provided ENABLE: false, PW: ""
false missing/empty ENABLE: false, PW: ""

@peter8498 peter8498 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@mthurstocisco

Copy link
Copy Markdown
Collaborator

Hi @ccoueffe my worry is that to transition away from the current setup and defaults (IE enable is true) then, at least for our customers, your change will silently turn off authentication with no warnings, I think that is too much of a breaking change. Currently we have customers just specifying a password (as that was all that was required previously), with your change (based on your matrix above) they will have their authentication turned off and password removed. Please can you look at this logic again to better handle this breaking change transition

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0.9.0 Release 0.9.0 ready for review PR Ready for Review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants