Update sonic-mgmt-common submodule with merged AAA transformer#28
Open
devin-ai-integration[bot] wants to merge 10 commits intomasterfrom
Open
Update sonic-mgmt-common submodule with merged AAA transformer#28devin-ai-integration[bot] wants to merge 10 commits intomasterfrom
devin-ai-integration[bot] wants to merge 10 commits intomasterfrom
Conversation
- Update sonic-mgmt-common submodule with AAA subtree transformer, YANG annotation, and AAA extension YANG - Update sonic-mgmt-framework submodule with Klish CLI XML definitions, Python actioner, and Jinja2 show template - Add AAA_Klish_CLI_HLD.md high-level design document Co-Authored-By: Arthur Poon <arthur.poon@windsurf.com>
Co-Authored-By: Arthur Poon <arthur.poon@windsurf.com>
Co-Authored-By: Arthur Poon <arthur.poon@windsurf.com>
Co-Authored-By: Arthur Poon <arthur.poon@windsurf.com>
Co-Authored-By: Arthur Poon <arthur.poon@windsurf.com>
Co-Authored-By: Arthur Poon <arthur.poon@windsurf.com>
…table Co-Authored-By: Arthur Poon <arthur.poon@windsurf.com>
…evert Co-Authored-By: Arthur Poon <arthur.poon@windsurf.com>
Co-Authored-By: Arthur Poon <arthur.poon@windsurf.com>
Co-Authored-By: Arthur Poon <arthur.poon@windsurf.com>
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
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
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.
Why I did it
Migrates AAA (Authentication, Authorization, Accounting) CLI commands from Click-based implementation in
sonic-utilitiesto Klish-based implementation insonic-mgmt-framework, using the three-layer architecture (XML → Python actioner → REST API) with a Go subtree transformer for bidirectional OpenConfig ↔ SONiC conversion.Work item tracking
How I did it
This PR updates two submodules and adds an HLD document. The actual new code lives in the submodule PRs:
New files across submodules:
openconfig-system-annot.yangaaa_subtree_xfmron/oc-sys:system/aaaopenconfig-aaa-ext.yangfailthrough,fallback,debugxfmr_aaa.goinParams.ygRoot+ reflect union handling; DbToYang via ocbinds)aaa_openconfig_test.gotestappbuild tagaaa.xmlsonic_cli_aaa.pyshow_aaa.j2show aaaoutputAAA_Klish_CLI_HLD.mdKey configuration changes in sonic-mgmt-common:
config/transformer/models_listopenconfig-system.yang,openconfig-system-annot.yangmodels/yang/sonic/import.mksonic-system-aaa.yangtoSONICYANG_IMPORTSSortDepTables()translib/app_interface.gogetAppModuleInfo()with longest prefix matching/openconfig-system:system/aaato CommonApp (transformer) instead of SysAppCI iteration history (7 builds):
inParams.ygRoot(typed YANG objects) instead ofinParams.param(JSON)openconfig-system-annot(must match root element's module)openconfig-system.yangandopenconfig-system-annot.yangtomodels_listSortDepTables(["AAA"])sonic-system-aaa.yangtoSONICYANG_IMPORTSbut test failed (CVL rejectedtacacs+without passkey)local,radiusinstead oftacacs+,localHow to verify it
Full verification requires:
aaa authentication login local radius→ ConfigDBAAA|authentication|login = "local,radius"aaa authentication failthrough enable→ ConfigDBAAA|authentication|failthrough = "True"show aaadisplays correct output/openconfig-system:system/aaareturns correct JSONWhich release branch to backport (provide reason below if selected)
Tested branch (Please provide the tested image version)
Description for the changelog
Add Klish-based AAA CLI commands with OpenConfig-to-SONiC subtree transformer
Critical Review Checklist for Human Reviewer
🔴 HIGH RISK — Requires immediate attention:
sonic-mgmt-framework PR Fix Y2K38: Comprehensive 2038 timestamp overflow fixes across submodules #3 has NOT been CI-validated — The XML, Python actioner, and Jinja2 template have not been tested in a build pipeline. Review the submodule PR carefully for syntax errors, incorrect REST paths, or template rendering issues.
sonic-buildimage VS build has NOT run — This PR updates submodule pointers but hasn't been through the full VS build. The following could fail:
xfmr_aaa.goDbToYang path (e.g.,OpenconfigSystem_System,To_OpenconfigSystem_System_Aaa_Authentication_Config_AuthenticationMethod_Union())kvmtest has NOT been run — User explicitly required kvmtest validation on DUT. This PR cannot be considered complete until kvmtest passes.
HLD document has path inconsistencies — Section 2 (Command Mapping Table) references
/openconfig-aaa:aaa/...but the actual implementation uses/openconfig-system:system/aaa/.... Update the HLD for consistency.🟡 MEDIUM RISK — Functional bugs to verify:
DELETE operations for augmented ext fields — DELETE to
authentication/config/openconfig-aaa-ext:failthroughmatches theAAA_AUTH_CONFIGprefix case in the transformer switch (line 116 ofxfmr_aaa.go) and may delete the entire authentication config entry instead of just the failthrough field. Test DELETE operations for individual ext fields.App routing change affects all paths — The longest prefix matching change in
app_interface.goaffects routing for all OpenConfig paths. Verify that existing paths (sflow, mclag, interfaces, VLAN, etc.) still route correctly. Build 264 passed all existing tests, but integration-level regressions are possible.🟢 LOW RISK — Polish items:
${__params}expansion works correctly with the actioner'sbuild_method_listfilteringLink to config_db schema for YANG module changes
No changes to config_db schema — this PR uses the existing
AAAtable structure defined in sonic-system-aaa.yang.A picture of a cute animal (not mandatory but encouraged)
🦦 (Otter — because AAA is all about authentication, authorization, and otter-ization)
Link to Devin run: https://cisco-demo.devinenterprise.com/sessions/d7ded909325e4f76b881ad440857c380
Requested by: @arthurkkp-cog