rbd: Add timeout for cryptsetup commands #4912

black-dragon74 · 2024-10-17T12:00:05Z

Describe what this PR does

This PR introduces a new wrapper around LUKS utility functions that allow us to have context aware subsequent operations, util.NewLuksWrapperWithContext. This context is shared across the functions exposed by this wrapper.

A sample use case is to have a context that times out after certain period of time.

How to Test

We can simulate a timeout by replacing the cryptsetup binary with a wrapper script that introduces some delay.

Exec into the pod where the cryptsetup binary is located (rbd-plugin in case of key rotation)
mv /usr/sbin/cryptsetup /usr/sbin/cryptsetup_orig
Create a new /usr/sbin/cryptsetup and paste the following:

#!/usr/bin/env bash

set -m

sleep 140

exec /usr/sbin/cryptsetup_orig "$@"

chmod +x /usr/sbin/cryptsetup
When the process is killed due to timeout you will see a log like:

timeout occurred while running cryptsetup args: [...]

nixpanic

This looks reasonable to me. Did you manage to test this somehow, and see how everything gets corrected by a different container?

Madhu-1 · 2024-10-18T08:36:55Z

This looks reasonable to me. Did you manage to test this somehow, and see how everything gets corrected by a different container?

we need to ensure that the CLI command we start is killed and doesn't continue to execute, please test it the changes to ensure that it works as expected and also provide some test results on how this is tested

Madhu-1 · 2024-10-18T08:38:12Z

internal/util/cryptsetup.go

-const cryptsetupPBKDFMemoryLimit = 32 << 10 // 32768 KiB
+const (
+	// Maximum time to wait for cryptsetup commands to complete.
+	CryptSetupExecutionTimeout = 3 * time.Minute


Lets keep the timeout which is almost less than the GRPC timeout between the sidecar and the cephcsi so that we try and if it didn't work as will kill the process before the next GRPC kicks in on any other nodes/same node.

I like the idea. Let's keep them a minute apart then? Accounting for any delays that might occur in sending gRPC request and also to keep some buffer period after sending SIGKILL?

minute might be too much, we can reduce it by 30 seconds to be on safer side, @nixpanic thoughts?

black-dragon74 · 2024-10-18T09:34:37Z

This looks reasonable to me. Did you manage to test this somehow, and see how everything gets corrected by a different container?

Trying but not able to yet. I am thinking of having a wrapper to cryptsetup that sleeps for some time and then tries to execute the intended command. The key is to kill the process in midst of operations.

I will test it and report.

nixpanic · 2024-10-18T13:07:57Z

Trying but not able to yet. I am thinking of having a wrapper to cryptsetup that sleeps for some time and then tries to execute the intended command. The key is to kill the process in midst of operations.

I also think the only way reproduce this, is by adding a wrapper for the command. It helps to make the command do something that can be verified after aborting, maybe something like this (untested):

#!/bin/sh
#
# Assume a 1GB block-device, write unencrypted data at 500MB offset.
#

# run normal crypsetup, it will be quick and exits cleanly
cryptsetup.real $@

# run dd in a loop, small I/O with sync to make it slower
# only runs the loop if /tmp/testing exists
while [ -e /tmp/testing ]
do
    # set correct device and verify parameters
    dd if=/dev/urandom of=/dev/rbd... bs=1 count=100m offset=500m
done

Make sure to name the script cryptsetup and place is in the location where cryptsetup normally lives. The original cryptsetup executable should be renamed to cryptsetup.real so that it still can be executed.

nixpanic · 2024-10-22T14:40:10Z

Hey @black-dragon74 , I'll mark this as a draft for now, otherwise I keep checking it for updates. Please move it back as ready for review once you have tested it. Thanks!

Madhu-1

LGTM

nixpanic

Renaming the type and functions make it better, please take care of that.

The suggestion with the interface and moving to a separate package are optional and can be done in a follow-up PR if you prefer.

internal/util/cryptsetup.go

Pull request has been modified.

Rakshith-R

LGTM, just need to start context before obtaining locks and use the same context for get/put calls to kms.

Rakshith-R · 2024-11-04T11:56:30Z

internal/rbd/encryption.go

+	// Create a new luks wrapper
+	timeoutCtx, cancel := context.WithTimeout(ctx, cryptsetup.ExecutionTimeout)
+	luks := cryptsetup.NewLUKSWrapper(timeoutCtx)
+	defer cancel()
+


This should be done before obtaining locks at line 481.
Pass the same context to get/put cryptopassphrase calls as well.

nixpanic · 2024-11-05T09:20:09Z

@Mergifyio rebase

This PR modifies the execCryptSetupCommand so that the process is killed in an event of lock timeout. Useful in cases where the volume lock is released but the command is still running. Signed-off-by: Niraj Yadav <[email protected]>

mergify · 2024-11-05T09:20:40Z

rebase

✅ Branch has been successfully rebased

Rakshits concern has been addressed

nixpanic · 2024-11-05T09:49:35Z

@Mergifyio queue

mergify · 2024-11-05T09:49:48Z

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 1c02e69

ceph-csi-bot · 2024-11-05T09:50:02Z

/test ci/centos/k8s-e2e-external-storage/1.31

ceph-csi-bot · 2024-11-05T09:50:03Z

/test ci/centos/mini-e2e-helm/k8s-1.31

ceph-csi-bot · 2024-11-05T09:50:03Z

/test ci/centos/k8s-e2e-external-storage/1.29

ceph-csi-bot · 2024-11-05T09:50:04Z

/test ci/centos/mini-e2e/k8s-1.31

ceph-csi-bot · 2024-11-05T09:50:04Z

/test ci/centos/mini-e2e-helm/k8s-1.29

ceph-csi-bot · 2024-11-05T09:50:05Z

/test ci/centos/k8s-e2e-external-storage/1.30

ceph-csi-bot · 2024-11-05T09:50:05Z

/test ci/centos/mini-e2e/k8s-1.29

ceph-csi-bot · 2024-11-05T09:50:05Z

/test ci/centos/mini-e2e-helm/k8s-1.30

ceph-csi-bot · 2024-11-05T09:50:07Z

/test ci/centos/mini-e2e/k8s-1.30

ceph-csi-bot · 2024-11-05T09:50:09Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2024-11-05T09:50:10Z

/test ci/centos/upgrade-tests-rbd

mergify bot added the component/rbd Issues related to RBD label Oct 17, 2024

black-dragon74 requested review from nixpanic, Madhu-1, iPraveenParihar and Rakshith-R October 17, 2024 12:00

nixpanic reviewed Oct 17, 2024

View reviewed changes

Madhu-1 reviewed Oct 18, 2024

View reviewed changes

nixpanic marked this pull request as draft October 22, 2024 14:40

black-dragon74 force-pushed the cryptsetup-add-timeout branch from a68d41f to 3802a3a Compare October 28, 2024 09:27

black-dragon74 marked this pull request as ready for review October 28, 2024 09:30

black-dragon74 force-pushed the cryptsetup-add-timeout branch 4 times, most recently from 943698e to 911d52f Compare October 28, 2024 11:42

black-dragon74 requested review from nixpanic and Madhu-1 October 29, 2024 11:27

Madhu-1 previously approved these changes Oct 30, 2024

View reviewed changes

nixpanic requested changes Nov 1, 2024

View reviewed changes

internal/util/cryptsetup.go Outdated Show resolved Hide resolved

internal/util/cryptsetup.go Outdated Show resolved Hide resolved

internal/util/cryptsetup.go Outdated Show resolved Hide resolved

black-dragon74 force-pushed the cryptsetup-add-timeout branch from 911d52f to 37c0f44 Compare November 4, 2024 07:21

black-dragon74 force-pushed the cryptsetup-add-timeout branch 2 times, most recently from f42438d to 22bcec3 Compare November 4, 2024 07:58

Rakshith-R previously requested changes Nov 4, 2024

View reviewed changes

black-dragon74 force-pushed the cryptsetup-add-timeout branch from 22bcec3 to 2ad0ad8 Compare November 4, 2024 14:13

nixpanic approved these changes Nov 5, 2024

View reviewed changes

nixpanic requested review from Rakshith-R and Madhu-1 November 5, 2024 09:20

rbd: Add timeout for cryptsetup commands

ddf7ec4

This PR modifies the execCryptSetupCommand so that the process is killed in an event of lock timeout. Useful in cases where the volume lock is released but the command is still running. Signed-off-by: Niraj Yadav <[email protected]>

nixpanic force-pushed the cryptsetup-add-timeout branch from 2ad0ad8 to ddf7ec4 Compare November 5, 2024 09:20

Madhu-1 approved these changes Nov 5, 2024

View reviewed changes

mergify bot added the ok-to-test Label to trigger E2E tests label Nov 5, 2024

ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Nov 5, 2024

mergify bot merged commit 1c02e69 into ceph:devel Nov 5, 2024
37 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rbd: Add timeout for cryptsetup commands #4912

rbd: Add timeout for cryptsetup commands #4912

black-dragon74 commented Oct 17, 2024 •

edited

Loading

nixpanic left a comment

Madhu-1 commented Oct 18, 2024

Madhu-1 Oct 18, 2024

black-dragon74 Oct 18, 2024

Madhu-1 Oct 18, 2024

black-dragon74 commented Oct 18, 2024

nixpanic commented Oct 18, 2024 •

edited

Loading

nixpanic commented Oct 22, 2024

Madhu-1 left a comment

nixpanic left a comment

Rakshith-R left a comment •

edited

Loading

Rakshith-R Nov 4, 2024

black-dragon74 Nov 4, 2024

nixpanic commented Nov 5, 2024

mergify bot commented Nov 5, 2024

nixpanic commented Nov 5, 2024

mergify bot commented Nov 5, 2024 •

edited

Loading

ceph-csi-bot commented Nov 5, 2024

ceph-csi-bot commented Nov 5, 2024

ceph-csi-bot commented Nov 5, 2024

ceph-csi-bot commented Nov 5, 2024

ceph-csi-bot commented Nov 5, 2024

ceph-csi-bot commented Nov 5, 2024

ceph-csi-bot commented Nov 5, 2024

ceph-csi-bot commented Nov 5, 2024

ceph-csi-bot commented Nov 5, 2024

ceph-csi-bot commented Nov 5, 2024

ceph-csi-bot commented Nov 5, 2024

rbd: Add timeout for cryptsetup commands #4912

rbd: Add timeout for cryptsetup commands #4912

Conversation

black-dragon74 commented Oct 17, 2024 • edited Loading

Describe what this PR does

How to Test

nixpanic left a comment

Choose a reason for hiding this comment

Madhu-1 commented Oct 18, 2024

Madhu-1 Oct 18, 2024

Choose a reason for hiding this comment

black-dragon74 Oct 18, 2024

Choose a reason for hiding this comment

Madhu-1 Oct 18, 2024

Choose a reason for hiding this comment

black-dragon74 commented Oct 18, 2024

nixpanic commented Oct 18, 2024 • edited Loading

nixpanic commented Oct 22, 2024

Madhu-1 left a comment

Choose a reason for hiding this comment

nixpanic left a comment

Choose a reason for hiding this comment

Rakshith-R left a comment • edited Loading

Choose a reason for hiding this comment

Rakshith-R Nov 4, 2024

Choose a reason for hiding this comment

black-dragon74 Nov 4, 2024

Choose a reason for hiding this comment

nixpanic commented Nov 5, 2024

mergify bot commented Nov 5, 2024

✅ Branch has been successfully rebased

nixpanic commented Nov 5, 2024

mergify bot commented Nov 5, 2024 • edited Loading

✅ The pull request has been merged automatically

ceph-csi-bot commented Nov 5, 2024

ceph-csi-bot commented Nov 5, 2024

ceph-csi-bot commented Nov 5, 2024

ceph-csi-bot commented Nov 5, 2024

ceph-csi-bot commented Nov 5, 2024

ceph-csi-bot commented Nov 5, 2024

ceph-csi-bot commented Nov 5, 2024

ceph-csi-bot commented Nov 5, 2024

ceph-csi-bot commented Nov 5, 2024

ceph-csi-bot commented Nov 5, 2024

ceph-csi-bot commented Nov 5, 2024

black-dragon74 commented Oct 17, 2024 •

edited

Loading

nixpanic commented Oct 18, 2024 •

edited

Loading

Rakshith-R left a comment •

edited

Loading

mergify bot commented Nov 5, 2024 •

edited

Loading