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

Fix intermediate rotation test in ztunnel #50130

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix intermediate rotation test in ztunnel #50130

wants to merge 4 commits into from

Conversation

howardjohn
Copy link
Member

@howardjohn howardjohn commented Mar 27, 2024 •
edited

Fixes #49648.

Also speeds up the test, which currently takes ~3.5min typically. Now it should take at most 30s

@howardjohn howardjohn requested a review from a team as a code owner March 27, 2024 18:23
@howardjohn howardjohn added the release-notes-none Indicates a PR that does not require release notes. label Mar 27, 2024
@howardjohn howardjohn requested review from a team as code owners March 27, 2024 18:23
@istio-testing istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 27, 2024
howardjohn
howardjohn commented Mar 27, 2024
View reviewed changes
tests/integration/ambient/cacert_rotation_test.go Outdated
Comment on lines 67 to 68
"ca-cert.pem", "ca-key.pem",
"cert-chain.pem", "root-cert.pem"); err != nil {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is broken since the test asserts we are using the same root (its testing intermediates). But I don't understand how we generated these (broken) certs to regenerate them correctly. There seems to be no code for this.

@zirain maybe you can help?

Copy link
Member

Choose a reason for hiding this comment

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

istio/samples/certs/README.md

Line 16 in 1ee1a43

- `cert-chain.pem`: certificate trust chain.

I don't know why @GregHanson didn't use ca-cert.pem and ca-cert-alt.pem in the beginning.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, haven't been following this debug too closely. @zirain here is the discussion for why I had to generate new ones:
#48168 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

@howardjohn cert should have been generated with the script security/samples/plugin_ca_certs/gen_certs.sh or using the steps here

This should be the same script @hzxuzhonghu used when writing the root cert rotation test

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, ran this script and updated a bit to try to automate things but failing some tests still.

howardjohn
howardjohn commented Mar 27, 2024
View reviewed changes
tests/integration/ambient/cacert_rotation_test.go
t.Errorf("failed to update CA secret: %v", err)
}

// perform one retry to handle race condition where ztunnel cert is refreshed before Istiod certificates are reloaded
retry.UntilSuccess(func() error {
retry.UntilSuccessOrFail(t, func() error {
Copy link
Member Author

Choose a reason for hiding this comment

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

this wasn't asserting, so they test always passed even though it was broken. Then it left things in a permanently broken state

craigbox
craigbox approved these changes Mar 27, 2024
View reviewed changes
Copy link
Contributor

@craigbox craigbox left a comment

Choose a reason for hiding this comment

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

one line docs change LGTM

MorrisLaw
MorrisLaw reviewed Mar 29, 2024
View reviewed changes
tests/integration/ambient/main_test.go
@@ -95,7 +95,7 @@ values:
ztunnel:
terminationGracePeriodSeconds: 5
env:
SECRET_TTL: 5m
SECRET_TTL: 1m
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this create unexpected behavior in the test assertions due to the recent cert refresh_at update?

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 29, 2024
@istio-testing istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR needs to be rebased before being merged size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 2, 2024
@istio-testing istio-testing added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 2, 2024
@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label May 3, 2024
@MorrisLaw MorrisLaw removed the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label May 3, 2024
@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label May 4, 2024
@istio-policy-bot istio-policy-bot added the lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. label May 18, 2024
@craigbox
Copy link
Contributor

/lifecycle staleproof

@craigbox craigbox added the lifecycle/staleproof Indicates a PR or issue has been deemed to be immune from becoming stale and/or automatically closed label May 19, 2024
@craigbox craigbox reopened this May 19, 2024
@istio-testing
Copy link
Collaborator

@howardjohn: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
lint_istio 35c8202 link true /test lint
unit-tests-arm64_istio 35c8202 link true /test unit-tests-arm64
unit-tests_istio 35c8202 link true /test unit-tests
integ-security-istiodremote_istio 35c8202 link true /test integ-security-istiodremote
integ-security_istio 35c8202 link true /test integ-security
integ-security-multicluster_istio 35c8202 link true /test integ-security-multicluster

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@istio-policy-bot istio-policy-bot removed the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zirain zirain zirain left review comments

@MorrisLaw MorrisLaw MorrisLaw left review comments

@GregHanson GregHanson GregHanson left review comments

@craigbox craigbox craigbox approved these changes

At least 1 approving review is required to merge this pull request.

Assignees

@howardjohn howardjohn

@zirain zirain

Labels
lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. lifecycle/staleproof Indicates a PR or issue has been deemed to be immune from becoming stale and/or automatically closed release-notes-none Indicates a PR that does not require release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test flake: private key does not match public key
7 participants
@howardjohn @craigbox @istio-testing @zirain @MorrisLaw @GregHanson @istio-policy-bot