pkg/k8s: fix invalid memory address or nil pointer dereference #17642

aanm · 2021-10-19T13:28:49Z

Since k8sMeta might return nil, we should check for it before accessing
the fields of that structure otherwise we will risk on panic for a nil
pointer dereference.

Fixes: 63c0b29 ("Checks k8s metadata for pod before removing IP from ipccahe")
Signed-off-by: André Martins andre@cilium.io

2021-10-18T14:33:44.882292070Z Observed a panic: "invalid memory address or nil pointer dereference" (runtime error: invalid memory address or nil pointer dereference)
2021-10-18T14:33:44.882354474Z goroutine 269 [running]:
2021-10-18T14:33:44.882361875Z k8s.io/apimachinery/pkg/util/runtime.logPanic({0x226b480, 0x4037bc0})
2021-10-18T14:33:44.882367475Z 	/go/src/github.com/cilium/cilium/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:74 +0x85
2021-10-18T14:33:44.882374176Z k8s.io/apimachinery/pkg/util/runtime.HandleCrash({0x0, 0x0, 0xc00013ebe0})
2021-10-18T14:33:44.882382676Z 	/go/src/github.com/cilium/cilium/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:48 +0x75
2021-10-18T14:33:44.882388577Z panic({0x226b480, 0x4037bc0})
2021-10-18T14:33:44.882392477Z 	/usr/local/go/src/runtime/panic.go:1038 +0x215
2021-10-18T14:33:44.882396577Z github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).endpointDeleted(0xc0009ec380, 0xc001f31bc0)
2021-10-18T14:33:44.882412178Z 	/go/src/github.com/cilium/cilium/pkg/k8s/watchers/cilium_endpoint.go:214 +0xc5
2021-10-18T14:33:44.882416979Z github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).ciliumEndpointsInit.func3({0x25f7ce0, 0xc001f31bc0})
2021-10-18T14:33:44.882421779Z 	/go/src/github.com/cilium/cilium/pkg/k8s/watchers/cilium_endpoint.go:71 +0xa5
2021-10-18T14:33:44.882425479Z k8s.io/client-go/tools/cache.ResourceEventHandlerFuncs.OnDelete(...)
2021-10-18T14:33:44.882429380Z 	/go/src/github.com/cilium/cilium/vendor/k8s.io/client-go/tools/cache/controller.go:245
2021-10-18T14:33:44.882434180Z github.com/cilium/cilium/pkg/k8s/informer.NewInformerWithStore.func1({0x22ae5a0, 0xc001e70708})
2021-10-18T14:33:44.882437880Z 	/go/src/github.com/cilium/cilium/pkg/k8s/informer/informer.go:114 +0x216
2021-10-18T14:33:44.882441380Z k8s.io/client-go/tools/cache.(*DeltaFIFO).Pop(0xc00013ebe0, 0xc0000b1300)
2021-10-18T14:33:44.882444981Z 	/go/src/github.com/cilium/cilium/vendor/k8s.io/client-go/tools/cache/delta_fifo.go:544 +0x2ea
2021-10-18T14:33:44.882448781Z k8s.io/client-go/tools/cache.(*controller).processLoop(0xc00041d440)
2021-10-18T14:33:44.882452581Z 	/go/src/github.com/cilium/cilium/vendor/k8s.io/client-go/tools/cache/controller.go:183 +0x36
2021-10-18T14:33:44.882456081Z k8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1(0x7f0cdf4d71e0)
2021-10-18T14:33:44.882459382Z 	/go/src/github.com/cilium/cilium/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:155 +0x67
2021-10-18T14:33:44.882462982Z k8s.io/apimachinery/pkg/util/wait.BackoffUntil(0xf579c8, {0x2a4b760, 0xc0004c6870}, 0x1, 0xc0000565a0)
2021-10-18T14:33:44.882466282Z 	/go/src/github.com/cilium/cilium/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:156 +0xb6
2021-10-18T14:33:44.882469682Z k8s.io/apimachinery/pkg/util/wait.JitterUntil(0xc00041d4a8, 0x3b9aca00, 0x0, 0x80, 0x7f0cdf2e81e8)
2021-10-18T14:33:44.882497484Z 	/go/src/github.com/cilium/cilium/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:133 +0x89
2021-10-18T14:33:44.882501484Z k8s.io/apimachinery/pkg/util/wait.Until(...)
2021-10-18T14:33:44.882504985Z 	/go/src/github.com/cilium/cilium/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:90
2021-10-18T14:33:44.882508685Z k8s.io/client-go/tools/cache.(*controller).Run(0xc00041d440, 0xc0000565a0)
2021-10-18T14:33:44.882512485Z 	/go/src/github.com/cilium/cilium/vendor/k8s.io/client-go/tools/cache/controller.go:154 +0x2fb
2021-10-18T14:33:44.882515885Z created by github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).ciliumEndpointsInit
2021-10-18T14:33:44.882519986Z 	/go/src/github.com/cilium/cilium/pkg/k8s/watchers/cilium_endpoint.go:87 +0x65

Since k8sMeta might return nil, we should check for it before accessing the fields of that structure otherwise we will risk on panic for a nil pointer dereference. Fixes: 63c0b29 ("Checks k8s metadata for pod before removing IP from ipccahe") Signed-off-by: André Martins <andre@cilium.io>

pchaigno

Do we know if it's expected that we try to remove from the ipcache an IP address that isn't in the ipcache? /cc @Weil0ng

nbusseneau

🎉

Weil0ng · 2021-10-19T18:07:40Z

Do we know if it's expected that we try to remove from the ipcache an IP address that isn't in the ipcache? /cc @Weil0ng

So removal happens when we get an CEP delete event and the insertion should happen when we first get the add event for the CEP, correct? I could be missing something but if the k8s cache is synced, then we should always have the IP address in the ipcache before getting the delete event?

aanm · 2021-10-20T12:11:10Z

Do we know if it's expected that we try to remove from the ipcache an IP address that isn't in the ipcache? /cc @Weil0ng

So removal happens when we get an CEP delete event and the insertion should happen when we first get the add event for the CEP, correct? I could be missing something but if the k8s cache is synced, then we should always have the IP address in the ipcache before getting the delete event?

@Weil0ng I think we might have received the CEP delete event for a CEP that didn't have any metadata in the ipcache. That could have happened before the k8s cache was synced.

Weil0ng · 2021-10-20T15:58:10Z

@Weil0ng I think we might have received the CEP delete event for a CEP that didn't have any metadata in the ipcache. That could have happened before the k8s cache was synced.

Yea makes sense.

aanm · 2021-10-20T21:50:11Z

/test

codablock · 2021-11-05T17:42:47Z

Can this be backported to 1.10? I stumbled across this crash today and was forced to switch to a CI image.

pchaigno · 2021-11-05T18:37:38Z

This is fixing a bug, so per our backporting guidelines should be backported to v1.10. I think it was just an oversight before. @codablock Thanks for reporting it! 🙏

aanm · 2021-11-16T12:17:57Z

@codablock can you open a GH issue with the panic that you have seen? The bug this PR is fixing does not exist on v1.10 since the code was introduced in v1.11. That's why it was not marked as to be backported. /cc @pchaigno

codablock · 2021-11-16T13:08:22Z

@aanm I don't have the logs anymore. But I later realised that I was on v1.11-rc0 due the Helm chart being on that version already. So I assume I got unlucky and was using a version that had the bug freshly introduced but the fix missing.

Weil0ng

Just realized, shouldn't this line need fix too? https://github.com/cilium/cilium/blob/master/pkg/k8s/watchers/pod.go#L828

But maybe this is superseded by #17909 and doesn't require a separate fix?

joestringer · 2021-12-08T03:31:53Z

@aanm do you know the answer to @Weil0ng 's question above?

Weil0ng · 2021-12-08T05:43:07Z

@aanm do you know the answer to @Weil0ng 's question above?

I think it is captured in #17909 :)

aanm · 2021-12-09T02:42:00Z

@aanm do you know the answer to @Weil0ng 's question above?

I think it is captured in #17909 :)

Sorry @Weil0ng if people don't ping me directly on GH I will miss messages :-)

Weil0ng · 2021-12-10T03:56:48Z

no worries, my bad :) Will make sure to tag folks when it's a directed question next time.

aanm added the release-note/misc This PR makes changes that have no direct user impact. label Oct 19, 2021

aanm requested review from a team and errordeveloper October 19, 2021 13:28

maintainer-s-little-helper bot assigned errordeveloper Oct 19, 2021

aanm added release-note/bug This PR fixes an issue in a previous release of Cilium. and removed release-note/misc This PR makes changes that have no direct user impact. labels Oct 19, 2021

aanm changed the title ~~kg/k8s: fix invalid memory address or nil pointer dereference~~ pkg/k8s: fix invalid memory address or nil pointer dereference Oct 19, 2021

aanm force-pushed the pr/fix-panic branch from fb86617 to 5c0df24 Compare October 19, 2021 13:29

pchaigno approved these changes Oct 19, 2021

View reviewed changes

nbusseneau approved these changes Oct 19, 2021

View reviewed changes

aanm added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 22, 2021

kkourt merged commit 5802e53 into cilium:master Oct 22, 2021

pchaigno added the needs-backport/1.10 label Nov 5, 2021

maintainer-s-little-helper bot added this to Needs backport from master in 1.10.6 Nov 5, 2021

ti-mo mentioned this pull request Nov 12, 2021

v1.10 backports 2021-11-12 #17861

Merged

ti-mo added backport-pending/1.10 and removed needs-backport/1.10 labels Nov 12, 2021

maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.6 Nov 12, 2021

Weil0ng reviewed Nov 16, 2021

View reviewed changes

ti-mo removed the backport-pending/1.10 label Nov 17, 2021

maintainer-s-little-helper bot removed this from Backport pending to v1.10 in 1.10.6 Nov 17, 2021

aanm deleted the pr/fix-panic branch December 9, 2021 02:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/k8s: fix invalid memory address or nil pointer dereference #17642

pkg/k8s: fix invalid memory address or nil pointer dereference #17642

aanm commented Oct 19, 2021 •

edited

pchaigno left a comment

nbusseneau left a comment

Weil0ng commented Oct 19, 2021

aanm commented Oct 20, 2021

Weil0ng commented Oct 20, 2021

aanm commented Oct 20, 2021

codablock commented Nov 5, 2021

pchaigno commented Nov 5, 2021

aanm commented Nov 16, 2021

codablock commented Nov 16, 2021

Weil0ng left a comment

joestringer commented Dec 8, 2021

Weil0ng commented Dec 8, 2021

aanm commented Dec 9, 2021

Weil0ng commented Dec 10, 2021

pkg/k8s: fix invalid memory address or nil pointer dereference #17642

pkg/k8s: fix invalid memory address or nil pointer dereference #17642

Conversation

aanm commented Oct 19, 2021 • edited

pchaigno left a comment

Choose a reason for hiding this comment

nbusseneau left a comment

Choose a reason for hiding this comment

Weil0ng commented Oct 19, 2021

aanm commented Oct 20, 2021

Weil0ng commented Oct 20, 2021

aanm commented Oct 20, 2021

codablock commented Nov 5, 2021

pchaigno commented Nov 5, 2021

aanm commented Nov 16, 2021

codablock commented Nov 16, 2021

Weil0ng left a comment

Choose a reason for hiding this comment

joestringer commented Dec 8, 2021

Weil0ng commented Dec 8, 2021

aanm commented Dec 9, 2021

Weil0ng commented Dec 10, 2021

aanm commented Oct 19, 2021 •

edited