Revert "Revert openshift hack to dup untagged packets (#266)" (#281)

This reverts commit e21bcd2.

Apparently they still need them.

Signed-off-by: Madhu Challa <challa@gmail.com>

agent-ovs/lib/FSEndpointSource.cpp

@@ -116,6 +116,7 @@ void FSEndpointSource::updated(const fs::path& filePath) {
     static const std::string SNAT_UUIDS("snat-uuids");
     static const std::string ACTIVE_ACTIVE_AAP("active-active-aap");
     static const std::string EP_DISABLE_ADV("disable-adv");
+    static const std::string EP_ACCESS_ALLOW_UNTAGGED("access-allow-untagged");
 
     static const std::string NEUTRON_NW("neutron-network");
 
@@ -474,6 +475,11 @@ void FSEndpointSource::updated(const fs::path& filePath) {
         if (disableAdv)
             newep.setDisableAdv(disableAdv.get());
 
+        optional<bool> accessAllowUntagged =
+            properties.get_optional<bool>(EP_ACCESS_ALLOW_UNTAGGED);
+        if (accessAllowUntagged)
+            newep.setAccessAllowUntagged(accessAllowUntagged.get());
+
         optional<bool> provider_vlan =
                 properties.get_optional<bool>(EP_PROVIDER_VLAN_FLAG);
         if(provider_vlan && provider_vlan.get()) {

agent-ovs/lib/FSExternalEndpointSource.cpp

@@ -90,6 +90,7 @@ void FSExternalEndpointSource::updated(const fs::path& filePath) {
     static const std::string DHCP_T2("t2");
     static const std::string DHCP_PREFERRED_LIFETIME("preferred-lifetime");
     static const std::string DHCP_VALID_LIFETIME("valid-lifetime");
+    static const std::string EP_ACCESS_ALLOW_UNTAGGED("access-allow-untagged");
 
     try {
         using boost::property_tree::ptree;
@@ -302,6 +303,11 @@ void FSExternalEndpointSource::updated(const fs::path& filePath) {
             newep.setDHCPv6Config(c);
         }
 
+        optional<bool> accessAllowUntagged =
+            properties.get_optional<bool>(EP_ACCESS_ALLOW_UNTAGGED);
+        if (accessAllowUntagged)
+            newep.setAccessAllowUntagged(accessAllowUntagged.get());
+
         ep_map_t::const_iterator it = knownEps.find(pathstr);
         if (it != knownEps.end()) {
             if (newep.getUUID() != it->second)

agent-ovs/lib/include/opflexagent/Endpoint.h

@@ -38,7 +38,8 @@ class Endpoint {
      * Default constructor for containers
      */
     Endpoint() : promiscuousMode(false), discoveryProxyMode(false), natMode(false),
-                 external(false), aapModeAA(false), disableAdv(false), extEncap(0) {
+                 external(false), aapModeAA(false), disableAdv(false),
+                 accessAllowUntagged(false), extEncap(0) {
 #ifdef HAVE_PROMETHEUS_SUPPORT
         annotateEpName = false;
         attr_hash = 0;
@@ -54,7 +55,8 @@ class Endpoint {
      */
     explicit Endpoint(const std::string& uuid_)
         : uuid(uuid_), promiscuousMode(false), discoveryProxyMode(false), natMode(false),
-          external(false), aapModeAA(false), disableAdv(false), extEncap(0) {
+          external(false), aapModeAA(false), disableAdv(false),
+          accessAllowUntagged(false), extEncap(0) {
 #ifdef HAVE_PROMETHEUS_SUPPORT
         annotateEpName = false;
         attr_hash = 0;
@@ -485,6 +487,28 @@ class Endpoint {
         return disableAdv;
     }
 
+    /**
+      * Set if access vlan should also allow untagged
+      * traffic like Openshift bootstrap use case
+      *
+      * @param accessAllowUntagged the new value of
+      * accessAllowUntagged flag
+      */
+    void setAccessAllowUntagged(bool accessAllowUntagged) {
+        this->accessAllowUntagged = accessAllowUntagged;
+    }
+
+    /**
+      * Get the current value of accessAllowUntagged flag
+      * for this endpoint
+      *
+      * @return true if untagged traffic should also be allowed
+      * else return false
+      */
+    bool isAccessAllowUntagged() const {
+        return accessAllowUntagged;
+    }
+
     /**
      * Clear the attribute map
      */
@@ -1306,6 +1330,7 @@ class Endpoint {
     bool external;
     bool aapModeAA;
     bool disableAdv;
+    bool accessAllowUntagged;
     attr_map_t attributes;
 #ifdef HAVE_PROMETHEUS_SUPPORT
     bool annotateEpName;

agent-ovs/ovs/AccessFlowManager.cpp

@@ -180,11 +180,18 @@ static FlowEntryPtr flowEmptySecGroup(uint32_t emptySecGrpSetId) {
     return noSecGrp.build();
 }
 
-static void flowBypassDhcpRequest(FlowEntryList& el, bool v4, uint32_t inport,
+static uint64_t getPushVlanMeta(std::shared_ptr<const Endpoint>& ep) {
+    return ep->isAccessAllowUntagged() ?
+        flow::meta::access_out::UNTAGGED_AND_PUSH_VLAN :
+        flow::meta::access_out::PUSH_VLAN;
+}
+
+static void flowBypassDhcpRequest(FlowEntryList& el, bool v4,
+                                  bool skip_pop_vlan, uint32_t inport,
                                   uint32_t outport,
                                   std::shared_ptr<const Endpoint>& ep ) {
     FlowBuilder fb;
-    if (ep->getAccessIfaceVlan()) {
+    if (ep->getAccessIfaceVlan() && !skip_pop_vlan) {
         fb.priority(201).inPort(inport);
     } else {
         fb.priority(200).inPort(inport);
@@ -193,22 +200,26 @@ static void flowBypassDhcpRequest(FlowEntryList& el, bool v4, uint32_t inport,
     flowutils::match_dhcp_req(fb, v4);
     fb.action().reg(MFF_REG7, outport);
 
-    if (ep->getAccessIfaceVlan()) {
+    if (ep->getAccessIfaceVlan() && !skip_pop_vlan) {
         fb.vlan(ep->getAccessIfaceVlan().get());
         fb.action().metadata(flow::meta::access_out::POP_VLAN,
                              flow::meta::out::MASK);
     }
 
+    if (skip_pop_vlan)
+        fb.tci(0, 0x1fff);
+
     fb.action().go(AccessFlowManager::OUT_TABLE_ID);
     fb.build(el);
 }
 
 static void flowBypassFloatingIP(FlowEntryList& el, uint32_t inport,
                                  uint32_t outport, bool in,
+                                 bool skip_pop_vlan,
                                  address& floatingIp,
                                  std::shared_ptr<const Endpoint>& ep ) {
     FlowBuilder fb;
-    if (ep->getAccessIfaceVlan()) {
+    if (ep->getAccessIfaceVlan() && !skip_pop_vlan) {
         fb.priority(201).inPort(inport);
     } else {
         fb.priority(200).inPort(inport);
@@ -227,12 +238,11 @@ static void flowBypassFloatingIP(FlowEntryList& el, uint32_t inport,
     }
 
     fb.action().reg(MFF_REG7, outport);
-    if (ep->getAccessIfaceVlan()) {
+    if (ep->getAccessIfaceVlan() && !skip_pop_vlan) {
         if (in) {
             fb.action()
                 .reg(MFF_REG5, ep->getAccessIfaceVlan().get())
-                .metadata(flow::meta::access_out::PUSH_VLAN,
-                          flow::meta::out::MASK);
+                .metadata(getPushVlanMeta(ep), flow::meta::out::MASK);
         } else {
             fb.vlan(ep->getAccessIfaceVlan().get());
             fb.action()
@@ -241,6 +251,9 @@ static void flowBypassFloatingIP(FlowEntryList& el, uint32_t inport,
         }
     }
 
+    if (skip_pop_vlan && !in)
+        fb.tci(0, 0x1fff);
+
     fb.action().go(AccessFlowManager::OUT_TABLE_ID);
     fb.build(el);
 }
@@ -264,6 +277,22 @@ void AccessFlowManager::createStaticFlows() {
             .action()
             .pushVlan().regMove(MFF_REG5, MFF_VLAN_VID).outputReg(MFF_REG7)
             .parent().build(outFlows);
+        /*
+         * The packet is replicated for a specical case of
+         * Openshift bootstrap that does not use vlan 4094
+         * This is ugly but they do not have iproute2/tc
+         * installed to do this in a cleaner way
+         * This duplication only happens when endpoint
+         * file has "access-interface-vlan" attr
+         */
+        FlowBuilder()
+            .priority(1)
+            .metadata(flow::meta::access_out::UNTAGGED_AND_PUSH_VLAN,
+                      flow::meta::out::MASK)
+            .action()
+            .outputReg(MFF_REG7)
+            .pushVlan().regMove(MFF_REG5, MFF_VLAN_VID).outputReg(MFF_REG7)
+            .parent().build(outFlows);
         outFlows.push_back(flowutils::default_out_flow());
 
         switchManager.writeFlow("static", OUT_TABLE_ID, outFlows);
@@ -384,17 +413,48 @@ void AccessFlowManager::handleEndpointUpdate(const string& uuid) {
 
         }
 
+        /*
+         * We allow without tags to handle Openshift bootstrap
+         */
+        if (ep->isAccessAllowUntagged() && ep->getAccessIfaceVlan()) {
+            FlowBuilder inSkipVlan;
+
+            inSkipVlan.priority(99).inPort(accessPort)
+                      .tci(0, 0x1fff);
+            if (zoneId != static_cast<uint16_t>(-1))
+                inSkipVlan.action()
+                    .reg(MFF_REG6, zoneId);
+
+            inSkipVlan.action()
+                .reg(MFF_REG0, secGrpSetId)
+                .reg(MFF_REG7, uplinkPort)
+                .go(SEC_GROUP_OUT_TABLE_ID);
+            inSkipVlan.build(el);
+        }
+
         /*
          * Allow DHCP request to bypass the access bridge policy when
          * virtual DHCP is enabled.
+         * We allow both with / without tags to handle Openshift
+         * bootstrap
          */
         optional<Endpoint::DHCPv4Config> v4c = ep->getDHCPv4Config();
-        if (v4c)
-            flowBypassDhcpRequest(el, true, accessPort, uplinkPort, ep);
+        if (v4c) {
+            flowBypassDhcpRequest(el, true, false, accessPort,
+                                  uplinkPort, ep);
+            if (ep->isAccessAllowUntagged() && ep->getAccessIfaceVlan())
+                flowBypassDhcpRequest(el, true, true, accessPort,
+                                      uplinkPort, ep);
+        }
 
         optional<Endpoint::DHCPv6Config> v6c = ep->getDHCPv6Config();
-        if(v6c)
-            flowBypassDhcpRequest(el, false, accessPort, uplinkPort, ep);
+        if(v6c) {
+            flowBypassDhcpRequest(el, false, false, accessPort,
+                                  uplinkPort, ep);
+            if (ep->isAccessAllowUntagged() && ep->getAccessIfaceVlan())
+                flowBypassDhcpRequest(el, false, true, accessPort,
+                                      uplinkPort, ep);
+        }
 
         {
             FlowBuilder out;
@@ -409,8 +469,7 @@ void AccessFlowManager::handleEndpointUpdate(const string& uuid) {
             if (ep->getAccessIfaceVlan()) {
                 out.action()
                     .reg(MFF_REG5, ep->getAccessIfaceVlan().get())
-                    .metadata(flow::meta::access_out::PUSH_VLAN,
-                              flow::meta::out::MASK);
+                    .metadata(getPushVlanMeta(ep), flow::meta::out::MASK);
             }
             out.action().go(SEC_GROUP_IN_TABLE_ID);
             out.build(el);
@@ -452,9 +511,19 @@ void AccessFlowManager::handleEndpointUpdate(const string& uuid) {
                 if (floatingIp.is_unspecified()) continue;
             }
             flowBypassFloatingIP(el, accessPort, uplinkPort, false,
-                                 floatingIp, ep);
+                                 false, floatingIp, ep);
             flowBypassFloatingIP(el, uplinkPort, accessPort, true,
-                                 floatingIp, ep);
+                                 false, floatingIp, ep);
+            /*
+             * We allow both with / without tags to handle Openshift
+             * bootstrap
+             */
+            if (ep->isAccessAllowUntagged() && ep->getAccessIfaceVlan()) {
+                flowBypassFloatingIP(el, accessPort, uplinkPort, false,
+                                     true, floatingIp, ep);
+                flowBypassFloatingIP(el, uplinkPort, accessPort, true,
+                                     true, floatingIp, ep);
+            }
         }
     }
     switchManager.writeFlow(uuid, GROUP_MAP_TABLE_ID, el);

agent-ovs/ovs/PacketInHandler.cpp

@@ -149,7 +149,10 @@ static void send_packet_out(Agent& agent,
     string iface;
     opt_output_act_t outActions =
         tunnelOutActions(intFlowManager, egUri, out_port);
+    opt_output_act_t outActionsSkipVlan;
     SwitchConnection* conn = intConn;
+    ep_ptr ep;
+    bool send_untagged = false;
 
     try {
         if (out_port == OFPP_IN_PORT)
@@ -168,7 +171,7 @@ static void send_packet_out(Agent& agent,
             LOG(WARNING) << "Multiple possible endpoints for output packet "
                          << " on " << iface;
 
-        ep_ptr ep = agent.getEndpointManager().getEndpoint(*eps.begin());
+        ep = agent.getEndpointManager().getEndpoint(*eps.begin());
         if (ep && ep->getAccessInterface() && ep->getAccessUplinkInterface()) {
             if (!accConn || !accPortMapper) {
                 return;
@@ -183,6 +186,9 @@ static void send_packet_out(Agent& agent,
                 out_port = accPort;
 
                 if (ep->getAccessIfaceVlan()) {
+                    outActionsSkipVlan = outActions;
+                    if (ep->isAccessAllowUntagged())
+                        send_untagged = true;
                     outActions = [&ep](ActionBuilder& ab) {
                         ab.pushVlan();
                         ab.setVlanVid(ep->getAccessIfaceVlan().get());
@@ -195,6 +201,11 @@ static void send_packet_out(Agent& agent,
     }
 
     send_packet_out(conn, b, proto, in_port, out_port, outActions);
+    /*
+     * Openshift bootstrap does not support vlans, so make a copy
+     */
+    if (send_untagged)
+        send_packet_out(conn, b, proto, in_port, out_port, outActionsSkipVlan);
 }
 
 /**

agent-ovs/ovs/include/FlowConstants.h

@@ -183,6 +183,11 @@ const uint64_t POP_VLAN = 0x1;
  */
 const uint64_t PUSH_VLAN = 0x2;
 
+/**
+ * Replicate the packet both untagged followed by tagged
+ */
+const uint64_t UNTAGGED_AND_PUSH_VLAN = 0x3;
+
 } // namespace access
 
 } // namespace meta

agent-ovs/ovs/test/AccessFlowManager_test.cpp

@@ -477,7 +477,11 @@ void AccessFlowManagerFixture::initExpStatic() {
          .actions().out(OUTPORT).done());
     ADDF(Bldr().table(OUT).priority(1)
          .isMdAct(opflexagent::flow::meta::access_out::PUSH_VLAN)
-          .actions().pushVlan().move(FD12, VLAN).out(OUTPORT).done());
+         .actions().pushVlan().move(FD12, VLAN).out(OUTPORT).done());
+    ADDF(Bldr().table(OUT).priority(1)
+         .isMdAct(opflexagent::flow::meta::access_out::UNTAGGED_AND_PUSH_VLAN)
+         .actions().out(OUTPORT).pushVlan()
+         .move(FD12, VLAN).out(OUTPORT).done());
     ADDF(Bldr().table(OUT).priority(1)
          .isMdAct(opflexagent::flow::meta::access_out::POP_VLAN)
          .isVlanTci("0x1000/0x1000")
@@ -513,6 +517,15 @@ void AccessFlowManagerFixture::initExpDhcpEp(shared_ptr<Endpoint>& ep) {
              .load(OUTPORT, uplink)
              .mdAct(opflexagent::flow::meta::access_out::POP_VLAN)
              .go(OUT).done());
+        if (ep->isAccessAllowUntagged() && ep->getAccessIfaceVlan()) {
+            ADDF(Bldr()
+                 .table(GRP).priority(200).udp().in(access)
+                 .isVlanTci("0x0000/0x1fff")
+                 .isTpSrc(68).isTpDst(67)
+                 .actions()
+                 .load(OUTPORT, uplink)
+                 .go(OUT).done());
+        }
     }
     if (ep->getDHCPv6Config()) {
         ADDF(Bldr()
@@ -523,6 +536,15 @@ void AccessFlowManagerFixture::initExpDhcpEp(shared_ptr<Endpoint>& ep) {
              .load(OUTPORT, uplink)
              .mdAct(opflexagent::flow::meta::access_out::POP_VLAN)
              .go(OUT).done());
+        if (ep->isAccessAllowUntagged() && ep->getAccessIfaceVlan()) {
+            ADDF(Bldr()
+                 .table(GRP).priority(200).udp6().in(access)
+                 .isVlanTci("0x0000/0x1fff")
+                 .isTpSrc(546).isTpDst(547)
+                 .actions()
+                 .load(OUTPORT, uplink)
+                 .go(OUT).done());
+        }
     }
 }
 
@@ -541,6 +563,14 @@ void AccessFlowManagerFixture::initExpEp(shared_ptr<Endpoint>& ep) {
              .load(OUTPORT, uplink)
              .mdAct(opflexagent::flow::meta::access_out::POP_VLAN)
              .go(OUT_POL).done());
+        if (ep->isAccessAllowUntagged()) {
+            ADDF(Bldr().table(GRP).priority(99).in(access)
+                 .isVlanTci("0x0000/0x1fff")
+                 .actions()
+                 .load(RD, zoneId).load(SEPG, 1)
+                 .load(OUTPORT, uplink)
+                 .go(OUT_POL).done());
+        }
         ADDF(Bldr().table(GRP).priority(100).in(uplink)
              .actions().load(RD, zoneId).load(SEPG, 1).load(OUTPORT, access)
              .load(FD, ep->getAccessIfaceVlan().get())