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

[storage] Add Crypto middle end volume #208

Merged
merged 2 commits into from
Nov 15, 2022
Merged

Conversation

glimchb
Copy link
Member

@glimchb glimchb commented Nov 14, 2022

Signed-off-by: Boris Glimcher [email protected]

@glimchb glimchb requested a review from a team as a code owner November 14, 2022 20:48
@glimchb glimchb self-assigned this Nov 14, 2022
@glimchb glimchb added the Storage APIs or code related to storage area label Nov 14, 2022
@sandersms
Copy link
Contributor

Should the Crypto functionality be included in the middle end protobuf or should there be a separate common crypto capability that can be tied to the function (Storage, Security, Networking, etc.) that would create, delete, update, ... the capability?

Perhaps we can iterate on that aspect of the common functionality.

@glimchb
Copy link
Member Author

glimchb commented Nov 15, 2022

Should the Crypto functionality be included in the middle end protobuf or should there be a separate common crypto capability that can be tied to the function (Storage, Security, Networking, etc.) that would create, delete, update, ... the capability?

Perhaps we can iterate on that aspect of the common functionality.

d@re (data at rest encryption) is only applicable for storage

we can definitely iterate on this common once we have another example that uses the same, we can easily move it around to a common place

Copy link
Contributor

@sandersms sandersms left a comment

Choose a reason for hiding this comment

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

LGTM - Approved

@glimchb
Copy link
Member Author

glimchb commented Nov 15, 2022

@glimchb glimchb merged commit ffe4aad into opiproject:main Nov 15, 2022
@glimchb glimchb deleted the crypto branch November 15, 2022 23:40
Copy link
Contributor

@jainvipin jainvipin left a comment

Choose a reason for hiding this comment

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

some comments @glimchb

service MiddleendService {
rpc CreateCrypto (CreateCryptoRequest) returns (Crypto) {
option (google.api.http) = {
post: "/v1/volumes"
Copy link
Contributor

Choose a reason for hiding this comment

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

the flat API structure is evident here v1/volumes is very generic, if we preceed it with v1/middleend/volumes that would decouple it from the backend volumes and overlap.

Copy link
Member Author

Choose a reason for hiding this comment

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

agree, part of #188

common.v1.ObjectKey volume_id = 2;

// Key to be used for encryption
bytes key = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a shared secret or a private key?

Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to provide secret for initial vector or do we believe use of IV is perhaps a stale thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

no idea, this is just first draft, let's discuss


message CryptoStatsResponse {
common.v1.ObjectKey crypto_id = 1;
string stats = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment about stats as in other PRs

}

message CreateCryptoRequest {
Crypto volume = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

minor, why do we call the field volume it should be called crypto? e.g. what if we have other service applied on the volume, would that also be called volume in the create/update requests?

Copy link
Member Author

Choose a reason for hiding this comment

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

will rename, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Storage APIs or code related to storage area
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants