Skip to content
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

Vlanmgr and Fdborch changes for PAC #3143

Open
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

anilkpan
Copy link
Contributor

Added Vlanmgr and Fdborch changes to support PAC.
The following PAC operations are supported to be passed down from Vlanmgr to OA for updating the ASIC DB:

  1. Port learning mode
  2. FDB entry
  3. VLAN membership

PAC HLD: https://github.com/sonic-net/SONiC/blob/master/doc/pac/Port%20Access%20Control.md

Added Vlanmgr and Fdborch changes to support  PAC.
@anilkpan anilkpan requested a review from prsunny as a code owner May 10, 2024 22:04
@anilkpan
Copy link
Contributor Author

The following PR needs to be merged to resolve the compilation error:
sonic-net/sonic-swss-common#871

@@ -1410,7 +1416,9 @@ bool FdbOrch::addFdbEntry(const FdbEntry& entry, const string& port_name,
attrs.push_back(attr);
}
}

attr.id = SAI_FDB_ENTRY_ATTR_PACKET_ACTION;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be changes in fdborch to push the packet to PACD socket? https://github.com/sonic-net/SONiC/blob/master/doc/pac/Port%20Access%20Control.md#313-mab-packet-receive-flow. How are the packets delivered for authentication when MAB is enabled on the interface please?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These packets that come to CPU are received by PACD as it registers socket for the same. If MAB is enabled, then PACD informs , the information that is required for authentication, such as mac address and the interface to MAB process. The communication between the PACD and MABD uses simple client-server socket communication.

@adyeung
Copy link

adyeung commented Oct 1, 2024

@sutharsansr pls review and sign off if you have no further comments

cfgmgr/vlanmgr.cpp Show resolved Hide resolved
cfgmgr/vlanmgr.cpp Show resolved Hide resolved
@anilkpan
Copy link
Contributor Author

/azpw run Azure.sonic-swss

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Collaborator

@prsunny prsunny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tests and fix code coverage

@@ -30,6 +30,20 @@ Orch::Orch(DBConnector *db, const vector<string> &tableNames)
}
}

Orch::Orch(swss::DBConnector *db1, swss::DBConnector *db2,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this change? Seems not related to feature. Please remove

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is needed, as Vlanmgr needs to process updates from 2 db's now, config db and state db.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain which tables. and where is it added in Vlanmgr code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, Vlanmgr processes updates only from the config db. PAC sends updates to Vlanmgr through the following tables in the state db:
#define STATE_OPER_VLAN_TABLE_NAME "OPER_VLAN"
#define STATE_OPER_VLAN_MEMBER_TABLE_NAME "OPER_VLAN_MEMBER"
#define STATE_OPER_FDB_TABLE_NAME "OPER_FDB"
#define STATE_OPER_PORT_TABLE_NAME "OPER_PORT"

To process these updates, Vlanmgr needs to add the state db tables as well to the Orch list:

VlanMgr::VlanMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, const vector &tableNames,
const vector &stateTableNames) :
Orch(cfgDb, stateDb, tableNames, stateTableNames),

So, in addition to cfgDb, we also add stateDb in the Orch list. The PAC related tables are in stateTableNames

@anilkpan
Copy link
Contributor Author

/azpw run Azure.sonic-swss

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@anilkpan
Copy link
Contributor Author

/azpw run Azure.sonic-swss

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants