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.

Sign up for GitHub

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

Jump to bottom

Fix configuration merge for nodes with substitution attributes #56694

Merged
merged 7 commits into from Nov 16, 2023
Merged

Fix configuration merge for nodes with substitution attributes #56694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2fddc17
Add test
thevar1able Nov 13, 2023
5bb4eb2
Fix
thevar1able Nov 13, 2023
cc3dfaf
Automatic style fix
robot-clickhouse Nov 13, 2023
e3842eb
Remove substitution attrs from target node if source already has a value
thevar1able Nov 15, 2023
9edcdf4
Add exceptions during substitution processing if node already has a v…
thevar1able Nov 15, 2023
67796b1
Add missing test config files
thevar1able Nov 15, 2023
e0f6c68
Lint
thevar1able Nov 15, 2023
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
12 changes: 12 additions & 0 deletions src/Common/Config/ConfigProcessor.cpp
View file
Open in desktop
Expand Up @@ -330,6 +330,12 @@ void ConfigProcessor::mergeRecursive(XMLDocumentPtr config, Node * config_root,
{
Element & config_element = dynamic_cast<Element &>(*config_node);

/// Remove substitution attributes from the merge target node if source node already has a value
bool source_has_value = with_element.hasChildNodes();
if (source_has_value)
for (const auto & attr_name: SUBSTITUTION_ATTRS)
config_element.removeAttribute(attr_name);

mergeAttributes(config_element, with_element);
mergeRecursive(config, config_node, with_node);
}
Expand Down Expand Up @@ -513,6 +519,9 @@ void ConfigProcessor::doIncludesRecursive(

if (attr_nodes["from_zk"]) /// we have zookeeper subst
{
if (node->hasChildNodes()) /// only allow substitution for nodes with no value
throw Poco::Exception("Element <" + node->nodeName() + "> has value, can't process from_zk substitution");

contributing_zk_paths.insert(attr_nodes["from_zk"]->getNodeValue());

if (zk_node_cache)
Expand All @@ -535,6 +544,9 @@ void ConfigProcessor::doIncludesRecursive(

if (attr_nodes["from_env"]) /// we have env subst
{
if (node->hasChildNodes()) /// only allow substitution for nodes with no value
throw Poco::Exception("Element <" + node->nodeName() + "> has value, can't process from_env substitution");
Copy link
Collaborator

@azat azat Nov 22, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used this behavior before (define default value in the xml with replace attribute and from_env and override it via env) for development, AFAICS the bug that described in #56537 is fixed by removing SUBSTITUTION_ATTRS from source node during merging before?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


XMLDocumentPtr env_document;
auto get_env_node = [&](const std::string & name) -> const Node *
{
Expand Down
17 changes: 17 additions & 0 deletions tests/integration/test_config_substitutions/configs/000-config_with_env_subst.xml
View file
Open in desktop
@@ -0,0 +1,17 @@
<clickhouse>
<profiles>
<default>
<max_query_size from_env="MAX_QUERY_SIZE" />
</default>
</profiles>
<users>
<default>
<password></password>
<profile>default</profile>
<quota>default</quota>
</default>

<include incl="users_1" />
<include incl="users_2" />
</users>
</clickhouse>
17 changes: 17 additions & 0 deletions tests/integration/test_config_substitutions/configs/010-env_subst_override.xml
View file
Open in desktop
@@ -0,0 +1,17 @@
<clickhouse>
<profiles>
<default>
<max_query_size>424242</max_query_size>
</default>
</profiles>
<users>
<default>
<password></password>
<profile>default</profile>
<quota>default</quota>
</default>

<include incl="users_1" />
<include incl="users_2" />
</users>
</clickhouse>
13 changes: 13 additions & 0 deletions tests/integration/test_config_substitutions/test.py
View file
Open in desktop
Expand Up @@ -30,6 +30,15 @@
},
main_configs=["configs/include_from_source.xml"],
)
node7 = cluster.add_instance(
"node7",
user_configs=[
"configs/000-config_with_env_subst.xml",
"configs/010-env_subst_override.xml",
],
env_variables={"MAX_QUERY_SIZE": "121212"},
instance_env_variables=True,
) # overridden with 424242


@pytest.fixture(scope="module")
Expand Down Expand Up @@ -78,6 +87,10 @@ def test_config(start_cluster):
node6.query("select value from system.settings where name = 'max_query_size'")
== "99999\n"
)
assert (
node7.query("select value from system.settings where name = 'max_query_size'")
== "424242\n"
)


def test_include_config(start_cluster):
Expand Down