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

[aclorch] add generic AclOrch::updateAclRule() method #1993

Merged
merged 19 commits into from
Nov 22, 2021

Conversation

stepanblyschak
Copy link
Contributor

@stepanblyschak stepanblyschak commented Oct 28, 2021

What I did

I added generic AclOrch::updateAclRule() implementation. This implementation takes a newly constructed shared_ptr<AclRule> provided by the user, calculates a difference between priority, matches and actions and calls sai_acl_api->set_acl_entry_attribute() for those attributes that were changed. The derivatives of AclRule can customize the behaviour of AclRule::update(const AclRule& updatedRule) by polymorphic override, although in that case the derivative needs dynamic_cast the updatedRule to a concrete derivative type.
Added unit test to cover update scenario. Currently this new API is not used by any clients nor handling ACL_RULE table updates. This API will be used by PBH ACL rule update flow.

Why I did it

To support the scenario for updating PBH ACL rule in the future.

How I verified it

Temporary changed the ACL_RULE table update to leverage this new API and tested updates through CONFIG_DB and unit test coverage.

Details if related

Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
@prsunny
Copy link
Collaborator

prsunny commented Oct 28, 2021

@stepanblyschak, the changes looks to be significant. Is there any document covering this new change? Request @bingwang-ms and @anish-n to sign-off

@stepanblyschak
Copy link
Contributor Author

@bingwang-ms
Copy link
Contributor

Reviewing

@stepanblyschak
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
…date

Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
@bingwang-ms bingwang-ms self-requested a review November 12, 2021 08:18
@bingwang-ms
Copy link
Contributor

Please ping me when the PR is ready for review. Thanks

@stepanblyschak
Copy link
Contributor Author

@bingwang-ms This PR is ready.

orchagent/aclorch.cpp Show resolved Hide resolved
orchagent/aclorch.cpp Outdated Show resolved Hide resolved
orchagent/aclorch.cpp Show resolved Hide resolved
Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
@bingwang-ms
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stepanblyschak
Copy link
Contributor Author

There is some issue with LGTM:

[2021-11-18 13:03:05] [build-stdout] g++ -DHAVE_CONFIG_H -I. -I.. -I .. -g -DNDEBUG  -ansi -fPIC -std=c++11 -Wall -Wcast-align -Wcast-qual -Wconversion -Wdisabled-optimization -Werror -Wextra -Wfloat-equal -Wformat=2 -Wformat-nonliteral -Wformat-security -Wformat-y2k -Wimport -Winit-self -Winvalid-pch -Wlong-long -Wmissing-field-initializers -Wmissing-format-attribute -Wmissing-include-dirs -Wmissing-noreturn -Wno-aggregate-return -Wno-padded -Wno-switch-enum -Wno-unused-parameter -Wpacked -Wpointer-arith -Wredundant-decls -Wshadow -Wstack-protector -Wstrict-aliasing=3 -Wswitch -Wswitch-default -Wunreachable-code -Wunused -Wvariadic-macros -Wno-write-strings -Wno-missing-format-attribute -Wno-long-long  -I/usr/include/libnl3  -Wdate-time -D_FORTIFY_SOURCE=2  -g -O2 -fdebug-prefix-map=/opt/src/sonic-swss-common=. -fstack-protector-strong -Wformat -Werror=format-security -c -o tests-saiaclschema_ut.o `test -f 'saiaclschema_ut.cpp' || echo './'`saiaclschema_ut.cpp
[2021-11-18 13:03:05] [build-stderr] saiaclschema_ut.cpp:1:10: fatal error: gmock/gmock.h: No such file or directory
[2021-11-18 13:03:05] [build-stderr]     1 | #include <gmock/gmock.h>
[2021-11-18 13:03:05] [build-stderr]       |          ^~~~~~~~~~~~~~~
[2021-11-18 13:03:05] [build-stderr] compilation terminated.

@stepanblyschak
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stepanblyschak
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stepanblyschak
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liat-grozovik liat-grozovik merged commit da21172 into sonic-net:master Nov 22, 2021
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.

5 participants