On Tue, Feb 15, 2022 at 12:18 PM Dumitru Ceara <[email protected]> wrote:
>
> It's useful to differentiate between ingress and egress pipelines in the
> ACL logs. To achieve this we determine the direction by interpreting the
> openflow table ID when processing packets punted to pinctrl by "log()"
> action.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1992641
> Acked-by: Numan Siddique <[email protected]>
> Signed-off-by: Dumitru Ceara <[email protected]>
Thanks. I went ahead and applied this patch to the main branch.
Numan
> ---
> v5:
> - Rebased and fixed minor conflict in NEWS.
> v4:
> - Log direction as from-lport/to-lport.
> - Add Numan's ack.
> v3: Simplify the patch as suggested by Numan; detect pipeline based on
> openflow table id.
> v2: Add the "direction" after the "verdict" and "severity" fields in the
> ACL logs.
> ---
> NEWS | 2 ++
> controller/pinctrl.c | 4 ++-
> lib/acl-log.c | 8 +++--
> lib/acl-log.h | 3 +-
> tests/ovn.at | 68 ++++++++++++++++++++++++++++++++++---------
> utilities/ovn-trace.c | 9 ++++--
> 6 files changed, 73 insertions(+), 21 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 984e9977e9..a309fe8df7 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -10,6 +10,8 @@ Post v21.12.0
> - Support version pinning between ovn-northd and ovn-controller-vtep as an
> option. If the option is enabled and the versions don't match,
> ovn-controller-vtep will not process any DB changes.
> + - When configured to log packtes matching ACLs, log the direction (logical
> + pipeline) too.
>
> OVN v21.12.0 - 22 Dec 2021
> --------------------------
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index 1b8b47523f..2dd1bc7bd5 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -3166,7 +3166,9 @@ process_packet_in(struct rconn *swconn, const struct
> ofp_header *msg)
> break;
>
> case ACTION_OPCODE_LOG:
> - handle_acl_log(&headers, &userdata);
> + handle_acl_log(&headers, &userdata,
> + pin.table_id < OFTABLE_LOG_EGRESS_PIPELINE
> + ? "from-lport" : "to-lport");
> break;
>
> case ACTION_OPCODE_PUT_ND_RA_OPTS:
> diff --git a/lib/acl-log.c b/lib/acl-log.c
> index 220b6dc30e..9530dd7635 100644
> --- a/lib/acl-log.c
> +++ b/lib/acl-log.c
> @@ -76,7 +76,8 @@ log_severity_from_string(const char *name)
> }
>
> void
> -handle_acl_log(const struct flow *headers, struct ofpbuf *userdata)
> +handle_acl_log(const struct flow *headers, struct ofpbuf *userdata,
> + const char *direction)
> {
> if (!VLOG_IS_INFO_ENABLED()) {
> return;
> @@ -94,9 +95,10 @@ handle_acl_log(const struct flow *headers, struct ofpbuf
> *userdata)
> struct ds ds = DS_EMPTY_INITIALIZER;
> ds_put_cstr(&ds, "name=");
> json_string_escape(name_len ? name : "<unnamed>", &ds);
> - ds_put_format(&ds, ", verdict=%s, severity=%s: ",
> + ds_put_format(&ds, ", verdict=%s, severity=%s, direction=%s: ",
> log_verdict_to_string(lph->verdict),
> - log_severity_to_string(lph->severity));
> + log_severity_to_string(lph->severity),
> + direction);
> flow_format(&ds, headers, NULL);
>
> VLOG_INFO("%s", ds_cstr(&ds));
> diff --git a/lib/acl-log.h b/lib/acl-log.h
> index 4f23f790dd..da7fa2f02c 100644
> --- a/lib/acl-log.h
> +++ b/lib/acl-log.h
> @@ -49,6 +49,7 @@ const char *log_verdict_to_string(uint8_t verdict);
> const char *log_severity_to_string(uint8_t severity);
> uint8_t log_severity_from_string(const char *name);
>
> -void handle_acl_log(const struct flow *headers, struct ofpbuf *userdata);
> +void handle_acl_log(const struct flow *headers, struct ofpbuf *userdata,
> + const char *direction);
>
> #endif /* lib/acl-log.h */
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 184594831a..47e3e18a99 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -8965,33 +8965,59 @@ ovn-nbctl lsp-set-addresses lp2 $lp2_mac
> ovn-nbctl --wait=sb sync
> wait_for_ports_up
>
> -ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==80' drop
> -ovn-nbctl --log --severity=alert --name=drop-flow acl-add lsw0 to-lport 1000
> 'tcp.dst==81' drop
> +ovn-nbctl acl-add lsw0 from-lport 1000 'tcp.dst==80' drop
> +ovn-nbctl --log --severity=alert --name=drop-flow acl-add lsw0 from-lport
> 1000 'tcp.dst==81' drop
> +
> +ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==180' drop
> +ovn-nbctl --log --severity=alert --name=drop-flow acl-add lsw0 to-lport 1000
> 'tcp.dst==181' drop
> +
> +ovn-nbctl acl-add lsw0 from-lport 1000 'tcp.dst==82' allow
> +ovn-nbctl --log --severity=info --name=allow-flow acl-add lsw0 from-lport
> 1000 'tcp.dst==83' allow
>
> ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==82' allow
> ovn-nbctl --log --severity=info --name=allow-flow acl-add lsw0 to-lport 1000
> 'tcp.dst==83' allow
>
> +ovn-nbctl acl-add lsw0 from-lport 1000 'tcp.dst==84' allow-related
> +ovn-nbctl --log acl-add lsw0 from-lport 1000 'tcp.dst==85' allow-related
> +
> ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==84' allow-related
> ovn-nbctl --log acl-add lsw0 to-lport 1000 'tcp.dst==85' allow-related
>
> -ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==86' reject
> -ovn-nbctl --wait=hv --log --severity=alert --name=reject-flow acl-add lsw0
> to-lport 1000 'tcp.dst==87' reject
> +ovn-nbctl acl-add lsw0 from-lport 1000 'tcp.dst==86' reject
> +ovn-nbctl --log --severity=alert --name=reject-flow acl-add lsw0 from-lport
> 1000 'tcp.dst==87' reject
> +
> +ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==186' reject
> +ovn-nbctl --log --severity=alert --name=reject-flow acl-add lsw0 to-lport
> 1000 'tcp.dst==187' reject
> +
> +ovn-nbctl --wait=hv sync
>
> ovn-sbctl dump-flows > sbflows
> AT_CAPTURE_FILE([sbflows])
>
> -# Send packet that should be dropped without logging.
> +# Send packet that should be dropped without logging in the ingress pipeline.
> packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac &&
> ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip &&
> tcp && tcp.flags==2 && tcp.src==4360 && tcp.dst==80"
> as hv ovs-appctl -t ovn-controller inject-pkt "$packet"
>
> -# Send packet that should be dropped with logging.
> +# Send packet that should be dropped with logging in the ingress pipeline.
> packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac &&
> ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip &&
> tcp && tcp.flags==2 && tcp.src==4361 && tcp.dst==81"
> as hv ovs-appctl -t ovn-controller inject-pkt "$packet"
>
> +# Send packet that should be dropped without logging in the eggress pipeline.
> +packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac &&
> + ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip &&
> + tcp && tcp.flags==2 && tcp.src==4360 && tcp.dst==180"
> +as hv ovs-appctl -t ovn-controller inject-pkt "$packet"
> +
> +# Send packet that should be dropped with logging in the egress pipeline.
> +packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac &&
> + ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip &&
> + tcp && tcp.flags==2 && tcp.src==4361 && tcp.dst==181"
> +as hv ovs-appctl -t ovn-controller inject-pkt "$packet"
> +
> # Send packet that should be allowed without logging.
> packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac &&
> ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip &&
> @@ -9016,25 +9042,41 @@ packet="inport==\"lp1\" && eth.src==$lp1_mac &&
> eth.dst==$lp2_mac &&
> tcp && tcp.flags==2 && tcp.src==4365 && tcp.dst==85"
> as hv ovs-appctl -t ovn-controller inject-pkt "$packet"
>
> -# Send packet that should be rejected without logging.
> +# Send packet that should be rejected without logging in the ingress
> pipeline.
> packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac &&
> ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip &&
> tcp && tcp.flags==2 && tcp.src==4366 && tcp.dst==86"
> as hv ovs-appctl -t ovn-controller inject-pkt "$packet"
>
> -# Send packet that should be rejected with logging.
> +# Send packet that should be rejected with logging in the ingress pipeline.
> packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac &&
> ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip &&
> tcp && tcp.flags==2 && tcp.src==4367 && tcp.dst==87"
> as hv ovs-appctl -t ovn-controller inject-pkt "$packet"
>
> -OVS_WAIT_UNTIL([ test 4 = $(grep -c 'acl_log' hv/ovn-controller.log) ])
> +# Send packet that should be rejected without logging in the egress pipeline.
> +packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac &&
> + ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip &&
> + tcp && tcp.flags==2 && tcp.src==4366 && tcp.dst==186"
> +as hv ovs-appctl -t ovn-controller inject-pkt "$packet"
> +
> +# Send packet that should be rejected with logging in the egress pipeline.
> +packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac &&
> + ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip &&
> + tcp && tcp.flags==2 && tcp.src==4367 && tcp.dst==187"
> +as hv ovs-appctl -t ovn-controller inject-pkt "$packet"
> +
> +OVS_WAIT_UNTIL([ test 8 = $(grep -c 'acl_log' hv/ovn-controller.log) ])
>
> AT_CHECK([grep 'acl_log' hv/ovn-controller.log | sed 's/.*name=/name=/'],
> [0], [dnl
> -name="drop-flow", verdict=drop, severity=alert:
> tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4361,tp_dst=81,tcp_flags=syn
> -name="allow-flow", verdict=allow, severity=info:
> tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4363,tp_dst=83,tcp_flags=syn
> -name="<unnamed>", verdict=allow, severity=info:
> tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4365,tp_dst=85,tcp_flags=syn
> -name="reject-flow", verdict=reject, severity=alert:
> tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4367,tp_dst=87,tcp_flags=syn
> +name="drop-flow", verdict=drop, severity=alert, direction=from-lport:
> tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4361,tp_dst=81,tcp_flags=syn
> +name="drop-flow", verdict=drop, severity=alert, direction=to-lport:
> tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4361,tp_dst=181,tcp_flags=syn
> +name="allow-flow", verdict=allow, severity=info, direction=from-lport:
> tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4363,tp_dst=83,tcp_flags=syn
> +name="allow-flow", verdict=allow, severity=info, direction=to-lport:
> tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4363,tp_dst=83,tcp_flags=syn
> +name="<unnamed>", verdict=allow, severity=info, direction=from-lport:
> tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4365,tp_dst=85,tcp_flags=syn
> +name="<unnamed>", verdict=allow, severity=info, direction=to-lport:
> tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4365,tp_dst=85,tcp_flags=syn
> +name="reject-flow", verdict=reject, severity=alert, direction=from-lport:
> tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4367,tp_dst=87,tcp_flags=syn
> +name="reject-flow", verdict=reject, severity=alert, direction=to-lport:
> tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4367,tp_dst=187,tcp_flags=syn
> ])
>
> OVN_CLEANUP([hv])
> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> index 0795913d3e..ece5803f29 100644
> --- a/utilities/ovn-trace.c
> +++ b/utilities/ovn-trace.c
> @@ -2457,12 +2457,14 @@ execute_select(const struct ovnact_select *select,
>
> static void
> execute_log(const struct ovnact_log *log, struct flow *uflow,
> - struct ovs_list *super)
> + struct ovs_list *super, const char *direction)
> {
> char *packet_str = flow_to_string(uflow, NULL);
> ovntrace_node_append(super, OVNTRACE_NODE_TRANSFORMATION,
> - "LOG: ACL name=%s, verdict=%s, severity=%s,
> packet=\"%s\"",
> + "LOG: ACL name=%s, direction=%s, verdict=%s, "
> + "severity=%s, packet=\"%s\"",
> log->name ? log->name : "<unnamed>",
> + direction,
> log_verdict_to_string(log->verdict),
> log_severity_to_string(log->severity),
> packet_str);
> @@ -2726,7 +2728,8 @@ trace_actions(const struct ovnact *ovnacts, size_t
> ovnacts_len,
> break;
>
> case OVNACT_LOG:
> - execute_log(ovnact_get_LOG(a), uflow, super);
> + execute_log(ovnact_get_LOG(a), uflow, super,
> + pipeline == OVNACT_P_INGRESS ? "IN" : "OUT");
> break;
>
> case OVNACT_SET_METER:
> --
> 2.27.0
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
