-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
feat: stream subsystem support eureka service discovery #8583
feat: stream subsystem support eureka service discovery #8583
Conversation
We need to describe in the document that nacos service discovery is available at layer 4. |
docs/en/latest/discovery.md
Outdated
Access-Control-Max-Age: 3600 | ||
X-API-VERSION: v3 | ||
|
||
{"key":"\/apisix\/stream_routes\/1","value":{"update_time":1672369610,"id":"1","remote_addr":"127.0.0.1","create_time":1672106762,"upstream":{"discovery_type":"eureka","service_name":"APISIX-EUREKA","pass_host":"pass","scheme":"http","type":"roundrobin","hash_on":"vars"}}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this example is not so good because in the stream proxy, why we will have an upstream which schema is http
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i will add "scheme": "tcp"
to request body.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
t/stream-node/discovery/eureka.t
Outdated
@@ -0,0 +1,91 @@ | |||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move the test case under t/discovery/stream. So we don't need to move the t/stream-node
into the first CI group and also keep the alphabetical order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
t/stream-node/discovery/eureka.t
Outdated
|
||
#END | ||
--- stream_request eval | ||
"\x47\x45\x54\x20\x2f\x65\x75\x72\x65\x6b\x61\x2f\x61\x70\x70\x73\x2f\x41\x50\x49\x53\x49\x58\x2d\x45\x55\x52\x45\x4b\x41\x20\x48\x54\x54\x50\x2f\x31\x2e\x31\x0d\x0a\x48\x6f\x73\x74\x3a\x20\x31\x32\x37\x2e\x30\x2e\x30\x2e\x31\x3a\x31\x39\x38\x35\x0d\x0a\x43\x6f\x6e\x6e\x65\x63\x74\x69\x6f\x6e\x3a\x20\x63\x6c\x6f\x73\x65\x0d\x0a\x0d\x0a" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be better to use "GET ...." directly for plaintext requests so other people can understand what's sent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i need to send
GET /eureka/apps/APISIX-EUREKA HTTP/1.1
Host: 127.0.0.1:1985
Connection: close
which include "\r\n", can you tell me how to send plaintext requests, thanks.
t/stream-node/discovery/eureka.t
Outdated
type: roundrobin | ||
|
||
#END | ||
--- stream_enable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As your first test, we can omit --- stream_enable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
docs/en/latest/discovery.md
Outdated
@@ -182,6 +182,8 @@ discovery: | |||
|
|||
## Upstream setting | |||
|
|||
### routes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use L4 and L7 instead of routes, as the configuration actually takes effect in the upstream
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, i will change it later.
t/discovery/stream/eureka.t
Outdated
|
||
#END | ||
--- stream_request eval | ||
"\x47\x45\x54\x20\x2f\x65\x75\x72\x65\x6b\x61\x2f\x61\x70\x70\x73\x2f\x41\x50\x49\x53\x49\x58\x2d\x45\x55\x52\x45\x4b\x41\x20\x48\x54\x54\x50\x2f\x31\x2e\x31\x0d\x0a\x48\x6f\x73\x74\x3a\x20\x31\x32\x37\x2e\x30\x2e\x30\x2e\x31\x3a\x31\x39\x38\x35\x0d\x0a\x43\x6f\x6e\x6e\x65\x63\x74\x69\x6f\x6e\x3a\x20\x63\x6c\x6f\x73\x65\x0d\x0a\x0d\x0a" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can write \r\n
directly in the string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i use code like below and add comment. so maybe other people can understand what's sent.
add_block_preprocessor(sub {
my ($block) = @_;
if (!$block->stream_request) {
# GET /eureka/apps/APISIX-EUREKA HTTP/1.1\r\nHost: 127.0.0.1:1985\r\nConnection: close\r\n\r\n
$block->set_value("stream_request", "\x47\x45\x54\x20\x2f\x65\x75\x72\x65\x6b\x61\x2f\x61\x70\x70\x73\x2f\x41\x50\x49\x53\x49\x58\x2d\x45\x55\x52\x45\x4b\x41\x20\x48\x54\x54\x50\x2f\x31\x2e\x31\x0d\x0a\x48\x6f\x73\x74\x3a\x20\x31\x32\x37\x2e\x30\x2e\x30\x2e\x31\x3a\x31\x39\x38\x35\x0d\x0a\x43\x6f\x6e\x6e\x65\x63\x74\x69\x6f\x6e\x3a\x20\x63\x6c\x6f\x73\x65\x0d\x0a\x0d\x0a");
}
});
Description
Fixes #7779
now i only add eureka service discovery. and i will create other PRs for other service discovery.
because of
t/stream-node/discovery
need installdocker-compose.first.yml
server, so i change the yml file of ci.Checklist