-
Notifications
You must be signed in to change notification settings - Fork 661
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CLI support for PAC #3265
base: master
Are you sure you want to change the base?
CLI support for PAC #3265
Conversation
@jeff-yin @ridahanif96 pls help review |
/azpw run Azure |
/AzurePipelines run Azure |
No pipelines are associated with this pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Please cover code coverage. Add more test cases to cover coverage 100%
vlan_ports_data = config_db.get_table('VLAN_MEMBER') | ||
for key in vlan_ports_data: | ||
if key[1] == port: | ||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now We also have notation of switchport (access, trunk), what will be behavior of this. Will it work for both ports? Please revisit this as well.
ctx.fail("Invalid ConfigDB. Error: {}".format(e)) | ||
|
||
|
||
def pac_is_port_any_vlan_member(config_db, port): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While a port is dot1X enabled, it should not be allowed to become a router-port. In other words, it should not be removed from all the vlans right. Is that taken care as part of seperate PR?
is_server = True | ||
else: | ||
val = int(status) | ||
if val not in range(1,65535): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why we have chose this upper and lower bound for range?
In most NOS i see the range as "60 to 7200"
1 --> would mean there would too frequent authentication request, this would cause load on radius server depending on the deployment/scale.
65535 --> conversely this range seems to high.
Also is there there a way to disable "reauth timer" ?
print(auth_type) | ||
print(interface_name) | ||
try: | ||
config_db.mod_entry('MAB_PORT_CONFIG_TABLE', (interface_name), {'mab_enable': True if status == 'enable' else False}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When dot1x/mab is enabled on an interface, do we need to check for static MAC if it exists on that port and error out? Alternatively once PAC is enabled on a port, we should restrict any static MAC add on that port.
What I did
The cli changes are implemented as per the HLD mentioned below.
sonic-net/SONiC#1655
How I did it
Implemented the below commands
Config commands
===============
Show commands
show authentication interface
show authentication clients
show mab interface
show dot1x
============
Clear commands
sonic-clear authentication sessions
How to verify it