Re: [ovs-dev] [PATCH ovn v2] Support vlan-passthru for tag=0 logical switch ports

2021-04-13 Thread Numan Siddique 
On Mon, Apr 12, 2021, 8:35 PM Ihar Hrachyshka  wrote:

> On Fri, Mar 26, 2021 at 12:08 AM Numan Siddique  wrote:
> >
> > On Wed, Mar 24, 2021 at 9:22 PM Ben Pfaff  wrote:
> > >
> > > On Tue, Mar 23, 2021 at 09:22:43PM -0400, Ihar Hrachyshka wrote:
> > > > When vlan-passthru is true for LS, for untagged localnet ports,
> tagged
> > > > VLAN traffic originating from VIFs should be passed through intact
> since
> > > > the VLAN header belongs to VIF user, not the localnet port fabric.
> > > >
> > > > This patch adds multinode test cases to cover scenarios where packets
> > > > traverse through fabric layer, for both tagged and untagged (tag=0)
> > > > localnet ports.
> > > >
> > > > Signed-off-by: Ihar Hrachyshka 
> > >
> > > Thanks for adding ddlog support.  It looks correct to me.
> >
> > Hi Ihar,
> >
> > The patch LGTM.  I just have one comment.  I noticed that there is
> > slight difference
> > in the way ovn-northd and ovn-northd-ddlog sets the vlan-passthru in
> > the options column
> > of Port_Binding.
> >
> > If the logical switch's other_config:vlan-passthru is set to false i.e
> >
> > ovn-nbctl set logical_switch ls other_config:vlan-passthru=false
> >
> > then, ovn-northd doesn't set "vlan-passthru=false" in the options
> > column of port binding.
> > It will clear the option if it was set earlier. Whereas
> > ovn-northd-ddlog, sets "vlan-passthru=false".
> >
> > Technically this will not have any effect on the functionality.
> > Although I'd suggest making it consistent.
> > Either make ovn-northd to set "vlan-passthru=false" or make
> > ovn-northd-ddlog to clear vlan-passthru.
> > I'm fine either way.
>
> This is solved in v3, up for review.
>

Thanks. I applied v3 to the main branch.

Numan


> >
> > Thanks
> > Numan
> >
> > > ___
> > > dev mailing list
> > > d...@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> >
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v2] Support vlan-passthru for tag=0 logical switch ports

2021-04-12 Thread Ihar Hrachyshka 
On Fri, Mar 26, 2021 at 12:08 AM Numan Siddique  wrote:
>
> On Wed, Mar 24, 2021 at 9:22 PM Ben Pfaff  wrote:
> >
> > On Tue, Mar 23, 2021 at 09:22:43PM -0400, Ihar Hrachyshka wrote:
> > > When vlan-passthru is true for LS, for untagged localnet ports, tagged
> > > VLAN traffic originating from VIFs should be passed through intact since
> > > the VLAN header belongs to VIF user, not the localnet port fabric.
> > >
> > > This patch adds multinode test cases to cover scenarios where packets
> > > traverse through fabric layer, for both tagged and untagged (tag=0)
> > > localnet ports.
> > >
> > > Signed-off-by: Ihar Hrachyshka 
> >
> > Thanks for adding ddlog support.  It looks correct to me.
>
> Hi Ihar,
>
> The patch LGTM.  I just have one comment.  I noticed that there is
> slight difference
> in the way ovn-northd and ovn-northd-ddlog sets the vlan-passthru in
> the options column
> of Port_Binding.
>
> If the logical switch's other_config:vlan-passthru is set to false i.e
>
> ovn-nbctl set logical_switch ls other_config:vlan-passthru=false
>
> then, ovn-northd doesn't set "vlan-passthru=false" in the options
> column of port binding.
> It will clear the option if it was set earlier. Whereas
> ovn-northd-ddlog, sets "vlan-passthru=false".
>
> Technically this will not have any effect on the functionality.
> Although I'd suggest making it consistent.
> Either make ovn-northd to set "vlan-passthru=false" or make
> ovn-northd-ddlog to clear vlan-passthru.
> I'm fine either way.

This is solved in v3, up for review.

>
> Thanks
> Numan
>
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v2] Support vlan-passthru for tag=0 logical switch ports

2021-03-25 Thread Numan Siddique 
On Wed, Mar 24, 2021 at 9:22 PM Ben Pfaff  wrote:
>
> On Tue, Mar 23, 2021 at 09:22:43PM -0400, Ihar Hrachyshka wrote:
> > When vlan-passthru is true for LS, for untagged localnet ports, tagged
> > VLAN traffic originating from VIFs should be passed through intact since
> > the VLAN header belongs to VIF user, not the localnet port fabric.
> >
> > This patch adds multinode test cases to cover scenarios where packets
> > traverse through fabric layer, for both tagged and untagged (tag=0)
> > localnet ports.
> >
> > Signed-off-by: Ihar Hrachyshka 
>
> Thanks for adding ddlog support.  It looks correct to me.

Hi Ihar,

The patch LGTM.  I just have one comment.  I noticed that there is
slight difference
in the way ovn-northd and ovn-northd-ddlog sets the vlan-passthru in
the options column
of Port_Binding.

If the logical switch's other_config:vlan-passthru is set to false i.e

ovn-nbctl set logical_switch ls other_config:vlan-passthru=false

then, ovn-northd doesn't set "vlan-passthru=false" in the options
column of port binding.
It will clear the option if it was set earlier. Whereas
ovn-northd-ddlog, sets "vlan-passthru=false".

Technically this will not have any effect on the functionality.
Although I'd suggest making it consistent.
Either make ovn-northd to set "vlan-passthru=false" or make
ovn-northd-ddlog to clear vlan-passthru.
I'm fine either way.

Thanks
Numan

> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v2] Support vlan-passthru for tag=0 logical switch ports

2021-03-24 Thread Ben Pfaff 
On Tue, Mar 23, 2021 at 09:22:43PM -0400, Ihar Hrachyshka wrote:
> When vlan-passthru is true for LS, for untagged localnet ports, tagged
> VLAN traffic originating from VIFs should be passed through intact since
> the VLAN header belongs to VIF user, not the localnet port fabric.
> 
> This patch adds multinode test cases to cover scenarios where packets
> traverse through fabric layer, for both tagged and untagged (tag=0)
> localnet ports.
> 
> Signed-off-by: Ihar Hrachyshka 

Thanks for adding ddlog support.  It looks correct to me.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v2] Support vlan-passthru for tag=0 logical switch ports

2021-03-23 Thread Ihar Hrachyshka 
When vlan-passthru is true for LS, for untagged localnet ports, tagged
VLAN traffic originating from VIFs should be passed through intact since
the VLAN header belongs to VIF user, not the localnet port fabric.

This patch adds multinode test cases to cover scenarios where packets
traverse through fabric layer, for both tagged and untagged (tag=0)
localnet ports.

Signed-off-by: Ihar Hrachyshka 

---

v1: initial version.
v2: added ddlog implementation for db transformation.
v2: explain security implications in documentation.
v2: add NEWS entry.
---
 NEWS  |   1 +
 controller/physical.c |  12 +++-
 northd/ovn-northd.c   |   5 ++
 northd/ovn_northd.dl  |  10 +++-
 ovn-nb.xml|   6 +-
 tests/ovn.at  | 132 ++
 6 files changed, 162 insertions(+), 4 deletions(-)

diff --git a/NEWS b/NEWS
index 530c5d42f..9f281cdc5 100644
--- a/NEWS
+++ b/NEWS
@@ -7,6 +7,7 @@ Post-v21.03.0
 (This may take testing and tuning to be effective.)  This version of OVN
 requires DDLog 0.36.
   - Introduce ovn-controller incremetal processing engine statistics
+  - Support vlan-passthru mode for tag=0 localnet ports.
 
 OVN v21.03.0 - 12 Mar 2021
 -
diff --git a/controller/physical.c b/controller/physical.c
index fa5d0d692..99b23b58d 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -1142,7 +1142,6 @@ consider_port_binding(struct ovsdb_idl_index 
*sbrec_port_binding_by_name,
  * for frames that lack any 802.1Q header later. */
 if (tag || !strcmp(binding->type, "localnet")
 || !strcmp(binding->type, "l2gateway")) {
-match_set_dl_vlan(&match, htons(tag), 0);
 if (nested_container) {
 /* When a packet comes from a container sitting behind a
  * parent_port, we should let it loopback to other containers
@@ -1151,7 +1150,16 @@ consider_port_binding(struct ovsdb_idl_index 
*sbrec_port_binding_by_name,
 put_load(1, MFF_LOG_FLAGS, MLF_NESTED_CONTAINER_BIT, 1,
  ofpacts_p);
 }
-ofpact_put_STRIP_VLAN(ofpacts_p);
+
+/* For vlan-passthru switch ports that are untagged, skip
+ * matching/stripping VLAN header that originates from the VIF
+ * itself. */
+bool passthru = smap_get_bool(&binding->options,
+  "vlan-passthru", false);
+if (!passthru || tag) {
+match_set_dl_vlan(&match, htons(tag), 0);
+ofpact_put_STRIP_VLAN(ofpacts_p);
+}
 }
 
 /* Remember the size with just strip vlan added so far,
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 4783e43d7..79775ed9a 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -3008,6 +3008,11 @@ ovn_port_update_sbrec(struct northd_context *ctx,
 smap_add_format(&options,
 "qdisc_queue_id", "%d", queue_id);
 }
+
+if (smap_get_bool(&op->od->nbs->other_config, "vlan-passthru", 
false)) {
+smap_add(&options, "vlan-passthru", "true");
+}
+
 sbrec_port_binding_set_options(op->sb, &options);
 smap_destroy(&options);
 if (ovn_is_known_nb_lsp_type(op->nbsp->type)) {
diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index 4262b83b9..6e89e1237 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -125,7 +125,7 @@ OutProxy_Port_Binding(._uuid  = lsp._uuid,
   .__type = lsp.__type,
   .gateway_chassis= set_empty(),
   .ha_chassis_group   = sp.hac_group_uuid,
-  .options= lsp.options,
+  .options= options,
   .datapath   = sw.ls._uuid,
   .parent_port= lsp.parent_name,
   .tag= tag,
@@ -146,6 +146,14 @@ OutProxy_Port_Binding(._uuid  = lsp._uuid,
 Some{name} -> eids.insert("name", name)
 };
 eids
+},
+var options = {
+var options = lsp.options;
+match (sw.ls.other_config.get("vlan-passthru")) {
+None -> (),
+Some{passthru} -> options.insert("vlan-passthru", passthru)
+};
+options
 }.
 
 
diff --git a/ovn-nb.xml b/ovn-nb.xml
index b0a4adffe..a68482320 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -562,7 +562,11 @@
 
   
-Determines whether VLAN tagged incoming traffic should be allowed.
+Determines whether VLAN tagged incoming traffic should be allowed. Note
+that this may have security implications when enabled for a logical
+switch with a tag=0 localnet port. If not properly isolated from other
+localnet ports, fabric traffic that belongs t