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

service: Fix panic when restoring services with enable-health-check-nodeport: false #13190

Merged

Conversation

gandro
Copy link
Member

@gandro gandro commented Sep 16, 2020

We do not instantiate the service health server if HealthCheckNodePort
support is disabled in our kube-proxy replacement. This means that we
need to check the EnableHealthCheckNodePort flag before accessing the
service health server to avoid a nil pointer panic.

This PR also adds a unit test to catch potential regressions.

Fixes: edff374 ("agent: Add an option to cilium-agent for disabling 'HealthCheckNodePort'")
Fixes: #13178

Note to backporters: The unit test (second commit) may be skipped for backports if it causes non-trivial conflicts.

Fix panic when restoring services with enable-health-check-nodeport: false

…odeport: false

We do not instanciate the service health server if HealthCheckNodePort
support is disabled in our kube-proxy replacement. This means that we
need to check the `EnableHealthCheckNodePort` flag before accessing the
service health server to avoid a nil pointer panic.

Fixes: edff374 ("agent: Add an option to cilium-agent for disabling 'HealthCheckNodePort'")
Fixes: #13178

Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
When HealthCheckNodePort is false, then the service health server will
be nil. This commit adds a unit test to ensure that the code does not
crash if the service health server is nil.

Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
@gandro gandro added release-note/bug This PR fixes an issue in a previous release of Cilium. needs-backport/1.7 labels Sep 16, 2020
@gandro gandro requested a review from a team September 16, 2020 13:30
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.4 Sep 16, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.7.10 Sep 16, 2020
@gandro
Copy link
Member Author

gandro commented Sep 16, 2020

test-me-please

@aanm aanm merged commit 584318f into master Sep 16, 2020
@aanm aanm deleted the pr/gandro/fix-nil-pointer-if-healthchecknodeport-is-disabled branch September 16, 2020 15:57
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.7 in 1.7.10 Sep 17, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.4 Sep 18, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.4 Sep 18, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.7 to Backport done to v1.7 in 1.7.10 Sep 18, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.7 to Backport done to v1.7 in 1.7.10 Sep 18, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.4 Sep 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.7.10
Backport done to v1.7
1.8.4
Backport done to v1.8
Development

Successfully merging this pull request may close these issues.

Panic on restoring services with enable-health-check-nodeport: false
4 participants