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

Justin Pettit jpettit at ovn.org
Fri Jul 28 22:46:43 UTC 2017


> On Jul 27, 2017, at 7:23 PM, Han Zhou <zhouhan at gmail.com> wrote:
> 
> On Thu, Jul 27, 2017 at 5:59 PM, Justin Pettit <jpettit at 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




More information about the dev mailing list