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
|
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...)
| status: string; | ||
| dataStoreVersionId: string; | ||
| destination?: string; | ||
| role?: string; |
There was a problem hiding this comment.
mandatory for CRR, right?
There was a problem hiding this comment.
Yes, but optional for backward compatibility.
| @@ -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.
| // Derive per-rule roles from Account after the top-level Role | ||
| // has been parsed, since role derivation depends on this._role. | ||
| if (this._format === 'v2' && this._rules) { | ||
| this._derivePerRuleRoles(); |
There was a problem hiding this comment.
should we do this at "parse" time (as you did), and internally store the role in each rule? or do this dynamically whenever the role is used?
- takes (some) space in metadata
- dupicates the information
+ may be slightly more efficient
+ maybe needed for better abstraction in other layers (i.e. work with "indepdendent" rules, not the whole replicationConfiguration)
There was a problem hiding this comment.
Switched to storing the per-rule account instead of the derived role. Top-level Role stays the single source of truth, and resolveDestinationRole() is exposed as a static helper for consumers.
| * the Account field by replacing the account ID in the destination | ||
| * role from the top-level Role field. | ||
| */ | ||
| _derivePerRuleRoles() { |
There was a problem hiding this comment.
should this not be done directly in parseAccount? it already goes through the "account" field, if any
There was a problem hiding this comment.
Can't be done in parseAccount, at that point the role hasn't been parsed yet. The derivation needs the top-level Role, so it's deferred once both are known.
| if (account) { | ||
| const destRole = this._deriveDestinationRole(account); | ||
| if (destRole) { | ||
| rule.role = destRole; |
There was a problem hiding this comment.
not stored everytime (e.g. if no account) : so code using role still needs to be aware, cannot be simplified that much...
There was a problem hiding this comment.
resolveDestinationRole returns the right ARN whether or not the rule has an account.
| // rules (and never for V1). | ||
| const isV2 = format | ||
| ? format === 'v2' | ||
| : rules.some(r => r.destination !== undefined); |
There was a problem hiding this comment.
seems weird to couple the output format to our internal representation.
in both AWS v1/v2 there is a Destination, so should not be used to distinguish...
at the same time, the difference between v1 and v2 are really about prefix.
while we could argue about being strict on parsing, there is no such problem here:
- we can generate
priorityif set (it won't be set on v1 if not allowed, no point depending on format) - we can generate
accountif set (it won't be set on v1 if not allowed, no point depending on format) - we need to know the format for
prefix, but only if a prefix was actually set: if it was not don't generate anything (no prefix or filter), and if it was we know the format
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 |
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