Conversation
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.
There was a problem hiding this comment.
Done. Added XValidation on AAASpec:
rule="has(self.serverGroups) || has(self.authentication) || has(self.authorization) || has(self.accounting)"
Also added additional XValidation rules:
AAAServerGroup: servers in a TACACS group must havetacacsconfigAAAMethod:groupNamerequired when type isGroupDeviceRef: immutable
api/core/v1alpha1/aaa_types.go
Outdated
| // 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.)?
There was a problem hiding this comment.
Done. Removed KeyEncryption entirely from the core API. It now lives in the Cisco AAAConfig CRD (api/cisco/nx/v1alpha1/aaaconfig_types.go) where it belongs, since Type6/Type7/Clear are all Cisco-specific encryption types. The core API only references secrets via Kubernetes SecretKeySelector — the provider handles encryption.
api/core/v1alpha1/aaa_types.go
Outdated
| // 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.
There was a problem hiding this comment.
Done. Renamed to VrfName string json:"vrfName,omitempty".
api/core/v1alpha1/aaa_types.go
Outdated
| // 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.
There was a problem hiding this comment.
Done. Renamed to SourceInterfaceName string json:"sourceInterfaceName,omitempty".
api/core/v1alpha1/aaa_types.go
Outdated
| // 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.
There was a problem hiding this comment.
Done. Moved to Cisco AAAConfig CRD. The provider reads it from cfg.Spec.LoginErrorEnable when building the NX-OS authentication config.
internal/provider/cisco/nxos/aaa.go
Outdated
| ) | ||
|
|
||
| // 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.
There was a problem hiding this comment.
Done. Removed all // Path: ... comments from the NX-OS model structs in internal/provider/cisco/nxos/aaa.go.
| } | ||
|
|
||
| // 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.
There was a problem hiding this comment.
Partially addressed. By switching from Patch (merge) to Update (replace), the server group's ProviderRef-list is fully replaced on each reconcile — so removed servers are dropped from the group. However, the TacacsPlusProvider host entries themselves are individual list items and would need explicit deletion if removed from the spec. This could be addressed in a follow-up by reading current state from the device and diffing against the desired spec.
internal/provider/cisco/nxos/aaa.go
Outdated
| } | ||
|
|
||
| // 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.
There was a problem hiding this comment.
Done. Removed the groupName parameter from MapRealmFromMethodType.
| 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.
There was a problem hiding this comment.
Done. DeleteAAA now batches all resets (auth, console auth, authorization, accounting) into a single p.Update() call, followed by group/server deletions.
| 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.
There was a problem hiding this comment.
Done. Changed EnsureAAA from p.Patch() to p.Update() (netconf replace). This ensures the operator fully owns the configuration and prevents leftover/unintended settings from persisting.
Done. The core API (
All Cisco NX-OS specific config has been moved to the |
weneghawi
force-pushed
the
feature/aaa-tacacs-support
branch
from
14c783c to
c4937c1
Compare
weneghawi
force-pushed
the
feature/aaa-tacacs-support
branch
from
715fa10 to
baa51de
Compare
weneghawi
force-pushed
the
feature/aaa-tacacs-support
branch
from
baa51de to
1d90839
Compare
weneghawi
force-pushed
the
feature/aaa-tacacs-support
branch
from
ce3748b to
9b58290
Compare
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. Changed unit test files
|
3 participants
Summary
Restructured the core AAA API to align with the OpenConfig
system/aaaYANG model, making it vendor-agnostic and suitable for multi-vendor support (Nokia, Juniper, Arista, etc.). All Cisco NX-OS specific configuration has been moved to a dedicatedAAAConfigprovider CRD. RADIUS server group support has been added alongside the existing TACACS+ implementation.Core API Changes (
api/core/v1alpha1/aaa_types.go)TACACSServers+TACACSGroupwithServerGroups []AAAServerGroup— protocol-agnostic containers with nested servers, following OpenConfig/system/aaa/server-groups/server-group. Supports bothTACACSandRADIUSgroup types.AAAServerRADIUSstruct withauthPort(default 1812),acctPort(default 1813), andkeySecretRefAuthentication,Authorization, andAccountingto simple method lists (removed NX-OS specific nesting likeLogin.Default/ConsoleandConfigCommands)VRF->VrfName,SourceInterface->SourceInterfaceName(leaves room for future object references)KeyEncryption,LoginErrorEnablemoved to CiscoAAAConfigCRDserverGroups,authentication,authorization, oraccountingmust be settacacsconfigradiusconfiggroupNameis required when method type isGroupdeviceRefis immutableCisco AAAConfig CRD (
api/cisco/nx/v1alpha1/aaaconfig_types.go)ConsoleAuthentication *NXOSMethodList— NX-OS:aaa authentication login consoleConfigCommandsAuthorization *NXOSMethodList— NX-OS:aaa authorization config-commands defaultRADIUSKeyEncryptiontype (Type6/Type7/Clear) withradiusKeyEncryptionfield (default Type7)KeyEncryption(Type6/Type7/Clear) andLoginErrorEnableController (
internal/controller/core/aaa_controller.go)ServerGroups[].Servers[].TACACS.KeySecretRefandServerGroups[].Servers[].RADIUS.KeySecretRefsecretToAAAwatch mapping to trigger reconciliation on changes to both TACACS and RADIUS key secretsNX-OS Provider (
internal/provider/cisco/nxos/)RadiusProvider,RadiusProviderGroup,RadiusProviderRefNX-OS DME structs. AddedMapRADIUSKeyEncryptionhelper. AddedgroupTypeByNameandMapRealmFromGroupto correctly resolve realm as"radius"or"tacacs"based on the referenced server group type. Removed read-onlyNameandRealmfields fromAAADefaultAuthor(NX-OS rejects writes to these). AddedMapNXOSRealm,MapNXOSLocal,MapNXOSFallbackhelpers. Note: RADIUS on NX-OS requires no feature flag (unlike TACACS+ which requiresfeature tacacs+).EnsureAAAto iterateServerGroupswith aswitchon group type covering both TACACS and RADIUS. RewroteDeleteAAAwith batched resets and RADIUS group/server cleanup. Changed fromPatchtoUpdate(netconf replace).Sample YAML (
config/samples/networking_v1alpha1_aaa.yaml)serverGroups, nestedservers, flat method lists, and separate CiscoAAAConfigwith console/config-commands authorization.Test Plan
go build ./...— compiles cleanlygo test ./api/... ./internal/provider/... ./internal/clientutil/...— all passmake run-golangci-lint— 0 issuesmake generate— CRDs and deepcopy regeneratedradius