[ovs-dev] [PATCH ovn 3/5] ovn-northd: Add CoPP policies for flows that punt packets to ovn-controller.
Lorenzo Bianconi
lorenzo.bianconi at redhat.com
Mon May 24 17:59:58 UTC 2021
> On 29/04/2021 18:04, Lorenzo Bianconi wrote:
> > From: Dumitru Ceara <dceara at redhat.com>
> >
> > Change the ovn-northd implementation to set the new 'controller_meter'
> > field for flows that need to punt packets to ovn-controller.
> >
> > Protocol packets for which CoPP is enforced when sending packets to
> > ovn-controller (if configured):
> > - ARP
> > - ND_NS
> > - ND_NA
> > - ND_RA
> > - DNS
> > - IGMP
> > - packets that require ARP resolution before forwarding
> > - packets that require ND_NS before forwarding
> > - packets that need to be replied to with ICMP Errors
> > - packets that need to be replied to with TCP RST
> > - packets that need to be replied to with DHCP_OPTS
> > - BFD
> >
> Add reject?
>
> Do you need to add event-elb?
ack, I will fix it in v2
>
> > Co-authored-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
> > Signed-off-by: Dumitru Ceara <dceara at redhat.com>
> > ---
> > lib/copp.c | 1 +
> > lib/copp.h | 1 +
> > northd/ovn-northd.c | 451 ++++++++++++++++++++++++--------------
> > ovn-nb.xml | 3 +
> > tests/atlocal.in | 3 +
> > tests/system-ovn.at | 119 ++++++++++
> > utilities/ovn-nbctl.8.xml | 1 +
> > 7 files changed, 417 insertions(+), 162 deletions(-)
> >
> > diff --git a/lib/copp.c b/lib/copp.c
> > index ac53a1094..7713046e5 100644
> > --- a/lib/copp.c
> > +++ b/lib/copp.c
> > @@ -37,6 +37,7 @@ static char *copp_proto_names[COPP_PROTO_MAX] = {
> > [COPP_ND_NS_RESOLVE] = "nd-ns-resolve",
> > [COPP_ND_RA_OPTS] = "nd-ra-opts",
> > [COPP_TCP_RESET] = "tcp-reset",
> > + [COPP_REJECT] = "reject",
> > [COPP_BFD] = "bfd",
> > };
> >
[...]
>
> Why do you add the same flow twice but one with a meter instead of
> modifying the flow above?
it is a bug :) thx for spotting it. I will fix in v2
>
> > const char *dns_action = "eth.dst <-> eth.src; ip4.src <-> ip4.dst; "
> > "udp.dst = udp.src; udp.src = 53; outport = inport; "
> > "flags.loopback = 1; output;";
> > @@ -7244,7 +7280,8 @@ build_lswitch_external_port(struct ovn_port *op,
> > static void
> > build_lswitch_destination_lookup_bmcast(struct ovn_datapath *od,
> > struct hmap *lflows,
> > - struct ds *actions)
> > + struct ds *actions,
> > + struct shash *meter_groups)
> > {
> > if (od->nbs) {
> >
> > @@ -7265,12 +7302,16 @@ build_lswitch_destination_lookup_bmcast(struct ovn_datapath *od,
> > }
> > ds_put_cstr(actions, "igmp;");
> > /* Punt IGMP traffic to controller. */
> > - ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
> > - "ip4 && ip.proto == 2", ds_cstr(actions));
> > + ovn_lflow_add_unique__(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
> > + "ip4 && ip.proto == 2", ds_cstr(actions),
> > + copp_meter_get(COPP_IGMP, od->nbs->copp,
> > + meter_groups));
> >
> > /* Punt MLD traffic to controller. */
> > - ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
> > - "mldv1 || mldv2", ds_cstr(actions));
> > + ovn_lflow_add_unique__(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
> > + "mldv1 || mldv2", ds_cstr(actions),
> > + copp_meter_get(COPP_IGMP, od->nbs->copp,
> > + meter_groups));
> >
> > /* Flood all IP multicast traffic destined to 224.0.0.X to all
> > * ports - RFC 4541, section 2.1.2, item 2.
> > @@ -8650,23 +8691,29 @@ add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
> > struct ds *match, struct ds *actions, int priority,
> > enum lb_snat_type snat_type, struct ovn_lb_vip *lb_vip,
> > const char *proto, struct nbrec_load_balancer *lb,
> > - struct shash *meter_groups, struct sset *nat_entries)
> > + struct shash *meter_groups, struct sset *nat_entries,
> > + bool reject)
> > {
> > build_empty_lb_event_flow(od, lflows, lb_vip, lb, S_ROUTER_IN_DNAT,
> > meter_groups);
> >
> > + const char *meter = NULL;
> > + if (reject) {
> > + meter = copp_meter_get(COPP_REJECT, od->nbr->copp, meter_groups);
> > + }
> > /* A match and actions for new connections. */
> > char *new_match = xasprintf("ct.new && %s", ds_cstr(match));
> > if (snat_type == FORCE_SNAT || snat_type == SKIP_SNAT) {
> > char *new_actions = xasprintf("flags.%s_snat_for_lb = 1; %s",
> > snat_type == SKIP_SNAT ? "skip" : "force",
> > ds_cstr(actions));
> > - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT, priority,
> > - new_match, new_actions, &lb->header_);
> > + ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_DNAT, priority,
> > + new_match, new_actions, meter, &lb->header_);
> > free(new_actions);
> > } else {
> > - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT, priority,
> > - new_match, ds_cstr(actions), &lb->header_);
> > + ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_DNAT, priority,
> > + new_match, ds_cstr(actions), meter,
> > + &lb->header_);
> > }
> >
> > /* A match and actions for established connections. */
> > @@ -8792,8 +8839,8 @@ build_lrouter_lb_flows(struct hmap *lflows, struct ovn_datapath *od,
> > struct ovn_lb_vip *lb_vip = &lb->vips[j];
> > struct ovn_northd_lb_vip *lb_vip_nb = &lb->vips_nb[j];
> > ds_clear(actions);
> > - build_lb_vip_actions(lb_vip, lb_vip_nb, actions,
> > - lb->selection_fields, false);
> > + bool reject = build_lb_vip_actions(lb_vip, lb_vip_nb, actions,
> > + lb->selection_fields, false);
> >
> > if (!sset_contains(&all_ips, lb_vip->vip_str)) {
> > sset_add(&all_ips, lb_vip->vip_str);
> > @@ -8858,7 +8905,7 @@ build_lrouter_lb_flows(struct hmap *lflows, struct ovn_datapath *od,
> > }
> > add_router_lb_flow(lflows, od, match, actions, prio,
> > snat_type, lb_vip, proto, nb_lb,
> > - meter_groups, nat_entries);
> > + meter_groups, nat_entries, reject);
> > }
> > }
> > sset_destroy(&all_ips);
> > @@ -9097,7 +9144,7 @@ build_lrouter_nd_flow(struct ovn_datapath *od, struct ovn_port *op,
> > const char *sn_ip_address, const char *eth_addr,
> > struct ds *extra_match, bool drop, uint16_t priority,
> > const struct ovsdb_idl_row *hint,
> > - struct hmap *lflows)
> > + struct hmap *lflows, struct shash *meter_groups)
> > {
> > struct ds match = DS_EMPTY_INITIALIZER;
> > struct ds actions = DS_EMPTY_INITIALIZER;
> > @@ -9119,6 +9166,8 @@ build_lrouter_nd_flow(struct ovn_datapath *od, struct ovn_port *op,
> >
> > if (drop) {
> > ds_put_format(&actions, "drop;");
> > + ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_IP_INPUT, priority,
> > + ds_cstr(&match), ds_cstr(&actions), hint);
> > } else {
> > ds_put_format(&actions,
> > "%s { "
> > @@ -9135,11 +9184,13 @@ build_lrouter_nd_flow(struct ovn_datapath *od, struct ovn_port *op,
> > ip_address,
> > ip_address,
> > eth_addr);
> > + ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_IP_INPUT, priority,
> > + ds_cstr(&match), ds_cstr(&actions),
> > + copp_meter_get(COPP_ND_NA, od->nbr->copp,
> > + meter_groups),
> > + hint);
> > }
> >
> > - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_IP_INPUT, priority,
> > - ds_cstr(&match), ds_cstr(&actions), hint);
> > -
> > ds_destroy(&match);
> > ds_destroy(&actions);
> > }
> > @@ -9147,7 +9198,8 @@ build_lrouter_nd_flow(struct ovn_datapath *od, struct ovn_port *op,
> > static void
> > build_lrouter_nat_arp_nd_flow(struct ovn_datapath *od,
> > struct ovn_nat *nat_entry,
> > - struct hmap *lflows)
> > + struct hmap *lflows,
> > + struct shash *meter_groups)
> > {
[...]
> > +ADD_NAMESPACES(server)
> > +NS_CHECK_EXEC([server], [ip link set dev lo up])
> > +ADD_VETH(s1, server, br-ext, "172.16.1.50/24", "f0:00:00:01:02:05", \
> > + "172.16.1.1")
> > +NS_CHECK_EXEC([server], [ip addr add 1000::b/64 dev s1])
>
> Why do you add this IPv6 address here?
it is not necessary, I will remove it.
>
> > +
> > +AT_CHECK([ovs-vsctl set Open_vSwitch . external-ids:ovn-bridge-mappings=phynet:br-ext])
> > +check ovn-nbctl lsp-add public public1 \
> > + -- lsp-set-addresses public1 unknown \
> > + -- lsp-set-type public1 localnet \
> > + -- lsp-set-options public1 network_name=phynet
> > +
> > +NS_CHECK_EXEC([sw01], [tcpdump -nni sw01 icmp -Q in > reject.pcap &])
> > +check ovn-nbctl meter-add acl-meter drop 1 pktps 0
> > +check ovn-nbctl --wait=hv ls-copp-add sw0 reject acl-meter
> > +check ovn-nbctl acl-add sw0 from-lport 1002 'inport == "sw01" && ip && udp' reject
> > +
> > +AT_CHECK([ovn-nbctl ls-copp-list sw0], [0], [dnl
> > +reject: acl-meter
> > +])
> > +
> > +ip netns exec sw01 scapy -H <<-EOF
> > +p = IP(src="192.168.1.2", dst="192.168.1.1")/ UDP(dport = 12345) / Raw(b"X"*64)
> > +send (p, iface='sw01', loop = 0, verbose = 0, count = 20)
> > +EOF
> > +
> > +sleep 2> +kill $(pidof tcpdump)
> > +
> > +# 1pps + 1 burst size
> > +OVS_WAIT_UNTIL([
> > + n_reject=$(grep unreachable reject.pcap | wc -l)
> > + test "${n_reject}" = "2"
> > +])
> > +
> > +NS_CHECK_EXEC([server], [tcpdump -nni s1 arp[[24:4]]=0xac100164 > arp.pcap &])
> > +check ovn-nbctl meter-add arp-meter drop 1 pktps 0
> > +check ovn-nbctl --wait=hv lr-copp-add R1 arp-resolve arp-meter
> > +AT_CHECK([ovn-nbctl lr-copp-list R1], [0], [dnl
> > +arp-resolve: arp-meter
> > +])
> > +
> > +ip netns exec sw01 scapy -H <<-EOF
> > +p = IP(src="192.168.1.2", dst="172.16.1.100")/ TCP(dport = 80, flags="S") / Raw(b"X"*64)
> > +send (p, iface='sw01', loop = 0, verbose = 0, count = 100)
> > +EOF
> > +
> > +sleep 2
> > +kill $(pidof tcpdump)
> > +
> > +# 1pps + 1 burst size
> > +OVS_WAIT_UNTIL([
> > + n_arp=$(grep ARP arp.pcap | wc -l)
> > + test "${n_arp}" = "2"
> > +])
> > +
> > +kill $(pidof ovn-controller)
> > +
> > +as ovn-sb
> > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > +
> > +as ovn-nb
> > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > +
> > +as northd
> > +OVS_APP_EXIT_AND_WAIT([ovn-northd])
> > +
> > +as
> > +OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d
> > +/.*terminating with signal 15.*/d"])
> > +AT_CLEANUP
>
> Did you test this with the meters disabled? Does it behave as expected?
yes, but I will cover it in the test case.
Regards,
Lorenzo
>
> > +])
> > diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
> > index 6c95e8104..5f5f71015 100644
> > --- a/utilities/ovn-nbctl.8.xml
> > +++ b/utilities/ovn-nbctl.8.xml
> > @@ -1151,6 +1151,7 @@
> > <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>packets that trigger a reject action</li>
> > <li>BFD</li>
> > </ul>
> > </p>
> >
>
More information about the dev
mailing list