sonic: Fix STATIC_ROUTE clobber in OOB management path#2238
Open
sonic: Fix STATIC_ROUTE clobber in OOB management path#2238
Conversation
When a device has an OOB IP, generate_sonic_config() built the
management default route by first resetting STATIC_ROUTE to an empty
dict and then writing the single route into it:
config["STATIC_ROUTE"] = {}
config["STATIC_ROUTE"]["mgmt|0.0.0.0/0"] = {"nexthop": metalbox_ip}
The assignment discards any routes loaded from /etc/sonic/config_db.json
before reaching this point, silently wiping them on every sync.
The reset was introduced in f4f2290 ("sonic: Add static default route
in mgmt VRF"), at a time when config_db.json base loading did not exist
yet. config was always an empty dict at that point, so the = {} was
effectively an initialisation and cost nothing. Base config loading was
added later, making the reset destructive without anyone noticing.
Fix by writing the management route directly into the existing dict
without resetting it first.
AI-assisted: Claude Code
Signed-off-by: Roger Luethi <[email protected]>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- When writing to
config["STATIC_ROUTE"]["mgmt|0.0.0.0/0"], consider usingconfig.setdefault("STATIC_ROUTE", {})[...]or otherwise ensuring the key is initialized so the function remains robust if called with a config that lacks aSTATIC_ROUTEsection.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When writing to `config["STATIC_ROUTE"]["mgmt|0.0.0.0/0"]`, consider using `config.setdefault("STATIC_ROUTE", {})[...]` or otherwise ensuring the key is initialized so the function remains robust if called with a config that lacks a `STATIC_ROUTE` section.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
generate_sonic_config()resetSTATIC_ROUTEto{}before writing the management default route, silently discarding any routes loaded from/etc/sonic/config_db.jsonon every sync.Test plan
pytest tests/unit/tasks/conductor/sonic/)STATIC_ROUTEin the base config and asserts entries survive the OOB management path (tracked as follow-up in PR Add unit tests for SONiC config_generator orchestrator and service ca… #2237 review)🤖 Generated with Claude Code