Skip to content

Add accounting-method and authorization-method leaf-lists inside the /system/aaa/accounting/events/event and /system/aaa/authorization/events/event lists, respectively. #1332

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

LeonGWang
Copy link
Contributor

@LeonGWang LeonGWang commented Jul 8, 2025

(M) system/openconfig-aaa.yang

Change Scope

AAA accounting-method and authorization-method should be able to be configured per event-type.
For example event-type=AAA_ACCOUNTING_EVENT_COMMAND can have a different accounting-method configured vs. event-type=AAA_ACCOUNTING_EVENT_LOGIN.

This change is an addition of AAA capabilities for accounting-method and authorization-method and is a non-breaking (backwards-compatible) change.

Platform Implementations

Arista AAA configuration:
accounting and authorization can have different accounting-methods and authorization-methods configured (e.g. group tacacs+, group radius, local, logging) per event type (e.g. commands, exec, etc.)

(config)#aaa accounting commands all console start-stop group tacacs+
(config)#aaa accounting exec console start-stop group radius logging
(config)#aaa authorization commands all default group radius
(config)#aaa authorization exec default group tacacs+ local

Arista documentation for authorization

Arista documentation for accounting

Pyang output for the new model:

module: openconfig-system
  +--rw system
     +--rw aaa
        +--rw config
        +--ro state
        +--rw authorization
        |  +--rw config
        |  |  +--rw authorization-method*   union
        |  +--ro state
        |  |  +--ro authorization-method*   union
        |  +--rw events
        |     +--rw event* [event-type]
        |        +--rw event-type    -> ../config/event-type
        |        +--rw config
        |        |  +--rw event-type?             identityref
        |        |  +--rw authorization-method*   union
        |        +--ro state
        |           +--ro event-type?             identityref
        |           +--ro authorization-method*   union
        +--rw accounting
        |  +--rw config
        |  |  +--rw accounting-method*   union
        |  +--ro state
        |  |  +--ro accounting-method*   union
        |  +--rw events
        |     +--rw event* [event-type]
        |        +--rw event-type    -> ../config/event-type
        |        +--rw config
        |        |  +--rw event-type?          identityref
        |        |  +--rw record?              enumeration
        |        |  +--rw accounting-method*   union
        |        +--ro state
        |           +--ro event-type?          identityref
        |           +--ro record?              enumeration
        |           +--ro accounting-method*   union

@LeonGWang LeonGWang requested a review from a team as a code owner July 8, 2025 06:59
@dplore
Copy link
Member

dplore commented Jul 8, 2025

Hi @LeonGWang , thanks for this PR. Please refactor to make this a non-breaking change. One way you could do this is by introducing only new containers/leaves. (ie: instead of move, simply add the authorization-method to the events/event list.).

@dplore
Copy link
Member

dplore commented Jul 8, 2025

/gcbrun

@OpenConfigBot
Copy link

OpenConfigBot commented Jul 8, 2025

No major YANG version changes in commit 69846c3

@LeonGWang LeonGWang changed the title Move accounting-method and authorization-method leaf-lists inside the /system/aaa/accounting/events/event and /system/aaa/authorization/events/event lists, respectively. Add accounting-method and authorization-method leaf-lists inside the /system/aaa/accounting/events/event and /system/aaa/authorization/events/event lists, respectively. Jul 9, 2025
@LeonGWang
Copy link
Contributor Author

Hi @LeonGWang , thanks for this PR. Please refactor to make this a non-breaking change. One way you could do this is by introducing only new containers/leaves. (ie: instead of move, simply add the authorization-method to the events/event list.).

Thanks for the quick review @dplore. I've added back the original leaf-lists but marked them as deprecated. This is now a non-breaking changes.

I have also changed the title, the description, and the pyang output.

@dplore
Copy link
Member

dplore commented Jul 10, 2025

/gcbrun

@dplore dplore moved this to Ready to discuss in OC Operator Review Jul 10, 2025
inside the accounting/events/event and authorization/events/event lists,
respectively.
@LeonGWang LeonGWang force-pushed the aaa-acct-authz-methods branch from 4cc96fb to 69846c3 Compare July 17, 2025 05:11
@LeonGWang
Copy link
Contributor Author

Updated the submodules openconfig-aaa-radius.yang and openconfig-aaa-tacacs.yang to version 1.1.0 as well. This should satisfy the failing check.

@jsterne
Copy link

jsterne commented Jul 17, 2025

Should we not be referencing at least a few other references to implementations with this granularity in this PR?

Although this is not currently a non-backwards-compatible change, by marking the current approach deprecated we are signalling the intent to remove it later (and break current implementations).

inside accounting/events/event and authorization/events/event
lists, respectively.";
reference "1.1.0";
}
Copy link

Choose a reason for hiding this comment

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

No actual change in this file (maybe vestigal from earlier drafts?). Same with the tacacs file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually only added in the latest draft to satisfy a linter check which states submodules openconfig-aaa-radius.yang and openconfig-aaa-tacacs.yang don't share the same version as the main openconfig-aaa.yang module.

@LeonGWang
Copy link
Contributor Author

Thanks for the feedback @jsterne

Should we not be referencing at least a few other references to implementations with this granularity in this PR?

As per company (Arista) policy, unfortunately I am unable to provide references outside of Arista. Usually @dplore is kind enough to provide additional references should that be required.

Although this is not currently a non-backwards-compatible change, by marking the current approach deprecated we are signalling the intent to remove it later (and break current implementations).

This is intended. The newly added *-method leaf-lists at a more granular level provides a functional superset of the original, so there is no reason to keep the original leaf-lists.

@LeonGWang
Copy link
Contributor Author

/gcbrun

1 similar comment
@dplore
Copy link
Member

dplore commented Jul 21, 2025

/gcbrun

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Ready to discuss
Development

Successfully merging this pull request may close these issues.

4 participants