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

Extend upgrade test for IPsec key rotation test #1798

Merged
merged 2 commits into from
Jul 20, 2023
Merged

Extend upgrade test for IPsec key rotation test #1798

merged 2 commits into from
Jul 20, 2023

Conversation

jschwinger233
Copy link
Member

@jschwinger233 jschwinger233 commented Jul 5, 2023 •
edited

This PR extends upgrade test, so IPsec key rotation interruption test can base on the same workflow.

The expected test flow is:

  1. Run "cilium-cli connectivity test --conn-disrupt-test-setup --include-conn-disrupt-test" to deploy connectivity sensitive pods, then record the current restart counts and xfrm error counts.
  2. Perform IPsec key rotation. This part can be done in the cilium's ci-e2e.
  3. Run "cilium-cli connectivity test --include-conn-disrupt-test" to check restart counts and xfrm error counts again, test fails if any diff is found.

Partially Fixes: cilium/cilium#26350

Signed-off-by: Zhichuan Liang gray.liang@isovalent.com

@jschwinger233 jschwinger233 temporarily deployed to ci July 5, 2023 08:32 — with GitHub Actions Inactive
@jschwinger233 jschwinger233 temporarily deployed to ci July 5, 2023 08:43 — with GitHub Actions Inactive
@jschwinger233 jschwinger233 temporarily deployed to ci July 5, 2023 08:56 — with GitHub Actions Inactive
@jschwinger233 jschwinger233 temporarily deployed to ci July 5, 2023 09:03 — with GitHub Actions Inactive
@jschwinger233 jschwinger233 marked this pull request as ready for review July 5, 2023 10:07
@jschwinger233 jschwinger233 requested a review from a team as a code owner July 5, 2023 10:07
@jschwinger233 jschwinger233 temporarily deployed to ci July 6, 2023 10:02 — with GitHub Actions Inactive
@jschwinger233 jschwinger233 marked this pull request as draft July 6, 2023 10:16
@jschwinger233 jschwinger233 temporarily deployed to ci July 10, 2023 07:08 — with GitHub Actions Inactive
@jschwinger233 jschwinger233 temporarily deployed to ci July 10, 2023 07:47 — with GitHub Actions Inactive
@jschwinger233
Copy link
Member Author

CI failed on "Clean up EKS":

Error: unable to describe cluster control plane: operation error EKS: DescribeCluster, https response error StatusCode: 404, RequestID: 15a82023-354c-4360-ad2d-0f20279da2cc, ResourceNotFoundException: No cluster found for name: cilium-cilium-cli-5505449294-1-classic.

@jschwinger233 jschwinger233 marked this pull request as ready for review July 10, 2023 10:37
@brb brb requested review from pchaigno and brb and removed request for asauber July 10, 2023 11:40
pchaigno
pchaigno reviewed Jul 11, 2023
View reviewed changes
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 PR! I can't wait to have those tests automated 😅

internal/cli/cmd/connectivity.go Show resolved Hide resolved
internal/cli/cmd/connectivity.go Outdated Show resolved Hide resolved
connectivity/tests/interruption.go Outdated Show resolved Hide resolved
connectivity/tests/interruption.go Outdated Show resolved Hide resolved
connectivity/tests/interruption.go Outdated Show resolved Hide resolved
connectivity/tests/interruption.go Outdated Show resolved Hide resolved
@jschwinger233 jschwinger233 temporarily deployed to ci July 13, 2023 04:53 — with GitHub Actions Inactive
brb
brb requested changes Jul 14, 2023
View reviewed changes
Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

Can't wait to see this in action! A few nits.

connectivity/check/check.go Outdated Show resolved Hide resolved
internal/cli/cmd/connectivity.go Show resolved Hide resolved
connectivity/tests/interruption.go Outdated Show resolved Hide resolved
pchaigno
pchaigno reviewed Jul 18, 2023
View reviewed changes
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.

It looks great!

Just need to fix the commit history and we should be good to merge on my side :-)

connectivity/check/check.go Show resolved Hide resolved
connectivity/tests/interruption.go Outdated Show resolved Hide resolved
connectivity/tests/interruption.go Outdated Show resolved Hide resolved
@jschwinger233
connectivity: generalize upgrade test
618bcb6 
This commit generalizes upgrade test so ipsec key rotation test can
work on that with little extension.

Basically it's just some rename works with no logic change, something
like moving "--include-upgrade-test" to "--include-conn-disrupt-test".

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
@jschwinger233 jschwinger233 temporarily deployed to ci July 20, 2023 05:41 — with GitHub Actions Inactive
brb
brb approved these changes Jul 20, 2023
View reviewed changes
Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

Thanks!

@brb brb requested a review from pchaigno July 20, 2023 06:21
@jschwinger233 jschwinger233 temporarily deployed to ci July 20, 2023 13:10 — with GitHub Actions Inactive
@jschwinger233
connectivity: add xfrm error check for IPsec conn disrupt test
3303538 
This commit adds a new scenario for checking IPsec xfrm errors as a part of
conn disrupt test. It compares the current xfrm status with previous
record.

In detail, the test consists of 3 steps:

1. Run "cilium-cli connectivity test --conn-disrupt-test-setup
   --include-conn-disrupt-test" to deploy connectivity
   sensitive pods, then record the current restart counts and xfrm error
   counts.
2. Perform IPsec key rotation. This part can be done in the ci-e2e.
3. Run "cilium-cli connectivity test
   --include-conn-disrupt-test" to check restart counts
   and xfrm error counts again, test fails if any diff found.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
@jschwinger233 jschwinger233 temporarily deployed to ci July 20, 2023 13:40 — with GitHub Actions Inactive
asauber
asauber approved these changes Jul 20, 2023
View reviewed changes
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.

Approving on behalf of CLI. I think the flag replacement is reasonable.

@michi-covalent michi-covalent merged commit e656f58 into cilium:main Jul 20, 2023
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brb brb brb approved these changes

@pchaigno pchaigno pchaigno approved these changes

@asauber asauber asauber approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Testing IPsec key rotation
5 participants
@jschwinger233 @brb @pchaigno @asauber @michi-covalent