Skip to content
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

SQS transport ignores Queue Name Prefix in Connection String #1432

Closed
AbdoHassanUMG opened this issue Sep 25, 2018 · 14 comments
Closed

SQS transport ignores Queue Name Prefix in Connection String #1432

AbdoHassanUMG opened this issue Sep 25, 2018 · 14 comments
Assignees

Comments

@AbdoHassanUMG
Copy link
Contributor

Problem :
When using SQS transport and specifying Queue Name Prefix in Connection String
https://docs.particular.net/transports/sqs/configuration-options#queuenameprefix

It is ignored by Service Control.

@AbdoHassanUMG
Copy link
Contributor Author

I do have a propose fix. but don't have permissions to start a pull request

@mauroservienti
Copy link
Member

@AbdoHassanUMG thanks for raising #1435
@Particular/servicecontrol-maintainers can you take a look?

@danielmarbach
Copy link
Contributor

@AbdoHassanUMG When we released support for AmazonSQS in 3.x we deliberately went for what we called sensible defaults in the transport configuration. Supporting QueueNamePrefixes was scoped out. Right now it is possible to name the ServiceControl instances appropriately that matches the queue name prefix of the environment. Then this will be taking into account when creating all the queues.

We are more than happy to take your PR (thanks for that) but will probably only release this in a next minor or patch version.

@AbdoHassanUMG
Copy link
Contributor Author

@danielmarbach
I am aware of the possibility to overcoming this for SC own queues by naming the instance accordingly.
But when messages are routed back to worker endpoints queues , How will SC resolve the correct endpoint SQS "Physical" Queue Name if it doesn't know the which prefix to append ?

@danielmarbach
Copy link
Contributor

@AbdoHassanUMG the queue name prefix would only be applied to the endpoint name of ServiceControl and doesn't affect endpoints when routing back to those queues. The destination to where to route back to is determined by the information that is available on the error message. Thus when messages are moved to the error queue by a worker endpoint having a prefix the failed queue header will contain that prefix.

@AbdoHassanUMG
Copy link
Contributor Author

Hi @danielmarbach
I have tested again using 3.2.0 (with fix for #1433 ). Retires still don't work. However when applying the PR on top of 3.2.0 Retries are routed correctly to the consuming endpoint.

I debugged the source code and I can see only the logical endpoint name is available in the header. So SC has no way to reconstruct the full queue name including the prefix
Have a look @ ReturnToSender.cs
`
var destination = outgoingHeaders["ServiceControl.TargetEndpointAddress"];
Log.DebugFormat("{0}: Forwarding message to {1}", messageId, destination);
if (!outgoingHeaders.TryGetValue("ServiceControl.RetryTo", out var retryTo))
{
retryTo = destination;
outgoingHeaders.Remove("ServiceControl.TargetEndpointAddress");
}
else
{
Log.DebugFormat("{0}: Found ServiceControl.RetryTo header. Rerouting to {1}", messageId, retryTo);
}

        var transportOp = new TransportOperation(outgoingMessage, new UnicastAddressTag(retryTo));

        await sender.Dispatch(new TransportOperations(transportOp), message.TransportTransaction, message.Extensions)
            .ConfigureAwait(false);
     `

I get the error
142|Error|NServiceBus.Transports.SQS.MessageDispatcher|Error while sending message, with MessageId '74e8ba24-72cd-48f1-8d32-a96f00e2bbb7', to 'Sales'
Amazon.SQS.Model.QueueDoesNotExistException: The specified queue does not exist for this wsdl version. ---> Amazon.Runtime.Internal.HttpErrorResponseException: The remote server returned an error: (400) Bad Request. ---> System.Net.WebException: The remote server returned an error: (400) Bad Request.

When adding the prefix configs to SQSTransportCustomization as per the PR the message is routed successfully.

unfortunately, SQS Retires still now working with 3.2.0 .. though we are one error closer now. I appreciate if we can accelerate this as patch

@danielmarbach
Copy link
Contributor

Hi @AbdoHassanUMG

I was on vacation last week and could not reply. I'm currently looking into this. You'll hear from me soon

Daniel

@danielmarbach
Copy link
Contributor

@AbdoHassanUMG

Thanks for being patient with us (or me ;) )

I looked into this with more details and you are certainly right that the queue name prefix is not properly taken into account. The QueueNamePrefix is only applied by the transport, NServiceBus itself has no knowledge of the prefix. That is why every endpoint has to have the QueueNamePrefix applied at the moment. Even ServiceControl, therefore, would require the queue name prefix like you pointed out. I'm a bit on the fence though right know about how the queue name prefix is implemented. For example, the NServiceBus core assumes that when messages are failed the failed queue header is always the name of the endpoint on which the message failed. But since NServiceBus is not aware of the customizations done by the SQS transport the header will not contain the queue name prefix. This leads to the exception you are seeing. Same applies to the reply feature. The assumption of NServiceBus is that when a reply address is provided it can just use that reply address. This means, in theory, an endpoint replying to a message that came from a prefixed endpoint would not need to specify the queue name prefix. In reality, it does need to because only when the actual reply is done the queue name prefix is applied. This smells to me that we have some homework to do regarding the Queue name prefix feature before we move ahead with changes to ServiceControl. There is a workaround though that you can apply until we have discussed this further. All the endpoints that have the queue name prefix should add the following recoverability config

        var recoverability = endpointConfiguration.Recoverability();
        recoverability.Failed(c => c.HeaderCustomization(headers =>
        {
            var failedQueueHeader = headers["NServiceBus.FailedQ"];
            headers["NServiceBus.FailedQ"] = $"YOURPREFIX{failedQueueHeader}";
        }));

This should get you unstuck. You can add this code into an extension method.

Hope that helps

@danielmarbach
Copy link
Contributor

Another option would be to add redirects to that ServiceControl instance as described in https://docs.particular.net/servicepulse/redirect

@danielmarbach
Copy link
Contributor

@AbdoHassanUMG I raised an internal discussion issue and I'll keep you posted how we move forward with this.

@AbdoHassanUMG
Copy link
Contributor Author

Hi @danielmarbach it is no problem. I hope you enjoyed your time off :)
I have read your explanation. I thought it will be useful to share with you how we utilise the queue name prefixes.
we use the sqs prefixes to allow us to split our AWS Account and Region scope into multiple virtual isolated scopes.
All end points and service controls for each of our environments ( QA , Stages , prod , each developer's machine etc) are assigned an environment specific prefix. Doing so, the whole set live in their isolated universe and has no info about what happens in other prefixes.
In our use case , having the SQS prefix merely at the transport layer and keeping that physical queue details away from SC works perfectly. We don't need the Endpoints assigned to a certain prefix to exchange massages with a SC out of that prefix (scope).

@danielmarbach
Copy link
Contributor

Created #1452 which will go into 3.2.2. Closing this one for now. Thanks @AbdoHassanUMG for reporting it and for the fix. I'm pulling in your commit in the work that will target 3.2.2

@danielmarbach
Copy link
Contributor

Work has started at #1453

@danielmarbach
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants