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

Expand ACL model by rate-limits (a.k.a. policers) and counters. #1002

Closed
wants to merge 52 commits into from

Conversation

rszarecki
Copy link
Contributor

@rszarecki rszarecki commented Nov 17, 2023

Change Scope

This change introduces 2 new actions to ACL

  • named counter for ACL entry
  • named rate-limit for ACL entry

This change also introduces "scope" attribute for above, that controlls aggregation level of counting/rate-limiting. This has direct impact on number of counter, rate-limit instances that need to be instantiated in datapalane.

The reate-limits are refined in dedicated branch under /acl and are reusable across acl-sets and their entries

module: openconfig-acl
  +--rw acl
     +--rw rate-limits
        +--rw rate-limit* [name]
           +--rw name      -> ../config/name
           +--rw config
           |  +--rw name?             string
           |  +--rw type?             identityref
           |  +--rw cir?              uint64
           |  +--rw cbs?              uint32
           |  +--rw pir?              uint64
           |  +--rw pbs?              uint32
           |  +--rw conform-action
           |  |  +--rw forwarding-action    identityref
           |  |  +--rw log-action?          identityref
           |  |  +--rw counter
           |  |     +--rw enabled?   boolean
           |  |     +--rw scope?
           |  +--rw exceed-action
           |  |  +--rw forwarding-action    identityref
           |  |  +--rw log-action?          identityref
           |  |  +--rw counter
           |  |     +--rw enabled?   boolean
           |  |     +--rw scope?
           |  +--rw violate-action
           |     +--rw forwarding-action?   identityref
           |     +--rw log-action?          identityref
           |     +--rw counter
           |        +--rw enabled?   boolean
           |        +--rw scope?
           +--rw state
              +--rw name?             string
              +--rw type?             identityref
              +--rw cir?              uint64
              +--rw cbs?              uint32
              +--rw pir?              uint64
              +--rw pbs?              uint32
              +--rw conform-action
              |  +--rw forwarding-action    identityref
              |  +--rw log-action?          identityref
              |  +--rw counter
              |     +--rw enabled?   boolean
              |     +--rw scope?
              +--rw exceed-action
              |  +--rw forwarding-action    identityref
              |  +--rw log-action?          identityref
              |  +--rw counter
              |     +--rw enabled?   boolean
              |     +--rw scope?
              +--rw violate-action
                 +--rw forwarding-action?   identityref
                 +--rw log-action?          identityref
                 +--rw counter
                    +--rw enabled?   boolean
                    +--rw scope?

The named rate-limit and counters are ACL entries actions

module: openconfig-acl
  +--rw acl
     +--rw acl-sets
        +--rw acl-set* [name type]
           +--rw acl-entries
              +--rw acl-entry* [sequence-id]
                 +--rw actions
                    +--rw config
                    |  +--rw forwarding-action    identityref
                    |  +--rw rate-limit
                    |  |  +--rw name     -> ../../../../../../../rate-limits/rate-limit/config/name
                    |  |  +--rw scope?   identityref
                    |  +--rw log-action?          identityref
                    |  +--rw log-action?          identityref
                    |  +--rw counter
                    |     +--rw enabled?   boolean
                    |     +--rw scope?
                    +--ro state
                       +--ro forwarding-action    identityref
                       +--ro rate-limit
                       |  +--ro name     -> ../../../../../../../rate-limits/rate-limit/config/name
                       |  +--ro scope?   identityref
                       +--ro log-action?          identityref
                       +--ro log-action?          identityref
                       +--ro counter
                          +--ro enabled?   boolean
                          +--ro scope?

Atatachment of acl-set to interface (or control-plane-traffic) produces respective state

module: openconfig-acl
  +--rw acl
     +--rw interfaces
        +--rw interface* [id]
           +--rw ingress-acl-sets
              +--rw ingress-acl-set* [set-name type]
                 +--ro acl-entries
                    +--ro acl-entry* [sequence-id]
                       +--ro state
                          +--ro sequence-id?         -> /acl/acl-sets/acl-set[oc-acl:name=current()/../../../../set-name][oc-acl:type=current()/../../../../type]/oc-acl:acl-entries/acl-entry/sequence-id
                          +--ro forwarding-action?   identityref
                          +--ro rate-limit-name?     string                                 <<
                          +--ro rate-limit-scope?    identityref                         <<
                          +--ro matched-packets?     oc-yang:counter64    
                          +--ro matched-octets?      oc-yang:counter64
                          +--ro oc-sys-copp:matched-scope?       identityref  <<
                          +--ro conformed-packets?   oc-yang:counter64     <<
                          +--ro conformed-octets?    oc-yang:counter64      <<
                          +--ro exceeding-packets?   oc-yang:counter64      <<
                          +--ro exceeding-octets?    oc-yang:counter64        <<
                          +--ro violating-packets?   oc-yang:counter64         <<
                          +--ro violating-octets?    oc-yang:counter64          <<

module: openconfig-system
  +--rw system
     +--rw oc-sys-copp:control-plane-traffic
        +--rw oc-sys-copp:ingress
           +--rw oc-sys-copp:acl
              +--rw oc-sys-copp:acl-set* [set-name type]
                 +--ro oc-sys-copp:acl-entries
                    +--ro oc-sys-copp:acl-entry* [sequence-id]
                       +--ro oc-sys-copp:sequence-id    -> ../state/sequence-id
                       +--ro oc-sys-copp:state
                          +--ro oc-sys-copp:sequence-id?         -> /oc-acl:acl/acl-sets/acl-set[oc-acl:name=current()/../../../../set-name][oc-acl:type=current()/../../../../type]/oc-acl:acl-entries/acl-entry/sequence-id
                          +--ro oc-sys-copp:forwarding-action?   identityref
                          +--ro oc-sys-copp:rate-limit-name?     string
                          +--ro oc-sys-copp:rate-limit-scope?    identityref
                          +--ro oc-sys-copp:matched-packets?     oc-yang:counter64
                          +--ro oc-sys-copp:matched-octets?      oc-yang:counter64
                          +--ro oc-sys-copp:matched-scope?       identityref
                          +--ro oc-sys-copp:conformed-packets?   oc-yang:counter64
                          +--ro oc-sys-copp:conformed-octets?    oc-yang:counter64
                          +--ro oc-sys-copp:exceeding-packets?   oc-yang:counter64
                          +--ro oc-sys-copp:exceeding-octets?    oc-yang:counter64
                          +--ro oc-sys-copp:violating-packets?   oc-yang:counter64
                          +--ro oc-sys-copp:violating-octets?    oc-yang:counter64

Finally "scope" is enums that alows to control is given instance of rate-limit (token-buckets) is shared among multiple acl-entries of same acl-set instance, shared among entries of multiple acl-set instances but at same attachment point (e.g. interface direction), etc

Platform Implementations

rszarecki and others added 30 commits October 16, 2023 10:22
@rszarecki rszarecki changed the title Expand ACL model by rate-limits (a.k.a. policers) and named counters. Expand ACL model by rate-limits (a.k.a. policers) and counters. Nov 17, 2023
@rszarecki
Copy link
Contributor Author

I have also added COPP VRF awarness

module: openconfig-system
  +--rw system
     +--rw oc-sys-copp:control-plane-traffic
        +--rw oc-sys-copp:ingress
           +--rw oc-sys-copp:network-instances
              +--rw oc-sys-copp:network-instance* [name]
                 +--rw oc-sys-copp:name      string
                 +--rw oc-sys-copp:config
                 |  +--rw oc-sys-copp:name?   string
                 +--rw oc-sys-copp:state
                 |  +--rw oc-sys-copp:name?   string
                 +--rw oc-sys-copp:acl
                    +--rw oc-sys-copp:acl-set* [set-name type]
                       +--rw oc-sys-copp:set-name       -> ../config/set-name
                       +--rw oc-sys-copp:type           -> ../config/type
                       +--rw oc-sys-copp:config
                       |  +--rw oc-sys-copp:set-name?   -> /oc-acl:acl/acl-sets/acl-set/config/name
                       |  +--rw oc-sys-copp:type?       -> /oc-acl:acl/acl-sets/acl-set[oc-acl:name=current()/../set-name]/config/type
                       +--ro oc-sys-copp:state
                       |  +--ro oc-sys-copp:set-name?   -> /oc-acl:acl/acl-sets/acl-set/config/name
                       |  +--ro oc-sys-copp:type?       -> /oc-acl:acl/acl-sets/acl-set[oc-acl:name=current()/../set-name]/config/type
                       +--ro oc-sys-copp:acl-entries
                          +--ro oc-sys-copp:acl-entry* [sequence-id]
                             +--ro oc-sys-copp:sequence-id    -> ../state/sequence-id
                             +--ro oc-sys-copp:state
                                +--ro oc-sys-copp:sequence-id?         -> /oc-acl:acl/acl-sets/acl-set[oc-acl:name=current()/../../../../set-name][oc-acl:type=current()/../../../../type]/oc-acl:acl-entries/acl-entry/sequence-id
                                +--ro oc-sys-copp:forwarding-action?   identityref
                                +--ro oc-sys-copp:rate-limit-name?     string
                                +--ro oc-sys-copp:rate-limit-scope?    identityref
                                +--ro oc-sys-copp:matched-packets?     oc-yang:counter64
                                +--ro oc-sys-copp:matched-octets?      oc-yang:counter64
                                +--ro oc-sys-copp:matched-scope?       identityref
                                +--ro oc-sys-copp:conformed-packets?   oc-yang:counter64
                                +--ro oc-sys-copp:conformed-octets?    oc-yang:counter64
                                +--ro oc-sys-copp:exceeding-packets?   oc-yang:counter64
                                +--ro oc-sys-copp:exceeding-octets?    oc-yang:counter64
                                +--ro oc-sys-copp:violating-packets?   oc-yang:counter64
                                +--ro oc-sys-copp:violating-octets?    oc-yang:counter64

@rszarecki rszarecki marked this pull request as ready for review November 30, 2023 00:14
@rszarecki rszarecki requested a review from a team as a code owner November 30, 2023 00:14
@robshakir
Copy link
Contributor

I have concerns with this change -- can you explain why this isn't using the QoS model, and/or why it is applicable to all ACLs across the system?

Copy link
Contributor

@robshakir robshakir left a comment

Choose a reason for hiding this comment

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

Adding a blocking comment as I think there are wider design questions for this approach.

@rszarecki rszarecki marked this pull request as draft November 30, 2023 20:35
@rszarecki
Copy link
Contributor Author

I have concerns with this change -- can you explain why this isn't using the QoS model, and/or why it is applicable to all ACLs across the system?

  • rate limit is not that exotic action, applicable in many places of system. Specificly on ingress interfaces, when limiting certain traffic is quite common need. So fare interfaces and control-plane-traffic are ONLY places when ACL could be attached. (Note rate-limits we have defined in /qos are attributes of schedulers, hence apply only if you have queue. Many system has only output queues.)
  • Classifies in /qos are missing multiple matching options, that are crutial (TCP flags, ICMP codes, etc)
  • Classifies in /qos support only one action - assign traffic-group. And we need count, rate-limit, discard, reject, log etc.

Sure we can extend /qos classifiers. E.g. making acl-set one of matching criteria and adding missing actions.

Or we can extend /acl as in this proposal. OR we can define new object "/traffic-policy" that has structure similar to /qos classifier but is "attachment point agnostic" - attachable to interfaces, control-plane, forwarding-table, qos/interface ...

@robshakir - WDYT ?

@rajatiitd
Copy link

Hi @rszarecki,

We have the below of queries regarding your proposed model, can you please clarify?

  1. The proposed model does not define way to configure single rate three colour rate limiter RFC 2697 (SrTCM), right now only RL_1R2C, RL_2R3C are defined in the current proposed model. May I know what is plan to support RFC 2697 SrTCM Policer?
  2. What is plan to support colour aware and colour bind? Right now this is not supported in proposed model
  3. What’s the use case of “accept” under violate-action ?
  4. What’s the use case of “accept” under exceed-action?
  5. RFC 2697/2698 defines packet marking, but the proposed model doesn’t define a way to configure loss-priority, forwarding-class action for rate limiter. May I know what is plan to support it?

Thank and regards
Rajat Rastogi

@github-actions github-actions bot added the Stale label Aug 20, 2024
@github-actions github-actions bot closed this Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants