[servicebus] Strip port and path from fully_qualified_namespace (#44034)#48062
[servicebus] Strip port and path from fully_qualified_namespace (#44034)#48062pranz1996 wants to merge 4 commits into
Conversation
…e#44034) strip_protocol_from_uri now reduces the namespace to its bare host, removing any port (e.g. :443) and trailing path in addition to the scheme. This accepts the https://<ns>.servicebus.windows.net:443/ form that Azure returns when provisioning a namespace, matching the .NET and JavaScript SDKs. Previously the retained port/path corrupted both the AMQP connection host and the CBS token audience, raising ServiceBusAuthenticationError. Also enables the previously-uncollected connection-string parser test class (renamed to Test* so pytest collects it), fixes 3 stale emulator assertions, and de-duplicates a shadowed test method.
|
Azure Pipelines: Successfully started running 1 pipeline(s). 9 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Fixes Service Bus namespace normalization for Azure-provided endpoints.
Changes:
- Removes schemes, ports, and paths from namespace URIs.
- Adds regression coverage and restores test collection.
- Updates release notes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
azure/servicebus/_common/utils.py |
Normalizes namespace URIs. |
tests/test_connection_string_parser.py |
Restores collection and adds regression tests. |
CHANGELOG.md |
Documents the fix. |
…ure#44034) Address PR review: strip_protocol_from_uri truncated bracketed IPv6 hosts (e.g. [fe80::1]) at the first colon inside the address. Port stripping now only applies after the closing ']' for bracketed literals, matching the AMQP transport's to_host_port handling. Added IPv6 regression cases.
|
Azure Pipelines: Successfully started running 1 pipeline(s). 9 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
SwayGom
left a comment
SwayGom
left a comment
There was a problem hiding this comment.
The core fix for #44034 is sound, and the latest IPv6 handling plus the test-collection cleanup look good. I found one blocking regression, though.
strip_protocol_from_uri now removes the port from every input. For an emulator connection string such as Endpoint=sb://localhost:5673;...;UseDevelopmentEmulator=true, _parse_conn_str preserves localhost:5673 and sets use_tls=False, but the client constructor then normalizes fully_qualified_namespace to localhost. PyAMQP consequently builds amqp://localhost and falls back to port 5672 instead of the requested 5673. There is no alternate connection-string field later in the sync or async path that restores the port; direct sender/receiver factories are affected as well.
Could we keep the bare namespace/token-audience normalization separate from the socket endpoint (or otherwise preserve the emulator port), and add sync and async regression coverage for a non-default emulator port? All current checks are green, but they only exercise portless/default-port emulator endpoints, so they do not catch this behavior change.
…namespace (Azure#44034) Address PR review: the Azure#44034 port strip dropped the development emulator's non-default port (e.g. localhost:5673 -> localhost), causing pyamqp to fall back to 5672. strip_protocol_from_uri now takes a strip_port keyword; the four client/base-handler constructors pass strip_port=use_tls so the emulator (use_tls=False) keeps its port while real namespaces are still reduced to a bare host. Added sync and async regression tests for a non-default emulator port.
…amespace-44034 # Conflicts: # sdk/servicebus/azure-servicebus/CHANGELOG.md
|
Thanks for the review @SwayGom Good catch — fixed. The port is now only stripped for real (TLS) namespaces; the development emulator keeps its non-default port, so it connects on the right port again. Added sync and async regression tests covering a non-default emulator port. |
Description
Fixes #44034.
ServiceBusClient(sync + async) and the base handler normalize the caller'sfully_qualified_namespacethroughstrip_protocol_from_uri. Previously that helper stripped only the scheme (http://,sb://), leaving any port and trailing path in place. When a caller passes the endpoint that Azure returns when provisioning a namespace —https://<namespace>.servicebus.windows.net:443/— the:443/survived normalization and corrupted both consumers of the value:create_connection(host=...)), and_auth_uri = sb://<host>), which the service validates.The result was
ServiceBusAuthenticationError(amqp:client-error), even though the bare-host form worked. The .NET and JavaScript SDKs already normalize this format; Python was the outlier (confirmed by @eerhardt and @sebastienros on the issue).Fix
strip_protocol_from_urinow reduces the input to its bare host — removing scheme, port, and path — so the value is valid as both the socket target and the token audience. The change only removes characters that are invalid in a hostname; every already-clean input (bare host, scheme-only, sovereign clouds, connection strings) is byte-identical to before.Verification
test_strip_protocol_from_uri_normalizes_to_bare_host(fails on the old code, passes on the new): covers scheme × port × trailing-slash, including the exacthttps://<ns>:443/form.DefaultAzureCredential: the:443/form failed before the fix (ServiceBusAuthenticationError) and connects + receives after; the bare-host path is unchanged.Additional test-infra fixes (surfaced while adding the test)
The
test_connection_string_parser.pytest class was never collected by pytest (its name didn't match the defaultTest*pattern and its base is not aunittest.TestCase), so all 14 of its tests silently never ran. This PR:ServiceBusConnectionStringParserTests→TestServiceBusConnectionStringParserso pytest collects it,endpointassertions intest_sb_parse_emulator_string(the parser has appended a trailing/since [SB] combine conn str parser logic in base handler and _common #16464, consistent with every sibling test), andResult: the file went from 0 collected to 16 passing.
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines