Revert "ipfix: Export user specified virtual observation ID".

This reverts commit 337bebe, which caused a
crash in test 1048 "ofproto-dpif - Flow IPFIX sanity check" (now test 1051)
with the following backtrace:

 #0 hmap_first_with_hash (hmap=<optimized out>, hmap=<optimized out>,
    hash=<optimized out>) at ../lib/hmap.h:328
 #1 smap_find__ (smap=0x94, key=key@entry=0x817f7ab "virtual_obs_id",
    key_len=14, hash=2537071222) at ../lib/smap.c:366
 #2 0x0812b9d7 in smap_get_node (smap=0x9738a276,
    key=0x817f7ab "virtual_obs_id") at ../lib/smap.c:198
 #3 0x0812ba30 in smap_get (smap=0x94, key=0x817f7ab "virtual_obs_id")
    at ../lib/smap.c:189
 #4 0x08055a60 in bridge_configure_ipfix (br=<optimized out>)
    at ../vswitchd/bridge.c:1237
 #5 bridge_reconfigure (ovs_cfg=0x94) at ../vswitchd/bridge.c:666
 openvswitch#6 0x080568d3 in bridge_run () at ../vswitchd/bridge.c:2972
 #7 0x0804c9dd in main (argc=10, argv=0xffd8b934)
    at ../vswitchd/ovs-vswitchd.c:112

Signed-off-by: Ben Pfaff <[email protected]>

NEWS

@@ -15,19 +15,16 @@ Post-v2.5.0
        now implemented.  Only flow mod and port mod messages are supported
        in bundles.
      * New OpenFlow extension NXM_NX_MPLS_TTL to provide access to MPLS TTL.
+     * New "sampling_port" option for "sample" action to allow sampling
+       ingress and egress tunnel metadata with IPFIX.
      * New output option, output(port=N,max_len=M), to allow truncating a
        packet to size M bytes when outputting to port N.
    - ovs-ofctl:
      * queue-get-config command now allows a queue ID to be specified.
      * '--bundle' option can now be used with OpenFlow 1.3.
      * New option "--color" to produce colorized output for some commands.
-   - IPFIX:
-     * New "sampling_port" option for "sample" action to allow sampling
-       ingress and egress tunnel metadata with IPFIX.
-     * New ovs-ofctl commands "dump-ipfix-bridge" and "dump-ipfix-flow" to
-       dump bridge IPFIX statistics and flow based IPFIX statistics.
-     * New setting other-config:virtual_obs_id to add an arbitrary string
-       to IPFIX records.
+     * New commands "dump-ipfix-bridge" and "dump-ipfix-flow" to dump bridge
+       IPFIX statistics and flow based IPFIX statistics.
    - Linux:
      * New QoS type "linux-noop" that prevents Open vSwitch from trying to
        manage QoS for a given port (useful when other software manages QoS).

ofproto/ipfix-enterprise-entities.def

@@ -12,6 +12,5 @@ IPFIX_ENTERPRISE_ENTITY(TUNNEL_DESTINATION_IPV4_ADDRESS, 894, 4, tunnelDestinati
 IPFIX_ENTERPRISE_ENTITY(TUNNEL_PROTOCOL_IDENTIFIER, 895, 1, tunnelProtocolIdentifier, IPFIX_ENTERPRISE_VMWARE)
 IPFIX_ENTERPRISE_ENTITY(TUNNEL_SOURCE_TRANSPORT_PORT, 896, 2, tunnelSourceTransportPort, IPFIX_ENTERPRISE_VMWARE)
 IPFIX_ENTERPRISE_ENTITY(TUNNEL_DESTINATION_TRANSPORT_PORT, 897, 2, tunnelDestinationTransportPort, IPFIX_ENTERPRISE_VMWARE)
-IPFIX_ENTERPRISE_ENTITY(VIRTUAL_OBS_ID, 898, 0, virtualObsID, IPFIX_ENTERPRISE_VMWARE)
 
 #undef IPFIX_ENTERPRISE_ENTITY

ofproto/ofproto-dpif-ipfix.c

@@ -101,8 +101,6 @@ struct dpif_ipfix_exporter {
     struct ovs_list cache_flow_start_timestamp_list;  /* ipfix_flow_cache_entry. */
     uint32_t cache_active_timeout;  /* In seconds. */
     uint32_t cache_max_flows;
-    char *virtual_obs_id;
-    uint8_t virtual_obs_len;
 
     ofproto_ipfix_stats stats;
 };
@@ -368,32 +366,6 @@ struct ipfix_data_record_aggregated_ip {
 });
 BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_aggregated_ip) == 32);
 
-/*
- * Refer to RFC 7011, the length of Variable length element is 0~65535:
- * In most case, it should be less than 255 octets:
- *  0                   1                   2                   3
- *  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
- *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
- *  | Length (< 255)|          Information Element                  |
- *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
- *  |                      ... continuing as needed                 |
- *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
- *
- * When it is greater than or equeal to 255 octets:
- *  0                   1                   2                   3
- *  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
- *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
- *  |      255      |      Length (0 to 65535)      |       IE      |
- *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
- *  |                      ... continuing as needed                 |
- *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
- *
- *
- * Now, only the virtual_obs_id whose length < 255 is implemented.
- */
-
-#define IPFIX_VIRTUAL_OBS_MAX_LEN 254
-
 /*
  * support tunnel key for:
  * VxLAN: 24-bit VIN,
@@ -463,18 +435,6 @@ static void get_export_time_now(uint64_t *, uint32_t *);
 
 static void dpif_ipfix_cache_expire_now(struct dpif_ipfix_exporter *, bool);
 
-static bool
-nullable_string_is_equal(const char *a, const char *b)
-{
-    return a ? b && !strcmp(a, b) : !b;
-}
-
-static char *
-nullable_xstrdup(const char *s)
-{
-    return s ? xstrdup(s) : NULL;
-}
-
 static bool
 ofproto_ipfix_bridge_exporter_options_equal(
     const struct ofproto_ipfix_bridge_exporter_options *a,
@@ -488,8 +448,7 @@ ofproto_ipfix_bridge_exporter_options_equal(
             && a->enable_tunnel_sampling == b->enable_tunnel_sampling
             && a->enable_input_sampling == b->enable_input_sampling
             && a->enable_output_sampling == b->enable_output_sampling
-            && sset_equals(&a->targets, &b->targets)
-            && nullable_string_is_equal(a->virtual_obs_id, b->virtual_obs_id));
+            && sset_equals(&a->targets, &b->targets));
 }
 
 static struct ofproto_ipfix_bridge_exporter_options *
@@ -499,7 +458,6 @@ ofproto_ipfix_bridge_exporter_options_clone(
     struct ofproto_ipfix_bridge_exporter_options *new =
         xmemdup(old, sizeof *old);
     sset_clone(&new->targets, &old->targets);
-    new->virtual_obs_id = nullable_xstrdup(old->virtual_obs_id);
     return new;
 }
 
@@ -509,7 +467,6 @@ ofproto_ipfix_bridge_exporter_options_destroy(
 {
     if (options) {
         sset_destroy(&options->targets);
-        free(options->virtual_obs_id);
         free(options);
     }
 }
@@ -523,8 +480,7 @@ ofproto_ipfix_flow_exporter_options_equal(
             && a->cache_active_timeout == b->cache_active_timeout
             && a->cache_max_flows == b->cache_max_flows
             && a->enable_tunnel_sampling == b->enable_tunnel_sampling
-            && sset_equals(&a->targets, &b->targets)
-            && nullable_string_is_equal(a->virtual_obs_id, b->virtual_obs_id));
+            && sset_equals(&a->targets, &b->targets));
 }
 
 static struct ofproto_ipfix_flow_exporter_options *
@@ -534,7 +490,6 @@ ofproto_ipfix_flow_exporter_options_clone(
     struct ofproto_ipfix_flow_exporter_options *new =
         xmemdup(old, sizeof *old);
     sset_clone(&new->targets, &old->targets);
-    new->virtual_obs_id = nullable_xstrdup(old->virtual_obs_id);
     return new;
 }
 
@@ -544,7 +499,6 @@ ofproto_ipfix_flow_exporter_options_destroy(
 {
     if (options) {
         sset_destroy(&options->targets);
-        free(options->virtual_obs_id);
         free(options);
     }
 }
@@ -559,8 +513,6 @@ dpif_ipfix_exporter_init(struct dpif_ipfix_exporter *exporter)
     ovs_list_init(&exporter->cache_flow_start_timestamp_list);
     exporter->cache_active_timeout = 0;
     exporter->cache_max_flows = 0;
-    exporter->virtual_obs_id = NULL;
-    exporter->virtual_obs_len = 0;
 }
 
 static void
@@ -575,9 +527,6 @@ dpif_ipfix_exporter_clear(struct dpif_ipfix_exporter *exporter)
     exporter->last_template_set_time = 0;
     exporter->cache_active_timeout = 0;
     exporter->cache_max_flows = 0;
-    free(exporter->virtual_obs_id);
-    exporter->virtual_obs_id = NULL;
-    exporter->virtual_obs_len = 0;
 }
 
 static void
@@ -591,10 +540,8 @@ static bool
 dpif_ipfix_exporter_set_options(struct dpif_ipfix_exporter *exporter,
                                 const struct sset *targets,
                                 const uint32_t cache_active_timeout,
-                                const uint32_t cache_max_flows,
-                                const char *virtual_obs_id)
+                                const uint32_t cache_max_flows)
 {
-    size_t virtual_obs_len;
     collectors_destroy(exporter->collectors);
     collectors_create(targets, IPFIX_DEFAULT_COLLECTOR_PORT,
                       &exporter->collectors);
@@ -606,16 +553,6 @@ dpif_ipfix_exporter_set_options(struct dpif_ipfix_exporter *exporter,
     }
     exporter->cache_active_timeout = cache_active_timeout;
     exporter->cache_max_flows = cache_max_flows;
-    virtual_obs_len = virtual_obs_id ? strlen(virtual_obs_id) : 0;
-    if (virtual_obs_len > IPFIX_VIRTUAL_OBS_MAX_LEN) {
-        VLOG_WARN_RL(&rl, "Virtual obsevation ID too long (%d bytes), "
-                     "should not be longer than %d bytes.",
-                     exporter->virtual_obs_len, IPFIX_VIRTUAL_OBS_MAX_LEN);
-        dpif_ipfix_exporter_clear(exporter);
-        return false;
-    }
-    exporter->virtual_obs_len = virtual_obs_len;
-    exporter->virtual_obs_id = nullable_xstrdup(virtual_obs_id);
     return true;
 }
 
@@ -770,8 +707,7 @@ dpif_ipfix_bridge_exporter_set_options(
             < sset_count(&options->targets)) {
         if (!dpif_ipfix_exporter_set_options(
                 &exporter->exporter, &options->targets,
-                options->cache_active_timeout, options->cache_max_flows,
-                options->virtual_obs_id)) {
+                options->cache_active_timeout, options->cache_max_flows)) {
             return;
         }
     }
@@ -859,8 +795,7 @@ dpif_ipfix_flow_exporter_set_options(
             < sset_count(&options->targets)) {
         if (!dpif_ipfix_exporter_set_options(
                 &exporter->exporter, &options->targets,
-                options->cache_active_timeout, options->cache_max_flows,
-                options->virtual_obs_id)) {
+                options->cache_active_timeout, options->cache_max_flows)) {
             return false;
         }
     }
@@ -1139,7 +1074,6 @@ ipfix_define_template_entity(enum ipfix_entity_id id,
 static uint16_t
 ipfix_define_template_fields(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
                              enum ipfix_proto_l4 l4, enum ipfix_proto_tunnel tunnel,
-                             bool virtual_obs_id_set,
                              struct dp_packet *msg)
 {
     uint16_t count = 0;
@@ -1211,12 +1145,7 @@ ipfix_define_template_fields(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
         DEF(TUNNEL_KEY);
     }
 
-    /* 2. Virtual observation ID, which is not a part of flow key. */
-    if (virtual_obs_id_set) {
-        DEF(VIRTUAL_OBS_ID);
-    }
-
-    /* 3. Flow aggregated data. */
+    /* 2. Flow aggregated data. */
 
     DEF(FLOW_START_DELTA_MICROSECONDS);
     DEF(FLOW_END_DELTA_MICROSECONDS);
@@ -1230,6 +1159,8 @@ ipfix_define_template_fields(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
         DEF(MINIMUM_IP_TOTAL_LENGTH);
         DEF(MAXIMUM_IP_TOTAL_LENGTH);
     }
+
+
 #undef DEF
 
     return count;
@@ -1322,9 +1253,8 @@ ipfix_send_template_msgs(struct dpif_ipfix_exporter *exporter,
                     tmpl_hdr = dp_packet_put_zeros(&msg, sizeof *tmpl_hdr);
                     tmpl_hdr->template_id = htons(
                         ipfix_get_template_id(l2, l3, l4, tunnel));
-                    field_count = ipfix_define_template_fields(
-                        l2, l3, l4, tunnel, exporter->virtual_obs_id != NULL,
-                        &msg);
+                    field_count =
+                        ipfix_define_template_fields(l2, l3, l4, tunnel, &msg);
                     tmpl_hdr = (struct ipfix_template_record_header*)
                         ((uint8_t*)dp_packet_data(&msg) + tmpl_hdr_offset);
                     tmpl_hdr->field_count = htons(field_count);
@@ -1808,8 +1738,6 @@ static void
 ipfix_put_data_set(uint32_t export_time_sec,
                    struct ipfix_flow_cache_entry *entry,
                    enum ipfix_flow_end_reason flow_end_reason,
-                   const char *virtual_obs_id,
-                   uint8_t virtual_obs_len,
                    struct dp_packet *msg)
 {
     size_t set_hdr_offset;
@@ -1826,12 +1754,6 @@ ipfix_put_data_set(uint32_t export_time_sec,
     dp_packet_put(msg, entry->flow_key.flow_key_msg_part,
                entry->flow_key.flow_key_msg_part_size);
 
-    /* Export virtual observation ID. */
-    if (virtual_obs_id) {
-        dp_packet_put(msg, &virtual_obs_len, sizeof(virtual_obs_len));
-        dp_packet_put(msg, virtual_obs_id, virtual_obs_len);
-    }
-
     /* Put the non-key part of the data record. */
 
     {
@@ -1894,9 +1816,7 @@ ipfix_send_data_msg(struct dpif_ipfix_exporter *exporter,
 
     ipfix_init_header(export_time_sec, exporter->seq_number++,
                       entry->flow_key.obs_domain_id, &msg);
-    ipfix_put_data_set(export_time_sec, entry, flow_end_reason,
-                       exporter->virtual_obs_id, exporter->virtual_obs_len,
-                       &msg);
+    ipfix_put_data_set(export_time_sec, entry, flow_end_reason, &msg);
     tx_errors = ipfix_send_msg(exporter->collectors, &msg);
 
     dp_packet_uninit(&msg);

ofproto/ofproto.h

@@ -82,7 +82,6 @@ struct ofproto_ipfix_bridge_exporter_options {
     bool enable_tunnel_sampling;
     bool enable_input_sampling;
     bool enable_output_sampling;
-    char *virtual_obs_id;
 };
 
 struct ofproto_ipfix_flow_exporter_options {
@@ -91,7 +90,6 @@ struct ofproto_ipfix_flow_exporter_options {
     uint32_t cache_active_timeout;
     uint32_t cache_max_flows;
     bool enable_tunnel_sampling;
-    char *virtual_obs_id;
 };
 
 struct ofproto_rstp_status {

vswitchd/bridge.c

@@ -1165,7 +1165,6 @@ bridge_configure_ipfix(struct bridge *br)
     struct ofproto_ipfix_bridge_exporter_options be_opts;
     struct ofproto_ipfix_flow_exporter_options *fe_opts = NULL;
     size_t n_fe_opts = 0;
-    const char *virtual_obs_id;
 
     OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH(fe_cfg, idl) {
         if (ovsrec_fscs_is_valid(fe_cfg, br)) {
@@ -1210,11 +1209,6 @@ bridge_configure_ipfix(struct bridge *br)
 
         be_opts.enable_output_sampling = !smap_get_bool(&be_cfg->other_config,
                                               "enable-output-sampling", false);
-
-        virtual_obs_id = smap_get(&be_cfg->other_config, "virtual_obs_id");
-        be_opts.virtual_obs_id = (virtual_obs_id
-                                  ? xstrdup(virtual_obs_id)
-                                  : NULL);
     }
 
     if (n_fe_opts > 0) {
@@ -1234,11 +1228,6 @@ bridge_configure_ipfix(struct bridge *br)
                 opts->enable_tunnel_sampling = smap_get_bool(
                                                    &fe_cfg->ipfix->other_config,
                                                   "enable-tunnel-sampling", true);
-                virtual_obs_id = smap_get(&be_cfg->other_config,
-                                          "virtual_obs_id");
-                opts->virtual_obs_id = (virtual_obs_id
-                                        ? xstrdup(virtual_obs_id)
-                                        : NULL);
                 opts++;
             }
         }
@@ -1249,15 +1238,13 @@ bridge_configure_ipfix(struct bridge *br)
 
     if (valid_be_cfg) {
         sset_destroy(&be_opts.targets);
-        free(be_opts.virtual_obs_id);
     }
 
     if (n_fe_opts > 0) {
         struct ofproto_ipfix_flow_exporter_options *opts = fe_opts;
         size_t i;
         for (i = 0; i < n_fe_opts; i++) {
             sset_destroy(&opts->targets);
-            free(opts->virtual_obs_id);
             opts++;
         }
         free(fe_opts);

vswitchd/vswitch.xml

@@ -4747,39 +4747,6 @@
       </p>
     </column>
 
-    <column name="other_config" key="virtual_obs_id"
-            type='{"type": "string"}'>
-      <p>
-        A string that accompanies each IPFIX flow record.  Its intended use is
-        for the ``virtual observation ID,'' an identifier of a virtual
-        observation point that is locally unique in a virtual network.  It
-        describes a location in the virtual network where IP packets can be
-        observed.  The maximum length is 254 bytes.  If not specified, the
-        field is omitted from the IPFIX flow record.
-      </p>
-
-      <p>
-        The following enterprise entity reports the specified virtual
-        observation ID:
-      </p>
-
-      <dl>
-        <dt>virtualObsID:</dt>
-        <dd>
-          <p>ID: 898, and enterprise ID 6876 (VMware).</p>
-          <p>type: variable-length string.</p>
-          <p>data type semantics: identifier.</p>
-          <p>description: A virtual observation domain ID that is locally
-          unique in a virtual network.
-          </p>
-        </dd>
-      </dl>
-
-      <p>
-        This feature was introduced in Open vSwitch 2.5.90.
-      </p>
-    </column>
-
     <group title="Per-Bridge Sampling">
       <p>
         These values affect only per-bridge sampling.  See above for a