On 24/05/2021 18:04, Lorenzo Bianconi wrote: >> On 29/04/2021 18:04, Lorenzo Bianconi wrote: >>> From: Dumitru Ceara <dce...@redhat.com> >>> > > [...] >> >> Could this be changed to copp_proto_get_name()? > > ack, I will fix it in v2 > >> >>> +{ >>> + if (proto >= COPP_PROTO_MAX) { >>> + return "<Invalid control protocol ID>"; >>> + } >>> + return copp_proto_names[proto]; >>> +} >>> + >>> +const char * >>> +copp_meter_get(enum copp_proto proto, const struct nbrec_copp *copp, >>> + const struct shash *meter_groups) >>> +{ >>> + if (!copp || proto >= COPP_PROTO_MAX) { >>> + return NULL; >>> + } >>> + >>> + const char *meter = smap_get(&copp->meters, copp_proto_names[proto]); >>> + >>> + if (meter && shash_find(meter_groups, meter)) { >>> + return meter; >>> + } >>> + >>> + return NULL; >>> +} >>> + >>> +void >>> +copp_list(struct ctl_context *ctx, const struct nbrec_copp *copp) >>> +{ >>> + if (!copp) { >>> + return; >>> + } >>> + >>> + struct smap_node *node; >>> + >>> + SMAP_FOR_EACH (node, &copp->meters) { >>> + ds_put_format(&ctx->output, "%s: %s\n", node->key, node->value); >>> + } >>> +} >>> + >>> +const struct nbrec_copp * >>> +copp_add_meter(struct ctl_context *ctx, const struct nbrec_copp *copp, >>> + const char *proto_name, const char *meter) >>> +{ >>> + if (!copp) { >>> + copp = nbrec_copp_insert(ctx->txn); >>> + } >>> + >>> + struct smap meters; >>> + smap_init(&meters); >>> + smap_clone(&meters, &copp->meters); >>> + smap_replace(&meters, proto_name, meter); >>> + nbrec_copp_set_meters(copp, &meters); >>> + smap_destroy(&meters); >>> + >>> + return copp; >>> +} >>> + >>> +void >>> +copp_del_meter(const struct nbrec_copp *copp, const char *proto_name) >>> +{ >>> + if (!copp) { >>> + return; >>> + } >>> + >>> + if (proto_name) { >>> + if (smap_get(&copp->meters, proto_name)) { >>> + struct smap meters; >>> + smap_init(&meters); >>> + smap_clone(&meters, &copp->meters); >>> + smap_remove(&meters, proto_name); >>> + nbrec_copp_set_meters(copp, &meters); >>> + smap_destroy(&meters); >>> + } >>> + } else { >>> + nbrec_copp_delete(copp); >>> + } >>> +} >>> + >>> +char * >>> +copp_proto_validate(const char *proto_name)> +{ >>> + for (size_t i = COPP_PROTO_FIRST; i < COPP_PROTO_MAX; i++) { >>> + if (!strcmp(proto_name, copp_proto_get(i))) { >>> + return NULL; >>> + } >>> + } >>> + >>> + struct ds usage = DS_EMPTY_INITIALIZER; >>> + >>> + ds_put_cstr(&usage, "Invalid control protocol. Allowed values: "); >>> + for (size_t i = COPP_PROTO_FIRST; i < COPP_PROTO_MAX; i++) { >>> + ds_put_format(&usage, "%s, ", copp_proto_get(i)); >>> + } >>> + ds_chomp(&usage, ' '); >>> + ds_chomp(&usage, ','); >>> + ds_put_cstr(&usage, "."); >>> + >>> + char *usage_str = xstrdup(ds_cstr(&usage)); >>> + ds_destroy(&usage); >>> + return usage_str; >>> +} >>> diff --git a/lib/copp.h b/lib/copp.h >>> new file mode 100644 >>> index 000000000..82581e7e4 >>> --- /dev/null >>> +++ b/lib/copp.h >>> @@ -0,0 +1,60 @@ >>> +/* Copyright (c) 2021, Red Hat, Inc. >>> + * >>> + * Licensed under the Apache License, Version 2.0 (the "License"); >>> + * you may not use this file except in compliance with the License. >>> + * You may obtain a copy of the License at: >>> + * >>> + * http://www.apache.org/licenses/LICENSE-2.0 >>> + * >>> + * Unless required by applicable law or agreed to in writing, software >>> + * distributed under the License is distributed on an "AS IS" BASIS, >>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. >>> + * See the License for the specific language governing permissions and >>> + * limitations under the License. >>> + */ >>> + >>> +#ifndef OVN_COPP_H >>> +#define OVN_COPP_H 1 >>> + >>> +/* >>> + * Control plane protection - metered actions. >>> + */ >>> +enum copp_proto { >>> + COPP_PROTO_FIRST, >>> + COPP_ARP = COPP_PROTO_FIRST, >>> + COPP_ARP_RESOLVE, >>> + COPP_DHCPV4_OPTS, >>> + COPP_DHCPV6_OPTS, >>> + COPP_DNS, >>> + COPP_EVENT_ELB, >>> + COPP_ICMP4_ERR, >>> + COPP_ICMP6_ERR, >>> + COPP_IGMP, >>> + COPP_ND_NA, >>> + COPP_ND_NS, >>> + COPP_ND_NS_RESOLVE, >>> + COPP_ND_RA_OPTS, >>> + COPP_TCP_RESET, >>> + COPP_BFD, >>> + COPP_PROTO_MAX, >>> + COPP_PROTO_INVALID = COPP_PROTO_MAX, >>> +}; >>> + >>> +struct nbrec_copp; >>> +struct ctl_context; >>> + >>> +const char *copp_proto_get(enum copp_proto); >> >> I think this could a static and declared in copp.c as it is not used >> outside of there. > > ack, I will fix it in v2 >> >>> + >>> +const char *copp_meter_get(enum copp_proto proto, >>> + const struct nbrec_copp *copp, >>> + const struct shash *meter_groups); >>> + >>> +void copp_list(struct ctl_context *ctx, const struct nbrec_copp *copp); >>> +const struct nbrec_copp * >>> +copp_add_meter(struct ctl_context *ctx, const struct nbrec_copp *copp, >>> + const char *proto_name, const char *meter); >>> +void >>> +copp_del_meter(const struct nbrec_copp *copp, const char *proto_name); >>> +char * copp_proto_validate(const char *proto_name); >> >> Could you change the names of these function prototypes to be more >> consistent? e.g. copp_meter_add/del/get/list() > > ack, I will fix it in v2 >> >>> + >>> +#endif /* lib/copp.h */ >>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c >>> index 94fae5648..2e9c7de22 100644 >>> --- a/northd/ovn-northd.c >>> +++ b/northd/ovn-northd.c >>> @@ -32,6 +32,7 @@ >>> #include "ovn/lex.h" >>> #include "lib/chassis-index.h" >>> #include "lib/ip-mcast-index.h" >>> +#include "lib/copp.h" >>> #include "lib/mcast-group-index.h" >>> #include "lib/ovn-l7.h" >>> #include "lib/ovn-nb-idl.h" >>> @@ -4018,6 +4019,7 @@ struct ovn_lflow { >>> char *match; >>> char *actions; >>> char *stage_hint; >>> + char *ctrl_meter; >>> const char *where; >>> }; >>> >>> @@ -4051,14 +4053,15 @@ ovn_lflow_equal(const struct ovn_lflow *a, const >>> struct ovn_lflow *b) >>> && a->stage == b->stage >>> && a->priority == b->priority >>> && !strcmp(a->match, b->match) >>> - && !strcmp(a->actions, b->actions)); >>> + && !strcmp(a->actions, b->actions) >>> + && nullable_string_is_equal(a->ctrl_meter, b->ctrl_meter)); >>> } >>> >>> static void >>> ovn_lflow_init(struct ovn_lflow *lflow, struct ovn_datapath *od, >>> enum ovn_stage stage, uint16_t priority, >>> - char *match, char *actions, char *stage_hint, >>> - const char *where) >>> + char *match, char *actions, char *ctrl_meter, >>> + char *stage_hint, const char *where) >>> { >>> hmapx_init(&lflow->od_group); >>> lflow->od = od; >>> @@ -4067,6 +4070,7 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct >>> ovn_datapath *od, >>> lflow->match = match; >>> lflow->actions = actions; >>> lflow->stage_hint = stage_hint; >>> + lflow->ctrl_meter = ctrl_meter; >>> lflow->where = where; >>> } >>> >>> @@ -4107,6 +4111,7 @@ static void >>> ovn_lflow_add_at(struct hmap *lflow_map, struct ovn_datapath *od, >>> enum ovn_stage stage, uint16_t priority, >>> const char *match, const char *actions, bool shared, >>> + const char *ctrl_meter, >>> const struct ovsdb_idl_row *stage_hint, const char *where) >>> { >>> ovs_assert(ovn_stage_to_datapath_type(stage) == >>> ovn_datapath_get_type(od)); >>> @@ -4120,6 +4125,7 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct >>> ovn_datapath *od, >>> * one datapath in a group, so it could be hashed correctly. */ >>> ovn_lflow_init(lflow, NULL, stage, priority, >>> xstrdup(match), xstrdup(actions), >>> + nullable_xstrdup(ctrl_meter), >>> ovn_lflow_hint(stage_hint), where); >>> >>> hash = ovn_lflow_hash(lflow); >>> @@ -4134,14 +4140,24 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct >>> ovn_datapath *od, >>> } >>> >>> /* Adds a row with the specified contents to the Logical_Flow table. */ >>> +#define ovn_lflow_add_with_hint__(LFLOW_MAP, OD, STAGE, PRIORITY, MATCH, \ >>> + ACTIONS, CTRL_METER, STAGE_HINT) \ >>> + ovn_lflow_add_at(LFLOW_MAP, OD, STAGE, PRIORITY, MATCH, ACTIONS, true, >>> \ >>> + CTRL_METER, STAGE_HINT, OVS_SOURCE_LOCATOR) >>> + >>> #define ovn_lflow_add_with_hint(LFLOW_MAP, OD, STAGE, PRIORITY, MATCH, \ >>> ACTIONS, STAGE_HINT) \ >>> - ovn_lflow_add_at(LFLOW_MAP, OD, STAGE, PRIORITY, MATCH, ACTIONS, true, >>> \ >>> - STAGE_HINT, OVS_SOURCE_LOCATOR) >>> + ovn_lflow_add_with_hint__(LFLOW_MAP, OD, STAGE, PRIORITY, MATCH, \ >>> + ACTIONS, NULL, STAGE_HINT) >>> >>> #define ovn_lflow_add(LFLOW_MAP, OD, STAGE, PRIORITY, MATCH, ACTIONS) \ >>> ovn_lflow_add_at(LFLOW_MAP, OD, STAGE, PRIORITY, MATCH, ACTIONS, true, >>> \ >>> - NULL, OVS_SOURCE_LOCATOR) >>> + NULL, NULL, OVS_SOURCE_LOCATOR) >>> + >>> +#define ovn_lflow_add_ctrl(LFLOW_MAP, OD, STAGE, PRIORITY, MATCH, ACTIONS, >>> \ >>> + CTRL_METER) \ >>> + ovn_lflow_add_with_hint__(LFLOW_MAP, OD, STAGE, PRIORITY, MATCH, \ >>> + ACTIONS, CTRL_METER, NULL) >>> >>> /* Adds a row with the specified contents to the Logical_Flow table. >>> * Combining of this logical flow with already existing ones, e.g., by >>> using >>> @@ -4156,21 +4172,22 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct >>> ovn_datapath *od, >>> #define ovn_lflow_add_unique_with_hint(LFLOW_MAP, OD, STAGE, PRIORITY, >>> MATCH, \ >>> ACTIONS, STAGE_HINT) \ >>> ovn_lflow_add_at(LFLOW_MAP, OD, STAGE, PRIORITY, MATCH, ACTIONS, >>> false, \ >>> - STAGE_HINT, OVS_SOURCE_LOCATOR) >>> + NULL, STAGE_HINT, OVS_SOURCE_LOCATOR) >>> >>> #define ovn_lflow_add_unique(LFLOW_MAP, OD, STAGE, PRIORITY, MATCH, >>> ACTIONS) \ >>> ovn_lflow_add_at(LFLOW_MAP, OD, STAGE, PRIORITY, MATCH, ACTIONS, >>> false, \ >>> - NULL, OVS_SOURCE_LOCATOR) >>> + NULL, NULL, OVS_SOURCE_LOCATOR) >>> >>> static struct ovn_lflow * >>> ovn_lflow_find(struct hmap *lflows, struct ovn_datapath *od, >>> enum ovn_stage stage, uint16_t priority, >>> - const char *match, const char *actions, uint32_t hash) >>> + const char *match, const char *actions, const char >>> *ctrl_meter, >>> + uint32_t hash) >>> { >>> struct ovn_lflow target; >>> ovn_lflow_init(&target, od, stage, priority, >>> CONST_CAST(char *, match), CONST_CAST(char *, actions), >>> - NULL, NULL); >>> + CONST_CAST(char *, ctrl_meter), NULL, NULL); >>> >>> return ovn_lflow_find_by_lflow(lflows, &target, hash); >>> } >>> @@ -4186,6 +4203,7 @@ ovn_lflow_destroy(struct hmap *lflows, struct >>> ovn_lflow *lflow) >>> free(lflow->match); >>> free(lflow->actions); >>> free(lflow->stage_hint); >>> + free(lflow->ctrl_meter); >>> free(lflow); >>> } >>> } >>> @@ -12333,7 +12351,8 @@ build_lflows(struct northd_context *ctx, struct >>> hmap *datapaths, >>> lflow = ovn_lflow_find( >>> &lflows, logical_datapath_od, >>> ovn_stage_build(dp_type, pipeline, sbflow->table_id), >>> - sbflow->priority, sbflow->match, sbflow->actions, >>> sbflow->hash); >>> + sbflow->priority, sbflow->match, sbflow->actions, >>> + sbflow->controller_meter, sbflow->hash); >>> if (lflow) { >>> /* This is a valid lflow. Checking if the datapath group needs >>> * updates. */ >>> @@ -12378,6 +12397,7 @@ build_lflows(struct northd_context *ctx, struct >>> hmap *datapaths, >>> sbrec_logical_flow_set_priority(sbflow, lflow->priority); >>> sbrec_logical_flow_set_match(sbflow, lflow->match); >>> sbrec_logical_flow_set_actions(sbflow, lflow->actions); >>> + sbrec_logical_flow_set_controller_meter(sbflow, lflow->ctrl_meter); >>> >>> /* Trim the source locator lflow->where, which looks something like >>> * "ovn/northd/ovn-northd.c:1234", down to just the part following >>> the >>> @@ -14082,6 +14102,8 @@ main(int argc, char *argv[]) >>> add_column_noalert(ovnsb_idl_loop.idl, >>> &sbrec_logical_flow_col_priority); >>> add_column_noalert(ovnsb_idl_loop.idl, &sbrec_logical_flow_col_match); >>> add_column_noalert(ovnsb_idl_loop.idl, >>> &sbrec_logical_flow_col_actions); >>> + add_column_noalert(ovnsb_idl_loop.idl, >>> + &sbrec_logical_flow_col_controller_meter); >>> >>> ovsdb_idl_add_table(ovnsb_idl_loop.idl, >>> &sbrec_table_logical_dp_group); >>> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema >>> index 29019809c..fb0f3a7c0 100644 >>> --- a/ovn-nb.ovsschema >>> +++ b/ovn-nb.ovsschema >>> @@ -1,7 +1,7 @@ >>> { >>> "name": "OVN_Northbound", >>> - "version": "5.31.0", >>> - "cksum": "2352750632 28701", >>> + "version": "5.32.0", >>> + "cksum": "2317195278 29378", >>> "tables": { >>> "NB_Global": { >>> "columns": { >>> @@ -30,6 +30,14 @@ >>> "ipsec": {"type": "boolean"}}, >>> "maxRows": 1, >>> "isRoot": true}, >>> + "Copp": { >>> + "columns": { >>> + "meters": { >>> + "type": {"key": "string", >>> + "value": "string", >>> + "min": 0, >>> + "max": "unlimited"}}}, >>> + "isRoot": true}, >>> "Logical_Switch": { >>> "columns": { >>> "name": {"type": "string"}, >>> @@ -58,6 +66,9 @@ >>> "refType": "weak"}, >>> "min": 0, >>> "max": "unlimited"}}, >>> + "copp": {"type": {"key": {"type": "uuid", "refTable": >>> "Copp", >>> + "refType": "weak"}, >>> + "min": 0, "max": 1}}, >>> "other_config": { >>> "type": {"key": "string", "value": "string", >>> "min": 0, "max": "unlimited"}}, >>> @@ -319,6 +330,9 @@ >>> "refType": "weak"}, >>> "min": 0, >>> "max": "unlimited"}}, >>> + "copp": {"type": {"key": {"type": "uuid", "refTable": >>> "Copp", >>> + "refType": "weak"}, >>> + "min": 0, "max": 1}}, >>> "options": { >>> "type": {"key": "string", >>> "value": "string", >>> diff --git a/ovn-nb.xml b/ovn-nb.xml >>> index feb38b5d3..0f03a12f2 100644 >>> --- a/ovn-nb.xml >>> +++ b/ovn-nb.xml >>> @@ -339,6 +339,68 @@ >>> >>> </table> >>> >>> + <table name="Copp" title="Control plane protection"> >>> + <p> >>> + This table is used to define control plane protection policies, i.e., >>> + associate entries from table <ref table="Meter"/> to control protocol >>> + names. >>> + </p> >>> + <column name="meters" key="arp"> >>> + Rate limiting meter for ARP packets (request/reply) used for learning >>> + neighbors. >>> + </column> >>> + <column name="meters" key="arp-resolve"> >>> + Rate limiting meter for packets that require resolving the next-hop >>> + (through ARP). >>> + </column> >>> + <column name="meters" key="dhcpv4-opts"> >>> + Rate limiting meter for packets that require adding DHCPv4 options. >>> + </column> >>> + <column name="meters" key="dhcpv6-opts"> >>> + Rate limiting meter for packets that require adding DHCPv6 options. >>> + </column> >>> + <column name="meters" key="dns"> >>> + Rate limiting meter for DNS query packets that need to be replied to. >>> + </column> >>> + <column name="meters" key="event-elb"> >>> + Rate limiting meter for empty load balancer events. >>> + </column> >>> + <column name="meters" key="icmp4-error"> >>> + Rate limiting meter for packets that require replying with an ICMP >>> + error. >>> + </column> >>> + <column name="meters" key="icmp6-error"> >>> + Rate limiting meter for packets that require replying with an ICMPv6 >>> + error. >>> + </column> >>> + <column name="meters" key="igmp"> >>> + Rate limiting meter for IGMP packets. >>> + </column> >>> + <column name="meters" key="nd-na"> >>> + Rate limiting meter for ND neighbor advertisement packets used for >>> + learning neighbors. >>> + </column> >>> + <column name="meters" key="nd-ns"> >>> + Rate limiting meter for ND neighbor solicitation packets used for >>> + learning neighbors. >>> + </column> >>> + <column name="meters" key="nd-ns-resolve"> >>> + Rate limiting meter for packets that require resolving the next-hop >>> + (through ND). >>> + </column> >>> + <column name="meters" key="nd-ra-opts"> >>> + Rate limiting meter for packets that require adding ND router >>> + advertisement options. >>> + </column> >>> + <column name="meters" key="tcp-reset"> >>> + Rate limiting meter for packets that require replying with TCP RST >>> + packet. >>> + </column> >>> + <column name="meters" key="bfd"> >>> + Rate limiting meter for BFD packets. >>> + </column> >>> + </table> >>> + >>> <table name="Logical_Switch" title="L2 logical switch"> >>> <p> >>> Each row represents one L2 logical switch. >>> @@ -572,6 +634,14 @@ >>> </column> >>> </group> >>> >>> + <column name="copp"> >>> + <p> >>> + The control plane protection policy from table <ref table="Copp"/> >>> + used for metering packets sent to <code>ovn-controller</code> from >>> + ports of this logical switch. >>> + </p> >>> + </column> >>> + >>> <group title="Other options"> >>> <column name="other_config" key="vlan-passthru" >>> type='{"type": "boolean"}'> >>> @@ -1929,6 +1999,14 @@ >>> </column> >>> </group> >>> >>> + <column name="copp"> >>> + <p> >>> + The control plane protection policy from table <ref table="Copp"/> >>> + used for metering packets sent to <code>ovn-controller</code> from >>> + logical ports of this router. >>> + </p> >>> + </column> >>> + >>> <group title="Options"> >>> <p> >>> Additional options for the logical router. >>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >>> index 32afb4fa8..7c18e6302 100644 >>> --- a/tests/ovn-northd.at >>> +++ b/tests/ovn-northd.at >> >> Should we be adding ovn-nbctl.at tests? Or do these tests belong in >> ovn-nbctl.at These tests don't test northd but rather test that things >> are written correctly to NBDB. > > I guess here we are testing values are properly written to NB db, so we are > testing both ovn-nbctl and the NB db interation, agree? Am I missing somthing?
Yes, but these types of tests are normally in ovn-nbctl.at. For example the following tests test that logical switches and switch ports are correctly added to the NB DB: https://github.com/ovn-org/ovn/blob/308e743736eb3a958c0e3892f1cb858f9af00018/tests/ovn-nbctl.at#L109 >> >> We probably should have tests (in ovn-northd.c) that configures NBDB and >> then we check that the SBDB has meter flows present? (and various >> negative tests as well) We don't have any tests that check that the generated SB flows have been added correctly. These belong in ovn-northd.at. >> >>> @@ -2649,6 +2649,55 @@ wait_row_count bfd 3 >>> AT_CLEANUP >>> ]) >>> >>> +OVN_FOR_EACH_NORTHD([ >>> +AT_SETUP([ovn -- check CoPP config]) >>> +AT_KEYWORDS([northd-CoPP]) >>> +ovn_start >>> + >>> +check ovn-nbctl --wait=sb lr-add r0 >>> +check ovn-nbctl --wait=sb lrp-add r0 r0-sw1 00:00:00:00:00:01 >>> 192.168.1.1/24 >>> +check ovn-nbctl --wait=sb ls-add sw1 >>> +check ovn-nbctl --wait=sb lsp-add sw1 sw1-r0 >>> +check ovn-nbctl --wait=sb lsp-set-type sw1-r0 router >>> +check ovn-nbctl --wait=sb lsp-set-options sw1-r0 router-port=r0-sw1 >>> +check ovn-nbctl --wait=sb lsp-set-addresses sw1-r0 00:00:00:00:00:01 >>> + >>> +ovn-nbctl --wait=hv meter-add meter0 drop 100 pktps 10 >>> +ovn-nbctl --wait=hv ls-copp-add sw1 event-elb meter0 >>> +AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl >>> +event-elb: meter0 >>> +]) >>> + >>> +ovn-nbctl --wait=hv meter-add meter1 drop 200 pktps 10 >>> +ovn-nbctl --wait=hv ls-copp-add sw1 arp meter1 >>> +AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl >>> +arp: meter1 >>> +event-elb: meter0 >>> +]) >>> + >>> +ovn-nbctl --wait=hv ls-copp-del sw1 arp >>> +AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl >>> +event-elb: meter0 >>> +]) >>> + >>> +ovn-nbctl --wait=hv ls-copp-del sw1 event-elb >>> +AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl >>> +]) >>> + >>> +ovn-nbctl --wait=hv lr-copp-add r0 bfd meter0 >>> +AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl >>> +bfd: meter0 >>> +]) >>> + >>> +ovn-nbctl --wait=hv lr-copp-add r0 igmp meter1 >>> +AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl >>> +bfd: meter0 >>> +igmp: meter1 >>> +]) >> >> What about a delete all case at the end >> >> ovn-nbctl lr-copp-del r0 >> >> Also, >> >> Can you test a few failure cases? For example, deleting a copp that is >> not there, adding a the same copp twice, etc > > ack, I will do in v2 > >> >>> + >>> +AT_CLEANUP >>> +]) >>> + >>> AT_SETUP([ovn -- check LSP attached to multiple LS]) >>> ovn_start >>> >>> diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml >>> index 03d47dba5..6c95e8104 100644 >>> --- a/utilities/ovn-nbctl.8.xml >>> +++ b/utilities/ovn-nbctl.8.xml >>> @@ -1131,6 +1131,122 @@ >>> </dd> >>> </dl> >>> >>> + <h1> Control Plane Protection Policy commands</h1> >>> + <p> >>> + These commands manages meters contained in <ref table="Copp"/> table >>> + linking them to logical datapaths through <code>copp</code> column in >>> + <ref table="Logical_Switch" /> or <ref table="Logical_Router" /> >>> tables. >>> + Protocol packets for which CoPP is enforced when sending packets to >>> + ovn-controller (if configured): >>> + <ul> >>> + <li>ARP</li> >>> + <li>ND_NS</li> >>> + <li>ND_NA</li> >>> + <li>ND_RA</li> >>> + <li>ND</li> >>> + <li>DNS</li> >>> + <li>IGMP</li> >>> + <li>packets that require ARP resolution before forwarding</li> >>> + <li>packets that require ND_NS before forwarding</li> >>> + <li>packets that need to be replied to with ICMP Errors</li> >>> + <li>packets that need to be replied to with TCP RST</li> >>> + <li>packets that need to be replied to with DHCP_OPTS</li> >>> + <li>BFD</li> >> >> Should event-elb be listed here? > > I think it is done in the subsequent patch > >> >>> + </ul> >>> + </p> >>> + >>> + <dl> >>> + <dt><code>ls-copp-add</code> <var>switch</var> <var>proto</var> >>> + <var>meter</var></dt> >>> + <dd> >>> + Adds the control <code>proto</code> to <code>meter</code> mapping >>> + to the <code>switch</code> control plane protection policy. If no >>> + policy exists yet, it creates one. If a mapping already existed for >>> + <code>proto</code>, this will overwrite it. >>> + </dd> >>> + >>> + <dt><code>ls-copp-del</code> <var>switch</var> >>> [<var>proto</var>]</dt> >>> + <dd> >>> + Removes the control <code>proto</code> mapping from the >>> + <code>switch</code> control plane protection policy. If >>> + <code>proto</code> is not specified, the whole control plane >>> + protection policy is destroyed. >>> + </dd> >>> + >>> + <dt><code>ls-copp-list</code> <var>switch</var></dt> >>> + <dd> >>> + Display the current control plane protection policy for >>> + <code>switch</code>. >>> + </dd> >>> + >>> + <dt><code>lsp-copp-add</code> <var>proto</var> <var>proto</var> >>> + <var>meter</var></dt> >>> + <dd> >>> + Adds the control <code>proto</code> to <code>meter</code> mapping >>> + to the <code>port</code> control plane protection policy. If no >>> + policy exists yet, it creates one. If a mapping already existed for >>> + <code>proto</code>, this will overwrite it. >>> + </dd> >>> + >>> + <dt><code>lsp-copp-del</code> <var>port</var> [<var>proto</var>]</dt> >>> + <dd> >>> + Removes the control <code>proto</code> mapping from the >>> + <code>port</code> control plane protection policy. If >>> + <code>proto</code> is not specified, the whole control plane >>> + protection policy is destroyed. >>> + </dd> >>> + <dt><code>lsp-copp-list</code> <var>port</var></dt> >>> + <dd> >>> + Display the current control plane protection policy for >>> + <code>port</code>. >>> + </dd> >>> + >>> + <dt><code>lr-copp-add</code> <var>router</var> <var>proto</var> >>> + <var>meter</var></dt> >>> + <dd> >>> + Adds the control <code>proto</code> to <code>meter</code> mapping >>> + to the <code>router</code> control plane protection policy. If no >>> + policy exists yet, it creates one. If a mapping already existed for >>> + <code>proto</code>, this will overwrite it. >>> + </dd> >>> + >>> + <dt><code>lr-copp-del</code> <var>router</var> >>> [<var>proto</var>]</dt> >>> + <dd> >>> + Removes the control <code>proto</code> mapping from the >>> + <code>router</code> control plane protection policy. If >>> + <code>proto</code> is not specified, the whole control plane >>> + protection policy is destroyed. >>> + </dd> >>> + >>> + <dt><code>lr-copp-list</code> <var>router</var></dt> >>> + <dd> >>> + Display the current control plane protection policy for >>> + <code>router</code>. >>> + </dd> >>> + >>> + <dt><code>lrp-copp-add</code> <var>proto</var> <var>proto</var> >>> + <var>meter</var></dt> >>> + <dd> >>> + Adds the control <code>proto</code> to <code>meter</code> mapping >>> + to the <code>port</code> control plane protection policy. If no >>> + policy exists yet, it creates one. If a mapping already existed for >>> + <code>proto</code>, this will overwrite it. >>> + </dd> >> >> I don't think the lrp-* and lsp-* variants are needed any more? > > ack, right. I will fix it in v2 > > Regards, > Lorenzo > >> >>> + >>> + <dt><code>lrp-copp-del</code> <var>port</var> [<var>proto</var>]</dt> >>> + <dd> >>> + Removes the control <code>proto</code> mapping from the >>> + <code>port</code> control plane protection policy. If >>> + <code>proto</code> is not specified, the whole control plane >>> + protection policy is destroyed. >>> + </dd> >>> + <dt><code>lrp-copp-list</code> <var>port</var></dt> >>> + <dd> >>> + Display the current control plane protection policy for >>> + <code>port</code>. >>> + </dd> >>> + </dl> >>> + >>> <h1>Database Commands</h1> >>> <p>These commands query and modify the contents of <code>ovsdb</code> >>> tables. >>> They are a slight abstraction of the <code>ovsdb</code> interface and >>> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c >>> index 042c21002..2299bc6ce 100644 >>> --- a/utilities/ovn-nbctl.c >>> +++ b/utilities/ovn-nbctl.c >>> @@ -27,6 +27,7 @@ >>> #include "jsonrpc.h" >>> #include "openvswitch/json.h" >>> #include "lib/acl-log.h" >>> +#include "lib/copp.h" >>> #include "lib/ovn-nb-idl.h" >>> #include "lib/ovn-util.h" >>> #include "memory.h" >>> @@ -835,6 +836,28 @@ chassis with optional PRIORITY to the HA chassis group >>> GRP\n\ >>> ha-chassis-group-remove-chassis GRP CHASSIS Removes the HA chassis\ >>> CHASSIS from the HA chassis group GRP\n\ >>> \n\ >>> +Control Plane Protection Policy commands:\n\ >>> + ls-copp-add SWITCH PROTO METER\n\ >>> + Add a copp policy for PROTO packets on >>> SWITCH\n\ >>> + based on an existing METER.\n\ >>> + ls-copp-del SWITCH [PROTO]\n\ >>> + Delete the copp policy for PROTO packets on\n\ >>> + SWITCH. If PROTO is not specified, delete >>> all\n\ >>> + copp policies on SWITCH.\n\ >>> + ls-copp-list SWITCH\n\ >>> + List all copp policies defined for control\n\ >>> + protocols on SWITCH.\n\ >>> + lr-copp-add ROUTER PROTO METER\n\ >>> + Add a copp policy for PROTO packets on >>> ROUTER\n\ >>> + based on an existing METER.\n\ >>> + lr-copp-del ROUTER [PROTO]\n\ >>> + Delete the copp policy for PROTO packets on\n\ >>> + ROUTER. If PROTO is not specified, delete >>> all\n\ >>> + copp policies on ROUTER.\n\ >>> + lr-copp-list ROUTER\n\ >>> + List all copp policies defined for control\n\ >>> + protocols on ROUTER.\n\ >>> +\n\ >>> %s\ >>> %s\ >>> \n\ >>> @@ -5711,6 +5734,138 @@ nbctl_lr_route_list(struct ctl_context *ctx) >>> free(ipv6_routes); >>> } >>> >>> +static void >>> +nbctl_ls_copp_add(struct ctl_context *ctx) >>> +{ >>> + const char *ls_name = ctx->argv[1]; >>> + const char *proto_name = ctx->argv[2]; >>> + const char *meter = ctx->argv[3]; >>> + >>> + char *error = copp_proto_validate(proto_name); >>> + if (error) { >>> + ctx->error = error; >>> + return; >>> + } >>> + >>> + const struct nbrec_logical_switch *ls = NULL; >>> + error = ls_by_name_or_uuid(ctx, ls_name, true, &ls); >>> + if (error) { >>> + ctx->error = error; >>> + return; >>> + } >>> + >>> + const struct nbrec_copp *copp = >>> + copp_add_meter(ctx, ls->copp, proto_name, meter); >>> + nbrec_logical_switch_set_copp(ls, copp); >>> +} >>> + >>> +static void >>> +nbctl_ls_copp_del(struct ctl_context *ctx) >>> +{ >>> + const char *ls_name = ctx->argv[1]; >>> + const char *proto_name = NULL; >>> + char *error; >>> + >>> + if (ctx->argc == 3) { >>> + proto_name = ctx->argv[2]; >>> + error = copp_proto_validate(proto_name); >>> + if (error) { >>> + ctx->error = error; >>> + return; >>> + } >>> + } >>> + >>> + const struct nbrec_logical_switch *ls = NULL; >>> + error = ls_by_name_or_uuid(ctx, ls_name, true, &ls); >>> + if (error) { >>> + ctx->error = error; >>> + return; >>> + } >>> + >>> + copp_del_meter(ls->copp, proto_name); >>> +} >>> + >>> +static void >>> +nbctl_ls_copp_list(struct ctl_context *ctx) >>> +{ >>> + const char *ls_name = ctx->argv[1]; >>> + >>> + const struct nbrec_logical_switch *ls = NULL; >>> + char *error = ls_by_name_or_uuid(ctx, ls_name, true, &ls); >>> + if (error) { >>> + ctx->error = error; >>> + return; >>> + } >>> + >>> + copp_list(ctx, ls->copp); >>> +} >>> + >>> +static void >>> +nbctl_lr_copp_add(struct ctl_context *ctx) >>> +{ >>> + const char *lr_name = ctx->argv[1]; >>> + const char *proto_name = ctx->argv[2]; >>> + const char *meter = ctx->argv[3]; >>> + >>> + char *error = copp_proto_validate(proto_name); >>> + if (error) { >>> + ctx->error = error; >>> + return; >>> + } >>> + >>> + const struct nbrec_logical_router *lr = NULL; >>> + error = lr_by_name_or_uuid(ctx, lr_name, true, &lr); >>> + if (error) { >>> + ctx->error = error; >>> + return; >>> + } >>> + >>> + const struct nbrec_copp *copp = >>> + copp_add_meter(ctx, lr->copp, proto_name, meter); >>> + nbrec_logical_router_set_copp(lr, copp); >>> +} >>> + >>> +static void >>> +nbctl_lr_copp_del(struct ctl_context *ctx) >>> +{ >>> + const char *lr_name = ctx->argv[1]; >>> + const char *proto_name = NULL; >>> + char *error; >>> + >>> + if (ctx->argc == 3) { >>> + proto_name = ctx->argv[2]; >>> + error = copp_proto_validate(proto_name); >>> + if (error) { >>> + ctx->error = error; >>> + return; >>> + } >>> + } >>> + >>> + const struct nbrec_logical_router *lr = NULL; >>> + error = lr_by_name_or_uuid(ctx, lr_name, true, &lr); >>> + if (error) { >>> + ctx->error = error; >>> + return; >>> + } >>> + >>> + copp_del_meter(lr->copp, proto_name); >>> +} >>> + >>> +static void >>> +nbctl_lr_copp_list(struct ctl_context *ctx) >>> +{ >>> + const char *lr_name = ctx->argv[1]; >>> + >>> + const struct nbrec_logical_router *lr = NULL; >>> + char *error = lr_by_name_or_uuid(ctx, lr_name, true, &lr); >>> + if (error) { >>> + ctx->error = error; >>> + return; >>> + } >>> + >>> + copp_list(ctx, lr->copp); >>> +} >>> + >>> static void >>> verify_connections(struct ctl_context *ctx) >>> { >>> @@ -6673,6 +6828,18 @@ static const struct ctl_command_syntax >>> nbctl_commands[] = { >>> {"dhcp-options-get-options", 1, 1, "DHCP_OPT_UUID", NULL, >>> nbctl_dhcp_options_get_options, NULL, "", RO }, >>> >>> + /* Control plane protection commands */ >>> + {"ls-copp-add", 3, 3, "SWITCH PROTO METER", NULL, nbctl_ls_copp_add, >>> NULL, >>> + "", RW}, >>> + {"ls-copp-del", 1, 2, "SWITCH [PROTO]", NULL, nbctl_ls_copp_del, NULL, >>> + "", RW}, >>> + {"ls-copp-list", 1, 1, "SWITCH", NULL, nbctl_ls_copp_list, NULL, "", >>> RO}, >>> + {"lr-copp-add", 3, 3, "ROUTER PROTO METER", NULL, nbctl_lr_copp_add, >>> NULL, >>> + "", RW}, >>> + {"lr-copp-del", 1, 2, "ROUTER [PROTO]", NULL, nbctl_lr_copp_del, NULL, >>> + "", RW}, >>> + {"lr-copp-list", 1, 1, "ROUTER", NULL, nbctl_lr_copp_list, NULL, "", >>> RO}, >>> + >>> /* Connection commands. */ >>> {"get-connection", 0, 0, "", pre_connection, cmd_get_connection, NULL, >>> "", RO}, >>> {"del-connection", 0, 0, "", pre_connection, cmd_del_connection, NULL, >>> "", RW}, >>> >> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev