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

Add IPv6 support to snapshot socket #1812

Merged
merged 3 commits into from
Sep 7, 2023
Merged

Conversation

eiffel-fl
Copy link
Member

Hi.

This PR adds IPv6 support to snapshot socket:

$ python
>>> import socket
>>> s = socket.socket(socket.AF_INET6, socket.SOCK_STREAM)
>>> s.bind(('::1', 8082, 0, 0))
>>> s.listen(5)
zsh: suspended  python3
$ sudo ig snapshot socket --host
CONTAINER                                                                PROTOCOL LOCAL                                  REMOTE                                STATUS
...
                                                                         TCP      ::1:8082                               :::0                                  LISTEN 
...

Best regards.

@alban
Copy link
Member

alban commented Jul 3, 2023

While you're working on this, it would be nice to print the v6only bool from IPv6 sockets.

sk->__sk_common.skc_ipv6only

See #1776 (comment)

(could be done in a different PR)

@eiffel-fl eiffel-fl force-pushed the francis/snapshot-socket-ipv6 branch 6 times, most recently from 1c8935d to 8187aec Compare July 5, 2023 09:46
@eiffel-fl eiffel-fl linked an issue Jul 5, 2023 that may be closed by this pull request
@mauriciovasquezbernal
Copy link
Member

I'll suggest to rebase this on top of #1895, the result code will be much simpler IMO.

@eiffel-fl
Copy link
Member Author

I'll suggest to rebase this on top of #1895, the result code will be much simpler IMO.

It is really funny because I would have suggest you to rebase on top of mine.

@mauriciovasquezbernal
Copy link
Member

I don't have any issue. Let's get this in and then I'll rebase mine on top.

@mauriciovasquezbernal
Copy link
Member

I also can implement ipv6 support on top of #1895, so we don't waste time reviewing this.

@blanquicet
Copy link
Member

From my point of view, it makes much more sense to rebased this on top of the refactoring introduced in #1895, which IMO, is ready to go.

@eiffel-fl
Copy link
Member Author

From my point of view, it makes much more sense to rebased this on top of the refactoring introduced in #1895, which IMO, is ready to go.

If I rebase this PR on top of the refactoring one, it will just delay its merge.

@eiffel-fl eiffel-fl force-pushed the francis/snapshot-socket-ipv6 branch 5 times, most recently from a7e7f8b to a1c5f61 Compare August 10, 2023 09:42
@eiffel-fl eiffel-fl force-pushed the francis/snapshot-socket-ipv6 branch 2 times, most recently from 62a7378 to 61fb17c Compare September 6, 2023 15:33
@eiffel-fl
Copy link
Member Author

I rebased everything and the unit test confirms everything is OK, so it is time to merge this PR.

Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

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

Looks good, some minor comments.

Remove

func parseIPv4(ipU32 uint32) string {

pkg/gadgets/snapshot/socket/tracer/bpf/socket.bpf.c Outdated Show resolved Hide resolved
Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for handling all the comments.

@eiffel-fl
Copy link
Member Author

Thank you for the review!

@eiffel-fl eiffel-fl merged commit 4104cba into main Sep 7, 2023
47 checks passed
@eiffel-fl eiffel-fl deleted the francis/snapshot-socket-ipv6 branch September 7, 2023 14:04
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.

snapshot/socket: Support IPv6
4 participants