Azure repository: Move to named configurations as we do for S3 repository and secure settings #23405

dadoonet · 2017-02-28T11:27:32Z

We should have the same behavior for Azure repositories as we have for S3 (see #22762).

So we want to use a similar name for keys and define explicitly the configuration name to use with client of instead account.
Also we secure our key/secret combination.

Instead of:

cloud:
    azure:
        storage:
            my_account1:
                account: your_azure_storage_account1
                key: your_azure_storage_key1
                default: true
            my_account2:
                account: your_azure_storage_account2
                key: your_azure_storage_key2

Support something like:

bin/elasticsearch-keystore add azure.client.default.account
bin/elasticsearch-keystore add azure.client.default.key
bin/elasticsearch-keystore add azure.client.my_account2.account
bin/elasticsearch-keystore add azure.client.my_account2.key

Then instead of:

PUT _snapshot/my_backup3
{
    "type": "azure",
    "settings": {
        "account": "my_account2"
    }
}

Use:

PUT _snapshot/my_backup3
{
    "type": "azure",
    "settings": {
        "client": "my_account2"
    }
}

If someone uses:

PUT _snapshot/my_backup3
{
    "type": "azure"
}

It will use the default azure repository settings.

And mark as deprecated old settings.
The plan is to then remove deprecated settings as a follow up in master branch only.

Closes #22763.

dadoonet · 2017-02-28T11:27:54Z

@rjernst Can you please look at it?

jasontedor

I left a comment. I didn't do a full review, just one thing I noticed.

jasontedor · 2017-02-28T12:27:53Z

...tory-azure/src/test/java/org/elasticsearch/cloud/azure/storage/AzureStorageServiceTests.java

@@ -60,6 +60,7 @@ public void testGetSelectedClientWithNoSecondary() {
            .build());
        CloudBlobClient client = azureStorageService.getSelectedClient("azure1", LocationMode.PRIMARY_ONLY);
        assertThat(client.getEndpoint(), is(URI.create("https://azure1")));
+        assertWarnings("cloud.azure.storage.azure1.account", "cloud.azure.storage.azure1.key");


You can use ESTestCase#assertSettingDeprecations here (and throughout).

rjernst · 2017-03-01T06:58:22Z

...pository-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageSettings.java

+     * Azure key
+     */
+    public static final AffixSetting<String> KEY_SETTING = Setting.affixKeySetting(PREFIX, "key",
+        key -> Setting.simpleString(key, Property.NodeScope));


I don't think we should add new settings like this which are unsecured. This and account above should be secure settings.

I agree but this is something I'm planning to do as a follow up instead of mixing it within this change.

That plan concerns me, as it means there is the (however distant) possibility these settings get released before being converted to secure settings.

Let's do it like this.

Can you review the first phase of this PR and then I can add more commits to implement the security part? So we merge only a good version?
Like if we were having a feature branch.

WDYT?

Well... Might not be a good idea. Ok. I'm going to add the commits to this PR then you will be able to review the whole change.

dadoonet · 2017-03-01T10:04:49Z

@rjernst I secured our azure key/secret within this PR, changed the documentation accordingly and also the title/description of this PR.
LMK what you think of it. Thanks!

dadoonet · 2017-03-16T10:33:16Z

@rjernst Could you please review this one? I'd like to merge it next week ideally.

dadoonet · 2017-03-30T14:57:47Z

I'm planning to merge this on friday, March 31st, unless anyone objects.

jasontedor · 2017-03-30T15:14:43Z

I'm planning to merge this on friday, March 31st, unless anyone objects.

I object to this, this is not how we should handle missing code reviews.

s1monw · 2017-03-31T09:48:09Z

@rjernst can you do a second pass on this please

rjernst

This looks ok, but I'm still unsure about merging client settings and legacy settings into one map of client name to settings.

rjernst · 2017-03-31T19:58:43Z

...itory-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageServiceImpl.java

        if (azureStorageSettings == null) {
            // We did not get an account. That's bad.
-            throw new IllegalArgumentException("Can not find azure account [" + account + "]. Check your elasticsearch.yml.");
+            throw new IllegalArgumentException("Can not find azure account [" + namedConfiguration + "]. Check your elasticsearch.yml.");


I think the message is a little deceptive, because it is not account, but the client name? And in the backcompat case, I think it may be confusing, because the client name is fake (ie primary or secondary, but a client configuration by that name does not actually exist).

rjernst · 2017-03-31T20:04:29Z

...ory-azure/src/main/java/org/elasticsearch/plugin/repository/azure/AzureRepositoryPlugin.java

+            AzureStorageService.Storage.STORAGE_ACCOUNTS,
+            // This is strange that deprecated group settings have never been registered before.
+            // How this worked? I was expecting something like:
+            // AzureStorageSettings.DEPRECATED_ACCOUNT_SETTING,


These should definitely be registered. Is there some other registration of a prefix that is masking validation of all of these settings?

rjernst · 2017-06-09T07:32:07Z

@dadoonet Is this something you still want to try to get in? Can you try to refactor the change to only us a single map of client name to client settings?

dadoonet · 2017-06-09T08:26:16Z

@rjernst Definitely yes. That's why I opened recently #25069. Now you fixed that I can move forward on this PR.

dadoonet · 2017-06-19T16:21:25Z

@rjernst I updated the PR. Could you review it?

dadoonet · 2017-07-28T12:15:51Z

I need to iterate again a bit more as the first tests I'm doing shows that actually it's a breaking change.

With an "old" elasticsearch.yml configuration:

cloud.azure.storage.my_account.account: ACCOUNT
cloud.azure.storage.my_account.key: KEY

When creating the repository:

curl -XDELETE 127.0.0.1:9200/_snapshot/azure?pretty
curl -XPUT '127.0.0.1:9200/_snapshot/azure?pretty' -H 'Content-Type: application/json' -d'
{
  "type": "azure"
}'

It is failing with:

{
  "error" : {
    "root_cause" : [
      {
        "type" : "illegal_argument_exception",
        "reason" : "Can not find named azure client [default]. Check your elasticsearch.yml."
      }
    ],
    "type" : "illegal_argument_exception",
    "reason" : "Can not find named azure client [default]. Check your elasticsearch.yml."
  },
  "status" : 400
}

Which means that the default legacy account is not found anymore if the account name is not default.

With:

cloud.azure.storage.default.account: ACCOUNT
cloud.azure.storage.default.key: KEY

It works well.

I'm going to fix that.

dadoonet · 2017-07-28T13:41:12Z

@rjernst I pushed a new commit to fix the problem I mentioned #23405 (comment)

Basically, it stores a "legacy" configuration under 2 names:

default which is the one we are defaulting to if no "client" is specified
the original name if not default so if someone is explicitly referencing that name in a repository configuration it will still work.

LMK

dadoonet · 2017-08-07T09:10:20Z

@rjernst Do you agree with the changes I proposed?

rjernst

LGTM, one small request for clarity.

rjernst · 2017-08-07T17:05:35Z

...pository-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageSettings.java

+     * @param settings settings to parse
+     * @return All the named configurations
+     */
+    public static Map<String, AzureStorageSettings> load(Settings settings) {


Can you please rename this method and/or parse to make it clear this method is for the new style settings and the other one is for the legacy settings?

…tory We should have the same behavior for Azure repositories as we have for S3 (see elastic#22762). Instead of: ```yml cloud: azure: storage: my_account1: account: your_azure_storage_account1 key: your_azure_storage_key1 default: true my_account2: account: your_azure_storage_account2 key: your_azure_storage_key2 ``` Support something like: ``` azure.client: default: account: your_azure_storage_account1 key: your_azure_storage_key1 my_account2: account: your_azure_storage_account2 key: your_azure_storage_key2 ``` Then instead of: ``` PUT _snapshot/my_backup3 { "type": "azure", "settings": { "account": "my_account2" } } ``` Use: ``` PUT _snapshot/my_backup3 { "type": "azure", "settings": { "config": "my_account2" } } ``` If someone uses: ``` PUT _snapshot/my_backup3 { "type": "azure" } ``` It will use the `default` azure repository settings. And mark as deprecated old settings. Closes elastic#22763.

…tory We should have the same behavior for Azure repositories as we have for S3 (see #22762). Instead of: ```yml cloud: azure: storage: my_account1: account: your_azure_storage_account1 key: your_azure_storage_key1 default: true my_account2: account: your_azure_storage_account2 key: your_azure_storage_key2 ``` Support something like: ``` azure.client: default: account: your_azure_storage_account1 key: your_azure_storage_key1 my_account2: account: your_azure_storage_account2 key: your_azure_storage_key2 ``` Then instead of: ``` PUT _snapshot/my_backup3 { "type": "azure", "settings": { "account": "my_account2" } } ``` Use: ``` PUT _snapshot/my_backup3 { "type": "azure", "settings": { "config": "my_account2" } } ``` If someone uses: ``` PUT _snapshot/my_backup3 { "type": "azure" } ``` It will use the `default` azure repository settings. And mark as deprecated old settings. Backport of #23405 in 6.x branch

dadoonet · 2017-08-08T13:31:02Z

Thanks @rjernst! I pushed it to master (7.0) and 6.x (6.1).
I'm going to add a follow up to remove the legacy settings in master branch (7.0).

Follow up for elastic#23405. We remove azure deprecated settings in 7.0: * The legacy azure settings which where starting with `cloud.azure.storage.` prefix have been removed. This includes `account`, `key`, `default` and `timeout`. You need to use settings which are starting with `azure.client.` prefix instead. * Global timeout setting `cloud.azure.storage.timeout` has been removed. You must set it per azure client instead. Like `azure.client.default.timeout: 10s` for example.

Follow up for #23405. We remove azure deprecated settings in 7.0: * The legacy azure settings which where starting with `cloud.azure.storage.` prefix have been removed. This includes `account`, `key`, `default` and `timeout`. You need to use settings which are starting with `azure.client.` prefix instead. * Global timeout setting `cloud.azure.storage.timeout` has been removed. You must set it per azure client instead. Like `azure.client.default.timeout: 10s` for example.

That was a missing part in elastic#23405.

* Use Azure upload method instead of our own implementation We are not following the Azure documentation about uploading blobs to Azure storage. https://docs.microsoft.com/en-us/azure/storage/blobs/storage-java-how-to-use-blob-storage#upload-a-blob-into-a-container Instead we are using our own implementation which might cause some troubles and rarely some blobs can be not immediately commited just after we close the stream. Using the standard implementation provided by Azure team should allow us to benefit from all the magic Azure SDK team already wrote. And well... Let's just read the doc! * Adapt integration tests to secure settings That was a missing part in #23405. * Simplify all the integration tests and *extends ESBlobStoreRepositoryIntegTestCase tests * removes IT `testForbiddenContainerName()` as it is useless. The plugin does not create anymore the container but expects that the user has created it before registering the repository * merges 2 IT classes so all IT tests are ran from one single class * We don't remove/create anymore the container between each single test but only for the test suite

* Use Azure upload method instead of our own implementation We are not following the Azure documentation about uploading blobs to Azure storage. https://docs.microsoft.com/en-us/azure/storage/blobs/storage-java-how-to-use-blob-storage#upload-a-blob-into-a-container Instead we are using our own implementation which might cause some troubles and rarely some blobs can be not immediately commited just after we close the stream. Using the standard implementation provided by Azure team should allow us to benefit from all the magic Azure SDK team already wrote. And well... Let's just read the doc! * Adapt integration tests to secure settings That was a missing part in #23405. * Simplify all the integration tests and *extends ESBlobStoreRepositoryIntegTestCase tests * removes IT `testForbiddenContainerName()` as it is useless. The plugin does not create anymore the container but expects that the user has created it before registering the repository * merges 2 IT classes so all IT tests are ran from one single class * We don't remove/create anymore the container between each single test but only for the test suite Backport of #26751 in 6.x branch

dadoonet · 2017-09-28T11:58:49Z

@rjernst @clintongormley I think I should also backport that one to 6.0. People will benefit from secured settings with 6.0 GA instead of having to wait for 6.1.

WDYT?

clintongormley · 2017-09-29T09:29:34Z

@dadoonet it's too late for 6.0. This can go into 6.1

) * Use Azure upload method instead of our own implementation We are not following the Azure documentation about uploading blobs to Azure storage. https://docs.microsoft.com/en-us/azure/storage/blobs/storage-java-how-to-use-blob-storage#upload-a-blob-into-a-container Instead we are using our own implementation which might cause some troubles and rarely some blobs can be not immediately commited just after we close the stream. Using the standard implementation provided by Azure team should allow us to benefit from all the magic Azure SDK team already wrote. And well... Let's just read the doc! * Adapt integration tests to secure settings That was a missing part in elastic#23405. * Simplify all the integration tests and *extends ESBlobStoreRepositoryIntegTestCase tests * removes IT `testForbiddenContainerName()` as it is useless. The plugin does not create anymore the container but expects that the user has created it before registering the repository * merges 2 IT classes so all IT tests are ran from one single class * We don't remove/create anymore the container between each single test but only for the test suite Backport of elastic#26751 in 6.0 branch

dadoonet added :Plugin Repository Azure :Core/Infra/Settings Settings infrastructure and APIs >deprecation v5.4.0 v6.0.0-alpha1 labels Feb 28, 2017

dadoonet self-assigned this Feb 28, 2017

dadoonet requested a review from rjernst February 28, 2017 11:27

jasontedor requested changes Feb 28, 2017

View reviewed changes

rjernst reviewed Mar 1, 2017

View reviewed changes

dadoonet changed the title ~~Azure repository: Move to named configurations as we do for S3 repository~~ Azure repository: Move to named configurations as we do for S3 repository and secure settings Mar 1, 2017

dadoonet added the review label Mar 16, 2017

rjernst requested changes Mar 31, 2017

View reviewed changes

clintongormley added v5.4.1 v6.0.0 and removed v5.4.0 v6.0.0-alpha1 labels Apr 28, 2017

clintongormley added v5.4.2 and removed v5.4.1 labels May 15, 2017

dadoonet force-pushed the pr/22763-azure-named-config branch from 9c0ca5d to 64065c6 Compare June 4, 2017 08:01

clintongormley added v5.4.3 and removed v5.4.2 labels Jun 14, 2017

dadoonet force-pushed the pr/22763-azure-named-config branch 3 times, most recently from 08d3f95 to 92473e2 Compare July 28, 2017 13:47

rjernst approved these changes Aug 7, 2017

View reviewed changes

dadoonet removed review v6.0.0 labels Aug 8, 2017

dadoonet force-pushed the pr/22763-azure-named-config branch from 92473e2 to 80b142d Compare August 8, 2017 13:15

dadoonet merged commit 80b142d into elastic:master Aug 8, 2017

dadoonet deleted the pr/22763-azure-named-config branch August 8, 2017 13:16

dadoonet mentioned this pull request Aug 8, 2017

Remove azure deprecated settings #26099

Merged

dadoonet added a commit to dadoonet/elasticsearch that referenced this pull request Sep 25, 2017

Adapt integration tests to secure settings

cb78313

That was a missing part in elastic#23405.

dadoonet mentioned this pull request Sep 28, 2017

Use Azure upload method instead of our own implementation #26751

Merged

clintongormley added :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs and removed :Plugin Repository Azure labels Feb 14, 2018

jpountz removed the :Core/Infra/Settings Settings infrastructure and APIs label Jan 29, 2019

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Azure repository: Move to named configurations as we do for S3 repository and secure settings #23405

Azure repository: Move to named configurations as we do for S3 repository and secure settings #23405

dadoonet commented Feb 28, 2017 •

edited

Loading

dadoonet commented Feb 28, 2017

jasontedor left a comment

jasontedor Feb 28, 2017 •

edited

Loading

rjernst Mar 1, 2017

dadoonet Mar 1, 2017

rjernst Mar 1, 2017

dadoonet Mar 1, 2017

dadoonet Mar 1, 2017

dadoonet commented Mar 1, 2017

dadoonet commented Mar 16, 2017

dadoonet commented Mar 30, 2017

jasontedor commented Mar 30, 2017

s1monw commented Mar 31, 2017

rjernst left a comment

rjernst Mar 31, 2017

rjernst Mar 31, 2017

rjernst commented Jun 9, 2017

dadoonet commented Jun 9, 2017 •

edited

Loading

dadoonet commented Jun 19, 2017

dadoonet commented Jul 28, 2017

dadoonet commented Jul 28, 2017

dadoonet commented Aug 7, 2017

rjernst left a comment

rjernst Aug 7, 2017

dadoonet commented Aug 8, 2017

dadoonet commented Sep 28, 2017

clintongormley commented Sep 29, 2017

Azure repository: Move to named configurations as we do for S3 repository and secure settings #23405

Azure repository: Move to named configurations as we do for S3 repository and secure settings #23405

Conversation

dadoonet commented Feb 28, 2017 • edited Loading

dadoonet commented Feb 28, 2017

jasontedor left a comment

Choose a reason for hiding this comment

jasontedor Feb 28, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadoonet commented Mar 1, 2017

dadoonet commented Mar 16, 2017

dadoonet commented Mar 30, 2017

jasontedor commented Mar 30, 2017

s1monw commented Mar 31, 2017

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst commented Jun 9, 2017

dadoonet commented Jun 9, 2017 • edited Loading

dadoonet commented Jun 19, 2017

dadoonet commented Jul 28, 2017

dadoonet commented Jul 28, 2017

dadoonet commented Aug 7, 2017

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadoonet commented Aug 8, 2017

dadoonet commented Sep 28, 2017

clintongormley commented Sep 29, 2017

dadoonet commented Feb 28, 2017 •

edited

Loading

jasontedor Feb 28, 2017 •

edited

Loading

dadoonet commented Jun 9, 2017 •

edited

Loading