Skip to content
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

Use reference of servers in /system/aaa/server-groups. Move the actual server list to top level of /system/aaa. #1105

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

Conversation

WenyiChengANET
Copy link

@WenyiChengANET WenyiChengANET commented May 3, 2024

(M) system/openconfig-aaa-radius.yang
(M) system/openconfig-aaa-tacacs.yang
(M) system/openconfig-aaa.yang

Change Scope:

Use name instead of only address as the key for a server. A server should be defined with address, port, network-instance and type. Using name as the single key because this need to be a leafref in the server group member list and multi-key ref is not well supported currently.
Deprecate port/authen-port from aaa-tacacs and aaa-radius config. Move it to server config.
Deprecate the server list in the “server-groups” container and move “servers” up to the top level for aaa.
Add a list of server ref matching the top level server list in the “server-groups” container.
This is a slow change which keeps the original nodes and marks them as deprecated.

Platform Implementations:

Arista AAA configuration:
Servers with the same host/port can be configured in different VRFs(network-instance).
One server can be added to multiple server groups.

(config)#radius-server host 1.2.3.4 vrf mgmt-vrf auth-port 123
RADIUS host 1.2.3.4 with auth-port 123 and acct-port 1813 created in vrf mgmt-vrf
(config)#radius-server host 1.2.3.4 vrf mgmt-vrf-2 auth-port 123
RADIUS host 1.2.3.4 with auth-port 123 and acct-port 1813 created in vrf mgmt-vrf-2
(config)#aaa group server radius rad-grp-1
(config-sg-radius-rad-grp-1)#server 1.2.3.4 vrf mgmt-vrf auth-port 123
(config-sg-radius-rad-grp-1)#server 1.2.3.4 vrf mgmt-vrf-2 auth-port 123
(config-sg-radius-rad-grp-1)#exit
(config)#aaa group server radius rad-grp-2
(config-sg-radius-rad-grp-2)#server 1.2.3.4 vrf mgmt-vrf auth-port 123
(config-sg-radius-rad-grp-2)#exit
(config)#show radius
…
RADIUS server-group: rad-grp-1
     0: 1.2.3.4, authentication port 123, accounting port 1813 (VRF mgmt-vrf)
     1: 1.2.3.4, authentication port 123, accounting port 1813 (VRF mgmt-vrf-2)

RADIUS server-group: rad-grp-2
     0: 1.2.3.4, authentication port 123, accounting port 1813 (VRF mgmt-vrf)
…

Arista documentation

Pyang output for the new model:
*please note that the old nodes has been removed from this output for easy review. In the actual change they are marked as deprecated.

module: openconfig-system
  +--rw system
     +--rw aaa
        +--rw config
        +--ro state
        ...<unchanged part>

        +--rw servers
        |  +--rw server* [name]
        |     +--rw name      -> ../config/name
        |     +--rw config
        |     |  +--rw name?               string
        |     |  +--rw address?            oc-inet:ip-address
        |     |  +--rw port?               oc-inet:port-number
        |     |  +--rw network-instance?   oc-ni:network-instance-ref
        |     |  +--rw type?               identityref
        |     |  +--rw timeout?            uint16
        |     +--ro state
        |     |  +--ro name?                  string
        |     |  +--ro address?               oc-inet:ip-address
        |     |  +--ro port?                  oc-inet:port-number
        |     |  +--ro network-instance?      oc-ni:network-instance-ref
        |     |  +--ro type?                  identityref
        |     |  +--ro timeout?               uint16
        |     |  +--ro connection-opens?      oc-yang:counter64
        |     |  +--ro connection-closes?     oc-yang:counter64
        |     |  +--ro connection-aborts?     oc-yang:counter64
        |     |  +--ro connection-failures?   oc-yang:counter64
        |     |  +--ro connection-timeouts?   oc-yang:counter64
        |     |  +--ro messages-sent?         oc-yang:counter64
        |     |  +--ro messages-received?     oc-yang:counter64
        |     |  +--ro errors-received?       oc-yang:counter64
        |     +--rw tacacs
        |     |  +--rw config
        |     |  |  x--rw port?                oc-inet:port-number
        |     |  |  +--rw secret-key?          oc-types:routing-password
        |     |  |  +--rw secret-key-hashed?   oc-aaa-types:crypt-password-type
        |     |  |  +--rw source-address?      oc-inet:ip-address
        |     |  +--ro state
        |     |     x--ro port?                oc-inet:port-number
        |     |     +--ro secret-key?          oc-types:routing-password
        |     |     +--ro secret-key-hashed?   oc-aaa-types:crypt-password-type
        |     |     +--ro source-address?      oc-inet:ip-address
        |     +--rw radius
        |        +--rw config
        |        |  x--rw auth-port?             oc-inet:port-number
        |        |  +--rw acct-port?             oc-inet:port-number
        |        |  +--rw secret-key?            oc-types:routing-password
        |        |  +--rw secret-key-hashed?     oc-aaa-types:crypt-password-type
        |        |  +--rw source-address?        oc-inet:ip-address
        |        |  +--rw retransmit-attempts?   uint8
        |        +--ro state
        |           x--ro auth-port?             oc-inet:port-number
        |           +--ro acct-port?             oc-inet:port-number
        |           +--ro secret-key?            oc-types:routing-password
        |           +--ro secret-key-hashed?     oc-aaa-types:crypt-password-type
        |           +--ro source-address?        oc-inet:ip-address
        |           +--ro retransmit-attempts?   uint8
        |           +--ro counters
        |              +--ro retried-access-requests?   oc-yang:counter64
        |              +--ro access-accepts?            oc-yang:counter64
        |              +--ro access-rejects?            oc-yang:counter64
        |              +--ro timeout-access-requests?   oc-yang:counter64
        +--rw server-groups
           +--rw server-group* [name]
              +--rw name             -> ../config/name
              +--rw config
              |  +--rw name?   string
              |  +--rw type?   identityref
              +--ro state
              |  +--ro name?   string
              |  +--ro type?   identityref
              +--rw group-members
              |  +--rw group-member* [name]
              |     +--rw name      -> ../config/name
              |     +--rw config
              |     |  +--rw name?   -> ../../../../../../servers/server/name
              |     +--ro state
              |        +--ro name?   -> ../../../../../../servers/server/name

@WenyiChengANET WenyiChengANET requested a review from a team as a code owner May 3, 2024 22:25
@dplore
Copy link
Member

dplore commented May 15, 2024

/gcbrun

@OpenConfigBot
Copy link

OpenConfigBot commented May 15, 2024

No major YANG version changes in commit 91152de

@WenyiChengANET
Copy link
Author

It looks like that some checks are complaining about the deref I use in path. Is deref not support in path? Pyang doesn't give me any error on that.

If it's not supported, any suggestion on how to use leafref on a node with multiple keys and make sure all keys are from the same element? Below is the code using deref in the PR:

  grouping aaa-servergroup-member-config {
    description
      "Common configuration data for server group member";

    leaf address {
      type leafref {
        path "../../../../../../servers/server/address";
      }
      description
        "Reference to the configured address of the server group member";
    }

    leaf port {
      type leafref {
        path "deref(../address)/../port";
      }
      description
        "Reference to the configured port of the server group member";
    }

    leaf network-instance {
      type leafref {
        path "deref(../port)/../network-instance";
      }
      description
        "Reference to the configured network-instance of the server group member";
    }

    leaf type {
      type leafref {
        path "deref(../network-instance)/../type";
      }
      description
        "Reference to the configured type of the server group member";
    }
  }

@earies
Copy link
Contributor

earies commented May 15, 2024

It looks like that some checks are complaining about the deref I use in path. Is deref not support in path? Pyang doesn't give me any error on that.

deref() is a valid YANG 1.1 XPath function - OpenConfig models are 1.0 based

deref() is also not a permitted path-arg in 1.1

https://datatracker.ietf.org/doc/html/rfc7950#section-9.9.2
https://datatracker.ietf.org/doc/html/rfc7950#page-205

@AI-Complete
Copy link

Wendyi who did you request review this commit?

@WenyiChengANET
Copy link
Author

Removed deref from the path. Use name as the single key for server and as the leafref in server group member list.

@dplore
Copy link
Member

dplore commented May 29, 2024

/gcbrun

@dplore
Copy link
Member

dplore commented May 30, 2024

/gcbrun

@dplore
Copy link
Member

dplore commented Jul 8, 2024

/gcbrun

description "Network-instance of the authentication server";
}

leaf type {
Copy link
Member

Choose a reason for hiding this comment

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

This aaa server type overlaps/conflicts with the type defined at the server-group level. Do we need this defined per server?

Also, I observe that there is only one base type, so maybe we don't really even need type at all?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for reviewing Darren.
This aaa server type overlaps/conflicts with the type defined at the server-group level. Do we need this defined per server?
An AAA server can be used either directly by itself or within a server group, so we need this type defined per server as well. Vendors can decide what to do if they see a conflict between the type of the group and the type of the server.

Also, I observe that there is only one base type, so maybe we don't really even need type at all?
leaf type here is an identityref which has existing identity RADIUS and TACACS added to it from their own files:



so it seems to have two types with it. Am I using this wrong?

@dplore
Copy link
Member

dplore commented Aug 13, 2024

Placeholder note for myself, this change looks like it is needed for supporting aaa servers that have the same IP address but are in different network instances, is that right @WenyiChengANET ?

@WenyiChengANET
Copy link
Author

Placeholder note for myself, this change looks like it is needed for supporting aaa servers that have the same IP address but are in different network instances, is that right @WenyiChengANET ?

This is correct. It also moves the servers list out from server-groups and puts it on the top level, so that the same server can be added to multiple groups without having duplicated copies.

@dplore
Copy link
Member

dplore commented Oct 17, 2024

/gcbrun

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

Successfully merging this pull request may close these issues.

5 participants