Add packetcapture api

[hangyan]

Add packetcapture's API changes.

For the full feature, please ref to:

#5821

[hangyan]

moved API to this MR. Thanks you previous feedback and welcome to help review this again.

@luolanzone @tnqn @jianjuns @antoninbas

Accoding to #5821 (comment) . IPHeader field was removed.

[antoninbas]

it looks like we are missing the srcIP / dstIP properties here?

[hangyan]

it's actually not missed, since we have a Source and Destination field in spec, so the srcIP/dstIP is not used. The golang structure still keep these fields in the PR, but i think we can removed them. The whole Packet structure has changed a lot during the review compared to the original one(==Traceflow's Packet strucuture).

[hangyan]

Updated this part. Currently the package related structure looks like this:

// Packet includes header info.
type Packet struct {
	IPv6Header      *IPv6Header     `json:"ipv6Header,omitempty"`
	TransportHeader TransportHeader `json:"transportHeader"`
}

Note: remove IPv4Header as it's unused and in another thread, we are discussing if a IPFamily field is needed.

Also in the TransportHeader, we have tcp/icmp/udp strcuture to allow user filter based on transport attributes. For icmp, we don't have any filter yet, so the strucutre is mainly used as a type indicator.

type ICMPEchoRequestHeader struct {
}

[hangyan]

cc @jianjuns @tnqn @luolanzone

Can you help review this MR again? Thank you

I will be actively working on this recently.

[antoninbas]

I feel like we should have the option to write the pcap to a local file as well, just for ease of use / testing
Not everyone will want to have an ftp server for this

[hangyan]

So do you think keep the files in container is enough? Since this is not a cli, it would be odd to export the files to a location on the host.

If keep it in the container, filepath + container name should be ok?

[antoninbas]

If keep it in the container, filepath + container name should be ok

Just filepath I think, the container will always be antrea-agent? If users want the file to be on the Node, they can provide a path that is mounted from the host, such as /var/log/antrea.

But there would need to be a way for users to figure out which antrea-agent Pod has the capture file, so that would need to be included in the Status. Are packets captured at the source or the destination or both?

[hangyan]

Both could happen, in the source or destination. pod name can be exposed in the status. Just need to think about the value format for the filepath field.

[antoninbas]

I think my original point still applies. Always requiring users to provide an SFTP server in order to do a packet capture is not ideal. That being said, if I am the only one who feels strongly about this, we can always address it in the future if needed.

[hangyan]

Agreed here. I have the following plans:

1. make FileServer field optional.
2. Change the status.packetsFileName to status.packetsFilePath and it's value will be like antrea-agent-xxxx:/tmp/xxx.pcap.

The impl should be trival.

Does this sounds good?

[hangyan]

see: 939fe06

[jianjuns]

Is it possible to share some types/constants with Traceflow?

[hangyan]

also see discussion #6257 (comment)

for the constants part i think it's unnecessary, each CRD may have more different status in the future.

About types, since Traceflow is in vebeta1 now, are you suggesting we define some common types in neutral place?

[jianjuns]

I am not sure if we should define common types across versions. Let us see if @tnqn and @antoninbas have an opinion.

[antoninbas]

I guess in theory we could have common definitions in a neutral directory, it wouldn't need to be under apis/crd/<version>. But if it's not a versioned directory, then we would have to be confident that they are not going to change.

K8s has meta/v1 with comm types, such as Time. I don't know how good of an analogy it is.

[tnqn]

Except those meta types in meta/v1. K8s doesn't define common types across versions. If a constant needs to be used by a version, it will just be replicated in that version. I think it's to keep every version's stability and compatability. Otherwise a change in one version would have an impact on other versions.

[hangyan]

@jianjuns Do you think we can add a ip family and ip protocol field? so we can remove ipv6header and icmpheader.

[jianjuns]

@jianjuns Do you think we can add a ip family and ip protocol field? so we can remove ipv6header and icmpheader.

That works for me, if you do not see a potential requirement to support other IP header fields, esp. v4 or v6 specific fields.
And I meant to use intstr.IntOrString for protocol.

[hangyan]

@jianjuns Do you think we can add a ip family and ip protocol field? so we can remove ipv6header and icmpheader.

That works for me, if you do not see a potential requirement to support other IP header fields, esp. v4 or v6 specific fields. And I meant to use intstr.IntOrString for protocol.

updated.

[jianjuns]

I am not sure if we should define common types across versions. Let us see if @tnqn and @antoninbas have an opinion.

[tnqn]

Except those meta types in meta/v1. K8s doesn't define common types across versions. If a constant needs to be used by a version, it will just be replicated in that version. I think it's to keep every version's stability and compatability. Otherwise a change in one version would have an impact on other versions.

[antrea-bot]

Can one of the admins verify this patch?

[antrea-bot]

Can one of the admins verify this patch?

[hangyan]

@tnqn

I get the points of XXXReference and pointer, but is it common to have the struct name in the names of its members? It seems useful when the structs are inline properties in their parents but seems redundant otherwise.

No you're right, I am not sure why I added the Source / Destination prefix in the field names.

updated. yeah this seems neater.

[antoninbas]

I think my original point still applies. Always requiring users to provide an SFTP server in order to do a packet capture is not ideal. That being said, if I am the only one who feels strongly about this, we can always address it in the future if needed.

[luolanzone]

What do you mean it's "either in the source or the destination", when the user defined both source and destination in the spec, how do we tell which Node the packets are captured from?

[tnqn]

The spec doesn't provide enough information to understand how to use the API. I have the questions when looking at the fields:

1. What's the behavior if Timeout is nil, capturing forever or there is a default value?
2. Could I set Source only or Destination only, or they must be set together? If not the latter, why they are not optional like other fields?
3. I suppose it means capturing all traffic matching Source and Destination when Packet is nil, but please make this explicit in the comment.
4. If FileServer is present, captured packets will be uploaded to this server, then what if FileServer is not present? This seems a valid input given it's a pointer.

[hangyan]

sure, will add more comments.

[hangyan]

updated

[tnqn]

Phase is a deprecated pattern of K8s API, it recommends to use conditions which is more flexible to express state of each condition. For example, there could be a condition for capture and another condition for upload. See https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties

I don't have strong opinion if the phase and a single reason can cover all possible results.

[antoninbas]

This was not addressed, so I assume we are sticking to Phase / Reason?

Note that this is hard to change in future API versions, so I personally think that we should go with the recommended approach right away.

[antoninbas]

LGTM, it would be good to get a second approval

[antoninbas]

As I pointed out in the other PR, it seems that we don't have an equivalent to BundleServerAuthConfiguration for PacketCapture (to configure sftp authentication). But that may have been intentional?

[hangyan]

yes. a dedicated secret is serving the purpose. It's hard to find the origin discussion in the previous PR..

[hangyan]

@luolanzone All updated. Can you take a look again?

I also removed the ipv6 related support in CRD schema. It should be easy to add it back in the following releases.

[tnqn]

LGTM

[luolanzone]

LGTM