[reference] Translate search attributes per namespace - #261
Draft
JayChung0258 wants to merge 3 commits into
Draft
Conversation
Drop comments that restate the code, keeping those that record facts the code cannot show: why namespace owners are an allowlist rather than a field name match, why resolution walks only upward, why a data blob is descended into even when the namespace is unresolved, and why one skip reason is counted and the other is not. Rename visitSearchAttributes' boundNamespaceID to fallbackNamespaceID so it matches resolveNamespaceID's fallback parameter, which is the same value. Move constMatcherResolver to reflection_test.go; it has no production caller and exists so the pre-existing table cases can apply one matcher to every namespace. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An earlier revision let a single mapping omit the namespaceId and applied it to every namespace, matching what configs deployed before per-namespace translation happen to do today. That keeps a config shape alive that nobody should write, and a blank id that silently applies to everything is the same ambiguity this change exists to remove. Validate now rejects any mapping without a namespaceId, naming the entry, so a config that cannot be applied per namespace fails at startup rather than translating an arbitrary namespace. Removes LegacyWildcardNamespaceID, HasLegacyWildcard and the resolver fallback. Consequence: the migration tooling still emits an empty namespaceId, so it has to emit real ids before this ships, and cells configured with a blank id need their configmap corrected in the same change. With no wildcard, visitSearchAttributes' unsupported-type branch is no longer reachable from Add/RemoveSearchAttributesRequest: neither is enclosed by a namespace owner, so the namespace resolves to empty, nothing matches, and the field is skipped before the type switch. Kept as a defensive path and its test collapsed to the one case that still exercises anything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
Splitting this into a stack of smaller PRs so it is easier to review. Keeping this one open as the place where the whole design is written up in one piece, and closing it when the last stage merges. Stage 1: #262 (pass a closure to visitDataBlobs) |
JayChung0258
marked this pull request as draft
August 25, 2026 02:10
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
Search attribute translation now resolves the namespace that owns each attribute and applies that namespace's mapping. Multiple namespaces can use custom search attributes through one proxy.
Why
The config has been keyed by
namespaceIdsince #95, but the runtime ignored the key:Go randomises map iteration and this runs per request, so two configured namespaces could each be translated with the other's mapping, differently from one request to the next. A panic kept that unreachable, which capped CSA namespaces at one per proxy and forced them to migrate one at a time.
Also silent: two mappings with the same or empty
namespaceIdoverwrote each other, leavingLenNamespaces() == 1, so the panic never fired.How
resolveNamespaceIDwalks up the parent chain to the nearest message that owns a namespace. Upward only, becausevisit.ValuesUnsafepops the front of its worklist and swaps in the last element, so traversal order is unspecified.NamespaceIdname match.StartChildWorkflowExecutionInitiatedEventAttributesholds the child'sNamespaceIdnext to the parent'sSearchAttributes, so a name match would apply the wrong mapping.WorkflowExecutionInfohas the same shape viaParentNamespaceId.visitDataBlobstakes ablobVisitorclosure.TranslateResponse(req, resp):GetWorkflowExecutionRawHistoryV2is in the shipped ACL default and its response carriesHistoryBatchesbut no namespace, so it comes from the paired request. Streams passnil.Breaking change:
namespaceIdis now requiredConfigs deployed before per-namespace translation omit it, and they work today only because the runtime ignored the key. There is no mapping that means "every namespace", so
Validaterejects a blanknamespaceIdat startup with a message naming the entry.This affects any deployment, not only the cloud side.
searchAttributeTranslationis documented in the chart (charts/s2s-proxy/files/default.yaml:63) as bi-directional, and both the inbound and outbound servers build a translator, so a self-hosted proxy can carry this config too. The chart's example has always included anamespaceId, so configs written against the docs are fine; the exposure is configs written before #95 added the field, or hand-written ones that omitted it.Before this ships, two things need to happen:
"namespaceId": "". It has to emit real namespace ids first.namespaceIdneeds its config corrected in the same change as the image.Both fail loudly at startup rather than silently, which is why this was preferred over grandfathering the empty id.
Other behaviour changes
namespaceId, duplicatenamespaceId, duplicatename. Previously one mapping silently overwrote another.Add/RemoveSearchAttributesRequestno longer abort the whole message. They also name a fieldSearchAttributesbut hold amap[string]enums.IndexedValueTypeand a[]string, so thedefault:branch warns, counts, and continues. With no wildcard this branch is unreachable from those two types (neither is enclosed by a namespace owner, so nothing resolves and the field is skipped first), so it is defensive only.search_attribute_translation_skipped{reason, message_type}. Not emitted when a namespace resolves but has no mapping, which is normal for any namespace not migrating. Nonamespace_idlabel, unbounded cardinality.Supersedes #96
Same approach to request pairing, but it tracked the last
NamespaceIdseen while descending. That is order dependent, so it cannot be correct for the multi-namespace case it was written for. It also matched on field name, and looked up real namespace ids against configs whose sole entry is keyed by"", which would have switched translation off silently rather than failing at startup.Reviewing
git diff main -- ':!*_test.go'is 216 added lines; the rest is tests.Worth the most attention: are the four owner types complete, and is the skip counter enough to notice if they stop being? They came from enumerating every search attribute and blob site in
adminservice,replication,persistenceandhistory.Known limitation: a cross-namespace child workflow's attributes inside a parent's history blob get the parent's mapping.
Checklist
Closes CGSCE-639
How was this tested:
Unit tests. The main case puts four replication tasks across three namespaces in one
StreamWorkflowReplicationMessagesResponse, two configured with different slots for the same source name and one unconfigured, then asserts each gets its own mapping. Looped 25 times, since one pass can pass by luck. Blobs andIndexedFieldsmaps are built per subtree, becausevisit.Valuesskips pointers it has already seen.Guards cover the child
NamespaceIdtrap,ParentNamespaceId, raw history responses paired and unpaired, an empty-keyed mapping matching nothing, unsupported field types, and a multi-namespace config reachingmakeServerOptions. Mutation checked: restoring the first-entry lookup, adding the child event type to the allowlist, or makingresolveNamespaceIDalways return its fallback each turns tests red.Not done: an end-to-end run with two replicated namespaces.
make generate-test-certs go test -race -timeout=12m -tags test_dep -count=1 ./... make lintYes, separately. Guidance elsewhere still says CSA namespaces migrate one at a time.
🤖 Generated with Claude Code