feat: stream subsystem support dns service discovery #8593

ronething · 2022-12-30T15:42:57Z

Description

For SRV record which has port 0, we will fallback to use the upstream protocol's default port.

i'm not sure what is the tcp protocol's default port, so my test case is temporary like below.

proxy request to 127.0.0.1:nil

=== TEST 14: SRV (port is 0)
--- apisix_yaml
upstreams:
    - service_name: "zero.srv.test.local"
      discovery_type: dns
      type: roundrobin
      id: 1
--- error_log
connect() failed
--- grep_error_log eval
qr/proxy request to \S+/
--- grep_error_log_out
proxy request to 127.0.0.1:nil

i will modify scheme_to_port if need.

local scheme_to_port = {
    http = 80,
    https = 443,
    grpc = 80,
    grpcs = 443
}

Checklist

I have explained the need for this PR and the problem it solves
I have explained the changes or the new features added to this PR
I have added tests corresponding to this change
I have updated the documentation to reflect this change
I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

…_discovery_dns

spacewander · 2023-01-05T05:55:00Z

t/discovery/stream/dns.t

+--- grep_error_log eval
+qr/proxy request to \S+/
+--- grep_error_log_out
+proxy request to 127.0.0.1:nil


Maybe we need to reject zero port in the stream, as there is not default port for TCP.
proxy request to 127.0.0.1:nil such an error log looks like a bug in APISIX.

For HTTP, we will fill node info in apisix/upstream.lua

apisix/apisix/upstream.lua

Line 178 in 030019b

local function fill_node_info(up_conf, scheme, is_stream)

For Stream, if node port is 0, it will not be filled node info, so the error log is like proxy request to 127.0.0.1:nil which in the apisix/balancer.lua

apisix/apisix/balancer.lua

Line 384 in 030019b

core.log.info("proxy request to ", server.host, ":", server.port)

i wonder if we need to reject zero port in the stream, we should reject in where, can you give me some hints? thank you.

i ignore zero port node when is in the stream subsystem.

spacewander · 2023-01-05T05:55:49Z

t/discovery/stream/dns.t

+
+
+
+=== TEST 17: Invalid order type in config.yaml


Do we need these tests? The order of type doesn't matter with the L4 or L7.

i will remove these tests later.

spacewander · 2023-01-05T05:59:18Z

t/discovery/stream/dns.t

+      type: roundrobin
+      id: 1
+--- error_log
+127.0.0.1:53


The expected error log is not long enough and confusing.
Why don't we use

apisix/t/discovery/dns/sanity.t

Line 89 in 9345425

connect to 127.0.0.1:53

spacewander · 2023-01-05T06:44:21Z

t/discovery/stream/dns.t

+
+    if (!$block->stream_request) {
+        # GET /hello HTTP/1.0\r\nHost: 127.0.0.1:1985\r\n\r\n
+        $block->set_value("stream_request", "\x47\x45\x54\x20\x2f\x68\x65\x6c\x6c\x6f\x20\x48\x54\x54\x50\x2f\x31\x2e\x30\x0d\x0a\x48\x6f\x73\x74\x3a\x20\x31\x32\x37\x2e\x30\x2e\x30\x2e\x31\x3a\x31\x39\x38\x35\x0d\x0a\x0d\x0a");


I spend some time and figure out we can set it directly with:

Suggested change

$block->set_value("stream_request", "\x47\x45\x54\x20\x2f\x68\x65\x6c\x6c\x6f\x20\x48\x54\x54\x50\x2f\x31\x2e\x30\x0d\x0a\x48\x6f\x73\x74\x3a\x20\x31\x32\x37\x2e\x30\x2e\x30\x2e\x31\x3a\x31\x39\x38\x35\x0d\x0a\x0d\x0a");

$block->set_value("stream_request", "GET /hello HTTP/1.0\r\nHost: 127.0.0.1:1985\r\n\r\n");

ronething · 2023-01-11T02:16:51Z

@tokers @soulbird Could you please take a look? thanks.

* upstream/master: (67 commits) fix: grpc-transcode plugin: fix map data population (apache#8731) change(jwt-auth): unify apisix/core/vault.lua and apisix/secret/vault.lua (apache#8660) feat: stream subsystem support consul_kv service discovery (apache#8633) fix(proxy-mirror): use with uri rewrite (apache#8718) ci: move helper script to the right dir (apache#8691) refactor(pubsub): simpify the get_cmd implementation (apache#8608) feat: stream subsystem support kubernetes service discovery (apache#8640) docs: fix deployment links (apache#8714) fix: remove backslash before slash when encoding (apache#8684) ci: kafka should register port in the zookeeper same as exposed (apache#8672) docs: fix plugin config naming (apache#8701) docs: fix code block (apache#8700) docs: rename kms to secret (apache#8697) docs: replace transparent logos with white background logos (apache#8689) fix: upgrade lua-resty-etcd to 1.10.3 (apache#8668) fix: upgrade casbin to 1.41.3 to improve performance (apache#8676) chore: make send_stream_request more clear (apache#8627) feat: stream subsystem support nacos service discovery (apache#8584) feat: stream subsystem support dns service discovery (apache#8593) refactor(admin): refactor resource routes (apache#8611) ...

ronething added 5 commits December 30, 2022 18:32

feat: stream subsystem support dns service discovery

219e2d9

fix: test case

85e11b1

Merge remote-tracking branch 'upstream/master' into feat/stream_route…

7b4c2dc

…_discovery_dns

Merge remote-tracking branch 'upstream/master' into feat/stream_route…

605f9c6

…_discovery_dns

docs: add L4 support

79767cd

ronething marked this pull request as ready for review January 4, 2023 10:55

spacewander reviewed Jan 5, 2023

View reviewed changes

spacewander mentioned this pull request Jan 5, 2023

feat: stream subsystem support nacos service discovery #8584

Merged

5 tasks

fix: review

7bd29d9

ronething requested a review from spacewander January 6, 2023 06:11

spacewander approved these changes Jan 6, 2023

View reviewed changes

soulbird approved these changes Jan 11, 2023

View reviewed changes

spacewander merged commit c9ed5d7 into apache:master Jan 12, 2023

ronething deleted the feat/stream_route_discovery_dns branch January 12, 2023 01:31

ronething mentioned this pull request Jan 12, 2023

bug: discovery/dns cause panic #8554

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: stream subsystem support dns service discovery #8593

feat: stream subsystem support dns service discovery #8593

ronething commented Dec 30, 2022 •

edited

Loading

spacewander Jan 5, 2023

ronething Jan 5, 2023 •

edited

Loading

ronething Jan 6, 2023

spacewander Jan 5, 2023

ronething Jan 5, 2023 •

edited

Loading

ronething Jan 6, 2023

spacewander Jan 5, 2023

ronething Jan 6, 2023

spacewander Jan 5, 2023

ronething commented Jan 11, 2023

	$block->set_value("stream_request", "\x47\x45\x54\x20\x2f\x68\x65\x6c\x6c\x6f\x20\x48\x54\x54\x50\x2f\x31\x2e\x30\x0d\x0a\x48\x6f\x73\x74\x3a\x20\x31\x32\x37\x2e\x30\x2e\x30\x2e\x31\x3a\x31\x39\x38\x35\x0d\x0a\x0d\x0a");
	$block->set_value("stream_request", "GET /hello HTTP/1.0\r\nHost: 127.0.0.1:1985\r\n\r\n");

feat: stream subsystem support dns service discovery #8593

feat: stream subsystem support dns service discovery #8593

Conversation

ronething commented Dec 30, 2022 • edited Loading

Description

Checklist

spacewander Jan 5, 2023

Choose a reason for hiding this comment

ronething Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

ronething Jan 6, 2023

Choose a reason for hiding this comment

spacewander Jan 5, 2023

Choose a reason for hiding this comment

ronething Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

ronething Jan 6, 2023

Choose a reason for hiding this comment

spacewander Jan 5, 2023

Choose a reason for hiding this comment

ronething Jan 6, 2023

Choose a reason for hiding this comment

spacewander Jan 5, 2023

Choose a reason for hiding this comment

ronething commented Jan 11, 2023

ronething commented Dec 30, 2022 •

edited

Loading

ronething Jan 5, 2023 •

edited

Loading

ronething Jan 5, 2023 •

edited

Loading