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(kuma-cp): don't let CA requests for other meshes block generation #6282

Merged
merged 3 commits into from
Mar 16, 2023
Merged

fix(kuma-cp): don't let CA requests for other meshes block generation #6282

merged 3 commits into from
Mar 16, 2023

Conversation

michaelbeaumont
Copy link
Contributor

@michaelbeaumont michaelbeaumont commented Mar 16, 2023 •
edited
Loading

This PR adds a default timeout to calls of GetRootCert for the case that the CA manager takes too long, so that it doesn't block generation, especially in the cross-mesh case.

Checklist prior to review

  • Link to relevant issue as well as docs and UI issues --
  • This will not break child repos: it doesn't hardcode values (.e.g "kumahq" as a image registry) and it will work on Windows, system specific functions like syscall.Mkfifo have equivalent implementation on the other OS --
  • Tests (Unit test, E2E tests, manual test on universal and k8s) --
  • Do you need to update UPGRADE.md? --
  • Does it need to be backported according to the backporting policy? --
  • Do you need to explicitly set a > Changelog: entry here or add a ci/ label to run fewer/more tests?

@michaelbeaumont
fix(kuma-cp): don't let CA requests for other meshes block generation
59409f6 
Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
@michaelbeaumont michaelbeaumont marked this pull request as ready for review March 16, 2023 09:14
@michaelbeaumont michaelbeaumont requested review from a team, slonka and jakubdyszkiewicz and removed request for a team March 16, 2023 09:14
jakubdyszkiewicz
jakubdyszkiewicz requested changes Mar 16, 2023
View reviewed changes
Copy link
Contributor

@jakubdyszkiewicz jakubdyszkiewicz left a comment

Choose a reason for hiding this comment

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

I think what should happen is that we have a default timeout for ~10s if it's not defined on the Mesh object. The default should be in ca_provider.go

Default 1 second is not a good idea IMHO, it's too low.

@michaelbeaumont
refactor: always use timeout, increase to 10 sec
09d4218 
Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
jakubdyszkiewicz
jakubdyszkiewicz reviewed Mar 16, 2023
View reviewed changes
Copy link
Contributor

@jakubdyszkiewicz jakubdyszkiewicz left a comment

Choose a reason for hiding this comment

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

Please change the description of the PR with short explanation why we need default timeout. Also update proto of Mesh to say that when not specified it's 10s.

I think we should also add a default timeout for the identity cert provider, because you may hit the same issue.

@michaelbeaumont
docs: add note about default CA timeout
a9ecd88 
Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
@michaelbeaumont michaelbeaumont merged commit 463e6b2 into kumahq:master Mar 16, 2023
@michaelbeaumont michaelbeaumont deleted the fix/ca_other-meshes branch March 16, 2023 11:28
@michaelbeaumont
Copy link
Contributor Author

@Mergifyio backport release-2.1

@mergify
Copy link
Contributor

mergify bot commented Mar 16, 2023 •
edited
Loading

backport release-2.1

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Mar 16, 2023
@michaelbeaumont @mergify
fix(kuma-cp): don't let CA requests for other meshes block generation (…
dcd5c2f 
…#6282)

This PR adds a default timeout of 10s to calls of GetRootCert for the case that the CA manager takes too long, so that it doesn't block generation, especially in the cross-mesh case.

Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
(cherry picked from commit 463e6b2)
@mergify mergify bot mentioned this pull request Mar 16, 2023
Merged
michaelbeaumont added a commit that referenced this pull request Mar 16, 2023
@mergify @michaelbeaumont
fix(kuma-cp): don't let CA requests for other meshes block generation…
d18c0a1 
… (backport #6282) (#6284)

fix(kuma-cp): don't let CA requests for other meshes block generation (#6282)

This PR adds a default timeout of 10s to calls of GetRootCert for the case that the CA manager takes too long, so that it doesn't block generation, especially in the cross-mesh case.

Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
(cherry picked from commit 463e6b2)

Co-authored-by: Mike Beaumont <mjboamail@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakubdyszkiewicz jakubdyszkiewicz jakubdyszkiewicz approved these changes

@slonka slonka Awaiting requested review from slonka

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

Successfully merging this pull request may close these issues.
2 participants
@michaelbeaumont @jakubdyszkiewicz