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.

Sign up for GitHub

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

Jump to bottom

[ACL] Add Tunnel Next Hop redirect support #3399

Merged
merged 9 commits into from
Jan 22, 2025
Merged

[ACL] Add Tunnel Next Hop redirect support #3399

merged 9 commits into from
Jan 22, 2025
+473 −33

Conversation

vivekrnv
Copy link
Contributor

@vivekrnv vivekrnv commented Nov 27, 2024 •
edited
Loading

What I did

  • Add support for Tunnel Next Hop as the Redirect Action in ACL Rules
  • Supported schema for REDIRECT_ACTION field : <ip>@<tunnel_name> (Eg: 20.0.0.3@tunnel0)
  • Add a new Macro in the SAI mocking infra to clear extern mock_fns set
  • Add UT's

Why I did it

How I verified it

[----------] 4 tests from AclRedirectActionTest
[ RUN      ] AclRedirectActionTest.TunnelNH
[       OK ] AclRedirectActionTest.TunnelNH (10 ms)
[ RUN      ] AclRedirectActionTest.TunnelNH_ExistingNhObject
[       OK ] AclRedirectActionTest.TunnelNH_ExistingNhObject (10 ms)
[ RUN      ] AclRedirectActionTest.TunnelNH_InvalidTunnel
[       OK ] AclRedirectActionTest.TunnelNH_InvalidTunnel (10 ms)
[ RUN      ] AclRedirectActionTest.TunnelNH_InvalidNextHop
[       OK ] AclRedirectActionTest.TunnelNH_InvalidNextHop (10 ms)
[----------] 4 tests from AclRedirectActionTest (40 ms total)

Details if related

@vivekrnv
Copy link
Contributor Author

vivekrnv commented Nov 28, 2024 •
edited
Loading 

[----------] 15 tests from MuxRollbackTest
[ RUN      ] MuxRollbackTest.StandbyToActiveNeighborAlreadyExists
[       OK ] MuxRollbackTest.StandbyToActiveNeighborAlreadyExists (86 ms)
[ RUN      ] MuxRollbackTest.ActiveToStandbyNeighborNotFound
pure virtual method called
terminate called without an active exception
FAIL tests (exit status: 134)

Unrelated UT failure. Not seen on local machine @lolyu Are you familiar on why this might fail?

@vivekrnv
Copy link
Contributor Author 

[----------] 15 tests from MuxRollbackTest
[ RUN      ] MuxRollbackTest.StandbyToActiveNeighborAlreadyExists
[       OK ] MuxRollbackTest.StandbyToActiveNeighborAlreadyExists (86 ms)
[ RUN      ] MuxRollbackTest.ActiveToStandbyNeighborNotFound
pure virtual method called
terminate called without an active exception
FAIL tests (exit status: 134)

Unrelated UT failure. Not seen on local machine @lolyu Are you familiar on why this might fail?

NVM, interestingly only seen on bullseye environment. Fixed the problem

@vivekrnv
Fix armhf build failure
03c7041 
Signed-off-by: Vivek Reddy <[email protected]>
@vivekrnv vivekrnv marked this pull request as ready for review November 29, 2024 00:34
@vivekrnv vivekrnv requested a review from prsunny as a code owner November 29, 2024 00:34
@vivekrnv vivekrnv mentioned this pull request Nov 29, 2024
Merged
@prsunny
Copy link
Collaborator

prsunny commented Dec 3, 2024

@bingwang-ms for viz

@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vivekrnv
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

prsunny
prsunny reviewed Jan 7, 2025
View reviewed changes
orchagent/aclorch.cpp
return SAI_NULL_OBJECT_ID;
m_redirect_target_next_hop = target;
m_pAclOrch->m_neighOrch->increaseNextHopRefCount(nh);
return m_pAclOrch->m_neighOrch->getNextHopId(nh);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like logic is changed. Previously it returned NULL OBJECT if hasnexthop failed. Should we keep the old behaviour?

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 field can take many variants, it can be Port, Lag, NH IP, NHG and now Tunnel NH.

Previous logic is incorrect as if get NH fails, it should try to check if it is one of other variants instead of returning SAI_NULL_OBJECT_ID

@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

prsunny
prsunny reviewed Jan 16, 2025
View reviewed changes
orchagent/aclorch.cpp
void AclRule::TunnelNH::parse(const std::string& target)
{
/* Supported Format: endpoint_ip@tunnel_name */
auto at_pos = target.find('@');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add this schema in the PR description and how it would look like with an example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

prsunny
prsunny approved these changes Jan 22, 2025
View reviewed changes
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.

lgtm, please plan to add sonic-mgmt test for this feature

@prsunny prsunny merged commit ae5a50b into sonic-net:master Jan 22, 2025
15 checks passed
@vivekrnv vivekrnv deleted the tunn_nh branch January 22, 2025 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleksandrivantsiv oleksandrivantsiv oleksandrivantsiv approved these changes

@prsunny prsunny prsunny approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vivekrnv @prsunny @mssonicbld @oleksandrivantsiv