diff --git a/sdk/core/azure-core-amqp/CHANGELOG.md b/sdk/core/azure-core-amqp/CHANGELOG.md index a6101b6625..4677b49289 100644 --- a/sdk/core/azure-core-amqp/CHANGELOG.md +++ b/sdk/core/azure-core-amqp/CHANGELOG.md @@ -36,6 +36,8 @@ - A claims based security open that fails now throws `CbsOpenFailedException`, which carries the `CbsOpenResult`. The three failures need different handling: `Error` reached the transport and may be retried, while `Cancelled` is the caller's own cancellation or deadline and `Invalid` is a state error. The result was previously readable only by matching the message text, so a reword would have changed caller behavior with no compiler error. The type derives from `std::runtime_error` and carries the same message, so existing handlers keep working. The Rust backend reports every open failure by throwing rather than by returning a result, so those throws are classified at the shared call site and carry the same type. - The uAMQP management client now closes the message sender when the message receiver fails to open. Two handlers returned a status without that close, and a message sender that stays open stops the process in its own destructor. - The uAMQP management client now names the management node and the open status in the lines that it writes when an open fails, and it keeps the text of the exception that ended the open. The message sender open failure moved from the Error level to the Warning level, because that call reports the failure to its caller. +- A claims based security open that fails now carries the reason that the layer below reported. `CbsOpenResult::Error` covers every transport, TLS and link failure, so a reader holding only the result could not separate a refused socket from a rejected attach. The management client wrote that reason to the log and then dropped it, because `ManagementClientImpl::Open` reports a failure as a status and the exception that named the cause was destroyed in the handler. The reason now travels with the status and reaches both the warning and the `CbsOpenFailedException` message, so a caller that logs the exception and has no log listener can still tell what failed. It names which of the two links failed, because the sender and the receiver fail for different causes. The reason is empty when the layer below gave none, and the sentence then reads exactly as it did before. It never holds the token. +- The claims based security object now keeps the AMQP error that the service sent. `ClaimsBasedSecurityImpl::OnError` receives the condition, the description, and the info map, which is the richest statement the service makes about a refused claim, and it only wrote them to the log. They are now added to the reason that the open failure carries. The capture takes a lock of its own, because that callback runs on the polling thread while the management client holds its open lock. ### Other Changes diff --git a/sdk/core/azure-core-amqp/src/amqp/connection.cpp b/sdk/core/azure-core-amqp/src/amqp/connection.cpp index 4f2f4be85d..cc2f569803 100644 --- a/sdk/core/azure-core-amqp/src/amqp/connection.cpp +++ b/sdk/core/azure-core-amqp/src/amqp/connection.cpp @@ -152,6 +152,10 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { auto claimsBasedSecurity = std::make_shared(session); auto const openStart = std::chrono::steady_clock::now(); CbsOpenResult cbsOpenStatus{CbsOpenResult::Invalid}; + // The reason the layer below reported. `CbsOpenResult::Error` covers every transport, TLS + // and link failure, so the status alone does not say what went wrong. Issue: a caller that + // saw only the status could not separate a refused socket from a rejected handshake. + std::string openFailureDetail; try { cbsOpenStatus = claimsBasedSecurity->Open(context); @@ -169,6 +173,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { // contract on both backends. A cancelled context is the one case a caller must not retry, // and it is the only distinction available at this point. cbsOpenStatus = context.IsCancelled() ? CbsOpenResult::Cancelled : CbsOpenResult::Error; + openFailureDetail = ex.what(); Log::Stream(Logger::Level::Warning) << "The claims based security open threw: " << ex.what(); } @@ -177,6 +182,10 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { auto const elapsed = std::chrono::duration_cast( std::chrono::steady_clock::now() - openStart); auto const connection = session->GetConnection(); + if (openFailureDetail.empty()) + { + openFailureDetail = claimsBasedSecurity->GetOpenFailureDetail(); + } Log::Stream(Logger::Level::Warning) << FormatCbsOpenFailureLog( cbsOpenStatus, audienceUrl, @@ -184,9 +193,11 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { expiresOn, caller, connection ? connection->GetDiagnosticSummary() : std::string{}, - elapsed); + elapsed, + openFailureDetail); throw CbsOpenFailedException( - cbsOpenStatus, DescribeCbsOpenFailure(cbsOpenStatus, audienceUrl, caller)); + cbsOpenStatus, + DescribeCbsOpenFailure(cbsOpenStatus, audienceUrl, caller, openFailureDetail)); } try diff --git a/sdk/core/azure-core-amqp/src/amqp/private/cbs_open_failure.hpp b/sdk/core/azure-core-amqp/src/amqp/private/cbs_open_failure.hpp index 0e08ec8399..930352c810 100644 --- a/sdk/core/azure-core-amqp/src/amqp/private/cbs_open_failure.hpp +++ b/sdk/core/azure-core-amqp/src/amqp/private/cbs_open_failure.hpp @@ -66,14 +66,30 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { // The sentence that the caller reads in the exception. Neither this function // nor the one below takes the token, so no failure text can hold a secret. + // + // `detail` is the reason the layer below reported. `CbsOpenResult::Error` covers every + // transport, TLS and link failure, so without the reason a reader cannot tell a refused socket + // from a rejected handshake. It is empty when that layer produced no reason, and the sentence + // then reads exactly as it did before. inline std::string DescribeCbsOpenFailure( CbsOpenResult result, std::string const& audienceUrl, - CbsOpenCaller caller) + CbsOpenCaller caller, + std::string const& detail = {}) { std::stringstream ss; ss << "Could not open Claims Based Security object. Result: " << result - << ", audience: " << audienceUrl << ", caller: " << CbsOpenCallerName(caller) << "."; + << ", audience: " << audienceUrl << ", caller: " << CbsOpenCallerName(caller); + if (!detail.empty()) + { + ss << ", reason: " << detail; + } + // The reason comes from a layer below and often ends in its own full stop. A second one + // reads as a typo in the customer's log. + if (detail.empty() || detail.back() != '.') + { + ss << "."; + } return ss.str(); } @@ -88,10 +104,11 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { Azure::DateTime const& expiresOn, CbsOpenCaller caller, std::string const& connectionSummary, - std::chrono::milliseconds elapsed) + std::chrono::milliseconds elapsed, + std::string const& detail = {}) { std::stringstream ss; - ss << DescribeCbsOpenFailure(result, audienceUrl, caller) + ss << DescribeCbsOpenFailure(result, audienceUrl, caller, detail) << " Token type: " << CbsTokenTypeName(tokenType) << ", token expires: " << FormatTokenExpiry(expiresOn) << ". Connection: " << (connectionSummary.empty() ? "unknown" : connectionSummary) diff --git a/sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/claim_based_security.cpp b/sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/claim_based_security.cpp index 5321deff96..01c5a306a2 100644 --- a/sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/claim_based_security.cpp +++ b/sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/claim_based_security.cpp @@ -54,6 +54,10 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { return CbsOpenResult::Ok; } } + + // This backend reports every open failure by throwing and returns Ok otherwise, so the shared + // call site reads the reason from the exception and never asks for one here. + std::string ClaimsBasedSecurityImpl::GetOpenFailureDetail() const { return {}; } void ClaimsBasedSecurityImpl::Close(Context const& context) { Common::_detail::CallContext callContext( diff --git a/sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/private/claims_based_security_impl.hpp b/sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/private/claims_based_security_impl.hpp index 5afb88a9ef..80b1c1133b 100644 --- a/sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/private/claims_based_security_impl.hpp +++ b/sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/private/claims_based_security_impl.hpp @@ -38,6 +38,10 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { ClaimsBasedSecurityImpl& operator=(ClaimsBasedSecurityImpl&&) noexcept = delete; _azure_NODISCARD CbsOpenResult Open(Context const& context); + + /** @brief The reason the last `Open` failed, or an empty string when it did not fail. */ + std::string GetOpenFailureDetail() const; + void Close(Context const& context); _azure_NODISCARD std::tuple PutToken( CbsTokenType type, diff --git a/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/claim_based_security.cpp b/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/claim_based_security.cpp index 0a07bfcb0a..93b61cacf6 100644 --- a/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/claim_based_security.cpp +++ b/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/claim_based_security.cpp @@ -38,9 +38,11 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { auto rv{m_management->Open(context)}; if (rv != ManagementOpenStatus::Ok) { + auto const detail = m_management->GetOpenFailureDetail(); Log::Stream(Logger::Level::Warning) << "ClaimsBasedSecurityImpl::Open: the $cbs management client did not open. Status: " - << ManagementOpenStatusName(rv) << "."; + << ManagementOpenStatusName(rv) << "." + << (detail.empty() ? std::string{} : " Reason: " + detail + "."); } switch (rv) { @@ -67,6 +69,39 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { void ClaimsBasedSecurityImpl::Close(Context const& context) { m_management->Close(context); } + // The management client holds the reason that its own layer produced. The AMQP error that the + // service sent is richer, because it names the condition, the description, and the info map, so + // it is added when one arrived. Either part may be absent. + std::string ClaimsBasedSecurityImpl::GetOpenFailureDetail() const + { + std::string detail{m_management ? m_management->GetOpenFailureDetail() : std::string{}}; + + Models::_internal::AmqpError lastError; + { + std::lock_guard lock(m_errorLock); + lastError = m_lastError; + } + if (lastError) + { + std::stringstream ss; + if (!detail.empty()) + { + ss << detail << "; "; + } + ss << "the service reported condition: " << lastError.Condition.ToString() + << ", description: " << lastError.Description; + // The info map carries the fields that make a condition actionable, such as the + // network-host and port of a redirect. It is usually empty, so it is only added when the + // service sent one. + if (!lastError.Info.empty()) + { + ss << ", info: " << lastError.Info; + } + return ss.str(); + } + return detail; + } + std::tuple ClaimsBasedSecurityImpl::PutToken( CbsTokenType tokenType, std::string const& audience, @@ -183,6 +218,10 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { void ClaimsBasedSecurityImpl::OnError(Models::_internal::AmqpError const& error) { Log::Stream(Logger::Level::Warning) << "AMQP Error processing ClaimsBasedSecurity: " << error; + // This is the only place the service's own condition and description reach this object. A + // caller that reads the exception and nothing else would otherwise never see them. + std::lock_guard lock(m_errorLock); + m_lastError = error; } }}}} // namespace Azure::Core::Amqp::_detail diff --git a/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/management.cpp b/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/management.cpp index 5934703b23..60acff9dee 100644 --- a/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/management.cpp +++ b/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/management.cpp @@ -90,6 +90,12 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { } } + std::string ManagementClientImpl::GetOpenFailureDetail() const + { + std::lock_guard lock(m_openCloseLock); + return m_openFailureDetail; + } + _internal::ManagementOpenStatus ManagementClientImpl::Open(Context const& context) { std::unique_lock lock(m_openCloseLock); @@ -98,6 +104,10 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { throw std::runtime_error("Management object is already open."); } + // A retry reuses this object, so a reason left by an earlier attempt must not be read as the + // reason for this one. + m_openFailureDetail.clear(); + try { /** Authentication needs to happen *before* the links are created. @@ -139,6 +149,10 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { auto senderResult{m_messageSender->Open(false, context)}; if (senderResult) { + std::stringstream detail; + detail << "the message sender for node '" << m_options.ManagementNodeName + << "' did not open: " << senderResult; + m_openFailureDetail = detail.str(); Log::Stream(Logger::Level::Warning) << "ManagementClientImpl::Open: Message sender open failed. Node: " << m_options.ManagementNodeName << ". Error: " << senderResult << "."; @@ -153,6 +167,10 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { // stays open stops the process in its own destructor, so close it here. catch (Azure::Core::OperationCancelledException const& e) { + // m_messageSenderOpen is set only after the sender open returned, and the receiver open + // is the next statement, so it names which of the two threw. + m_openFailureDetail = std::string("the message ") + + (m_messageSenderOpen ? "receiver" : "sender") + " open was cancelled: " + e.what(); Log::Stream(Logger::Level::Warning) << "Operation cancelled opening message sender and receiver." << e.what(); CloseSenderAndReceiverAfterFailedOpen(); @@ -160,6 +178,10 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { } catch (std::runtime_error const& e) { + // This is the reason a reader needs. It names the transport, TLS or link failure that + // made the open fail, and the status alone cannot carry it. + m_openFailureDetail = std::string("the message ") + + (m_messageSenderOpen ? "receiver" : "sender") + " open threw: " + e.what(); Log::Stream(Logger::Level::Warning) << "Exception thrown opening message sender and receiver." << e.what(); CloseSenderAndReceiverAfterFailedOpen(); @@ -174,9 +196,21 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { _internal::ManagementOpenStatus rv = std::get<0>(*result); if (rv != _internal::ManagementOpenStatus::Ok) { + // The handler that completed the queue knows which link failed and what state it + // entered. Fall back to the status only when it gave nothing. + auto queuedDetail = std::get<1>(*result); + if (queuedDetail.empty()) + { + std::stringstream detail; + detail << "the open completed with status " << ManagementOpenStatusName(rv) + << " for node '" << m_options.ManagementNodeName << "'"; + queuedDetail = detail.str(); + } + m_openFailureDetail = queuedDetail; Log::Stream(Logger::Level::Warning) << "Management operation failed to open. Node: " << m_options.ManagementNodeName - << ". Status: " << ManagementOpenStatusName(rv) << "."; + << ". Status: " << ManagementOpenStatusName(rv) << ". Reason: " << queuedDetail + << "."; m_messageSender->Close(context); m_messageSenderOpen = false; m_messageReceiver->Close(context); @@ -191,6 +225,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { // If result is null, then it means that the context was cancelled. Close the things we opened // earlier (if any) and return the error. + m_openFailureDetail = "the caller's context was cancelled while the open was in flight"; m_messageSender->Close({}); m_messageSenderOpen = false; m_messageReceiver->Close({}); @@ -199,8 +234,12 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { } catch (...) { + // This handler rethrows, so the caller reads the reason from the exception itself. The + // reason is recorded anyway so both paths out of Open leave it set. + auto const exceptionText = CurrentExceptionText(); + m_openFailureDetail = "the management open threw: " + exceptionText; Log::Stream(Logger::Level::Warning) - << "Exception thrown during management open. " << CurrentExceptionText(); + << "Exception thrown during management open. " << exceptionText; // If an exception is thrown, ensure that the message sender and receiver are closed. if (m_messageSenderOpen) { @@ -446,7 +485,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { if (m_messageReceiverOpen) { SetState(ManagementState::Open); - m_openCompleteQueue.CompleteOperation(_internal::ManagementOpenStatus::Ok); + m_openCompleteQueue.CompleteOperation(_internal::ManagementOpenStatus::Ok, {}); } break; // If the message sender is transitioning to an error or state other than open, @@ -454,14 +493,19 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { default: case _internal::MessageSenderState::Idle: case _internal::MessageSenderState::Closing: - case _internal::MessageSenderState::Error: + case _internal::MessageSenderState::Error: { Log::Stream(Logger::Level::Warning) << "Message Sender Changed State to " << newState << " while management client is opening" << ". Node: " << m_options.ManagementNodeName << "."; SetState(ManagementState::Closing); - m_openCompleteQueue.CompleteOperation(_internal::ManagementOpenStatus::Error); + std::stringstream detail; + detail << "the message sender for node '" << m_options.ManagementNodeName + << "' moved to " << newState << " while the management client was opening"; + m_openCompleteQueue.CompleteOperation( + _internal::ManagementOpenStatus::Error, detail.str()); break; + } } break; case ManagementState::Open: @@ -571,7 +615,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { if (m_messageSenderOpen) { SetState(ManagementState::Open); - m_openCompleteQueue.CompleteOperation(_internal::ManagementOpenStatus::Ok); + m_openCompleteQueue.CompleteOperation(_internal::ManagementOpenStatus::Ok, {}); } break; // If the message receiver is transitioning to an error or state other than open, @@ -579,14 +623,19 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { default: case _internal::MessageReceiverState::Idle: case _internal::MessageReceiverState::Closing: - case _internal::MessageReceiverState::Error: + case _internal::MessageReceiverState::Error: { Log::Stream(Logger::Level::Warning) << "Message Receiver Changed State to " << newState << " while management client is opening" << ". Node: " << m_options.ManagementNodeName << "."; SetState(ManagementState::Closing); - m_openCompleteQueue.CompleteOperation(_internal::ManagementOpenStatus::Error); + std::stringstream detail; + detail << "the message receiver for node '" << m_options.ManagementNodeName + << "' moved to " << newState << " while the management client was opening"; + m_openCompleteQueue.CompleteOperation( + _internal::ManagementOpenStatus::Error, detail.str()); break; + } } break; case ManagementState::Open: diff --git a/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/private/claims_based_security_impl.hpp b/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/private/claims_based_security_impl.hpp index 1d5b41440e..25417447dd 100644 --- a/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/private/claims_based_security_impl.hpp +++ b/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/private/claims_based_security_impl.hpp @@ -8,6 +8,9 @@ #include "azure/core/amqp/internal/management.hpp" #include + +#include +#include struct CBS_INSTANCE_TAG; namespace Azure { namespace Core { namespace Amqp { namespace _detail { @@ -34,6 +37,10 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { ClaimsBasedSecurityImpl& operator=(ClaimsBasedSecurityImpl&&) noexcept = delete; _azure_NODISCARD CbsOpenResult Open(Context const& context); + + /** @brief The reason the last `Open` failed, or an empty string when it did not fail. */ + std::string GetOpenFailureDetail() const; + void Close(Context const& context); _azure_NODISCARD std::tuple PutToken( CbsTokenType type, @@ -45,6 +52,13 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { private: std::shared_ptr<_detail::SessionImpl> m_session; std::shared_ptr<_detail::ManagementClientImpl> m_management; + + // OnError runs on the polling thread while ManagementClientImpl::Open holds its own open + // lock across WaitForResult. This capture therefore uses a lock of its own; taking the + // management open lock here would block the polling thread for the whole open. + mutable std::mutex m_errorLock; + Models::_internal::AmqpError m_lastError; + // Inherited via ManagementClientEvents void OnError(Models::_internal::AmqpError const& error) override; }; diff --git a/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/private/management_impl.hpp b/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/private/management_impl.hpp index aa21a021b2..a9bdc44de3 100644 --- a/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/private/management_impl.hpp +++ b/sdk/core/azure-core-amqp/src/impl/uamqp/amqp/private/management_impl.hpp @@ -74,6 +74,16 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { */ _internal::ManagementOpenStatus Open(Context const& context = {}); + /** + * @brief The reason the last `Open` failed, or an empty string when it did not fail. + * + * `Open` reports a failure as a `ManagementOpenStatus`, and every transport, TLS and link + * failure collapses into `Error`. The reason the layer below gave was written to the log and + * then dropped, so a caller holding only the status could not say what went wrong. This keeps + * it for the caller that builds the exception. + */ + std::string GetOpenFailureDetail() const; + /** * @brief Close the management instance. */ @@ -99,12 +109,17 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { std::shared_ptr m_messageSender; std::shared_ptr m_messageReceiver; ManagementState m_state = ManagementState::Idle; - std::mutex m_openCloseLock; + mutable std::mutex m_openCloseLock; bool m_isOpen{false}; bool m_messageSenderOpen{false}; bool m_messageReceiverOpen{false}; - Azure::Core::Amqp::Common::_internal::AsyncOperationQueue<_internal::ManagementOpenStatus> - m_openCompleteQueue; + // The status alone cannot say which link failed or what state it entered, and the handlers + // that complete this queue run on the polling thread while Open holds m_openCloseLock. The + // reason therefore travels with the status rather than through a member the handler would + // have to lock. + Azure::Core::Amqp::Common::_internal:: + AsyncOperationQueue<_internal::ManagementOpenStatus, std::string> + m_openCompleteQueue; bool m_sendCompleted{false}; @@ -123,6 +138,9 @@ namespace Azure { namespace Core { namespace Amqp { namespace _detail { std::string m_managementEntityPath; Azure::Core::Credentials::AccessToken m_accessToken; + // Written by Open under m_openCloseLock, read by GetOpenFailureDetail under the same lock. + std::string m_openFailureDetail; + using ManagementOperationQueue = Azure::Core::Amqp::Common::_internal::AsyncOperationQueue< _internal::ManagementOperationStatus, std::uint32_t, diff --git a/sdk/core/azure-core-amqp/test/ut/claim_based_security_tests.cpp b/sdk/core/azure-core-amqp/test/ut/claim_based_security_tests.cpp index 1cf43c5a2e..1aae14c03c 100644 --- a/sdk/core/azure-core-amqp/test/ut/claim_based_security_tests.cpp +++ b/sdk/core/azure-core-amqp/test/ut/claim_based_security_tests.cpp @@ -379,6 +379,15 @@ namespace Azure { namespace Core { namespace Amqp { namespace Tests { = dynamic_cast(&e); ASSERT_NE(nullptr, typed); EXPECT_EQ(Azure::Core::Amqp::_detail::CbsOpenResult::Error, typed->Result); + + // The result alone collapses every transport, TLS and link failure into one value, so + // the sentence must also carry the reason the layer below gave. A caller that logs the + // exception and has no log listener has nothing else to read. + EXPECT_NE(std::string::npos, what.find("reason:")) << what; + // The reason must name which link failed, not "sender or receiver". + EXPECT_NE(std::string::npos, what.find("the message sender")) << what; + // A generic state error must never be the whole story the caller gets. + EXPECT_EQ(std::string::npos, what.find("Message Sender entered the Error State.")) << what; } EXPECT_TRUE(caught); diff --git a/sdk/core/azure-core-amqp/test/ut/connection_tests.cpp b/sdk/core/azure-core-amqp/test/ut/connection_tests.cpp index 1a6026d658..fbda497d2f 100644 --- a/sdk/core/azure-core-amqp/test/ut/connection_tests.cpp +++ b/sdk/core/azure-core-amqp/test/ut/connection_tests.cpp @@ -536,6 +536,55 @@ namespace Azure { namespace Core { namespace Amqp { namespace Tests { } } + // `CbsOpenResult::Error` covers every transport, TLS and link failure, so a reader holding only + // the result cannot say what went wrong. The reason the layer below gave must reach the + // sentence the caller logs. + TEST_F(TestCbsOpenFailureText, TheFailureTextCarriesTheReasonFromTheLayerBelow) + { + using Azure::Core::Amqp::_detail::CbsOpenCaller; + using Azure::Core::Amqp::_detail::CbsOpenResult; + + std::string const reason{"the message sender or receiver open threw: connection refused"}; + + auto const withReason = Azure::Core::Amqp::_detail::DescribeCbsOpenFailure( + CbsOpenResult::Error, Audience(), CbsOpenCaller::Authenticate, reason); + EXPECT_NE(std::string::npos, withReason.find(reason)) << withReason; + // The fields that were already there must survive. + EXPECT_NE(std::string::npos, withReason.find("Error")) << withReason; + EXPECT_NE(std::string::npos, withReason.find(Audience())) << withReason; + EXPECT_NE(std::string::npos, withReason.find("ConnectionImpl::AuthenticateAudience")) + << withReason; + + // A layer that gave no reason must leave the sentence exactly as it was, so a caller that + // never sees a reason reads no empty clause. + auto const withoutReason = Azure::Core::Amqp::_detail::DescribeCbsOpenFailure( + CbsOpenResult::Error, Audience(), CbsOpenCaller::Authenticate); + EXPECT_EQ(std::string::npos, withoutReason.find("reason:")) << withoutReason; + EXPECT_EQ( + Azure::Core::Amqp::_detail::DescribeCbsOpenFailure( + CbsOpenResult::Error, Audience(), CbsOpenCaller::Authenticate, {}), + withoutReason); + } + + // The log line carries the reason as well, next to the connection summary and the elapsed time. + TEST_F(TestCbsOpenFailureText, TheLogTextCarriesTheReasonWithTheConnectionSummary) + { + std::string const reason{"the open completed with status Error for node '$cbs'"}; + + auto const text = Azure::Core::Amqp::_detail::FormatCbsOpenFailureLog( + Azure::Core::Amqp::_detail::CbsOpenResult::Error, + Audience(), + Azure::Core::Amqp::_detail::CbsTokenType::Jwt, + Azure::DateTime(2035, 1, 2, 3, 4, 5), + Azure::Core::Amqp::_detail::CbsOpenCaller::Refresh, + "instance 7, host example.servicebus.windows.net:5671, state End", + std::chrono::milliseconds(12), + reason); + EXPECT_NE(std::string::npos, text.find(reason)) << text; + EXPECT_NE(std::string::npos, text.find("instance 7")) << text; + EXPECT_NE(std::string::npos, text.find("12 ms")) << text; + } + TEST_F(TestCbsOpenFailureText, TheLogTextAddsTheTokenTypeAndTheExpiry) { Azure::DateTime const expiresOn(2035, 1, 2, 3, 4, 5);