Support capture at dest in PacketCapture

[hangyan]

Describe the problem/challenge you have

Currently PacketCapture can only capture packet in one location and it's usually the source pod. In some cases, it might be needed to capture on the dest location or both( to see the difference). If capture on both , we may results with 2 pcapng files as the result.

Describe the solution you'd like

add new flag in the PacketCapture spec. Note this may cause confusion with #6862 ( bi-direction) . but it sure fine and they can work together.

Anything else you would like to add?

[AryanBakliwal]

Hi @hangyan, is this and other issues related to PacketCapture (from 6860 till 6864) open or part of some other process like LFX mentorship?

[hangyan]

Hi @hangyan, is this and other issues related to PacketCapture (from 6860 till 6864) open or part of some other process like LFX mentorship?

they maybe good candidates for the LFX mentorship but also open. If you are interested, feel free to started working on one.

[luolanzone]

cc @antoninbas to confirm the candidate issues for LFX mentorship

[antoninbas]

@luolanzone @hangyan I was thinking that #6864 would be a good candidate for the LFX mentorship program next term. Let me know what you think.

@AryanBakliwal you're welcome to pick an issue to work on, but those PacketCapture issues may not necessary all be good first issues, and may involve a non trivial amount of work.

[hangyan]

@luolanzone @hangyan I was thinking that #6864 would be a good candidate for the LFX mentorship program next term. Let me know what you think.

@AryanBakliwal you're welcome to pick an issue to work on, but those PacketCapture issues may not necessary all be good first issues, and may involve a non trivial amount of work.

My concern is that tcp flags support is tricky, tcpdump supports various filter syntax like tcp[tcpflags] & (tcp-syn|tcp-ack) == tcp-syn|tcp-ack or 'tcp[13] & 4!=0'tcpdump 'tcp[tcpflags] == tcp-rst'. Either we support them all, means a non-trivial parser is needed, or we support a small subset, and document it well.

So I think maybe this issue is not a good choice

[antoninbas]

Either we support them all, means a non-trivial parser is needed, or we support a small subset, and document it well.

@hangyan I don't think we would need a parser as we would stick with simple CRD fields, and not "arbitrary" expressions. That may mean only supporting a subset of what tcpdump supports. A typical use cases may be "only capture packets with the SYN flag". Something like this may be a good tradeoff:

// requirements are ORed
TCPFlags []TCPFlagsMatcher

type TCPFlagsMatcher struct {
        Flags int32
        Mask int32
}

For example, to capture all TCP packets which are either SYN or RST packets, one could use:

[]TCPFlagsMatcher{
        {
                Flags: TCP_SYN,
                Mask: TCP_SYN,
        },
        {
                Flags: TCP_RST,
                Mask: TCP_RST,
        },
}

And, to capture only SYN+ACK packets (for some reason...):

[]TCPFlagsMatcher{
        {
                Flags: TCP_SYN | TCP_ACK,
                Mask: TCP_SYN | TCP_ACK,
        },
}

And, to capture all packets except the ones with the ACK bit set:

[]TCPFlagsMatcher{
        {
                Flags: 0,
                Mask: ^TCP_ACK,
        },
}

Of course, a simpler approach would be to have a single TCPFlags int32 field, and only capture packets which have the provided flags set (this is equivalent to tcp[tcpflags] & x == x, where x is the value of the TCPFlags field in the PacketCapture CR.

[hangyan]

Either we support them all, means a non-trivial parser is needed, or we support a small subset, and document it well.

@hangyan I don't think we would need a parser as we would stick with simple CRD fields, and not "arbitrary" expressions. That may mean only supporting a subset of what tcpdump supports. A typical use cases may be "only capture packets with the SYN flag". Something like this may be a good tradeoff:

// requirements are ORed
TCPFlags []TCPFlagsMatcher

type TCPFlagsMatcher struct {
        Flags int32
        Mask int32
}

For example, to capture all TCP packets which are either SYN or RST packets, one could use:

[]TCPFlagsMatcher{
        {
                Flags: TCP_SYN,
                Mask: TCP_SYN,
        },
        {
                Flags: TCP_RST,
                Mask: TCP_RST,
        },
}

And, to capture only SYN+ACK packets (for some reason...):

[]TCPFlagsMatcher{
        {
                Flags: TCP_SYN | TCP_ACK,
                Mask: TCP_SYN | TCP_ACK,
        },
}

And, to capture all packets except the ones with the ACK bit set:

[]TCPFlagsMatcher{
        {
                Flags: 0,
                Mask: ^TCP_ACK,
        },
}

Of course, a simpler approach would be to have a single TCPFlags int32 field, and only capture packets which have the provided flags set (this is equivalent to tcp[tcpflags] & x == x, where x is the value of the TCPFlags field in the PacketCapture CR.

Sure, i'm ok with this too. Should we also considering align with Traceflow? In that case, TCPFlags int32 is better.

[AryanBakliwal]

Hey @antoninbas, I have started working on this and thinking to add a new field capturePoint or captureDest which can be an enum and take either source or destination values. Only when both source and destination are defined in the spec, the user can specify the place (source or destination) at which to capture packets. When only one of them is defined (like what we want to do in #6976), the field will default to either the source or destination depending on whichever is present. Also, specifying the direction field then makes sense only if both source and destination are present in the spec.
Since we require that the packet capture controller supports capturing packets at both locations (source and destination) before we can implement packets capture at arbitrary source/destination, this issue needs to be addressed before #6976.
I’m waiting for changes in #6882 to be approved before opening a PR for this. Let me know if there’s anything else I should take into account.