hubble: let ServiceGetter.GetServiceByAddr return service by pointer

This avoids copying the Service protobuf object and improves performance and memory usage: name old time/op new time/op delta L34Decode-8 4.10µs ± 5% 3.63µs ± 8% -11.51% (p=0.000 n=10+10) name old alloc/op new alloc/op delta L34Decode-8 1.24kB ± 0% 1.03kB ± 0% -16.77% (p=0.000 n=10+10) name old allocs/op new allocs/op delta L34Decode-8 26.0 ± 0% 21.0 ± 0% -19.23% (p=0.000 n=10+10) name old time/op new time/op delta L7Decode-8 6.37µs ±11% 5.79µs ± 4% -9.02% (p=0.000 n=10+10) name old alloc/op new alloc/op delta L7Decode-8 1.53kB ± 0% 1.37kB ± 0% -10.47% (p=0.000 n=10+10) name old allocs/op new allocs/op delta L7Decode-8 36.0 ± 0% 34.0 ± 0% -5.56% (p=0.000 n=10+10) Also, the ok bool return is no longer necessary as callers can just check for a nil return value. This simplifies the logic in (*Parser).Decode a bit and avoids allocating an empty object for cases when no service is found. Signed-off-by: Tobias Klauser <tobias@cilium.io>
cilium · Sep 28, 2021 · 44e5ce3 · 44e5ce3
1 parent 22ca6de
commit 44e5ce3
Show file tree

Hide file tree

Showing 7 changed files with 23 additions and 31 deletions.
diff --git a/daemon/cmd/hubble.go b/daemon/cmd/hubble.go
@@ -326,7 +326,7 @@ func (d *Daemon) GetNamesOf(sourceEpID uint32, ip net.IP) []string {
 // with service information.
 //
 //  - ServiceGetter: https://github.com/cilium/hubble/blob/04ab72591faca62a305ce0715108876167182e04/pkg/parser/getters/getters.go#L52
-func (d *Daemon) GetServiceByAddr(ip net.IP, port uint16) (flowpb.Service, bool) {
+func (d *Daemon) GetServiceByAddr(ip net.IP, port uint16) *flowpb.Service {
 	addr := loadbalancer.L3n4Addr{
 		IP: ip,
 		L4Addr: loadbalancer.L4Addr{
@@ -335,12 +335,12 @@ func (d *Daemon) GetServiceByAddr(ip net.IP, port uint16) (flowpb.Service, bool)
 	}
 	namespace, name, ok := d.svc.GetServiceNameByAddr(addr)
 	if !ok {
-		return flowpb.Service{}, false
+		return nil
 	}
-	return flowpb.Service{
+	return &flowpb.Service{
 		Namespace: namespace,
 		Name:      name,
-	}, true
+	}
 }
 
 // GetK8sMetadata returns the Kubernetes metadata for the given IP address.

diff --git a/pkg/hubble/parser/getters/getters.go b/pkg/hubble/parser/getters/getters.go
@@ -53,7 +53,7 @@ type IPGetter interface {
 
 // ServiceGetter fetches service metadata.
 type ServiceGetter interface {
-	GetServiceByAddr(ip net.IP, port uint16) (service flowpb.Service, ok bool)
+	GetServiceByAddr(ip net.IP, port uint16) *flowpb.Service
 }
 
 // StoreGetter ...

diff --git a/pkg/hubble/parser/seven/parser.go b/pkg/hubble/parser/seven/parser.go
@@ -110,12 +110,8 @@ func (p *Parser) Decode(r *accesslog.LogRecord, decoded *pb.Flow) error {
 	l4, sourcePort, destinationPort := decodeLayer4(r.TransportProtocol, sourceEndpoint, destinationEndpoint)
 	var sourceService, destinationService *pb.Service
 	if p.serviceGetter != nil {
-		if srcService, ok := p.serviceGetter.GetServiceByAddr(sourceIP, sourcePort); ok {
-			sourceService = &srcService
-		}
-		if dstService, ok := p.serviceGetter.GetServiceByAddr(destinationIP, destinationPort); ok {
-			destinationService = &dstService
-		}
+		sourceService = p.serviceGetter.GetServiceByAddr(sourceIP, sourcePort)
+		destinationService = p.serviceGetter.GetServiceByAddr(destinationIP, destinationPort)
 	}
 
 	decoded.Time = pbTimestamp

diff --git a/pkg/hubble/parser/seven/parser_test.go b/pkg/hubble/parser/seven/parser_test.go
@@ -108,14 +108,14 @@ func TestDecodeL7HTTPRecord(t *testing.T) {
 		},
 	}
 	serviceGetter := &testutils.FakeServiceGetter{
-		OnGetServiceByAddr: func(ip net.IP, port uint16) (service pb.Service, ok bool) {
+		OnGetServiceByAddr: func(ip net.IP, port uint16) *pb.Service {
 			if ip.Equal(net.ParseIP(fakeDestinationEndpoint.IPv4)) && (port == fakeDestinationEndpoint.Port) {
-				return pb.Service{
+				return &pb.Service{
 					Name:      "service-1234",
 					Namespace: "default",
-				}, true
+				}
 			}
-			return
+			return nil
 		},
 	}
 

diff --git a/pkg/hubble/parser/threefour/parser.go b/pkg/hubble/parser/threefour/parser.go
@@ -180,12 +180,8 @@ func (p *Parser) Decode(data []byte, decoded *pb.Flow) error {
 	dstEndpoint := p.resolveEndpoint(dstIP, dstLabelID)
 	var sourceService, destinationService *pb.Service
 	if p.serviceGetter != nil {
-		if srcService, ok := p.serviceGetter.GetServiceByAddr(srcIP, srcPort); ok {
-			sourceService = &srcService
-		}
-		if dstService, ok := p.serviceGetter.GetServiceByAddr(dstIP, dstPort); ok {
-			destinationService = &dstService
-		}
+		sourceService = p.serviceGetter.GetServiceByAddr(srcIP, srcPort)
+		destinationService = p.serviceGetter.GetServiceByAddr(dstIP, dstPort)
 	}
 
 	decoded.Verdict = decodeVerdict(dn, tn, pvn)

diff --git a/pkg/hubble/parser/threefour/parser_test.go b/pkg/hubble/parser/threefour/parser_test.go
@@ -132,20 +132,20 @@ func TestL34Decode(t *testing.T) {
 		},
 	}
 	serviceGetter := &testutils.FakeServiceGetter{
-		OnGetServiceByAddr: func(ip net.IP, port uint16) (service flowpb.Service, ok bool) {
+		OnGetServiceByAddr: func(ip net.IP, port uint16) *flowpb.Service {
 			if ip.Equal(net.ParseIP("192.168.33.11")) && (port == 6443) {
-				return flowpb.Service{
+				return &flowpb.Service{
 					Name:      "service-1234",
 					Namespace: "remote",
-				}, true
+				}
 			}
 			if ip.Equal(net.ParseIP("10.16.236.178")) && (port == 54222) {
-				return flowpb.Service{
+				return &flowpb.Service{
 					Name:      "service-4321",
 					Namespace: "default",
-				}, true
+				}
 			}
-			return
+			return nil
 		},
 	}
 	identityCache := &testutils.NoopIdentityGetter

diff --git a/pkg/hubble/testutils/fake.go b/pkg/hubble/testutils/fake.go
@@ -360,11 +360,11 @@ var NoopIPGetter = FakeIPGetter{
 
 // FakeServiceGetter is used for unit tests that need ServiceGetter.
 type FakeServiceGetter struct {
-	OnGetServiceByAddr func(ip net.IP, port uint16) (service flowpb.Service, ok bool)
+	OnGetServiceByAddr func(ip net.IP, port uint16) *flowpb.Service
 }
 
 // GetServiceByAddr implements FakeServiceGetter.GetServiceByAddr.
-func (f *FakeServiceGetter) GetServiceByAddr(ip net.IP, port uint16) (service flowpb.Service, ok bool) {
+func (f *FakeServiceGetter) GetServiceByAddr(ip net.IP, port uint16) *flowpb.Service {
 	if f.OnGetServiceByAddr != nil {
 		return f.OnGetServiceByAddr(ip, port)
 	}
@@ -373,8 +373,8 @@ func (f *FakeServiceGetter) GetServiceByAddr(ip net.IP, port uint16) (service fl
 
 // NoopServiceGetter always returns an empty response.
 var NoopServiceGetter = FakeServiceGetter{
-	OnGetServiceByAddr: func(ip net.IP, port uint16) (service flowpb.Service, ok bool) {
-		return flowpb.Service{}, false
+	OnGetServiceByAddr: func(ip net.IP, port uint16) *flowpb.Service {
+		return nil
 	},
 }