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.

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

cilium: v1.11 backport for ipsec key rotation #27552

Merged
merged 2 commits into from
Aug 21, 2023
Merged

cilium: v1.11 backport for ipsec key rotation #27552

merged 2 commits into from
Aug 21, 2023

Conversation

jrfastab
Copy link
Contributor

@jrfastab jrfastab commented Aug 17, 2023 •
edited

Backport of PR #27319

for pr in 27319; do contrib/backporting/set-labels.py $pr done 1.11; done

@jrfastab
cilium: trigger validation of all nodes when key updated
ce4d713 
[upstream commit bba3dfc]

[backporter's notes: This was applied manually because it was
 mostly conflicts and even patch had some trouble.]

On key update we only update the policy for our local node. But with
latest round of changes we need to update for all nodes in the node
cache. With out this we would rely on the validation interval timer
to sync the nodes policy before we remove the old policy. This
may or may not happen depending on how large the cluster is.

Further I've seen us miss it even on relatively small clusters say
around 30 nodes so seems its not entirely reliable to count on.

Rather than rely on some external to ipsec timer to fire and sync
the policies and to do it hopefully in our time window lets just
force the nodeUpdate() call on all nodes in the cache when we
get the key rotate event.

Signed-off-by: John Fastabend <john.fastabend@gmail.com>
@jrfastab jrfastab requested a review from a team as a code owner August 17, 2023 05:58
@maintainer-s-little-helper maintainer-s-little-helper bot added backport/1.11 This PR represents a backport for Cilium 1.11.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master. labels Aug 17, 2023
@ti-mo
Copy link
Contributor

ti-mo commented Aug 17, 2023

@jrfastab the title of the first commit is [backport commit ], is that intended? We should always include upstream commit SHAs in backport commits as well, which doesn't seem to be the case here.

@joestringer
Copy link
Member

We expect that for any fix on older branches, the following steps also take place:

  • Propose and get the fix merged into main
  • Update the commit messages in the backport to mention the backported commit sha from the official version in the main branch, unless the implementation needs to be completely different. git log v1.11 should give a good sense for what the usual format for this in commit messages looks like.
  • Update the PR description here to include the following stanza so that we can correlate PRs and generate release notes. Note the PRs section and the Cilium branch version at the end: 
    ```upstream-prs
for pr in xxxxx; do contrib/backporting/set-labels.py $pr done 1.11; done
```
    It's also helpful to directly mention the upstream PR so that reviewers can contrast/compare with the original proposed fix, and also to ensure that github breadcrumbs are placed on the original PR. If we ever trace down the main PR it can be helpful to understand which branches that PR got backported to, so we can see whether subsequent fixes also need to be backported further.
  • Use /test-backport-X.Y to run CI

@jrfastab
Copy link
Contributor Author

@ti-mo I did the backport before the main PR was merged so I didn't have a commit to add yet. But yes will have one.

@jrfastab
cilium/ipsec: only update cilium_encrypt_map after xfrm is configured
e3c2e31 
[upstream commit 4f3bc9f]

[backporter's note: Had to apply manually because of many conflicts.]

The cilium_encrypt_map is used to determine what key the datapath should
use. This is done by setting the mark value of the skb and then in the
XFRM policy the mark value is matched to designate what encryption
policy/state to use.

However, on key rotation we have an issue where the map entry with the
key is updated before the xfrm policy is plumbed. The result is its
possible to mark the skb with a value that will have no matching
xfrm policy and result in a policy block error and drop the skb.

To resolve ensure we do setup in the correct order and only set the
min key in the cilium_encrypt_map after the policy has been updated.

Signed-off-by: John Fastabend <john.fastabend@gmail.com>
@jrfastab jrfastab force-pushed the pr/jrfastab/backport-v111-rollkeys branch from beda21e to e3c2e31 Compare August 17, 2023 22:43
@jrfastab
Copy link
Contributor Author

/test-backport-1.11

@jrfastab
Copy link
Contributor Author

@joestringer That 1.18-kernel-4.9 test failure looks unrelated to the patches. Also added upstream commits now that we have upstream PR merged.

@jrfastab
Copy link
Contributor Author

/test-1.18-4.9

joestringer
joestringer approved these changes Aug 21, 2023
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Ack on behalf of tophat

@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 Aug 21, 2023
@jrfastab jrfastab merged commit 95f8175 into v1.11 Aug 21, 2023
83 checks passed
@jrfastab jrfastab deleted the pr/jrfastab/backport-v111-rollkeys branch August 21, 2023 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer approved these changes

Assignees
No one assigned
Labels
backport/1.11 This PR represents a backport for Cilium 1.11.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master. ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jrfastab @ti-mo @joestringer