Validate input of ```config mirror_session add``` by bingwang-ms · Pull Request #1825 · sonic-net/sonic-utilities

bingwang-ms · 2021-09-17T08:03:45Z

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)

Signed-off-by: bingwang <bingwang@microsoft.com>

bingwang-ms · 2021-09-18T12:04:37Z

/azp run

azure-pipelines · 2021-09-18T12:04:47Z

Azure Pipelines successfully started running 1 pipeline(s).

raphaelt-nvidia

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

Signed-off-by: bingwang <wang.bing@microsoft.com>

bingwang-ms · 2021-09-20T01:08:53Z

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 · 2021-09-20T15:00:06Z

/azp run

azure-pipelines · 2021-09-20T15:00:25Z

Azure Pipelines successfully started running 1 pipeline(s).

bingwang-ms · 2021-09-22T00:50:25Z

/azp run

azure-pipelines · 2021-09-22T00:50:36Z

Azure Pipelines successfully started running 1 pipeline(s).

bingwang-ms · 2021-09-30T00:34:35Z

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

raphaelt-nvidia · 2021-09-30T06:22:38Z

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 · 2021-10-08T00:40:41Z

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 · 2021-10-10T05:50:29Z

SO LGTM. Should someone else review?

prsunny · 2021-10-12T02:20:37Z

config/main.py

    return success

+
+def validate_ipv4_address(ctx, param, ip_addr):


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

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.

* Validate input of add mirror session Signed-off-by: bingwang <bingwang@microsoft.com>

raphaelt-nvidia · 2022-02-23T14:32:22Z

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?

…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.

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>

…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.

bingwang-ms added 2 commits September 16, 2021 04:58

Validate input of add mirror session

9c54714

Signed-off-by: bingwang <bingwang@microsoft.com>

Add UT for mirror_session

74b3fd6

Signed-off-by: bingwang <bingwang@microsoft.com>

bingwang-ms mentioned this pull request Sep 17, 2021

[mirroring] Missing type validations in config mirror-session command sonic-net/sonic-buildimage#7990

Closed

raphaelt-nvidia suggested changes Sep 19, 2021

View reviewed changes

Add span test case

f4b5b18

Signed-off-by: bingwang <wang.bing@microsoft.com>

prsunny reviewed Oct 12, 2021

View reviewed changes

prsunny approved these changes Oct 14, 2021

View reviewed changes

bingwang-ms merged commit c950a55 into sonic-net:master Oct 14, 2021

bingwang-ms added the Request for 202012 Branch label Oct 14, 2021

qiluo-msft pushed a commit that referenced this pull request Oct 15, 2021

Validate input of config mirror_session add (#1825)

ae5d90c

* Validate input of add mirror session Signed-off-by: bingwang <bingwang@microsoft.com>

qiluo-msft added the Included in 202012 Branch label Oct 15, 2021

raphaelt-nvidia mentioned this pull request Feb 27, 2022

[ERSPAN] Hexadecimal values are not accepted for GRE parameter sonic-net/sonic-buildimage#10096

Closed

bingwang-ms mentioned this pull request Mar 3, 2022

[yang] Update YANG model for mirror session to support decimal value for GRE type sonic-net/sonic-buildimage#10140

Merged

6 tasks

arfeigin mentioned this pull request May 10, 2022

[mirroring] Queue value 0 is missing after validation in config mirror-session command sonic-net/sonic-buildimage#10795

Closed

		return success


		def validate_ipv4_address(ctx, param, ip_addr):

Conversation

bingwang-ms commented Sep 17, 2021

What I did

How I did it

How to verify it

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)

Uh oh!

bingwang-ms commented Sep 18, 2021

Uh oh!

azure-pipelines bot commented Sep 18, 2021

Uh oh!

raphaelt-nvidia left a comment

Choose a reason for hiding this comment

Uh oh!

bingwang-ms commented Sep 20, 2021

Uh oh!

bingwang-ms commented Sep 20, 2021

Uh oh!

azure-pipelines bot commented Sep 20, 2021

Uh oh!

bingwang-ms commented Sep 22, 2021

Uh oh!

azure-pipelines bot commented Sep 22, 2021

Uh oh!

bingwang-ms commented Sep 30, 2021

Uh oh!

raphaelt-nvidia commented Sep 30, 2021

Uh oh!

bingwang-ms commented Oct 8, 2021

Uh oh!

raphaelt-nvidia commented Oct 10, 2021

Uh oh!

prsunny Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

bingwang-ms Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

raphaelt-nvidia commented Feb 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants