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 restriction for ICMP/ICMPv6 type and code. #18137

Closed
lizhijianrd opened this issue Feb 20, 2024 · 5 comments
Closed

[Yang] Incorrect restriction for ICMP/ICMPv6 type and code. #18137

lizhijianrd opened this issue Feb 20, 2024 · 5 comments
Assignees
Labels
Triaged this issue has been triaged YANG YANG model related changes

Comments

@lizhijianrd
Copy link
Contributor

lizhijianrd commented Feb 20, 2024

Description

For ICMP, sonic-acl.yang.j2 require type between 1 and 44, and code between 1 and 16. However, this is incorrect. For example, according to RFC 792, type=0 and code=0 means echo reply message. These are valid ICMP type/code but cannot pass Yang validation.

For ICMPv6, sonic-acl.yang.j2 also require type between 1 and 44, and code between 1 and 16. This is incorrect, too. For example, according to RFC 4443, type=129 and code=0 means echo reply message.

Steps to reproduce the issue:

Currently, this issue impacts GCU dataplane ACL update. Below is the repro steps:

  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 a data-plane ACL table: sudo config acl add table SAMPLE_DATAPLANE_ACL_TABLE BMCDATAV6 -d SAMPLE -s ingress -p Ethernet1,Ethernet2
  3. Add data-plane ACL rule to config_db via acl-loader:
    a. Write below content to acl_rules.json file on DUT:
    {
        "acl": {
            "acl-sets": {
                "acl-set": {
                    "SAMPLE_DATAPLANE_ACL_TABLE": {
                        "acl-entries": {
                            "acl-entry": {
                                "DROP_ICMPv6_ECHO_REPLY": {
                                    "actions": {
                                        "config": {
                                            "forwarding-action": "DROP"
                                        }
                                    },
                                    "config": {
                                        "sequence-id": 1
                                    },
                                    "icmp": {
                                        "config": {
                                            "code": 0,
                                            "type": 129
                                        }
                                    },
                                    "ip": {
                                        "config": {
                                            "protocol": "58"
                                        }
                                    }
                                }
                            }
                        }
                    }
                }
            }
        }
    }
    b. Load ACL rule: acl-loader update full acl_rules.json
  4. Add control-plane ACL rule to config_db via GCU:
    a. Write below content to file patch.json on DUT:
    [
        {
            "op": "add",
            "path": "/ACL_RULE/SSH_ONLY|RULE_3",
            "value": {
                "PACKET_ACTION": "ACCEPT",
                "PRIORITY": "9997",
            "SRC_IP": "1.1.1.1/32"
            }
        }
    ]
    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/SSH_ONLY|RULE_3", "value": {"PACKET_ACTION": "ACCEPT", "PRIORITY": "9997", "SRC_IP": "1.1.1.1/32"}}]
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
Value "0" does not satisfy the constraint "1..16" (range, length, or pattern).

Describe the results you expected:

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

Output of show version:

I can repro this issue on 202205 image.

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):

@arlakshm
Copy link
Contributor

@qiluo-msft, can you triage this issue in the yang subgroup

@arlakshm arlakshm added the YANG YANG model related changes label Feb 28, 2024
@zhangyanzhao
Copy link
Collaborator

Assign this to Qi, please help to take a look and see if someone can help. Thanks.

@zhangyanzhao zhangyanzhao added the Triaged this issue has been triaged label Mar 7, 2024
@qiluo-msft qiluo-msft assigned bingwang-ms and unassigned qiluo-msft Mar 7, 2024
@lizhijianrd
Copy link
Contributor Author

lizhijianrd commented Mar 8, 2024

FYI, In the implementation of openconfig, it defines each ICMPv6 type in Yang model. https://github.com/openconfig/public/blob/master/release/models/acl/openconfig-icmpv6-types.yang

@Blueve
Copy link
Contributor

Blueve commented Mar 20, 2024

@bingwang-ms Can we close this issue?

@bingwang-ms
Copy link
Contributor

Closed by #18311

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Triaged this issue has been triaged YANG YANG model related changes
Projects
Status: Done
Development

No branches or pull requests

6 participants