Skip to content

Conversation

viktor-kurchenko
Copy link
Contributor

@viktor-kurchenko viktor-kurchenko commented Nov 15, 2023

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!

Fixes: #issue-number

Handle non-AEAD IPsec keys in `cilium encrypt status`.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 15, 2023
@pchaigno pchaigno added release-note/bug This PR fixes an issue in a previous release of Cilium. area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. labels Nov 15, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Nov 15, 2023
@pchaigno pchaigno linked an issue Nov 15, 2023 that may be closed by this pull request
2 tasks
@pchaigno pchaigno added area/cli Impacts the command line interface of any command in the repository. needs-backport/1.12 labels Nov 15, 2023
@viktor-kurchenko viktor-kurchenko force-pushed the pr/vk/ipsec/key/count/fix branch from dd20aba to 389776d Compare November 15, 2023 10:44
@viktor-kurchenko viktor-kurchenko marked this pull request as ready for review November 15, 2023 10:45
@viktor-kurchenko viktor-kurchenko requested a review from a team as a code owner November 15, 2023 10:45
@julianwiedmann julianwiedmann added the feature/ipsec Relates to Cilium's IPsec feature label Nov 15, 2023
Copy link
Member

@pchaigno pchaigno 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!

Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Minor comments around error message and readability.

Copy link
Contributor

@chancez chancez left a comment

Choose a reason for hiding this comment

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

I got pulled in via the metrics group but do not see any metrics changes. IPSEC isn't in my area of expertise so I'll defer to others.

@gentoo-root
Copy link
Contributor

Could you add some description of what's being fixed to the commit message? Fixes: #29181 would work best as the last line, after a proper title and a short description.

@viktor-kurchenko viktor-kurchenko force-pushed the pr/vk/ipsec/key/count/fix branch from ad2c266 to 53c58f3 Compare November 20, 2023 06:58
@viktor-kurchenko
Copy link
Contributor Author

Could you add some description of what's being fixed to the commit message? Fixes: #29181 would work best as the last line, after a proper title and a short description.

Done. I also squashed and renamed other commits (hope it looks better now).

@viktor-kurchenko viktor-kurchenko force-pushed the pr/vk/ipsec/key/count/fix branch from 53c58f3 to b247299 Compare November 21, 2023 06:52
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.

I noted this PR causes regression for #27996. For instance, after running ip x s a src 1.1.1.1 dst 1.1.1.2 proto esp spi 0x3 reqid 1 mode tunnel enc aes 0xf0e1d2c3b4a5f60708090a0b0c0d0e0f on kind-worker, cilium encrypt status at kind-worker failed with "Error: Cannot count unique IPSec keys: an unsupported key with only Auth or Crypt set has been found".
Is this what we want? In some cases users would install xfrm states out of cilium's management, do we expect cilium CLI to continue working?

@pchaigno pchaigno requested a review from asauber November 28, 2023 09:12
Copy link
Member

@asauber asauber left a comment

Choose a reason for hiding this comment

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

The logic and tests appear to be correct, so I'll approve.

Adding error handling here will be important for catching similar issues in the future. It can be done with a first pass that doesn't impact the existing logic.

…lement error handling.

Signed-off-by: viktor-kurchenko <viktor.kurchenko@isovalent.com>
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.

@viktor-kurchenko Nice work! Thanks for the updates Viktor.

@viktor-kurchenko
Copy link
Contributor Author

/test

@pchaigno pchaigno added this pull request to the merge queue Nov 30, 2023
@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 30, 2023
Merged via the queue into main with commit 6f227fb Nov 30, 2023
@pchaigno pchaigno deleted the pr/vk/ipsec/key/count/fix branch November 30, 2023 07:21
@nbusseneau nbusseneau mentioned this pull request Dec 5, 2023
10 tasks
@nbusseneau nbusseneau 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 Dec 5, 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 Dec 6, 2023
@github-actions github-actions bot added backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.12 labels Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli Impacts the command line interface of any command in the repository. area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. backport-done/1.14 The backport for Cilium 1.14.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/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
Status: Released
Status: Released
Status: Released
Development

Successfully merging this pull request may close these issues.

IPSec key count for AES-CBC algorithm
10 participants