-
Notifications
You must be signed in to change notification settings - Fork 469
Add support for host specialization with unencrypted payload #11302
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
base: dev
Are you sure you want to change the base?
Changes from 12 commits
a621e27
4c83a23
8a42a5b
33166aa
495d03f
ad59764
f5f10c4
8f4fdb1
b3f2e5f
da0a14b
971fb73
fded652
8458d8c
d92d0db
b4156fe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -135,12 +135,17 @@ private StartupContext GetStartupContextOrNull() | |
/// Decrypt and deserialize the specified context, and apply values from it to the | ||
/// startup cache context. | ||
/// </summary> | ||
/// <param name="encryptedContext">The encrypted assignment context.</param> | ||
manikantanallagatla marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// <returns>The decrypted assignment context</returns> | ||
public virtual HostAssignmentContext SetContext(EncryptedHostAssignmentContext encryptedContext) | ||
/// <param name="hostAssignmentRequest">The Host assignment request.</param> | ||
/// <returns>The decrypted assignment context.</returns> | ||
manikantanallagatla marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
public virtual HostAssignmentContext SetContext(HostAssignmentRequest hostAssignmentRequest) | ||
{ | ||
string decryptedContext = EncryptionHelper.Decrypt(encryptedContext.EncryptedContext, environment: _environment); | ||
var hostAssignmentContext = JsonConvert.DeserializeObject<HostAssignmentContext>(decryptedContext); | ||
var hostAssignmentContext = hostAssignmentRequest.AssignmentContext; | ||
|
||
if (!string.IsNullOrEmpty(hostAssignmentRequest.EncryptedContext)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if you shouldn't add a clause hostAssignmentContext == null to this if, making it clear that only one of these should ever be specified. As opposed to the precedence handling you have here, where both can be specified, but encrypted wins. I realize in the controller method you do validation, but there's another path where this method is called, where the request is pulled from storage. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure how the storage path works. But shall we move the handling that only one needs to be specified here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we are sure storage account path only takes encrypted payload, we can have method overloading for SetContext method. One which takes encrypted payload and other that takes unencrypted payload There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The storage path where context is pulled from storage MUST be encrypted since data must be encrypted at rest. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is a bit besides the point, but is this the customer's storage account? The encrypted at rest requirement here would be the storage account itself satisfying that. All data in a storage account is already "encrypted at rest": https://learn.microsoft.com/en-us/azure/storage/common/storage-service-encryption. The only time we, the Functions Host team, would have an "encrypted at rest" requirement is if we were offering some storage solution which did not already have encrypted-at-rest features. But back to the main point: I agree that if we expect storage scenario to always have client-side encryption (which is what this is), then we should assert that as necessary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Storage is only used in CV1 on ACI path. We are not touching that path. So, storage wise, no change with this PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think no changes are needed for storage and current validations in StartupContextProvider.cs seems fine |
||
{ | ||
string decryptedContext = EncryptionHelper.Decrypt(hostAssignmentRequest.EncryptedContext, environment: _environment); | ||
hostAssignmentContext = JsonConvert.DeserializeObject<HostAssignmentContext>(decryptedContext); | ||
manikantanallagatla marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
// Don't update StartupContext for warmup requests | ||
if (!hostAssignmentContext.IsWarmupRequest) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above TryGetStartContextOrNullAsync virtual call has a different implementation on Legion and Atlas. This same hosted service code is running on each SKU. Just want to confirm that there isn't any breaking format change here - the persisted assignment context is always encrypted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried for legion changes by giving encrypted and unencrypted payload for controller and it is working fine. Means encrypted path of controller which is existing is working fine.
Any way to test this storage path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just pointing out how the system behaves. Likely things are OK, since up to this point only encrypted was supported, and your new deserialization supports both old and new formats. I'm just this out so you can think about this and ensure no breaks. Bala knows the context storage logic the best so you might have him sign off on this as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Will check with Bala
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Storage one is only used in CV1 on ACI path(Atlas). We are not changing anything wrt encryption payload. So, storage path should not be affected by this repo. They should continue the encrypted payload and no changes needed.
For legion path, technically it should work as we are storing the volume in https://msazure.visualstudio.com/One/_git/AAPT-Antares-Functions-Docker?path=/images/flexconsumption/components/appserver/cv2/pkg/instance/instance.go&version=GBdev&line=76&lineEnd=77&lineStartColumn=1&lineEndColumn=1&lineStyle=plain&_a=contents
I will check legion for both encrypted and unencrypted path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have tested both paths and they are working fine.