Azure servicebus transport has hidden dependency on azure-identity package

[Korijn]

The azure-servicebus transport depends on azure-identity package, even though it is not listed:

kombu/requirements/extras/azureservicebus.txt

Line 1 in d620132

azure-servicebus>=7.10.0

It looks like the code intends for it to be optional:

kombu/kombu/transport/azureservicebus.py

Lines 72 to 77 in d620132

	try:
	from azure.identity import (DefaultAzureCredential,
	ManagedIdentityCredential)
	except ImportError:
	DefaultAzureCredential = None
	ManagedIdentityCredential = None

But of course it crashes here when (effectively) calling isinstance(..., None)

kombu/kombu/transport/azureservicebus.py

Lines 140 to 146 in d620132

	def _try_parse_connection_string(self) -> None:
	self._namespace, self._credential = Transport.parse_uri(
	self.conninfo.hostname)
	if (isinstance(self._credential, DefaultAzureCredential) or
	isinstance(self._credential, ManagedIdentityCredential)):
	return None

File "/app/.venv/lib/python3.9/site-packages/kombu/transport/azureservicebus.py", line 144, in _try_parse_connection_string
    if (isinstance(self._credential, DefaultAzureCredential) or
TypeError: isinstance() arg 2 must be a type or tuple of types

This problem was introduced in #1641 #1801

[Nusnus]

This problem was introduced in #1641

@jasonwbarnett hey friend - can you check this out please?

[jesusgarmanz]

Hi! Seems like this is the same issue I found on #1947

[Nusnus]

@Korijn @jesusgarmanz any of you want to offer a PR to fix this issue? 🙂

[jasonwbarnett]

@Nusnus I got this.

[jasonwbarnett]

This break was introduced in #1801, not #1641

You know what. The original intent was to make this optional. In other words, don't force azure-identity down people's throats. They should only install it, optionally, if they require the use of either of the new features introduced in #1641

That said, I could update the code to throw an exception detailing what steps are necessary if they wish to use it and update the docs. I don't think adding a forced requirement makes sense here.

Thoughts?

[Korijn]

Yeah, that's what I said:

It looks like the code intends for it to be optional:

But it's implemented in a way that it isn't actually optional.

[jasonwbarnett]

@Korijn I would like input from @marnikow. He introduced the isinstance() check which introduced this bug. I don't know what the isinstance() check does here. I no longer have the job where I was using this code in production for over a year (code without @marnikow fixes). I need input from him so I can get more context.

[jasonwbarnett]

Actually, I just hacked this into place. This should work now.

[jasonwbarnett]

@Korijn can you update the issue description so that it accurately references #1801 as the PR which introduced the bug you've reported?

[Korijn]

@Korijn can you update the issue description so that it accurately references #1801 as the PR which introduced the bug you've reported?

Done!