Add support for parsing couchbase Flexiable framing

[NimrodAvni78]

Checklist

Couchbase packets have an alternative header structure for flexible frames

We were incorrectly parsing these packets and creating a situation where the size of the key looks to big to be included in the packet, hence why we didn't classify the header as valid

• If this enhances / fixes / changes a core feature, I have updated the features documentation

[codecov]

Codecov Report

❌ Patch coverage is 83.94161% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 43.44%. Comparing base (2b13ee5) to head (5f10937).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/internal/ebpf/couchbasekv/header.go	85.98%	14 Missing and 1 partial ⚠️
pkg/ebpf/common/couchbase_detect_transform.go	76.66%	6 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1265      +/-   ##
==========================================
- Coverage   43.51%   43.44%   -0.08%     
==========================================
  Files         305      304       -1     
  Lines       32866    32780      -86     
==========================================
- Hits        14303    14241      -62     
+ Misses      17642    17621      -21     
+ Partials      921      918       -3     

Flag	Coverage Δ
integration-test	21.57% <16.52%> (+0.12%)	⬆️
integration-test-arm	0.00% <0.00%> (ø)
integration-test-vm-x86_64-5.15.152	?
integration-test-vm-x86_64-6.10.6	?
k8s-integration-test	2.37% <0.00%> (+0.01%)	⬆️
oats-test	0.00% <0.00%> (ø)
unittests	44.19% <95.04%> (-0.11%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mmat11]

do we need some safety checks here?

[grcevski]

+1 maybe also good idea to add some checks on remaining not going < 0?

[NimrodAvni78]

will do!

[NimrodAvni78]

Correct me if i am wrong but i think it already checks that remaining can't be under 0
it first sets what we try to read (framingExtrasLen / KeyLen, ...) to be no bigger then remaining, then we check if it is larger then 0 then we read, and continue to the next, then if remaining is smaller then 0 we will stop reading the next variables no?

[mmat11]

I think this is unused

[rafaelroquetto]

I have a few concerns - in particular, parsing the binary data with the reader allocates the header every single time (and then copies it around).

Could we instead:

• have ParseHeader to be non-allocating (i.e. return a view of the original data)
• the same for ParsePacket
• if we must store the packets, we can then do a deep-copy on the spot, at least we are cognisant about it
• as a follow up, turn ParsePackets into an iterator-like API (this would do without allocating the slice return value of ParsePackets)

All of this would mean the couchbase parser would be operating directly into the original eBPF buffer memory - no copies, no allocs just views.

The only allocations/copies happening would be the materialisation of CouchBaseInfo - which is fair enough.

[rafaelroquetto]

Suggested change

	FramingExtrasLen uint8 // Only used for flexible framing (magic 0x08/0x18)
	KeyLen uint16 // For flexible framing, this is only 8 bits but stored as uint16
	KeyLen uint16 // For flexible framing, this is only 8 bits but stored as uint16
	FramingExtrasLen uint8 // Only used for flexible framing (magic 0x08/0x18)

rationale: https://github.com/couchbase/libcouchbase/blob/a129df617431cac279413fcdb2ec808c3f9c890e/include/memcached/protocol_binary.h#L428

[rafaelroquetto]

wouldn't it make sense to not reuse this, instead declaring a new struct for the response?

The way this is laid out now, it prevents us from reinterpreting binary data into this type (also For requests: VBucket ID; For responses: Status code is detrimental for code readability - the comment is buried in the definition)

[NimrodAvni78]

yeah you are right, just seemed like code duplication only for 1 field
but that said it does remove ambiguity so i agree its a good idea

[NimrodAvni78]

I have a few concerns - in particular, parsing the binary data with the reader allocates the header every single time (and then copies it around).

@rafaelroquetto just to make sure i understand correctly, i should make header and packet just a view of the original buffer, and not a stuct containing those fields, and have helper functions to access data for parsing, like:

type Header []byte

func (h Header) Magic() Magic { return Magic(h[0]) }

func (h Header) KeyLen() uint16 {
	if h.Magic().IsAltFormat() {
		return uint16(h[3])
	}
	return binary.BigEndian.Uint16(h[2:4])
}
...

?

[rafaelroquetto]

@NimrodAvni78 exactly - that would be awesome. And if for whatever reason you need to "clone" the packet, you can implement a Clone() method that simply copies the underlying buffer into a new one, and switches the internal pointer to this new copy.

So we end up with an object that is "implicitly shared" (i.e. every copy references the same internal buffer) unless you call "Clone()" (which detaches/returns a copy with a new buffer). This works well for objects whose "getters/methods" are "const" (i.e. they never modify the object state, instead just return some info). Then the object can be initialised from whatever buffer, including the original ebpf buffer (so in this case no copies).

[grcevski]

LGTM!

[rafaelroquetto]

Gnarly!