| Message ID | 20170418194743.27431-6-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
Just a very nit opinion/comment. nice work. On Tue, Apr 18, 2017 at 9:47 PM, Ben Pfaff <blp@ovn.org> wrote: > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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 <majopela@redhat.com> On Thu, Apr 20, 2017 at 1:04 PM, Miguel Angel Ajo Pelayo < majopela@redhat.com> wrote: > Just a very nit opinion/comment. nice work. > > On Tue, Apr 18, 2017 at 9:47 PM, Ben Pfaff <blp@ovn.org> wrote: > >> Signed-off-by: Ben Pfaff <blp@ovn.org> >> --- >> 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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > >
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 <majopela@redhat.com> > > On Thu, Apr 20, 2017 at 1:04 PM, Miguel Angel Ajo Pelayo < > majopela@redhat.com> wrote: > > > Just a very nit opinion/comment. nice work. > > > > On Tue, Apr 18, 2017 at 9:47 PM, Ben Pfaff <blp@ovn.org> wrote: > > > >> Signed-off-by: Ben Pfaff <blp@ovn.org> > >> --- > >> 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 > >> dev@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > > > >
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; + 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, 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:
Signed-off-by: Ben Pfaff <blp@ovn.org> --- NEWS | 1 + ovn/utilities/ovn-trace.8.xml | 58 +++++++++++++++++++++++++ ovn/utilities/ovn-trace.c | 99 ++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 156 insertions(+), 2 deletions(-)