[ovs-dev] [PATCH 5/5] ovn-trace: Implement ct_next and ct_clear actions.

Ben Pfaff blp at ovn.org
Fri Apr 21 15:50:45 UTC 2017


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 at redhat.com>
> 
> On Thu, Apr 20, 2017 at 1:04 PM, Miguel Angel Ajo Pelayo <
> majopela at redhat.com> wrote:
> 
> > Just a very nit opinion/comment. nice work.
> >
> > On Tue, Apr 18, 2017 at 9:47 PM, Ben Pfaff <blp at ovn.org> wrote:
> >
> >> Signed-off-by: Ben Pfaff <blp at 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 at openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>
> >
> >


More information about the dev mailing list