Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions extensions/replication/queueProcessor/QueueProcessor.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ const RoundRobin = require('arsenal').network.RoundRobin;
const BackbeatProducer = require('../../../lib/BackbeatProducer');
const BackbeatConsumer = require('../../../lib/BackbeatConsumer');
const VaultClientCache = require('../../../lib/clients/VaultClientCache');
const CredentialsManager = require('../../../lib/credentials/CredentialsManager');
const QueueEntry = require('../../../lib/models/QueueEntry');
const TaskScheduler = require('../../../lib/tasks/TaskScheduler');
const { getTaskSchedulerQueueKey,
Expand Down Expand Up @@ -229,6 +230,12 @@ class QueueProcessor extends EventEmitter {
this.logger = new Logger(
`Backbeat:Replication:QueueProcessor:${this.site}`);

this.assumedRoleCredentialsManager = new CredentialsManager(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'replication-copy-location', this.logger);
this.assumedRoleS3Clients = {};
this.assumedRoleHTTPAgent = new HttpAgent.Agent({ keepAlive: true });
this.assumedRoleHTTPSAgent = new HttpsAgent.Agent({ keepAlive: true });

// global variables
if (sourceConfig.transport === 'https') {
this.sourceHTTPAgent = new HttpsAgent.Agent({
Expand Down Expand Up @@ -690,6 +697,10 @@ class QueueProcessor extends EventEmitter {
destHTTPAgent: this.destHTTPAgent,
vaultclientCache: this.vaultclientCache,
accountCredsCache: this.accountCredsCache,
assumedRoleCredentialsManager: this.assumedRoleCredentialsManager,
assumedRoleS3Clients: this.assumedRoleS3Clients,
assumedRoleHTTPAgent: this.assumedRoleHTTPAgent,
assumedRoleHTTPSAgent: this.assumedRoleHTTPSAgent,
replicationStatusProducer: this.replicationStatusProducer,
mProducer: this._mProducer,
logger: this.logger,
Expand Down
158 changes: 137 additions & 21 deletions extensions/replication/tasks/CopyLocationTask.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@ const { v4: uuid } = require('uuid');

const { errors, jsutil, models } = require('arsenal');
const { ObjectMD } = models;
const { S3Client: AwsS3Client, GetObjectCommand: AwsGetObjectCommand } =

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can discuss these renaming
It's just that we are using getObject from both the official aws SDK, and from our cloudserverClient

require('@aws-sdk/client-s3');

const BackbeatMetadataProxy = require('../../../lib/BackbeatMetadataProxy');
const BackbeatTask = require('../../../lib/tasks/BackbeatTask');
const {
const {
BackbeatRoutesClient,
GetObjectCommand,
GetObjectCommand: BackbeatRoutesGetObjectCommand,
MultipleBackendPutObjectCommand,
MultipleBackendInitiateMPUCommand,
MultipleBackendPutMPUPartCommand,
Expand All @@ -24,6 +26,8 @@ const { getAccountCredentials } =
require('../../../lib/credentials/AccountCredentials');
const RoleCredentials =
require('../../../lib/credentials/RoleCredentials');
const config = require('../../../lib/Config');
const { authTypeAssumeRole } = require('../../../lib/constants');
const { metricsExtension, metricsTypeQueued, metricsTypeCompleted } =
require('../constants');

Expand Down Expand Up @@ -101,6 +105,117 @@ class CopyLocationTask extends BackbeatTask {
.setSourceClient(log);
}

/**
* Get a cached S3 client, authenticated with the assumed-role
* credentials for the role carried on a location part.
* @param {Object} locationConfig - the location's config
* @param {String} roleArn - the role ARN carried by the location part
* @param {Werelogs} log - the logger instance
* @return {AwsS3Client} the client
* @throws {ArsenalError} AccessDenied (retryable) if credentials
* could not be obtained for the role
*/
_getAssumedRoleS3Client(locationConfig, roleArn, log) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't we have this (or similar) function in CRR ?
can't we dedup and use the same function?

const { details } = locationConfig;
const s3Endpoint = `${details.transport}://${details.servers[0]}`;
const cacheKey = `${s3Endpoint}::${roleArn}`;
if (this.assumedRoleS3Clients[cacheKey]) {
return this.assumedRoleS3Clients[cacheKey];
}
const accountId = roleArn.split(':')[4];
const roleName = roleArn.split(':role/')[1];
const credentials = this.assumedRoleCredentialsManager.getCredentials({
id: roleArn,
accountId,
authConfig: {
type: authTypeAssumeRole,
roleName,
},
stsConfig: {
endpoint: `${details.transport}://${details.sts.host}:${details.sts.port}`,
credentials: {
accessKeyId: details.sts.accessKey,
secretAccessKey: details.sts.secretKey,
},
},
});
if (!credentials) {
log.error('unable to obtain assumed-role credentials for source location', {
method: 'CopyLocationTask._getAssumedRoleS3Client',
roleArn,
endpoint: s3Endpoint,
});
const err = errors.AccessDenied.customizeDescription(
`unable to assume role ${roleArn} for isCRR source location`);
err.retryable = true;
throw err;
}
const isHttps = details.transport === 'https';
const client = new AwsS3Client({
endpoint: s3Endpoint,
credentials: credentials.getCredentialsProvider(),
region: 'us-east-1',
forcePathStyle: true,
requestHandler: {
[isHttps ? 'httpsAgent' : 'httpAgent']:
isHttps ? this.assumedRoleHTTPSAgent : this.assumedRoleHTTPAgent,
requestTimeout: TIMEOUT_MS,
},
Comment thread
SylvainSenechal marked this conversation as resolved.
maxAttempts: 1,
});
client.middlewareStack.add(isRetryableMiddleware(), {
step: 'deserialize',
priority: 'high',
});
this.assumedRoleS3Clients[cacheKey] = client;
return this.assumedRoleS3Clients[cacheKey];
}

/**
* Send a GetObject request for the object's data,
* reading either through Cloudserver's multiple-backend routes,
* or directly from a CRR source location's own S3
* endpoint via an assumed role.
* @param {ActionQueueEntry} actionEntry - the action entry
* @param {ObjectMD} objMD - metadata object
* @param {Object} [range] - byte range to request, or undefined for the whole object
* @param {Werelogs} log - the logger instance
* @param {AbortController} abortController - abort controller for the GET request
* @return {Promise} resolves to the GetObject response
*/
async _sendGetObject(actionEntry, objMD, range, log, abortController) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const locationConfig = config.getLocationConstraint(objMD.getDataStoreName());
if (locationConfig?.isCRR === true) {
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;
}
Comment on lines +189 to +196

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

const s3Client = this._getAssumedRoleS3Client(locationConfig, part.role, log);
const command = new AwsGetObjectCommand({
Bucket: part.bucket,
Key: objMD.getKey(),
VersionId: part.dataStoreVersionId,
Range: range && `bytes=${range.start}-${range.end}`,
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the parameters should be mostly the same as the regular ones:

  • bucket, key and version shoud come from the message's actionEntry.getAttribute('target'), same as the other case
  • need to pass the requestUids as well

the only difference is the locationConstraint indeed.....and the object type AwsGetObjectCommand vs BackbeatRoutesGetObjectCommand

  • I wonder if/why locationConstraint is 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)

return await s3Client.send(command, { abortSignal: abortController.signal });
}

const { bucket, key, version } = actionEntry.getAttribute('target');
const command = new BackbeatRoutesGetObjectCommand({
Bucket: bucket,
Key: key,
VersionId: version,
Range: range && `bytes=${range.start}-${range.end}`,
LocationConstraint: objMD.getDataStoreName(),
RequestUids: log.getSerializedUids(),
});
return await this.backbeatClient.send(command, { abortSignal: abortController.signal });
}

processQueueEntry(actionEntry, kafkaEntry, done) {
const startTime = Date.now();
const log = this.logger.newRequestLogger();
Expand Down Expand Up @@ -237,16 +352,8 @@ class CopyLocationTask extends BackbeatTask {
let sourceStreamAborted = false;
let abortedByPut = false;
const abortController = new AbortController();
const { bucket, key, version } = actionEntry.getAttribute('target');
const getObjectCommand = new GetObjectCommand({
Bucket: bucket,
Key: key,
VersionId: version,
LocationConstraint: objMD.getDataStoreName(),
RequestUids: log.getSerializedUids(),
});

return this.backbeatClient.send(getObjectCommand, { abortSignal: abortController.signal })
return this._sendGetObject(actionEntry, objMD, undefined, log, abortController)
.then(response => {
const incomingMsg = response.Body;
incomingMsg.on('error', err => {
Expand Down Expand Up @@ -293,6 +400,14 @@ class CopyLocationTask extends BackbeatTask {
actionEntry, objMD, size, incomingMsg, log, putDone);
})
.catch(err => {
if (err.name === 'NoSuchVersion') {
log.info('source version no longer exists', Object.assign({
method: 'CopyLocationTask._getAndPutObjectOnce',
error: err.message,
}, actionEntry.getLogInfo()));
return doneOnce(errors.InvalidObjectState.customizeDescription(
'source version no longer exists'));
}
if (err.$metadata?.httpStatusCode === 404) {
log.error('the source object was not found', Object.assign({
method: 'CopyLocationTask._getAndPutObjectOnce',
Expand All @@ -307,6 +422,7 @@ class CopyLocationTask extends BackbeatTask {
method: 'CopyLocationTask._getAndPutObjectOnce',
peer: this.sourceConfig.s3,
error: err.message,
errorName: err.name,
httpStatus: err.$metadata?.httpStatusCode,
}, actionEntry.getLogInfo()));
return doneOnce(err);
Expand Down Expand Up @@ -418,27 +534,27 @@ class CopyLocationTask extends BackbeatTask {
}

const abortController = new AbortController();
const { bucket, key, version } = actionEntry.getAttribute('target');
const getObjectCommand = new GetObjectCommand({
Bucket: bucket,
Key: key,
VersionId: version,
Range: range && `bytes=${range.start}-${range.end}`,
LocationConstraint: objMD.getDataStoreName(),
RequestUids: log.getSerializedUids(),
});

return this.backbeatClient.send(getObjectCommand, { abortSignal: abortController.signal })
return this._sendGetObject(actionEntry, objMD, range, log, abortController)
.then(response => this._putMPUPart(actionEntry, objMD, response.Body, size,
uploadId, partNumber, log, abortController, done))
.catch(err => {
if (err.name === 'NoSuchVersion') {
log.info('source version no longer exists', Object.assign({
method: 'CopyLocationTask._getRangeAndPutMPUPartOnce',
error: err.message,
}, actionEntry.getLogInfo()));
return done(errors.InvalidObjectState.customizeDescription(
'source version no longer exists'));
}
if (err.$metadata?.httpStatusCode === 404) {
return done(err);
}
log.error('an error occurred on getObject from S3',
Object.assign({
method: 'CopyLocationTask._getRangeAndPutMPUPartOnce',
error: err.message,
errorName: err.name,
httpStatus: err.$metadata?.httpStatusCode,
}, actionEntry.getLogInfo()));
return done(err);
Expand Down
14 changes: 14 additions & 0 deletions lib/Config.js
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ class Config extends EventEmitter {
Object.assign(this, parsedConfig);

this.transientLocations = {};
this.locationConstraints = {};

this._setTimeOptions();
this._setLifecycleConductorOptions();
Expand Down Expand Up @@ -311,6 +312,19 @@ class Config extends EventEmitter {
return this.transientLocations[locationName] || false;
}

setLocationConstraints(locationConstraints) {
this.locationConstraints = locationConstraints;
}

/**
* Get the raw location constraint config for a given location name
* @param {String} locationName - the location constraint name
* @return {Object|undefined} the location config, or undefined if unknown
*/
getLocationConstraint(locationName) {
return this.locationConstraints[locationName];
}

getPublicInstanceId() {
return this.publicInstanceId;
}
Expand Down
1 change: 1 addition & 0 deletions lib/management/operatorBackend.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ function initManagement(params, done) {
}));
const locations = require('../../conf/locationConfig.json') || {};

config.setLocationConstraints(locations);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is relevant, we don't really care bout backbeat running in orbit mode 🤔

Object.keys(locations).forEach(locName => {
config.setIsTransientLocation(
locName, locations[locName].isTransient);
Expand Down
Loading
Loading