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

[proxy] Fix proxy nil check #18198

Merged
merged 1 commit into from
Dec 15, 2021
Merged

[proxy] Fix proxy nil check #18198

merged 1 commit into from
Dec 15, 2021

Conversation

chaosbox
Copy link

@chaosbox chaosbox commented Dec 9, 2021 •
edited by joestringer

cilium-agent panic when enable-l7-proxy is toggled

This PR fixes the proxy check condition by calling !isProxyDisabled() on the endpoint instead of doing a nil check on the interface because the type exists but value is nil.

Fixes: #18195

Signed-off-by: chaosbox raamnathmani@gmail.com

Fix crash on startup if proxy is disabled

@maintainer-s-little-helper

This comment has been minimized.

Sign in to view
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Dec 9, 2021
[proxy] Fix proxy nil check
b72e65e 
Signed-off-by: chaosbox <ram29@bskyb.com>
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Dec 9, 2021
@chaosbox
Copy link
Author

chaosbox commented Dec 9, 2021

We deployed with the PR docker image in our environment and can confirm that cilium-agent doesnt crash anymore. Thanks guys!

@chaosbox chaosbox marked this pull request as ready for review December 9, 2021 14:46
@chaosbox chaosbox requested a review from a team as a code owner December 9, 2021 14:46
@pchaigno pchaigno added kind/bug This is a bug in the Cilium logic. needs-backport/1.10 release-note/bug This PR fixes an issue in a previous release of Cilium. labels Dec 9, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 9, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.6 Dec 9, 2021
@pchaigno
Copy link
Member

pchaigno commented Dec 9, 2021 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-GKE' hit: #17270 (93.03% similarity)

@maintainer-s-little-helper maintainer-s-little-helper bot mentioned this pull request Dec 9, 2021
Closed
@joestringer joestringer added this to Needs backport from master in 1.10.7 Dec 10, 2021
@joestringer joestringer removed this from Needs backport from master in 1.10.6 Dec 10, 2021
jrajahalme
jrajahalme approved these changes Dec 13, 2021
View reviewed changes
Copy link
Member

@jrajahalme jrajahalme left a comment

Choose a reason for hiding this comment

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

Thanks for the fix :-)

@nbusseneau nbusseneau added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. and removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Dec 14, 2021
@nbusseneau
Copy link
Member

nbusseneau commented Dec 14, 2021 •
edited

/test-1.23-net-next

Retriggering as previous run seemingly hit a variant of #17270 but I want to confirm.

@nbusseneau
Copy link
Member

nbusseneau commented Dec 14, 2021 •
edited

/ci-awscni

Retriggering as it didn't run on previous batch due to GH Actions shenanigans.

@nbusseneau nbusseneau added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Dec 15, 2021
@nbusseneau
Copy link
Member

CI passed, merging. @chaosbox Thanks for the contribution :)

@nbusseneau nbusseneau merged commit 13a7125 into cilium:master Dec 15, 2021
@tklauser tklauser mentioned this pull request Dec 16, 2021
Merged
@tklauser tklauser mentioned this pull request Dec 16, 2021
Merged
@tklauser tklauser added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels Dec 17, 2021
@tklauser tklauser moved this from Needs backport from master to Backport pending to v1.10 in 1.10.7 Dec 20, 2021
@tklauser tklauser moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.7 Dec 22, 2021
This was referenced Jan 18, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pchaigno pchaigno pchaigno approved these changes

@christarazi christarazi christarazi approved these changes

@jrajahalme jrajahalme jrajahalme approved these changes

Assignees
No one assigned
Labels
backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.10.7
Backport done to v1.10
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cilium-agent crash and unable to recover when enable-l7-proxy is toggled
6 participants
@chaosbox @pchaigno @nbusseneau @christarazi @jrajahalme @tklauser