fix(core-amqp): name the audience and caller of a CBS open failure - #7344
Draft
Johnathan W (j7nw4r) wants to merge 9 commits into
Draft
fix(core-amqp): name the audience and caller of a CBS open failure#7344Johnathan W (j7nw4r) wants to merge 9 commits into
Johnathan W (j7nw4r) wants to merge 9 commits into
Conversation
A CBS open that fails tells the caller "Could not open Claims Based Security object." That sentence does not say which result came back, which audience failed, or which function asked for the token. The retry warnings have the same problem: every attempt writes the same line, so the reader cannot tell attempt 1 from attempt 4. Add seven tests that pin the text these paths must write. - connection_tests.cpp gets TestCbsOpenFailureText. The two tests call the pure text builders in the new src/amqp/private/cbs_open_failure.hpp and make sure the text names the result, the audience, the caller, the token type, and the expiry year. They run on every platform. - claim_based_security_tests.cpp gets an end to end test. It opens a message sender against a port with no listener and makes sure the exception and the warning line name the audience and ConnectionImpl::AuthenticateAudience, and that the token appears in no line at any level. CbsOpenNoListener now opens the object a second time and makes sure that branch writes a line of its own. - retry_operation_test.cpp gets three tests. They make sure each retry warning names the 1-based attempt, and that the exhausted line names both the attempt reached and the maximum. The tests are red. The two connection tests do not compile, because cbs_open_failure.hpp does not exist yet. The three retry tests fail on the digit assertions, because all four warning lines are the same today. The two CBS tests need a non-Mac platform with uAMQP to run. Each test file carries its own LogCapture helper, because the two test binaries have separate include roots. The helper removes the log listener before the connection goes away, so the polling thread cannot call a listener that is gone.
A CBS open that failed threw "Could not open Claims Based Security object." That sentence does not say which result came back, which audience failed, or which function asked for the token. Add src/amqp/private/cbs_open_failure.hpp. It builds the exception text and the log text from the result, the audience, and the caller. The log text adds the token type and the token expiry. Neither function takes the token, so no failure text can hold a secret. The expiry helper catches the throw from Azure::DateTime::ToString, because a date outside the range of year 1 to year 9999 would replace the exception that the caller must throw. PutTokenForAudience now writes a warning and throws the same sentence. The thrown type stays std::runtime_error. The uAMQP management client and the claims based security object now name the management node, the open status, and the text of the exception that ended an open. The message sender open failure moves from the Error level to the Warning level, because that call reports the failure to its caller. The class documentation no longer links the OASIS CBS draft. That draft defines a set-token request and no status code, so it describes a different protocol from the one this class implements. The Service Bus protocol guide stays as the only link. The change adds and rewrites log lines. It changes no thrown type, no return value, and no state transition.
Every attempt wrote the same warning, so a reader could not tell attempt 1 from attempt 4. The line that reports the exhausted retries named neither the attempt that the operation reached nor the limit. The two warning lines and the exhausted line now name the attempt. The counter is 0-based, so each rendered number adds 1. The exhausted line also names MaxRetries. The levels and the retry counts do not change.
|
Azure Pipelines: Successfully started running 3 pipeline(s). 7 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
The cspell gate in the release artifact job rejected two tokens in this file. The comment used a British spelling, and the assertion matched the fragment "xhaust" to survive a change of the leading letter. Match the whole word "exhausted" instead. The log line holds that word in lower case, so the assertion keeps its meaning without a spelling exception in the shared cspell configuration.
Contributor
There was a problem hiding this comment.
Pull request overview
Improves AMQP CBS and Event Hubs retry diagnostics.
Changes:
- Adds contextual CBS failure and management-client logging.
- Adds retry attempt numbers to Event Hubs logs.
- Adds tests, documentation, and changelog entries.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
sdk/eventhubs/azure-messaging-eventhubs/test/ut/retry_operation_test.cpp |
Tests retry log context. |
sdk/eventhubs/azure-messaging-eventhubs/src/retry_operation.cpp |
Logs retry attempt numbers. |
sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md |
Documents retry diagnostics. |
sdk/core/azure-core-amqp/test/ut/connection_tests.cpp |
Tests CBS message formatting. |
sdk/core/azure-core-amqp/test/ut/claim_based_security_tests.cpp |
Tests CBS failure logs. |
sdk/core/azure-core-amqp/src/impl/uamqp/amqp/private/management_impl.hpp |
Formats management status names. |
sdk/core/azure-core-amqp/src/impl/uamqp/amqp/management.cpp |
Enriches management failure logs. |
sdk/core/azure-core-amqp/src/impl/uamqp/amqp/claim_based_security.cpp |
Reports CBS open failures. |
sdk/core/azure-core-amqp/src/amqp/private/cbs_open_failure.hpp |
Adds CBS failure formatters. |
sdk/core/azure-core-amqp/src/amqp/connection.cpp |
Logs and throws contextual CBS failures. |
sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/claims_based_security.hpp |
Corrects CBS documentation. |
sdk/core/azure-core-amqp/CHANGELOG.md |
Documents AMQP diagnostics. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (3)
sdk/core/azure-core-amqp/src/impl/uamqp/amqp/claim_based_security.cpp:63
m_managementremains non-null even when the firstm_management->OpenreturnedInvalid,Error, orCancelled, so this branch can follow a failed first open; the extendedCbsOpenNoListenertest takes exactly that path. Calling the object “already open” is therefore misleading. Report only thatOpenwas called again.
Log::Stream(Logger::Level::Warning)
<< "ClaimsBasedSecurityImpl::Open: this claims based security object is already open. "
"Open was called a second time on the same object.";
sdk/core/azure-core-amqp/src/amqp/private/cbs_open_failure.hpp:60
- The new invalid-expiry fallback is not exercised: the added formatter test uses only a valid 2035 date. Add an out-of-range
DateTimecase and assert that the log containstoken expires: unknown, so a future change cannot letToString()replace the intended CBS-open failure.
try
{
return expiresOn.ToString();
}
catch (std::exception const&)
sdk/core/azure-core-amqp/src/impl/uamqp/amqp/management.cpp:114
- This return path still omits
m_options.ManagementNodeName, so whenm_messageSender->Openreturns an error it exits before the later node-aware diagnostic and the reader still cannot distinguish$cbsfrom$management. This contradicts the PR's stated goal of naming the management node on failed opens.
Log::Stream(Logger::Level::Warning)
<< "ManagementClientImpl::Open: Message sender open failed: " << senderResult;
…ilure The failure text named the result, the audience, and the caller, but a reader still could not tell a client fault from a service fault. Both look the same when the only evidence is the word "Error". Add the connection instance number, the host, the connection state, and the time that the open took. An open that ends in about no time, on a connection that is already in the End state, never reached the service. A repeated instance number across failures shows that the client did not build a new connection. Make the uAMQP connection state atomic and add an accessor. The polling thread writes that state while the caller thread now reads it. Give the Rust connection the same accessor, so the shared throw site needs no backend guard.
The message sender wrote its link detach line only when the AMQP trace option was on, and that option is off by default. A producer therefore got no record of the condition that the service sent when it detached the link, and the next send reported a stale cached error instead. The message receiver already writes this line at the Warning level for every detach, so the sender now matches it and names the node.
… a link uAMQP reads the error out of a close performative and offers it through one subscription. The connection never took that subscription, so the reason the service gave reached no log, and a client saw only the failures that came after it. The connection now subscribes and writes the condition and the description at the Warning level. A detach that arrives while a link is opening was also silent, because both link detach handlers test the open flag before they log. The open then failed with a generic error. Each handler now logs before that test and names the open state. The message sender no longer needs the AMQP trace option for this line, which matches the message receiver. The management client leaked an open message sender when the message receiver failed to open. Two handlers returned a status without a close, and the outer handler that does the close never ran for them, because they swallowed the exception. A message sender that stays open stops the process in its own destructor.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PutTokenForAudiencenow names theCbsOpenResult, failed audience, and caller for a claims based security open failure. The caller identifies the token request path.The old message gave no cause:
Motivation
Two Warning lines already include exception text: the send path in
retry_operation.cppand the token-refresh path inconnection.cpp.PutTokenForAudiencecheckedCbsOpenResult, but discarded the result, audience, token type, and expiry. Updating its message changes both lines without editing them.Changes
sdk/core/azure-core-amqp/src/amqp/private/cbs_open_failure.hppwith two inline formatters. The formatters do not accept the token, so failure text cannot contain a secret.PutTokenForAudiencelogs a Warning and throws a message with the result, audience, and caller. The message keeps the original leading sentence, so customer output remains recognizable.std::runtime_erroras the thrown type.RetryOperation::Executecatches this type, but notstd::exception. A different type stops the retry.Azure::DateTime::ToStringthrows for a year outside 0001 through 9999. The formatter catches this exception and writesunknown.catch (...)retains the exception text. Failed-open and state-change lines name the management node, which distinguishes$cbsfailures from$managementfailures.ClaimsBasedSecurityImpl::OpennamesManagementOpenStatusand uses different text for a secondOpenon the same object. Both cases previously mapped toCbsOpenResult::Error.RetryOperationadds a 1-based attempt number to both Warning lines. The exhausted line names the reached attempt andMaxRetries.set-tokenwithout a status code. The code sendsput-tokenand readsstatus-codeandstatus-description. The citation points only to the Service Bus AMQP protocol guide.Known gaps
The AMQP condition and description reach the log stream, but not the exception
what(). Passing them needs three private signature changes across both backends. The Rust backend provides noAmqpError. Applications that log only the exception still need the log stream.ManagementClientEvents::OnErrorpasses an emptyAmqpError, so the hook cannot report a cause on the Open and Closing state paths. A payload change changes behavior, so this PR excludes it.A permanent authorization failure, such as
amqp:unauthorized-access, retries until the retry budget expires. No layer carries a condition that can classify it. Three mechanisms cause this:std::runtime_errorinRetryOperation::Executeand callsShouldRetry(false, ...). The Boolean means "did the operation succeed," so the call makes no transience test.TranslateAuthenticationFailureto build anEventHubsExceptionwith an emptyErrorCondition.ShouldRebuildReceiverreturns true for an empty condition.IsErrorTransientalso returns true for an empty condition.A fix needs a type change or a classification change. This PR excludes both changes.
Test plan
Adds 7 tests.
azure-core-amqp-testspasses 153 of 153. The change builds and passes with the uAMQP and Rust backends. The uAMQP build uses-DDISABLE_RUST_IN_BUILD=ON.This command passes 25 of 25
RetryOperationTesttests:azure-messaging-eventhubs-test --gtest_filter='RetryOperationTest.*'All 5 available macOS tests failed against the unchanged source. Two had a missing-header compile error. Three failed assertions because each retry log line was byte-identical.
TestCbs.AuthenticationFailureNamesTheCbsOpenFailureand the extendedTestCbs.CbsOpenNoListeneruse the existing#if !defined(AZ_PLATFORM_MAC)guard. They do not run on macOS. A non-Mac configuration compiles both tests.The full eventhubs suite has 41 failures on the development machine. 40 are
_LIVEONLY_, and one needsEVENTHUB_CONSUMER_GROUP. All failures need live Azure credentials and do not relate to this change.clang-format-11, the CI version, makes no change.