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

Filter out bash kubernetes healthchecks #2262

Merged
merged 1 commit into from
Apr 15, 2024
Merged

Conversation

alexeysofin
Copy link
Contributor

This patch adds this by

  1. filtering *.sh binaries in healthcheck filter
  2. removing first argument in exec to correctly compare it to k8s pod's exec probe

Fixes: #2261

pkg/filters/health_check.go Show resolved Hide resolved
@tixxdz tixxdz requested a review from willfindlay April 9, 2024 14:42
@tixxdz
Copy link
Member

tixxdz commented Apr 9, 2024

Let's cc @willfindlay for more input

@tixxdz tixxdz added the release-note/minor This PR introduces a minor user-visible change label Apr 9, 2024
Copy link

netlify bot commented Apr 10, 2024

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit f6f5277
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/66165fd93ca4db00080cdff6
😎 Deploy Preview https://deploy-preview-2262--tetragon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@alexeysofin alexeysofin requested a review from tixxdz April 10, 2024 09:49
Copy link
Member

@tixxdz tixxdz left a comment

Choose a reason for hiding this comment

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

LGTM overall, thanks!

Still two things please: can you make it in one patch, and investigate the failing CI test as it could be related to your patch?

pkg/filters/health_check.go Show resolved Hide resolved
This patch adds this by removing first argument in exec to correctly compare it to k8s pod's exec probe

Signed-off-by: Aliaksei Sofin <sofin.moffin@gmail.com>
@alexeysofin
Copy link
Contributor Author

alexeysofin commented Apr 12, 2024

@tixxdz I rebased code to one patch, and synced my fork with latest main branch, e2e tests pass in my fork so guess it was some stale code issue, because the error seemed completely unrelated.

@alexeysofin alexeysofin requested a review from tixxdz April 12, 2024 12:57
Copy link
Member

@tixxdz tixxdz left a comment

Choose a reason for hiding this comment

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

Alright much appreciated ;-) !

@alexeysofin
Copy link
Contributor Author

@tixxdz thank you very much! Let's see how CI passes

@alexeysofin
Copy link
Contributor Author

@tixxdz well, it failed, now with some transient error in hostedtoolcache and a huge stack trace.

These tests seem rather flaky, maybe try restarting the job?

@tixxdz tixxdz merged commit 7203fb7 into cilium:main Apr 15, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor This PR introduces a minor user-visible change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filter out bash kubernetes healthchecks
2 participants