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

[Sub-ports] ACL doesn't support sub-ports as port type #1533

Open
OleksandrKozodoi opened this issue Dec 9, 2020 · 2 comments
Open

[Sub-ports] ACL doesn't support sub-ports as port type #1533

OleksandrKozodoi opened this issue Dec 9, 2020 · 2 comments

Comments

@OleksandrKozodoi
Copy link

OleksandrKozodoi commented Dec 9, 2020

Summary

Based on Sub-port HLD ACL could support sub-ports to be assigned as a port type in ACL table.

HLD:

SAI attributes attribute value/type
SAI_ROUTER_INTERFACE_ATTR_INGRESS_ACL ACL table or ACL table group oid
SAI_ROUTER_INTERFACE_ATTR_EGRESS_ACL ACL table or ACL table group oid

But sub-ports port type is not implemented in SONiC realization of ACL.
portsorch.cpp:

switch (port.m_type)
    {
        case Port::PHY:
        {
            attr.id = ingress ?
                    SAI_PORT_ATTR_INGRESS_ACL : SAI_PORT_ATTR_EGRESS_ACL;
            status = sai_port_api->set_port_attribute(port.m_port_id, &attr);
            break;
        }
        case Port::LAG:
        {
            attr.id = ingress ?
                    SAI_LAG_ATTR_INGRESS_ACL : SAI_LAG_ATTR_EGRESS_ACL;
            status = sai_lag_api->set_lag_attribute(port.m_lag_id, &attr);
            break;
        }
        case Port::VLAN:
        {
            attr.id = ingress ?
                    SAI_VLAN_ATTR_INGRESS_ACL : SAI_VLAN_ATTR_EGRESS_ACL;
            status =
                sai_vlan_api->set_vlan_attribute(port.m_vlan_info.vlan_oid,
                                                 &attr);
            break;
        }
        default:
        {
            SWSS_LOG_ERROR("Failed to %s %s port with type %d",
                           bind_str.c_str(), port.m_alias.c_str(), port.m_type);
            return false;
        }
    }

Is this expected and ACL doesn't support sub-ports or needs to add realization of sub-ports to ACL?

Test scenario

I've tried to assign ACL to sub-port interface.
ACL config:

{
    "ACL_TABLE": {
        "test_acl_table": {
            "stage": "ingress",
            "type": "L3",
            "policy_desc": "test_policy",
            "ports": [
            "Ethernet4.10",
            "Ethernet4.20"
            ]
        }
    },
    "ACL_RULE": {
        "test_acl_table|1": {
            "PRIORITY": "10",
            "PACKET_ACTION": "DROP",
            "ICMP_TYPE": "8"
        }
    }
}

After this, I send traffic and expected that it will be dropped on sub-port interface. Actually, it was forwarded.

@wendani
Copy link
Contributor

wendani commented Dec 9, 2020

field ports is a placeholder only for physical port and LAG list. This said, swss only supports these two bind point types as of now. swss does not support acl binding to router interface (sub port being one type of router interfaces).

field ports does not include vlan. There was proposal to introduce schema field vlan to support vlan bind point type. https://github.com/Azure/SONiC/blob/master/doc/acl/ACL-enhancements-on-SONIC.docx but has not yet been materialized.

There is pending effort to support vlan router interface (being one type of router interfaces). #1218

If there is a use case for router interface bind point, a new field schema may be necessary to hold router interface list and to distinguish between physical port/lag/vlan bind point and router interface (type port, type vlan) bind point created on the corresponding objects.

@anshuv-mfst
Copy link

Enhancement request

EdenGri pushed a commit to EdenGri/sonic-swss that referenced this issue Feb 28, 2022
- What I did
Fixed next image mount

- How I did it
Removed lstrip

- How to verify it
root@sonic:/home/admin# fwutil update chassis component <component_name> fw -i next

Signed-off-by: Nazarii Hnydyn <nazariig@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants