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

[caclmgrd] Add check for valid ethertype in IPv4/v6 rules #6193

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

Conversation

anilkpan
Copy link

@anilkpan anilkpan commented Dec 11, 2020

Added check for valid ethertype in IPv4/v6 rule.

- Why I did it
Added check for valid ethertype in IPv4/v6 rule.

- How I did it

- How to verify it

- Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006

- Description for the changelog
[caclmgrd] Add check for valid ethertype in IPv4/v6 rules

This was done to support stop all SSH sessions for IPv4 and IPv6 only. One option was to use source IP as 0.0.0.0/0 or ::/0 but now we just need to use Ether type or IP type instead of 0 for SIP and DIP.

  • Added a validation for IPv4/6 ethertype to restrict the IPv4/6 rules to only these ethertypes.

- A picture of a cute animal (not mandatory but encouraged)

Added check for valid ethertype in IPv4/v6 rule.
@jleveque
Copy link
Contributor

jleveque commented Dec 11, 2020

@anilkpan: Your description should be the PR title (e.g., [caclmgrd] Add check for valid ethertype in IPv4/v6 rules). Can you please update that and then add more information to the description as to what you changed and why?

@ben-gale
Copy link
Collaborator

@anilkpan , @jleveque - can we get this one moving please? Time is of the essence! Firstly, Anil pls update the PR title as Joe suggests, and provide the extra info requested. In the meantime, I would hope that there is sufficient information (and the PR is small enough) that Joe can proceed with the review anyway?

@jleveque
Copy link
Contributor

In the meantime, I would hope that there is sufficient information (and the PR is small enough) that Joe can proceed with the review anyway?

It's difficult to review the code when I don't fully understand why the change was necessary and exactly what it is intended to do.

@anilkpan
Copy link
Author

@jleveque @ben-gale This was done to support stop all SSH sessions for IPv4 and IPv6 only. One option was to use source IP as 0.0.0.0/0 or ::/0 but now we just need to use Ether type or IP type instead of 0 for SIP and DIP.

@jleveque
Copy link
Contributor

@anilkpan: Can you please update the PR title and description?

Copy link
Contributor

@jleveque jleveque left a comment

Choose a reason for hiding this comment

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

As comments. Also, please update the PR title to be more descriptive.

@jleveque jleveque changed the title Update caclmgrd [caclmgrd] Add check for valid ethertype in IPv4/v6 rules Dec 17, 2020
@jleveque
Copy link
Contributor

Retest broadcom please

@jleveque
Copy link
Contributor

Retest vsimage please

@lguohan
Copy link
Collaborator

lguohan commented Dec 20, 2020

i think this change breaks the sonic-mgmt cacl test, please check see if we need to fix the test, and if this behavior is back-compatibile.

@anilkpan
Copy link
Author

retest this please

1 similar comment
@anilkpan
Copy link
Author

retest this please

@anilkpan anilkpan requested a review from jleveque December 22, 2020 18:53
@jleveque
Copy link
Contributor

Retest vsimage please

@anilkpan
Copy link
Author

retest this please

@jleveque
Copy link
Contributor

Retest vsimage please

@anilkpan
Copy link
Author

retest vsimage please

@anilkpan
Copy link
Author

retest vsimage please

@anilkpan
Copy link
Author

anilkpan commented Jan 6, 2021

@jleveque @ben-gale The vsimage test is failing even without my changes in this PR.

@ben-gale
Copy link
Collaborator

ben-gale commented Jan 6, 2021

@jleveque @ben-gale The vsimage test is failing even without my changes in this PR.

Yep - I see this same failure in many other PRs in the repo - @jleveque, @lguohan - who can debug/fix this please?

@lguohan
Copy link
Collaborator

lguohan commented Jan 6, 2021

@ben-gale , we have created an issue to track this. sonic-net/sonic-mgmt#2743

@ben-gale
Copy link
Collaborator

retest vsimage please

2 similar comments
@ben-gale
Copy link
Collaborator

retest vsimage please

@ben-gale
Copy link
Collaborator

retest vsimage please

@lguohan
Copy link
Collaborator

lguohan commented Feb 3, 2021

/Azurepipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@anilkpan anilkpan requested a review from lguohan as a code owner February 6, 2021 20:29
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