On Wed, Jul 26, 2017 at 1:55 PM, Justin Pettit <jpet...@ovn.org> wrote:
>
> Signed-off-by: Justin Pettit <jpet...@ovn.org>
> ---
> NEWS | 1 +
> include/ovn/actions.h | 67 ++++++++++++-------
> ovn/controller/ovn-controller.8.xml | 9 +++
> ovn/controller/pinctrl.c | 38 +++++++++++
> ovn/lib/actions.c | 130
++++++++++++++++++++++++++++++++++++
> ovn/lib/automake.mk | 2 +
> ovn/lib/ovn-log.c | 69 +++++++++++++++++++
> ovn/lib/ovn-log.h | 52 +++++++++++++++
> ovn/northd/ovn-northd.c | 77 ++++++++++++++++++---
> ovn/ovn-nb.ovsschema | 15 ++++-
> ovn/ovn-nb.xml | 16 ++++-
> ovn/ovn-sb.xml | 32 +++++++++
> ovn/utilities/ovn-nbctl.8.xml | 35 +++++++---
> ovn/utilities/ovn-nbctl.c | 24 ++++++-
> ovn/utilities/ovn-trace.c | 19 ++++++
> tests/ovn.at | 105 +++++++++++++++++++++++++++++
> 16 files changed, 640 insertions(+), 51 deletions(-)
> create mode 100644 ovn/lib/ovn-log.c
> create mode 100644 ovn/lib/ovn-log.h
>
Thanks for the patch!
It seems "invalid" packets won't get logged in this patch. I think it would
be useful: in addition to log packets per-ACL, enable logging for packets
returned as "invalid" state from conntrack. It can be a global
configuration to enable/disable this behavior.
Please see my other comments inlined:
> static void
> +pinctrl_handle_log(struct dp_packet *packet OVS_UNUSED,
Why need this parameter if it is not useful?
> + const struct flow *headers,
> + struct ofpbuf *userdata)
> +{
> + struct log_pin_header *lph = ofpbuf_try_pull(userdata, sizeof *lph);
> + if (!lph) {
> + VLOG_WARN("log data missing");
> + return;
> + }
> +
> + int name_len = ntohs(lph->name_len);
> + char *name = xmalloc(name_len+1);
> + if (name_len) {
> + char *tmp_name = ofpbuf_try_pull(userdata, name_len);
> + if (!name) {
It should be: if (!tmp_name)
> + VLOG_WARN("name missing");
> + free(name);
> + return;
> + }
> + memcpy(name, tmp_name, name_len);
> + }
> + name[name_len] = '\0';
> +
> + char *packet_str = flow_to_string(headers, NULL);
> + VLOG_INFO("ACL name=%s, verdict=%s, severity=%s, packet=\"%s\"",
The log action may be more generic than just for ACL. So would it better to
avoid hardcode "ACL" here in the message? "ACL" could be indicated in
verdict instead, since there could be more use cases that need packet
logging in the future.
> + name_len ? name : "<unnamed>",
> + log_verdict_to_string(lph->verdict),
> + log_severity_to_string(lph->severity), packet_str);
> + free(packet_str);
> + free(name);
> +}
> +
> +enum log_verdict {
> + LOG_VERDICT_ALLOW,
> + LOG_VERDICT_DROP,
> + LOG_VERDICT_REJECT,
Better to be LOG_VERDICT_ACL_ALLOW, LOG_VERDICT_ACL_DROP,
LOG_VERDICT_ACL_REJECT, so that in the future we can support other use
cases for logging.
> + LOG_VERDICT_UNKNOWN = UINT8_MAX
> +};
> static void
> +build_acl_log(struct ds *actions, const struct nbrec_acl *acl)
> +{
> + if (!acl->log) {
> + return;
> + }
> +
> + ds_put_cstr(actions, "log(");
> +
> + if (acl->name) {
> + ds_put_format(actions, "name=\"%s\", ", acl->name);
Would it be good to use first byte of ACL uuid (which is also used as
stage_hint) when name is empty?
>
> + <column name="severity">
> <p>
> - Logging is not yet implemented.
> + When <ref column="log"/> is <code>true</code> indicates the
> + severity of the ACL. The severity levels match those of syslog
s/severity of the ACL/severity of the log/ because ACL doesn't have
severity.
> + with the following values (from more to less serious):
> + <code>alert</code>, <code>warning</code>, <code>notice</code>,
> + <code>info</code>, or <code>debug</code>. If a severity is not
> + specified, the default is <code>info</code>.
> </p>
> </column>
>
> @@ -332,7 +333,7 @@ Logical switch commands:\n\
> ls-list print the names of all logical switches\n\
> \n\
> ACL commands:\n\
> - acl-add SWITCH DIRECTION PRIORITY MATCH ACTION [log]\n\
> + acl-add [--log] SWITCH DIRECTION PRIORITY MATCH ACTION\n\
The optional "name" and "severity" are missing in the help message.
Acked-by: Han Zhou <zhou...@gmail.com>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
