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

Add schema for ACL: APP_ACL_TABLE and APP_ACL_RULE_TABLE (#325) #275

Merged
merged 3 commits into from
Jul 24, 2019

Conversation

shine4chen
Copy link
Contributor

Signed-off-by: shine.chen shine.chen@nephosinc.com

Signed-off-by: shine.chen <shine.chen@nephosinc.com>
common/schema.h Outdated
@@ -39,6 +39,8 @@ namespace swss {
#define APP_VXLAN_VRF_TABLE_NAME "VXLAN_VRF_TABLE"
#define APP_VXLAN_TUNNEL_MAP_TABLE_NAME "VXLAN_TUNNEL_MAP_TABLE"
#define APP_VXLAN_TUNNEL_TABLE_NAME "VXLAN_TUNNEL_TABLE"
#define APP_ACL_TABLE_NAME "APP_ACL_TABLE"
#define APP_ACL_RULE_TABLE_NAME "APP_ACL_RULE"
Copy link
Contributor

Choose a reason for hiding this comment

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

APP_ACL_RULE_TABLE

Copy link
Contributor

Choose a reason for hiding this comment

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

do we want the APP_ prefix?

Copy link
Contributor

Choose a reason for hiding this comment

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

change to

#define APP_ACL_TABLE_TABLE_NAME        "ACL_TABLE_TABLE"
#define APP_ACL_RULE_TABLE_NAME         "ACL_RULE_TABLE"

@prsunny
Copy link
Contributor

prsunny commented May 23, 2019

retest this please

1 similar comment
@lguohan
Copy link
Contributor

lguohan commented May 24, 2019

retest this please

Copy link
Contributor

@stcheng stcheng left a comment

Choose a reason for hiding this comment

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

please check the comment

@shine4chen
Copy link
Contributor Author

@stcheng Sure , will correct it soon.

Signed-off-by: shine.chen <shine.chen@nephosinc.com>
Copy link
Contributor

@stcheng stcheng left a comment

Choose a reason for hiding this comment

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

APP_ACL_TABLE_NAME shall be changed to APP_ACL_TABLE_TABLE_NAME
the first table is for ACL_TABLE the second table is for TABLE_NAME

@shine4chen shine4chen closed this Jun 10, 2019
@shine4chen shine4chen reopened this Jun 10, 2019
@shine4chen
Copy link
Contributor Author

APP_ACL_TABLE_NAME shall be changed to APP_ACL_TABLE_TABLE_NAME
the first table is for ACL_TABLE the second table is for TABLE_NAME
@stcheng If we follow the "CFG_ACL_TABLE_NAME" name criteria APP_ACL_TABLE_NAME is suitable.

@stcheng
Copy link
Contributor

stcheng commented Jun 13, 2019

I see. I think CFG_ACL_TABLE_NAME is not correct. I'll discuss internally and get back.

@lguohan
Copy link
Contributor

lguohan commented Jun 29, 2019

@stcheng, do you still have concern?

@stcheng
Copy link
Contributor

stcheng commented Jul 18, 2019

@shine4chen I have fixed the CFG_ACL_TABLE_NAME issue, could you update the APP_ACL_TABLE_NAME as well?

@stcheng
Copy link
Contributor

stcheng commented Jul 18, 2019

by the way, could you add the link to the design for adding these two tables?

@shine4chen
Copy link
Contributor Author

@stcheng This is the Appdb-acl-table definition. I will change APP_ACL_TABLE_NAME to APP_ACL_TABLE_TABLE_NAME and resolve the confilcts.

Signed-off-by: shine.chen <shine.chen@nephosinc.com>
@stcheng
Copy link
Contributor

stcheng commented Jul 23, 2019

may i ask which application is writing to these two tables? what's the different purposes for having both configuration ACL table and application ACL table?

@shine4chen
Copy link
Contributor Author

Currently MCLAG apps will use these two acl table to dynamically install/uninstall acl rule for traffic isolation. Generally the static configured acl rules user defines will be put on cfg table. The dynamic acl rules apps defines will be put on app table. @stcheng

@stcheng stcheng merged commit 1c68a71 into sonic-net:master Jul 24, 2019
@stcheng
Copy link
Contributor

stcheng commented Jul 24, 2019

Thanks for the response!

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

Successfully merging this pull request may close these issues.

5 participants