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

Recognize more addresses as local #24316

Merged
merged 5 commits into from
May 21, 2021
Merged

Recognize more addresses as local #24316

merged 5 commits into from
May 21, 2021

Conversation

alexey-milovidov
Copy link
Member

@alexey-milovidov alexey-milovidov commented May 20, 2021

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Recognize IPv4 addresses like 127.0.1.1 as local. This is controversial and closes #23504. Michael Filimonov will test this feature.

@robot-clickhouse robot-clickhouse added the pr-improvement Pull request with some product improvements label May 20, 2021
}
else if (address.family() == Poco::Net::AddressFamily::IPv6)
{
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

So now any ipv6 address will be treated as local or I missed something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, missed the check above.

@alexey-milovidov
Copy link
Member Author

I will add a unit test.

Copy link
Member

@alesapin alesapin left a comment

Choose a reason for hiding this comment

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

Quite hacky, but OK. It will solve the problem caused by strange Debian's decision.

@alesapin alesapin self-assigned this May 20, 2021
@alexey-milovidov alexey-milovidov force-pushed the more-localhost-addresses branch from 5310584 to 87a7133 Compare May 21, 2021 05:19
@alesapin
Copy link
Member

Fuzzer:

$ grep -a 'Fatal' server.log 
2021.05.21 10:51:17.742899 [ 39 ] {} <Fatal> Application: Child process was terminated by signal 6.

??

@alesapin
Copy link
Member

alesapin commented May 21, 2021

quota_dcl

2021-05-21 09:36:42 +quota_by_forwarded_ip
2021-05-21 09:36:42 +quota_by_ip

hmm, but unrelated, just flaky and cannot work concurrently.

@alesapin
Copy link
Member

#24393

@alesapin
Copy link
Member

No related failures.

@alesapin alesapin merged commit 95a053e into master May 21, 2021
@alesapin alesapin deleted the more-localhost-addresses branch May 21, 2021 15:41
* But 127.{0,1}.{0,1}.{0,1} are treat as local addresses,
* because they are used in Debian for localhost.
*/
if (address.isLoopback())
Copy link

@jhettler jhettler Jun 15, 2021

Choose a reason for hiding this comment

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

Hi @alesapin,
I am afraid this will work only for IP address 127.0.0.1, related to https://pocoproject.org/docs/Poco.Net.IPAddress.html#24435 where documentation says loopback address is 127.0.0.1 for IPv4, otherwise it will never reach the internal part of if statement. I have this trouble in docker environment and I have 127.0.1.1 in /etc/hosts and unfortunately is_local is still 0.
Thanks for hints!

Copy link
Contributor

Choose a reason for hiding this comment

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

Poco source
https://github.com/ClickHouse-Extras/poco/blob/5994506908028612869fee627d68d8212dfe7c1e/Net/src/IPAddressImpl.cpp#L182

So rather poco docs are not in sync, and not clear what is happening in your case

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW - are you sure you use 21.7?

Choose a reason for hiding this comment

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

Hi @filimonov, sorry i didn't catch the difference between code and docs in poco. Unfortunately no, I am using 21.6.4 - I have clickhouse running in docker and there is no 21.7 docker image at the moment. Once there is 21.7 I will check if it works.

At the moment I solved it by setting real IP address of the server in /etc/hosts instead of 127.0.1.1 and it works as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-improvement Pull request with some product improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make is_local recognize loopback addresses 127.0.1.1
6 participants