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

[Yang] Incorrect definition of IN_PORTS and OUT_PORTS in sonic-acl.yang #16190

Closed
lizhijianrd opened this issue Aug 17, 2023 · 2 comments · Fixed by #16220
Closed

[Yang] Incorrect definition of IN_PORTS and OUT_PORTS in sonic-acl.yang #16190

lizhijianrd opened this issue Aug 17, 2023 · 2 comments · Fixed by #16220

Comments

@lizhijianrd
Copy link
Contributor

lizhijianrd commented Aug 17, 2023

Description

The type of IN_PORTS and OUT_PORTS at sonic-acl.yang.j2#L131 is incorrect.

According to Everflow-test-plan, it should be a string, not uint16. The string format should be a list of SONiC interface name separated by comma, like `"Ethernet3,Ethernet4".

According to Sonic-mclag-hld, OUT_PORTS should be a string of the same format.

This bug causes GCU cannot add/update data-plane ACL rules with IN_PORTS and OUT_PORTS.

Steps to reproduce the issue:

  1. Setup custom ACL table type on DUT.
    a. Write below content to file acl_table_types.json on DUT:
    {
        "ACL_TABLE_TYPE": {
            "BMCDATAV6": {
                "MATCHES": "SRC_IPV6,DST_IPV6,ETHER_TYPE,IP_TYPE,IP_PROTOCOL,IN_PORTS,L4_SRC_PORT,L4_DST_PORT,L4_SRC_PORT_RANGE,L4_DST_PORT_RANGE",
                "ACTIONS": "PACKET_ACTION,COUNTER",
                "BIND_POINTS": "PORT"
            }
        }
    }
    b. Add custom ACL table type to config_db: sonic-cfggen -j acl_table_types.json -w
  2. Create ACL table on DUT: sudo config acl add table SAMPLE_ACL_TABLE BMCDATAV6 -p Ethernet1,Ethernet2 -s ingress.
  3. Add first ACL rule to config_db via acl-loader:
    a. Write below content to file acl_rules.json on DUT:
    {
        "acl": {
            "acl-sets": {
                "acl-set": {
                    "SAMPLE_ACL_TABLE": {
                        "acl-entries": {
                            "acl-entry": {
                                "RULE_1": {
                                    "actions": {
                                        "config": {
                                            "forwarding-action": "DROP"
                                        }
                                    },
                                    "config": {
                                        "sequence-id": 1
                                    },
                                    "input_interface": {
                                        "interface_ref": {
                                            "config": {
                                                "interface": "Ethernet1"
                                            }
                                        }
                                    }
                                }
                            }
                        }
                    }
                }
            }
        }
    }
    b. Use acl-loader to load above ACL rule: acl-loader update full --table_name SAMPLE_ACL_TABLE acl_rules.json.
  4. Add second ACL rule to config_db via GCU:
    a. Write below content to file patch.json on DUT:
    [
        {
            "op": "add",
            "path": "/ACL_RULE/SAMPLE_ACL_TABLE|RULE_3000",
            "value": {
                "PACKET_ACTION": "FORWARD",
                "PRIORITY": "7000",
                "IN_PORTS": "Ethernet2"
            }
        }
    ]
    b. Use GCU to apply above patch: sudo config apply-patch patch.json

Describe the results you received:

At step 4.b, I got below error on DUT:

$ sudo config apply-patch patch.json
Patch Applier: Patch application starting.
Patch Applier: Patch: [{"op": "add", "path": "/ACL_RULE/SAMPLE_ACL_TABLE|RULE_3000", "value": {"PACKET_ACTION": "FORWARD", "PRIORITY": "7000", "IN_PORTS": "Ethernet2"}}]
Patch Applier: Getting current config db.
Patch Applier: Simulating the target full config after applying the patch.
Patch Applier: Validating all JsonPatch operations are permitted on the specified fields
Patch Applier: Validating target config does not have empty tables, since they do not show up in ConfigDb.
Patch Applier: Sorting patch updates.
Failed to apply patch
Usage: config apply-patch [OPTIONS] PATCH_FILE_PATH
Try "config apply-patch -h" for help.

Error: Given patch will produce invalid config. Error: Data Loading Failed
All Keys are not parsed in ACL_RULE
dict_keys(['SAMPLE_ACL_TABLE|RULE_1', 'SAMPLE_ACL_TABLE|RULE_3000'])
exceptionList:["invalid literal for int() with base 10: 'E'", "invalid literal for int() with base 10: 'E'"]

From the error message we can see both the first ACL rule (already exist in config_db) and the second ACL rule (in GCU patch) cannot pass Yang validation. They failed on key IN_PORTS, which should be a string, but incorrectly defined as uint16 in Yang model.

Describe the results you expected:

Expect step 4.b can apply the second ACL rule on DUT successfully.

Output of show version:

Can repro this issue on 202205.

Output of show techsupport:

(paste your output here or download and attach the file here )

Additional information you deem important (e.g. issue happens only occasionally):

@lizhijianrd lizhijianrd changed the title [Yang] Incorrect definition of IN_PORTS in sonic-acl.yang [Yang] Incorrect definition of IN_PORTS and OUT_PORTS in sonic-acl.yang Aug 21, 2023
@bingwang-ms
Copy link
Contributor

So we can load ACL rules with "IN_PORTS" with acl-loader, but it's not accepted when using config apply-patch, right?

yxieca pushed a commit that referenced this issue Aug 22, 2023
How I did it
Update Yang definition of IN_PORTS and OUT_PORTS to string.
Since we cannot split the string with comma (,) and validate each substring is a valid SONiC port name. The only restriction for them is must be a string.

How to verify it
Verified by building sonic_yang_models-1.0-py3-none-any.whl. While building the target package, unit tests were run and passed.
Build a SONiC image based on 202205 branch and installed on physical DUT. Re try the steps in [Yang] Incorrect definition of IN_PORTS and OUT_PORTS in sonic-acl.yang #16190 and can see below success response:
mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this issue Aug 22, 2023
…-net#16220)

How I did it
Update Yang definition of IN_PORTS and OUT_PORTS to string.
Since we cannot split the string with comma (,) and validate each substring is a valid SONiC port name. The only restriction for them is must be a string.

How to verify it
Verified by building sonic_yang_models-1.0-py3-none-any.whl. While building the target package, unit tests were run and passed.
Build a SONiC image based on 202205 branch and installed on physical DUT. Re try the steps in [Yang] Incorrect definition of IN_PORTS and OUT_PORTS in sonic-acl.yang sonic-net#16190 and can see below success response:
mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this issue Aug 22, 2023
…-net#16220)

How I did it
Update Yang definition of IN_PORTS and OUT_PORTS to string.
Since we cannot split the string with comma (,) and validate each substring is a valid SONiC port name. The only restriction for them is must be a string.

How to verify it
Verified by building sonic_yang_models-1.0-py3-none-any.whl. While building the target package, unit tests were run and passed.
Build a SONiC image based on 202205 branch and installed on physical DUT. Re try the steps in [Yang] Incorrect definition of IN_PORTS and OUT_PORTS in sonic-acl.yang sonic-net#16190 and can see below success response:
@lizhijianrd
Copy link
Contributor Author

lizhijianrd commented Aug 23, 2023

So we can load ACL rules with "IN_PORTS" with acl-loader, but it's not accepted when using config apply-patch, right?

Yes, you are right.

mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this issue Aug 23, 2023
…-net#16220)

How I did it
Update Yang definition of IN_PORTS and OUT_PORTS to string.
Since we cannot split the string with comma (,) and validate each substring is a valid SONiC port name. The only restriction for them is must be a string.

How to verify it
Verified by building sonic_yang_models-1.0-py3-none-any.whl. While building the target package, unit tests were run and passed.
Build a SONiC image based on 202205 branch and installed on physical DUT. Re try the steps in [Yang] Incorrect definition of IN_PORTS and OUT_PORTS in sonic-acl.yang sonic-net#16190 and can see below success response:
mssonicbld pushed a commit that referenced this issue Aug 25, 2023
How I did it
Update Yang definition of IN_PORTS and OUT_PORTS to string.
Since we cannot split the string with comma (,) and validate each substring is a valid SONiC port name. The only restriction for them is must be a string.

How to verify it
Verified by building sonic_yang_models-1.0-py3-none-any.whl. While building the target package, unit tests were run and passed.
Build a SONiC image based on 202205 branch and installed on physical DUT. Re try the steps in [Yang] Incorrect definition of IN_PORTS and OUT_PORTS in sonic-acl.yang #16190 and can see below success response:
yxieca pushed a commit that referenced this issue Aug 26, 2023
… (#16235)

How I did it
Update Yang definition of IN_PORTS and OUT_PORTS to string.
Since we cannot split the string with comma (,) and validate each substring is a valid SONiC port name. The only restriction for them is must be a string.

How to verify it
Verified by building sonic_yang_models-1.0-py3-none-any.whl. While building the target package, unit tests were run and passed.
Build a SONiC image based on 202205 branch and installed on physical DUT. Re try the steps in [Yang] Incorrect definition of IN_PORTS and OUT_PORTS in sonic-acl.yang #16190 and can see below success response:

Co-authored-by: Zhijian Li <[email protected]>
mssonicbld pushed a commit that referenced this issue Aug 31, 2023
How I did it
Update Yang definition of IN_PORTS and OUT_PORTS to string.
Since we cannot split the string with comma (,) and validate each substring is a valid SONiC port name. The only restriction for them is must be a string.

How to verify it
Verified by building sonic_yang_models-1.0-py3-none-any.whl. While building the target package, unit tests were run and passed.
Build a SONiC image based on 202205 branch and installed on physical DUT. Re try the steps in [Yang] Incorrect definition of IN_PORTS and OUT_PORTS in sonic-acl.yang #16190 and can see below success response:
sonic-otn pushed a commit to sonic-otn/sonic-buildimage that referenced this issue Sep 20, 2023
…-net#16220)

How I did it
Update Yang definition of IN_PORTS and OUT_PORTS to string.
Since we cannot split the string with comma (,) and validate each substring is a valid SONiC port name. The only restriction for them is must be a string.

How to verify it
Verified by building sonic_yang_models-1.0-py3-none-any.whl. While building the target package, unit tests were run and passed.
Build a SONiC image based on 202205 branch and installed on physical DUT. Re try the steps in [Yang] Incorrect definition of IN_PORTS and OUT_PORTS in sonic-acl.yang sonic-net#16190 and can see below success response:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants