Skip to content

[DoNotMerge]Added EncryptionIdentity to Encryption properties. #11602

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.

Sign up for GitHub

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

Closed
wants to merge 17 commits into from
Closed

[DoNotMerge]Added EncryptionIdentity to Encryption properties. #11602

Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
cec4983
Added EncryptionIdentity to Encryption properties.
dhanvia Nov 9, 2020
80ba422
Made suggested changes. Added examples for account creation.
dhanvia Dec 15, 2020
7867e10
Made changes to include UserAssignedIdentity in MSIType
dhanvia Dec 15, 2020
afc7da3
Linked Examples. Fixed minor syntax issues.
dhanvia Jan 6, 2021
58757e7
fixed type error in dictionary
dhanvia Jan 6, 2021
ef13ece
removed certain properties from responses.
dhanvia Jan 6, 2021
6c56fb4
Minor Syntax changes in examples.
dhanvia Jan 6, 2021
e9beee0
white space changes
dhanvia Jan 6, 2021
0ff50e9
Removed EncryptionIdentity properties for testing purposes
dhanvia Jan 6, 2021
192bec2
Added encryptionIdentities back.
dhanvia Jan 6, 2021
1e8ef3c
Added type to Encryption Identity
dhanvia Jan 6, 2021
14bb9ef
Added EncryptionIdentity in EncryptionProperties in the example
dhanvia Jan 14, 2021
cd9db0a
Fixed identityType
dhanvia Jan 30, 2021
9b4c776
IdentityType modeasstring set to true
dhanvia Jan 30, 2021
9f831c9
Added update example
dhanvia Jan 30, 2021
faa5744
Fixed ModelValidation Errors
dhanvia Feb 2, 2021
60cdf16
fix prettier check
Juliehzl Feb 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions ...cation/storage/resource-manager/Microsoft.Storage/preview/2020-08-01-preview/storage.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2132,6 +2132,16 @@
}
}
},
"EncryptionIdentity":{
Copy link
Member

@blueww blueww Feb 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

As we discussed in mail, as we don't want to expose this part in SDK, please remove it from swagger. (SDK is generated from swagger).
Please discussed with feature PM to deside which properties won't expose in swagger.

Copy link
Author

@dhanvia dhanvia Feb 2, 2021

Choose a reason for hiding this comment

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

@blueww The EncryptionIdentity property is necessary in order for the user to be able to create a Storage Account with CMKL at creation time. Will update this thread after hearing back from PM.

"description": "Encryption Identity properties when storage account is associated with a UserAssignedIdentity.",
Copy link
Contributor

@anthonykjose anthonykjose Nov 24, 2020

Choose a reason for hiding this comment

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

"Identity to be used for server-side encryption on the storage account."

"properties": {
"userAssignedIdentity":{
"type": "string",
"description": "ResourceID of the UserAssignedIdentity to be associated with the storage account.",
Copy link
Contributor

@anthonykjose anthonykjose Nov 24, 2020

Choose a reason for hiding this comment

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

"ResourceID of the UserAssignedIdentity to be associated with server-side encryption on the storage account."

"x-ms-client-name": "UserAssignedIdentity"
}
}
},
"Encryption": {
"properties": {
"services": {
Expand Down Expand Up @@ -2160,6 +2170,11 @@
"$ref": "#/definitions/KeyVaultProperties",
"x-ms-client-name": "KeyVaultProperties",
"description": "Properties provided by key vault."
},
"identity": {
"$ref": "#/definitions/EncryptionIdentity",
"x-ms-client-name": "Identity",
"description": "Encryption Identity properties when storage account is associated with a UserAssignedIdentity."
Copy link
Member

@blueww blueww Nov 10, 2020

Choose a reason for hiding this comment

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

Could user both assign a user identity here, and also assign a system identity?

Copy link
Author

@dhanvia dhanvia Nov 10, 2020

Choose a reason for hiding this comment

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

No. "EncryptionIdentity" property is only for UserAssignedIdentities.

Copy link
Member

@blueww blueww Nov 11, 2020

Choose a reason for hiding this comment

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

Then if user try to assign system identity to an storage account which already has user assigned identity, what will happen? vise version?
It seems we didn't block user to do that from rest API definition.

Copy link
Author

@dhanvia dhanvia Nov 11, 2020

Choose a reason for hiding this comment

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

I think the 'Identity' property you are referring to is the property on the storage account level which can be either 'SystemAssigned' and 'UserAssigned'. The changes in the current PR is not modifying that property.

The "EncryptionIdentity" here is referring to a new 'identity' property within the scope of the EncryptionProperties of the storage account. These properties only apply to UserAssignedIdentities.

Copy link
Member

@blueww blueww Nov 11, 2020

Choose a reason for hiding this comment

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

Thanks for the reply!
Is there any relationship of the 2 identities setting?
Is there any feature spec rest API spec for this change? After user set this identity, what will happen?

Copy link
Author

@dhanvia dhanvia Nov 24, 2020

Choose a reason for hiding this comment

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

Hi Sorry for the late reply. I got pulled into higher priority tasks for two weeks.

I will confirm the details with feature owner and reply here soon

Copy link
Contributor

@anthonykjose anthonykjose Nov 24, 2020

Choose a reason for hiding this comment

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

EncryptionIdentity allows you to specify the specific user assigned identity that needs to be used for encryption. When not applied, Storage defaults to attempting to use the SystemAssigned Identity.

}
},
"required": [
Expand Down