Skip to content

sonic: Generate the entries of the default VRF#2516

Merged
ideaship merged 1 commit into
mainfrom
fix/2515-default-vrf-bgp-globals
Jul 24, 2026
Merged

sonic: Generate the entries of the default VRF#2516
ideaship merged 1 commit into
mainfrom
fix/2515-default-vrf-bgp-globals

Conversation

@berendt

@berendt berendt commented Jul 24, 2026

Copy link
Copy Markdown
Member

Closes #2515

Since the table-ownership enforcement, generate_sonic_config() drops the content of every owned table up front and rebuilds it. The default-VRF entries that the base config_db.json shipped (#1937) were never regenerated, because the generator only writes these tables for the VRFs NetBox carries.

Change

The generator now owns the default-VRF entries as hardcoded SONiC policy, with the values the base config used to carry:

  • VRF|default
  • BGP_GLOBALS|default attributes (network_import_check, ebgp_requires_policy, holdtime/keepalive, …), with router_id/local_asn merged into the entry instead of replacing it
  • BGP_GLOBALS_AF default|ipv4_unicast, default|ipv6_unicast, default|l2vpn_evpn
  • BGP_GLOBALS_ROUTE_ADVERTISE default|L2VPN_EVPN|IPV4_UNICAST and …|IPV6_UNICAST
  • ROUTE_REDISTRIBUTE default|connected|bgp|ipv4 and default|connected|bgp|ipv6

The entries are dropped from files/sonic/config_db.json. Since VRFs with a VNI copy their BGP_GLOBALS entry from the default VRF, they pick up the full attribute set again as well.

All five tables are now written by both the orchestrator (default VRF) and _add_vrf_configuration (per-VRF entries), so they join MULTI_OWNER_OWNED_TABLE_KEYS; the existing static guard keeps both writers on the per-key merge pattern. _add_vrf_configuration creates ROUTE_REDISTRIBUTE via setdefault instead of rebinding it.

Beyond the three BGP_GLOBALS* tables named in the issue, VRF|default and the default-VRF ROUTE_REDISTRIBUTE entries were lost in exactly the same way, so they are covered here too. The only other owned table with base content is SNMP_SERVER|SYSTEM, which _add_snmp_configuration already regenerates.

Tests

  • Regression test: a device without VNI VRFs gets all default-VRF entries, asserted with literal values
  • The policy constants are deep-copied per device, so one device's config cannot rewrite the policy for the next
  • Completeness test: every policy entry reaches the config under the expected key
  • The ownership tests were extended/adjusted: a stale BGP_GLOBALS_AF|default|ipv4_unicast from the base config is overwritten by the generated policy, and a device without a primary IP now gets the policy without router_id/local_asn

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • The DEFAULT_VRF_* policy dictionaries are mutable module-level globals; consider wrapping them in immutable structures (e.g. MappingProxyType or dataclasses with frozen=True) to prevent accidental in-place modification elsewhere in the codebase.
  • The default VRF emission logic added to generate_sonic_config() is fairly substantial; factoring it into a small helper (e.g. _add_default_vrf_configuration(config, device)) would keep the main generator function easier to scan and isolate the default-VRF behavior.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The DEFAULT_VRF_* policy dictionaries are mutable module-level globals; consider wrapping them in immutable structures (e.g. MappingProxyType or dataclasses with frozen=True) to prevent accidental in-place modification elsewhere in the codebase.
- The default VRF emission logic added to generate_sonic_config() is fairly substantial; factoring it into a small helper (e.g. _add_default_vrf_configuration(config, device)) would keep the main generator function easier to scan and isolate the default-VRF behavior.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

VRF, BGP_GLOBALS, BGP_GLOBALS_AF, BGP_GLOBALS_ROUTE_ADVERTISE and
ROUTE_REDISTRIBUTE are owned tables, so their base content is dropped
up front and rebuilt from NetBox data and hardcoded policy. The
default-VRF entries, however, only ever shipped in the image-provided
config_db.json and were never regenerated: the generator writes these
tables only for the VRFs NetBox carries, and it replaced
BGP_GLOBALS|default wholesale with router_id and local_asn.

As a result every regen left BGP_GLOBALS|default without its global
BGP attributes and dropped VRF|default as well as the connected-route
redistribution of the default VRF; on switches without VNI VRFs
BGP_GLOBALS_AF and BGP_GLOBALS_ROUTE_ADVERTISE came back empty. The
default VRF then stopped advertising its IPv4/IPv6 unicast routes into
L2VPN EVPN, so traffic between subnets attached to different leaves
was dropped. VRFs with a VNI inherited the attribute loss as well,
because their BGP_GLOBALS entry is copied from the default one.

Emit the default-VRF entries from the new _add_default_vrf_configuration
as hardcoded SONiC policy, with the values the base config used to
carry, and drop them from files/sonic/config_db.json. router_id and
local_asn are merged into the generated entry instead of replacing it.
The policy lives in read-only module constants that are copied per
device, so it cannot be rewritten process-wide by an in-place edit.

All five tables are co-owned by _add_default_vrf_configuration and
_add_vrf_configuration from now on, so they are listed in
MULTI_OWNER_OWNED_TABLE_KEYS to keep both writers on the per-key merge
pattern.

Closes #2515

Assisted-by: Claude:claude-opus-5
Signed-off-by: Christian Berendt <berendt@osism.tech>
@berendt
berendt force-pushed the fix/2515-default-vrf-bgp-globals branch from b60f215 to 38599bf Compare July 24, 2026 20:17
@berendt berendt moved this from New to Ready for review in Human Board Jul 24, 2026
@berendt
berendt requested a review from ideaship July 24, 2026 20:22
@ideaship
ideaship merged commit e382d05 into main Jul 24, 2026
3 checks passed
@ideaship
ideaship deleted the fix/2515-default-vrf-bgp-globals branch July 24, 2026 20:40
@github-project-automation github-project-automation Bot moved this from Ready for review to Done in Human Board Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

sonic: regen drops default-VRF BGP_GLOBALS/BGP_GLOBALS_AF/BGP_GLOBALS_ROUTE_ADVERTISE entries, breaking EVPN route advertisement

3 participants