diff mbox

[ovs-dev] ovn: Add support for ACL logging.

Message ID 1501102514-92293-1-git-send-email-jpettit@ovn.org
State Changes Requested
Headers show

Commit Message

Justin Pettit July 26, 2017, 8:55 p.m. UTC
Signed-off-by: Justin Pettit <jpettit@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

Comments

Han Zhou July 27, 2017, 11:03 p.m. UTC | #1
On Wed, Jul 26, 2017 at 1:55 PM, Justin Pettit <jpettit@ovn.org> wrote:
>
> Signed-off-by: Justin Pettit <jpettit@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 <zhouhan@gmail.com>
Justin Pettit July 28, 2017, 12:59 a.m. UTC | #2
> On Jul 27, 2017, at 4:03 PM, Han Zhou <zhouhan@gmail.com> wrote:
> 
> 
> 
> On Wed, Jul 26, 2017 at 1:55 PM, Justin Pettit <jpettit@ovn.org> wrote:
> >
> > Signed-off-by: Justin Pettit <jpettit@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!

Thanks for your patience on it.

> 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.

That's a great point.  I'd meant to do that when I started this series, but forgot along the way.  I think we need more than just logging for invalid packets, though, so I'll add that as a follow-up patch.

> >  static void
> > +pinctrl_handle_log(struct dp_packet *packet OVS_UNUSED,
> 
> Why need this parameter if it is not useful?

Fixed.

> > +                   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)

Good catch.  Fixed.

> > +            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.

These are part of the ACL infrastructure, so I'd think ACL makes sense.  I also wanted to give some sort of handle that someone can easily grep through the logs.  Do you have a specific suggestion?

> > +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.

Do you have an example or two of the type of thing you think we might need in the future?  If it needs to be that flexible, we could just pass around strings, but I do worry about passing too much data through the datapath upcall mechanism.

> >  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?

That's an interesting idea, but I have mixed feelings about it.  The first byte would only allow 16 different identifiers, but I think generally people have many more.  We could use more bytes from the UUID, but these logs could last quite a while, so it may cause confusion if they've changed, and people are trying to correlate them later.  Obviously, if they really want to correlate them, they should use a name, but I wonder if it would be just useful enough to be frustrating.

> >
> > +    <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.

I'm worried that that phrasing may indicate that it's the level it will logged at, when it's really just an indication by the user about how concerned they are with this ACL (or whatever you want to call it) entry.

> > @@ -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.

Yes, I was running out of space on an 80-column terminal.  I think we have other examples of dropping non-essential parameters.  I'll poke around to see what other project do.

> Acked-by: Han Zhou <zhouhan@gmail.com>

Thanks!

--Justin
Han Zhou July 28, 2017, 2:23 a.m. UTC | #3
On Thu, Jul 27, 2017 at 5:59 PM, Justin Pettit <jpettit@ovn.org> wrote:
>
> > 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.
>
> That's a great point.  I'd meant to do that when I started this series,
but forgot along the way.  I think we need more than just logging for
invalid packets, though, so I'll add that as a follow-up patch.
>
Sure, I am ok with a follow-up patch. But I wonder what else is needed for
invalid packets beside logging? We are dropping them already.

> > >  static void
> > > +pinctrl_handle_log(struct dp_packet *packet OVS_UNUSED,
> >
> > Why need this parameter if it is not useful?
>
> Fixed.
>
> > > +                   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)
>
> Good catch.  Fixed.
>
> > > +            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.
>
> These are part of the ACL infrastructure, so I'd think ACL makes sense.
I also wanted to give some sort of handle that someone can easily grep
through the logs.  Do you have a specific suggestion?
>
I'd suggest to have a more general keyword e.g. "LOG", and include the
"ACL" keyword in the verdict itself, i.e. generated by
log_verdict_to_string(). So user can grep "LOG" for all packet logging, and
grep "ACL" for all ACL related packet logging.

> > > +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.
>
> Do you have an example or two of the type of thing you think we might
need in the future?  If it needs to be that flexible, we could just pass
around strings, but I do worry about passing too much data through the
datapath upcall mechanism.
>
I have no real example yet. But since the action is defined as "log"
instead of "acl_log", I think it makes sense to be more generic. I don't
think we need to be more flexible but just make it more extensible. I don't
have strong opinion though. Maybe we can change it in the future if new
type of logging is needed. Or maybe there is use case from someone else?

> > >  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?
>
> That's an interesting idea, but I have mixed feelings about it.  The
first byte would only allow 16 different identifiers, but I think generally
people have many more.  We could use more bytes from the UUID, but these
logs could last quite a while, so it may cause confusion if they've
changed, and people are trying to correlate them later.  Obviously, if they
really want to correlate them, they should use a name, but I wonder if it
would be just useful enough to be frustrating.
>
Sorry, I meant first 8 bytes, not just one :). Existing deployment may
already (e.g. openstack) stored information in external-ids, which could be
used to correlate. However, I agree with you that it is better to have
exactly one behavior.

> > >
> > > +    <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.
>
> I'm worried that that phrasing may indicate that it's the level it will
logged at, when it's really just an indication by the user about how
concerned they are with this ACL (or whatever you want to call it) entry.

OK. I don't know why I was under the impression it was for the log level.
Checked the code again and I realized it is actually just a keyword in the
log. Now I am not sure if this key word is really useful, because I really
don't think ACL has severity. If user is more concerned about one ACL over
another, they can anyway figure out from the correlation.

BTW, I have another point for this code:
+    VLOG_INFO("ACL name=%s, verdict=%s, severity=%s, packet=\"%s\"",
+              name_len ? name : "<unnamed>",
+              log_verdict_to_string(lph->verdict),
+              log_severity_to_string(lph->severity), packet_str);

I think we need ratelimit for the log itself. It can be configurable
through local ovsdb for the ovn-controller. It is not critical, but I think
it is good to have. (I have some code for that already so I can come up
with a follow-up patch if you don't mind).

>
> > > @@ -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.
>
> Yes, I was running out of space on an 80-column terminal.  I think we
have other examples of dropping non-essential parameters.  I'll poke around
to see what other project do.
>
> > Acked-by: Han Zhou <zhouhan@gmail.com>
>
> Thanks!
>
> --Justin
>
>
Ben Pfaff July 28, 2017, 5:05 p.m. UTC | #4
On Thu, Jul 27, 2017 at 05:59:54PM -0700, Justin Pettit wrote:
> 
> > On Jul 27, 2017, at 4:03 PM, Han Zhou <zhouhan@gmail.com> wrote:
> > 
> > 
> > 
> > On Wed, Jul 26, 2017 at 1:55 PM, Justin Pettit <jpettit@ovn.org> wrote:
> > > @@ -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.
> 
> Yes, I was running out of space on an 80-column terminal.  I think we
> have other examples of dropping non-essential parameters.  I'll poke
> around to see what other project do.

Perhaps abbreviations:
    acl-add [--log] LS DIR PRI MATCH ACTION NAME SEVERITY
or similar.

(However, this is getting more and more awful and maybe we should
support a key=value format.)
Gurucharan Shetty July 28, 2017, 7:06 p.m. UTC | #5
On 28 July 2017 at 10:05, Ben Pfaff <blp@ovn.org> wrote:

> On Thu, Jul 27, 2017 at 05:59:54PM -0700, Justin Pettit wrote:
> >
> > > On Jul 27, 2017, at 4:03 PM, Han Zhou <zhouhan@gmail.com> wrote:
> > >
> > >
> > >
> > > On Wed, Jul 26, 2017 at 1:55 PM, Justin Pettit <jpettit@ovn.org>
> wrote:
> > > > @@ -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.
> >
> > Yes, I was running out of space on an 80-column terminal.  I think we
> > have other examples of dropping non-essential parameters.  I'll poke
> > around to see what other project do.
>
> Perhaps abbreviations:
>     acl-add [--log] LS DIR PRI MATCH ACTION NAME SEVERITY
> or similar.
>
> (However, this is getting more and more awful and maybe we should
> support a key=value format.)
>

A little unrelated, but it would have beeen nice if acl-add returns a uuid.
Now, after a acl-add we need to be smart trying to figure out the UUID and
add any external-ids to it.


> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Justin Pettit July 28, 2017, 10:46 p.m. UTC | #6
> On Jul 27, 2017, at 7:23 PM, Han Zhou <zhouhan@gmail.com> wrote:
> 
> On Thu, Jul 27, 2017 at 5:59 PM, Justin Pettit <jpettit@ovn.org> wrote:
> 
> > > 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.
> >
> > These are part of the ACL infrastructure, so I'd think ACL makes sense.  I also wanted to give some sort of handle that someone can easily grep through the logs.  Do you have a specific suggestion?
> >
> I'd suggest to have a more general keyword e.g. "LOG", and include the "ACL" keyword in the verdict itself, i.e. generated by log_verdict_to_string(). So user can grep "LOG" for all packet logging, and grep "ACL" for all ACL related packet logging.

I've changed the module from "pinctrl" to "acl_log", which is clearer and provides an easy handle for locating.  I dropped "ACL" from the log message.

> > > > +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.
> >
> > Do you have an example or two of the type of thing you think we might need in the future?  If it needs to be that flexible, we could just pass around strings, but I do worry about passing too much data through the datapath upcall mechanism.
> >
> I have no real example yet. But since the action is defined as "log" instead of "acl_log", I think it makes sense to be more generic. I don't think we need to be more flexible but just make it more extensible. I don't have strong opinion though. Maybe we can change it in the future if new type of logging is needed. Or maybe there is use case from someone else?

This is all part of the ACL subsystem, so I think using "acl" in the name makes sense.  I'm worried that "log" is a bit too generic.

> > > >  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?
> >
> > That's an interesting idea, but I have mixed feelings about it.  The first byte would only allow 16 different identifiers, but I think generally people have many more.  We could use more bytes from the UUID, but these logs could last quite a while, so it may cause confusion if they've changed, and people are trying to correlate them later.  Obviously, if they really want to correlate them, they should use a name, but I wonder if it would be just useful enough to be frustrating.
> >
> Sorry, I meant first 8 bytes, not just one :). Existing deployment may already (e.g. openstack) stored information in external-ids, which could be used to correlate. However, I agree with you that it is better to have exactly one behavior.

The row UUIDs are not obviously visible in most interactions with the ACLs.  As Guru suggested, it might be nice to expose them more generally.  I think this would make sense to do in a follow-on patch.  I can look at adding that.

> > > >
> > > > +    <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.
> >
> > I'm worried that that phrasing may indicate that it's the level it will logged at, when it's really just an indication by the user about how concerned they are with this ACL (or whatever you want to call it) entry.
> 
> OK. I don't know why I was under the impression it was for the log level. Checked the code again and I realized it is actually just a keyword in the log. Now I am not sure if this key word is really useful, because I really don't think ACL has severity. If user is more concerned about one ACL over another, they can anyway figure out from the correlation.

I've seen other system use this.  I think it can be helpful for looking for serious violations without having to encode that in the name.  Also, I was planning to use them as part of the decision to log when we eventually extend logging beyond just writing into ovn-controller.log.

> BTW, I have another point for this code:
> +    VLOG_INFO("ACL name=%s, verdict=%s, severity=%s, packet=\"%s\"",
> +              name_len ? name : "<unnamed>",
> +              log_verdict_to_string(lph->verdict),
> +              log_severity_to_string(lph->severity), packet_str);
> 
> I think we need ratelimit for the log itself. It can be configurable through local ovsdb for the ovn-controller. It is not critical, but I think it is good to have. (I have some code for that already so I can come up with a follow-up patch if you don't mind).

If you want to propose a follow-on patch, that would be great.

Thanks,

--Justin
diff mbox

Patch

diff --git a/NEWS b/NEWS
index b2deac57d6bd..facea0228d3a 100644
--- a/NEWS
+++ b/NEWS
@@ -48,6 +48,7 @@  Post-v2.7.0
      * Multiple chassis may now be specified for L3 gateways.  When more than
        one chassis is specified, OVN will manage high availability for that
        gateway.
+     * Add support for ACL logging.
    - Tracing with ofproto/trace now traces through recirculation.
    - OVSDB:
      * New support for role-based access control (see ovsdb-server(1)).
diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 9e4a5c5ab1e8..b88effee7437 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -48,30 +48,31 @@  struct simap;
  *    "ovnact".  The structure must have a fixed length, that is, it may not
  *    end with a flexible array member.
  */
-#define OVNACTS                                     \
-    OVNACT(OUTPUT,        ovnact_null)              \
-    OVNACT(NEXT,          ovnact_next)              \
-    OVNACT(LOAD,          ovnact_load)              \
-    OVNACT(MOVE,          ovnact_move)              \
-    OVNACT(EXCHANGE,      ovnact_move)              \
-    OVNACT(DEC_TTL,       ovnact_null)              \
-    OVNACT(CT_NEXT,       ovnact_ct_next)           \
-    OVNACT(CT_COMMIT,     ovnact_ct_commit)         \
-    OVNACT(CT_DNAT,       ovnact_ct_nat)            \
-    OVNACT(CT_SNAT,       ovnact_ct_nat)            \
-    OVNACT(CT_LB,         ovnact_ct_lb)             \
-    OVNACT(CT_CLEAR,      ovnact_null)              \
-    OVNACT(CLONE,         ovnact_nest)              \
-    OVNACT(ARP,           ovnact_nest)              \
-    OVNACT(ND_NA,         ovnact_nest)              \
-    OVNACT(GET_ARP,       ovnact_get_mac_bind)      \
-    OVNACT(PUT_ARP,       ovnact_put_mac_bind)      \
-    OVNACT(GET_ND,        ovnact_get_mac_bind)      \
-    OVNACT(PUT_ND,        ovnact_put_mac_bind)      \
-    OVNACT(PUT_DHCPV4_OPTS, ovnact_put_dhcp_opts)   \
-    OVNACT(PUT_DHCPV6_OPTS, ovnact_put_dhcp_opts)   \
-    OVNACT(SET_QUEUE,       ovnact_set_queue)       \
-    OVNACT(DNS_LOOKUP,      ovnact_dns_lookup)
+#define OVNACTS                                       \
+    OVNACT(OUTPUT,            ovnact_null)            \
+    OVNACT(NEXT,              ovnact_next)            \
+    OVNACT(LOAD,              ovnact_load)            \
+    OVNACT(MOVE,              ovnact_move)            \
+    OVNACT(EXCHANGE,          ovnact_move)            \
+    OVNACT(DEC_TTL,           ovnact_null)            \
+    OVNACT(CT_NEXT,           ovnact_ct_next)         \
+    OVNACT(CT_COMMIT,         ovnact_ct_commit)       \
+    OVNACT(CT_DNAT,           ovnact_ct_nat)          \
+    OVNACT(CT_SNAT,           ovnact_ct_nat)          \
+    OVNACT(CT_LB,             ovnact_ct_lb)           \
+    OVNACT(CT_CLEAR,          ovnact_null)            \
+    OVNACT(CLONE,             ovnact_nest)            \
+    OVNACT(ARP,               ovnact_nest)            \
+    OVNACT(ND_NA,             ovnact_nest)            \
+    OVNACT(GET_ARP,           ovnact_get_mac_bind)    \
+    OVNACT(PUT_ARP,           ovnact_put_mac_bind)    \
+    OVNACT(GET_ND,            ovnact_get_mac_bind)    \
+    OVNACT(PUT_ND,            ovnact_put_mac_bind)    \
+    OVNACT(PUT_DHCPV4_OPTS,   ovnact_put_dhcp_opts)   \
+    OVNACT(PUT_DHCPV6_OPTS,   ovnact_put_dhcp_opts)   \
+    OVNACT(SET_QUEUE,         ovnact_set_queue)       \
+    OVNACT(DNS_LOOKUP,        ovnact_dns_lookup)      \
+    OVNACT(LOG,               ovnact_log)
 
 /* enum ovnact_type, with a member OVNACT_<ENUM> for each action. */
 enum OVS_PACKED_ENUM ovnact_type {
@@ -265,6 +266,14 @@  struct ovnact_dns_lookup {
     struct expr_field dst;      /* 1-bit destination field. */
 };
 
+/* OVNACT_LOG. */
+struct ovnact_log {
+    struct ovnact ovnact;
+    uint8_t verdict;            /* One of LOG_VERDICT_*. */
+    uint8_t severity;           /* One of LOG_SEVERITY_*. */
+    char *name;
+};
+
 /* Internal use by the helpers below. */
 void ovnact_init(struct ovnact *, enum ovnact_type, size_t len);
 void *ovnact_put(struct ofpbuf *, enum ovnact_type, size_t len);
@@ -400,6 +409,16 @@  enum action_opcode {
      *
      */
     ACTION_OPCODE_DNS_LOOKUP,
+
+    /* "log(arguments)".
+     *
+     * Arguments are as follows:
+     *   - An 8-bit verdict.
+     *   - An 8-bit severity.
+     *   - An 16-bit string length for the name.
+     *   - A variable length string containing the name.
+     */
+    ACTION_OPCODE_LOG,
 };
 
 /* Header. */
diff --git a/ovn/controller/ovn-controller.8.xml b/ovn/controller/ovn-controller.8.xml
index d1fcd8a7b3c4..f4be097a5cc8 100644
--- a/ovn/controller/ovn-controller.8.xml
+++ b/ovn/controller/ovn-controller.8.xml
@@ -20,6 +20,15 @@ 
       machine-local and do not run over a physical network.
     </p>
 
+    <h1>ACL Logging</h1>
+    <p>
+      ACL log messages are logged through <code>ovn-controller</code>'s
+      logging mechanism.  ACL log entries have the module
+      <code>pinctrl</code> at log level <code>info</code>.  Configuring
+      logging is described below in the <code>Logging Options</code>
+      section.
+    </p>
+
     <h1>Options</h1>
 
     <h2>Daemon Options</h2>
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index b7bcee65f909..de0f2aae662e 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -41,6 +41,7 @@ 
 #include "ovn/lex.h"
 #include "ovn/lib/logical-fields.h"
 #include "ovn/lib/ovn-dhcp.h"
+#include "ovn/lib/ovn-log.h"
 #include "ovn/lib/ovn-util.h"
 #include "poll-loop.h"
 #include "rconn.h"
@@ -918,6 +919,39 @@  exit:
 }
 
 static void
+pinctrl_handle_log(struct dp_packet *packet OVS_UNUSED,
+                   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) {
+            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\"",
+              name_len ? name : "<unnamed>",
+              log_verdict_to_string(lph->verdict),
+              log_severity_to_string(lph->severity), packet_str);
+    free(packet_str);
+    free(name);
+}
+
+static void
 process_packet_in(const struct ofp_header *msg, struct controller_ctx *ctx)
 {
     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
@@ -981,6 +1015,10 @@  process_packet_in(const struct ofp_header *msg, struct controller_ctx *ctx)
         pinctrl_handle_dns_lookup(&packet, &pin, &userdata, &continuation, ctx);
         break;
 
+    case ACTION_OPCODE_LOG:
+        pinctrl_handle_log(&packet, &headers, &userdata);
+        break;
+
     default:
         VLOG_WARN_RL(&rl, "unrecognized packet-in opcode %"PRIu32,
                      ntohl(ah->opcode));
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 937f94d80bbb..6a8f54523f2c 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -33,6 +33,7 @@ 
 #include "ovn/actions.h"
 #include "ovn/expr.h"
 #include "ovn/lex.h"
+#include "ovn/lib/ovn-log.h"
 #include "packets.h"
 #include "openvswitch/shash.h"
 #include "simap.h"
@@ -1759,6 +1760,133 @@  ovnact_dns_lookup_free(struct ovnact_dns_lookup *dl OVS_UNUSED)
 {
 }
 
+static void
+parse_log_arg(struct action_context *ctx, struct ovnact_log *log)
+{
+
+    if (lexer_match_id(ctx->lexer, "verdict")) {
+        if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
+            return;
+        }
+        if (lexer_match_id(ctx->lexer, "drop")) {
+            log->verdict = LOG_VERDICT_DROP;
+        } else if (lexer_match_id(ctx->lexer, "reject")) {
+            log->verdict = LOG_VERDICT_REJECT;
+        } else if (lexer_match_id(ctx->lexer, "allow")) {
+            log->verdict = LOG_VERDICT_ALLOW;
+        } else {
+            lexer_syntax_error(ctx->lexer, "unknown acl verdict");
+        }
+    } else if (lexer_match_id(ctx->lexer, "name")) {
+        if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
+            return;
+        }
+        /* If there are multiple "name" arguments, use the first. */
+        if (log->name) {
+            lexer_get(ctx->lexer);
+            return;
+        }
+        if (ctx->lexer->token.type == LEX_T_STRING) {
+            /* Arbitrarily limit the name length to 64 bytes, since
+             * these will be encoded in datapath actions. */
+            if (strlen(ctx->lexer->token.s) >= 64) {
+                lexer_syntax_error(ctx->lexer, "name must be shorter "
+                                               "than 64 characters");
+                return;
+            }
+            log->name = xstrdup(ctx->lexer->token.s);
+        } else {
+            lexer_syntax_error(ctx->lexer, "expecting string");
+            return;
+        }
+        lexer_get(ctx->lexer);
+    } else if (lexer_match_id(ctx->lexer, "severity")) {
+        if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
+            return;
+        }
+        if (ctx->lexer->token.type == LEX_T_ID) {
+            uint8_t severity = log_severity_from_string(ctx->lexer->token.s);
+            if (severity != UINT8_MAX) {
+                log->severity = severity;
+            } else {
+                lexer_syntax_error(ctx->lexer, "expecting integer");
+                lexer_get(ctx->lexer);
+                return;
+            }
+        } else {
+            lexer_syntax_error(ctx->lexer, "expecting token");
+        }
+        lexer_get(ctx->lexer);
+    } else {
+        lexer_syntax_error(ctx->lexer, NULL);
+    }
+}
+
+static void
+parse_LOG(struct action_context *ctx)
+{
+    struct ovnact_log *log = ovnact_put_LOG(ctx->ovnacts);
+
+    /* Provide default values. */
+    log->severity = LOG_SEVERITY_INFO;
+    log->verdict = LOG_VERDICT_UNKNOWN;
+
+    if (lexer_match(ctx->lexer, LEX_T_LPAREN)) {
+        while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) {
+            parse_log_arg(ctx, log);
+            if (ctx->lexer->error) {
+                return;
+            }
+            lexer_match(ctx->lexer, LEX_T_COMMA);
+        }
+    }
+}
+
+static void
+format_LOG(const struct ovnact_log *log, struct ds *s)
+{
+    ds_put_cstr(s, "log(");
+
+    if (log->name) {
+        ds_put_format(s, "name=\"%s\", ", log->name);
+    }
+
+    ds_put_format(s, "verdict=%s, ", log_verdict_to_string(log->verdict));
+    ds_put_format(s, "severity=%s, ", log_severity_to_string(log->severity));
+
+    ds_chomp(s, ' ');
+    ds_chomp(s, ',');
+    ds_put_cstr(s, ");");
+}
+
+static void
+encode_LOG(const struct ovnact_log *log,
+           const struct ovnact_encode_params *ep OVS_UNUSED,
+           struct ofpbuf *ofpacts)
+{
+    size_t oc_offset = encode_start_controller_op(ACTION_OPCODE_LOG, false,
+                                                  ofpacts);
+
+    struct log_pin_header *lph = ofpbuf_put_uninit(ofpacts, sizeof *lph);
+    lph->verdict = log->verdict;
+    lph->severity = log->severity;
+    lph->name_len = 0;
+
+    if (log->name) {
+        int name_len = strlen(log->name);
+        lph->name_len = htons(name_len);
+        ofpbuf_put(ofpacts, log->name, name_len);
+    }
+
+    encode_finish_controller_op(oc_offset, ofpacts);
+}
+
+static void
+ovnact_log_free(struct ovnact_log *log)
+{
+    free(log->name);
+}
+
 /* Parses an assignment or exchange or put_dhcp_opts action. */
 static void
 parse_set_action(struct action_context *ctx)
@@ -1838,6 +1966,8 @@  parse_action(struct action_context *ctx)
         parse_put_mac_bind(ctx, 128, ovnact_put_PUT_ND(ctx->ovnacts));
     } else if (lexer_match_id(ctx->lexer, "set_queue")) {
         parse_SET_QUEUE(ctx);
+    } else if (lexer_match_id(ctx->lexer, "log")) {
+        parse_LOG(ctx);
     } else {
         lexer_syntax_error(ctx->lexer, "expecting action");
     }
diff --git a/ovn/lib/automake.mk b/ovn/lib/automake.mk
index 9ad8b6af7564..a215b92398b2 100644
--- a/ovn/lib/automake.mk
+++ b/ovn/lib/automake.mk
@@ -10,6 +10,8 @@  ovn_lib_libovn_la_SOURCES = \
 	ovn/lib/expr.c \
 	ovn/lib/lex.c \
 	ovn/lib/ovn-dhcp.h \
+	ovn/lib/ovn-log.c \
+	ovn/lib/ovn-log.h \
 	ovn/lib/ovn-util.c \
 	ovn/lib/ovn-util.h \
 	ovn/lib/logical-fields.c \
diff --git a/ovn/lib/ovn-log.c b/ovn/lib/ovn-log.c
new file mode 100644
index 000000000000..0c5467269eff
--- /dev/null
+++ b/ovn/lib/ovn-log.c
@@ -0,0 +1,69 @@ 
+/*
+ * Copyright (c) 2017 Nicira, 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.
+ */
+
+#include <config.h>
+#include <string.h>
+#include "ovn/lib/ovn-log.h"
+
+const char *
+log_verdict_to_string(uint8_t verdict)
+{
+    if (verdict == LOG_VERDICT_ALLOW) {
+        return "allow";
+    } else if (verdict == LOG_VERDICT_DROP) {
+        return "drop";
+    } else if (verdict == LOG_VERDICT_REJECT) {
+        return "reject";
+    } else {
+        return "<unknown>";
+    }
+}
+
+const char *
+log_severity_to_string(uint8_t severity)
+{
+    if (severity == LOG_SEVERITY_ALERT) {
+        return "alert";
+    } else if (severity == LOG_SEVERITY_WARNING) {
+        return "warning";
+    } else if (severity == LOG_SEVERITY_NOTICE) {
+        return "notice";
+    } else if (severity == LOG_SEVERITY_INFO) {
+        return "info";
+    } else if (severity == LOG_SEVERITY_DEBUG) {
+        return "debug";
+    } else {
+        return "<unknown>";
+    }
+}
+
+uint8_t
+log_severity_from_string(const char *name)
+{
+    if (!strcmp(name, "alert")) {
+        return LOG_SEVERITY_ALERT;
+    } else if (!strcmp(name, "warning")) {
+        return LOG_SEVERITY_WARNING;
+    } else if (!strcmp(name, "notice")) {
+        return LOG_SEVERITY_NOTICE;
+    } else if (!strcmp(name, "info")) {
+        return LOG_SEVERITY_INFO;
+    } else if (!strcmp(name, "debug")) {
+        return LOG_SEVERITY_DEBUG;
+    } else {
+        return UINT8_MAX;
+    }
+}
diff --git a/ovn/lib/ovn-log.h b/ovn/lib/ovn-log.h
new file mode 100644
index 000000000000..3208aaeaa5fe
--- /dev/null
+++ b/ovn/lib/ovn-log.h
@@ -0,0 +1,52 @@ 
+/*
+ * Copyright (c) 2017 Nicira, 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_LOG_H
+#define OVN_LOG_H 1
+
+#include <stdint.h>
+#include "openvswitch/types.h"
+
+
+struct log_pin_header {
+    uint8_t verdict;            /* One of LOG_VERDICT_*. */
+    uint8_t severity;           /* One of LOG_SEVERITY*. */
+    ovs_be16 name_len;
+    /* Followed by a 'name_len' string containing the rule's name.  If
+     * there is no name for this rule, 'name_len' may be zero. */
+};
+
+enum log_verdict {
+    LOG_VERDICT_ALLOW,
+    LOG_VERDICT_DROP,
+    LOG_VERDICT_REJECT,
+    LOG_VERDICT_UNKNOWN = UINT8_MAX
+};
+
+const char *log_verdict_to_string(uint8_t verdict);
+
+
+/* Severity levels.  Based on RFC5424 levels. */
+#define LOG_SEVERITY_ALERT    1
+#define LOG_SEVERITY_WARNING  4
+#define LOG_SEVERITY_NOTICE   5
+#define LOG_SEVERITY_INFO     6
+#define LOG_SEVERITY_DEBUG    7
+
+const char *log_severity_to_string(uint8_t severity);
+uint8_t log_severity_from_string(const char *name);
+
+#endif
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index a3f138d448ce..bf881f55dedf 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -2998,6 +2998,40 @@  build_pre_stateful(struct ovn_datapath *od, struct hmap *lflows)
 }
 
 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);
+    }
+
+    /* If a severity level isn't specified, default to "info". */
+    if (acl->severity) {
+        ds_put_format(actions, "severity=%s, ", acl->severity);
+    } else {
+        ds_put_format(actions, "severity=info, ");
+    }
+
+    if (!strcmp(acl->action, "drop")) {
+        ds_put_cstr(actions, "verdict=drop, ");
+    } else if (!strcmp(acl->action, "reject")) {
+        ds_put_cstr(actions, "verdict=reject, ");
+    } else if (!strcmp(acl->action, "allow")
+        || !strcmp(acl->action, "allow-related")) {
+        ds_put_cstr(actions, "verdict=allow, ");
+    }
+
+    ds_chomp(actions, ' ');
+    ds_chomp(actions, ',');
+    ds_put_cstr(actions, "); ");
+}
+
+static void
 build_acls(struct ovn_datapath *od, struct hmap *lflows)
 {
     bool has_stateful = has_stateful_acl(od);
@@ -3111,11 +3145,17 @@  build_acls(struct ovn_datapath *od, struct hmap *lflows)
              * may and then its return traffic would not have an
              * associated conntrack entry and would return "+invalid". */
             if (!has_stateful) {
+                struct ds actions = DS_EMPTY_INITIALIZER;
+                build_acl_log(&actions, acl);
+                ds_put_cstr(&actions, "next;");
                 ovn_lflow_add_with_hint(lflows, od, stage,
                                         acl->priority + OVN_ACL_PRI_OFFSET,
-                                        acl->match, "next;", stage_hint);
+                                        acl->match, ds_cstr(&actions),
+                                        stage_hint);
+                ds_destroy(&actions);
             } else {
                 struct ds match = DS_EMPTY_INITIALIZER;
+                struct ds actions = DS_EMPTY_INITIALIZER;
 
                 /* Commit the connection tracking entry if it's a new
                  * connection that matches this ACL.  After this commit,
@@ -3133,10 +3173,13 @@  build_acls(struct ovn_datapath *od, struct hmap *lflows)
                                       " || (!ct.new && ct.est && !ct.rpl "
                                            "&& ct_label.blocked == 1)) "
                                       "&& (%s)", acl->match);
+                ds_put_cstr(&actions, REGBIT_CONNTRACK_COMMIT" = 1; ");
+                build_acl_log(&actions, acl);
+                ds_put_cstr(&actions, "next;");
                 ovn_lflow_add_with_hint(lflows, od, stage,
                                         acl->priority + OVN_ACL_PRI_OFFSET,
                                         ds_cstr(&match),
-                                        REGBIT_CONNTRACK_COMMIT" = 1; next;",
+                                        ds_cstr(&actions),
                                         stage_hint);
 
                 /* Match on traffic in the request direction for an established
@@ -3146,20 +3189,26 @@  build_acls(struct ovn_datapath *od, struct hmap *lflows)
                  * connection is still allowed by the currently defined
                  * policy. */
                 ds_clear(&match);
+                ds_clear(&actions);
                 ds_put_format(&match,
                               "!ct.new && ct.est && !ct.rpl"
                               " && ct_label.blocked == 0 && (%s)",
                               acl->match);
+
+                build_acl_log(&actions, acl);
+                ds_put_cstr(&actions, "next;");
                 ovn_lflow_add_with_hint(lflows, od, stage,
                                         acl->priority + OVN_ACL_PRI_OFFSET,
-                                        ds_cstr(&match), "next;",
+                                        ds_cstr(&match), ds_cstr(&actions),
                                         stage_hint);
 
                 ds_destroy(&match);
+                ds_destroy(&actions);
             }
         } else if (!strcmp(acl->action, "drop")
                    || !strcmp(acl->action, "reject")) {
             struct ds match = DS_EMPTY_INITIALIZER;
+            struct ds actions = DS_EMPTY_INITIALIZER;
 
             /* XXX Need to support "reject", treat it as "drop;" for now. */
             if (!strcmp(acl->action, "reject")) {
@@ -3177,9 +3226,12 @@  build_acls(struct ovn_datapath *od, struct hmap *lflows)
                               "(!ct.est || (ct.est && ct_label.blocked == 1)) "
                               "&& (%s)",
                               acl->match);
+                ds_clear(&actions);
+                build_acl_log(&actions, acl);
+                ds_put_cstr(&actions, "/* drop */");
                 ovn_lflow_add_with_hint(lflows, od, stage,
                                         acl->priority + OVN_ACL_PRI_OFFSET,
-                                        ds_cstr(&match), "drop;",
+                                        ds_cstr(&match), ds_cstr(&actions),
                                         stage_hint);
 
                 /* For an existing connection without ct_label set, we've
@@ -3193,25 +3245,32 @@  build_acls(struct ovn_datapath *od, struct hmap *lflows)
                  * ct_commit() to the "stateful" stage, but since we're
                  * dropping the packet, we go ahead and do it here. */
                 ds_clear(&match);
+                ds_clear(&actions);
                 ds_put_format(&match,
                               "ct.est && ct_label.blocked == 0 && (%s)",
                               acl->match);
+                ds_put_cstr(&actions, "ct_commit(ct_label=1/1); ");
+                build_acl_log(&actions, acl);
+                ds_put_cstr(&actions, "/* drop */");
                 ovn_lflow_add_with_hint(lflows, od, stage,
                                         acl->priority + OVN_ACL_PRI_OFFSET,
-                                        ds_cstr(&match),
-                                        "ct_commit(ct_label=1/1);",
+                                        ds_cstr(&match), ds_cstr(&actions),
                                         stage_hint);
 
-                ds_destroy(&match);
             } else {
                 /* There are no stateful ACLs in use on this datapath,
                  * so a "drop" ACL is simply the "drop" logical flow action
                  * in all cases. */
+                ds_clear(&actions);
+                build_acl_log(&actions, acl);
+                ds_put_cstr(&actions, "/* drop */");
                 ovn_lflow_add_with_hint(lflows, od, stage,
                                         acl->priority + OVN_ACL_PRI_OFFSET,
-                                        acl->match, "drop;", stage_hint);
-                ds_destroy(&match);
+                                        acl->match, ds_cstr(&actions),
+                                        stage_hint);
             }
+            ds_destroy(&match);
+            ds_destroy(&actions);
         }
         free(stage_hint);
     }
diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
index d85a3fe98e44..a077bfb8107a 100644
--- a/ovn/ovn-nb.ovsschema
+++ b/ovn/ovn-nb.ovsschema
@@ -1,7 +1,7 @@ 
 {
     "name": "OVN_Northbound",
-    "version": "5.7.0",
-    "cksum": "3754583060 16164",
+    "version": "5.8.0",
+    "cksum": "2812300190 16766",
     "tables": {
         "NB_Global": {
             "columns": {
@@ -116,7 +116,7 @@ 
             "isRoot": true},
         "Load_Balancer": {
             "columns": {
-		"name": {"type": "string"},
+                "name": {"type": "string"},
                 "vips": {
                     "type": {"key": "string", "value": "string",
                              "min": 0, "max": "unlimited"}},
@@ -130,6 +130,9 @@ 
             "isRoot": true},
         "ACL": {
             "columns": {
+                "name": {"type": {"key": {"type": "string",
+                                          "maxLength": 63},
+                                          "min": 0, "max": 1}},
                 "priority": {"type": {"key": {"type": "integer",
                                               "minInteger": 0,
                                               "maxInteger": 32767}}},
@@ -139,6 +142,12 @@ 
                 "action": {"type": {"key": {"type": "string",
                                             "enum": ["set", ["allow", "allow-related", "drop", "reject"]]}}},
                 "log": {"type": "boolean"},
+                "severity": {"type": {"key": {"type": "string",
+                                              "enum": ["set",
+                                                       ["alert", "warning",
+                                                        "notice", "info",
+                                                        "debug"]]},
+                                      "min": 0, "max": 1}},
                 "external_ids": {
                     "type": {"key": "string", "value": "string",
                              "min": 0, "max": "unlimited"}}},
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index 1e73465663e0..12a293943958 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -958,6 +958,13 @@ 
       <ref column="match"/>, and <code>deny</code> as <ref column="action"/>.)
     </p>
 
+    <column name="name">
+      <p>
+        A name for the ACL.  The name has no special meaning, but may be
+        used by the logging system to identify the source rule.
+      </p>
+    </column>
+
     <column name="priority">
       <p>
         The ACL rule's priority.  Rules with numerically higher priority
@@ -1048,9 +1055,16 @@ 
         log message on the transport node or nodes that perform ACL processing.
         Logging may be combined with any <ref column="action"/>.
       </p>
+    </column>
 
+    <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
+        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>
 
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index c1731d284531..3ffc2c417d9e 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -1515,6 +1515,38 @@ 
         </dd>
       </dl>
 
+      <dl>
+        <dt>
+          <code>log(<var>argument</var>, </code>...<code>);</code>
+        </dt>
+
+        <dd>
+          <p>
+            Send the packet to <code>ovn-controller</code> for potential
+            logging.  The local <code>ovn-controller</code> will make a
+            decision whether or where to log the packet.
+          </p>
+          <p>
+            The <code>log</code> action takes zero or more of the
+            following <var>arguments</var>:
+          </p>
+          <ul>
+            <li><code>name</code> An optional name for the ACL.</li>
+            <li><code>severity</code> An indication of the severity
+                of the event.  The value is one of following (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 provided, the
+                default is <code>info</code>.
+            </li>
+            <li><code>verdict</code> The verdict for packets matching
+                the flow.  One of <code>allow</code>, <code>deny</code>,
+                or <code>reject</code>.
+            </li>
+          </ul>
+        </dd>
+      </dl>
+
       <p>
         The following actions will likely be useful later, but they have not
         been thought out carefully.
diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index 15012af6b58d..fdfc61b3fe8a 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -76,17 +76,30 @@ 
 
     <h1>Logical Switch ACL Commands</h1>
     <dl>
-      <dt>[<code>--log</code>] [<code>--may-exist</code>] <code>acl-add</code> <var>switch</var> <var>direction</var> <var>priority</var> <var>match</var> <var>action</var></dt>
-      <dd>
-        Adds the specified ACL to <var>switch</var>.
-        <var>direction</var> must be either <code>from-lport</code> or
-        <code>to-lport</code>.  <var>priority</var> must be between
-        <code>0</code> and <code>32767</code>, inclusive.  If
-        <code>--log</code> is specified, packet logging is enabled for the
-        ACL.  A full description of the fields are in <code>ovn-nb</code>(5).
-        If <code>--may-exist</code> is specified, adding a duplicated ACL
-        succeeds but the ACL is not really created. Without <code>--may-exist</code>,
-        adding a duplicated ACL results in error.
+      <dt>[<code>--log</code>] [<code>--may-exist</code>] <code>acl-add</code> <var>switch</var> <var>direction</var> <var>priority</var> <var>match</var> <var>verdict</var> [<code>severity=</code><var>severity</var>] [<code>name=</code><var>name</var>]</dt>
+      <dd>
+        <p>
+          Adds the specified ACL to <var>switch</var>.
+          <var>direction</var> must be either <code>from-lport</code> or
+          <code>to-lport</code>.  <var>priority</var> must be between
+          <code>0</code> and <code>32767</code>, inclusive.  A full
+          description of the fields are in <code>ovn-nb</code>(5).  If
+          <code>--may-exist</code> is specified, adding a duplicated ACL
+          succeeds but the ACL is not really created. Without
+          <code>--may-exist</code>, adding a duplicated ACL results in
+          error.
+        </p>
+
+        <p>
+          If <code>--log</code> is specified, packet logging is enabled
+          for the ACL.  When logging is enabled, the optional arguments
+          <code>severity</code> and <code>name</code> specify a severity
+          and name for the log entries, respectively.  The severity must
+          be one of <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>
       </dd>
 
       <dt><code>acl-del</code> <var>switch</var> [<var>direction</var> [<var>priority</var> <var>match</var>]]</dt>
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 0488318c41b3..860911691528 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -25,6 +25,7 @@ 
 #include "fatal-signal.h"
 #include "openvswitch/json.h"
 #include "ovn/lib/ovn-nb-idl.h"
+#include "ovn/lib/ovn-log.h"
 #include "ovn/lib/ovn-util.h"
 #include "packets.h"
 #include "poll-loop.h"
@@ -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\
                             add an ACL to SWITCH\n\
   acl-del SWITCH [DIRECTION [PRIORITY MATCH]]\n\
                             remove ACLs from SWITCH\n\
@@ -1362,10 +1363,27 @@  nbctl_acl_add(struct ctl_context *ctx)
     nbrec_acl_set_direction(acl, direction);
     nbrec_acl_set_match(acl, ctx->argv[4]);
     nbrec_acl_set_action(acl, action);
+
     if (shash_find(&ctx->options, "--log") != NULL) {
         nbrec_acl_set_log(acl, true);
     }
 
+    /* Optional fields for logging. */
+    char **settings = (char **) &ctx->argv[6];
+    int n_settings = ctx->argc - 6;
+
+    for (int i = 0; i < n_settings; i++) {
+        if (!strncmp(settings[i], "severity=", 9)) {
+            const char *severity = settings[i] + 9;
+	        if (log_severity_from_string(severity) == UINT8_MAX) {
+                ctl_fatal("bad severity: %s", severity);
+            }
+            nbrec_acl_set_severity(acl, severity);
+        } else if (!strncmp(settings[i], "name=", 5)) {
+            nbrec_acl_set_name(acl, settings[i] + 5);
+        }
+    }
+
     /* Check if same acl already exists for the ls */
     for (size_t i = 0; i < ls->n_acls; i++) {
         if (!acl_cmp(&ls->acls[i], &acl)) {
@@ -3364,8 +3382,8 @@  static const struct ctl_command_syntax nbctl_commands[] = {
     { "ls-list", 0, 0, "", NULL, nbctl_ls_list, NULL, "", RO },
 
     /* acl commands. */
-    { "acl-add", 5, 5, "SWITCH DIRECTION PRIORITY MATCH ACTION", NULL,
-      nbctl_acl_add, NULL, "--log,--may-exist", RW },
+    { "acl-add", 5, 8, "SWITCH DIRECTION PRIORITY MATCH ACTION", NULL,
+      nbctl_acl_add, NULL, "--log,--may-exist,--name=,--severity=", RW },
     { "acl-del", 1, 4, "SWITCH [DIRECTION [PRIORITY MATCH]]", NULL,
       nbctl_acl_del, NULL, "", RW },
     { "acl-list", 1, 1, "SWITCH", NULL, nbctl_acl_list, NULL, "", RO },
diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
index ab56221d7829..6230b100f2dc 100644
--- a/ovn/utilities/ovn-trace.c
+++ b/ovn/utilities/ovn-trace.c
@@ -37,6 +37,7 @@ 
 #include "ovn/lib/logical-fields.h"
 #include "ovn/lib/ovn-sb-idl.h"
 #include "ovn/lib/ovn-dhcp.h"
+#include "ovn/lib/ovn-log.h"
 #include "ovn/lib/ovn-util.h"
 #include "ovsdb-idl.h"
 #include "poll-loop.h"
@@ -1682,6 +1683,20 @@  execute_ct_nat(const struct ovnact_ct_nat *ct_nat,
 }
 
 static void
+execute_log(const struct ovnact_log *log, struct flow *uflow,
+            struct ovs_list *super)
+{
+    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->name ? log->name : "<unnamed>",
+                    log_verdict_to_string(log->verdict),
+                    log_severity_to_string(log->severity),
+                    packet_str);
+    free(packet_str);
+}
+
+static void
 trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
               const struct ovntrace_datapath *dp, struct flow *uflow,
               uint8_t table_id, enum ovnact_pipeline pipeline,
@@ -1816,6 +1831,10 @@  trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
         case OVNACT_DNS_LOOKUP:
             execute_dns_lookup(ovnact_get_DNS_LOOKUP(a), uflow, super);
             break;
+
+        case OVNACT_LOG:
+            execute_log(ovnact_get_LOG(a), uflow, super);
+            break;
         }
 
     }
diff --git a/tests/ovn.at b/tests/ovn.at
index 248aea4b26df..3720656079eb 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -5741,6 +5741,111 @@  OVN_CLEANUP([hv1],[hv2])
 
 AT_CLEANUP
 
+
+AT_SETUP([ovn -- ACL logging])
+AT_KEYWORDS([ovn])
+ovn_start
+
+net_add n1
+
+sim_add hv
+as hv
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+for i in lp1 lp2; do
+    ovs-vsctl -- add-port br-int $i -- \
+        set interface $i external-ids:iface-id=$i \
+        options:tx_pcap=hv/$i-tx.pcap \
+        options:rxq_pcap=hv/$i-rx.pcap
+done
+
+lp1_mac="f0:00:00:00:00:01"
+lp1_ip="192.168.1.2"
+
+lp2_mac="f0:00:00:00:00:02"
+lp2_ip="192.168.1.3"
+
+ovn-nbctl ls-add lsw0
+ovn-nbctl --wait=sb lsp-add lsw0 lp1
+ovn-nbctl --wait=sb lsp-add lsw0 lp2
+ovn-nbctl lsp-set-addresses lp1 $lp1_mac
+ovn-nbctl lsp-set-addresses lp2 $lp2_mac
+ovn-nbctl --wait=sb sync
+
+ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==80' drop
+ovn-nbctl --log acl-add lsw0 to-lport 1000 'tcp.dst==81' drop severity=alert name=drop-flow
+
+ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==82' allow
+ovn-nbctl --log acl-add lsw0 to-lport 1000 'tcp.dst==83' allow severity=info name=allow-flow
+
+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 --log acl-add lsw0 to-lport 1000 'tcp.dst==87' reject severity=alert name=reject-flow
+
+ovn-sbctl dump-flows
+
+
+# Send packet that should be dropped 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 &&
+        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.
+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 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 &&
+        tcp && tcp.flags==2 && tcp.src==4362 && tcp.dst==82"
+as hv ovs-appctl -t ovn-controller inject-pkt "$packet"
+
+# Send packet that should be allowed with logging.
+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==4363 && tcp.dst==83"
+as hv ovs-appctl -t ovn-controller inject-pkt "$packet"
+
+# Send packet that should allow related flows 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 &&
+        tcp && tcp.flags==2 && tcp.src==4364 && tcp.dst==84"
+as hv ovs-appctl -t ovn-controller inject-pkt "$packet"
+
+# Send packet that should allow related flows with logging.
+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==4365 && tcp.dst==85"
+as hv ovs-appctl -t ovn-controller inject-pkt "$packet"
+
+# Send packet that should allow related flows with logging.
+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 allow related flows with logging.
+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"
+
+AT_CHECK([grep 'ACL' hv/ovn-controller.log | sed 's/.*ACL /ACL /'], [0], [dnl
+ACL name=drop-flow, verdict=drop, severity=alert, packet="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"
+ACL name=allow-flow, verdict=allow, severity=info, packet="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"
+ACL name=<unnamed>, verdict=allow, severity=info, packet="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"
+ACL name=reject-flow, verdict=reject, severity=alert, packet="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"
+])
+
+OVN_CLEANUP([hv])
+AT_CLEANUP
+
+
 AT_SETUP([ovn -- DSCP marking check])
 AT_KEYWORDS([ovn])
 ovn_start