-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
Implementation of Batch fetch via feature toggle. #96
base: master
Are you sure you want to change the base?
Implementation of Batch fetch via feature toggle. #96
Conversation
// This is for sake of cleanliness vs getting 'fancy' with things. | ||
// We can always optimize later. | ||
return source | ||
.Where(sle=> !optionsSecretFilter(sle)) |
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.
By default, SecretFilter is set to always return true
, So assuming it is meant to be a positive filter, The !
operator here prevents those secrets from passing. I.e. on a default setup, this drops all secrets and return an empty list.
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 derped hard on this one, my bad. Fixing.
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.
Don't we all. No worries :]
...k.Extensions.Configuration.AWSSecretsManager/Internal/SecretsManagerConfigurationProvider.cs
Outdated
Show resolved
Hide resolved
return source | ||
.Where(sle=> !optionsSecretFilter(sle)) | ||
.Select((item, index) => new { item, index }) | ||
.GroupBy(x => x.index / chunkSize) |
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 think later versions of .net include a chunk method but not sure if it's compatible with the targeted frameworks.
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.
Yeah, NET 6.0 and up - https://learn.microsoft.com/en-us/dotnet/api/system.linq.enumerable.chunk?view=net-9.0
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.
Per this not being part of netstandard2.0
decided to leave as-is. Doubly so since currently lib is only published that way for now.
In future if desired, folks can do a #if NET6_0_OR_GREATER
dance for the more optimized call in separate PR.
Similar for multitargeting; I -thought- about it for a moment but decided better to keep changes small. (That said I may do it in separate PR if someone doesn't beat me to it)
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.
Ignoring the multi-targeting changes required, here it is with #if NET6_0_OR_GREATER
:
private static IEnumerable<SecretListEntry[]> ChunkList(IReadOnlyList<SecretListEntry> source,
Func<SecretListEntry, bool> optionsSecretFilter, int chunkSize)
{
#if NET6_0_OR_GREATER
return source.Where(optionsSecretFilter).Chunk(20);
#else
// This is for sake of cleanliness vs getting 'fancy' with things.
// We can always optimize later.
return source
.Where(optionsSecretFilter)
.Select(static (item, index) => (item, index))
.GroupBy(x => x.index / chunkSize)
.Select(static group => group.Select(static x => x.item).ToArray())
.ToList();
#endif
}
Addressed feedback, also while cleaning up I observed that the way I was looping on nexttoken wasn't checking for errors in all cases. I've switched it to a |
Thanks for the nice work. I'll check everything sometimes this week. I hope this is not blocking you. |
Appreciate the consideration. This is not blocking me so I'm not in a rush on this (but can't speak for @ransagy ). |
I'm going to use a NuGet package made from @to11mtm 's branch in our local private feed for now, until this lands in a release. So not technically blocked either, but would be nice to see it coming officially. |
(Will edit title if this is acceptable to see through to completion and I switch from draft)Proposal for resolving #95
Basically, via options toggle, we can let users opt-in to batch fetch requests. This can be especially helpful for use cases where lots of secrets are involved and refreshes are enabled to minimize API calls and improve overall load times.
I will note some of this implementation is naive/paranoid, AWS Batch fetches can sometimes have vexing behavior so I coded this POC defensively based on experiences with other services batch calls.
There's also opportunity for some code consolidation, but in my head this makes for easier diff.