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

Refactor | logging for src ip #74

Merged
merged 7 commits into from
Mar 8, 2023
Merged

Conversation

rushi47
Copy link
Contributor

@rushi47 rushi47 commented Feb 28, 2023

Fixes : #71

Replace custom function with formatter from aaya itself.

@rushi47 rushi47 changed the title using aya log helper Refactor | logging for src ip Feb 28, 2023
@rushi47 rushi47 marked this pull request as ready for review March 2, 2023 05:03
shaneutt
shaneutt previously approved these changes Mar 2, 2023
Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

Couple small comments to resolve before we merge, but overall looks good thanks @rushi47!

Wish I had known that you could use {:ipv4} some time back, that's terribly convenient 😂

dataplane/ebpf/src/egress/icmp.rs Outdated Show resolved Hide resolved
dataplane/ebpf/src/ingress/tcp.rs Outdated Show resolved Hide resolved
dataplane/ebpf/src/ingress/udp.rs Outdated Show resolved Hide resolved
@rushi47
Copy link
Contributor Author

rushi47 commented Mar 5, 2023

Integration tests are failing because :
Well interesting enough, same {:ipv4} formatter is behaving differently for icmp. Basically its loosing 2 values in last octect of ip address. Ex. instead of printing 172.0.2.100 its printing 172.0.2.1. I am trying to figure out whats the issue.

@shaneutt
Copy link
Member

shaneutt commented Mar 5, 2023

Integration tests are failing because : Well interesting enough, same {:ipv4} formatter is behaving differently for icmp. Basically its loosing 2 values in last octect of ip address. Ex. instead of printing 172.0.2.100 its printing 172.0.2.1. I am trying to figure out whats the issue.

Sounds good thanks for looking into it @rushi47. If it ends up looking like there's something wrong on the aya-log side, would definitely encourage you to submit an issue there (https://github.com/aya-rs/aya/issues/new). They also have a discord server which is active and helpful as well. Let us know if you get stuck, and how we can help you!

@rushi47
Copy link
Contributor Author

rushi47 commented Mar 5, 2023

Integration tests are failing because : Well interesting enough, same {:ipv4} formatter is behaving differently for icmp. Basically its loosing 2 values in last octect of ip address. Ex. instead of printing 172.0.2.100 its printing 172.0.2.1. I am trying to figure out whats the issue.

Sounds good thanks for looking into it @rushi47. If it ends up looking like there's something wrong on the aya-log side, would definitely encourage you to submit an issue there (https://github.com/aya-rs/aya/issues/new). They also have a discord server which is active and helpful as well. Let us know if you get stuck, and how we can help you!

Thank you @shaneutt, i was looking at code for aya log yesterday for formatter and tried to test it, with what they do internally it looked fine. But yes if this doesn't work I will raise issue.

@rushi47
Copy link
Contributor Author

rushi47 commented Mar 7, 2023

Well @shaneutt I reverted my commit and looked at main branch. The behavior is actually same for the main commit as well. And looks like that its not actually missing 00s but what its essentially doing i guess is returning the Gateway Ip of my network. So we can confirm that formatter is working fine. However,I need to check, how egress is called to make sure that port is unreachable in ingress udp path and debug from there.

shaneutt
shaneutt previously approved these changes Mar 7, 2023
Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

Ok thanks @rushi47

Seems reasonable to get this merged for now, appreciate the contribution!

@shaneutt
Copy link
Member

shaneutt commented Mar 7, 2023

One last thing, @rushi47 we are configured to required signed commits, if you could please rebase and sign your commits we can then merge. Here's a help doc if needed: https://docs.github.com/articles/about-gpg/

rushi47 and others added 3 commits March 7, 2023 23:41
Co-authored-by: Shane Utt <[email protected]>
Signed-off-by: rushi47 <[email protected]>
Co-authored-by: Shane Utt <[email protected]>
Signed-off-by: rushi47 <[email protected]>
Co-authored-by: Shane Utt <[email protected]>
Signed-off-by: rushi47 <[email protected]>
@rushi47
Copy link
Contributor Author

rushi47 commented Mar 8, 2023

One last thing, @rushi47 we are configured to required signed commits, if you could please rebase and sign your commits we can then merge. Here's a help doc if needed: https://docs.github.com/articles/about-gpg/

Done @shaneutt I rebased and pushed again. Looks like our co-authered commits are unable to be verified. When I however it shows verified for me. Please lmk if anything else is needed.

@rushi47 rushi47 requested a review from shaneutt March 8, 2023 04:47
@shaneutt shaneutt merged commit 822eed6 into kubernetes-sigs:main Mar 8, 2023
@rushi47 rushi47 deleted the issue_71 branch March 14, 2023 16:35
shaneutt added a commit to shaneutt/blixt that referenced this pull request Oct 11, 2023
Fixes : kubernetes-sigs#71 

Replace custom function with formatter from aaya itself.

---------

Signed-off-by: rushi47 <[email protected]>
Co-authored-by: Shane Utt <[email protected]>
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.

Cleanup ip address print helper in the dataplane
3 participants