[CRE] Allow the Gateway to handle multiple dons#22437
Merged
Merged
Conversation
… the gatewayConfig.ShardedDONs.DonName
Contributor
|
I see you updated files related to
|
Contributor
|
✅ No conflicts with other open PRs targeting |
|
cedric-cordenier
approved these changes
May 14, 2026
george-dorin
approved these changes
May 14, 2026
agparadiso
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Risk Rating: MEDIUM
This PR updates the Vault gateway aggregation path so that when the capabilities registry reports multiple DONs hosting the Vault capability, the handler selects the DON associated with the gateway job’s configured DON name.
Changes:
- Passes the handler DON identifier into
baseAggregator. - Adds multi-DON Vault capability selection by DON name, with legacy numeric ID fallback.
- Adds tests for name-based selection, ID fallback, no-match, and ambiguous-match cases.
Targeted human review areas:
- Multi-DON selection logic in
donForVaultCapability. - Consistency between gateway
ShardedDONs.DonName, legacyDONConfig.DonId, and registry DON names/IDs.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
core/services/gateway/handlers/vault/handler.go |
Wires the configured DON identifier into the Vault aggregator. |
core/services/gateway/handlers/vault/aggregator.go |
Selects the correct Vault DON when multiple registry DONs host the capability. |
core/services/gateway/handlers/vault/handler_test.go |
Extends the mock registry and updates a public-key test aggregator setup. |
core/services/gateway/handlers/vault/aggregator_test.go |
Adds tests for multi-DON selection and error cases. |
Comment on lines
+69
to
+70
| return nil, fmt.Errorf("multiple DONs (%d) host vault capability %s but vault handler DonId is empty; set ShardedDONConfig.DonName so DONConfig.DonId matches the vault DON name or id in the registry (%s)", | ||
| len(dons), vaultcommon.CapabilityID, summarizeVaultRegistryDONs(dons)) |
Comment on lines
+59
to
+60
| if len(dons) == 0 { | ||
| return nil, fmt.Errorf("no DON found for vault capability %s", vaultcommon.CapabilityID) |
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.
Description
if multiple vault dons are listed, select the correct one using the gatewayConfig.ShardedDONs.DonName
currently our motivation is to have cre reliability environment vault don, given this is in staging we need to handle two different vault dons (staging and reliability). The change is backwards compatible, so if one only vault don is present the same behaviour is maintained
Requires
Supports