-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat: Add support for specifying the message destination and AWS service client when publishing a specific message. #93
Conversation
@@ -22,11 +22,8 @@ public class EventBridgePublisherConfiguration : IMessagePublisherConfiguration | |||
/// Creates an instance of <see cref="EventBridgePublisherConfiguration"/>. | |||
/// </summary> | |||
/// <param name="eventBusName">The name or the ARN of the event bus where the message is published</param> | |||
public EventBridgePublisherConfiguration(string eventBusName) | |||
public EventBridgePublisherConfiguration(string? eventBusName) |
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.
Update the NDoc to explain what endpoint being null means and what they need to do about 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.
Expanded in 8428288
@@ -18,15 +18,15 @@ public interface IMessageBusBuilder | |||
/// </summary> | |||
/// <param name="queueUrl">The SQS queue URL to publish the message to.</param> | |||
/// <param name="messageTypeIdentifier">The language-agnostic message type identifier. If not specified, the .NET type will be used.</param> | |||
IMessageBusBuilder AddSQSPublisher<TMessage>(string queueUrl, string? messageTypeIdentifier = null); | |||
IMessageBusBuilder AddSQSPublisher<TMessage>(string? queueUrl, string? messageTypeIdentifier = 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.
Update the NDoc to explain what endpoint being null means and what they need to do about 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.
Expanded in 8428288
|
||
/// <summary> | ||
/// Adds an SNS Publisher to the framework which will handle publishing | ||
/// the defined message type to the specified SNS topic URL. | ||
/// </summary> | ||
/// <param name="topicUrl">The SNS topic URL to publish the message to.</param> | ||
/// <param name="messageTypeIdentifier">The language-agnostic message type identifier. If not specified, the .NET type will be used.</param> | ||
IMessageBusBuilder AddSNSPublisher<TMessage>(string topicUrl, string? messageTypeIdentifier = null); | ||
IMessageBusBuilder AddSNSPublisher<TMessage>(string? topicUrl, string? messageTypeIdentifier = 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.
Update the NDoc to explain what endpoint being null means and what they need to do about 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.
Expanded in 8428288
@@ -35,7 +35,7 @@ public interface IMessageBusBuilder | |||
/// <param name="eventBusName">The EventBridge event bus name or ARN where the message will be published.</param> | |||
/// <param name="messageTypeIdentifier">The language-agnostic message type identifier. If not specified, the .NET type will be used.</param> | |||
/// <param name="options">Contains additional properties that can be set while configuring an EventBridge publisher</param> | |||
IMessageBusBuilder AddEventBridgePublisher<TMessage>(string eventBusName, string? messageTypeIdentifier = null, EventBridgePublishOptions? options = null); | |||
IMessageBusBuilder AddEventBridgePublisher<TMessage>(string? eventBusName, string? messageTypeIdentifier = null, EventBridgePublishOptions? options = 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.
Update the NDoc to explain what endpoint being null means and what they need to do about 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.
Expanded in 8428288
_logger.LogDebug("Sending the message of type '{MessageType}' to EventBridge. Publisher Endpoint: {Endpoint}", typeof(T), publisherEndpoint); | ||
var request = CreatePutEventsRequest(publisherMapping, messageEnvelope.Source?.ToString(), messageBody, eventBridgeOptions); | ||
await _eventBridgeClient.PutEventsAsync(request, token); | ||
var client = _eventBridgeClient; |
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 a users is only using the OverrideClient
property then it seems possible there are not environment configuration that the awsClientProvider.GetServiceClient
will be able to resolve the creation of the service client. The AWSClientProvider
will throw a FailedToFindAWSServiceClientException
exception for a service client the user didn't even want.
I'm guessing the problem exists in all 3 publishers.
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.
Still working on this, but documenting current state. Since MessageBusBuilder
is still calling services.TryAddAWSService<Amazon.SQS.IAmazonSQS>()
, even for a destination-less publisher, I don't think we're actually triggering the FailedToFindAWSServiceClientException
With no [default]
credentials or region at all is actually an exception from down in NetCore.Setup, not the FailedToFindAWSServiceClientException
.
System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
---> Amazon.Runtime.AmazonClientException: No RegionEndpoint or ServiceURL configured
at Amazon.Runtime.ClientConfig.Validate()
at Amazon.Runtime.AmazonServiceClient..ctor(AWSCredentials credentials, ClientConfig config)
--- End of inner exception stack trace ---
at System.RuntimeMethodHandle.InvokeMethod(Object target, Span`1& arguments, Signature sig, Boolean constructor, Boolean wrapExceptions)
at System.Reflection.RuntimeConstructorInfo.Invoke(BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
at Amazon.Extensions.NETCore.Setup.ClientFactory.CreateClient(Type serviceInterfaceType, AWSCredentials credentials, ClientConfig config)
at Amazon.Extensions.NETCore.Setup.ClientFactory.CreateServiceClient(ILogger logger, Type serviceInterfaceType, AWSOptions options)
at Amazon.Extensions.NETCore.Setup.ClientFactory.CreateServiceClient(IServiceProvider provider)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSiteMain(ServiceCallSite callSite, TArgument argument)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitRootCache(ServiceCallSite callSite, RuntimeResolverContext context)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(ServiceCallSite callSite, TArgument argument)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.Resolve(ServiceCallSite callSite, ServiceProviderEngineScope scope)
at Microsoft.Extensions.DependencyInjection.ServiceProvider.CreateServiceAccessor(Type serviceType)
at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
at Microsoft.Extensions.DependencyInjection.ServiceProvider.GetService(Type serviceType, ServiceProviderEngineScope serviceProviderEngineScope)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngineScope.GetService(Type serviceType)
at AWS.Messaging.Configuration.AWSClientProvider.GetServiceClient[T]() in C:\dev\aws-dotnet-messaging\src\AWS.Messaging\Configuration\AWSClientProvider.cs:line 31
at AWS.Messaging.Publishers.SQS.SQSPublisher..ctor(IAWSClientProvider awsClientProvider, ILogger`1 logger, IMessageConfiguration messageConfiguration, IEnvelopeSerializer envelopeSerializer, ITelemetryFactory telemetryFactory) in C:\dev\aws-dotnet-messaging\src\AWS.Messaging\Publishers\SQS\SQSPublisher.cs:line 38
at System.RuntimeMethodHandle.InvokeMethod(Object target, Span`1& arguments, Signature sig, Boolean
With a region but no credentials then we get this from in the SDK
Amazon.Runtime.AmazonServiceException: Unable to get IAM security credentials from EC2 Instance Metadata Service.
at Amazon.Runtime.DefaultInstanceProfileAWSCredentials.FetchCredentials()
at Amazon.Runtime.DefaultInstanceProfileAWSCredentials.GetCredentials()
at Amazon.Runtime.DefaultInstanceProfileAWSCredentials.GetCredentialsAsync()
...
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.
Alright, took a stab at this in a55b5e5
The publishers will no longer acquire a service client from DI until the point of publishing if the override client is not set.
client = eventBridgeOptions.OverrideClient; | ||
|
||
// But still update the user agent to match the built-in client | ||
if (client is 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.
Unfortunately I don't think there is a safe way to add this callback because you need to add only if hasn't already been added to this instance. In your case if the same instance is used for OverrideClient
then the AWSServiceClient_BeforeServiceRequest
is added multiple times and called for each time it is added. This callback should generally only be added as part of service client creation not on a per operation situation.
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.
Removed the user agent handling after our internal discussion. We lose some future ability to filter these requests based on whether they're coming from the messaging framework vs. the underlying SDK, but we also want to be cautious around manipulating the user-provided clients.
…ice client when publishing a specific message.
ab59ff1
to
cf5e769
Compare
DetailType = publisherMapping.MessageTypeIdentifier, | ||
Detail = messageBody | ||
}; | ||
|
||
var putEventsRequest = new PutEventsRequest | ||
{ | ||
EndpointId = publisherConfiguration.EndpointID, | ||
// Give precendence to the endpoint ID if specified on the message-specific eventBridgeOptions, |
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: typo in "precendence"
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 in f638895
Issue #, if available: #90, DOTNET-7333
Description of changes: This adds support for configuring the destination and AWS client at the point of publishing. This may be useful for multi-tenant applications as described in #90 , where the destination of the message (and therefore the credentials required to publish it) aren't known until building the message.
For configuring the publisher without a destination in advance, I considered:
AddGenericSQSPublisher
, (or maybeDestinationless
?Unknown
?)AddSQSPublisher("WILL_OVERRIDE_LATER")
string queueURL
parameter tostring? queueURL
so allowingAddSQSPublisher(null)
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.