rf: azure location config, use account / key #968

electrachong · 2017-10-27T19:13:11Z

No description provided.

bennettbuchanan · 2017-10-27T19:32:09Z

lib/Config.js

+        }
+    });
+    assert(azureContainerName, 'bad location constraint: ' +
+        `"${location}" azureContainerName must be defined`);


This would assert false if locationObj.details.azureContainerName was defined as something falsy. I don't know why someone would set their config that way, but I guess it's possible.

Made it more explicit to check if it is actually undefined and added some checks to make sure it is a string. The isValidBucketName function will check for length (make sure it's not an empty string)

bennettbuchanan · 2017-10-27T19:42:57Z

tests/unit/testConfigs/locConstraintAssert.js

+        },
+        '/should not throw for a valid azure location constraint/');
+    });
+    it('should throw error for if type is azure and azureContainerName is ' +


~~for if~~ if

thanks, updated those

Do it again human slave!:point_right: :runner: (Oh and the pull request has been updated, by the way.)

ironman-machine · 2017-10-27T20:01:27Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2017-10-27T20:01:27Z

CONFLICT (add/add): Merge conflict in tests/unit/testConfigs/locConstraintAssert.js
CONFLICT (add/add): Merge conflict in tests/locationConfigTests.json
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/utils.js
CONFLICT (add/add): Merge conflict in lib/data/locationConstraintParser.js
CONFLICT (add/add): Merge conflict in lib/data/external/AzureClient.js
CONFLICT (add/add): Merge conflict in lib/Config.js
CONFLICT (add/add): Merge conflict in constants.js

jonathan-gramain · 2017-10-27T19:51:01Z

constants.js

@@ -111,6 +111,10 @@ const constants = {
    mpuMDStoredExternallyBackend: { aws_s3: true },
    /* eslint-enable camelcase */
    mpuMDStoredOnS3Backend: { azure: true },
+    azureAccountNameRegex: /^[a-z0-9]{3,24}$/,
+    azureAccessKeyRegex:
+        new RegExp('^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==' +


This looks like a regular base64 validation regexp, could it be shared in a more generic base64Regex constant? (ideally in Arsenal but not worth a PR just for this), then azureAccessKeyRegex can point to it.

Also you may shorten it by removing the last |[A-Za-z0-9+/]{4} and make the trailer match block optional.

I think we can just use the general base64Regex for the azureAccessKey check then.
Thanks for the tip

jonathan-gramain · 2017-10-27T21:23:38Z

lib/Config.js

+        if (!locationConstraintDest) {
+            return true;
+        }
+        const accountNames = [locationConstraintSrc, locationConstraintDest]


style: for a simple comparison, I think calling twice getAzureStorageAccountName and comparing the results would be more straigthtforward than relying on a map.

That's true, I updated it

jonathan-gramain · 2017-10-27T21:47:15Z

tests/functional/aws-node-sdk/test/multipleBackend/utils.js

-            azureBlobSAS = process.env[`${azureLocation}_AZURE_BLOB_SAS`];
-            isTestingAzure = true;
-        } else if (config.locationConstraints[azureLocation] &&
+    const isTestingAzure = Object.keys(envMap).every(key => {


Maybe you can just rely on azureStorageAccountName presence, since an azure test would not make sense without the access key set as well.

We also create the blobservice client below, and we need to get the azureStorageAccessKey (stored in params in the every step) for that.

LaurenSpiegel · 2017-10-27T22:02:08Z

lib/data/external/AzureClient.js

+        this._client = azure.createBlobService(
+            this._azureStorageCredentials.storageAccountName,
+            this._azureStorageCredentials.storageAccessKey);
+        this._azureStorageEndpoint = this._client.host.primaryHost;


are we relying on a documented property of the azure node sdk here? do they have a method for the endpoint? is this the best way to do this?

Doesn't seem documented, they also don't have a method (they just access the StorageServiceClient.host.primaryHost property to get the value when sending a request). Maybe it's safest to require the blob endpoint explicitly again?

do you have to tell the client the endpoint? since it has this primary concept loaded, why do you need to tell it what it knows?

we don't need it for the blob storage client, there is so far one case where we use it:
for the copy source uri https://github.com/scality/S3/pull/968/files#diff-96a8cd65c80ea7fb9b908a81388494d4R366
We always use a put object if copying from one account to another (instead of copy object) so the storage endpoint for the copy source uri should be the same as the destination endpoint

LaurenSpiegel · 2017-10-27T22:03:18Z

lib/data/locationConstraintParser.js

@@ -74,11 +75,12 @@ function parseLC() {
            clients[location].clientType = 'aws_s3';
        }
        if (locationObj.type === 'azure') {
-            const azureBlobEndpoint = config.getAzureEndpoint(location);
-            const azureBlobSAS = config.getAzureBlobSas(location);
+            assert(locationObj.details, 'bad location constraint: details ' +


why is this assertion here rather than in config.js?

electrachong · 2017-10-30T20:05:45Z

@ironman-machine try

ironman-machine · 2017-10-30T20:05:47Z

Hello @electrachong

"try": Success: Try build successfully launched on 'http://ci.ironmann.io/gh/scality/Integration/16555' with the following env. args:

{
    "SCALITY_INTEGRATION_BRANCH": "ultron/master",
    "REPO_NAME": "S3",
    "DEFAULT_BRANCH": "master",
    "SCALITY_S3_BRANCH": "rf/azureCredentials"
}

ironman-machine · 2017-10-30T20:24:20Z

💔 ☔ circleCI test failed.

electrachong · 2017-10-30T21:16:01Z

end to end retried here: http://ci.ironmann.io/gh/scality/Integration/16559
and here: http://ci.ironmann.io/gh/scality/Integration/16562
and here: http://ci.ironmann.io/gh/scality/Integration/16571
for all three above, tests failed due to replication flakiness

electrachong · 2017-10-31T18:18:01Z

@ironman-machine try

ironman-machine · 2017-10-31T18:18:03Z

Hello @electrachong

"try": Success: Try build successfully launched on 'http://ci.ironmann.io/gh/scality/Integration/16611' with the following env. args:

{
    "SCALITY_INTEGRATION_BRANCH": "ultron/master",
    "REPO_NAME": "S3",
    "DEFAULT_BRANCH": "master",
    "SCALITY_S3_BRANCH": "rf/azureCredentials"
}

ironman-machine · 2017-10-31T19:11:43Z

☀️ 👍 circleCI test succeeded!

bennettbuchanan previously approved these changes Oct 27, 2017

View reviewed changes

JianqinWang approved these changes Oct 27, 2017

View reviewed changes

jonathan-gramain approved these changes Oct 27, 2017

View reviewed changes

LaurenSpiegel suggested changes Oct 27, 2017

View reviewed changes

electrachong force-pushed the rf/azureCredentials branch 3 times, most recently from f3b8d03 to f774bae Compare October 30, 2017 19:33

electrachong force-pushed the rf/azureCredentials branch from 879124b to 1ce30b4 Compare October 30, 2017 22:40

rf: azure location config, use account / key

249d401

electrachong force-pushed the rf/azureCredentials branch from 1ce30b4 to 249d401 Compare October 30, 2017 23:48

LaurenSpiegel approved these changes Oct 31, 2017

View reviewed changes

LaurenSpiegel merged commit d2ff4e0 into master Oct 31, 2017

LaurenSpiegel deleted the rf/azureCredentials branch October 31, 2017 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rf: azure location config, use account / key #968

rf: azure location config, use account / key #968

electrachong commented Oct 27, 2017 •

edited

Loading

bennettbuchanan Oct 27, 2017

electrachong Oct 27, 2017

bennettbuchanan Oct 27, 2017

electrachong Oct 27, 2017

ironman-machine commented Oct 27, 2017

ironman-machine commented Oct 27, 2017

jonathan-gramain Oct 27, 2017

electrachong Oct 28, 2017

jonathan-gramain Oct 27, 2017

electrachong Oct 28, 2017

jonathan-gramain Oct 27, 2017

electrachong Oct 28, 2017

LaurenSpiegel Oct 27, 2017

electrachong Oct 28, 2017

LaurenSpiegel Oct 28, 2017

electrachong Oct 30, 2017

LaurenSpiegel Oct 27, 2017

electrachong commented Oct 30, 2017

ironman-machine commented Oct 30, 2017

ironman-machine commented Oct 30, 2017

electrachong commented Oct 30, 2017 •

edited

Loading

electrachong commented Oct 31, 2017

ironman-machine commented Oct 31, 2017

ironman-machine commented Oct 31, 2017

rf: azure location config, use account / key #968

rf: azure location config, use account / key #968

Conversation

electrachong commented Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ironman-machine commented Oct 27, 2017

ironman-machine commented Oct 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

electrachong commented Oct 30, 2017

ironman-machine commented Oct 30, 2017

ironman-machine commented Oct 30, 2017

electrachong commented Oct 30, 2017 • edited Loading

electrachong commented Oct 31, 2017

ironman-machine commented Oct 31, 2017

ironman-machine commented Oct 31, 2017

electrachong commented Oct 27, 2017 •

edited

Loading

electrachong commented Oct 30, 2017 •

edited

Loading