Skip to content

Commit b4822bf

Browse files
benzekrimahabenzekrimaha
committed
fixups post reviews
1 parent 27f3b70 commit b4822bf

File tree

14 files changed

+223
-496
lines changed

14 files changed

+223
-496
lines changed

‎index.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -152,8 +152,7 @@ export const storage = {
152152
external: {
153153
AwsClient: require('./lib/storage/data/external/AwsClient'),
154154
AzureClient: require('./lib/storage/data/external/AzureClient'),
155-
GcpClient: require('./lib/storage/data/external/GcpClient'),
156-
GCP: require('./lib/storage/data/external/GCP'),
155+
GcpClient: require('./lib/storage/data/external/GCP/GcpClient'),
157156
GcpUtils: require('./lib/storage/data/external/GCP/GcpUtils'),
158157
PfsClient: require('./lib/storage/data/external/PfsClient'),
159158
backendUtils: require('./lib/storage/data/external/utils'),

‎lib/network/kmsAWS/Client.ts‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,15 @@
11
'use strict';
22

33
import { arsenalErrorAWSKMS } from '../utils';
4-
import { KMSClient, CreateKeyCommand, ScheduleKeyDeletionCommand,
5-
GenerateDataKeyCommand, EncryptCommand, DecryptCommand, ListKeysCommand } from '@aws-sdk/client-kms';
4+
import { KMSClient,
5+
CreateKeyCommand,
6+
ScheduleKeyDeletionCommand,
7+
GenerateDataKeyCommand,
8+
EncryptCommand,
9+
DecryptCommand,
10+
ListKeysCommand,
11+
NotFoundException,
12+
KMSInvalidStateException } from '@aws-sdk/client-kms';
613
import * as werelogs from 'werelogs';
714
import assert from 'assert';
815
import { KMSInterface, KmsBackend, getKeyIdFromArn, KmsProtocol, KmsType, makeBackend } from '../KMSInterface';
@@ -61,7 +68,7 @@ export default class Client implements KMSInterface {
6168
this.client = new KMSClient({
6269
region,
6370
endpoint,
64-
requestHandler: undefined, // v3 handles agents differently
71+
requestHandler: undefined,
6572
credentials,
6673
});
6774
this.backend = makeBackend(KmsType.external, KmsProtocol.aws_kms, providerName);
@@ -107,6 +114,7 @@ export default class Client implements KMSInterface {
107114
if (this.noAwsArn) {
108115
keyId = keyMetadata?.KeyId || '';
109116
} else {
117+
// Prefer ARN, but fall back to KeyId if ARN is missing
110118
keyId = keyMetadata?.Arn ?? (keyMetadata?.KeyId || '');
111119
}
112120
const arn = `${this.backend.arnPrefix}${keyId}`;
@@ -135,7 +143,9 @@ export default class Client implements KMSInterface {
135143
KeyId: masterKeyId,
136144
PendingWindowInDays: 7,
137145
};
138-
this.client.send(new ScheduleKeyDeletionCommand(params)).then(data => {
146+
const command = new ScheduleKeyDeletionCommand(params);
147+
148+
this.client.send(command).then(data => {
139149
if (data?.KeyState && data.KeyState !== 'PendingDeletion') {
140150
const error = arsenalErrorAWSKMS('key is not in PendingDeletion state');
141151
logger.error('AWS KMS: failed to delete master encryption key', { data });
@@ -144,7 +154,7 @@ export default class Client implements KMSInterface {
144154
}
145155
cb(null);
146156
}).catch(err => {
147-
if (err.name === 'NotFoundException' || err.name === 'KMSInvalidStateException') {
157+
if (err instanceof NotFoundException || err instanceof KMSInvalidStateException) {
148158
logger.info('AWS KMS: key does not exist or is already pending deletion', { masterKeyId, error: err });
149159
cb(null);
150160
return;
@@ -166,7 +176,7 @@ export default class Client implements KMSInterface {
166176
assert.strictEqual(cryptoScheme, 1);
167177
const params = {
168178
KeyId: masterKeyId,
169-
KeySpec: "AES_256" as const,
179+
KeySpec: 'AES_256' as const,
170180
};
171181
this.client.send(new GenerateDataKeyCommand(params)).then(data => {
172182
if (!data) {

‎lib/network/utils.ts‎

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -32,28 +32,36 @@ export function arsenalErrorKMIP(err: string | Error) {
3232
const allowedKmsErrorCodes = Object.keys(allowedKmsErrors) as unknown as (keyof typeof allowedKmsErrors)[];
3333

3434
// Local AWSError type for compatibility with v3 error handling
35-
export type AWSError = Error & {
36-
code?: string;
37-
retryable?: boolean;
38-
statusCode?: number;
39-
time?: Date;
40-
requestId?: string;
35+
export type AWSError = Error & {
36+
name?: string;
37+
$fault?: 'client' | 'server';
38+
$metadata?: {
39+
httpStatusCode?: number;
40+
requestId?: string;
41+
attempts?: number;
42+
totalRetryDelay?: number;
43+
};
44+
$retryable?: {
45+
throttling?: boolean;
46+
};
47+
message?: string;
4148
};
4249

4350
function isAWSError(err: string | Error | AWSError): err is AWSError {
44-
return (err as AWSError).code !== undefined
45-
&& (err as AWSError).retryable !== undefined;
51+
return (err as AWSError).name !== undefined
52+
&& (err as AWSError).$metadata !== undefined;
4653
}
4754

4855
export function arsenalErrorAWSKMS(err: string | Error | AWSError) {
4956
if (isAWSError(err)) {
50-
if (allowedKmsErrorCodes.includes(err.code as keyof typeof allowedKmsErrors)) {
51-
return errorInstances[`KMS.${err.code}`].customizeDescription(err.message);
57+
const errorCode = err.name;
58+
if (allowedKmsErrorCodes.includes(errorCode as keyof typeof allowedKmsErrors)) {
59+
return errorInstances[`KMS.${errorCode}`].customizeDescription(err.message);
5260
} else {
5361
// Encapsulate into a generic ArsenalError but keep the aws error code
5462
return ArsenalError.unflatten({
5563
is_arsenal_error: true,
56-
type: `KMS.${err.code}`, // aws s3 prefix kms errors with KMS.
64+
type: `KMS.${errorCode}`, // aws s3 prefix kms errors with KMS.
5765
code: 500,
5866
description: `unexpected AWS_KMS error`,
5967
stack: err.stack,

‎lib/storage/data/LocationConstraintParser.js‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
const { http, https } = require('httpagent');
22
const url = require('url');
33
const { S3Client } = require('@aws-sdk/client-s3');
4+
const { NodeHttpHandler } = require('@smithy/node-http-handler');
45
const { fromIni } = require('@aws-sdk/credential-providers');
56
const Sproxy = require('sproxydclient');
67
const Hyperdrive = require('@scality/hdclient');
@@ -11,7 +12,7 @@ const DataFileBackend = require('./file/DataFileInterface');
1112
const inMemory = require('./in_memory/datastore').backend;
1213
const AwsClient = require('./external/AwsClient');
1314
const AzureClient = require('./external/AzureClient');
14-
const GcpClient = require('./external/GcpClient');
15+
const GcpClient = require('./external/GCP/GcpClient');
1516
const PfsClient = require('./external/PfsClient');
1617
const backendUtils = require('./external/utils');
1718

@@ -76,19 +77,26 @@ function parseLC(config, vault) {
7677
// https or http proxy
7778
connectionAgent = new HttpsProxyAgent(options);
7879
} else {
79-
connectionAgent = sslEnabled ?
80-
new https.Agent(httpAgentConfig, { maxSockets: false }) :
81-
new http.Agent(httpAgentConfig, { maxSockets: false });
80+
const agentOptions = { ...httpAgentConfig, maxSockets: false };
81+
82+
connectionAgent = sslEnabled
83+
? new https.Agent(agentOptions)
84+
: new http.Agent(agentOptions);
8285
}
8386
const httpOptions = { agent: connectionAgent, timeout: 0 };
8487
const s3Params = {
8588
endpoint: `${protocol}://${endpoint}`,
8689
region,
90+
requestHandler: new NodeHttpHandler({
91+
requestTimeout: 0,
92+
...(sslEnabled
93+
? { httpsAgent: connectionAgent }
94+
: { httpAgent: connectionAgent }),
95+
}),
8796
forcePathStyle: pathStyle,
97+
customUserAgent: constants.productName,
8898
maxAttempts: 1,
89-
requestHandler: undefined, // v3 handles agents differently
90-
// v3 does not use signatureVersion or sslEnabled directly
91-
// v3 does not use customUserAgent directly
99+
tls: sslEnabled,
92100
};
93101
if (locationObj.details.credentialsProfile) {
94102
s3Params.credentials = fromIni({ profile: locationObj.details.credentialsProfile });

‎lib/storage/data/external/AwsClient.js‎

Lines changed: 36 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,12 @@ const { S3Client,
1111
PutObjectTaggingCommand,
1212
DeleteObjectTaggingCommand,
1313
CopyObjectCommand,
14-
GetBucketLocationCommand } = require('@aws-sdk/client-s3');
14+
GetBucketLocationCommand,
15+
GetBucketVersioningCommand,
16+
NoSuchKeyException,
17+
NotFoundException,
18+
NoSuchVersionException,
19+
AccessDeniedException } = require('@aws-sdk/client-s3');
1520
const werelogs = require('werelogs');
1621

1722
const errors = require('../../../errors').default;
@@ -68,7 +73,7 @@ class AwsClient {
6873
return cb();
6974
})
7075
.catch(err => {
71-
if (err && err.code !== 'AuthorizationHeaderMalformed') {
76+
if (err.name !== 'AuthorizationHeaderMalformed') {
7277
this._logger.error('error during setup', {
7378
error: err,
7479
method: 'AwsClient.setup',
@@ -169,7 +174,7 @@ class AwsClient {
169174
.catch(err => {
170175
let logLevel;
171176
let retError;
172-
if (err.code === 'NotFound') {
177+
if (err instanceof NotFoundException) {
173178
logLevel = 'info';
174179
retError = errors.LocationNotFound;
175180
} else {
@@ -194,15 +199,23 @@ class AwsClient {
194199
this._client.send(new GetObjectCommand(params)).then(data => {
195200
// Always return an object with .createReadStream for test compatibility
196201
const stream = data.Body;
197-
stream.abort = () => {};
198-
const response = {
202+
let isAborted = false;
203+
const destroy = () => {
204+
if (isAborted) return;
205+
isAborted = true;
206+
(stream?.destroy || stream?.abort || stream?.close || stream?.end || stream?.removeAllListeners)?.();
207+
};
208+
if (!stream?.abort) {
209+
stream.abort = abort;
210+
}
211+
return callback(null, {
199212
createReadStream: () => stream,
200213
Body: stream,
201-
abort: () => {},
202-
};
203-
return callback(null, response);
214+
abort,
215+
destroy,
216+
});
204217
}).catch(err => {
205-
if (err.code === 'NoSuchKey' || err.code === 'NotFound') {
218+
if (err instanceof NoSuchKeyException || err instanceof NotFoundException) {
206219
logHelper(log, 'info', 'object not found', err, this._dataStoreName);
207220
} else {
208221
logHelper(log, 'error', 'error getting object from datastore', err, this._dataStoreName);
@@ -220,12 +233,10 @@ class AwsClient {
220233
if (deleteVersion) {
221234
params.VersionId = dataStoreVersionId;
222235
}
223-
return this._client.send(new DeleteObjectCommand(params)).then(() => {
224-
return callback();
225-
}).catch(err => {
236+
return this._client.send(new DeleteObjectCommand(params)).then(() => callback()).catch(err => {
226237
logHelper(log, 'error', 'error deleting object from ' +
227-
'datastore', err, this._dataStoreName, this.clientType);
228-
if (err.code === 'NoSuchVersion' || err.code === 'NoSuchKey') {
238+
'datastore', err, this._dataStoreName, this.clientType);
239+
if (err instanceof NoSuchVersionException || err instanceof NoSuchKeyException) {
229240
// data may have been deleted directly from the AWS backend
230241
// don't want to retry the delete and errors are not
231242
// sent back to client anyway, so no need to return err
@@ -241,22 +252,19 @@ class AwsClient {
241252
healthcheck(location, callback) {
242253
const awsResp = {};
243254
this._client.send(new HeadObjectCommand({ Bucket: this._awsBucketName }))
244-
.catch(err => {
245-
246-
awsResp[location] = { error: err, external: true };
247-
return callback(null, awsResp);
248-
}).then(() => {
255+
.then(() => {
249256
if (!this._supportsVersioning) {
250257
awsResp[location] = {
251258
message: 'Congrats! You own the bucket',
252259
};
253260
return callback(null, awsResp);
254261
}
255-
return this._client.send(new GetObjectCommand({
262+
return this._client.send(new GetBucketVersioningCommand({
256263
Bucket: this._awsBucketName })).catch(err => {
257264
awsResp[location] = { error: err, external: true };
258265
}).then(data => {
259-
if (!data.VersionId) {
266+
if (!data.Status ||
267+
data.Status === 'Suspended') {
260268
awsResp[location] = {
261269
versioningStatus: data.Status,
262270
error: 'Versioning must be enabled',
@@ -270,6 +278,9 @@ class AwsClient {
270278
}
271279
return callback(null, awsResp);
272280
});
281+
}).catch(err => {
282+
awsResp[location] = { error: err, external: true };
283+
return callback(null, awsResp);
273284
});
274285
}
275286

@@ -419,10 +430,10 @@ class AwsClient {
419430
const completeObjData = { key: awsKey };
420431
return this._client.send(new CompleteMultipartUploadCommand(mpuParams)).catch(err => {
421432

422-
if (mpuError[err.code]) {
433+
if (mpuError[err.name]) {
423434
logHelper(log, 'trace', 'err from data backend on ' +
424435
'completeMPU', err, this._dataStoreName, this.clientType);
425-
return callback(errors[err.code]);
436+
return callback(errors[err.name]);
426437
}
427438
logHelper(log, 'error', 'err from data backend on ' +
428439
'completeMPU', err, this._dataStoreName, this.clientType);
@@ -551,7 +562,7 @@ class AwsClient {
551562
awsParams.ServerSideEncryption = 'AES256';
552563
}
553564
return this._client.send(new CopyObjectCommand(awsParams)).catch(err => {
554-
if (err.code === 'AccessDenied') {
565+
if (err instanceof AccessDeniedException) {
555566
logHelper(log, 'error', 'Unable to access ' +
556567
`${sourceAwsBucketName} ${this.type} bucket`, err,
557568
this._dataStoreName, this.clientType);
@@ -608,7 +619,7 @@ class AwsClient {
608619
UploadId: uploadId,
609620
};
610621
return this._client.send(new CopyObjectCommand(params)).catch(err => {
611-
if (err.code === 'AccessDenied') {
622+
if (err instanceof AccessDeniedException) {
612623
logHelper(log, 'error', 'Unable to access ' +
613624
`${sourceAwsBucketName} AWS bucket`, err,
614625
this._dataStoreName, this.clientType);

‎lib/storage/data/external/AzureClient.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -263,7 +263,7 @@ class AzureClient {
263263
} catch (err) {
264264
let logLevel;
265265
let retError;
266-
if (err.code === 'NotFound') {
266+
if (err.name === 'NotFound') {
267267
logLevel = 'info';
268268
retError = errors.LocationNotFound;
269269
} else {

0 commit comments

Comments
 (0)