hubble/parser/threefour: decode layers only if there is a packet

[tklauser]

When bumping github.com/google/gopacket to v1.1.19 (done as part of a
follow-up PR bumping all of Cilium's vendored dependencies),
gopacket.DecodingLayerParser.DecodeLayers will return an error such as
"Ethernet packet too small" in case the provided packet is empty. In
previous gopacket versions, a nil error was returned in this case. This
was reported upstream as google/gopacket#846.

This behavior will break tests that have no additional packet data e.g.
TestLocalObserverServer_GetFlows which uses raw monitor.TraceNotifyV0
messages.

This can e.g. be reproduced using:

$ go get github.com/google/gopacket@v1.1.19
$ go mod tidy && go mod vendor
$ go test -v -run "^TestLocalObserverServer_GetFlows$" ./pkg/hubble/observer
=== RUN   TestLocalObserverServer_GetFlows
    local_observer_test.go:131:
        	Error Trace:	local_observer_test.go:131
        	Error:      	Not equal:
    	            	expected: 0xa
    	            	actual  : 0x0
        	Test:       	TestLocalObserverServer_GetFlows
--- FAIL: TestLocalObserverServer_GetFlows (0.00s)
FAIL
exit status 1
FAIL	github.com/cilium/cilium/pkg/hubble/observer	0.066s

Avoid breaking these tests and also avoid an unnecessary call in case
there is no packet data. Once google/gopacket#846 is fixed upstream, the
check could be removed again when bumping the dependency.

[tklauser]

test-me-please

[gandro]

Thanks a lot!

[tklauser]

test-me-please

[tklauser]

retest-runtime

vm provisioning failed: https://jenkins.cilium.io/job/Cilium-PR-Runtime-4.9/3088

[tklauser]

retest-4.19

hit #12690 https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Kernel/4268/

[tklauser]

retest-gke

failed to scale cluster

[tklauser]

retest-gke

[glibsm]

We already check for data len on line 147, maybe we should combine somehow? Aside from breaking tests (are they even properly set up?) what does it mean to not have any layers in a packet?

[tklauser]

To me it looks like we want to error out at line 147 because the packet is too short to contain a monitor header (for lack of a better term), but the len(data) == 0 check applies to the data after the monitor header, i.e. there is some info in the monitor header which we potentially want to report. I agree that re-slicing data makes that logic harder to follow, so checking len(data[packetOffset:]) == 0 is probably better here.

[tklauser]

Re. breaking tests: To be honest, I don't know whether this can happen aside from tests, i.e. with real monitor traffic. I only noticed when bumping gopacket that said tests are failing. To me it looks like the tests could also be extended to contain some packet data. But then again, given that gopacket accepted empty packets just fine in previous versions (and will do so again if the proposed upstream fix is applied), I figured we rather follow that behavior.

[tklauser]

test-me-please

[kaworu]

LGTM! Could you please on the way improve the comment of Decode please:

--- a/pkg/hubble/parser/threefour/parser.go
+++ b/pkg/hubble/parser/threefour/parser.go
@@ -96,7 +96,7 @@ func New(
        }, nil
 }

-// Decode decodes the data from 'payload' into 'decoded'
+// Decode decodes the data from 'data' into 'decoded'
 func (p *Parser) Decode(data []byte, decoded *pb.Flow) error {
        if data == nil || len(data) == 0 {
                return errors.ErrEmptyData

[tklauser]

test-me-please

[tklauser]

retest-4.19

hit #12690 https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Kernel/4285/