Repository navigation
fix(ess): authenticate and durably record ChartArtifact provisioning - #809
Merged
AntonTyb merged 4 commits intoSep 9, 2026
Merged
Conversation
The observer dialed the Dashboard Service with insecure credentials and no per-RPC credentials, and grpc-go does not propagate incoming metadata onto outgoing calls. CreateChart therefore arrived anonymous and came back Unauthenticated, which the UI rendered in place of the chart. withForwardedAuth copies the caller's IAP assertion or bearer token from the incoming metadata onto the outgoing context. The server's own identity would not do: CreateChart authorizes against the *user's* WRITER role on the experiment, so the user's token is the one that has to make the trip. An uncredentialed call still goes out -- whether to accept it is the Dashboard Service's decision, not the observer's -- but it logs a warning first, which is far easier to act on than the error it turns into. TAG=agy CONV=96ac94bd-c2b9-4753-b54f-b0b6b02d801f
The goroutine bounds itself to two minutes and then wrote the artifact's status under that same context. When CreateChart is what spent the budget, the write is refused before it reaches Spanner, so the FAILED status never lands: the artifact sits in PROVISIONING for good, and the UI keeps reporting a chart that is "still being provisioned" by a goroutine that exited long ago. context.WithoutCancel detaches the write and gives it its own 15-second budget, so the outcome is recorded whichever way the RPC went. The test drives OnArtifactCreated with an already-cancelled context and asserts the write context is still live and the status persisted is FAILED. TAG=agy CONV=96ac94bd-c2b9-4753-b54f-b0b6b02d801f
There was a problem hiding this comment.
Code Review
This pull request introduces credential forwarding (such as authorization and IAP assertion headers) from incoming metadata to outgoing metadata when calling the Dashboard Service, and ensures that artifact status updates are persisted under a separate context timeout even if the main call times out. The review feedback highlights a potential issue where metadata.NewOutgoingContext completely overwrites any existing outgoing metadata (e.g., tracing headers), and suggests copying the existing outgoing metadata instead. Corresponding test updates are also recommended to verify that existing outgoing metadata is preserved.
…ng auth withForwardedAuth built a fresh metadata.MD and handed it to NewOutgoingContext, which replaces whatever the context was already carrying rather than adding to it. Nothing is lost today -- this is the only NewOutgoingContext in the tree and the Dashboard connection is dialed without interceptors -- but a tracing or request-ID interceptor added later would have its headers silently dropped on this one call, which is a hard thing to go looking for. Copy the existing outgoing metadata and set the credentials on the copy. The "no credentials to forward" signal moves off len(outgoing), which is no longer zero when the context arrives carrying something else, onto an explicit flag. TAG=agy CONV=96ac94bd-c2b9-4753-b54f-b0b6b02d801f
AntonTyb
marked this pull request as ready for review
September 8, 2026 08:43
andrzejfudro
reviewed
Sep 8, 2026
Collaborator
There was a problem hiding this comment.
one question, one smaller request and two optional.
Review round 3 on #809. resolveUserEmail in internal/auth prefers x-goog-user-email over the token's own email claim, so a call that carried the header lost it on the way to CreateChart and was authorized against a different principal than the rest of the caller's requests. It is forwarded now, but on a separate list: it is an identity hint the Dashboard Service only honors next to a valid token, so it does not count as credentials and does not suppress the missing-credentials warning on its own. An empty header value is also treated as absent, matching how jwtFromMetadata reads the same headers. Previously it was forwarded and reported as credentials, which silenced the warning for a call that had none. TAG=agy CONV=96ac94bd-c2b9-4753-b54f-b0b6b02d801f
andrzejfudro
approved these changes
Sep 9, 2026
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.
What
Stacked on #676 (base branch
feat/chart-artifact-example). Slice 1 of 4 from the split of#710, requested in review.
Two defects in
dashboard_observer.gothat leave a chart artifact permanently broken. Bothpredate #676; they were invisible because nothing in the UI rendered the artifact's
error_message.1.
CreateChartwas never authenticatedEvery chart artifact came back
FAILEDwithThe Dashboard Service connection is dialed insecure with no per-RPC credentials
(
prod.go:initDashboardClient), and grpc-go does not carry incoming metadata onto outgoingcalls, so the observer's
CreateChartarrived with no token at all andcreatechart.gorejected it.
Giving the Experiment State Server its own identity would not have fixed it:
CreateChartcalls
authorizeExperimentWrite(ctx, user, xid), authorizing the user's WRITER role onthe experiment, so the user's own token is the one that has to make the trip.
withForwardedAuthcopiesx-goog-iap-jwt-assertion/authorizationoff the incomingcontext onto the outgoing one. When there is nothing to copy it logs a warning and calls
anyway — whether an anonymous call is acceptable is the Dashboard Service's decision, not the
observer's.
2. The status write reused the context that bounded
CreateChartIf
CreateChartspent its full two minutes, theUpdateArtifactthat records the outcomefailed before reaching Spanner.
FAILEDnever persisted and the artifact sat inPROVISIONINGfor good — the one state where the operator most needs to know what happened isthe one state that could not be written down. The write now runs under
context.WithoutCancelwith its own 15s deadline.Testing
gofmtclean,golangci-lintexit 0.Not run locally: Go tests. The import paths here are Bazel-mapped
(
xmanager_cloud/dashboard_service/proto/api_go_protoand friends have no on-disk Go package)and Bazel is not installed in this environment, so
go build/go testfail on every import.CI is their first real run.
New tests in
dashboard_observer_test.go:TestWithForwardedAuth— both header names, a non-credential header that must not beforwarded, no incoming metadata, and incoming metadata with nothing worth copying.
..._ForwardsCallerCredentials— the credentials reach the client, captured off theoutgoing context by the mock.
..._UncredentialedCallerStillCalls— the call is issued rather than dropped...._PersistsOutcomeAfterDeadline— checked to fail against the old code: with analready-cancelled governing context the status write still lands.
Notes for the reviewer
PerRPCCredentialsor interceptor patternexists anywhere in this repo to reuse, and only one outbound call needs it. If more
server-to-server hops appear this belongs in a client interceptor instead.
NewDashboardObserveris registered atprod.go:156, so thisis a live production call path, not dead code, and the behaviour is observable through the
artifact's stored status. But no UI surface draws a work-unit-parented chart until slice 3:
dashboards_page.ts:130is the onlylistChartscaller and it lists by dashboard parent.ChartIdnot taken. Raised on feat(ui): render ChartArtifact plots full-width on the work unit page #710. It does not stop an orphaned chart —there is no retry path here, so it would only make a future retry idempotent. Worth doing
separately.
Stack
feat/chart-artifact-example(#676)fix/ess-chart-provisioning-lifecyclefeat/ess-chart-artifact-plot-specsfeat/ui-chart-artifact-viewfeat/ui-chart-artifact-view-statesThe four slices together reproduce #710 byte-for-byte
(
git diffbetween the top of the stack and #710's head is empty).TAG=agy
CONV=96ac94bd-c2b9-4753-b54f-b0b6b02d801f