Test host_regexp against all PTR records instead of only one by arthurpassos · Pull Request #37827 · ClickHouse/ClickHouse

arthurpassos · 2022-06-03T12:19:17Z

Changelog category (leave one):

New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

In #17202 was reported that host_regexp was being tested against only one of the possible PTR responses. This PR makes the necessary changes so that host_regexp is applied against all possible PTR responses and validate if any matches.

c-ares is being used because the vanilla getnameinfo is limited to one PTR.

Integration tests for this feature seemed to be an overkill because of the amount of effort required. It would require setting up a DNS server with the necessary resolutions and tweak clickhouse node host DNS settings so it points to the DNS server so it can finally be tested.

I have tested it locally using a setup similar to the gist provided by the OP in the issue. The scenario is:

$client_ip_address resolving to test1.example.com test2.example.com test3.example.com

test1.example.com resolving to $client_ip_address
test2.example.com resolving to $client_ip_address
test3.example.com resolving to $client_ip_address

host_regexp set to <host_regexp>test3\.example\.com$</host_regexp>

Previously, it would fail to auth. With the changes in this PR, it allows the request.

Closes #17202

alexey-milovidov · 2022-06-06T13:11:07Z

Most likely it needs to be added to docker/test/fasttest.

alexey-milovidov · 2022-06-09T10:43:27Z

https://s3.amazonaws.com/clickhouse-test-reports/37827/7fa65c2b85afe3b86ae191c375ccd8272b578599/style_check__actions_/style_output.txt

alexey-milovidov · 2022-06-09T10:43:42Z

url = [email protected]:YukiWorkshop/cpp-dns.git
All the submodules should be from https://github.com/

arthurpassos · 2022-06-09T12:33:00Z

Just updated it to use https url. Can you re-approve the run @alexey-milovidov?

In the meantime, I am working on the fasttest local setup so I can iterate quicker without having to push changes to try things out, even tho this might be the last necessary change for fasttest...

arthurpassos · 2022-06-13T13:45:39Z

Kind reminder for CI re-run @alexey-milovidov

arthurpassos · 2022-06-15T13:04:41Z

Just fixed style case and DNS error message. Kind ping for CI re-run. @alexey-milovidov

arthurpassos · 2022-06-20T11:44:27Z

Hi. I checked the CI failures and it seems like it's not related to the changes in this PR. Plus, checking other recent merged PRs, same failure was present. I believe it's ready for review, please let me know if there is anything else missing.

arthurpassos · 2022-06-23T18:16:22Z

Kind ping, is there anything missing here? @alexey-milovidov

arthurpassos · 2022-06-24T12:06:01Z

Just resolved conflicts

alexey-milovidov · 2022-06-24T23:33:26Z

Someone should take it for review. I did not review it yet.

rschu1ze

Looks good overall, thanks a lot. I added just some minor comments after which we can merge.

.gitmodules

contrib/CMakeLists.txt

.gitmodules

contrib/cpp-dns-cmake/CMakeLists.txt

contrib/udns-cmake/CMakeLists.txt

src/Common/DNSResolver.cpp

src/CMakeLists.txt

tavplubix · 2022-07-04T12:30:31Z

src/Common/DNSResolver.cpp

Hm, why do we need "async DNS resolver" if the only place where it's used actually works synchronously?
Also I don't like the namespace name YukiWorkshop.
Can we get rid of cpp-dns library?

We need a library that returns multiple PTR records for a given ip address. The problem is that most libraries return only one, including getnameinfo. I am also not a big fan of the namespace, but that's the only working library I could find.

The easiest path is to keep cpp-dns, but I believe we could get rid of it if we implemented something similar, it'll just take more time.

We need a library that returns multiple PTR records for a given ip address.

Yes, and udns does the job, cpp-dns looks like useless wrapper. Am I missing something?

I wouldn't consider it to be useless. It's much more idiomatic and easier to use than libudns. libudns is cryptic and hard to use, I believe that we need a wrapper. The question is if we want to build a wrapper or use an existing one.

If you want, I can implement the wrapper myself and get rid of cpp-dns. Would that be enough?

Sorry, I mean ares_gethostbyaddr, not ares_getnameinfo

Ok, I just tested ares_gethostbyaddr and it indeed works. I will build the wrapper around it then, ok?

Ok, so both udns and c-ares support multiple PTR records and work fine. But c-ares looks more popular, does not look abandoned and its interface is quite similar to system functions. So I think yes, we should use c-ares library.

Also there's alive c-ares repo on GitHub. I've created a fork in ClickHouse organization (just in case we will need change something a bit): https://github.com/ClickHouse/c-ares

ClickHouse already builds c-ares because grpc depends on it. I believe that whenever -DENABLE_GRPC is set to ON, c-ares is built as well. Adding c-ares as a submodule as it is fails because it tries to add yet another target called c-ares and it conflicts with the one built by grpc. Based on that, I see a couple of paths forward:

Build c-ares as a submodule and link grpc against it. Requires changes in grpc, which is not in the scope of this PR, but it would be necessary.

Link clickhouse_common_io against c-ares built by grpc. If c-ares target doesn't exist because grpc wasn't built, build c-ares submodule. Dependecy graph becomes a bit messy and not clear, but leaves grpc untouched.

Build c-ares as a submodule, but with different name. I don't see this one as a good option, but leaving it on the table.

Which one do you guys prefer and what do you suggest?

Btw, I have a draft implementation of a c-ares wrapper and already tested it on clickhouse with approach #2 (without the conditional build, tho). It worked!

Cool, so we don't need to add extra libraries! The first option is the best, it makes sense to try it first.

… fork

arthurpassos · 2022-07-15T15:43:31Z

CI run was all green: https://github.com/ClickHouse/ClickHouse/actions/runs/2666225397, aside from style check, which I just pushed a fix for.

I'll see what I can do about the integ tests. I might reach out for help since it's my first time looking at integ tests code.

arthurpassos · 2022-07-20T17:11:09Z

Alright, that took some effort :). Just published integ tests, please let me know if you want to change anything @tavplubix .

arthurpassos · 2022-07-20T23:05:26Z

I see three failures:

Style. I will fix it tomorrow morning.
CI machines failing like before. A re-run should help.
BuilderDebSplitted. Doesn't seem to be related to this PR.

Kind ping for review and re-run @tavplubix

Felixoid · 2022-07-21T07:49:31Z

I restarted it. I'm trying to dig into the issue with a rerun

This particular time the behavior is valid, the ci has status Cancelled, and we don't restart it automatically intentionally.

arthurpassos · 2022-07-21T11:32:35Z

I have the style fix ready, but will wait until CI run is finished to avoid yet another long run. Btw, I am curious, are you satisfied with the integ tests or you haven't had a chance to review it yet?

tavplubix · 2022-07-21T12:26:37Z

Tests look fine, thanks!

arthurpassos · 2022-07-21T13:29:07Z

Just pushed what I believe to have fixed the style check, couldn't confirm it since I don't know how to run it locally.

tavplubix · 2022-07-21T13:42:23Z

Style Check (actions) — Style check success
Other checks passed on previous commit
Integration tests (asan, actions) [3/3] - test_postgresql_replica_database_engine_1 is broken in master, cc: @kssenii

alexey-milovidov · 2022-07-23T21:50:50Z

contrib/c-ares-cmake/CMakeLists.txt

+# Force use of c-ares inet_net_pton instead of libresolv one
+set(HAVE_INET_NET_PTON OFF CACHE BOOL "" FORCE)
+
+add_subdirectory("../c-ares/" "../c-ares/")


We don't allow reusing third-party CMake files.

alexey-milovidov · 2022-07-23T21:51:24Z

@tavplubix You've merged prematurally. Need to revert or fix it quickly.

alexey-milovidov · 2022-07-23T21:53:29Z

@rschu1ze this goes in the opposite direction of #35630

alexey-milovidov · 2022-07-23T21:54:01Z

contrib/CMakeLists.txt

 add_contrib (sqlite-cmake sqlite-amalgamation)
 add_contrib (s2geometry-cmake s2geometry)
 add_contrib (base-x-cmake base-x)
+add_contrib(c-ares-cmake c-ares)


Inconsistent style.

rschu1ze · 2022-07-24T18:30:25Z

@rschu1ze this goes in the opposite direction of #35630

Yes, and sorry for missing that, I have not looked at the latest versions of this PR.

Looking at contrib/cares/CMakeLists.txt, there is some complexity but nothing impossible. The MS Win build and the build of "tools" can be omitted. The file "lib/ares_config.h" can be generated by running the original build system on Linux (x86 + ARM + PPC LE), FreeBSD and Mac and checking in the result below "contrib/c-ares-cmake/" (see contrib/vectorscan-cmake/x86_64/config.h for a similar situation).

tavplubix · 2022-07-25T13:21:47Z

@alexey-milovidov, we already reuse third-party cmake files for grpc, our "own" cmake file simply calls it:

ClickHouse/contrib/grpc-cmake/CMakeLists.txt

Line 14 in a0055fc

set(_gRPC_SOURCE_DIR "${ClickHouse_SOURCE_DIR}/contrib/grpc")

ClickHouse/contrib/grpc-cmake/CMakeLists.txt

Line 53 in a0055fc

add_subdirectory("${_gRPC_SOURCE_DIR}" "${_gRPC_BINARY_DIR}")

Seems like we had our own cmake files for grpc, but it was changed back to third-party cmakes in #15491, cc: @vitlibar

Before this PR c-ares used to be built inside grpc as kind of recursive submodule. This PR does not add new libraries and new third-party cmake files, it only moves c-ares from contrib/grpc/third_party/cares/cares to contrib/c-ares, so there's nothing to fix quickly (and it's too late to revert #15491).

I will fix style inconsistency and remove contrib/grpc/third_party/cares

Follow up to #37827

porting ClickHouse/ClickHouse#37827 original bdc9153

robot-ch-test-poll added pr-feature Pull request with new product feature submodule changed At least one submodule changed in this PR. labels Jun 3, 2022

alexey-milovidov added the can be tested Allows running workflows for external contributors label Jun 3, 2022

alexey-milovidov marked this pull request as draft June 5, 2022 12:00

alexey-milovidov marked this pull request as ready for review June 15, 2022 01:54

rschu1ze self-assigned this Jun 28, 2022

rschu1ze reviewed Jun 30, 2022

View reviewed changes

tavplubix reviewed Jul 4, 2022

View reviewed changes

arthurpassos added 8 commits July 4, 2022 10:05

Test host_regexp against all PTR records instead of only one

d66154e

Remove unnecessary CMake comments

e030d49

Fix wrong udns submodule

7851984

Remove dbms ctarget ppp-dns and udns link

3d4d71c

Add cpp-dns and udns to docker/test/fasttest

52c5d7e

Use https submodule url instead of ssh for cpp-dns

9d842ee

Fix style case and DNS error message

dcda1c5

Link cpp-dns/udns unconditionally and update submodules to clickhouse…

5c00dcd

… fork

arthurpassos force-pushed the host_regexp_multiple_domains branch from b1f6747 to 5c00dcd Compare July 4, 2022 13:08

Minor adjustments

6c987fb

arthurpassos requested review from rschu1ze and tavplubix July 4, 2022 13:30

minor style fixes

abb550d

Add host regexp multiple PTR records integration tests

828de2a

arthurpassos added 2 commits July 20, 2022 14:20

remove unused coredns symbols

29613f4

remove custom debugging log

8b49952

Minor style fix in dns integ test

03bd7ce

tavplubix approved these changes Jul 21, 2022

View reviewed changes

tavplubix merged commit 9e9969c into ClickHouse:master Jul 21, 2022

alexey-milovidov reviewed Jul 23, 2022

View reviewed changes

tavplubix mentioned this pull request Jul 25, 2022

Follow up to #37827 #39557

Merged

tavplubix added a commit that referenced this pull request Jul 25, 2022

Merge pull request #39557 from ClickHouse/follow_up_37827

9254ae1

Follow up to #37827

rschu1ze mentioned this pull request Jul 26, 2022

Build all contribs using our own CMake files (2) #35630

Open

8 tasks

Felixoid mentioned this pull request Jul 26, 2022

Attempt to fix wrong workflow_run data for rerun #39630

Merged

tavplubix mentioned this pull request Aug 4, 2022

When I upgrade to 22.7.1.2484 from 21.8.14.5, after a few minutes all servers crash.The part of log are as follows. #39807

Closed

alexey-milovidov mentioned this pull request Aug 25, 2022

22.8.2 version users.xml <host_regexp> abnormal #40595

Closed

rschu1ze mentioned this pull request Sep 12, 2022

Bundle c-ares 3rd-party library #41239

Merged

yokofly added a commit to timeplus-io/proton that referenced this pull request Oct 18, 2023

Test host_regexp against all PTR records instead of only one

0690437

porting ClickHouse/ClickHouse#37827 original bdc9153

arthurpassos mentioned this pull request Apr 5, 2024

Send original IP in HTTP Authentication #62351

Closed

Conversation

arthurpassos commented Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Uh oh!

alexey-milovidov commented Jun 6, 2022

Uh oh!

alexey-milovidov commented Jun 9, 2022

Uh oh!

alexey-milovidov commented Jun 9, 2022

Uh oh!

arthurpassos commented Jun 9, 2022

Uh oh!

arthurpassos commented Jun 13, 2022

Uh oh!

arthurpassos commented Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arthurpassos commented Jun 20, 2022

Uh oh!

arthurpassos commented Jun 23, 2022

Uh oh!

arthurpassos commented Jun 24, 2022

Uh oh!

alexey-milovidov commented Jun 24, 2022

Uh oh!

rschu1ze left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arthurpassos commented Jul 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arthurpassos commented Jul 20, 2022

Uh oh!

arthurpassos commented Jul 20, 2022

Uh oh!

Felixoid commented Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arthurpassos commented Jul 21, 2022

Uh oh!

tavplubix commented Jul 21, 2022

Uh oh!

arthurpassos commented Jul 21, 2022

Uh oh!

tavplubix commented Jul 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov commented Jul 23, 2022

Uh oh!

alexey-milovidov commented Jul 23, 2022

arthurpassos commented Jun 3, 2022 •

edited

Loading

arthurpassos commented Jun 15, 2022 •

edited

Loading

arthurpassos commented Jul 15, 2022 •

edited

Loading

Felixoid commented Jul 21, 2022 •

edited

Loading

tavplubix commented Jul 25, 2022 •

edited

Loading