-
Notifications
You must be signed in to change notification settings - Fork 3.4k
CountUniqueIPsecKeys function fixed. #29182
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
Conversation
dd20aba
to
389776d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
There was a problem hiding this 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.
There was a problem hiding this 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.
Could you add some description of what's being fixed to the commit message? |
ad2c266
to
53c58f3
Compare
Done. I also squashed and renamed other commits (hope it looks better now). |
53c58f3
to
b247299
Compare
There was a problem hiding this 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?
There was a problem hiding this 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>
There was a problem hiding this 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.
/test |
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXX
line if the commit addresses a particularGitHub issue.
Fixes: <commit-id>
tag, thenplease add the commit author[s] as reviewer[s] to this issue.
Fixes: #issue-number