Just a very nit opinion/comment. nice work. On Tue, Apr 18, 2017 at 9:47 PM, Ben Pfaff <[email protected]> wrote:
> Signed-off-by: Ben Pfaff <[email protected]> > --- > NEWS | 1 + > ovn/utilities/ovn-trace.8.xml | 58 +++++++++++++++++++++++++ > ovn/utilities/ovn-trace.c | 99 ++++++++++++++++++++++++++++++ > ++++++++++++- > 3 files changed, 156 insertions(+), 2 deletions(-) > > diff --git a/NEWS b/NEWS > index eb825ac72161..f0ada38b4019 100644 > --- a/NEWS > +++ b/NEWS > @@ -19,6 +19,7 @@ Post-v2.7.0 > * Gratuitous ARP for NAT addresses on a distributed logical router. > * Allow ovn-controller SSL configuration to be obtained from vswitchd > database. > + * ovn-trace now has basic support for tracing distributed firewalls. > - Add the command 'ovs-appctl stp/show' (see ovs-vswitchd(8)). > - OpenFlow: > * Increased support for OpenFlow 1.6 (draft). > diff --git a/ovn/utilities/ovn-trace.8.xml b/ovn/utilities/ovn-trace.8.xml > index 78914240d88c..8bb329bfbd71 100644 > --- a/ovn/utilities/ovn-trace.8.xml > +++ b/ovn/utilities/ovn-trace.8.xml > @@ -307,6 +307,64 @@ > </li> > </ul> > </dd> > + > + <dt><code>--ct=<var>flags</var></code></dt> > + <dd> > + <p> > + This option sets the <code>ct_state</code> flags that a > + <code>ct_next</code> logical action will report. The > <var>flags</var> > + must be a comma- or space-separated list of the following > connection > + tracking flags: > + </p> > + > + <ul> > + <li> > + <code>trk</code>: Include to indicate connection tracking has > taken > + place. (This bit is set automatically even if not listed in > + <var>flags</var>. > + </li> > + <li><code>new</code>: Include to indicate a new flow.</li> > + <li><code>est</code>: Include to indicate an established > flow.</li> > + <li><code>rel</code>: Include to indicate a related flow.</li> > + <li><code>rpl</code>: Include to indicate a reply flow.</li> > + <li><code>inv</code>: Include to indicate a connection entry in a > + bad state.</li> > + <li><code>dnat</code>: Include to indicate a packet whose > + destination IP address has been changed.</li> > + <li><code>snat</code>: Include to indicate a packet whose source > IP > + address has been changed.</li> > + </ul> > + > + <p> > + The <code>ct_next</code> action is used to implement the OVN > + distributed firewall. For testing, useful flag combinations > include: > + </p> > + > + <ul> > + <li><code>trk,new</code>: A packet in a flow in either direction > + through a firewall that has not yet been committed (with > + <code>ct_commit</code>).</li> > + <li><code>trk,est</code>: A packet in an established flow going > out > + through a firewall.</li> > + <li><code>trk,rpl</code>: A packet coming in through a firewall in > + reply to an established flow.</li> > + <li><code>trk,inv</code>: An invalid packet in either > direction.</li> > + </ul> > + > + <p> > + A packet might pass through the connection tracker twice in one > trip > + through OVN: once following egress from a VM as it passes outward > + through a firewall, and once preceding ingress to a second VM as > it > + passes inward through a firewall. Use multiple <code>--ct</code> > + options to specify the flags for multiple <code>ct_next</code> > actions. > + </p> > + > + <p> > + When <code>--ct</code> is unspecified, or when there are fewer > + <code>--ct</code> options than <code>ct_next</code> actions, the > + <var>flags</var> default to <code>trk,est</code>. > + </p> > + </dd> > </dl> > > <h2>Daemon Options</h2> > diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c > index 66844b11ac1d..e9463f02a70f 100644 > --- a/ovn/utilities/ovn-trace.c > +++ b/ovn/utilities/ovn-trace.c > @@ -69,6 +69,11 @@ static bool minimal; > static const char *ovs; > static struct vconn *vconn; > > +/* --ct: Connection tracking state to use for ct_next() actions. */ > +static uint32_t *ct_states; > +static size_t n_ct_states; > +static size_t ct_state_idx; > + > OVS_NO_RETURN static void usage(void); > static void parse_options(int argc, char *argv[]); > static char *trace(const char *datapath, const char *flow); > @@ -156,6 +161,42 @@ default_ovs(void) > } > > static void > +parse_ct_option(const char *state_s_) > +{ > + uint32_t state = CS_TRACKED; > + > + char *state_s = xstrdup(state_s_); > + char *save_ptr = NULL; > + for (char *cs = strtok_r(state_s, ", ", &save_ptr); cs; > why do we enforce the use of spaces? ", ", would it make sense to accept just "," as delimiter and then strip/ignore spaces for usability reasons? > + cs = strtok_r(NULL, ", ", &save_ptr)) { > + uint32_t bit = ct_state_from_string(cs); > + if (!bit) { > + ovs_fatal(0, "%s: unknown connection tracking state flag", > cs); > + } > + state |= bit; > + } > + free(state_s); > + > + /* Check constraints listed in ovs-fields(7). */ > + if (state & CS_INVALID && state & ~(CS_TRACKED | CS_INVALID)) { > + VLOG_WARN("%s: invalid connection state: " > + "when \"inv\" is set, only \"trk\" may also be set", > + state_s_); > + } > + if (state & CS_NEW && state & CS_ESTABLISHED) { > + VLOG_WARN("%s: invalid connection state: " > + "\"new\" and \"est\" are mutually exclusive", state_s_); > + } > + if (state & CS_NEW && state & CS_REPLY_DIR) { > + VLOG_WARN("%s: invalid connection state: " > + "\"new\" and \"rpy\" are mutually exclusive", state_s_); > + } > + > + ct_states = xrealloc(ct_states, (n_ct_states + 1) * sizeof > *ct_states); > + ct_states[n_ct_states++] = state; > +} > + > +static void > parse_options(int argc, char *argv[]) > { > enum { > @@ -166,6 +207,7 @@ parse_options(int argc, char *argv[]) > OPT_MINIMAL, > OPT_ALL, > OPT_OVS, > + OPT_CT, > DAEMON_OPTION_ENUMS, > SSL_OPTION_ENUMS, > VLOG_OPTION_ENUMS > @@ -178,6 +220,7 @@ parse_options(int argc, char *argv[]) > {"minimal", no_argument, NULL, OPT_MINIMAL}, > {"all", no_argument, NULL, OPT_ALL}, > {"ovs", optional_argument, NULL, OPT_OVS}, > + {"ct", required_argument, NULL, OPT_CT}, > {"help", no_argument, NULL, 'h'}, > {"version", no_argument, NULL, 'V'}, > DAEMON_LONG_OPTIONS, > @@ -225,6 +268,10 @@ parse_options(int argc, char *argv[]) > ovs = optarg ? optarg : default_ovs(); > break; > > + case OPT_CT: > + parse_ct_option(optarg); > + break; > + > case 'h': > usage(); > > @@ -1429,6 +1476,40 @@ execute_next(const struct ovnact_next *next, > } > > static void > +execute_ct_next(const struct ovnact_ct_next *ct_next, > + const struct ovntrace_datapath *dp, struct flow *uflow, > + enum ovnact_pipeline pipeline, struct ovs_list *super) > +{ > + /* Figure out ct_state. */ > + uint32_t state; > + const char *comment; > + if (ct_state_idx < n_ct_states) { > + state = ct_states[ct_state_idx++]; > + comment = ""; > + } else { > + state = CS_ESTABLISHED | CS_TRACKED; > + comment = " /* default (use --ct to customize) */"; > + } > + > + /* Make a sub-node for attaching the next table. */ > + struct ds s = DS_EMPTY_INITIALIZER; > + format_flags(&s, ct_state_to_string,an state, '|'); > + struct ovntrace_node *node = ovntrace_node_append( > + super, OVNTRACE_NODE_TRANSFORMATION, "ct_next(ct_state=%s%s)", > + ds_cstr(&s), comment); > + ds_destroy(&s); > + > + /* Trace the actions in the next table. */ > + struct flow ct_flow = *uflow; > + ct_flow.ct_state = state; > + trace__(dp, &ct_flow, ct_next->ltable, pipeline, &node->subs); > + > + /* Upon return, we will trace the actions following the ct action in > the > + * original table. The pipeline forked, so we're using the original > + * flow, not ct_flow. */ > +} > + > +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, > @@ -1484,13 +1565,27 @@ trace_actions(const struct ovnact *ovnacts, size_t > ovnacts_len, > break; > > case OVNACT_CT_NEXT: > + execute_ct_next(ovnact_get_CT_NEXT(a), dp, uflow, pipeline, > super); > + break; > + > case OVNACT_CT_COMMIT: > + /* Nothing to do. */ > + break; > + > case OVNACT_CT_DNAT: > case OVNACT_CT_SNAT: > + ovntrace_node_append(super, OVNTRACE_NODE_ERROR, > + "*** ct_dnat and ct_snat actions " > + "not implemented"); > + break; > + > case OVNACT_CT_LB: > - case OVNACT_CT_CLEAR: > ovntrace_node_append(super, OVNTRACE_NODE_ERROR, > - "*** ct_* actions not implemented"); > + "*** ct_lb action not implemented"); > + break; > + > + case OVNACT_CT_CLEAR: > + flow_clear_conntrack(uflow); > break; > > case OVNACT_CLONE: > -- > 2.10.2 > > _______________________________________________ > 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
