-
Notifications
You must be signed in to change notification settings - Fork 851
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
V4 updates for AWSSDK.Extensions.NETCore.Setup (Native AOT) #3353
Conversation
var clientTypeName = serviceInterfaceType.Namespace + "." + serviceInterfaceType.Name.Substring(1) + "Client"; | ||
var clientType = serviceInterfaceType.GetTypeInfo().Assembly.GetType(clientTypeName); | ||
#if NET8_0_OR_GREATER | ||
return T.CreateDefaultServiceClient(credentials, config) as AmazonServiceClient; |
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.
This is where the new static interface methods that are added to the generator are called. Interesting thing about static interface methods especially with being declared as abstract
is the type has to be known at compile time. That is what drove the change in this class to use generic T
instead of passing in the Type
.
var configTypeName = serviceInterfaceType.Namespace + "." + serviceInterfaceType.Name.Substring(1) + "Config"; | ||
var configType = serviceInterfaceType.GetTypeInfo().Assembly.GetType(configTypeName); | ||
#if NET8_0_OR_GREATER | ||
ClientConfig config = T.CreateDefaultClientConfig(); |
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 other new static interface method for creating the service client config.
{ | ||
if (property.GetMethod != null && property.SetMethod != null) |
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.
This was all of the blanket generic reflection code that used to be used to automatically use any properties on client configs. Below where the property.GetMethod.Invoke(defaultConfig, emptyArray);
was used to call the getter this often caused problems because the getters on the ClientConfig often have lazy evaluated logic that would get messed up here.
/// <param name="config"></param> | ||
/// <param name="configSection">The config section to extract AWS options from.</param> | ||
/// <returns>The AWSOptions containing the values set in configuration system.</returns> | ||
public static AWSOptions GetAWSOptions<TConfig>(this IConfiguration config, string configSection) where TConfig : ClientConfig, new() |
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 generic overloads passing in the type of service client config got removed because DefaultClientConfig
no longer extends from ClientConfig. The purpose of these overloads were for later parts in the library when constructing the service client to have the service client config specific be passed in here so that service specific settings could be set. This now accomplished differently using the ServiceSpecificSettings
property on DefaultClientConfig
and the ProcessServiceSpecificSettings
method above for using reflection to copy the values over.
@@ -0,0 +1,6 @@ | |||
<Project> |
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.
Used in the NuGet package for installing the JSON schema. I learned about this from the Aspire repository https://github.com/dotnet/aspire/blob/78609847e2759fd43bf13f2760ae30f39a08c932/src/Components/Common/Package.targets
@@ -0,0 +1,150 @@ | |||
{ |
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 JSON schema that drives intellisense in Visual Studio.
var clientConfig = client.Config as AmazonS3Config; | ||
Assert.NotNull(clientConfig); | ||
Assert.False(clientConfig.ForcePathStyle); | ||
Assert.True(clientConfig.ForcePathStyle); |
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 changed this to true
because the GetClientConfigSettingsTest.json
file says it should be true. The test was saying it was false because it was call the GetAWSOptions
option without the generic passing in the AmazonS3Config. I view that as a limitation of the old implementation as it makes more logical sense that if the user set the value it should be on the client config. With the new approach in this PR that removed the generic version the service specific settings are always set if they are set in appsettings.json and the service client config being created.
TLDR: The behavior got more accurate to the user's intent.
For .NET 8 LangVersion is set to 11 to allow using static interface methods. This is to allow | ||
the service interface factory method to exist for AWSSDK.Extensions.NETCore.Setup. | ||
--> | ||
<PropertyGroup Condition="'$(TargetFramework)' == 'net8.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.
Any reason not to use newer LangVersion for older TargetFrameworks?
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.
In general I prefer to keep the lang version as close as possible to what our lowest targeted version of .NET Framework was implemented with. We obviously make exceptions since we are targeting .NET Framework 4.7.2 which was implemented with C# 8. But keep the number down lowers the blast radius.
What ends up happening is somebody works in the .NET Standard solution and uses a new C# feature then the .NET Framework solution fails to compile because it can't support the new language feature because it requires runtime changes. For example in this PR the use of static interface methods fails to compile in .NET Framework due to required runtime changes.
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.
Ok, now it won't compile due to the wrong LangVersion in the same scenario :)
private void ProcessServiceSpecificSettings(ClientConfig clientConfig, IDictionary<string, string> serviceSettings) | ||
{ | ||
var singleArray = new object[1]; | ||
var properties = clientConfig.GetType().GetProperties(BindingFlags.Public | BindingFlags.Instance); |
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 pretty sure this line will produce AOT warnings once you try to AOT compile some project, as it is not possible to know statically which properties are present in ClientConfig's descendant classes.
Roslyn analyzers are not extensive, more warnings are possible during the AOT build, so I'd suggest to add a small AOT-compiled console app.
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 assume you could add something like
_ = typeof(AmazonS3Config).GetProperties();
to CreateDefaultClientConfig
to force to preserve all Config
properties, and then add UnconditionalSuppressMessage (knowing that properties are preserved).
Not sure if there's a significantly better way.
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 was surprised that the GetProperties
method didn't have any attributes for requiring DynamicallyAccessedMembers
or RequiresUnreferencedCode
but I'll double check in action. My intention was to add UnconditionalSuppressMessage
with defensive code and consider setting service specific settings a best effort in a trimmed environment. There is no way for this package to force the service specific configs not to be trimmed. Although since request pipeline in the service client is going to call of those settings at some point I don't see it likely they would be trimmed.
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.
@Dreamescaper you were right it did still trigger a warning. I have added the suppression as I mentioned before. I can confirm I can build and run a Native AOT with AWSSDK.Extensions.NETCore.Setup with no warnings.
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.
Although since request pipeline in the service client is going to call of those settings at some point I don't see it likely they would be trimmed.
Per my understanding, even if the properties themselves are not trimmed, the reflection metadata still might, therefore they won't be returned in GetProperties call.
E.g. this code prints "False", even though the property is crearly used:
var config = new AmazonS3Config();
config.UseAccelerateEndpoint = true;
_ = config.UseAccelerateEndpoint;
var props = GetProperties(config);
Console.WriteLine(props.Contains("UseAccelerateEndpoint"));
string[] GetProperties(ClientConfig config)
{
return config.GetType().GetProperties().Select(p => p.Name).ToArray();
}
There is no way for this package to force the service specific configs not to be trimmed.
No way for this package, but it is possible from other packages.
You can add the attribute similar to
[DynamicDependency(DynamicallyAccessedMemberTypes.PublicProperties, typeof(AmazonS3Config))]
to the CreateDefaultClientConfig
methods you're generating.
This way the compiler will know to preserve properties if this method is invoked.
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.
Awesome thanks! I didn't know about the DynamicDependencyAttribute
. Latest commit adds that to generator 7db32a3
…ditionalSuppressMessage when setting service specific config settings.
…formation to address the failure
…gs are not trimmed.
{ | ||
PerformGlobalConfig(logger, options); | ||
var credentials = CreateCredentials(logger, options); | ||
|
||
if (!string.IsNullOrEmpty(options?.SessionRoleArn)) | ||
{ | ||
credentials = new AssumeRoleAWSCredentials(credentials, options.SessionRoleArn, options.SessionName); | ||
credentials = new AssumeRoleAWSCredentials(credentials, _awsOptions.SessionRoleArn, _awsOptions.SessionName); |
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.
Why is this using _awsOptions
instead of options
? It checks that the options SessionRoleArn is not null or empty but then uses _awsOptions. Granted _awsOptions and options could be the same reference at this point but it looks possible that _awsOptions
can be null. Checking and usage should match.
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.
Fixed, this should have been options
.
|
||
// There is intertwined logic between ServiceURL, Region and DefaultConfigurationMode | ||
// in the SDK. They are handled at the start together to make it easier to debug SDK behavior. | ||
if (defaultConfig.ServiceURL != null) |
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.
Should this be a string.IsNullOrEmpty check as well like below?
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.
Done
config.ServiceURL = defaultConfig.ServiceURL; | ||
} | ||
// Setting RegionEndpoint only if ServiceURL was not set, because ServiceURL value will be lost otherwise | ||
if (options.Region != null && string.IsNullOrEmpty(defaultConfig.ServiceURL)) |
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.
This can be simplified with an else if (options.Region != null)
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.
Done
/// <returns></returns> | ||
[UnconditionalSuppressMessage("AssemblyLoadTrimming", "IL2026:RequiresUnreferencedCode", | ||
Justification = "This suppression is here to ignore the warnings caused by CognitoSync. All other service clients have been confirmed to be trim safe. " + | ||
"If service clients become unsafe for trimming other compiler warnings will ocurr forcing the behavor to be addressed. " + |
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.
ocurr
=> occur
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.
Fixed, that is one of those words I always mess up.
<dependency id="Microsoft.Extensions.Configuration.Abstractions" version="2.0.0" /> | ||
<dependency id="Microsoft.Extensions.DependencyInjection.Abstractions" version="2.0.0" /> | ||
<dependency id="Microsoft.Extensions.Logging.Abstractions" version="2.0.0" /> | ||
</group> | ||
|
||
</dependencies> | ||
<group targetFramework="netcoreapp3.1"> |
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.
Why did we need to add a section for each target framework if the dependencies are exactly the same?
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 nuget pack
command was giving warnings that we have multiple targets but only a single group. I didn't like it but you know it has been my goal to get our warnings to go away.
var config = CreateConfig(serviceInterfaceType, options); | ||
var client = CreateClient(serviceInterfaceType, credentials, config); | ||
var config = CreateConfig(options); | ||
var client = CreateClient(credentials, config); | ||
return client as IAmazonService; |
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.
Can we return client as T
instead of client as IAmazonService;
and update this method return type?
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.
If I used generic T
then AWSSDK.Extensions.NETCore.Setup would have to know the type which it can't because it doesn't have a dependency on all of our service packages.
@@ -18,10 +18,10 @@ | |||
|
|||
<ItemGroup> | |||
<ProjectReference Include="../../src/AWSSDK.Extensions.NETCore.Setup/AWSSDK.Extensions.NETCore.Setup.csproj" /> | |||
<ProjectReference Include="..\..\..\sdk\src\Services\S3\AWSSDK.S3.NetStandard.csproj" /> |
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.
nit: Can we use the same slash type in both references?
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.
Done
private AWSOptions _awsOptions; | ||
|
||
/// <summary> | ||
/// Constructs an instance of the ClientFactory | ||
/// </summary> | ||
/// <param name="type">The type object for the Amazon service client interface, for example IAmazonS3.</param> | ||
internal ClientFactory(Type type, AWSOptions awsOptions) | ||
internal ClientFactory(AWSOptions awsOptions) |
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.
param name="type"
this parameter was removed
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.
Fixed
@@ -79,18 +79,18 @@ internal object CreateServiceClient(IServiceProvider provider) | |||
/// </summary> | |||
/// <param name="provider">The dependency injection provider.</param> | |||
/// <returns>The AWS service client</returns> | |||
internal static IAmazonService CreateServiceClient(ILogger logger, Type serviceInterfaceType, AWSOptions options) | |||
internal IAmazonService CreateServiceClient(ILogger logger, AWSOptions options) |
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.
<param name="provider">
wasn't updated to reflect the parameters change
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.
Fixed
This PR updates AWSSDK.Extensions.NETCore.Setup to be compatible with V4 version of the SDK. It also makes the library Native AOT safe by removing a lot of the reflection that was used in the SDK.
Separate from ClientConfig
The
DefaultClientConfig
is used as a place holder for the client configs that user's can manually set or represent the values set inappsettings.json
. When a service client is created the service specific client config is created and the values fromDefaultClientConfig
are copied over to the new service client.In V3 the
DefaultClientConfig
extended from service clients config typeClientConfig
. This caused the following problems.ClientConfig
doesn't use nullable types so when copying over values we didn't if properties were actually set.ClientConfig
have logic behind that are not cheap to call. We have had several issues with the triggering things like EC2 instance metadata when accessing properties causing significant delay.ClientConfig
constructors have changed so that it only has a constructor that takes in aIDefaultConfigurationProvider
. There is no valid instance ofIDefaultConfigurationProvider
forDefaultClientConfig
.To address these issues as well as Native AOT issues
DefaultClientConfig
has been changed to no longer extendClientConfig
. The config properties were copied toDefaultClientConfig
but used nullable versions with no logic behind the properties. Now when the settings are copied over we can check the nullability and only set the property on the service client config if there is a value.This does add maintenance to the SDK that
DefaultClientConfig
needs to be updated every time a new setting is added. I think it is worth it to avoid the untested runtime issues we have run into in the past.Native AOT
The library used to do Assembly type loads to create instances of service clients including doing string manipulation from the service interface name to compute the service client type. There is no way to make that work correctly in Native AOT because no information is know at compile time.
This library introduces a new method for creating service clients by using C# 11 static interface methods. Note this is only works for .NET 8+ where we care about Native AOT. Older versions use the existing method. The new static interface are added as
abstract
methods onIAmazonService
with generator changes to implement theabstract
methods on each service client interface. These methods will create the config and service client. Here is an example of what is generated on the service client interface.This allows when the service interface is registered using
app.Services.AddAWSService<IAmazonS3>();
from AWSSDK.Extensions.NETCore.Setup it can use the interface to create the config and client without relying on reflection.V3 used reflection to copy settings from
DefaultClientConfig
to the service specific config. To get rid of the reflection the package has been updated to check each property onDefaultClientConfig
and call the appropriate setter on the service config. The exception to this is supporting service specific config settings like S3'sForcePathStyle
. That still uses reflection because AWSSDK.Extensions.NETCore.Setup doesn't have a dependency on all of the service packages and doesn't know about any of the service specific config settings to call.Intellisense for appsettings.json
Within the last year or so Visual Studio and possibly other IDEs have added support for intellisense when editing the
appsettings.json
file. The process is controlled by added aConfigurationSchema.json
JSON schema file and theAWSSDK.Extensions.NETCore.Setup.targets
MSBuild targets file that injects the schema.Other changes
AWSSDK.Extensions.NETCore.Setup
to use a project reference toAWSSDK.Core
. The package reference was a legacy issue when the repo used to swap out differentsnk
files.4.0.0.0-preview