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

fix(encrypt): close encrypted volume if it is opened #3140

Merged

Conversation

mantissahz
Copy link
Contributor

@mantissahz mantissahz commented Sep 5, 2024

Which issue(s) this PR fixes:

Issue # longhorn/longhorn#9385

What this PR does / why we need it:

In the normal process of attaching a volume via CSI, the encrypted volume should be closed or inactivated before Longhorn attempts to open it.

Special notes for your reviewer:

Additional documentation or context

Full regression test:
https://ci.longhorn.io/job/private/job/longhorn-tests-regression/7529
https://ci.longhorn.io/job/private/job/longhorn-tests-regression/7538/

csi/crypto/crypto.go Outdated Show resolved Hide resolved
csi/crypto/crypto.go Outdated Show resolved Hide resolved
csi/node_server.go Outdated Show resolved Hide resolved
csi/node_server.go Outdated Show resolved Hide resolved
@mantissahz mantissahz force-pushed the close-encrypted-volume-before-open-it branch from f8206c6 to c6c1511 Compare September 9, 2024 02:20
c3y1huang
c3y1huang previously approved these changes Sep 9, 2024
Copy link
Contributor

@c3y1huang c3y1huang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

csi/crypto/crypto.go Outdated Show resolved Hide resolved
csi/crypto/crypto.go Outdated Show resolved Hide resolved
csi/node_server.go Outdated Show resolved Hide resolved
csi/node_server.go Outdated Show resolved Hide resolved
csi/node_server.go Show resolved Hide resolved
@mantissahz mantissahz force-pushed the close-encrypted-volume-before-open-it branch 2 times, most recently from 8d9a273 to acade6b Compare September 9, 2024 05:58
Copy link
Member

@derekbit derekbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, LGTM

  1. Update the error messages
  2. QA test cases need to include
    • encrypted volume attachment and detachment
    • validate the case when path is (null)

csi/node_server.go Outdated Show resolved Hide resolved
csi/node_server.go Outdated Show resolved Hide resolved
In normal process of attaching a volume via CSI, the encrypted volume
should be in closed or inactivated state before Longhorn attempts to
open it.

ref: longhorn/longhorn 9385

Signed-off-by: James Lu <james.lu@suse.com>
@mantissahz mantissahz force-pushed the close-encrypted-volume-before-open-it branch from acade6b to 6a55644 Compare September 9, 2024 09:05
Copy link
Member

@derekbit derekbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mergify mergify bot merged commit 04f95d2 into longhorn:master Sep 9, 2024
7 of 8 checks passed
@mantissahz
Copy link
Contributor Author

@Mergifyio backport v1.7.x v1.6.x

Copy link

mergify bot commented Sep 9, 2024

backport v1.7.x v1.6.x

✅ Backports have been created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants