From 215c9c617d3c7fb035b7654bebb4deb2c0faaa7b Mon Sep 17 00:00:00 2001 From: Pavel Kovalenko Date: Thu, 7 May 2020 17:01:58 +0300 Subject: [PATCH 1/7] Update AWS module. --- contrib/aws | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/aws b/contrib/aws index 04d54dfa034..fb5c604525f 160000 --- a/contrib/aws +++ b/contrib/aws @@ -1 +1 @@ -Subproject commit 04d54dfa0342d9465fb2eb3bfd4b77a3f7682e99 +Subproject commit fb5c604525f5151d75a856462653e7e38b559b79 From 6165c2aa991639ef8b4581b77ff5cc76d595c6a0 Mon Sep 17 00:00:00 2001 From: Pavel Kovalenko Date: Fri, 8 May 2020 13:53:12 +0300 Subject: [PATCH 2/7] Add possibility to use proxy resolver in DiskS3. --- .../compose/docker_compose_minio.yml | 7 ++ src/Disks/S3/DiskS3.cpp | 2 +- src/Disks/S3/DiskS3.h | 6 +- src/Disks/S3/DynamicProxyConfiguration.h | 24 ------ src/Disks/S3/ProxyConfiguration.h | 18 +++++ ...uration.cpp => ProxyListConfiguration.cpp} | 7 +- src/Disks/S3/ProxyListConfiguration.h | 23 ++++++ src/Disks/S3/ProxyResolverConfiguration.cpp | 59 ++++++++++++++ src/Disks/S3/ProxyResolverConfiguration.h | 28 +++++++ src/Disks/S3/registerDiskS3.cpp | 76 ++++++++++++++----- src/Disks/S3/ya.make | 3 +- tests/integration/helpers/cluster.py | 53 +++++++------ .../configs/config.d/storage_conf.xml | 20 +++++ .../proxy-resolver/__init__.py | 0 .../proxy-resolver/entrypoint.sh | 4 + .../proxy-resolver/resolver.py | 13 ++++ tests/integration/test_s3_with_proxy/test.py | 23 +++++- 17 files changed, 288 insertions(+), 78 deletions(-) delete mode 100644 src/Disks/S3/DynamicProxyConfiguration.h create mode 100644 src/Disks/S3/ProxyConfiguration.h rename src/Disks/S3/{DynamicProxyConfiguration.cpp => ProxyListConfiguration.cpp} (55%) create mode 100644 src/Disks/S3/ProxyListConfiguration.h create mode 100644 src/Disks/S3/ProxyResolverConfiguration.cpp create mode 100644 src/Disks/S3/ProxyResolverConfiguration.h create mode 100644 tests/integration/test_s3_with_proxy/proxy-resolver/__init__.py create mode 100644 tests/integration/test_s3_with_proxy/proxy-resolver/entrypoint.sh create mode 100644 tests/integration/test_s3_with_proxy/proxy-resolver/resolver.py diff --git a/docker/test/integration/compose/docker_compose_minio.yml b/docker/test/integration/compose/docker_compose_minio.yml index 5b93369e3e2..ec35c87fa00 100644 --- a/docker/test/integration/compose/docker_compose_minio.yml +++ b/docker/test/integration/compose/docker_compose_minio.yml @@ -38,5 +38,12 @@ services: ports: - "4082:8888" +# Empty container to run proxy resolver. + resolver: + image: python:3 + ports: + - "4083:8080" + tty: true + volumes: data1-1: diff --git a/src/Disks/S3/DiskS3.cpp b/src/Disks/S3/DiskS3.cpp index 47ca8231001..7643516e197 100644 --- a/src/Disks/S3/DiskS3.cpp +++ b/src/Disks/S3/DiskS3.cpp @@ -391,7 +391,7 @@ private: DiskS3::DiskS3( String name_, std::shared_ptr client_, - std::shared_ptr proxy_configuration_, + std::shared_ptr proxy_configuration_, String bucket_, String s3_root_path_, String metadata_path_, diff --git a/src/Disks/S3/DiskS3.h b/src/Disks/S3/DiskS3.h index 889a6aa97a3..9930a83fd2b 100644 --- a/src/Disks/S3/DiskS3.h +++ b/src/Disks/S3/DiskS3.h @@ -1,7 +1,7 @@ #pragma once #include "Disks/DiskFactory.h" -#include "DynamicProxyConfiguration.h" +#include "ProxyListConfiguration.h" #include #include @@ -22,7 +22,7 @@ public: DiskS3( String name_, std::shared_ptr client_, - std::shared_ptr proxy_configuration_, + std::shared_ptr proxy_configuration_, String bucket_, String s3_root_path_, String metadata_path_, @@ -102,7 +102,7 @@ private: private: const String name; std::shared_ptr client; - std::shared_ptr proxy_configuration; + std::shared_ptr proxy_configuration; const String bucket; const String s3_root_path; const String metadata_path; diff --git a/src/Disks/S3/DynamicProxyConfiguration.h b/src/Disks/S3/DynamicProxyConfiguration.h deleted file mode 100644 index 17eb2f0bb9e..00000000000 --- a/src/Disks/S3/DynamicProxyConfiguration.h +++ /dev/null @@ -1,24 +0,0 @@ -#pragma once - -#include -#include -#include -#include - -namespace DB::S3 -{ -class DynamicProxyConfiguration -{ -public: - explicit DynamicProxyConfiguration(std::vector _proxies); - /// Returns proxy configuration on each HTTP request. - Aws::Client::ClientConfigurationPerRequest getConfiguration(const Aws::Http::HttpRequest & request); - -private: - /// List of configured proxies. - const std::vector proxies; - /// Access counter to get proxy using round-robin strategy. - std::atomic access_counter; -}; - -} diff --git a/src/Disks/S3/ProxyConfiguration.h b/src/Disks/S3/ProxyConfiguration.h new file mode 100644 index 00000000000..62aec0e005e --- /dev/null +++ b/src/Disks/S3/ProxyConfiguration.h @@ -0,0 +1,18 @@ +#pragma once + +#include +#include +#include +#include + +namespace DB::S3 +{ +class ProxyConfiguration +{ +public: + virtual ~ProxyConfiguration() = default; + /// Returns proxy configuration on each HTTP request. + virtual Aws::Client::ClientConfigurationPerRequest getConfiguration(const Aws::Http::HttpRequest & request) = 0; +}; + +} diff --git a/src/Disks/S3/DynamicProxyConfiguration.cpp b/src/Disks/S3/ProxyListConfiguration.cpp similarity index 55% rename from src/Disks/S3/DynamicProxyConfiguration.cpp rename to src/Disks/S3/ProxyListConfiguration.cpp index 92a38762e39..bc68635fa7b 100644 --- a/src/Disks/S3/DynamicProxyConfiguration.cpp +++ b/src/Disks/S3/ProxyListConfiguration.cpp @@ -1,21 +1,22 @@ -#include "DynamicProxyConfiguration.h" +#include "ProxyListConfiguration.h" #include #include namespace DB::S3 { -DynamicProxyConfiguration::DynamicProxyConfiguration(std::vector _proxies) : proxies(std::move(_proxies)), access_counter(0) +ProxyListConfiguration::ProxyListConfiguration(std::vector proxies_) : proxies(std::move(proxies_)), access_counter(0) { } -Aws::Client::ClientConfigurationPerRequest DynamicProxyConfiguration::getConfiguration(const Aws::Http::HttpRequest &) +Aws::Client::ClientConfigurationPerRequest ProxyListConfiguration::getConfiguration(const Aws::Http::HttpRequest &) { /// Avoid atomic increment if number of proxies is 1. size_t index = proxies.size() > 1 ? (access_counter++) % proxies.size() : 0; Aws::Client::ClientConfigurationPerRequest cfg; + cfg.proxyScheme = Aws::Http::SchemeMapper::FromString(proxies[index].getScheme().c_str()); cfg.proxyHost = proxies[index].getHost(); cfg.proxyPort = proxies[index].getPort(); diff --git a/src/Disks/S3/ProxyListConfiguration.h b/src/Disks/S3/ProxyListConfiguration.h new file mode 100644 index 00000000000..a3fe83bfc49 --- /dev/null +++ b/src/Disks/S3/ProxyListConfiguration.h @@ -0,0 +1,23 @@ +#pragma once + +#include "ProxyConfiguration.h" + +namespace DB::S3 +{ +/** + * For each request to S3 it chooses a proxy from the specified list using round-robin strategy. + */ +class ProxyListConfiguration : public ProxyConfiguration +{ +public: + explicit ProxyListConfiguration(std::vector proxies_); + Aws::Client::ClientConfigurationPerRequest getConfiguration(const Aws::Http::HttpRequest & request) override; + +private: + /// List of configured proxies. + const std::vector proxies; + /// Access counter to get proxy using round-robin strategy. + std::atomic access_counter; +}; + +} diff --git a/src/Disks/S3/ProxyResolverConfiguration.cpp b/src/Disks/S3/ProxyResolverConfiguration.cpp new file mode 100644 index 00000000000..dd5af0990b8 --- /dev/null +++ b/src/Disks/S3/ProxyResolverConfiguration.cpp @@ -0,0 +1,59 @@ +#include "ProxyResolverConfiguration.h" + +#include +#include "Poco/StreamCopier.h" +#include +#include +#include + +namespace DB::S3 +{ +ProxyResolverConfiguration::ProxyResolverConfiguration(const Poco::URI & endpoint_, String proxy_scheme_, unsigned proxy_port_) + : endpoint(endpoint_), proxy_scheme(std::move(proxy_scheme_)), proxy_port(proxy_port_) +{ +} + +Aws::Client::ClientConfigurationPerRequest ProxyResolverConfiguration::getConfiguration(const Aws::Http::HttpRequest &) +{ + LOG_DEBUG(&Logger::get("AWSClient"), "Obtain proxy using resolver: " << endpoint.toString()); + + /// 1 second is enough for now. + /// TODO: Make timeouts configurable. + ConnectionTimeouts timeouts( + Poco::Timespan(1000000), /// Connection timeout. + Poco::Timespan(1000000), /// Send timeout. + Poco::Timespan(1000000) /// Receive timeout. + ); + auto session = makeHTTPSession(endpoint, timeouts); + + Aws::Client::ClientConfigurationPerRequest cfg; + try + { + /// It should be just empty GET / request. + Poco::Net::HTTPRequest request(Poco::Net::HTTPRequest::HTTP_1_1); + session->sendRequest(request); + + Poco::Net::HTTPResponse response; + auto & response_body_stream = session->receiveResponse(response); + + String proxy_host; + /// Read proxy host as string from response body. + Poco::StreamCopier::copyToString(response_body_stream, proxy_host); + + LOG_DEBUG(&Logger::get("AWSClient"), "Use proxy: " << proxy_scheme << "://" << proxy_host << ":" << proxy_port); + + cfg.proxyScheme = Aws::Http::SchemeMapper::FromString(proxy_scheme.c_str()); + cfg.proxyHost = proxy_host; + cfg.proxyPort = proxy_port; + + return cfg; + } + catch (Exception & e) + { + LOG_ERROR(&Logger::get("AWSClient"), "Failed to obtain proxy: " << e.message()); + /// Don't use proxy if it can't be obtained. + return cfg; + } +} + +} diff --git a/src/Disks/S3/ProxyResolverConfiguration.h b/src/Disks/S3/ProxyResolverConfiguration.h new file mode 100644 index 00000000000..2944121a078 --- /dev/null +++ b/src/Disks/S3/ProxyResolverConfiguration.h @@ -0,0 +1,28 @@ +#pragma once + +#include "ProxyConfiguration.h" +#include + +namespace DB::S3 +{ +/** + * Proxy configuration where proxy host is obtained each time from specified endpoint. + * For each request to S3 it makes GET request to specified endpoint and reads proxy host from a response body. + * Specified scheme and port added to obtained proxy host to form completed proxy URL. + */ +class ProxyResolverConfiguration : public ProxyConfiguration +{ +public: + ProxyResolverConfiguration(const Poco::URI & endpoint_, String proxy_scheme_, unsigned proxy_port_); + Aws::Client::ClientConfigurationPerRequest getConfiguration(const Aws::Http::HttpRequest & request) override; + +private: + /// Endpoint to obtain a proxy host. + const Poco::URI endpoint; + /// Scheme for obtained proxy. + const String proxy_scheme; + /// Port for obtained proxy. + const unsigned proxy_port; +}; + +} diff --git a/src/Disks/S3/registerDiskS3.cpp b/src/Disks/S3/registerDiskS3.cpp index ae753a2e9c3..c4eb877f30e 100644 --- a/src/Disks/S3/registerDiskS3.cpp +++ b/src/Disks/S3/registerDiskS3.cpp @@ -6,7 +6,9 @@ #include #include "DiskS3.h" #include "Disks/DiskFactory.h" -#include "DynamicProxyConfiguration.h" +#include "ProxyConfiguration.h" +#include "ProxyListConfiguration.h" +#include "ProxyResolverConfiguration.h" namespace DB { @@ -35,35 +37,67 @@ namespace void checkRemoveAccess(IDisk & disk) { disk.remove("test_acl"); } - std::shared_ptr getProxyConfiguration(const Poco::Util::AbstractConfiguration * config) + std::shared_ptr getProxyResolverConfiguration(const Poco::Util::AbstractConfiguration * proxy_resolver_config) { - if (config->has("proxy")) - { - std::vector keys; - config->keys("proxy", keys); + auto endpoint = Poco::URI(proxy_resolver_config->getString("endpoint")); + auto scheme = proxy_resolver_config->getString("scheme"); + if (scheme != "http" && scheme != "https") + throw Exception("Only HTTP/HTTPS schemas allowed in proxy resolver config: " + scheme, ErrorCodes::BAD_ARGUMENTS); + auto port = proxy_resolver_config->getUInt("port"); - std::vector proxies; - for (const auto & key : keys) - if (startsWith(key, "uri")) - { - Poco::URI proxy_uri(config->getString("proxy." + key)); + LOG_DEBUG( + &Logger::get("DiskS3"), "Configured proxy resolver: " << endpoint.toString() << ", Scheme: " << scheme << ", Port: " << port); - if (proxy_uri.getScheme() != "http") - throw Exception("Only HTTP scheme is allowed in proxy configuration at the moment, proxy uri: " + proxy_uri.toString(), ErrorCodes::BAD_ARGUMENTS); - if (proxy_uri.getHost().empty()) - throw Exception("Empty host in proxy configuration, proxy uri: " + proxy_uri.toString(), ErrorCodes::BAD_ARGUMENTS); + return std::make_shared(endpoint, scheme, port); + } - proxies.push_back(proxy_uri); + std::shared_ptr getProxyListConfiguration(const Poco::Util::AbstractConfiguration * proxy_config) + { + std::vector keys; + proxy_config->keys(keys); - LOG_DEBUG(&Logger::get("DiskS3"), "Configured proxy: " << proxy_uri.toString()); - } + std::vector proxies; + for (const auto & key : keys) + if (startsWith(key, "uri")) + { + Poco::URI proxy_uri(proxy_config->getString(key)); + + if (proxy_uri.getScheme() != "http" && proxy_uri.getScheme() != "https") + throw Exception("Only HTTP/HTTPS schemas allowed in proxy uri: " + proxy_uri.toString(), ErrorCodes::BAD_ARGUMENTS); + if (proxy_uri.getHost().empty()) + throw Exception("Empty host in proxy uri: " + proxy_uri.toString(), ErrorCodes::BAD_ARGUMENTS); + + proxies.push_back(proxy_uri); + + LOG_DEBUG(&Logger::get("DiskS3"), "Configured proxy: " << proxy_uri.toString() << " " << key); + } + + if (!proxies.empty()) + return std::make_shared(proxies); - if (!proxies.empty()) - return std::make_shared(proxies); - } return nullptr; } + std::shared_ptr getProxyConfiguration(const Poco::Util::AbstractConfiguration * config) + { + if (!config->has("proxy")) + return nullptr; + + const auto * proxy_config = config->createView("proxy"); + + std::vector configKeys; + proxy_config->keys(configKeys); + + if (auto resolverConfigs = std::count(configKeys.begin(), configKeys.end(), "resolver")) + { + if (resolverConfigs > 1) + throw Exception("Multiple proxy resolver configurations aren't allowed", ErrorCodes::BAD_ARGUMENTS); + + return getProxyResolverConfiguration(proxy_config->createView("resolver")); + } + + return getProxyListConfiguration(proxy_config); + } } diff --git a/src/Disks/S3/ya.make b/src/Disks/S3/ya.make index 446e7bd1cb2..66a32e6f0df 100644 --- a/src/Disks/S3/ya.make +++ b/src/Disks/S3/ya.make @@ -7,7 +7,8 @@ PEERDIR( SRCS( DiskS3.cpp registerDiskS3.cpp - DynamicProxyConfiguration.cpp + ProxyListConfiguration.cpp + ProxyResolverConfiguration.cpp ) END() diff --git a/tests/integration/helpers/cluster.py b/tests/integration/helpers/cluster.py index 17f3ed76ed5..53c36ff8924 100644 --- a/tests/integration/helpers/cluster.py +++ b/tests/integration/helpers/cluster.py @@ -309,6 +309,32 @@ class ClickHouseCluster: container_id = self.get_container_id(instance_name) return self.docker_client.api.logs(container_id) + def exec_in_container(self, container_id, cmd, detach=False, **kwargs): + exec_id = self.docker_client.api.exec_create(container_id, cmd, **kwargs) + output = self.docker_client.api.exec_start(exec_id, detach=detach) + + output = output.decode('utf8') + exit_code = self.docker_client.api.exec_inspect(exec_id)['ExitCode'] + if exit_code: + container_info = self.docker_client.api.inspect_container(container_id) + image_id = container_info.get('Image') + image_info = self.docker_client.api.inspect_image(image_id) + print("Command failed in container {}: ".format(container_id)) + pprint.pprint(container_info) + print("") + print("Container {} uses image {}: ".format(container_id, image_id)) + pprint.pprint(image_info) + print("") + raise Exception('Cmd "{}" failed in container {}. Return code {}. Output: {}'.format(' '.join(cmd), container_id, exit_code, output)) + return output + + def copy_file_to_container(self, container_id, local_path, dest_path): + with open(local_path, 'r') as fdata: + data = fdata.read() + encoded_data = base64.b64encode(data) + self.exec_in_container(container_id, ["bash", "-c", "echo {} | base64 --decode > {}".format(encoded_data, dest_path)], + user='root') + def wait_mysql_to_start(self, timeout=60): start = time.time() while time.time() - start < timeout: @@ -746,24 +772,8 @@ class ClickHouseInstance: assert_eq_with_retry(self, "select 1", "1", retry_count=int(stop_start_wait_sec / 0.5), sleep_time=0.5) def exec_in_container(self, cmd, detach=False, **kwargs): - container = self.get_docker_handle() - exec_id = self.docker_client.api.exec_create(container.id, cmd, **kwargs) - output = self.docker_client.api.exec_start(exec_id, detach=detach) - - output = output.decode('utf8') - exit_code = self.docker_client.api.exec_inspect(exec_id)['ExitCode'] - if exit_code: - container_info = self.docker_client.api.inspect_container(container.id) - image_id = container_info.get('Image') - image_info = self.docker_client.api.inspect_image(image_id) - print("Command failed in container {}: ".format(container.id)) - pprint.pprint(container_info) - print("") - print("Container {} uses image {}: ".format(container.id, image_id)) - pprint.pprint(image_info) - print("") - raise Exception('Cmd "{}" failed in container {}. Return code {}. Output: {}'.format(' '.join(cmd), container.id, exit_code, output)) - return output + container_id = self.get_docker_handle().id + return self.cluster.exec_in_container(container_id, cmd, detach, **kwargs) def contains_in_log(self, substring): result = self.exec_in_container( @@ -771,11 +781,8 @@ class ClickHouseInstance: return len(result) > 0 def copy_file_to_container(self, local_path, dest_path): - with open(local_path, 'r') as fdata: - data = fdata.read() - encoded_data = base64.b64encode(data) - self.exec_in_container(["bash", "-c", "echo {} | base64 --decode > {}".format(encoded_data, dest_path)], - user='root') + container_id = self.get_docker_handle().id + return self.cluster.copy_file_to_container(container_id, local_path, dest_path) def get_process_pid(self, process_name): output = self.exec_in_container(["bash", "-c", diff --git a/tests/integration/test_s3_with_proxy/configs/config.d/storage_conf.xml b/tests/integration/test_s3_with_proxy/configs/config.d/storage_conf.xml index 7827eec4498..2e4d4e6eaf1 100644 --- a/tests/integration/test_s3_with_proxy/configs/config.d/storage_conf.xml +++ b/tests/integration/test_s3_with_proxy/configs/config.d/storage_conf.xml @@ -11,6 +11,19 @@ http://proxy2:8888 + + s3 + http://minio1:9001/root/data/ + minio + minio123 + + + http://resolver:8080 + http + 8888 + + + @@ -20,6 +33,13 @@ + + +
+ s3_with_resolver +
+ + diff --git a/tests/integration/test_s3_with_proxy/proxy-resolver/__init__.py b/tests/integration/test_s3_with_proxy/proxy-resolver/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/integration/test_s3_with_proxy/proxy-resolver/entrypoint.sh b/tests/integration/test_s3_with_proxy/proxy-resolver/entrypoint.sh new file mode 100644 index 00000000000..e456be666a9 --- /dev/null +++ b/tests/integration/test_s3_with_proxy/proxy-resolver/entrypoint.sh @@ -0,0 +1,4 @@ +#!/bin/bash + +pip install bottle +python resolver.py diff --git a/tests/integration/test_s3_with_proxy/proxy-resolver/resolver.py b/tests/integration/test_s3_with_proxy/proxy-resolver/resolver.py new file mode 100644 index 00000000000..ecafe92cb83 --- /dev/null +++ b/tests/integration/test_s3_with_proxy/proxy-resolver/resolver.py @@ -0,0 +1,13 @@ +import bottle +import random + + +@bottle.route('/') +def index(): + if random.randrange(2) == 0: + return 'proxy1' + else: + return 'proxy2' + + +bottle.run(host='0.0.0.0', port=8080) diff --git a/tests/integration/test_s3_with_proxy/test.py b/tests/integration/test_s3_with_proxy/test.py index 8881d0e4b74..2b616ea2d02 100644 --- a/tests/integration/test_s3_with_proxy/test.py +++ b/tests/integration/test_s3_with_proxy/test.py @@ -1,4 +1,5 @@ import logging +import os import pytest from helpers.cluster import ClickHouseCluster @@ -17,6 +18,17 @@ def prepare_s3_bucket(cluster): minio_client.make_bucket(cluster.minio_bucket) +# Runs simple proxy resolver in python env container. +def run_resolver(cluster): + container_id = cluster.get_container_id('resolver') + current_dir = os.path.dirname(__file__) + cluster.copy_file_to_container(container_id, os.path.join(current_dir, "proxy-resolver", "resolver.py"), + "resolver.py") + cluster.copy_file_to_container(container_id, os.path.join(current_dir, "proxy-resolver", "entrypoint.sh"), + "entrypoint.sh") + cluster.exec_in_container(container_id, ["/bin/bash", "entrypoint.sh"], detach=True) + + @pytest.fixture(scope="module") def cluster(): try: @@ -29,6 +41,9 @@ def cluster(): prepare_s3_bucket(cluster) logging.info("S3 bucket created") + run_resolver(cluster) + logging.info("Proxy resolver started") + yield cluster finally: cluster.shutdown() @@ -41,7 +56,10 @@ def check_proxy_logs(cluster, proxy_instance): assert logs.find(http_method + " http://minio1") >= 0 -def test_s3_with_proxy_list(cluster): +@pytest.mark.parametrize( + "policy", ["s3", "s3_with_resolver"] +) +def test_s3_with_proxy_list(cluster, policy): node = cluster.instances["node"] node.query( @@ -51,8 +69,9 @@ def test_s3_with_proxy_list(cluster): data String ) ENGINE=MergeTree() ORDER BY id - SETTINGS storage_policy='s3' + SETTINGS storage_policy='{}' """ + .format(policy) ) node.query("INSERT INTO s3_test VALUES (0,'data'),(1,'data')") From 8602f7b47ca766de7fea0ee63ff7ab9ae120b832 Mon Sep 17 00:00:00 2001 From: Pavel Kovalenko Date: Fri, 8 May 2020 13:58:56 +0300 Subject: [PATCH 3/7] Minor updates in proxy resolver parameters naming. --- src/Disks/S3/DiskS3.h | 2 +- src/Disks/S3/registerDiskS3.cpp | 12 ++++++------ .../configs/config.d/storage_conf.xml | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/Disks/S3/DiskS3.h b/src/Disks/S3/DiskS3.h index 9930a83fd2b..5fa8e8358a6 100644 --- a/src/Disks/S3/DiskS3.h +++ b/src/Disks/S3/DiskS3.h @@ -1,7 +1,7 @@ #pragma once #include "Disks/DiskFactory.h" -#include "ProxyListConfiguration.h" +#include "ProxyConfiguration.h" #include #include diff --git a/src/Disks/S3/registerDiskS3.cpp b/src/Disks/S3/registerDiskS3.cpp index c4eb877f30e..a1dda6c704f 100644 --- a/src/Disks/S3/registerDiskS3.cpp +++ b/src/Disks/S3/registerDiskS3.cpp @@ -40,15 +40,15 @@ namespace std::shared_ptr getProxyResolverConfiguration(const Poco::Util::AbstractConfiguration * proxy_resolver_config) { auto endpoint = Poco::URI(proxy_resolver_config->getString("endpoint")); - auto scheme = proxy_resolver_config->getString("scheme"); - if (scheme != "http" && scheme != "https") - throw Exception("Only HTTP/HTTPS schemas allowed in proxy resolver config: " + scheme, ErrorCodes::BAD_ARGUMENTS); - auto port = proxy_resolver_config->getUInt("port"); + auto proxy_scheme = proxy_resolver_config->getString("proxy_scheme"); + if (proxy_scheme != "http" && proxy_scheme != "https") + throw Exception("Only HTTP/HTTPS schemas allowed in proxy resolver config: " + proxy_scheme, ErrorCodes::BAD_ARGUMENTS); + auto proxy_port = proxy_resolver_config->getUInt("proxy_port"); LOG_DEBUG( - &Logger::get("DiskS3"), "Configured proxy resolver: " << endpoint.toString() << ", Scheme: " << scheme << ", Port: " << port); + &Logger::get("DiskS3"), "Configured proxy resolver: " << endpoint.toString() << ", Scheme: " << proxy_scheme << ", Port: " << proxy_port); - return std::make_shared(endpoint, scheme, port); + return std::make_shared(endpoint, proxy_scheme, proxy_port); } std::shared_ptr getProxyListConfiguration(const Poco::Util::AbstractConfiguration * proxy_config) diff --git a/tests/integration/test_s3_with_proxy/configs/config.d/storage_conf.xml b/tests/integration/test_s3_with_proxy/configs/config.d/storage_conf.xml index 2e4d4e6eaf1..c791997b6f9 100644 --- a/tests/integration/test_s3_with_proxy/configs/config.d/storage_conf.xml +++ b/tests/integration/test_s3_with_proxy/configs/config.d/storage_conf.xml @@ -19,8 +19,8 @@ http://resolver:8080 - http - 8888 + http + 8888 From 5cc0927874052ec2800542ab5ccb32b5c70908be Mon Sep 17 00:00:00 2001 From: Pavel Kovalenko Date: Fri, 8 May 2020 14:07:26 +0300 Subject: [PATCH 4/7] Add comment explaining protocol for proxy resolver configuration. --- .../test_s3_with_proxy/configs/config.d/storage_conf.xml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/integration/test_s3_with_proxy/configs/config.d/storage_conf.xml b/tests/integration/test_s3_with_proxy/configs/config.d/storage_conf.xml index c791997b6f9..a83c875b134 100644 --- a/tests/integration/test_s3_with_proxy/configs/config.d/storage_conf.xml +++ b/tests/integration/test_s3_with_proxy/configs/config.d/storage_conf.xml @@ -17,6 +17,11 @@ minio minio123 + http://resolver:8080 http From 8d8dc3035cca5588f92ee44e7a0c1d13face0fb0 Mon Sep 17 00:00:00 2001 From: Pavel Kovalenko Date: Fri, 8 May 2020 16:10:21 +0300 Subject: [PATCH 5/7] Fix naming. --- src/Disks/S3/registerDiskS3.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Disks/S3/registerDiskS3.cpp b/src/Disks/S3/registerDiskS3.cpp index a1dda6c704f..36966d1f34b 100644 --- a/src/Disks/S3/registerDiskS3.cpp +++ b/src/Disks/S3/registerDiskS3.cpp @@ -85,12 +85,12 @@ namespace const auto * proxy_config = config->createView("proxy"); - std::vector configKeys; - proxy_config->keys(configKeys); + std::vector config_keys; + proxy_config->keys(config_keys); - if (auto resolverConfigs = std::count(configKeys.begin(), configKeys.end(), "resolver")) + if (auto resolver_configs = std::count(config_keys.begin(), config_keys.end(), "resolver")) { - if (resolverConfigs > 1) + if (resolver_configs > 1) throw Exception("Multiple proxy resolver configurations aren't allowed", ErrorCodes::BAD_ARGUMENTS); return getProxyResolverConfiguration(proxy_config->createView("resolver")); From 8bea42e2505c2517a721a4b14ad104e028b860cf Mon Sep 17 00:00:00 2001 From: Pavel Kovalenko Date: Tue, 12 May 2020 12:23:18 +0300 Subject: [PATCH 6/7] Fix includes in ProxyResolverConfiguration.h --- src/Disks/S3/ProxyResolverConfiguration.cpp | 1 + src/Disks/S3/ProxyResolverConfiguration.h | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Disks/S3/ProxyResolverConfiguration.cpp b/src/Disks/S3/ProxyResolverConfiguration.cpp index dd5af0990b8..bf06804cd23 100644 --- a/src/Disks/S3/ProxyResolverConfiguration.cpp +++ b/src/Disks/S3/ProxyResolverConfiguration.cpp @@ -1,6 +1,7 @@ #include "ProxyResolverConfiguration.h" #include +#include #include "Poco/StreamCopier.h" #include #include diff --git a/src/Disks/S3/ProxyResolverConfiguration.h b/src/Disks/S3/ProxyResolverConfiguration.h index 2944121a078..0b23ae77c4a 100644 --- a/src/Disks/S3/ProxyResolverConfiguration.h +++ b/src/Disks/S3/ProxyResolverConfiguration.h @@ -1,7 +1,6 @@ #pragma once #include "ProxyConfiguration.h" -#include namespace DB::S3 { From 506634786e849d691fd2774a1d93470e42722f04 Mon Sep 17 00:00:00 2001 From: Pavel Kovalenko Date: Tue, 12 May 2020 17:34:37 +0300 Subject: [PATCH 7/7] Handle exceptions in ProxyResolverConfiguration properly. --- docker/test/integration/compose/docker_compose_minio.yml | 6 ++++++ src/Disks/S3/ProxyResolverConfiguration.cpp | 4 ++-- src/Disks/S3/registerDiskS3.cpp | 4 ++-- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/docker/test/integration/compose/docker_compose_minio.yml b/docker/test/integration/compose/docker_compose_minio.yml index ec35c87fa00..79023e24244 100644 --- a/docker/test/integration/compose/docker_compose_minio.yml +++ b/docker/test/integration/compose/docker_compose_minio.yml @@ -16,6 +16,9 @@ services: interval: 30s timeout: 20s retries: 3 + depends_on: + - redirect + - resolver # Redirects all requests to origin Minio. redirect: @@ -44,6 +47,9 @@ services: ports: - "4083:8080" tty: true + depends_on: + - proxy1 + - proxy2 volumes: data1-1: diff --git a/src/Disks/S3/ProxyResolverConfiguration.cpp b/src/Disks/S3/ProxyResolverConfiguration.cpp index bf06804cd23..a574809596f 100644 --- a/src/Disks/S3/ProxyResolverConfiguration.cpp +++ b/src/Disks/S3/ProxyResolverConfiguration.cpp @@ -49,9 +49,9 @@ Aws::Client::ClientConfigurationPerRequest ProxyResolverConfiguration::getConfig return cfg; } - catch (Exception & e) + catch (...) { - LOG_ERROR(&Logger::get("AWSClient"), "Failed to obtain proxy: " << e.message()); + tryLogCurrentException("AWSClient", "Failed to obtain proxy"); /// Don't use proxy if it can't be obtained. return cfg; } diff --git a/src/Disks/S3/registerDiskS3.cpp b/src/Disks/S3/registerDiskS3.cpp index 36966d1f34b..2b72f872dd2 100644 --- a/src/Disks/S3/registerDiskS3.cpp +++ b/src/Disks/S3/registerDiskS3.cpp @@ -69,7 +69,7 @@ namespace proxies.push_back(proxy_uri); - LOG_DEBUG(&Logger::get("DiskS3"), "Configured proxy: " << proxy_uri.toString() << " " << key); + LOG_DEBUG(&Logger::get("DiskS3"), "Configured proxy: " << proxy_uri.toString()); } if (!proxies.empty()) @@ -134,7 +134,7 @@ void registerDiskS3(DiskFactory & factory) auto s3disk = std::make_shared( name, client, - std::move(proxy_config), + proxy_config, uri.bucket, uri.key, metadata_path,