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

fix: upgrade K8s client library to add Keep Alive to watches #704

Merged
merged 3 commits into from
Apr 9, 2021

Conversation

ivanstanev
Copy link
Contributor

@ivanstanev ivanstanev commented Apr 9, 2021

  • Tests written and linted ℹ︎
  • Documentation written ℹ︎
  • Commit history is tidy ℹ︎

What this does

This has been an outstanding issue for over a month, especially prevalent in AKS. After some inactivity in the cluster, the persistent connection to the K8s API server got interrupted and was never able to recover.

This new fix in the K8s client library puts a 30s keep alive to the Watch connection so that it does not get interrupted.

More information

@ivanstanev ivanstanev requested a review from a team as a code owner April 9, 2021 08:01
@ivanstanev ivanstanev self-assigned this Apr 9, 2021
Copy link
Contributor

@kat1906 kat1906 left a comment

Choose a reason for hiding this comment

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

LGTM!

This has been an outstanding issue for over a month, especially prevalent in AKS. After some inactivity in the cluster, the persistent connection to the K8s API server got interrupted and was never able to recover.

This new fix in the K8s client library puts a 30s keep alive to the Watch connection so that it does not get interrupted.
This adds a new requirement of having extra environment variables. They are used to create imagePullSecrets to use for our fixtures, thus avoiding getting rate limited by Docker Hub when these images need to be pulled by KinD.

This does not solve the rate limiting from inside snyk-monitor using skopeo, which we've seen only very rarely. If that happens then the workaround is to set the dockercfg.json of snyk-monitor with auth details for Docker Hub.
We deploy a Pod in our integration tests using the --generator flag of kubectl.
Since 1.18 of kubectl, it seems that this flag has been deprecated and is no longer used: "has no effect and will be removed in the future".

This was caught by our tests because we try to use the latest version of kubectl, so breaking changes like this will surface.
@github-actions
Copy link

github-actions bot commented Apr 9, 2021

Expected release notes (by @ivanstanev)

fixes:
upgrade K8s client library to add Keep Alive to watches (cf44c9b)

others (will not be included in Semantic-Release notes):
remove --generator flag for kubectl in tests (f6cd596)
use imagePullSecrets in tests to avoid Docker Hub rate limiting (4510198)

  • I hereby acknowledge these release notes are 🥙 AWESOME 🥙

@ivanstanev ivanstanev merged commit aa2ab5c into staging Apr 9, 2021
@ivanstanev ivanstanev deleted the fix/keep-alive-informer branch April 9, 2021 12:49
@snyksec
Copy link

snyksec commented Apr 9, 2021

🎉 This PR is included in version 1.50.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants