Skip to content

feat: stream subsystem support dns service discovery #8593

Merged
spacewander merged 6 commits into
apache:masterfrom
ronething:feat/stream_route_discovery_dns
Jan 12, 2023
Merged

feat: stream subsystem support dns service discovery #8593
spacewander merged 6 commits into
apache:masterfrom
ronething:feat/stream_route_discovery_dns

Conversation

@ronething

@ronething ronething commented Dec 30, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Fixes #7779

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)

@ronething
ronething marked this pull request as ready for review January 4, 2023 10:55
spacewander
spacewander reviewed Jan 5, 2023
View reviewed changes
Comment thread t/discovery/stream/dns.t Outdated
--- grep_error_log eval
qr/proxy request to \S+/
--- grep_error_log_out
proxy request to 127.0.0.1:nil

@spacewander spacewander Jan 5, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@ronething ronething Jan 5, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@ronething ronething Jan 6, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Comment thread t/discovery/stream/dns.t Outdated



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

@spacewander spacewander Jan 5, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@ronething ronething Jan 5, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i will remove these tests later.

@ronething ronething Jan 6, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done.

Comment thread t/discovery/stream/dns.t Outdated
type: roundrobin
id: 1
--- error_log
127.0.0.1:53

@spacewander spacewander Jan 5, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@ronething ronething Jan 6, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done.

Comment thread t/discovery/stream/dns.t Outdated

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");

@spacewander spacewander Jan 5, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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");

@spacewander spacewander mentioned this pull request Jan 5, 2023
Merged
5 tasks
@ronething
ronething requested a review from spacewander January 6, 2023 06:11
@ronething

ronething commented Jan 11, 2023

Copy link
Copy Markdown
Contributor Author

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

@spacewander
spacewander merged commit c9ed5d7 into apache:master Jan 12, 2023
@ronething
ronething deleted the feat/stream_route_discovery_dns branch January 12, 2023 01:31
@ronething ronething mentioned this pull request Jan 12, 2023
Closed
hongbinhsu added a commit to fitphp/apix that referenced this pull request Jan 30, 2023
@hongbinhsu
Merge branch 'upstream/master' into github/master
2e3529a 
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@spacewander spacewander spacewander approved these changes
@soulbird soulbird soulbird approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

feat: As a user, I want to support service discovery in the stream subsystem

3 participants

@ronething @spacewander @soulbird