Support multi-destination CRR with V2 replication configuration#2627
Support multi-destination CRR with V2 replication configuration#2627maeldonn wants to merge 1 commit 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.79% +0.23%
===================================================
Files 222 222
Lines 18214 18308 +94
Branches 3774 3817 +43
===================================================
+ Hits 13398 13510 +112
+ Misses 4811 4793 -18
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...)
| @@ -1254,7 +1256,7 @@ export default class ObjectMD { | |||
| } | |||
|
|
|||
| getReplicationTargetBucket() { | |||
There was a problem hiding this comment.
should this function still be used?
(if it is just for backwards compatibility with existing backbeat, maybe time to create a new arsenal branch, that we'll bump in the next backbeat branch)
There was a problem hiding this comment.
This function is now deprecated.
There was a problem hiding this comment.
should we not remove them?
if they are used, some features will not work: so they MUST NOT be used in newer cloudserver/backbeat/s3utils which support multi-destination/... ?
→ if this is correct, we can remove them UNLESS we want to use this arsenal branch in older branches?
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: |
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
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:
|
| @@ -1254,7 +1256,7 @@ export default class ObjectMD { | |||
| } | |||
|
|
|||
| getReplicationTargetBucket() { | |||
There was a problem hiding this comment.
should we not remove them?
if they are used, some features will not work: so they MUST NOT be used in newer cloudserver/backbeat/s3utils which support multi-destination/... ?
→ if this is correct, we can remove them UNLESS we want to use this arsenal branch in older branches?
| } | ||
|
|
||
| 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" ?
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