Skip to content

docs: XFRM reference guide for IPsec development #35322

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 2 commits into from
Oct 25, 2024
Merged

docs: XFRM reference guide for IPsec development #35322

merged 2 commits into from
Oct 25, 2024

Conversation

pchaigno
Copy link
Member

@pchaigno pchaigno commented Oct 9, 2024

No description provided.

@pchaigno pchaigno added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact. feature/ipsec Relates to Cilium's IPsec feature labels Oct 9, 2024
@pchaigno pchaigno force-pushed the xfrm-reference-guide branch 2 times, most recently from 50321d2 to 274d9b7 Compare October 9, 2024 17:42
@pchaigno pchaigno force-pushed the xfrm-reference-guide branch from 274d9b7 to c63564f Compare October 9, 2024 17:43
@pchaigno pchaigno force-pushed the xfrm-reference-guide branch 3 times, most recently from fa0f5cb to 3c1639e Compare October 10, 2024 09:35
@pchaigno pchaigno requested review from ldelossa and rgo3 October 14, 2024 12:57
@pchaigno pchaigno marked this pull request as ready for review October 14, 2024 12:58
@pchaigno pchaigno requested review from a team as code owners October 14, 2024 12:58
@pchaigno pchaigno requested review from a user, tklauser and smagnani96 and removed request for rgo3 October 14, 2024 12:58
ghost
ghost approved these changes Oct 14, 2024
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

nice, lgtm

@pchaigno pchaigno force-pushed the xfrm-reference-guide branch 2 times, most recently from 0ca48f0 to 36ced40 Compare October 16, 2024 08:40
jschwinger233
jschwinger233 approved these changes Oct 21, 2024
View reviewed changes
Copy link
Member

@jschwinger233 jschwinger233 left a comment

Choose a reason for hiding this comment

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

Paul, how do you like this figure from https://thermalcircle.de/doku.php?id=blog:linux:nftables_ipsec_packet_flow
image
I found it more accurate than https://en.m.wikipedia.org/wiki/File:Netfilter-packet-flow.svg in terms of xfrm. For example the figure above suggests ipsec decryption doesn't necessarily use xfrm policy, I think I verified this by pwru --filter-non-skb-funcs xfrm_state_look_at,xfrm_state_lookup,xfrm_state_lookup_byaddr,xfrm_state_lookup_byspi.

There are some other figures in the article which also help.

ldelossa
ldelossa approved these changes Oct 21, 2024
View reviewed changes
Copy link
Contributor

@ldelossa ldelossa left a comment

Choose a reason for hiding this comment

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

Very nice! Especially the annotated xfrm output.

All comments I have left are nits where I thought just a tiny bit more explanation could help. I don't feel strongly on these.

smagnani96
smagnani96 approved these changes Oct 22, 2024
View reviewed changes
Copy link
Contributor

@smagnani96 smagnani96 left a comment

Choose a reason for hiding this comment

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

Amazing work, thanks Paul!
Few probably unwanted spacing issues (confirmed while locally rendering the doc with make render-docs), but the content is very clear to me 👍

@maintainer-s-little-helper maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Oct 22, 2024
@pchaigno pchaigno force-pushed the xfrm-reference-guide branch 4 times, most recently from 32ee44e to 80d00ae Compare October 23, 2024 14:28
@derailed
Copy link
Contributor

/test

@pchaigno pchaigno added dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. and removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Oct 24, 2024
@pchaigno pchaigno force-pushed the xfrm-reference-guide branch from 80d00ae to 89a142a Compare October 24, 2024 21:37
@pchaigno
docs: Move BPF reference guide to subdir
4ce4feb 
The subsequent commit will introduce a sibling reference guide in the
same directory.

Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
@pchaigno
docs: XFRM reference guide
a8882b6 
Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
@pchaigno pchaigno force-pushed the xfrm-reference-guide branch from 89a142a to a8882b6 Compare October 24, 2024 21:38
@pchaigno pchaigno removed the dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. label Oct 24, 2024
@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 Oct 24, 2024
@pchaigno
Copy link
Member Author

Thanks everyone for the reviews! That was really helpful! I believe I addressed all comments. I also reworked the flow diagram (sources uploaded) by taking inspiration from the one Gray pointed to and made various minor edits. I'll wait until tomorrow evening before merging this, in case someone spots something else.

@pchaigno pchaigno added this pull request to the merge queue Oct 25, 2024
@pchaigno pchaigno added the needs-backport/1.16 This PR / issue needs backporting to the v1.16 branch label Oct 25, 2024
Merged via the queue into cilium:main with commit 30de2e2 Oct 25, 2024
63 checks passed
@pchaigno pchaigno deleted the xfrm-reference-guide branch October 25, 2024 16:15
@rastislavs rastislavs mentioned this pull request Oct 28, 2024
Merged
4 tasks
@rastislavs rastislavs added backport-pending/1.16 The backport for Cilium 1.16.x for this PR is in progress. and removed needs-backport/1.16 This PR / issue needs backporting to the v1.16 branch labels Oct 28, 2024
@github-actions github-actions bot added backport-done/1.16 The backport for Cilium 1.16.x for this PR is done. and removed backport-pending/1.16 The backport for Cilium 1.16.x for this PR is in progress. labels Oct 28, 2024
@cilium-release-bot cilium-release-bot bot mentioned this pull request Nov 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tklauser tklauser tklauser approved these changes

@ldelossa ldelossa ldelossa approved these changes

@jschwinger233 jschwinger233 jschwinger233 approved these changes

@smagnani96 smagnani96 smagnani96 approved these changes

+1 more reviewer
Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. backport-done/1.16 The backport for Cilium 1.16.x for this PR is done. feature/ipsec Relates to Cilium's IPsec feature ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@pchaigno @derailed @tklauser @ldelossa @jschwinger233 @smagnani96 @rastislavs