Prevent SQS from defaulting to localhost hostname

[alukach]

Commit cdbfe9a64e1884dc99fc9806a834f23cf8398a3b added tooling to retrieve the hostname from the SQS Transport. If no hostname is provided (for example, if you only configure an app with broker_transport = 'sqs'), the hostname is set to a default value from the Transport baseclass' default_connection_params property. This is problematic, as the default is 'localhost', which is likely not correct for an SQS queue and results in failed message retrieval. Interestingly, a Celery worker is still able to successfully start, retrieve the list of queues, and retrieve messages being that Celery provides the Transport a default url value of None.

This PR omits hostname from the SQS Transport's default_connection_params property, thereby letting Boto3 use its default logic if no hostname is manually provided to the SQS Transport.

[alukach]

Pinging the author of the first commit, @georgepsarakis.

[georgepsarakis]

@alukach thanks, perhaps I missed this case. Would you mind expanding the unit tests, probably here ?

[georgepsarakis]

Also, do you think this is actually breaking backwards compatibility?

[alukach]

@georgepsarakis Looking into this bug, I've realized that the true issue is that when the ConnectionPool generates new connections (by cloning the connection instance originally sent to the pool from celery.app.connection_for_read()), the NoneType value is being overwritten by the hostname value from SQS.Transport.default_connection_params. I think the test best exemplifies the core of the issue. This tests fails on master.

While this could technically break backwards functionality, I don't think this should be an issue as:

• I can't think of any reason why a user would want a boto3 connection to localhost
• Cloned connections maintaining the same hostname as its parent seems to be completely expected behaviour, hard to imagine people assuming otherwise

[georgepsarakis]

I was mostly asking if you think that this will cause an issue for anyone that uses SQS and upgrading to the upcoming release of 4.0.3. If yes, then perhaps we should include this fix as well.

[alukach]

@georgepsarakis Ah, I do think that this is an issue introduced by the commit referenced in the PR's description which would break anyone's system that doesn't explicitly define the SQS endpoint (which, I'd be surprised if anyone actually does specify the endpoint rather than simply specifying the region).

Admittedly, I'm really only looking at this from the perspective of code from #693, I would recommend against deploying any release that includes #693 and #715 without this PR.

[georgepsarakis]

@alukach thanks for your efforts! @thedrow @auvipy do you think we can merge just this fix for the 4.0.3 release?

[georgepsarakis]

By the way, @alukach if you happen to have any comments for this Pull Request, they will be more than welcome 😃 !

[auvipy]

a release note is also needed

[alukach]

@auvipy is that something I could help out with or are those done in-house? Would it be a new entry in the CHANGELOG?

[auvipy]

yes

[auvipy]

from now on we shouldn't merged any pr without release note [if there is any]

[alukach]

@auvipy The last entry in the Changelog is:

.. _version-4.0.2:

4.0.2

:release-date: 2016-12-15 03:31 P.M PST
:release-by: Ask Solem

• Now depends on :mod:amqp 2.1.4

  This new version takes advantage of TCP Keepalive settings on Linux,
  making it better at detecting closed connections, also in failover
  conditions.

• Redis: Priority was reversed so, e.g. priority 0 became priority 9.

.. _version-4.0.1:

I'm assuming you're suggesting adding an entry for 4.0.3. This PR is a bug fix for and issue raised on master. It feels strange to report both the creation and resolution of a bug within the same release (we may as well just pretend it never happened). There may be other features that will go into 4.0.3, however I think organizing and recording those changes is a bit out of scope for this PR.

[georgepsarakis]

@auvipy @alukach thanks 😄 !