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

[acl-loader] egress mirror action support and action ASIC support check #575

Merged
merged 14 commits into from
Oct 7, 2019

Conversation

stepanblyschak
Copy link
Contributor

@stepanblyschak stepanblyschak commented Jul 5, 2019

- What I did

  • support egress mirror action
  • support action check via state DB

- How I did it

- How to verify it

  • UT
  • Manual tests on DUT

- Previous command output (if the output of a command-line utility has changed)

- New command output (if the output of a command-line utility has changed)

-->

DEPENDS sonic-net/sonic-swss#963

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 some minor requests. thanks.

.gitignore Show resolved Hide resolved
acl_loader/main.py Outdated Show resolved Hide resolved
for action_key in dict(action_props):
key = "{}|{}".format(self.ACL_ACTIONS_CAPABILITY_FIELD, stage.upper())
if key not in capability:
del action_props[action_key]
Copy link
Contributor

Choose a reason for hiding this comment

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

why not raise immediately?

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 method validate_actions can validate several actions in one call. This is done to allow later check if some combination of actions in one rule is allowed

if self.is_table_control_plane(table_name):
return True

action_count = len(action_props)
Copy link
Contributor

Choose a reason for hiding this comment

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

right now you're validating the actions based on the number of the actions before and after the check. however, it is based on the assumption that the input action_props only contains actions. but this assumption is based on the order of the conversion in the convert_rule_to_db_schema, which seems weak to me.

will it be better to raise the exceptions immediately when the rules violate the capabilities of the switch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

convert_rule_to_db_schema performs deep_update of rule_props dict (action, l2, ip, etc. key/values). It does not depend on the order. Thus convert_action should return a dict with acl actions only, so safely to assume action_props has only actions

if val["type"] == AclLoader.ACL_TABLE_TYPE_CTRLPLANE:
services = natsorted(val["services"])
data.append([key, val["type"], services[0], val["policy_desc"]])
data.append([key, val["type"], services[0], val["policy_desc"], stage])
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 need to specify the ingress stage for control plane ACLs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

control plane ACLs are in INPUT chain. Does it make sense to mark them as ingress in this case?

doc/Command-Reference.md Outdated Show resolved Hide resolved
Stepan Blyschak added 2 commits August 27, 2019 09:26
@lguohan lguohan merged commit 5564d87 into sonic-net:master Oct 7, 2019
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.

4 participants