Skip to content
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

Implement zilla dump command similar to tcpdump #121

Merged
merged 24 commits into from
Nov 17, 2022

Conversation

bmaidics
Copy link
Contributor

@bmaidics bmaidics commented Oct 24, 2022

Fix #114

@bmaidics bmaidics marked this pull request as ready for review October 27, 2022 20:28
@bmaidics
Copy link
Contributor Author

What's still needed here:

  • Turned out Pcap4j is not working with java 17. I was using java 11 during the development. Another drawback is that it is a wrapper around a native lib. Based on these we should drop the lib, and come up with something
  • Couldn't get rid of some Wireshark warnings: previous segment not arrived/dup ack etc.. We need to figure out a way to smartly assign sequence number to packet and the segment types as well.

@jfallows jfallows changed the title Issue-114: Implement zilla dump command similar to tcpdump Implement zilla dump command similar to tcpdump Oct 31, 2022
@jfallows jfallows self-requested a review October 31, 2022 18:31
Copy link
Contributor

@jfallows jfallows left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job making so much progress on this task, @bmaidics. 👍

In general though, the dump command design needs to be more closely aligned with other zilla commands using the ZillaCommandSpi, like ZillaTuneCommand and not so heavily influenced by the command-log internals, which does not implement the same pattern.

Your intention behind the reuse is good, but it seems to be interfering a bit with getting the most natural solution for zilla dump.

I suggest you complete the implementation for zilla dump without any reuse, copying whatever classes you need from command-log such as the layouts, and cleaning up the rest of the implementation to simplify as if it would stand alone and reduce the coupling.

Let's treat this as the next checkpoint.

Then we can re-introduce the dependency from command-log to command-dump to reuse the layout implementations for example, leaving the other internals unmodified.

Later, we might be picking up the layout implementations from the zilla engine, so this approach will evolve naturally to remove them altogether from the individual command projects over time.

Copy link
Contributor

@jfallows jfallows left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I built this PR locally, and modified zilla-examples tcp.echo example stack.yml to use develop-SNAPSHOT instead of latest image, then followed the README to launch the stack.

I then executed the following to get a shell inside the zilla docker service:

docker exec -it $(docker ps -q -f name=example_zilla) sh

When I then run ./zilla dump -v or ./zilla dump -v -o zilla.pcap the executable exits without producing any output nor error which is unexpected.

Copy link
Contributor

@jfallows jfallows left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor change then we should be able to merge!

@jfallows jfallows merged commit 13cc4ba into aklivity:develop Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement zilla dump command similar to tcpdump
2 participants