Merge pull request #508 from msherif1234/xlat-icmp

NETOBSERV-2055: Xlat ICMP code is redundant with icmp code field so removing it
netobserv · Jan 15, 2025 · 472038b · 472038b
2 parents 73256ab + 63dcf7c
commit 472038b
Show file tree

Hide file tree

Showing 17 changed files with 49 additions and 51 deletions.
diff --git a/bpf/pkt_translation.h b/bpf/pkt_translation.h
@@ -10,8 +10,7 @@
 #define s6_addr in6_u.u6_addr8
 
 static inline void dump_xlated_flow(struct translated_flow_t *flow) {
-    BPF_PRINTK("zone_id %d sport %d dport %d icmpId %d\n", flow->zone_id, flow->sport, flow->dport,
-               flow->icmp_id);
+    BPF_PRINTK("zone_id %d sport %d dport %d\n", flow->zone_id, flow->sport, flow->dport);
     int i;
     for (i = 0; i < IP_MAX_LEN; i += 4) {
         BPF_PRINTK("scrIP[%d]:%d.%d.%d.%d\n", i, flow->saddr[0 + i], flow->saddr[1 + i],
@@ -23,12 +22,15 @@ static inline void dump_xlated_flow(struct translated_flow_t *flow) {
     }
 }
 
-static inline void parse_tuple(struct nf_conntrack_tuple *t, struct translated_flow_t *flow,
-                               u16 zone_id, u16 family, bool invert) {
+static void __always_inline parse_tuple(struct nf_conntrack_tuple *t,
+                                        struct translated_flow_t *flow, u16 zone_id, u16 family,
+                                        u8 protocol, bool invert) {
     __builtin_memset(flow, 0, sizeof(*flow));
     if (invert) {
-        flow->dport = bpf_ntohs(t->src.u.all);
-        flow->sport = bpf_ntohs(t->dst.u.all);
+        if (is_transport_protocol(protocol)) {
+            flow->dport = bpf_ntohs(t->src.u.all);
+            flow->sport = bpf_ntohs(t->dst.u.all);
+        }
 
         switch (family) {
         case AF_INET:
@@ -44,8 +46,10 @@ static inline void parse_tuple(struct nf_conntrack_tuple *t, struct translated_f
             break;
         }
     } else {
-        flow->dport = bpf_ntohs(t->dst.u.all);
-        flow->sport = bpf_ntohs(t->src.u.all);
+        if (is_transport_protocol(protocol)) {
+            flow->dport = bpf_ntohs(t->dst.u.all);
+            flow->sport = bpf_ntohs(t->src.u.all);
+        }
 
         switch (family) {
         case AF_INET:
@@ -61,7 +65,6 @@ static inline void parse_tuple(struct nf_conntrack_tuple *t, struct translated_f
             break;
         }
     }
-    flow->icmp_id = t->src.u.icmp.id;
     flow->zone_id = zone_id;
     dump_xlated_flow(flow);
 }
@@ -73,7 +76,7 @@ static inline long translate_lookup_and_update_flow(flow_id *id, u16 flags,
     long ret = 0;
     struct translated_flow_t orig;
 
-    parse_tuple(orig_t, &orig, zone_id, family, false);
+    parse_tuple(orig_t, &orig, zone_id, family, id->transport_protocol, false);
 
     // update id with original flow info
     __builtin_memcpy(id->src_ip, orig.saddr, IP_MAX_LEN);
@@ -85,7 +88,8 @@ static inline long translate_lookup_and_update_flow(flow_id *id, u16 flags,
     additional_metrics *extra_metrics = bpf_map_lookup_elem(&additional_flow_metrics, id);
     if (extra_metrics != NULL) {
         extra_metrics->end_mono_time_ts = current_time;
-        parse_tuple(reply_t, &extra_metrics->translated_flow, zone_id, family, true);
+        parse_tuple(reply_t, &extra_metrics->translated_flow, zone_id, family,
+                    id->transport_protocol, true);
         return ret;
     }
 
@@ -95,7 +99,8 @@ static inline long translate_lookup_and_update_flow(flow_id *id, u16 flags,
         .end_mono_time_ts = current_time,
         .eth_protocol = eth_protocol,
     };
-    parse_tuple(reply_t, &new_extra_metrics.translated_flow, zone_id, family, true);
+    parse_tuple(reply_t, &new_extra_metrics.translated_flow, zone_id, family,
+                id->transport_protocol, true);
     ret = bpf_map_update_elem(&additional_flow_metrics, id, &new_extra_metrics, BPF_NOEXIST);
     if (ret != 0) {
         if (trace_messages && ret != -EEXIST) {
@@ -105,7 +110,8 @@ static inline long translate_lookup_and_update_flow(flow_id *id, u16 flags,
             additional_metrics *extra_metrics = bpf_map_lookup_elem(&additional_flow_metrics, id);
             if (extra_metrics != NULL) {
                 extra_metrics->end_mono_time_ts = current_time;
-                parse_tuple(reply_t, &extra_metrics->translated_flow, zone_id, family, true);
+                parse_tuple(reply_t, &extra_metrics->translated_flow, zone_id, family,
+                            id->transport_protocol, true);
                 return 0;
             }
         }

diff --git a/bpf/types.h b/bpf/types.h
@@ -133,7 +133,6 @@ typedef struct additional_metrics_t {
         u16 sport;
         u16 dport;
         u16 zone_id;
-        u8 icmp_id;
     } translated_flow;
     struct observed_intf_t {
         u8 direction;

diff --git a/bpf/utils.h b/bpf/utils.h
@@ -358,4 +358,14 @@ static inline void fill_in_others_protocol(flow_id *id, u8 protocol) {
     id->transport_protocol = protocol;
 }
 
+static inline bool is_transport_protocol(u8 protocol) {
+    switch (protocol) {
+    case IPPROTO_TCP:
+    case IPPROTO_UDP:
+    case IPPROTO_SCTP:
+        return true;
+    }
+    return false;
+}
+
 #endif // __UTILS_H__
diff --git a/pkg/decode/decode_protobuf.go b/pkg/decode/decode_protobuf.go
@@ -126,11 +126,14 @@ func RecordToMap(fr *model.Record) config.GenericMap {
 		if !model.AllZeroIP(model.IP(fr.Metrics.AdditionalMetrics.TranslatedFlow.Daddr)) &&
 			!model.AllZeroIP(model.IP(fr.Metrics.AdditionalMetrics.TranslatedFlow.Saddr)) {
 			out["ZoneId"] = fr.Metrics.AdditionalMetrics.TranslatedFlow.ZoneId
-			out["XlatSrcPort"] = fr.Metrics.AdditionalMetrics.TranslatedFlow.Sport
-			out["XlatDstPort"] = fr.Metrics.AdditionalMetrics.TranslatedFlow.Dport
+			if fr.Metrics.AdditionalMetrics.TranslatedFlow.Sport != 0 {
+				out["XlatSrcPort"] = fr.Metrics.AdditionalMetrics.TranslatedFlow.Sport
+			}
+			if fr.Metrics.AdditionalMetrics.TranslatedFlow.Dport != 0 {
+				out["XlatDstPort"] = fr.Metrics.AdditionalMetrics.TranslatedFlow.Dport
+			}
 			out["XlatSrcAddr"] = model.IP(fr.Metrics.AdditionalMetrics.TranslatedFlow.Saddr).String()
 			out["XlatDstAddr"] = model.IP(fr.Metrics.AdditionalMetrics.TranslatedFlow.Daddr).String()
-			out["XlatIcmpId"] = fr.Metrics.AdditionalMetrics.TranslatedFlow.IcmpId
 		}
 	}
 

diff --git a/pkg/decode/decode_protobuf_test.go b/pkg/decode/decode_protobuf_test.go
@@ -150,6 +150,5 @@ func TestPBFlowToMap(t *testing.T) {
 		"XlatSrcPort": uint16(1),
 		"XlatDstPort": uint16(2),
 		"ZoneId":      uint16(100),
-		"XlatIcmpId":  uint8(0),
 	}, out)
 }
diff --git a/pkg/ebpf/bpf_arm64_bpfel.go b/pkg/ebpf/bpf_arm64_bpfel.go
diff --git a/pkg/ebpf/bpf_arm64_bpfel.o b/pkg/ebpf/bpf_arm64_bpfel.o
diff --git a/pkg/ebpf/bpf_powerpc_bpfel.go b/pkg/ebpf/bpf_powerpc_bpfel.go
diff --git a/pkg/ebpf/bpf_powerpc_bpfel.o b/pkg/ebpf/bpf_powerpc_bpfel.o
diff --git a/pkg/ebpf/bpf_s390_bpfeb.go b/pkg/ebpf/bpf_s390_bpfeb.go
diff --git a/pkg/ebpf/bpf_s390_bpfeb.o b/pkg/ebpf/bpf_s390_bpfeb.o
diff --git a/pkg/ebpf/bpf_x86_bpfel.go b/pkg/ebpf/bpf_x86_bpfel.go
diff --git a/pkg/ebpf/bpf_x86_bpfel.o b/pkg/ebpf/bpf_x86_bpfel.o
diff --git a/pkg/model/record_test.go b/pkg/model/record_test.go
@@ -105,8 +105,7 @@ func TestAdditionalMetricsBinaryEncoding(t *testing.T) {
 		0x00, 0x00,
 		0x00, 0x00,
 		0x02, 0x00,
-		0x00,
-		0x00, // 1 byte padding
+		0x00, 0x00, // 2bytes padding
 		// observed_intf_t[4]
 		0x01, 0x00, 0x00, 0x00, 0x07, 0x00, 0x00, 0x00, // [0]: u8 direction + 3 bytes padding + u32 if_index
 		0x00, 0x00, 0x00, 0x00, 0x08, 0x00, 0x00, 0x00, // [1]: u8 direction + 3 bytes padding + u32 if_index
@@ -151,7 +150,6 @@ func TestAdditionalMetricsBinaryEncoding(t *testing.T) {
 			Sport:  0,
 			Dport:  0,
 			ZoneId: 2,
-			IcmpId: 0,
 		},
 		NbObservedIntf: 2,
 		ObservedIntf: [MaxObservedInterfaces]ebpf.BpfObservedIntfT{

diff --git a/pkg/pbflow/flow.pb.go b/pkg/pbflow/flow.pb.go
diff --git a/pkg/pbflow/proto.go b/pkg/pbflow/proto.go
@@ -86,7 +86,6 @@ func FlowToPB(fr *model.Record, s *ovnobserv.SampleDecoder) *Record {
 			SrcPort: uint32(fr.Metrics.AdditionalMetrics.TranslatedFlow.Sport),
 			DstPort: uint32(fr.Metrics.AdditionalMetrics.TranslatedFlow.Dport),
 			ZoneId:  uint32(fr.Metrics.AdditionalMetrics.TranslatedFlow.ZoneId),
-			IcmpId:  uint32(fr.Metrics.AdditionalMetrics.TranslatedFlow.IcmpId),
 		}
 	}
 	pbflowRecord.DupList = make([]*DupMapEntry, 0)
@@ -197,7 +196,6 @@ func PBToFlow(pb *Record) *model.Record {
 					Sport:  uint16(pb.Xlat.GetSrcPort()),
 					Dport:  uint16(pb.Xlat.GetDstPort()),
 					ZoneId: uint16(pb.Xlat.GetZoneId()),
-					IcmpId: uint8(pb.Xlat.GetIcmpId()),
 				},
 			},
 		},

diff --git a/proto/flow.proto b/proto/flow.proto
@@ -108,5 +108,4 @@ message Xlat {
   uint32 src_port = 3;
   uint32 dst_port = 4;
   uint32 zone_id = 5;
-  uint32 icmp_id = 7;
 }