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.
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
3 times, most recently
from
ce3748b to
9b58290
Compare
internal/provider/cisco/nxos/aaa.go
Outdated
| } | ||
|
|
||
| // MapNXOSRealm maps an NX-OS method type string to NX-OS realm. | ||
| func MapNXOSRealm(methodType string) string { |
There was a problem hiding this comment.
Please leave out the 'NXOS' in the naming here, as this is already clear from the package nxos.
Ref/ https://google.github.io/styleguide/go/decisions.html#package-vs-exported-symbol-name
There was a problem hiding this comment.
renamed to MapRealm, MapLocal, and MapFallback
There was a problem hiding this comment.
This was not consistently done. I still see NXOSMethodList, NXOSMethody, ...
hardikdr
added
area/switch-automation
weneghawi
force-pushed
the
feature/aaa-tacacs-support
branch
from
98588a4 to
b202cf6
Compare
weneghawi
force-pushed
the
feature/aaa-tacacs-support
branch
from
5c5f3d9 to
3fb343d
Compare
weneghawi
force-pushed
the
feature/aaa-tacacs-support
branch
from
f177bb2 to
cc8c8bd
Compare
Enable or disable the DHCP feature based on AdminState. When enabled, configure DHCP relay on each referenced interface with the specified server addresses. The provider uses the VRF context from VrfRef (or the NXOS default "!unspecified" if no VRF is specified) when configuring server addresses. The implementation uses the Update operation to ensure stale DHCP relay entries are removed when the configuration changes. This also affects entries referencing interfaces not managed by the operator. The entire tree is removed on deletion, affecting non-managed interfaces., It leaves the DHCP feature in its current state. GetDHCPRelayStatus queries the device for all interfaces with DHCP relay configured and returns their names.
weneghawi
force-pushed
the
feature/aaa-tacacs-support
branch
from
cc8c8bd to
9e64d28
Compare
AAA TACACS/RADIUS gNMI Testing
Device: Baseline Readgnmic -a localhost:9339 -u mooapi -p '***' --skip-verify \
get --path 'System/fm-items/tacacsplus-items' \
--path 'System/userext-items/authrealm-items/defaultauth-items' \
--path 'System/userext-items/authrealm-items/consoleauth-items' \
--path 'System/userext-items/tacacsext-items'Result: TACACS disabled, all auth TACACSEnable featuregnmic -a localhost:9339 -u mooapi -p '***' --skip-verify \
set --update-path 'System/fm-items/tacacsplus-items/adminSt' \
--update-value '"enabled"'Add servergnmic -a localhost:9339 -u mooapi -p '***' --skip-verify \
set --update-path 'System/userext-items/tacacsext-items/tacacsplusprovider-items/TacacsPlusProvider-list[name=192.0.2.1]' \
--update-value '{"name":"192.0.2.1","port":49,"keyEnc":"7","key":"testkey","timeout":5}'Create server groupgnmic -a localhost:9339 -u mooapi -p '***' --skip-verify \
set --update-path 'System/userext-items/tacacsext-items/tacacsplusprovidergroup-items/TacacsPlusProviderGroup-list[name=test-group]' \
--update-value '{"name":"test-group","providerref-items":{"ProviderRef-list":[{"name":"192.0.2.1"}]}}'Cleanup# Clear providerGroup reference first (required before group delete)
gnmic -a localhost:9339 -u mooapi -p '***' --skip-verify \
set --update-path 'System/userext-items/authrealm-items/defaultauth-items' \
--update-value '{"realm":"local","providerGroup":"","fallback":"yes","local":"yes"}'
gnmic -a localhost:9339 -u mooapi -p '***' --skip-verify \
set --delete 'System/userext-items/tacacsext-items/tacacsplusprovidergroup-items/TacacsPlusProviderGroup-list[name=test-group]' \
--delete 'System/userext-items/tacacsext-items/tacacsplusprovider-items/TacacsPlusProvider-list[name=192.0.2.1]'
gnmic -a localhost:9339 -u mooapi -p '***' --skip-verify \
set --update-path 'System/fm-items/tacacsplus-items/adminSt' \
--update-value '"disabled"'All operations: ✅ RADIUSAdd servergnmic -a localhost:9339 -u mooapi -p '***' --skip-verify \
set --update-path 'System/userext-items/radiusext-items/radiusprovider-items/RadiusProvider-list[name=192.0.2.2]' \
--update-value '{"name":"192.0.2.2","authPort":1812,"acctPort":1813,"keyEnc":"7","key":"testkey","timeout":5}'Create server groupgnmic -a localhost:9339 -u mooapi -p '***' --skip-verify \
set --update-path 'System/userext-items/radiusext-items/radiusprovidergroup-items/RadiusProviderGroup-list[name=test-radius-group]' \
--update-value '{"name":"test-radius-group","providerref-items":{"ProviderRef-list":[{"name":"192.0.2.2"}]}}'Cleanupgnmic -a localhost:9339 -u mooapi -p '***' --skip-verify \
set --delete 'System/userext-items/radiusext-items/radiusprovidergroup-items/RadiusProviderGroup-list[name=test-radius-group]' \
--delete 'System/userext-items/radiusext-items/radiusprovider-items/RadiusProvider-list[name=192.0.2.2]'All operations: ✅ AAA Authorizationgnmic -a localhost:9339 -u mooapi -p '***' --skip-verify \
set --update-path 'System/userext-items/authrealm-items/defaultauthor-items/DefaultAuthor-list[cmdType=config]' \
--update-value '{"cmdType":"config","localRbac":true}'Result: ✅ — NX-OS correctly populates read-only AAA Accountinggnmic -a localhost:9339 -u mooapi -p '***' --skip-verify \
set --update-path 'System/userext-items/authrealm-items/defaultacc-items' \
--update-value '{"realm":"local","localRbac":true}'Result: ✅ AAA Console Authgnmic -a localhost:9339 -u mooapi -p '***' --skip-verify \
set --update-path 'System/userext-items/authrealm-items/consoleauth-items' \
--update-value '{"realm":"local","fallback":"yes","local":"yes"}'Result: ✅ Notes
|
| } | ||
|
|
||
| // NXOSMethod represents a single AAA method in an NX-OS context. | ||
| type NXOSMethod struct { |
There was a problem hiding this comment.
Please reuse the AAAMethod type from the core/v1alpha1 package.
| if err := (&corecontroller.AAAReconciler{ | ||
| Client: mgr.GetClient(), | ||
| Scheme: mgr.GetScheme(), | ||
| Recorder: mgr.GetEventRecorderFor("aaa-controller"), |
There was a problem hiding this comment.
| Recorder: mgr.GetEventRecorderFor("aaa-controller"), | |
| Recorder: Recorder: mgr.GetEventRecorder("aaa-controller"), |
|
|
||
| // Recorder is used to record events for the controller. | ||
| // More info: https://book.kubebuilder.io/reference/raising-events | ||
| Recorder record.EventRecorder |
There was a problem hiding this comment.
| Recorder record.EventRecorder | |
| Recorder events.EventRecorder |
| // +kubebuilder:rbac:groups=networking.metal.ironcore.dev,resources=aaa,verbs=get;list;watch;create;update;patch;delete | ||
| // +kubebuilder:rbac:groups=networking.metal.ironcore.dev,resources=aaa/status,verbs=get;update;patch | ||
| // +kubebuilder:rbac:groups=networking.metal.ironcore.dev,resources=aaa/finalizers,verbs=update | ||
| // +kubebuilder:rbac:groups=core,resources=events,verbs=create;patch |
There was a problem hiding this comment.
| // +kubebuilder:rbac:groups=core,resources=events,verbs=create;patch | |
| // +kubebuilder:rbac:groups=events.k8s.io,resources=events,verbs=create;patch |
| // - https://ahmet.im/blog/controller-pitfalls/#reconcile-method-shape | ||
| func (r *AAAReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Result, reterr error) { | ||
| log := ctrl.LoggerFrom(ctx) | ||
| log.Info("Reconciling resource") |
There was a problem hiding this comment.
Please adapt this controller to the logging scheme introduced in 4e1b59c.
| if err := r.Locker.AcquireLock(ctx, device.Name, "aaa-controller"); err != nil { | ||
| if errors.Is(err, resourcelock.ErrLockAlreadyHeld) { | ||
| log.Info("Device is already locked, requeuing reconciliation") | ||
| return ctrl.Result{RequeueAfter: time.Second * 5}, nil |
There was a problem hiding this comment.
| return ctrl.Result{RequeueAfter: time.Second * 5}, nil | |
| return ctrl.Result{RequeueAfter: Jitter(time.Second), Priority: new(LockWaitPriorityDefault)}, nil |
see 10af9d7.
| for _, server := range group.Servers { | ||
| if err := p.client.Delete(ctx, &TacacsPlusProvider{Name: server.Address}); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we do a single delete call here with all the servers, instead of doing so many individual gnmi requests?
Also you shouldn't assume that the spec contains all servers, that were ever created. So this DeleteAAA func should also take care of removing any potential leftover from previous reconciles.
| Name: server.Address, | ||
| KeyEnc: MapKeyEncryption(cfg.Spec.KeyEncryption), | ||
| } | ||
| if server.TACACS != nil { |
There was a problem hiding this comment.
We still append the server to the updates later on despite server.TACACS == nil? This doesn't seem right.
Isn't it enforced that when the group is tacacs, all servers have to have that setting? Making this check here redundant?
Same also applies to the RADIUS servers.
There was a problem hiding this comment.
Please also add a test file, as done with the other configuration items including a golden cli script under the testdata/ directory, i.e. testdata/aaa.json.txt and the expected YANG json payload of that as testdata/aaa.json.
See how this is done on other items, e.g. the user.go/user_test.go/user.json/user.json.txt.
weneghawi
force-pushed
the
feature/aaa-tacacs-support
branch
from
28c67ae to
a0af584
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
|
4 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