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

Backport #50240 to 23.4: Fix reconnecting of HTTPS session when target host IP was changed #50769

Merged
merged 1 commit into from Jun 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions base/poco/Net/include/Poco/Net/HTTPClientSession.h
Expand Up @@ -127,6 +127,9 @@ namespace Net

void setResolvedHost(std::string resolved_host) { _resolved_host.swap(resolved_host); }

std::string getResolvedHost() const { return _resolved_host; }
/// Returns the resolved IP address of the target HTTP server.

Poco::UInt16 getPort() const;
/// Returns the port number of the target HTTP server.

Expand Down
29 changes: 21 additions & 8 deletions src/IO/HTTPCommon.cpp
Expand Up @@ -68,7 +68,8 @@ namespace
if (https)
{
#if USE_SSL
String resolved_host = resolve_host ? DNSResolver::instance().resolveHost(host).toString() : host;
/// Cannot resolve host in advance, otherwise SNI won't work in Poco.
/// For more information about SNI, see the https://en.wikipedia.org/wiki/Server_Name_Indication
auto https_session = std::make_shared<Poco::Net::HTTPSClientSession>(host, port);
if (resolve_host)
https_session->setResolvedHost(DNSResolver::instance().resolveHost(host).toString());
Expand Down Expand Up @@ -184,6 +185,24 @@ namespace
std::mutex mutex;
std::unordered_map<Key, PoolPtr, Hasher> endpoints_pool;

void updateHostIfIpChanged(Entry & session, const String & new_ip)
{
const auto old_ip = session->getResolvedHost().empty() ? session->getHost() : session->getResolvedHost();

if (new_ip != old_ip)
{
session->reset();
if (session->getResolvedHost().empty())
{
session->setHost(new_ip);
}
else
{
session->setResolvedHost(new_ip);
}
}
}

protected:
HTTPSessionPool() = default;

Expand Down Expand Up @@ -238,13 +257,7 @@ namespace

if (resolve_host)
{
/// Host can change IP
const auto ip = DNSResolver::instance().resolveHost(host).toString();
if (ip != session->getHost())
{
session->reset();
session->setHost(ip);
}
updateHostIfIpChanged(session, DNSResolver::instance().resolveHost(host).toString());
}
}
/// Reset the message, once it has been printed,
Expand Down
@@ -0,0 +1,3 @@
<clickhouse>
<listen_host>::</listen_host>
</clickhouse>
96 changes: 96 additions & 0 deletions tests/integration/test_https_replication/test_change_ip.py
@@ -0,0 +1,96 @@
import pytest
from helpers.cluster import ClickHouseCluster
from helpers.test_tools import assert_eq_with_retry

"""
Both ssl_conf.xml and no_ssl_conf.xml have the same port
"""


def _fill_nodes(nodes, shard):
for node in nodes:
node.query(
"""
CREATE DATABASE test;

CREATE TABLE test_table(date Date, id UInt32)
ENGINE = ReplicatedMergeTree('/clickhouse/tables/test{shard}/replicated', '{replica}') PARTITION BY toYYYYMM(date) ORDER BY id;
""".format(
shard=shard, replica=node.name
)
)


cluster = ClickHouseCluster(__file__)
node1 = cluster.add_instance(
"node1",
main_configs=[
"configs/remote_servers.xml",
"configs/listen_host.xml",
"configs/ssl_conf.xml",
"configs/server.crt",
"configs/server.key",
"configs/dhparam.pem",
],
with_zookeeper=True,
ipv6_address="2001:3984:3989::1:1111",
)
node2 = cluster.add_instance(
"node2",
main_configs=[
"configs/remote_servers.xml",
"configs/listen_host.xml",
"configs/ssl_conf.xml",
"configs/server.crt",
"configs/server.key",
"configs/dhparam.pem",
],
with_zookeeper=True,
ipv6_address="2001:3984:3989::1:1112",
)


@pytest.fixture(scope="module")
def both_https_cluster():
try:
cluster.start()

_fill_nodes([node1, node2], 1)

yield cluster

finally:
cluster.shutdown()


def test_replication_when_node_ip_changed(both_https_cluster):
"""
Test for a bug when replication over HTTPS stops working when the IP of the source replica was changed.

node1 is a source node
node2 fethes data from node1
"""
node1.query("truncate table test_table")
node2.query("truncate table test_table")

# First we check, that normal replication works
node1.query(
"INSERT INTO test_table VALUES ('2022-10-01', 1), ('2022-10-02', 2), ('2022-10-03', 3)"
)
assert node1.query("SELECT count(*) from test_table") == "3\n"
assert_eq_with_retry(node2, "SELECT count(*) from test_table", "3")

# We change source node ip
cluster.restart_instance_with_ip_change(node1, "2001:3984:3989::1:7777")

# Put some data to source node1
node1.query(
"INSERT INTO test_table VALUES ('2018-10-01', 4), ('2018-10-02', 4), ('2018-10-03', 6)"
)
# Check that data is placed on node1
assert node1.query("SELECT count(*) from test_table") == "6\n"

# drop DNS cache
node2.query("SYSTEM DROP DNS CACHE")
# Data is fetched
assert_eq_with_retry(node2, "SELECT count(*) from test_table", "6")