Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for accessing Azure repositories through a proxy #23518

Merged
merged 9 commits into from Sep 13, 2017

Conversation

@dadoonet
Copy link
Member

dadoonet commented Mar 9, 2017

You can define a proxy using the following settings:

azure.client.default.proxy.host: proxy.host
azure.client.default.proxy.port: 8888
azure.client.default.proxy.type: http

Supported values for proxy.type are http or socks. Defaults to no proxy.

Note that this commit depends on #23517 which needs to be merged first (upgrade azure client to 5.0.0).

Closes #23506

@dadoonet

This comment has been minimized.

Copy link
Member Author

dadoonet commented Mar 16, 2017

@tlrx Once you have reviewed #23517, could you review this one please?

@dadoonet

This comment has been minimized.

Copy link
Member Author

dadoonet commented Mar 30, 2017

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

@jasontedor

This comment has been minimized.

Copy link
Member

jasontedor commented Mar 30, 2017

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

I object to this, this requires a review.

Copy link
Member

danielmitterdorfer left a comment

I left a couple of very minor comments. However, I think it'd be good to add a test?

...pository-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageSettings.java Outdated
this.name = name;
this.account = account;
this.key = key;
this.timeout = timeout;
this.activeByDefault = activeByDefault;
if (proxyType.equals(Proxy.Type.DIRECT)) {
proxy = null;

This comment has been minimized.

Copy link
@danielmitterdorfer

danielmitterdorfer Mar 30, 2017

Member

nit: every field assignment uses this except for proxy. I though initially that it's a local variable. Maybe use this.proxy instead of just proxy?

...pository-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageSettings.java Outdated
@@ -43,20 +45,43 @@
Setting.affixKeySetting(Storage.PREFIX, "key", (key) -> Setting.simpleString(key, Setting.Property.NodeScope));
private static final Setting<Boolean> DEFAULT_SETTING =
Setting.affixKeySetting(Storage.PREFIX, "default", (key) -> Setting.boolSetting(key, false, Setting.Property.NodeScope));
/** The host name of a proxy to connect to s3 through. */

This comment has been minimized.

Copy link
@danielmitterdorfer

danielmitterdorfer Mar 30, 2017

Member

Nit: copy & paste error: "The host name of a proxy to connect to azure through." (also: shouldn't it be "Azure" instead of "azure"?)

Copy link
Member

tlrx left a comment

Sorry @dadoonet, this review request ran out of my scope.

I left some comments, nothing big. I'd like a test for this too as @danielmitterdorfer already suggested.

docs/plugins/repository-azure.asciidoc Outdated
type: http
----

Supported values for `proxy.type` are `direct`, `http` or `socks`. Defaults to `direct` (no proxy).

This comment has been minimized.

Copy link
@tlrx

tlrx Mar 31, 2017

Member

I don't think we need to document the proxy.type direct, because it actually does nothing on Azure client side. We could instead check that proxy.type must be either http or socks and require non null host and port if type is set? Similarly, if host (or port) is set, port (or host) and type are required?

...pository-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageSettings.java Outdated
if (proxyType.equals(Proxy.Type.DIRECT)) {
proxy = null;
} else {
proxy = new Proxy(proxyType, new InetSocketAddress(proxyHost, proxyPort));

This comment has been minimized.

Copy link
@tlrx

tlrx Mar 31, 2017

Member

I think we need to check that proxyHost is non null? Otherwise it's going to use 0.0.0.0 as address.

...pository-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageSettings.java Outdated
Setting.affixKeySetting(Storage.PREFIX, "proxy.host", (key) -> Setting.simpleString(key, Setting.Property.NodeScope));
/** The port of a proxy to connect to azure through. */
private static final Setting<Integer> PROXY_PORT_SETTING = Setting.affixKeySetting(Storage.PREFIX, "proxy.port",
key -> Setting.intSetting(key, 80, 0, 1<<16, Setting.Property.NodeScope));

This comment has been minimized.

Copy link
@tlrx

tlrx Mar 31, 2017

Member

Maximum value allowed here is 65536 but later in the code new InetSocketAddress(proxyHost, proxyPort)) checks that A valid port value is between 0 and 65535. Max value here should be 65535.

...pository-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageSettings.java Outdated
private final Proxy.Type proxyType;
private final Proxy proxy;

@SuppressForbidden(reason = "we know we pass a IP address or a host name")

This comment has been minimized.

Copy link
@rjernst

rjernst Mar 31, 2017

Member

Why is this suppression here? The "reason" doesn't really explain anything. I assume it is because of call to construction of InetSocketAddress below. But this is not a valid reason to suppress; the message with forbidden apis clearly states you should look up the address of the host yourself with InetAddress, and then use that ctor for InetSocketAddress.

This comment has been minimized.

Copy link
@dadoonet

dadoonet May 4, 2017

Author Member

Makes sense. Should I simply throw the UnknownHostException here or catch it and wrap it into another Exception?

This comment has been minimized.

Copy link
@rjernst

rjernst May 5, 2017

Member

I would just let the UnknownHostException bubble up. Wrapping just creates longer stacktraces that are more difficult to read.

@dadoonet dadoonet added v5.5.0 and removed v5.4.1 labels May 4, 2017
@clintongormley

This comment has been minimized.

Copy link
Member

clintongormley commented May 26, 2017

@dadoonet are you planning on coming back to this?

@dadoonet

This comment has been minimized.

Copy link
Member Author

dadoonet commented May 26, 2017

Definitely yes

dadoonet added 3 commits Mar 9, 2017
You can define a proxy using the following settings:

```yml
cloud:
    azure:
        storage:
            my_account:
                account: your_azure_storage_account
                key: your_azure_storage_key
                proxy:
                    host: proxy.host
                    port: 8888
                    type: http
```

Supported values for `proxy.type` are `direct`, `http` or `socks`. Defaults to `direct` (no proxy).

Note that this commit depends on #23517 which needs to be merged first (upgrade azure client to 5.0.0).

Closes #23506
@dadoonet dadoonet force-pushed the dadoonet:pr/23506-azure-proxy branch to d3e7d35 Jun 2, 2017
dadoonet added 3 commits Jun 2, 2017
Well actually we can only register a default proxy for all azure
requests whatever the client is.

So instead of setting the proxy within a client definition we now
define it globally with:

```
cloud.azure.storage.proxy.type: http
cloud.azure.storage.proxy.host: 127.0.0.1
cloud.azure.storage.proxy.port: 8080
```
@dadoonet

This comment has been minimized.

Copy link
Member Author

dadoonet commented Jun 2, 2017

While I was working on it, I realized that you can actually define only a proxy globally and not per client instance which is not a big deal IMO.

But then it was not making sense anymore to define proxy settings per client. So I changed it and now you define proxy settings globally whatever the credentials you are going to use:

cloud.azure.storage.proxy:
    host: proxy.host
    port: 8888
    type: http

I updated the description of this PR accordingly.

I added some tests about proxy settings configuration but I did not add any integration test which uses a Proxy behind the scene.
If you want me to implement such a thing, can you give me some pointers if we already did something like this in the code (simulating a Proxy in IT).

Thanks!

@rjernst

This comment has been minimized.

Copy link
Member

rjernst commented Jun 9, 2017

@dadoonet Is this something you still want to get in? I'm a little hesistant of having these proxy settings differ from those in repository-s3 (which are per client). Also, what about secure proxies? For repository-s3 we have the username/password as secure settings for the proxy.

@clintongormley clintongormley added v6.1.0 and removed v5.6.0 labels Jul 25, 2017
@rjernst

This comment has been minimized.

Copy link
Member

rjernst commented Sep 11, 2017

@dadoonet I'm sorry for the delayed reply. I think your suggestion of using OperationContext.setProxy sounds like something reasonable to try in order to achieve per client proxies to match those we have in repository-s3. Do you want to try that and bring this PR up to date?

@dadoonet dadoonet added v7.0.0 and removed v6.0.0 v6.1.0 labels Sep 12, 2017
dadoonet added 2 commits Sep 12, 2017
# Conflicts:
#	plugins/repository-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageService.java
#	plugins/repository-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageServiceImpl.java
#	plugins/repository-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageSettings.java
#	plugins/repository-azure/src/main/java/org/elasticsearch/plugin/repository/azure/AzureRepositoryPlugin.java
#	plugins/repository-azure/src/test/java/org/elasticsearch/cloud/azure/storage/AzureStorageServiceTests.java
#	plugins/repository-azure/src/test/java/org/elasticsearch/repositories/azure/AzureSnapshotRestoreTests.java
Instead of having a global client, we can define every time an operation context for each Azure call.

With that, we can control what is the proxy to use for the next request.

Also this commit is merging changes from master as we now have prefix settings under `azure.client.`.

BTW I changed a test `testGetSelectedClientBackoffPolicyNbRetries` as it was using an old setting name `cloud.azure.storage.azure.max_retries` instead of `azure.client.azure1.max_retries`.
@dadoonet

This comment has been minimized.

Copy link
Member Author

dadoonet commented Sep 12, 2017

@rjernst I pushed a new change. I think it's definitely better than the global setting. BTW the new implementation will allow us to have a finer control to the Azure API calls if needed in the future as we now call azureSdk.performXXX(null /* accessCondition */, /* options */, operationContext) instead of azureSdk.performXXX() and the operationContext is now build from the azure Settings (generateOperationContext(clientName) method).

BTW as a next step not related to this change, I'd like to squash all the packages we have in one single o.e.repositories.azure.

Copy link
Member

rjernst left a comment

LGTM, thanks @dadoonet

key -> SecureSetting.secureString(key, null));
/** Azure account name */
public static final AffixSetting<SecureString> ACCOUNT_SETTING =
Setting.affixKeySetting(PREFIX, "account",key -> SecureSetting.secureString(key, null));

This comment has been minimized.

Copy link
@rjernst

rjernst Sep 13, 2017

Member

nit: space after "account",

public static final AffixSetting<SecureString> KEY_SETTING = Setting.affixKeySetting(PREFIX, "key",
key -> SecureSetting.secureString(key, null));

public static final AffixSetting<TimeValue> TIMEOUT_SETTING = Setting.affixKeySetting(PREFIX, "timeout",
(key) -> Setting.timeSetting(key, TimeValue.timeValueMinutes(-1), Property.NodeScope));

/** The type of the proxy to connect to azure through. Can be direct (no proxy, default), http or socks */
public static final AffixSetting<Proxy.Type> PROXY_TYPE_SETTING = Setting.affixKeySetting(PREFIX, "proxy.type",
(key) -> new Setting<>(key, "direct", s -> Proxy.Type.valueOf(s.toUpperCase(Locale.ROOT)), Property.NodeScope));

This comment has been minimized.

Copy link
@rjernst

rjernst Sep 13, 2017

Member

direct is not one of the values in the docs added for proxy.type in this PR?

@dadoonet dadoonet merged commit a34db4e into elastic:master Sep 13, 2017
1 of 2 checks passed
1 of 2 checks passed
elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details
@dadoonet dadoonet deleted the dadoonet:pr/23506-azure-proxy branch Sep 13, 2017
@dadoonet dadoonet added the v6.1.0 label Sep 13, 2017
dadoonet added a commit that referenced this pull request Sep 13, 2017
You can define a proxy using the following settings:

```yml
azure.client.default.proxy.host: proxy.host
azure.client.default.proxy.port: 8888
azure.client.default.proxy.type: http
```

Supported values for `proxy.type` are `direct`, `http` or `socks`. Defaults to `direct` (no proxy).

Closes #23506

BTW I changed a test `testGetSelectedClientBackoffPolicyNbRetries` as it was using an old setting name `cloud.azure.storage.azure.max_retries` instead of `azure.client.azure1.max_retries`.

Backport of #23518 in 6.x branch.
@dadoonet

This comment has been minimized.

Copy link
Member Author

dadoonet commented Sep 13, 2017

Thanks @rjernst! Pushed in master and 6.x.

dadoonet added a commit to dadoonet/elasticsearch that referenced this pull request Sep 13, 2017
As we did for S3, we can collapse all packages within one single `org.elasticsearch.repositories.azure` package name.

Follow up for elastic#23518 (comment)
jasontedor added a commit to jpountz/elasticsearch that referenced this pull request Sep 13, 2017
* master: (21 commits)
  Ensure module is bundled before installing in tests
  Add boolean similarity to built in similarity types (elastic#26613)
  [Tests] Remove skip tests in search/30_limits.yml
  Let search phases override max concurrent requests
  Add a soft limit for the number of requested doc-value fields (elastic#26574)
  Support for accessing Azure repositories through a proxy (elastic#23518)
  Add beta tag to MSI Windows Installer (elastic#26616)
  Fix Lucene version of 5.6.1.
  Remove azure deprecated settings (elastic#26099)
  Handle the 5.6.0 release
  Allow plugins to validate cluster-state on join (elastic#26595)
  Remove index mapper dynamic settings (elastic#25734)
  update AWS SDK for ECS Task IAM support in discovery-ec2 (elastic#26479)
  Azure repository: Accelerate the listing of files (used in delete snapshot) (elastic#25710)
  Build: Remove norelease from forbidden patterns (elastic#26592)
  Fix reference to painless inside expression engine (elastic#26528)
  Build: Move javadoc linking to root build.gradle (elastic#26529)
  Test: Remove leftover static bwc test case (elastic#26584)
  Docs: Remove remaining references to file and native scripts (elastic#26580)
  Snapshot fallback should consider build.snapshot
  ...
dadoonet added a commit that referenced this pull request Sep 14, 2017
As we did for S3, we can collapse all packages within one single `org.elasticsearch.repositories.azure` package name.

Follow up for #23518 (comment)
dadoonet added a commit to dadoonet/elasticsearch that referenced this pull request Feb 21, 2018
When someone migrates from 5.6 to 6.x with deprecated settings like:

```
cloud:
    azure:
        storage:
            foo:
                account: <my_account>
                key: <my_key>
```

It produces a NPE anytime someone wants to use a repository which name is not `default`.

This has been introduced by elastic#23518 when I backported it to 6.x branch.
In this case, when we generate an OperationContext, we only try to get azure settings from "normal" `storageSettings` with:

```java
this.storageSettings.get(clientName)
```

But in the context of deprecated settings, this returns `null` which causes the NPE just after.

This commit adds a check and if no settings are found in the "normal" `storageSettings`, we look at settings from `deprecatedStorageSettings`.
The reason I missed it in the 7.0 version (master branch) is because actually `deprecatedStorageSettings` has been removed there already.

Also I modified the `testGetSelectedClientDefault` method which was only doing exactly the same thing as `testGetDefaultClientWithPrimaryAndSecondaries`.

Closes elastic#28299.
dadoonet added a commit that referenced this pull request Feb 26, 2018
* Fix NPE when using deprecated Azure settings

When someone migrates from 5.6 to 6.x with deprecated settings like:

```
cloud:
    azure:
        storage:
            foo:
                account: <my_account>
                key: <my_key>
```

It produces a NPE anytime someone wants to use a repository which name is not `default`.

This has been introduced by #23518 when I backported it to 6.x branch.
In this case, when we generate an OperationContext, we only try to get azure settings from "normal" `storageSettings` with:

```java
this.storageSettings.get(clientName)
```

But in the context of deprecated settings, this returns `null` which causes the NPE just after.

This commit adds a check and if no settings are found in the "normal" `storageSettings`, we look at settings from `deprecatedStorageSettings`.
The reason I missed it in the 7.0 version (master branch) is because actually `deprecatedStorageSettings` has been removed there already.

Also I renamed the `testGetSelectedClientDefault` method to `testGenerateOperationContext`
as it was only doing exactly the same thing as `testGetDefaultClientWithPrimaryAndSecondaries`.

Closes #28299.
dadoonet added a commit that referenced this pull request Feb 27, 2018
* Fix NPE when using deprecated Azure settings

When someone migrates from 5.6 to 6.x with deprecated settings like:

```
cloud:
    azure:
        storage:
            foo:
                account: <my_account>
                key: <my_key>
```

It produces a NPE anytime someone wants to use a repository which name is not `default`.

This has been introduced by #23518 when I backported it to 6.x branch.
In this case, when we generate an OperationContext, we only try to get azure settings from "normal" `storageSettings` with:

```java
this.storageSettings.get(clientName)
```

But in the context of deprecated settings, this returns `null` which causes the NPE just after.

This commit adds a check and if no settings are found in the "normal" `storageSettings`, we look at settings from `deprecatedStorageSettings`.
The reason I missed it in the 7.0 version (master branch) is because actually `deprecatedStorageSettings` has been removed there already.

Also I renamed the `testGetSelectedClientDefault` method to `testGenerateOperationContext`
as it was only doing exactly the same thing as `testGetDefaultClientWithPrimaryAndSecondaries`.

Closes #28299.

Backport of #28769 in 6.2 branch.
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.