Adding AAA TACACS Support #176
Conversation
Merging this branch will decrease overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. |
hardikdr
added
the
area/metal-automation
felix-kaestner
left a comment
felix-kaestner
left a comment
There was a problem hiding this comment.
@weneghawi Please have a look at the openconfig-system:system/aaa yang model. Only configurations that are part of this or are otherwise commonly found on all vendors (Nokia, Juniper, Arista & Co.) should be part of the core api. All Cisco NX-OS specific configuration should be refactored into a vendor specific provider config, see e.g. the ManagementAccess resource on how this is done. There is a separate api package for cisco specific CRDs.
| // | ||
| // It models the Authentication, Authorization, and Accounting (AAA) configuration on a network device, | ||
| // including TACACS+ server configuration and AAA group/method settings. | ||
| type AAASpec struct { |
There was a problem hiding this comment.
Can we add some XValidation rules to verify that at least one of the fields in this spec is set? Otherwise, I'd be able to create a resource without any configuration.
| // KeyEncryption specifies the encryption type for the key. | ||
| // Type7 is the Cisco Type 7 encryption (reversible). | ||
| // Type6 is the AES encryption (more secure). | ||
| // Clear means the key is sent in cleartext (not recommended). | ||
| // +optional | ||
| // +kubebuilder:validation:Enum=Type6;Type7;Clear | ||
| // +kubebuilder:default=Type7 |
There was a problem hiding this comment.
We don't want to include cisco specific stuff in the core api (we want this to be vendor agnostic). So at a minimum, we have to remove "Type7". Is Type6/AES universally supported across vendors (Nokia, Juniper, Arista etc.)?
| // VRF is the VRF to use for communication with the TACACS+ servers. | ||
| // +optional | ||
| // +kubebuilder:validation:MaxLength=63 | ||
| VRF string `json:"vrf,omitempty"` |
There was a problem hiding this comment.
| VRF string `json:"vrf,omitempty"` | |
| VrfName string `json:"vrfName,omitempty"` |
Let's use a string name here, to make clear that we are talking about the name of the VRF directly. This would leave us the possibility have "VRF" field in the future, which could then be a resource reference to a "VRF" resource.
| // SourceInterface is the source interface to use for communication with the TACACS+ servers. | ||
| // +optional | ||
| // +kubebuilder:validation:MaxLength=63 | ||
| SourceInterface string `json:"sourceInterface,omitempty"` |
There was a problem hiding this comment.
| SourceInterface string `json:"sourceInterface,omitempty"` | |
| SourceInterfaceName string `json:"sourceInterfaceName,omitempty"` |
Same reasoning as with the VRF, lets append the "Name" suffix to have the possibility for an "SourceInterface" object reference to a "Interface" resource.
| // LoginErrorEnable enables login error messages. | ||
| // +optional | ||
| LoginErrorEnable bool `json:"loginErrorEnable,omitempty"` |
There was a problem hiding this comment.
This configuration seems cisco nx-os specific and should go into a provider config crd.
| ) | ||
|
|
||
| // TacacsPlusProvider represents a TACACS+ server host configuration. | ||
| // Path: System/userext-items/tacacsext-items/tacacsplusprovider-items/TacacsPlusProvider-list[name=<address>] |
There was a problem hiding this comment.
I would leave out these comments as those are already clear from the XPath() functions.
| } | ||
|
|
||
| // Configure TACACS+ server hosts | ||
| for _, server := range req.AAA.Spec.TACACSServers { |
There was a problem hiding this comment.
With this logic, we are leaving over tacacs servers that were once part of the resource, but got removed in a later reconcile, as we only make sure to have those that are in the spec but never clean up any previous ones. We should do this.
| } | ||
|
|
||
| // MapRealmFromMethodType maps the API method type to NX-OS realm. | ||
| func MapRealmFromMethodType(method v1alpha1.AAAMethodType, groupName string) string { |
There was a problem hiding this comment.
The groupName parameter is never used.
| return p.Patch(ctx, conf...) | ||
| } | ||
|
|
||
| func (p *Provider) DeleteAAA(ctx context.Context, req *provider.DeleteAAARequest) error { |
There was a problem hiding this comment.
Please batch all the updates into a single call, similar to the EnsureAAA.
| conf = append(conf, acct) | ||
| } | ||
|
|
||
| return p.Patch(ctx, conf...) |
There was a problem hiding this comment.
Is a Patch (netconf merge) really the operation we'd like to perform here? Or could this lead to leftover configurations that the operator didn't intent to and is unaware of? In general we prefer to do a Update (netconf replace) to make sure that the operator is in charge of all the configuration.
3 participants
Summary
Adds AAA + TACACS+ support with SecretRef for secure key storage, server groups (VRF/source-interface), and full authentication/authorization/accounting configuration.
Manual Testing with gnmic