Add proxy settings for GCS repository plugin #2096

willyborankin · 2022-02-12T16:32:29Z

Description

Now to configure GCS plugin to use any proxy you need to set standard process variables:

proxyHost
socksProxyHost
proxyPort
proxyUser
proxyPassword

with proxy types prefix.

Such settings have 2 main problems:

Storing password this way is insecure
In case of password leaking it is necessary to restart all nodes in the cluster

To avoid of a such problem all proxy settings were added to OS settings and security settings
Storing username/password in security settings is more secure compare to process settings and in case of any problem with username and password such security settings could be reloaded without restart of each node in the cluster.

Re-loadable security settings:

gcs.client.*.proxy.username - Proxy user name
gcs.client.*.proxy.password - Proxy user password

Static settings:

gcs.client.*.proxy.type - java Proxy.Type names: HTTP, SOCKS. default is DIRECT
gcs.client.*.proxy.host - Proxy host name
gcs.client.*.proxy.port - Proxy port

Note: Such properties can be added for S3 and Azure as well, so I'm planning to add theme separate PRs.

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

opensearch-ci-bot · 2022-02-12T16:34:02Z

Can one of the admins verify this patch?

opensearch-ci-bot · 2022-02-12T16:38:14Z

❌ Gradle Check failure 4e97e3de3d712669f00693dfdc2be47aa94a62c2
Log 2354

Reports 2354

opensearch-ci-bot · 2022-02-12T17:03:36Z

❌ Gradle Check failure 0d5765806e205fd76fea108e833b96cb7466018a
Log 2355

Reports 2355

opensearch-ci-bot · 2022-02-12T17:12:56Z

❌ Gradle Check failure e279defbb455a3da6e51e1b6dd0ded22b73e4239
Log 2356

Reports 2356

opensearch-ci-bot · 2022-02-12T18:06:20Z

❌ Gradle Check failure a0d064fdf844c426e5883dab6996efc5bec3dee1
Log 2357

Reports 2357

opensearch-ci-bot · 2022-02-12T19:05:24Z

✅ Gradle Check success 1c316a8178af843213e9eb3c1c0b7c05c1a79ed5
Log 2358

Reports 2358

opensearch-ci-bot · 2022-02-12T21:22:37Z

✅ Gradle Check success 60c1a19865ef0b812b568fb7132ab2477397ff14
Log 2359

Reports 2359

opensearch-ci-bot · 2022-02-12T22:10:59Z

✅ Gradle Check success f72a1812ba59f88b33ee570ba3975f675feb90bf
Log 2360

Reports 2360

opensearch-ci-bot · 2022-02-13T11:44:04Z

✅ Gradle Check success e1b3b886fc42e742607aea83388bd18be15a8b84
Log 2364

Reports 2364

opensearch-ci-bot · 2022-02-14T21:52:47Z

❌ Gradle Check failure 83573d5c509b34aa5289003213694870bd1918c7
Log 2385

Reports 2385

dblock · 2022-02-14T22:21:49Z

#2105

dblock · 2022-02-14T22:21:58Z

start gradle check

opensearch-ci-bot · 2022-02-14T22:44:58Z

❌ Gradle Check failure 83573d5c509b34aa5289003213694870bd1918c7
Log 2389

Reports 2389

kartg · 2022-02-15T01:03:56Z

#1957

kartg · 2022-02-15T01:04:04Z

start gradle check

opensearch-ci-bot · 2022-02-15T01:53:35Z

❌ Gradle Check failure 83573d5c509b34aa5289003213694870bd1918c7
Log 2395

Reports 2395

willyborankin · 2022-02-16T07:29:00Z

x Gradle Check failure 83573d5 Log 2395

Reports 2395

As far as I understand the tests failure is not related to this PR:

[MovePrimaryFirstTests](https://github.com/opensearch-project/OpenSearch/pull/classes/org.opensearch.cluster.routing.MovePrimaryFirstTests.html). [testClusterGreenAfterPartialRelocation](https://github.com/opensearch-project/OpenSearch/pull/classes/org.opensearch.cluster.routing.MovePrimaryFirstTests.html#testClusterGreenAfterPartialRelocation)

dblock · 2022-02-16T17:10:03Z

start gradle check

dblock

Is there logging missing similar to the Azure plugin for whether proxy is set vs. not?

opensearch-ci-bot · 2022-02-16T19:04:07Z

❌ Gradle Check failure 83573d5c509b34aa5289003213694870bd1918c7
Log 2458

Reports 2458

willyborankin · 2022-02-16T20:03:32Z

Is there logging missing similar to the Azure plugin for whether proxy is set vs. not?

let me check.

willyborankin · 2022-02-16T20:09:11Z

Is there logging missing similar to the Azure plugin for whether proxy is set vs. not?

Double checked all classes in org.opensearch.repositories.gcs no toString.

dblock · 2022-02-16T21:51:29Z

#2143

dblock · 2022-02-16T21:51:35Z

start gradle check

opensearch-ci-bot · 2022-02-16T22:28:50Z

❌ Gradle Check failure 83573d5c509b34aa5289003213694870bd1918c7
Log 2469

Reports 2469

kartg · 2022-02-16T23:01:35Z

...tory-gcs/src/main/java/org/opensearch/repositories/gcs/GoogleCloudStorageClientSettings.java

+    static final Setting.AffixSetting<Integer> PROXY_PORT_SETTING = Setting.affixKeySetting(
+        PREFIX,
+        "proxy.port",
+        key -> Setting.intSetting(key, 0, 0, 1 << 16, Setting.Property.NodeScope),


Super minor - the maxValue condition should really be (1 << 16) - 1

good point, will fix.

kartg · 2022-02-16T23:19:24Z

...tory-gcs/src/main/java/org/opensearch/repositories/gcs/GoogleCloudStorageClientSettings.java

+        // Validate proxy settings
+        if (proxyType == Proxy.Type.DIRECT
+            && (proxyPort != 0 || Strings.hasText(proxyHost) || Strings.hasText(proxyUserName) || Strings.hasText(proxyPassword))) {
+            throw new SettingsException(
+                "Google Cloud Storage proxy port or host or username or password have been set but proxy type is not defined."
+            );
+        }
+        if (proxyType != Proxy.Type.DIRECT && (proxyPort == 0 || Strings.isEmpty(proxyHost))) {
+            throw new SettingsException("Google Cloud Storage proxy type has been set but proxy host or port is not defined.");
+        }


Minor - given that this is identical to the logic introduced in #2098 , it might be worth it to move this to a common StorageSettings superclass and have the two classes inherit from this. We could also move getConfigValue into this parent class.

Yes and this is a problem. All these plugins do not have a common repo or something and as a result, all of them contains the same classes e.g. AccessSocket is almost the same and with the same functionality for all plugins. I was planning to open a discussion here about improvements for Cloud Plugins. And how we (I mean our company) can help with it.

Let's have that discussion in an issue so we can close out this PR. I've opened #2155 around it.

sure. will leave comments.

kartg · 2022-02-16T23:27:36Z

plugins/repository-gcs/src/main/java/org/opensearch/repositories/gcs/ProxySettings.java

+import java.net.Proxy;
+import java.util.Objects;
+
+public class ProxySettings {


Minor - Most of this is identical to the Azure ProxySettings class. Could you move the majority of this to a superclass and declare Azure and GCS specific subclasses?

same as above :-(

willyborankin · 2022-02-17T14:20:22Z

I'm a bit worried about test failures. Could it be that my changes somehow affected it? At a first glance, I did not change anything critical and the plugin is not involved in the failing test. But still not clear for me.

andrross · 2022-02-17T14:34:07Z

#2147 will fix the backward compatibility test failures. The other failures I see frequently are related to timeouts trying to download CentOS linux. I haven't seen any failures related to the GCS repository plugin.

opensearch-ci-bot · 2022-02-17T15:04:14Z

❌ Gradle Check failure b40dd7c9ef5646d52af7fc62b94e811e6e2191c0
Log 2495

Reports 2495

willyborankin · 2022-02-17T15:53:36Z

#2147 will fix the backward compatibility test failures. The other failures I see frequently are related to timeouts trying to download CentOS linux. I haven't seen any failures related to the GCS repository plugin.

Ok I see. Yes I see same failures for CentOS as well on my machine something with docker.

kartg · 2022-02-17T18:53:35Z

@willyborankin Could you rebase to pull in the fix in #2147 so we can confirm that Gradle_Check passes before we merge this change in?

Added proxy settings for GCS repository. Security settings: - gcs.client.*.proxy.username - Proxy user name - gcs.client.*.proxy.password - Proxy user password Common settings: - gcs.client.*.proxy.type - java Proxy.Type names: HTTP, SOCKS. default is DIRECT - gcs.client.*.proxy.host - Proxy host name - gcs.client.*.proxy.port - Proxy port Signed-off-by: Andrey Pleskach <[email protected]>

willyborankin · 2022-02-17T19:04:59Z

@willyborankin Could you rebase to pull in the fix in #2147 so we can confirm that Gradle_Check passes before we merge this change in?

done

opensearch-ci-bot · 2022-02-17T19:46:01Z

✅ Gradle Check success 533f57a
Log 2507

Reports 2507

willyborankin · 2022-02-21T10:24:56Z

the ticket for docs: opensearch-project/documentation-website#418

willyborankin force-pushed the gcs-proxy-support branch from 4e97e3d to 0d57658 Compare February 12, 2022 16:58

willyborankin force-pushed the gcs-proxy-support branch from 0d57658 to e279def Compare February 12, 2022 17:08

willyborankin force-pushed the gcs-proxy-support branch from e279def to a0d064f Compare February 12, 2022 18:01

willyborankin force-pushed the gcs-proxy-support branch from a0d064f to 1c316a8 Compare February 12, 2022 18:24

willyborankin marked this pull request as ready for review February 12, 2022 18:52

willyborankin requested a review from a team as a code owner February 12, 2022 18:52

willyborankin changed the title ~~Add proxy settings for GCS repository~~ Add proxy settings for GCS repository plugin Feb 12, 2022

willyborankin force-pushed the gcs-proxy-support branch from 1c316a8 to 60c1a19 Compare February 12, 2022 20:41

willyborankin force-pushed the gcs-proxy-support branch from 60c1a19 to f72a181 Compare February 12, 2022 21:32

willyborankin force-pushed the gcs-proxy-support branch from f72a181 to e1b3b88 Compare February 13, 2022 11:03

dblock requested review from adnapibar and kartg February 14, 2022 17:07

willyborankin force-pushed the gcs-proxy-support branch from e1b3b88 to 83573d5 Compare February 14, 2022 21:09

reta approved these changes Feb 14, 2022

View reviewed changes

dblock mentioned this pull request Feb 14, 2022

[BUG] CreateIndexIT.testRestartIndexCreationAfterFullClusterRestart intermittent failure #2105

Closed

kartg mentioned this pull request Feb 15, 2022

[BUG] Timeout on org.opensearch.cluster.routing.MovePrimaryFirstTests.testClusterGreenAfterPartialRelocation #1957

Closed

dblock reviewed Feb 16, 2022

View reviewed changes

dblock approved these changes Feb 16, 2022

View reviewed changes

dblock mentioned this pull request Feb 16, 2022

[BUG] BWC failures for aggregations in MixedClusterClientYamlTestSuiteIT #2143

Closed

kartg approved these changes Feb 16, 2022

View reviewed changes

willyborankin force-pushed the gcs-proxy-support branch from 83573d5 to b40dd7c Compare February 17, 2022 14:17

kartg mentioned this pull request Feb 17, 2022

Code Duplication in Cloud Plugins #2155

Open

willyborankin force-pushed the gcs-proxy-support branch from b40dd7c to 533f57a Compare February 17, 2022 19:02

kartg merged commit b9ff91d into opensearch-project:main Feb 17, 2022

Add proxy settings for GCS repository plugin #2096

Add proxy settings for GCS repository plugin #2096

Conversation

willyborankin commented Feb 12, 2022 • edited Loading

Description

Issues Resolved

Check List

opensearch-ci-bot commented Feb 12, 2022

opensearch-ci-bot commented Feb 12, 2022

opensearch-ci-bot commented Feb 12, 2022

opensearch-ci-bot commented Feb 12, 2022

opensearch-ci-bot commented Feb 12, 2022

opensearch-ci-bot commented Feb 12, 2022

opensearch-ci-bot commented Feb 12, 2022

opensearch-ci-bot commented Feb 12, 2022

opensearch-ci-bot commented Feb 13, 2022

opensearch-ci-bot commented Feb 14, 2022

dblock commented Feb 14, 2022

dblock commented Feb 14, 2022

opensearch-ci-bot commented Feb 14, 2022

kartg commented Feb 15, 2022

kartg commented Feb 15, 2022

opensearch-ci-bot commented Feb 15, 2022

willyborankin commented Feb 16, 2022

dblock commented Feb 16, 2022

dblock left a comment

Choose a reason for hiding this comment

opensearch-ci-bot commented Feb 16, 2022

willyborankin commented Feb 16, 2022

willyborankin commented Feb 16, 2022

dblock commented Feb 16, 2022

dblock commented Feb 16, 2022

opensearch-ci-bot commented Feb 16, 2022

kartg Feb 16, 2022

Choose a reason for hiding this comment

willyborankin Feb 17, 2022

Choose a reason for hiding this comment

kartg Feb 16, 2022

Choose a reason for hiding this comment

willyborankin Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

kartg Feb 17, 2022

Choose a reason for hiding this comment

willyborankin Feb 17, 2022

Choose a reason for hiding this comment

kartg Feb 16, 2022

Choose a reason for hiding this comment

willyborankin Feb 17, 2022

Choose a reason for hiding this comment

willyborankin commented Feb 17, 2022

andrross commented Feb 17, 2022

opensearch-ci-bot commented Feb 17, 2022

willyborankin commented Feb 17, 2022

kartg commented Feb 17, 2022

willyborankin commented Feb 17, 2022

opensearch-ci-bot commented Feb 17, 2022

willyborankin commented Feb 21, 2022

willyborankin commented Feb 12, 2022 •

edited

Loading

willyborankin Feb 17, 2022 •

edited

Loading