Skip to content

Fix NatDumpCreated #27062

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Nov 13, 2023
Merged

Fix NatDumpCreated #27062

merged 3 commits into from
Nov 13, 2023

Conversation

gentoo-root
Copy link
Contributor

See the commit messages.

Fix the Created timestamps in `cilium bpf nat list` that used to display the same values.

@gentoo-root gentoo-root added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Jul 25, 2023
@gentoo-root gentoo-root requested review from a team as code owners July 25, 2023 19:04
@gentoo-root gentoo-root requested a review from aspsk July 25, 2023 19:04
@maintainer-s-little-helper
Copy link

Commit 0432a8367369cf618c090932b1a1214e16b8dda9 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jul 25, 2023
@gentoo-root gentoo-root force-pushed the fix-nat-dump-created-2 branch from f55a9cd to b65e278 Compare July 25, 2023 19:04
@maintainer-s-little-helper
Copy link

Commit 0432a8367369cf618c090932b1a1214e16b8dda9 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@gentoo-root gentoo-root force-pushed the fix-nat-dump-created-2 branch from b65e278 to f29c105 Compare July 26, 2023 11:22
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jul 26, 2023
@gentoo-root
Copy link
Contributor Author

/test

@julianwiedmann julianwiedmann added kind/bug This is a bug in the Cilium logic. feature/snat Relates to SNAT or Masquerading of traffic labels Aug 22, 2023
derailed
derailed requested changes Aug 22, 2023
View reviewed changes
Copy link
Contributor

@derailed derailed left a comment

Choose a reason for hiding this comment

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

Nice work! just a few picks...

aspsk
aspsk approved these changes Aug 23, 2023
View reviewed changes
@gentoo-root gentoo-root force-pushed the fix-nat-dump-created-2 branch from f29c105 to d51f3d3 Compare September 1, 2023 17:25
@github-actions
Copy link

github-actions bot commented Oct 2, 2023

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Oct 2, 2023
@gentoo-root gentoo-root force-pushed the fix-nat-dump-created-2 branch from d51f3d3 to 01ebed8 Compare October 2, 2023 17:24
@gentoo-root gentoo-root removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Oct 2, 2023
@gentoo-root gentoo-root requested a review from derailed October 2, 2023 17:25
@gentoo-root gentoo-root force-pushed the fix-nat-dump-created-2 branch from 01ebed8 to 1b19b51 Compare October 2, 2023 17:44
derailed
derailed reviewed Oct 6, 2023
View reviewed changes
derailed
derailed reviewed Oct 6, 2023
View reviewed changes
Copy link
Contributor

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@gentoo-root Thank you for the updates. Just a few minor comments

@gentoo-root gentoo-root force-pushed the fix-nat-dump-created-2 branch from 1b19b51 to 71a9ff3 Compare October 12, 2023 20:19
@gentoo-root gentoo-root requested a review from derailed October 18, 2023 14:06
derailed
derailed approved these changes Oct 26, 2023
View reviewed changes
Copy link
Contributor

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@gentoo-root Thank you for the updates Maxim!

@gentoo-root gentoo-root force-pushed the fix-nat-dump-created-2 branch 2 times, most recently from 35ae468 to 101c56e Compare October 30, 2023 16:17
@gentoo-root
Copy link
Contributor Author

/test

@gentoo-root gentoo-root force-pushed the fix-nat-dump-created-2 branch 2 times, most recently from 45f55cf to a32f253 Compare November 10, 2023 14:29
@gentoo-root
bpf, maps: Add the timestamp package
1f73807 
Timestamps in CT and NAT maps use different units depending on the
clocksource: seconds with ktime, and scaled jiffies with jiffies. Put
the common functions into a new package that will also be used in the
following commits.

Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
@gentoo-root
bpf, maps: Fix Created timestamp in NAT dump
d8a1391 
entryCreated in NatDumpCreated is measured in seconds or scaled jiffies,
depending on the clocksource. However, the function treats this value as
nanoseconds, returning wrong values. Fix it by doing the proper
calculation that considers the clocksource.

Fixes: e838397 ("bpf: switch NAT entry to more lightweight mono clock source")
Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
@gentoo-root
bpf, maps: Fallback to getting clocksource from config file
06730af 
If the agent is not running, get the clocksource from the config file,
similar to getIpv6EnableStatus().

Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
@gentoo-root gentoo-root force-pushed the fix-nat-dump-created-2 branch from a32f253 to 06730af Compare November 10, 2023 14:33
@gentoo-root
Copy link
Contributor Author

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 10, 2023
@squeed squeed merged commit 6144442 into cilium:main Nov 13, 2023
@julianwiedmann julianwiedmann added affects/v1.12 This issue affects v1.12 branch affects/v1.13 This issue affects v1.13 branch needs-backport/1.14 labels Nov 13, 2023
@gandro gandro mentioned this pull request Nov 15, 2023
Merged
6 tasks
@gandro gandro added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 labels Nov 15, 2023
@github-actions github-actions bot added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Nov 16, 2023
@nebril nebril mentioned this pull request Dec 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derailed derailed derailed approved these changes

@aspsk aspsk aspsk approved these changes

@youngnick youngnick youngnick approved these changes

Assignees
No one assigned
Labels
affects/v1.12 This issue affects v1.12 branch affects/v1.13 This issue affects v1.13 branch backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. feature/snat Relates to SNAT or Masquerading of traffic kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
cilium v1.14.5
Status: Released
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@gentoo-root @derailed @aspsk @youngnick @gandro @squeed @julianwiedmann