Support multi-destination CRR with V2 replication configuration#2627
Support multi-destination CRR with V2 replication configuration#2627maeldonn wants to merge 2 commits into
Conversation
Hello maeldonn,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review by Claude Code |
maeldonn
force-pushed
the
improvement/ARSN-571/crr-multi
branch
from
a4421cd to
a7bbbbc
Compare
| _derivePerRuleRoles() { | ||
| const { Rule } = this._parsedXML.ReplicationConfiguration; | ||
| this._rules!.forEach((rule, i) => { | ||
| const account = Rule[i].Destination[0].Account?.[0]; |
There was a problem hiding this comment.
_derivePerRuleRoles couples the _rules array ordering to the raw parsed XML Rule array by index. This works because _parseEachRule builds rules sequentially, but it is fragile — if rules are ever filtered, skipped, or reordered during parsing, the indices will silently mismatch and assign the wrong Account/role to rules.
Consider extracting the Account in _buildRuleObject (which already receives the XMLRule) and deriving the role there, so each rule carries its own Account context without depending on array-position alignment.
— Claude Code
| '</ReplicationConfiguration>' | ||
| `<?xml version="1.0" encoding="UTF-8"?>` + | ||
| `<ReplicationConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/">` + | ||
| `<Role>${escapeForXml(role)}</Role>` + |
There was a problem hiding this comment.
getConfigXML now emits Role before Rule elements. The old V1 code emitted them after (rulesXML + Role). While XML parsers are order-independent, this changes the byte-level output for all existing V1 configs. If any downstream consumer (e.g. checksums, string-matching assertions, ETag comparisons) depends on the exact XML string, this could be a subtle regression.
— Claude Code
| })).not.toThrow(); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
No round-trip test for V2: the tests verify parsing and XML generation separately, but there is no test that parses a V2 XML, extracts the config via getReplicationConfiguration(), feeds it to getConfigXML(), and re-parses the output to verify data fidelity. A round-trip test would catch any discrepancy between the two paths (e.g. lost priorities, wrong Account, prefix differences).
— Claude Code
There was a problem hiding this comment.
round-trip test for both v1 and v2 would be nice... (including the "unknown" case)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development/8.4 #2627 +/- ##
===================================================
+ Coverage 73.55% 73.78% +0.22%
===================================================
Files 222 222
Lines 18214 18314 +100
Branches 3774 3847 +73
===================================================
+ Hits 13398 13513 +115
+ Misses 4811 4796 -15
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
francoisferrand
left a comment
francoisferrand
left a comment
There was a problem hiding this comment.
can be greatly simplified by not being strict in parsing:
- always parse
account - always parse
priority - when parsing prefix, remember if it was in a or directly in the
This will always lots of back-and-forth and coupling with the _v2 variable in parsing, and simplify generation.
(And if we really want or need to be strict in parsing, we can add an extra validation at the end of parsing : after we have parsed everything, fail if format == v1 && account && priority. But IMHO not needed...)
DarkIsDude
left a comment
DarkIsDude
left a comment
There was a problem hiding this comment.
Not much more than Francois 🙏
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following reviewers are expecting changes from the author, or must review again: |
maeldonn
force-pushed
the
improvement/ARSN-571/crr-multi
branch
from
a7bbbbc to
acc58f0
Compare
| * pair; the destination side is used when present. If the rule carries | ||
| * an `account` override, its 12-digit ID replaces the account segment. | ||
| */ | ||
| static resolveDestinationRole(topRole: string, account?: string): string | undefined { |
There was a problem hiding this comment.
resolveDestinationRole is a new public static method with non-trivial ARN-splitting and account-substitution logic, but has no direct unit tests. Consider adding tests for: single ARN without account, comma-separated pair, account override replacing the correct segment, and empty/falsy topRole.
— Claude Code
| obj.storageClass = storageClass; | ||
| } | ||
|
|
||
| if (rule.Priority?.[0] !== undefined) { |
There was a problem hiding this comment.
_parsePriority skips validation when rule.Priority[0] is an empty string (!"" is true), but _buildRuleObject still converts it to a number: +'' → 0. An empty element would silently set priority to 0 without validation.
```suggestion
if (rule.Priority?.[0] !== undefined && rule.Priority[0] !== '') {
| @@ -144,6 +175,7 @@ export default class ReplicationConfiguration { | |||
| destination: this.getDestination(), | |||
There was a problem hiding this comment.
getDestination() returns string | null (always null now since _destination is never set), but ReplicationConfigurationMetadata.destination is typed as string | undefined. Under strict: true, assigning null to string | undefined is a type error. Consider changing _destination to string | undefined and initializing to undefined, or adjusting the return type here.
— Claude Code
Review by Claude Code |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
| } | ||
|
|
||
| if (rule.Priority?.[0] !== undefined) { | ||
| obj.priority = +rule.Priority[0]; |
There was a problem hiding this comment.
would be more explicit with Number or parseInt
| obj.priority = +rule.Priority[0]; | |
| obj.priority = Number(rule.Priority[0]); |
| const rules = instance.getRules(); | ||
| expect(rules.length).toEqual(1); | ||
| expect(rules[0].prefix).toEqual('docs/'); | ||
| expect(rules[0].destination).toEqual('arn:aws:s3:::crr-dest'); |
There was a problem hiding this comment.
expect(instance.getFormat()).toEqual('v1');
| rulesArr.push(this._buildRuleObject(rules[i])); | ||
| } | ||
|
|
||
| const hasAnyPriority = rulesArr.some(r => r.priority !== undefined); |
There was a problem hiding this comment.
the rule applies in any case : when 2 rules have overlapping prefix, they MUST have either a different destination (i.e. region for AWS, storageClass for us) or different priority
i.e. algorithm may be to create one "bucket" for each destination/priority, and compare prefixes in the bucket (or do a NxN comparison: check each rule either does not have the same destination, priority or "startsWith" all other rules)
| })).not.toThrow(); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
round-trip test for both v1 and v2 would be nice... (including the "unknown" case)
| const Status = `<Status>${enabled ? 'Enabled' : 'Disabled'}</Status>`; | ||
|
|
||
| const prefixContent = prefix === '' ? '<Prefix/>' : `<Prefix>${escapeForXml(prefix)}</Prefix>`; | ||
| const prefixXML = isV2 ? `<Filter>${prefixContent}</Filter>` : prefixContent; |
There was a problem hiding this comment.
do we HAVE TO specify a prefix (i.e. empty ) ?
if the user sends no prefix at all (neither v1 nor v2), this XML serialization would end up adding an empty filter/prefix : <Filer><Prefix/></Filter> instead of "nothing" ?
maeldonn
force-pushed
the
improvement/ARSN-571/crr-multi
branch
from
acc58f0 to
21b9bb3
Compare
maeldonn
force-pushed
the
improvement/ARSN-571/crr-multi
branch
2 times, most recently
from
f682065 to
fc0600f
Compare
|
|
||
| const backends: Backend[] = []; | ||
| for (const item of byKey.values()) { | ||
| const existing = existingBackends?.find(b => b.site === item.site); |
There was a problem hiding this comment.
With multi-destination CRR, byKey can hold multiple entries for the same site with different destinations (tested in "keeps two CRR backends for same site, different destinations"). This find matches by site alone, so both iterations pick up the same existing backend and carry forward the wrong dataStoreVersionId.
Match on destination as well for CRR sites:
```suggestion
const existing = existingBackends?.find(b => {
if (b.site !== item.site) return false;
if (!isCloud(item.site)) {
return b.destination === item.destination;
}
return true;
});
Review by Claude Code |
maeldonn
force-pushed
the
improvement/ARSN-571/crr-multi
branch
from
fc0600f to
c4207e1
Compare
| return this; | ||
| } | ||
|
|
||
| /** @deprecated in favor of per-backend role for multi-destination CRR. */ |
There was a problem hiding this comment.
Four public methods were removed here (setReplicationStorageType, setReplicationStorageClass, setReplicationTargetBucket, getReplicationDataStoreVersionId). As a shared library consumed by CloudServer and Backbeat, deleting public methods is a breaking API change that will fail downstream callers at runtime. Consider marking them @deprecated (like setReplicationRoles above) and keeping stubs that delegate or no-op, so consumers can migrate at their own pace.
— Claude Code
|
|
||
| const backends: Backend[] = []; | ||
| for (const item of byKey.values()) { | ||
| const existing = existingBackends?.find(b => b.site === item.site); |
There was a problem hiding this comment.
In multi-destination CRR, two backends can share the same site but target different destinations (the dedup key is site|destination|role). This find matches on site alone, so the first existing backend for that site would be reused for all new backends of the same site — carrying forward the wrong dataStoreVersionId.
— Claude Code
|
Run prettier on the files modified by the upcoming multi-destination CRR commit, so the logical changes land on a pre-formatted base and the next commit's diff stays focused on behavior rather than whitespace. Issue: ARSN-571
Add V2 replication configuration format with Filter, Priority, and per-rule Account/Destination fields. V1 format remains fully supported. Issue: ARSN-571
maeldonn
force-pushed
the
improvement/ARSN-571/crr-multi
branch
from
c4207e1 to
644d6dd
Compare
|
|
||
| const backends: Backend[] = []; | ||
| for (const item of byKey.values()) { | ||
| const existing = existingBackends?.find(b => b.site === item.site); |
There was a problem hiding this comment.
existingBackends lookup matches on site only, but multi-destination CRR (which this PR enables) can produce multiple backends for the same site with different destinations. find(b => b.site === item.site) returns the first match, so the wrong dataStoreVersionId is carried forward when two existing backends share a site but differ in destination.
| const existing = existingBackends?.find(b => b.site === item.site); | |
| const existing = existingBackends?.find(b => | |
| b.site === item.site && | |
| (!item.destination || b.destination === item.destination) | |
| ); |
— Claude Code
| dataStoreVersionId: '', | ||
| destination: undefined, | ||
| storageClass: undefined, | ||
| role: undefined, |
There was a problem hiding this comment.
Changing these defaults from '' to undefined is a breaking change for downstream callers. getReplicationRoles() (no args) previously always returned a string; now it returns undefined for new/default ObjectMD instances. Any code doing getReplicationRoles().split(',') will throw a TypeError. Same applies to getReplicationStorageType() and getReplicationStorageClass().
Consider keeping '' as the default here and only making the type optional, or updating all deprecated getters to fall back to '' (e.g. return this._data.replicationInfo.role ?? '').
— Claude Code
Review by Claude Code |
4 participants
Add V2 replication configuration support (Filter, Priority, per-rule Account) with multi-destination CRR. V1 remains fully supported and round-trips via a persisted format hint. Per-rule destination/role move into the Backend; legacy top-level ReplicationInfo fields kept as optional for backward-compatible reads.
Issue: ARSN-571