fix: address felix review comments on AAA

Author: weneghawi

api/cisco/nx/v1alpha1/aaaconfig_types.go

@@ -13,28 +13,23 @@ import (
 
 // AAAConfigSpec defines the desired state of AAAConfig
 type AAAConfigSpec struct {
-	// LoginErrorEnable enables login error messages (NX-OS specific).
-	// Maps to: aaa authentication login error-enable
+	// LoginErrorEnable enables login error messages.
 	// +optional
 	LoginErrorEnable bool `json:"loginErrorEnable,omitempty"`
 
 	// KeyEncryption specifies the default encryption type for TACACS+ keys.
-	// +kubebuilder:validation:Enum=Type6;Type7;Clear
 	// +kubebuilder:default=Type7
 	KeyEncryption TACACSKeyEncryption `json:"keyEncryption,omitempty"`
 
 	// RADIUSKeyEncryption specifies the default encryption type for RADIUS server keys.
-	// +kubebuilder:validation:Enum=Type6;Type7;Clear
 	// +kubebuilder:default=Type7
 	RADIUSKeyEncryption RADIUSKeyEncryption `json:"radiusKeyEncryption,omitempty"`
 
-	// ConsoleAuthentication defines NX-OS console-specific authentication methods.
-	// Maps to: aaa authentication login console <methods>
+	// ConsoleAuthentication defines console-specific authentication methods.
 	// +optional
 	ConsoleAuthentication *NXOSMethodList `json:"consoleAuthentication,omitempty"`
 
-	// ConfigCommandsAuthorization defines NX-OS config-commands authorization methods.
-	// Maps to: aaa authorization config-commands default <methods>
+	// ConfigCommandsAuthorization defines config-commands authorization methods.
 	// +optional
 	ConfigCommandsAuthorization *NXOSMethodList `json:"configCommandsAuthorization,omitempty"`
 }

api/core/v1alpha1/aaa_types.go

@@ -12,8 +12,7 @@ import (
 
 // AAASpec defines the desired state of AAA.
 //
-// It models the Authentication, Authorization, and Accounting (AAA) configuration on a network device,
-// aligned with the OpenConfig system/aaa YANG model.
+// It models the Authentication, Authorization, and Accounting (AAA) configuration on a network device.
 // +kubebuilder:validation:XValidation:rule="has(self.serverGroups) || has(self.authentication) || has(self.authorization) || has(self.accounting)",message="at least one of serverGroups, authentication, authorization, or accounting must be set"
 type AAASpec struct {
 	// DeviceName is the name of the Device this object belongs to. The Device object must exist in the same namespace.
@@ -28,25 +27,21 @@ type AAASpec struct {
 	ProviderConfigRef *TypedLocalObjectReference `json:"providerConfigRef,omitempty"`
 
 	// ServerGroups is the list of AAA server groups.
-	// OpenConfig: /system/aaa/server-groups/server-group
 	// +optional
 	// +listType=map
 	// +listMapKey=name
 	// +kubebuilder:validation:MaxItems=8
 	ServerGroups []AAAServerGroup `json:"serverGroups,omitempty"`
 
 	// Authentication defines the AAA authentication method list.
-	// OpenConfig: /system/aaa/authentication
 	// +optional
 	Authentication *AAAAuthentication `json:"authentication,omitempty"`
 
 	// Authorization defines the AAA authorization method list.
-	// OpenConfig: /system/aaa/authorization
 	// +optional
 	Authorization *AAAAuthorization `json:"authorization,omitempty"`
 
 	// Accounting defines the AAA accounting method list.
-	// OpenConfig: /system/aaa/accounting
 	// +optional
 	Accounting *AAAAccounting `json:"accounting,omitempty"`
 }
@@ -63,7 +58,6 @@ const (
 )
 
 // AAAServerGroup represents a named group of AAA servers.
-// OpenConfig: /system/aaa/server-groups/server-group[name]
 // +kubebuilder:validation:XValidation:rule="self.type != 'TACACS' || self.servers.all(s, has(s.tacacs))",message="servers in a TACACS group must have tacacs config"
 // +kubebuilder:validation:XValidation:rule="self.type != 'RADIUS' || self.servers.all(s, has(s.radius))",message="servers in a RADIUS group must have radius config"
 type AAAServerGroup struct {
@@ -78,7 +72,6 @@ type AAAServerGroup struct {
 	Type AAAServerGroupType `json:"type"`
 
 	// Servers is the list of servers in this group.
-	// OpenConfig: /system/aaa/server-groups/server-group/servers/server
 	// +required
 	// +listType=map
 	// +listMapKey=address
@@ -98,7 +91,6 @@ type AAAServerGroup struct {
 }
 
 // AAAServer represents a single AAA server within a group.
-// OpenConfig: /system/aaa/server-groups/server-group/servers/server[address]
 type AAAServer struct {
 	// Address is the IP address or hostname of the server.
 	// +required
@@ -114,13 +106,11 @@ type AAAServer struct {
 
 	// TACACS contains TACACS+ specific server configuration.
 	// Required when the parent server group type is TACACS.
-	// OpenConfig augmentation: /system/aaa/server-groups/server-group/servers/server/tacacs
 	// +optional
 	TACACS *AAAServerTACACS `json:"tacacs,omitempty"`
 
 	// RADIUS contains RADIUS specific server configuration.
 	// Required when the parent server group type is RADIUS.
-	// OpenConfig augmentation: /system/aaa/server-groups/server-group/servers/server/radius
 	// +optional
 	RADIUS *AAAServerRADIUS `json:"radius,omitempty"`
 }
@@ -166,7 +156,6 @@ type AAAServerRADIUS struct {
 }
 
 // AAAAuthentication defines the AAA authentication method list.
-// OpenConfig: /system/aaa/authentication
 type AAAAuthentication struct {
 	// Methods is the ordered list of authentication methods.
 	// Methods are tried in order until one succeeds or all fail.
@@ -178,7 +167,6 @@ type AAAAuthentication struct {
 }
 
 // AAAAuthorization defines the AAA authorization method list.
-// OpenConfig: /system/aaa/authorization
 type AAAAuthorization struct {
 	// Methods is the ordered list of authorization methods.
 	// Methods are tried in order until one succeeds or all fail.
@@ -190,7 +178,6 @@ type AAAAuthorization struct {
 }
 
 // AAAAccounting defines the AAA accounting method list.
-// OpenConfig: /system/aaa/accounting
 type AAAAccounting struct {
 	// Methods is the ordered list of accounting methods.
 	// Methods are tried in order until one succeeds or all fail.

internal/provider/cisco/nxos/aaa.go

@@ -268,8 +268,8 @@ func MapFallbackFromMethodList(methods []v1alpha1.AAAMethod) string {
 	return AAAValueNo
 }
 
-// MapNXOSRealm maps an NX-OS method type string to NX-OS realm.
-func MapNXOSRealm(methodType string) string {
+// MapRealm maps a method type string to an NX-OS realm.
+func MapRealm(methodType string) string {
 	switch methodType {
 	case "Group":
 		return AAARealmTacacs
@@ -282,8 +282,8 @@ func MapNXOSRealm(methodType string) string {
 	}
 }
 
-// MapNXOSLocal checks if local is in an NX-OS method list.
-func MapNXOSLocal(methods []nxv1alpha1.NXOSMethod) string {
+// MapLocal checks if local is in a method list.
+func MapLocal(methods []nxv1alpha1.NXOSMethod) string {
 	for _, m := range methods {
 		if m.Type == "Local" {
 			return AAAValueYes
@@ -292,8 +292,8 @@ func MapNXOSLocal(methods []nxv1alpha1.NXOSMethod) string {
 	return AAAValueNo
 }
 
-// MapNXOSFallback determines fallback setting from an NX-OS method list.
-func MapNXOSFallback(methods []nxv1alpha1.NXOSMethod) string {
+// MapFallback determines fallback setting from a method list.
+func MapFallback(methods []nxv1alpha1.NXOSMethod) string {
 	if len(methods) > 1 {
 		return AAAValueYes
 	}

internal/provider/cisco/nxos/provider.go

@@ -2941,15 +2941,12 @@ func (p *Provider) EnsureAAA(ctx context.Context, req *provider.EnsureAAARequest
 		}
 	}
 
-	// Process server groups
 	for _, group := range req.AAA.Spec.ServerGroups {
 		switch group.Type {
 		case v1alpha1.AAAServerGroupTypeTACACS:
-			// Enable TACACS+ feature
 			tacacsFeature := TACACSFeatureEnabled
 			conf = append(conf, &tacacsFeature)
 
-			// Configure individual TACACS+ server hosts
 			for _, server := range group.Servers {
 				srv := &TacacsPlusProvider{
 					Name:   server.Address,
@@ -2967,7 +2964,6 @@ func (p *Provider) EnsureAAA(ctx context.Context, req *provider.EnsureAAARequest
 				conf = append(conf, srv)
 			}
 
-			// Configure the TACACS+ server group
 			grp := &TacacsPlusProviderGroup{
 				Name:  group.Name,
 				Vrf:   group.VrfName,
@@ -2979,7 +2975,6 @@ func (p *Provider) EnsureAAA(ctx context.Context, req *provider.EnsureAAARequest
 			conf = append(conf, grp)
 
 		case v1alpha1.AAAServerGroupTypeRADIUS:
-			// Configure individual RADIUS server hosts
 			for _, server := range group.Servers {
 				srv := &RadiusProvider{
 					Name:   server.Address,
@@ -2998,7 +2993,6 @@ func (p *Provider) EnsureAAA(ctx context.Context, req *provider.EnsureAAARequest
 				conf = append(conf, srv)
 			}
 
-			// Configure the RADIUS server group
 			grp := &RadiusProviderGroup{
 				Name:  group.Name,
 				Vrf:   group.VrfName,
@@ -3011,7 +3005,6 @@ func (p *Provider) EnsureAAA(ctx context.Context, req *provider.EnsureAAARequest
 		}
 	}
 
-	// Configure AAA default authentication (from core API flat method list)
 	if req.AAA.Spec.Authentication != nil && len(req.AAA.Spec.Authentication.Methods) > 0 {
 		methods := req.AAA.Spec.Authentication.Methods
 		authen := &AAADefaultAuth{
@@ -3028,24 +3021,22 @@ func (p *Provider) EnsureAAA(ctx context.Context, req *provider.EnsureAAARequest
 		conf = append(conf, authen)
 	}
 
-	// Configure AAA console authentication (from Cisco AAAConfig)
 	if cfg.Spec.ConsoleAuthentication != nil && len(cfg.Spec.ConsoleAuthentication.Methods) > 0 {
 		methods := cfg.Spec.ConsoleAuthentication.Methods
 		consoleAuth := &AAAConsoleAuth{
 			ErrEn:    cfg.Spec.LoginErrorEnable,
-			Fallback: MapNXOSFallback(methods),
-			Local:    MapNXOSLocal(methods),
+			Fallback: MapFallback(methods),
+			Local:    MapLocal(methods),
 		}
 		if methods[0].Type == "Group" {
 			consoleAuth.Realm = MapRealmFromGroup(methods[0].GroupName, req.AAA.Spec.ServerGroups)
 			consoleAuth.ProviderGroup = methods[0].GroupName
 		} else {
-			consoleAuth.Realm = MapNXOSRealm(methods[0].Type)
+			consoleAuth.Realm = MapRealm(methods[0].Type)
 		}
 		conf = append(conf, consoleAuth)
 	}
 
-	// Configure AAA authorization (from core API flat method list)
 	if req.AAA.Spec.Authorization != nil && len(req.AAA.Spec.Authorization.Methods) > 0 {
 		methods := req.AAA.Spec.Authorization.Methods
 		author := &AAADefaultAuthor{
@@ -3058,20 +3049,18 @@ func (p *Provider) EnsureAAA(ctx context.Context, req *provider.EnsureAAARequest
 		conf = append(conf, author)
 	}
 
-	// Configure AAA config-commands authorization (from Cisco AAAConfig)
 	if cfg.Spec.ConfigCommandsAuthorization != nil && len(cfg.Spec.ConfigCommandsAuthorization.Methods) > 0 {
 		methods := cfg.Spec.ConfigCommandsAuthorization.Methods
 		author := &AAADefaultAuthor{
 			CmdType:   "config",
-			LocalRbac: MapNXOSLocal(methods) == AAAValueYes,
+			LocalRbac: MapLocal(methods) == AAAValueYes,
 		}
 		if methods[0].Type == "Group" {
 			author.ProviderGroup = methods[0].GroupName
 		}
 		conf = append(conf, author)
 	}
 
-	// Configure AAA accounting (from core API flat method list)
 	if req.AAA.Spec.Accounting != nil && len(req.AAA.Spec.Accounting.Methods) > 0 {
 		methods := req.AAA.Spec.Accounting.Methods
 		acct := &AAADefaultAcc{
@@ -3091,97 +3080,43 @@ func (p *Provider) EnsureAAA(ctx context.Context, req *provider.EnsureAAARequest
 }
 
 func (p *Provider) DeleteAAA(ctx context.Context, req *provider.DeleteAAARequest) error {
-	var conf []gnmiext.Configurable
-
-	// Read Cisco-specific config from ProviderConfig
-	var cfg nxv1alpha1.AAAConfig
-	if req.ProviderConfig != nil {
-		if err := req.ProviderConfig.Into(&cfg); err != nil {
-			return err
-		}
-	}
-
-	// Reset AAA accounting to local
-	if req.AAA.Spec.Accounting != nil {
-		conf = append(conf, &AAADefaultAcc{
-			Name:      "Accounting",
-			Realm:     AAARealmLocal,
-			LocalRbac: true,
-		})
-	}
-
-	// Reset AAA authorization to local
-	if req.AAA.Spec.Authorization != nil || cfg.Spec.ConfigCommandsAuthorization != nil {
-		conf = append(conf, &AAADefaultAuthor{
-			CmdType:       "config",
-			ProviderGroup: "",
-			LocalRbac:     true,
-		})
+	// Reset all AAA method config to device defaults unconditionally.
+	// gNMI deletes are idempotent, so it is safe to reset even if a field
+	// was never configured.
+	conf := []gnmiext.Configurable{
+		&AAADefaultAcc{Name: "Accounting", Realm: AAARealmLocal, LocalRbac: true},
+		&AAADefaultAuthor{CmdType: "config", LocalRbac: true},
+		&AAADefaultAuth{Realm: AAARealmLocal, Local: AAAValueYes, Fallback: AAAValueYes},
+		&AAAConsoleAuth{Realm: AAARealmLocal, Local: AAAValueYes, Fallback: AAAValueYes},
 	}
 
-	// Reset AAA authentication to local
-	if req.AAA.Spec.Authentication != nil {
-		conf = append(conf, &AAADefaultAuth{
-			Realm:    AAARealmLocal,
-			Local:    AAAValueYes,
-			Fallback: AAAValueYes,
-			ErrEn:    false,
-		})
-	}
-
-	// Reset console authentication to local
-	if cfg.Spec.ConsoleAuthentication != nil {
-		conf = append(conf, &AAAConsoleAuth{
-			Realm:    AAARealmLocal,
-			Local:    AAAValueYes,
-			Fallback: AAAValueYes,
-			ErrEn:    false,
-		})
-	}
-
-	// Delete server groups and hosts
-	hasTACACS := false
 	for _, group := range req.AAA.Spec.ServerGroups {
 		switch group.Type {
 		case v1alpha1.AAAServerGroupTypeTACACS:
-			hasTACACS = true
-
-			grp := &TacacsPlusProviderGroup{Name: group.Name}
-			if err := p.client.Delete(ctx, grp); err != nil {
+			if err := p.client.Delete(ctx, &TacacsPlusProviderGroup{Name: group.Name}); err != nil {
 				return err
 			}
 			for _, server := range group.Servers {
-				srv := &TacacsPlusProvider{Name: server.Address}
-				if err := p.client.Delete(ctx, srv); err != nil {
+				if err := p.client.Delete(ctx, &TacacsPlusProvider{Name: server.Address}); err != nil {
 					return err
 				}
 			}
+			tacacsFeature := TACACSFeatureDisabled
+			conf = append(conf, &tacacsFeature)
 
 		case v1alpha1.AAAServerGroupTypeRADIUS:
-			grp := &RadiusProviderGroup{Name: group.Name}
-			if err := p.client.Delete(ctx, grp); err != nil {
+			if err := p.client.Delete(ctx, &RadiusProviderGroup{Name: group.Name}); err != nil {
 				return err
 			}
 			for _, server := range group.Servers {
-				srv := &RadiusProvider{Name: server.Address}
-				if err := p.client.Delete(ctx, srv); err != nil {
+				if err := p.client.Delete(ctx, &RadiusProvider{Name: server.Address}); err != nil {
 					return err
 				}
 			}
 		}
 	}
 
-	// Disable TACACS+ feature
-	if hasTACACS {
-		tacacsFeature := TACACSFeatureDisabled
-		conf = append(conf, &tacacsFeature)
-	}
-
-	if len(conf) > 0 {
-		return p.Update(ctx, conf...)
-	}
-
-	return nil
+	return p.Update(ctx, conf...)
 }
 
 func init() {