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.

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

Adds new Cilium subcommand: cilium encrypt status and cilium encrypt flush #16770

Merged
merged 3 commits into from Aug 2, 2021

Conversation

h3llix
Copy link
Contributor

@h3llix h3llix commented Jul 4, 2021

Fixes: #14638

cilium encrypt status displays information on the current status of the IPSec configuration while cilium encrypt flush flushes the current XFRM States.

Here is the cli output:
image

@h3llix h3llix requested review from a team as code owners July 4, 2021 21:07
@maintainer-s-little-helper

This comment has been minimized.

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jul 4, 2021
@h3llix h3llix requested review from twpayne and rolinh July 4, 2021 21:08
@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 4, 2021
@rolinh rolinh changed the title Adds new Cilium subcommand: cilium encypt status and cilium encrypt flush Adds new Cilium subcommand: cilium encrypt status and cilium encrypt flush Jul 5, 2021
@rolinh rolinh added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Jul 5, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 5, 2021
cilium/cmd/encrypt.go Outdated Show resolved Hide resolved
cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
@pchaigno pchaigno added the area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. label Jul 5, 2021
cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
@h3llix h3llix force-pushed the hellixSoc1 branch 2 times, most recently from b67277d to fe5c304 Compare July 8, 2021 20:17
@h3llix h3llix requested review from pchaigno and rolinh July 8, 2021 20:18
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.

Looks good. I only have minor comments.

We will also need unit tests for the helper functions in encrypt_status.go: getXfrmStats, countUniqueIPsecKeys, maxSequenceNumber. You can check examples for other commands in the same directory as that file.

cilium/cmd/encrypt_flush.go Outdated Show resolved Hide resolved
cilium/cmd/encrypt.go Outdated Show resolved Hide resolved
cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
vendor/modules.txt Show resolved Hide resolved
cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
@h3llix h3llix force-pushed the hellixSoc1 branch 2 times, most recently from 0aacb73 to 189f667 Compare July 12, 2021 08:07
Copy link
Contributor

@twpayne twpayne left a comment

Choose a reason for hiding this comment

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

Code looks good, looks like you need to run make -C Documentation update-cmdref to update the docs following the changes to the CLI option descriptions.

Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

LGTM (but with some nits below)

cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
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.

A couple remaining nits below. The output in case of failure could also be improved a bit. For example, this is what I get just after having run cilium encrypt flush:

Encryption: IPsec                     
Keys in use: 0                         
Max Seq. Number: /0xffffffff
Errors: 9                         
XfrmInNoStates: 9

For Max Seq. Number we could either default to a 0 value or even explicitly state N/A when they are no oseq matches. XfrmInNoStates should also be indented for clarity.

cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
@pchaigno pchaigno marked this pull request as draft July 26, 2021 14:36
@h3llix h3llix marked this pull request as ready for review July 28, 2021 14:10
@pchaigno pchaigno added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jul 29, 2021
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.

Found a couple errors while testing. We could also cover a couple more cases in the unit tests.

General remarks:

  • Commit fix(cilium encrypt): fix helper functions and minor nits should be squashed in the relevant previous commits. For example, if there is a fix to helper function A, then that fix should be in the commit that introduced function A.
  • We don't usually have the format feat(), test(), etc. in Cilium. I like it, but I think it would be best to drop it for consistency with the rest of the commit history.
  • You will need to rebase with latest master. In general, I would advise to always rebase with latest master when you update the branch. That's the best way to keep the branch up-to-date.

cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
cilium/cmd/fixtures/proc/net/xfrm_stat Outdated Show resolved Hide resolved
cilium/cmd/encrypt_status_test.go Show resolved Hide resolved
cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
cilium/cmd/encrypt_status.go Show resolved Hide resolved
@h3llix h3llix force-pushed the hellixSoc1 branch 2 times, most recently from 44c2a68 to 707999e Compare July 29, 2021 22:17
@h3llix h3llix requested a review from pchaigno July 29, 2021 22:19
Add subcommand `cilium encrypt status` and `cilium encrypt flush` to
interact with IPsec mode of the node.

Signed-off-by: Gaurav Genani <h3llix.pvt@gmail.com>
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.

Only one nit below. LGTM otherwise!

cilium/cmd/encrypt_status.go Outdated Show resolved Hide resolved
Signed-off-by: Gaurav Genani <h3llix.pvt@gmail.com>
Signed-off-by: Gaurav Genani <h3llix.pvt@gmail.com>
@pchaigno
Copy link
Member

The end-to-end tests don't cover those new CLIs and other tests are all passing. All team review requests are covered, sometimes with multiple reviews. Marking ready to merge.

@pchaigno pchaigno added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. and removed dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. labels Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI: cilium encrypt command
6 participants