Thank you for all the reviews! I'm glad to see new people helping out with reviews. I applied this series to master.
(No worries about the mistake with strtok_r(). The comment helped to reassure me that you were reading the code; sometimes I'm a little worried about that when reviews don't have much to criticize.) On Fri, Apr 21, 2017 at 10:51:41AM +0200, Miguel Angel Ajo Pelayo wrote: > Sorry, ignore my previous email. > > "delim" parameter of strtok_r will take any of the characters as delimiter, > so either "," or " " will work when we pass ", ". > > Apparently my C has got a bit rusty ;) > > > The whole patch series looks good to me. > > Acked-By: Miguel Angel Ajo <[email protected]> > > On Thu, Apr 20, 2017 at 1:04 PM, Miguel Angel Ajo Pelayo < > [email protected]> wrote: > > > 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.xm > >> l > >> 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
