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

[Ben Pfaff]

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 
> 
> On Thu, Apr 20, 2017 at 1:04 PM, Miguel Angel Ajo Pelayo <
> majop...@redhat.com> wrote:
> 
> > Just a very nit opinion/comment. nice work.
> >
> > On Tue, Apr 18, 2017 at 9:47 PM, Ben Pfaff  wrote:
> >
> >> Signed-off-by: Ben Pfaff 
> >> ---
> >>  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 @@
> >>  
> >>
> >>  
> >> +
> >> +--ct=flags
> >> +
> >> +  
> >> +This option sets the ct_state flags that a
> >> +ct_next logical action will report.  The
> >> flags
> >> +must be a comma- or space-separated list of the following
> >> connection
> >> +tracking flags:
> >> +  
> >> +
> >> +  
> >> +
> >> +  trk: Include to indicate connection tracking has
> >> taken
> >> +  place.  (This bit is set automatically even if not listed in
> >> +  flags.
> >> +
> >> +new: Include to indicate a new flow.
> >> +est: Include to indicate an established
> >> flow.
> >> +rel: Include to indicate a related flow.
> >> +rpl: Include to indicate a reply flow.
> >> +inv: Include to indicate a connection entry in a
> >> +bad state.
> >> +dnat: Include to indicate a packet whose
> >> +destination IP address has been changed.
> >> +snat: Include to indicate a packet whose source
> >> IP
> >> +address has been changed.
> >> +  
> >> +
> >> +  
> >> +The ct_next action is used to implement the OVN
> >> +distributed firewall.  For testing, useful flag combinations
> >> include:
> >> +  
> >> +
> >> +  
> >> +trk,new: A packet in a flow in either direction
> >> +through a firewall that has not yet been committed (with
> >> +ct_commit).
> >> +trk,est: A packet in an established flow going
> >> out
> >> +through a firewall.
> >> +trk,rpl: A packet coming in through a firewall
> >> in
> >> +reply to an established flow.
> >> +trk,inv: An invalid packet in either
> >> direction.
> >> +  
> >> +
> >> +  
> >> +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 --ct
> >> +options to specify the flags for multiple ct_next
> >> actions.
> >> +  
> >> +
> >> +  
> >> +When --ct is unspecified, or when there are fewer
> >> +--ct options than ct_next actions, the
> >> +flags default to trk,est.
> >> +  
> >> +
> >>
> >>
> >>Daemon Options
> >> 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;
> >> +
> >>  

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

[Miguel Angel Ajo Pelayo]

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 

On Thu, Apr 20, 2017 at 1:04 PM, Miguel Angel Ajo Pelayo <
majop...@redhat.com> wrote:

> Just a very nit opinion/comment. nice work.
>
> On Tue, Apr 18, 2017 at 9:47 PM, Ben Pfaff  wrote:
>
>> Signed-off-by: Ben Pfaff 
>> ---
>>  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 @@
>>  
>>
>>  
>> +
>> +--ct=flags
>> +
>> +  
>> +This option sets the ct_state flags that a
>> +ct_next logical action will report.  The
>> flags
>> +must be a comma- or space-separated list of the following
>> connection
>> +tracking flags:
>> +  
>> +
>> +  
>> +
>> +  trk: Include to indicate connection tracking has
>> taken
>> +  place.  (This bit is set automatically even if not listed in
>> +  flags.
>> +
>> +new: Include to indicate a new flow.
>> +est: Include to indicate an established
>> flow.
>> +rel: Include to indicate a related flow.
>> +rpl: Include to indicate a reply flow.
>> +inv: Include to indicate a connection entry in a
>> +bad state.
>> +dnat: Include to indicate a packet whose
>> +destination IP address has been changed.
>> +snat: Include to indicate a packet whose source
>> IP
>> +address has been changed.
>> +  
>> +
>> +  
>> +The ct_next action is used to implement the OVN
>> +distributed firewall.  For testing, useful flag combinations
>> include:
>> +  
>> +
>> +  
>> +trk,new: A packet in a flow in either direction
>> +through a firewall that has not yet been committed (with
>> +ct_commit).
>> +trk,est: A packet in an established flow going
>> out
>> +through a firewall.
>> +trk,rpl: A packet coming in through a firewall
>> in
>> +reply to an established flow.
>> +trk,inv: An invalid packet in either
>> direction.
>> +  
>> +
>> +  
>> +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 --ct
>> +options to specify the flags for multiple ct_next
>> actions.
>> +  
>> +
>> +  
>> +When --ct is unspecified, or when there are fewer
>> +--ct options than ct_next actions, the
>> +flags default to trk,est.
>> +  
>> +
>>
>>
>>Daemon Options
>> 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, ", ", _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, ", ", _ptr)) {
>> +

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

[Miguel Angel Ajo Pelayo]

Just a very nit opinion/comment. nice work.

On Tue, Apr 18, 2017 at 9:47 PM, Ben Pfaff  wrote:

> Signed-off-by: Ben Pfaff 
> ---
>  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 @@
>  
>
>  
> +
> +--ct=flags
> +
> +  
> +This option sets the ct_state flags that a
> +ct_next logical action will report.  The
> flags
> +must be a comma- or space-separated list of the following
> connection
> +tracking flags:
> +  
> +
> +  
> +
> +  trk: Include to indicate connection tracking has
> taken
> +  place.  (This bit is set automatically even if not listed in
> +  flags.
> +
> +new: Include to indicate a new flow.
> +est: Include to indicate an established
> flow.
> +rel: Include to indicate a related flow.
> +rpl: Include to indicate a reply flow.
> +inv: Include to indicate a connection entry in a
> +bad state.
> +dnat: Include to indicate a packet whose
> +destination IP address has been changed.
> +snat: Include to indicate a packet whose source
> IP
> +address has been changed.
> +  
> +
> +  
> +The ct_next action is used to implement the OVN
> +distributed firewall.  For testing, useful flag combinations
> include:
> +  
> +
> +  
> +trk,new: A packet in a flow in either direction
> +through a firewall that has not yet been committed (with
> +ct_commit).
> +trk,est: A packet in an established flow going
> out
> +through a firewall.
> +trk,rpl: A packet coming in through a firewall in
> +reply to an established flow.
> +trk,inv: An invalid packet in either
> direction.
> +  
> +
> +  
> +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 --ct
> +options to specify the flags for multiple ct_next
> actions.
> +  
> +
> +  
> +When --ct is unspecified, or when there are fewer
> +--ct options than ct_next actions, the
> +flags default to trk,est.
> +  
> +
>
>
>Daemon Options
> 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, ", ", _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, ", ", _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 && 

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

[Ben Pfaff]

Signed-off-by: Ben Pfaff 
---
 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 @@
 
   
 
+
+--ct=flags
+
+  
+This option sets the ct_state flags that a
+ct_next logical action will report.  The flags
+must be a comma- or space-separated list of the following connection
+tracking flags:
+  
+
+  
+
+  trk: Include to indicate connection tracking has taken
+  place.  (This bit is set automatically even if not listed in
+  flags.
+
+new: Include to indicate a new flow.
+est: Include to indicate an established flow.
+rel: Include to indicate a related flow.
+rpl: Include to indicate a reply flow.
+inv: Include to indicate a connection entry in a
+bad state.
+dnat: Include to indicate a packet whose
+destination IP address has been changed.
+snat: Include to indicate a packet whose source IP
+address has been changed.
+  
+
+  
+The ct_next action is used to implement the OVN
+distributed firewall.  For testing, useful flag combinations include:
+  
+
+  
+trk,new: A packet in a flow in either direction
+through a firewall that has not yet been committed (with
+ct_commit).
+trk,est: A packet in an established flow going out
+through a firewall.
+trk,rpl: A packet coming in through a firewall in
+reply to an established flow.
+trk,inv: An invalid packet in either direction.
+  
+
+  
+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 --ct
+options to specify the flags for multiple ct_next actions.
+  
+
+  
+When --ct is unspecified, or when there are fewer
+--ct options than ct_next actions, the
+flags default to trk,est.
+  
+
   
 
   Daemon Options
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, ", ", _ptr); cs;
+ cs = strtok_r(NULL, ", ", _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