WIP: [cinder-csi-plugin] Add basic support for an encrypted storage class parameter
#2525
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
do-not-merge/release-note-label-needed
|
Hi @NotTheEvilOne. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/M
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
pkg/csi/cinder/controllerserver.go
Outdated
| encrypted := (req.GetParameters()["encrypted"] == "true") | ||
|
|
||
| if encrypted && volType == "" { | ||
| volType = cinderCSIDefaultEncryptedVolumeType |
There was a problem hiding this comment.
this seems assume "LUKS", at least my devstack doesn't have this volume type at all, should we expect input instead of doing Default to LUKS here?
# openstack volume type list
+--------------------------------------+-------------+-----------+
| ID | Name | Is Public |
+--------------------------------------+-------------+-----------+
| d9bb6d66-3173-4092-807d-9536c5e70f5d | lvmdriver-1 | True |
| cad86940-33e8-4b9e-bce4-f2e9d5f63c21 | __DEFAULT__ | True |
+--------------------------------------+-------------+-----------+
There was a problem hiding this comment.
I'm not sure which is the right way here. "LUKS" is documented as the default name but you are right, it's not guaranteed that it exist at all. In that case the API will raise an error which might be fine.
I can add another call to volumetypes.List() similar to cloud.GetVolumesByName() as searching by name is not supported by the gophercloud API. This approach would be more expensive than the built in check (as OpenStack uses the database internally to look up by name). As an advantage I can check for the encryption there before creating a volume and error out without the need to cleanup.
There was a problem hiding this comment.
There's full freedom on the name of the volume types. You shouldn't assume the encrypted one will be named "LUKS". I don't think this PR has the right approach to the problem. What's the difference in just providing the encrypted volume type as the type parameter instead?
Instead of relying on a name you could list all the volume types and their encryption information [1] (seems like gophercloud supports this [2]) and make decisions based on that. The problem is - not all clouds will support that and what to do if there are multiple encrypted volume types? Again - I don't understand why relying on setting type is not enough to satisfy this use case.
[1] https://docs.openstack.org/api-ref/block-storage/v3/#show-an-encryption-type
[2] https://github.com/gophercloud/gophercloud/blob/ea71744690516fe42b6dba075265587990fdfa09/openstack/blockstorage/v3/volumetypes/results.go#L248-L269
the failure is not related to the PR , not sure what's wrong here lead to OCCM report ImageInspectError |
|
/retest |
There was a problem hiding this comment.
Can you please remove unrelated changes from this commit? You can propose them in a separate PR, but adding newline removals here make reviewing harder.
pkg/csi/cinder/controllerserver.go
Outdated
| encrypted := (req.GetParameters()["encrypted"] == "true") | ||
|
|
||
| if encrypted && volType == "" { | ||
| volType = cinderCSIDefaultEncryptedVolumeType |
There was a problem hiding this comment.
There's full freedom on the name of the volume types. You shouldn't assume the encrypted one will be named "LUKS". I don't think this PR has the right approach to the problem. What's the difference in just providing the encrypted volume type as the type parameter instead?
Instead of relying on a name you could list all the volume types and their encryption information [1] (seems like gophercloud supports this [2]) and make decisions based on that. The problem is - not all clouds will support that and what to do if there are multiple encrypted volume types? Again - I don't understand why relying on setting type is not enough to satisfy this use case.
[1] https://docs.openstack.org/api-ref/block-storage/v3/#show-an-encryption-type
[2] https://github.com/gophercloud/gophercloud/blob/ea71744690516fe42b6dba075265587990fdfa09/openstack/blockstorage/v3/volumetypes/results.go#L248-L269
|
/retest |
|
The changes of this PR are marginal @dulek, that's true. In the issue linked I have added more details why I (we) suggested this change in the first place: Anything else we need to know?: This PR is more the ground work for the other PR intended to be requested later. I understand your objections as follow:
Does that sound more reasonable? |
I don't exactly buy it, the meaning of "encrypted" parameter will be non-deterministic. What if there are multiple encrypted types? For the user it will be non-deterministic which one will be used in that case. Can you elaborate how next steps would look like with BYOK? Is there some CSI spec explaining how provider-based encryption works? In general I think there are problems with the API proposed here. As a counterexample - a design that would work for me:
Approach proposed in this PR would make sense if there was something like default encrypted volume type, but there's none, it's all at admin discretion. |
pkg/csi/cinder/controllerserver.go
Outdated
| } else if encrypted && !vol.Encrypted { | ||
| klog.Errorf("Failed to CreateVolume with encryption enabled. Check volume type set.") | ||
|
|
||
| err := cloud.DeleteVolume(vol.ID) | ||
| if err != nil && !cpoerrors.IsNotFound(err) { | ||
| klog.Errorf("Failed to CreateVolume (cleanup): %v", err) | ||
| return nil, status.Errorf(codes.Internal, "CreateVolume with encryption enabled cleanup failed with error %v", err) | ||
| } | ||
|
|
||
| return nil, status.Errorf(codes.InvalidArgument, "Failed to CreateVolume with encryption enabled. Check volume type set.") |
There was a problem hiding this comment.
Even if we agree that this is a good idea, we should check the volume type up front, not "try" creating a volume of that volume type to check if it's encrypted.
The idea behind this PR (once changed to check for the encrypted volume type before creation) is to give the user the safety that the volume type he defines in the StorageClass really produced an volume that is encrypted. Right now for a k8s end-user (e.g. in a KaaS environment) it is a black box and as you describe is up to the "mercy" of the admin to configure things right.
There's no such default specification unfortunately. Idea is to if "encrypted" is "true" to check for a secret with the key based on "CSI Driver Secrets" [1]. If that key has been given it will be used, otherwise the default internally generated key.
|
NotTheEvilOne
force-pushed
the
pr/cinder-csi-encryption-parameter
branch
3 times, most recently
from
86b647f
to
26829f9
Compare
NotTheEvilOne
force-pushed
the
pr/cinder-csi-encryption-parameter
branch
from
26829f9
to
9606937
Compare
NotTheEvilOne
changed the title
encrypted storage class parameterencrypted storage class parameter
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
After further analysis and discussion with upstream and the author(s) image-encryption proposal in OpenStack we found out that we are currently unable to determine in advance if a volume originating from a given volume type will be encrypted or not. The topic will be brought to the next Cinder 2024.1 Virtual Midcycle 2. Only solution available at the moment would be the original implementation of validating the "encrypted" flag provided by the OpenStack API for |
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
A new solution to validate if a volume type encrypts it's content will most likely be brought to 2024.2 "Dalmation". Integration in Please let me know how you would like to continue with this PR:
|
There was a problem hiding this comment.
A new solution to validate if a volume type encrypts it's content will most likely be brought to 2024.2 "Dalmation". Integration in
gophercloudwill be needed to reuse it for this feature request.Please let me know how you would like to continue with this PR:
- I can either revert to the "create volume and check if it encrypted" approach for the currently supported feature set of OpenStack
- Close this PR and reopen it after a new approach is available
- Leave the PR as it is until then
- Or last but not least leave the code as it is requiring manual change of the
volume_extension:volume_type_encryption:get(admin_api) permission
I think I'd rather not introduce an API to check for volume encryption right now. Unfortunately the OpenStack's philosophy is that you're mostly at mercy of the admin and what they configured. I don't really see a clean way to do this properly right now.
I didn't knew that volume encryption API is admin-only, sorry about suggestions to use it then, we cannot really be requiring admin permissions in CPO code.
|
|
||
| for _, volType := range volTypes { | ||
| if n == volType.Name { | ||
| matchingVolType = &volType |
There was a problem hiding this comment.
We should probably fail if there are more than one VolumeType of a certain name, or we're non-deterministic.
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
What this PR does / why we need it:
This PR introduces a basic
encryptedparameter to request an volume to be encrypted in OpenStack. As we currently have no approach to provide custom keys (separate request) this PR does validating if new volumes for the given volume type are encrypted if the parameter istrueand cleanup (delete and error out) the volume otherwise.Which issue this PR fixes(if applicable):
Fixes #2524
Release note: