On Thu, Jul 27, 2017 at 5:59 PM, Justin Pettit <jpet...@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 <zhou...@gmail.com>
>
> Thanks!
>
> --Justin
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
