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

rbd: add EncryptionLoad2 implementing rbd_encryption_load2 #1061

Merged
merged 5 commits into from
Feb 10, 2025

Conversation

phlogistonjohn
Copy link
Collaborator

Add a new Image method EncryptionLoad2 implementing rbd_encryption_load2.
This method adds the ability to have different encryption schemes across
parent images.

Fixes: #1059

Checklist

  • Added tests for features and functional changes
  • Public functions and types are documented
  • Standard formatting is applied to Go code
  • Is this a new API? Added a new file that begins with //go:build ceph_preview
  • Ran make api-update to record new APIs

New or infrequent contributors may want to review the go-ceph Developer's Guide including the section on how we track API Status and the API Stability Plan.

The go-ceph project uses mergify. View the mergify command guide for information on how to interact with mergify. Add a comment with @Mergifyio rebase to rebase your PR when github indicates that the PR is out of date with the base branch.

@phlogistonjohn phlogistonjohn added the API This PR includes a change to the public API of a go-ceph package label Jan 22, 2025
@phlogistonjohn
Copy link
Collaborator Author

I'm looking to add a 2nd test, one that actually tests the use of multiple encryption specs, but I haven't found any good examples yet. In the meantime, while it's still draft, I figured I'd get a test run out of what I have already.

@phlogistonjohn
Copy link
Collaborator Author

Additional test added.

@phlogistonjohn phlogistonjohn marked this pull request as ready for review January 23, 2025 19:21
Copy link
Collaborator

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

I've made few suggestions, all along the same pattern, to some of the length comparison assert statements. Somehow I feel like comparing the length of data read with the length of prior(fixed) data to make more sense than comparing it with the length of read buffer for img.ReadAt(). WDYT?

rbd/encryption_load2_test.go Outdated Show resolved Hide resolved
rbd/encryption_load2_test.go Outdated Show resolved Hide resolved
rbd/encryption_load2_test.go Outdated Show resolved Hide resolved
rbd/encryption_test.go Show resolved Hide resolved
rbd/encryption_test.go Show resolved Hide resolved
rbd/encryption_load2_test.go Show resolved Hide resolved
docs/api-status.json Outdated Show resolved Hide resolved
rbd/encryption_load2.go Outdated Show resolved Hide resolved
@phlogistonjohn
Copy link
Collaborator Author

There must be something else wrong with this change as other old tests are now panicking a bunch, I filed #1063 just to see if this was something pre-existing but those all passed without so much as a poke needed (earlier this week some of the dnf mirrors were being flaky). Unfortunately, I thought this would be a quick and easy thing to add, but I guess not.

@phlogistonjohn phlogistonjohn marked this pull request as draft January 24, 2025 18:47
docs/api-status.json Outdated Show resolved Hide resolved
rbd/encryption_opt_luks.go Show resolved Hide resolved
Copy link
Collaborator

@ansiwen ansiwen left a comment

Choose a reason for hiding this comment

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

Here my first review. I will make a seconds pass though. Not sure there is more.

rbd/encryption_opt_luks.go Show resolved Hide resolved
rbd/encryption.go Show resolved Hide resolved
rbd/encryption_load2.go Outdated Show resolved Hide resolved
entrypoint.sh Show resolved Hide resolved
@phlogistonjohn phlogistonjohn force-pushed the jjm-rbd-enc-load2-1059 branch 2 times, most recently from e8c883e to bdac7dc Compare February 7, 2025 15:55
@phlogistonjohn phlogistonjohn marked this pull request as ready for review February 7, 2025 20:10
@phlogistonjohn
Copy link
Collaborator Author

PTA(nother)L @ansiwen @anoopcs9

@phlogistonjohn
Copy link
Collaborator Author

@Mergifyio rebase

Copy link

mergify bot commented Feb 9, 2025

rebase

✅ Branch has been successfully rebased

anoopcs9
anoopcs9 previously approved these changes Feb 10, 2025
Copy link
Collaborator

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

lgtm, see below for a small suggestion.

rbd/encryption_opt_luks_test.go Show resolved Hide resolved
Fix a couple of comments to make them a bit more accurate. They were
probably true at one point in the development of the feature but
now referred to functions that don't exist.

Signed-off-by: John Mulligan <[email protected]>
Restructure the existing rbd encryption load test function so that it
makes use of defer and uses subtests to divide things up more clearly.
I needed to make this change in preparation for adding a binding for
rbd_encryption_load2 and a similar test - but I couldn't make sense of
the existing test in it's more monolithic form.

Signed-off-by: John Mulligan <[email protected]>
Add a new Image method EncryptionLoad2 implementing rbd_encryption_load2.
This method adds the ability to have different encryption schemes across
parent images.

Signed-off-by: John Mulligan <[email protected]>
Fixes: ceph#1059
Add EncryptionOptionsLUKS type for opening, but not formatting, existing
LUKS images generically.

Signed-off-by: John Mulligan <[email protected]>
Increase the test timeout from 10m (Go default) to 15m. Adding the three
new tests for load 2 and the new generic LUKS type (mainly the longer
entryption load2 test) push us over the 10m mark by just about 1 minute
:-\

Signed-off-by: John Mulligan <[email protected]>
@mergify mergify bot dismissed anoopcs9’s stale review February 10, 2025 13:05

Pull request has been modified.

Copy link
Collaborator

@ansiwen ansiwen left a comment

Choose a reason for hiding this comment

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

LG(ish)TM, give you a chance to see my comments before merging,. Remove the do-not-merge when you read it and want to merge it as is.

rbd/encryption_opt_luks.go Show resolved Hide resolved
@mergify mergify bot merged commit 84919dd into ceph:master Feb 10, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API This PR includes a change to the public API of a go-ceph package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing rbd API components: function rbd_encryption_load2
4 participants