Add signal-reboot #814

ckotzbauer · 2023-08-05T13:38:53Z

Based on #813
close #416
close #722

This PR adds a --reboot-method flag with "command" (default) and "signal" option. The "command" option uses the --reboot-command on the host with nsenter as before. The new "signal" mode uses a SIGRTMIN+5 signal by default against PID 1 (systemd) to reboot the node. The signal can be changed via --reboot-signal flag.

With this, the kured pod runs without privileged mode.

This PR is published as docker-image (amd64 and arm64): ghcr.io/ckotzbauer/kured:1.14.0-alpha.2
Usage (based on the latest helm-chart) - Helm values.yaml:

image:
  repository: ghcr.io/ckotzbauer/kured
  tag: 1.14.0-alpha.2
updateStrategy: RollingUpdate
configuration:
  period: "0h0m30s"
  rebootDelay: 0h1m0s
  rebootSentinel: /sentinel/reboot-required
extraArgs:
  reboot-method: signal
containerSecurityContext:
  readOnlyRootFilesystem: true
  privileged: false
  allowPrivilegeEscalation: false
  capabilities:
    drop: ["*"]
    add: ["CAP_KILL"]
volumes:
  - name: sentinel
    hostPath:
      path: /var/run
      type: Directory
volumeMounts:
  - name: sentinel
    mountPath: /sentinel
    readOnly: true

rptaylor · 2023-08-08T22:30:35Z

In my test this works, using the same values.yml file:

$ helm -n kured install -f ~/kured/values.yml   kured kubereboot/kured
$ helm -n kured list
NAME 	NAMESPACE	REVISION	UPDATED                                	STATUS  	CHART      	APP VERSION
kured	kured    	1       	2023-08-08 21:50:33.438477295 +0000 UTC	deployed	kured-5.1.0	1.13.2     
$ kubectl -n kured exec -it kured-vs4xb -- /bin/sh
/ # kill -s SIGRTMIN+5   1

Then journald shows:

Aug 08 14:52:08 kermes-dev-k8s-node-a03 systemd[1]: Received SIGRTMIN+6 from PID 22396 (sh).

and the system immediately does a clean reboot. However note that "SIGRTMIN+5" was sent but it was interpreted by host as SIGRTMIN+6. I believe that is related to the different output of kill -l in the pod vs on the host, which is a potential compatibility issue that we want to avoid by not using the pod's kill executable in the first place; the go library will send the signal instead. When I instead did kill -s 39 1 it worked as expected. I'm not sure how universal the interpretation 39=SIGRTMIN+5 is or whether it could be a good idea for the precise signal to be configurable so that users can ensure the signal is definitively the right one for their particular OS.

Anyway this was running on a v1.23 k8s cluster with the old PSP admission controller disabled. The nodes are Almalinux 8.8. I'm afraid I could not reproduce the issue; it works fine for me. Looking at the capabilities of the kured process it also appears fine:

/ # ps -ef | grep kured | grep -v grep
 6107 root      0:00 /usr/bin/kured --ds-name=kured --ds-namespace=kured --metrics-port=8080 --period=0h0m30s --reboot-sentinel=/sentinel/reboot-required --reboot-command=/bin/systemctl reboot --reboot-delay=0h1m0s --log-format=text --reboot-method=signal
/ # grep ^Cap /proc/6107/status
CapInh:	0000000000000000
CapPrm:	00000000a80425fb
CapEff:	00000000a80425fb
CapBnd:	00000000a80425fb
CapAmb:	0000000000000000

Also when I touched the sentinel file on the host, kured worked as expected and the node rebooted:

time="2023-08-08T22:27:19Z" level=info msg="Reboot required"
.....   #evicting pods etc
time="2023-08-08T22:27:58Z" level=info msg="Delaying reboot for 1m0s"
time="2023-08-08T22:28:58Z" level=info msg="Emit reboot-signal for node: cluster-dev-k8s-node-a03"
time="2023-08-08T22:28:58Z" level=info msg="Waiting for reboot"

I confirmed with kubectl get pod -o yaml that the kured pods have

  hostPID: true


    securityContext:
      allowPrivilegeEscalation: false
      capabilities:
        add:
        - CAP_KILL
        drop:
        - '*'
      privileged: false
      readOnlyRootFilesystem: true

ckotzbauer · 2023-08-09T05:30:09Z

Thanks for your tests and the positive feedback!

Yes, the specific signal value may indeed be a problem. We would need to add a good guidance for the user in our docs, to determine which signal value is correct for their host OS. Most of the users won't know as this is expert knowlege 😆

ckotzbauer · 2023-08-12T09:53:41Z

I made it work on my cluster with priviliged: false, by disabling AppArmor for the container with the Pod-Annotation

container.apparmor.security.beta.kubernetes.io/kured: unconfined

rptaylor · 2023-08-14T18:18:05Z

Ah of course, I'm not familiar with AppArmor at all but that is the SELinux equivalent on some OSes.

github-actions · 2023-10-14T01:44:55Z

This PR was automatically considered stale due to lack of activity. Please refresh it and/or join our slack channels to highlight it, before it automatically closes (in 7 days).

rptaylor · 2023-10-16T18:40:39Z

Is anyone available to review this?

cmd/kured/main.go

jackfrancis · 2023-10-16T20:53:39Z

@rptaylor thanks for your patience, added some review comments for @ckotzbauer, I think this generally looks like a great evolution, thanks this work!

ckotzbauer · 2023-12-14T07:32:24Z

Sorry for the very long delay, this is ready for another review @jackfrancis @rptaylor

Signed-off-by: Christian Kotzbauer <git@ckotzbauer.de>

rptaylor · 2023-12-14T22:08:53Z

@ckotzbauer very exciting! I can test it on a dev cluster, is there a new image available in ghcr.io/ckotzbauer/ ?

ckotzbauer · 2023-12-15T08:03:38Z

Thanks for the hint: ghcr.io/ckotzbauer/kured:1.15.0-alpha.0 (amd64 and arm64) @rptaylor

rptaylor · 2023-12-15T22:46:52Z

Okay refreshing my memory on what this is even about 4 months later .... ;p

On a dev cluster I temporarily disabled PSP admission control for testing purposes.

$ cat values.yml 
image:
  repository: ghcr.io/ckotzbauer/kured
  tag: 1.15.0-alpha.0
configuration:
  period: "0h0m30s"
  rebootDelay: 0h1m0s
  rebootSentinel: /sentinel/reboot-required
# update faster
maxUnavailable: 10
extraArgs:
  reboot-method: signal
containerSecurityContext:
  readOnlyRootFilesystem: true
  privileged: false
  allowPrivilegeEscalation: false
  capabilities:
    drop: ["*"]
    add: ["CAP_KILL"]
volumes:
  - name: sentinel
    hostPath:
      path: /var/run
      type: Directory
volumeMounts:
  - name: sentinel
    mountPath: /sentinel
    readOnly: true

$ helm -n kured install -f values.yml   kured kubereboot/kured
$ helm -n kured list
NAME 	NAMESPACE	REVISION	UPDATED                                	STATUS  	CHART      	APP VERSION
kured	kured    	1       	2023-12-15 22:20:49.185347412 +0000 UTC	deployed	kured-5.2.0	1.14.0

Then on a node I touched /var/run/reboot-required.

pod logs:

$ kubectl -n kured logs -f kured-79vrp
time="2023-12-15T22:37:53Z" level=info msg="Binding node-id command flag to environment variable: KURED_NODE_ID"
time="2023-12-15T22:37:53Z" level=info msg="Kubernetes Reboot Daemon: 544a287"
time="2023-12-15T22:37:53Z" level=info msg="Node ID: cluster-dev-k8s-node-a02"
time="2023-12-15T22:37:53Z" level=info msg="Lock Annotation: kured/kured:weave.works/kured-node-lock"
time="2023-12-15T22:37:53Z" level=info msg="Lock TTL not set, lock will remain until being released"
time="2023-12-15T22:37:53Z" level=info msg="Lock release delay not set, lock will be released immediately after rebooting"
time="2023-12-15T22:37:53Z" level=info msg="PreferNoSchedule taint: "
time="2023-12-15T22:37:53Z" level=info msg="Blocking Pod Selectors: []"
time="2023-12-15T22:37:53Z" level=info msg="Reboot schedule: SunMonTueWedThuFriSat between 00:00 and 23:59 UTC"
time="2023-12-15T22:37:53Z" level=info msg="Reboot check command: [test -f /sentinel/reboot-required] every 30s"
time="2023-12-15T22:37:53Z" level=info msg="Concurrency: 1"
time="2023-12-15T22:37:53Z" level=info msg="Reboot method: signal"
time="2023-12-15T22:37:53Z" level=info msg="Reboot signal: 39"
time="2023-12-15T22:39:41Z" level=info msg="Reboot required"
time="2023-12-15T22:39:41Z" level=info msg="Acquired reboot lock"
time="2023-12-15T22:39:41Z" level=info msg="Draining node cluster-dev-k8s-node-a02"
evicting pod  ....
time="2023-12-15T22:40:35Z" level=info msg="Delaying reboot for 1m0s"
time="2023-12-15T22:41:35Z" level=info msg="Emit reboot-signal for node: cluster-dev-k8s-node-a02"
time="2023-12-15T22:41:35Z" level=info msg="Waiting for reboot"

Eventually .... node rebooted:

Dec 15 14:41:35 cluster-dev-k8s-node-a02 systemd[1]: Received SIGRTMIN+5 from PID 2802298 (kured).

Does that cover everything needed? Is it good to merge now?

rptaylor · 2023-12-15T22:51:20Z

Actually wait ... the node powered off instead of rebooting, that's not good...

rptaylor · 2023-12-15T23:04:05Z

I tried again and the node rebooted normally. Sometimes our cloud infrastructure is a bit flakey... Anyway as long as systemd on the node reports "Received SIGRTMIN+5" it should be good.

rptaylor · 2023-12-15T23:51:15Z

cmd/kured/main.go

+	// MethodSignal is used as "--reboot-method" value when rebooting with a SIGRTMIN+5 signal.
+	MethodSignal = "signal"
+
+	sigTrminPlus5 = 34 + 5


Is this meant to be sigRtminPlus5 ?

Yes, I fix the typo.

Looks like the typo is still there? But otherwise this should be finally good to go!

Sorry, I didn't catch that. Let's fix the typo. If we're going to use camel case I'd vote for:

sigRTMin

Maybe:

const ( ... sigRTMin = 34 defaultSignal = sigRTMin + 5 ... )

And we should probably change the MethodSignal comment so that it doesn't imply that SIGRTMIN+5 is static (it's just a default, but the user can use whatever number within the min-max range that their OS allows). Maybe:

// MethodSignal is used as "--reboot-method" value when rebooting according to a signal in the allowed range between SIGRTMIN and SIGRTMAX as defined on the OS platform running kured

ckotzbauer · 2023-12-16T09:35:49Z

Thanks for testing @rptaylor, this should cover all needed changes, so this should be ready for merge.

jackfrancis

lgtm

great work, thank you!

ckotzbauer added enhancement This was triaged as an enhancement security labels Aug 5, 2023

ckotzbauer requested review from jackfrancis and evrardjp August 5, 2023 13:38

ckotzbauer mentioned this pull request Aug 5, 2023

improve security of reboot mechanism #416

Closed

ckotzbauer changed the title ~~[WIP] Add signal-reboot~~ Add signal-reboot Aug 12, 2023

ckotzbauer mentioned this pull request Aug 12, 2023

Integrate "reboot-signal" kubereboot/charts#51

Closed

ckotzbauer added this to the 1.15.0 milestone Aug 12, 2023

ckotzbauer force-pushed the feature/signal-reboot branch from 38dfbb2 to ae027af Compare August 14, 2023 17:15

github-actions bot added the no-pr-activity label Oct 14, 2023

ckotzbauer added keep This won't be closed by the stale bot. and removed no-pr-activity labels Oct 14, 2023

jackfrancis reviewed Oct 16, 2023

View reviewed changes

cmd/kured/main.go Outdated Show resolved Hide resolved

jackfrancis reviewed Oct 16, 2023

View reviewed changes

cmd/kured/main.go Outdated Show resolved Hide resolved

ckotzbauer added 8 commits December 14, 2023 09:59

feat: sentinel-command without nsenter by default

31101fe

Signed-off-by: Christian Kotzbauer <git@ckotzbauer.de>

fix: no readonly mount

0fdcd6b

Signed-off-by: Christian Kotzbauer <git@ckotzbauer.de>

fix: mount at different folder

db33b8e

Signed-off-by: Christian Kotzbauer <git@ckotzbauer.de>

feat: add signal-reboot

5a58a73

Signed-off-by: Christian Kotzbauer <git@ckotzbauer.de>

feat: make signal configurable and add tests

431e466

Signed-off-by: Christian Kotzbauer <git@ckotzbauer.de>

build: rename job

38529aa

Signed-off-by: Christian Kotzbauer <git@ckotzbauer.de>

cleanup: linter

e663b14

Signed-off-by: Christian Kotzbauer <git@ckotzbauer.de>

build: also adjust signal manifest

128b9e0

Signed-off-by: Christian Kotzbauer <git@ckotzbauer.de>

ckotzbauer added 3 commits December 14, 2023 09:59

test: add e2e-tests

301474d

Signed-off-by: Christian Kotzbauer <git@ckotzbauer.de>

fix: small code restructure

267870e

Signed-off-by: Christian Kotzbauer <git@ckotzbauer.de>

fix: adjust version-range

544a287

Signed-off-by: Christian Kotzbauer <git@ckotzbauer.de>

ckotzbauer force-pushed the feature/signal-reboot branch from 2e2a527 to 544a287 Compare December 14, 2023 09:00

This was referenced Dec 15, 2023

path-based reboot mechanism #868

Closed

Sentinel-command without nsenter by default #813

Closed

rptaylor reviewed Dec 15, 2023

View reviewed changes

jackfrancis approved these changes Jan 3, 2024

View reviewed changes

ckotzbauer merged commit 87202d8 into main Jan 6, 2024

ckotzbauer deleted the feature/signal-reboot branch January 6, 2024 09:25

ckotzbauer mentioned this pull request Jan 6, 2024

improve security of sentinel mechanism #526

Closed

bh185140 mentioned this pull request Feb 8, 2024

Make nsenter in reboot command optional #896

Closed

andreas-wirth mentioned this pull request Feb 13, 2024

'Permission denied' when using signal reboot mechanism to reboot AKS nodes #898

Closed

OlleLarsson mentioned this pull request Jun 5, 2024

Upgrade Kured to v1.15.1 elastisys/compliantkubernetes-apps#2160

Merged

33 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add signal-reboot #814

Add signal-reboot #814

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Add signal-reboot #814

Add signal-reboot #814

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!