Copy location task reads source location with s3 sdk with STS - #2805
Copy location task reads source location with s3 sdk with STS#2805SylvainSenechal wants to merge 1 commit into
Conversation
Hello sylvainsenechal,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}
2c522ac to
da250cd
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 2 files with indirect coverage changes
@@ Coverage Diff @@
## development/9.5 #2805 +/- ##
===================================================
- Coverage 75.76% 75.55% -0.22%
===================================================
Files 200 200
Lines 13922 13968 +46
===================================================
+ Hits 10548 10553 +5
- Misses 3364 3405 +41
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
da250cd to
363db9e
Compare
363db9e to
b4fd620
Compare
| })); | ||
| const locations = require('../../conf/locationConfig.json') || {}; | ||
|
|
||
| config.setLocationConstraints(locations); |
There was a problem hiding this comment.
patchConfiguration.js::updateLocations also updates setBootstrapList and setIsTransientLocation but does not call setLocationConstraints. Locations updated through the dynamic configuration path (Orbit/cloud deployments) won't be stored, so config.getLocationConstraint() will return undefined and the isCRR branch in _sendGetObject will never be taken.
Add config.setLocationConstraints(locations) in patchConfiguration.js::updateLocations alongside the existing setBootstrapList call.
There was a problem hiding this comment.
I don't think this is relevant, we don't really care bout backbeat running in orbit mode 🤔
|
|
||
| const { errors, jsutil, models } = require('arsenal'); | ||
| const { ObjectMD } = models; | ||
| const { S3Client: AwsS3Client, GetObjectCommand: AwsGetObjectCommand } = |
There was a problem hiding this comment.
Can discuss these renaming
It's just that we are using getObject from both the official aws SDK, and from our cloudserverClient
b4fd620 to
28c18cd
Compare
| this.logger = new Logger( | ||
| `Backbeat:Replication:QueueProcessor:${this.site}`); | ||
|
|
||
| this.assumedRoleCredentialsManager = new CredentialsManager( |
There was a problem hiding this comment.
Not sure if this is the best place : queueProcessor does create a new copyLocation Task for each kafka entry so we can't put these in CopyLocationTask class otherwise its useless, but we may also have multiple instances of queue processor depending on the config which means we would have multiple credentials managers/clients for the same authentification
| * @param {AbortController} abortController - abort controller for the GET request | ||
| * @return {Promise} resolves to the GetObject response | ||
| */ | ||
| async _sendGetObject(actionEntry, objMD, range, log, abortController) { |
There was a problem hiding this comment.
For those who wanna understand the pr's main idea :
Before : We only used our backbeatClient (cloudserver client) to get object, that backbeat client asked cloudserver to deal with reading the data based on the location.
But Cloudserver is not capable of reading data from external CRR locations.
Now : When the object's location is "CRR", we use a classic S3 client with sts to directly get the data without going through cloudserver
28c18cd to
b003530
Compare
b003530 to
c9e4bb6
Compare
SylvainSenechal
changed the title
| * @param {AbortController} abortController - abort controller for the GET request | ||
| * @return {Promise} resolves to the GetObject response | ||
| */ | ||
| async _sendGetObject(actionEntry, objMD, range, log, abortController) { |
There was a problem hiding this comment.
this function does 2 things, which are orthogonal concerns:
- it retrieves/builds a client, depending on location : either the (exisitng/global) backbeatClient, or the new STS client if required.
- it send the actual command
it would seem more appropriate to split responsability, and introduce a "getClient" function - then fallthrough to the existing code.......but I see you don't pass the same parameter, to the command: see https://github.com/scality/backbeat/pull/2805/changes#r3819951224
| Key: objMD.getKey(), | ||
| VersionId: part.dataStoreVersionId, | ||
| Range: range && `bytes=${range.start}-${range.end}`, | ||
| }); |
There was a problem hiding this comment.
the parameters should be mostly the same as the regular ones:
bucket,keyandversionshoud come from the message'sactionEntry.getAttribute('target'), same as the other case- need to pass the
requestUidsas well
the only difference is the locationConstraint indeed.....and the object type AwsGetObjectCommand vs BackbeatRoutesGetObjectCommand
- I wonder if/why
locationConstraintis used here, should not be needed most of the time - but maybe in case of transient location or similarly advanced/corner case. So indeed should not be changed I guess (for the regular transition path), let's not take risk ; and must not be added to the STS path indeed. - the "target" of CopyLocationTask is not a random S3 server, this is really a cloudserver. So we can and should use CloudServerClient and our own 'extensions' (RequestUids)
| const locations = objMD.getLocation(); | ||
| const part = locations && locations[0]; | ||
| if (!part || !part.role) { | ||
| const err = errors.AccessDenied.customizeDescription( | ||
| 'missing role on location part for isCRR source location'); | ||
| err.retryable = true; | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
I don't have the full design in mind anymore, but I think params should be in the kafka message instea?
when we create the message, we will anyway parse the ObjMD : so this location validation/... would better be done there only (in particular adding the role to actionEntry.getAttribute('target')), so in CopyLocationTask we only need to do it.
this also matches the current 'design' of CopyLocationTask : it copies data from actionEntry.getAttribute('target') to the object's location, and does try to check what is currently in ObjMD's location - which may (or may not) help to avoid some race conditions, not sure...
(STS could even be 'triggered' not just by locationConfig?.isCRR but by actionEntry.getAttribute('target').role ? Either way, will need to access the location for creds/hosts...)
What do you think? Can you evaluate impact and confirm?
| * @throws {ArsenalError} AccessDenied (retryable) if credentials | ||
| * could not be obtained for the role | ||
| */ | ||
| _getAssumedRoleS3Client(locationConfig, roleArn, log) { |
There was a problem hiding this comment.
don't we have this (or similar) function in CRR ?
can't we dedup and use the same function?
3 participants
This pr handles the necessary modification to copyLocation task, so that the clean room can use it to pull object data from the production site.
The main change is basically modifying the way we set up the s3 clients in copyLocationTask, depending on the location of the object. This requires storing and exposing the location configuration which we didn't do before.
Issue: BB-812