Skip to content

Conversation

joamaki
Copy link
Contributor

@joamaki joamaki commented Nov 12, 2024

To make sure that the shell commands cannot bring down the agent, catch the panics in the handler. On panic log it and also send it over the connection:

cilium> help tiuheot
PANIC: runtime error: invalid memory address or nil pointer dereference 
goroutine 1684 [running]:
github.com/cilium/cilium/daemon/cmd.shell.handleConn.func2()
        .../cilium/cilium/daemon/cmd/shell.go:109 +0x85
panic({0x3cba7e0?, 0x75d9af0?})
        .../share/go/src/runtime/panic.go:785 +0x132
github.com/cilium/hive/script.(*Engine).ListCmds(0xc000d68a38, ...
        .../cilium/cilium/vendor/github.com/cilium/hive/script/engine.go:731 +0x14c
        ...

@joamaki joamaki requested a review from a team as a code owner November 12, 2024 12:26
@joamaki joamaki requested a review from ldelossa November 12, 2024 12:26
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 12, 2024
@joamaki joamaki added release-note/misc This PR makes changes that have no direct user impact. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Nov 12, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Nov 12, 2024
@joamaki
Copy link
Contributor Author

joamaki commented Nov 12, 2024

/test

@joamaki joamaki enabled auto-merge November 14, 2024 15:23
To make sure that the shell commands cannot bring down the agent,
catch the panics in the handler. On panic log it and also send it
over the connection:

cilium> help tiuheot
PANIC: runtime error: invalid memory address or nil pointer dereference
goroutine 1684 [running]:
github.com/cilium/cilium/daemon/cmd.shell.handleConn.func2()
        .../cilium/cilium/daemon/cmd/shell.go:109 +0x85
panic({0x3cba7e0?, 0x75d9af0?})
        .../share/go/src/runtime/panic.go:785 +0x132
github.com/cilium/hive/script.(*Engine).ListCmds(0xc000d68a38, ...
        .../cilium/cilium/vendor/github.com/cilium/hive/script/engine.go:731 +0x14c
        ...

Fixes: 897fcb7 ("daemon,cilium-dbg: Cilium shell")
Signed-off-by: Jussi Maki <jussi@isovalent.com>
No point including all the commands as many of them are really
only useful in tests plus they clutter up the help.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
@joamaki
Copy link
Contributor Author

joamaki commented Nov 14, 2024

/test

@joamaki joamaki added this pull request to the merge queue Nov 18, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 18, 2024
Merged via the queue into cilium:main with commit cae7244 Nov 18, 2024
64 checks passed
@joamaki joamaki deleted the pr/joamaki/shell-recover branch November 18, 2024 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants