Send auth on cluster recommendations and report cluster source (C#, Go, TS) - NO MERGE YET. server side telemetry need to rollout first - #674
Open
Arun G (ArunGopinathan) wants to merge 4 commits into
Conversation
GetClusterRecommendationsAsync hardcoded authHeader: null, so the call was always anonymous even when the same client authenticated the create that followed it. The service therefore could not identify the caller, and every SDK caller resolved to the default service tier regardless of configuration. Pass the token from the existing userTokenCallback, which is what every other request already uses. The service rejects an invalid or expired token before the controller runs, so it does not degrade to treating the caller as anonymous - it returns 401. A rejected token is now retried once without the header, so a token problem falls back to the previous anonymous behaviour instead of losing recommendations entirely. Clients with no token are unaffected: no header is sent and no retry is attempted. CreateTunnelAsync caught every recommendation failure with an empty block. The create still succeeds via global routing, so a caller had no way to tell that recommendation-based placement had stopped working - it looks identical to normal operation while silently reverting to nearest-by-latency selection. Failures are now classified and reported two ways: a ClusterSelected event for embedders, and an X-Tunnel-Cluster-Source header on the create so the path is visible in service telemetry rather than only on the client. The header requires a service-side change to be logged before it is observable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 09709668-833e-4c9a-bea4-fdb93f91b8e5
Mirrors the C# change in the Go SDK. The recommendations call was always anonymous, so the service could not identify the caller or apply their service tier. It now sends the caller's token. The service rejects a bad token before the controller runs rather than treating the caller as anonymous, so a rejected token is retried once without it. Without that retry a single expired token would silently disable recommendation-based routing for that caller. When the recommendations call fails, the create still succeeds via global routing, so the fallback was invisible. The selection path is now reported two ways: an X-Tunnel-Cluster-Source header, so it is visible in service telemetry without any cooperation from the embedder, and an optional OnClusterSelected callback for embedders that want to log it locally. The header values match the C# ones exactly. The header is passed explicitly to sendRequest because TunnelRequestOptions.AdditionalHeaders is written in four places but never read when the request is built; turning that on would also start sending If-Match and If-Not-Match, which is a separate behaviour change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 09709668-833e-4c9a-bea4-fdb93f91b8e5
…cript) The recommendations call now sends the caller token so the service can identify them and apply their service tier. A rejected token is retried anonymously, because the service rejects a bad token before the controller runs and would otherwise silently disable recommendation-based routing for that caller. createTunnel no longer swallows recommendation failures in an empty catch. It classifies the outcome and sends it in the X-Tunnel-Cluster-Source header, so a silent fallback to Traffic Manager routing is visible service-side, and raises onClusterSelected for embedders that want to log it. Matches the C# and Go changes on this branch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 09709668-833e-4c9a-bea4-fdb93f91b8e5
Required by the PR checklist. The Go PackageVersion goes to 0.1.28. The TS management and connections manifests still required >1.3.50 while the published packages are at 1.3.55, so yarn could resolve an older contracts or management package than the one these changes are built against and produce a mismatched dependency set. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 09709668-833e-4c9a-bea4-fdb93f91b8e5
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Arun G (ArunGopinathan)
changed the title
Arun G (ArunGopinathan)
changed the title
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
1 participant
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.
Fixes #642
Changes proposed:
createTunnelhad an emptycatchin all three SDKs: any failure fell back to nearest-cluster routing with nothing recorded. It now classifies the outcome and sends it in a newX-Tunnel-Cluster-Sourcerequest header, so a silent fallback is visible service-side. Values:explicit,recommended,recommended-after-auth-rejected,fallback-auth-failed,fallback-empty,fallback-error.ClusterSelectedevent (C#), anOnClusterSelectedcallback field (Go), and anonClusterSelectedemitter (TypeScript). This is secondary; the header is the mechanism that needs no embedder cooperation.PackageVersionto0.1.28, and raise the TSmanagement/connectionsdependency floors from>1.3.50to>1.3.55to match the published packages.The header is inert until the service logs it; that change is a follow-up PR in the service repo and is already queued.
Behaviour is unchanged when recommendations succeed. The fallback path is the same one as before — it is now attributed rather than silent.
Java and Rust are deliberately not in scope: their management clients never call the recommendations API at all, so there is no failure to attribute. Adopting recommendations there is a separate, larger change per language.
Testing
Each SDK has tests covering: the token being sent, no token being sent when the caller is anonymous, the anonymous retry after a 401/403, no retry on a non-auth failure, and the header value for each of the six classifications.
Every new check was fault-injected to confirm it can fail. Removing the token suppression on retry, dropping the header, and removing the retry each fail the expected tests (2, 6, and 2 respectively in TypeScript, with equivalents in C# and Go).
Other Tasks:
npm view @microsoft/dev-tunnels-contracts). This will fix issues where yarn will pull the old version of packages and will cause mismatched dependencies. See example PR