public icon indicating copy to clipboard operation
public copied to clipboard

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

Open WenyiChengANET opened this issue 1 year ago • 24 comments

(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 avatar May 03 '24 22:05 WenyiChengANET

/gcbrun

dplore avatar May 15 '24 21:05 dplore

No major YANG version changes in commit 4ff1cbf398cf955f1c061548ee64f52bd49be4f8

OpenConfigBot avatar May 15 '24 21:05 OpenConfigBot

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";
    }
  }

WenyiChengANET avatar May 15 '24 21:05 WenyiChengANET

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

earies avatar May 15 '24 22:05 earies

Wendyi who did you request review this commit?

AI-Complete avatar May 28 '24 12:05 AI-Complete

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

WenyiChengANET avatar May 29 '24 22:05 WenyiChengANET

/gcbrun

dplore avatar May 29 '24 23:05 dplore

/gcbrun

dplore avatar May 30 '24 02:05 dplore

/gcbrun

dplore avatar Jul 08 '24 16:07 dplore

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 ?

dplore avatar Aug 13 '24 16:08 dplore

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.

WenyiChengANET avatar Aug 14 '24 22:08 WenyiChengANET

/gcbrun

dplore avatar Oct 17 '24 04:10 dplore

/gcbrun

dplore avatar Oct 23 '24 20:10 dplore

/gcbrun

dplore avatar Oct 31 '24 23:10 dplore

This change is needed for configuring AAA servers in network-instances. I also observe 3 implementations which already support single and server group based configuration.

Quoting more implementation references:

Arista EOS documentation shows server and server group configuration.

Cisco IOS XR supports single aaa server and grouped aaa server configuration.

JunOS supports group and single server configuration (the example uses a group, but it is mentioned that the group is optional).

SRLinux appears to only support aaa configuration in a group

dplore avatar Oct 31 '24 23:10 dplore

Setting last call to Nov 14, 2024

dplore avatar Oct 31 '24 23:10 dplore

Should this one maybe get a bit more visibility and discussion? I don't think it has come up on the monthly vendor meeting. Although it isn't NBC right now (since the old model is still available but deprecated) it will be a breaking change later.

The references above do show implementations with servers defined outside of server-groups, but all those implementations seem to use address as the server key today.

Do any OpenConfig models actually point to an individual server rather than a server-group? Is there a plan to do so? I can see how some implementations may support that but if OpenConfig models don't refer to an individual server then is there a need to make that change in the model?

Adding the NI leafref does seem like a needed addition though.

jsterne avatar Nov 07 '24 19:11 jsterne

/gcbrun

dplore avatar Dec 03 '24 17:12 dplore

Revised last call date 2024-Dec-20

dplore avatar Dec 12 '24 16:12 dplore

Deprecate the server list in the “server-groups” container and move “servers” up to the top level for aaa.

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.

  1. OpenConfig today does not define the usage of a single server by itself. Nor it should, in my opinion.
    Accounting and authorization method reference server groups (which makes sense) or an identity. Is there an operational use case for singular, group-independent server usage?

https://github.com/openconfig/public/blob/f01a86de7e54f5c6f54f6b74c408a08014e6d4a3/release/models/system/openconfig-aaa.yang#L464-L481

https://github.com/openconfig/public/blob/f01a86de7e54f5c6f54f6b74c408a08014e6d4a3/release/models/system/openconfig-aaa.yang#L613-L630

Vendors can decide what to do if they see a conflict between the type of the group and the type of the server.

  1. Leaving such things up to "vendors to decide" is not great since the purpose of OC is to define a consistent behavior across all implementations.

  2. This seems like a major change (which is breaking from the operational point of view, even thought not breaking in yang terms as noted by Jason) that is done with the primary purpose of aligning the OC model with the Arista's implementation.

    • As already noted by Jason, other provided references don't seem to use global servers list together with the groups [where the servers are referenced by name].
    • It is unclear whether the need for "multiple servers in the same group with the same IP address but in different network instances" is actually a real use case that cannot be solved otherwise. If an operator has multiple mgmt network instances, over which a specific server (that has a single IP) can be reached, they can use multiple groups to provide redundancy.

LimeHat avatar Dec 12 '24 16:12 LimeHat

I've checked with the IOS XR AAA team, and we allow a server to be configured outside a server group and then referenced (but as Jason indicates it is keyed by IP address).

I think that the change that is being proposed here is more impactful than it needs to be. I.e., I suspect in some (perhaps many) cases, defining the servers under a server-group is logically the right place to put them. So, I don't think that we should be deprecating those existing leaves.

It seems that the core desire is to allow servers being listed outside server-groups, so that they can be shared across multiple groups. This seems reasonable, but I would perhaps renamed the top level list from servers to shared-servers, and rename group-members to something like shared-server-members.

rgwilton avatar Dec 16 '24 14:12 rgwilton

Based on feedback from the OC operators and OC community meetings, there is consensus that we need to add network-instance to the aaa models. There is not consensus to refactor the model to allow aaa servers to be defined outside of a aaa group.

@WenyiChengANET to move this PR partially forward, can you refactor this one or open a new PR which only adds network-instance.

dplore avatar Jan 21 '25 18:01 dplore

Hi @WenyiChengANET we would still like to see the change for just the network-instance and move the grouping changes to a separate PR. We are not seeing requests from operators or vendors outside of Arista to change the model regarding the groupings.

dplore avatar Feb 11 '25 17:02 dplore

Hi @WenyiChengANET we are waiting to refactor this to only include the network-instance addition.

dplore avatar Mar 05 '25 00:03 dplore