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

Validate input of config mirror_session add #1825

Merged
merged 3 commits into from
Oct 14, 2021
Merged

Validate input of config mirror_session add #1825

merged 3 commits into from
Oct 14, 2021

Conversation

bingwang-ms
Copy link
Contributor

What I did

This PR fixed sonic-net/sonic-buildimage#7990

Type and value validation is added for CLI config mirror_session add.
The value range is defined below

Value Range
src_ip Invalid IPv4 address
dst_ip Invalid IPv4 address
DSCP [0, 63]
TTL [0, 255]
Queue [0, 255]
GRE_TYPE [0, 65535]

How I did it

Add a value validation at CLI level.

How to verify it

Verified by UT

tests/config_mirror_session_test.py::test_mirror_session_add PASSED                                                                                                                                                                                      [ 50%]
tests/config_mirror_session_test.py::test_mirror_session_erspan_add PASSED                                                                                                                                                                               [100%]

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)

@bingwang-ms
Validate input of add mirror session
9c54714 
Signed-off-by: bingwang <bingwang@microsoft.com>
@bingwang-ms
Add UT for mirror_session
74b3fd6 
Signed-off-by: bingwang <bingwang@microsoft.com>
@bingwang-ms bingwang-ms mentioned this pull request Sep 17, 2021
Closed
@bingwang-ms
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

raphaelt-nvidia
raphaelt-nvidia suggested changes Sep 19, 2021
View reviewed changes
Copy link
Contributor

@raphaelt-nvidia raphaelt-nvidia left a comment

Choose a reason for hiding this comment

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

Since the queue parameter is also used in span, I suggest adding a test for it with the config mirror-session span add command.

@bingwang-ms
Add span test case
f4b5b18 
Signed-off-by: bingwang <wang.bing@microsoft.com>
@bingwang-ms
Copy link
Contributor Author

Since the queue parameter is also used in span, I suggest adding a test for it with the config mirror-session span add command.

Thanks. Updated

@bingwang-ms
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bingwang-ms
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bingwang-ms
Copy link
Contributor Author

@raphaelt-nvidia Hi, could you help to take a look again? Thanks

@raphaelt-nvidia
Copy link
Contributor

Question about how the test works: If the test runs only in the mock environment, then LGTM. If it calls real SAI, then whether trying to configure queue=100 produces errors in the log is platform-dependent, and how it is dependent should change soon when I submit a PR to check the actual maximum queue value via SAI.

@bingwang-ms
Copy link
Contributor Author

Question about how the test works: If the test runs only in the mock environment, then LGTM. If it calls real SAI, then whether trying to configure queue=100 produces errors in the log is platform-dependent, and how it is dependent should change soon when I submit a PR to check the actual maximum queue value via SAI.

The UT is only running in mocked environment, and there is no orchagent and no syncd, and so there is no SAI. The verification is on CLI level.

@raphaelt-nvidia
Copy link
Contributor

SO LGTM. Should someone else review?

prsunny
prsunny reviewed Oct 12, 2021
View reviewed changes
config/main.py
@@ -953,6 +958,19 @@ def cache_arp_entries():
open(restore_flag_file, 'w').close()
return success


def validate_ipv4_address(ctx, param, ip_addr):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please move this to common so it can be used by other files in future? -

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Impmenting a common function to validate IP address is a good suggestion, but this interface is not to be common because this one is implemented as a callback interface of click. I will implement that common interface in another PR.

@bingwang-ms bingwang-ms merged commit c950a55 into sonic-net:master Oct 14, 2021
qiluo-msft pushed a commit that referenced this pull request Oct 15, 2021
@bingwang-ms @qiluo-msft
Validate input of config mirror_session add (#1825)
ae5d90c 
* Validate input of add mirror session

Signed-off-by: bingwang <bingwang@microsoft.com>
@raphaelt-nvidia
Copy link
Contributor

Our QA has noticed that the new range checking for gre requires the value to be entered in decimal. The command reference https://github.com/Azure/sonic-utilities/blob/master/doc/Command-Reference.md#mirroring-config-commands doesn't define it explicitly, but the following text suggests that hex format should at least be allowed, if not preferred:

optional - GRE Type in case if user wants to send the packet via GRE tunnel. GRE type could be anything; it could also be left as empty; by default, it is 0x8949 for Mellanox; and 0x88be for the rest of the chips.

@bingwang-ms , what do you think?

@raphaelt-nvidia raphaelt-nvidia mentioned this pull request Feb 27, 2022
Closed
@bingwang-ms bingwang-ms mentioned this pull request Mar 3, 2022
Merged
6 tasks
qiluo-msft pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Mar 19, 2022
@bingwang-ms
[yang] Update YANG model for mirror session to support decimal value …
fb7f046 
…for GRE type (#10140)

#### Why I did it
PR  sonic-net/sonic-utilities#1825 added validation for the input of `config mirror session add`, and only decimal value is accepted.
An issue #10096 was raised to suggest accepting HEX value as well, and the suggestion makes sense to me.

To accept HEX value for GRE type, and keep backward compatibility as well, I updated the YANG model to support both decimal and hexadecimal input for GRE type.

#### How I did it
Update the regex for GRE type.

#### How to verify it
Verified by UT
```
platform linux -- Python 3.9.2, pytest-6.0.2, py-1.10.0, pluggy-0.13.0
rootdir: /sonic/src/sonic-yang-models
plugins: pyfakefs-4.5.4, cov-2.10.1
collected 3 items                                                                                                                                                                                     

tests/test_sonic_yang_models.py ..                                                                                                                                                              [ 66%]
tests/yang_model_tests/test_yang_model.py .                                                                                                                                                     [100%]

========================================================================================== 3 passed in 2.53s ==========================================================================================
```

#### Description for the changelog
Update YANG model for mirror session to support decimal value for GRE type.
judyjoseph pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Mar 20, 2022
@bingwang-ms @judyjoseph
[yang] Update YANG model for mirror session to support decimal value …
ef9df9e 
…for GRE type (#10140)

#### Why I did it
PR  sonic-net/sonic-utilities#1825 added validation for the input of `config mirror session add`, and only decimal value is accepted.
An issue #10096 was raised to suggest accepting HEX value as well, and the suggestion makes sense to me.

To accept HEX value for GRE type, and keep backward compatibility as well, I updated the YANG model to support both decimal and hexadecimal input for GRE type.

#### How I did it
Update the regex for GRE type.

#### How to verify it
Verified by UT
```
platform linux -- Python 3.9.2, pytest-6.0.2, py-1.10.0, pluggy-0.13.0
rootdir: /sonic/src/sonic-yang-models
plugins: pyfakefs-4.5.4, cov-2.10.1
collected 3 items                                                                                                                                                                                     

tests/test_sonic_yang_models.py ..                                                                                                                                                              [ 66%]
tests/yang_model_tests/test_yang_model.py .                                                                                                                                                     [100%]

========================================================================================== 3 passed in 2.53s ==========================================================================================
```

#### Description for the changelog
Update YANG model for mirror session to support decimal value for GRE type.
stepanblyschak pushed a commit to stepanblyschak/sonic-utilities that referenced this pull request Apr 18, 2022
@vdahiya12
[202012][sonic-utilities] submodule update (#9023)
f63e3cd 
8768089 (HEAD -> 202012, origin/202012) Remove exec from platform_reboot_plugin call to handle any hang issue. (sonic-net#1879)
ae5d90c Validate input of ```config mirror_session add``` (sonic-net#1825)
44d3a3b [show][config] fix the muxcable commands for interface naming mode (sonic-net#1862)
0a4933e [TH3] Skipp Control Plane Assist on WARM Reboot for TH3 HWSKUs (sonic-net#1861)

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
@arfeigin arfeigin mentioned this pull request May 10, 2022
Closed
praveen-li pushed a commit to praveen-li/sonic-buildimage that referenced this pull request Dec 23, 2022
@bingwang-ms
[yang] Update YANG model for mirror session to support decimal value …
ef07f58 
…for GRE type (sonic-net#10140)

#### Why I did it
PR  sonic-net/sonic-utilities#1825 added validation for the input of `config mirror session add`, and only decimal value is accepted.
An issue sonic-net#10096 was raised to suggest accepting HEX value as well, and the suggestion makes sense to me.

To accept HEX value for GRE type, and keep backward compatibility as well, I updated the YANG model to support both decimal and hexadecimal input for GRE type.

#### How I did it
Update the regex for GRE type.

#### How to verify it
Verified by UT
```
platform linux -- Python 3.9.2, pytest-6.0.2, py-1.10.0, pluggy-0.13.0
rootdir: /sonic/src/sonic-yang-models
plugins: pyfakefs-4.5.4, cov-2.10.1
collected 3 items                                                                                                                                                                                     

tests/test_sonic_yang_models.py ..                                                                                                                                                              [ 66%]
tests/yang_model_tests/test_yang_model.py .                                                                                                                                                     [100%]

========================================================================================== 3 passed in 2.53s ==========================================================================================
```

#### Description for the changelog
Update YANG model for mirror session to support decimal value for GRE type.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raphaelt-nvidia raphaelt-nvidia raphaelt-nvidia requested changes

@prsunny prsunny prsunny approved these changes

Assignees
No one assigned
Labels
Included in 202012 Branch Request for 202012 Branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[mirroring] Missing type validations in config mirror-session command
4 participants
@bingwang-ms @raphaelt-nvidia @prsunny @qiluo-msft