-
Notifications
You must be signed in to change notification settings - Fork 366
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add packetcatpure feature #6756
base: main
Are you sure you want to change the base?
Conversation
524ea56
to
9130c40
Compare
cc @tnqn @luolanzone Can you help review this again? I may need to add more unittest and fix the golangci( libpcap issue) but please provide suggestions on the current code. |
fed8994
to
0819cd8
Compare
@@ -29,6 +29,9 @@ RUN --mount=type=cache,target=/go/pkg/mod/ \ | |||
--mount=type=cache,target=/root/.cache/go-build/ \ | |||
make antctl-linux && mv bin/antctl-linux bin/antctl | |||
|
|||
|
|||
RUN apt-get update && apt-get install -y libpcap-dev && apt-get autoremove -y && rm -rf /var/cache/apt/* /var/lib/apt/lists/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be in the Dockerfile for the antrea/base-ubuntu: https://github.com/antrea-io/antrea/blob/main/build/images/base/Dockerfile#L53, maybe required in ubi Dockerfile as well: https://github.com/antrea-io/antrea/blob/main/build/images/base/Dockerfile.ubi. cc @tnqn @antoninbas to confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i'm not totally sure about introduce this libpcap and cgo thing because it required lots of changes in the build files. will move this part once it's confirmed.
470a9f0
to
2c71de0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't finished reviewing all changes. There are several compilation error in the controller's implementation, I suppose it's not finished yet.
if matchPacket.SourceIP != nil { | ||
exp += "src " + matchPacket.SourceIP.String() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The expression wouldn't be correct when SourceIP is nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added check to make sure it's not nil. the current CRD has a oneOf
(Pod, IP), so a valid ip address should be present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It means it doesn't support specifying destination only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can. i'm not sure how to express these fields requirements in crd schema. the following wont work
properties:
spec:
type: object
required:
- captureConfig # removed source/destination
anyOf:
- properties:
source:
required: [pod]
- properties:
destination:
required: [pod]
A CR with no source and destination is valid upon this.
2c71de0
to
bb18af7
Compare
caused by directly apply review suggestions (rename func) from github page. fixed. currently there are some issues on the arm platform image build. working on that. |
} | ||
if err := c.setPacketsFilePathStatus(pc.Name); err != nil { | ||
return err | ||
} | ||
} | ||
err = c.updatePacketCaptureStatus(pc, crdv1alpha1.PacketCaptureRunning, "", captureState.numCapturedPackets) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like setPacketsFilePathStatus will overwrite all status setting by updatePacketCaptureStatus if it's called later after updatePacketCaptureStatus?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setPacketsFilePathStatus
will only be called before the final status sync. As it's status is still Running
, not succeeded yet.
|
||
// initPacketCapture mark the PacketCapture as running | ||
func (c *Controller) initPacketCapture(pc *crdv1alpha1.PacketCapture) error { | ||
err := c.updatePacketCaptureStatus(pc, crdv1alpha1.PacketCaptureRunning, "", 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we mark it as Running directly?
I feel this should be a validatePacketCapture() instead of initPacketCapture() to set it as running directly.
validatePacketCapture can set the status as failure and update the validation failure in the reason.
The failure during package capture should be updated during the PacketCaptureRunning status.
@tnqn any suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The suggestion makes sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the validation
has been handled by the CRD schema. Maybe the getPodIP
part can be checked first?
1c9e3f6
to
d14a1a3
Compare
Signed-off-by: Hang Yan <[email protected]>
Signed-off-by: Hang Yan <[email protected]>
d14a1a3
to
2768afa
Compare
Signed-off-by: Hang Yan <[email protected]>
This is a refactor of #5821 (ovs based) which use
gopacket(https://github.com/google/gopacket)https://github.com/packetcap/go-pcap to perform the capture.Notes: