Skip to content

feat: stream subsystem support eureka service discovery#8583

Merged
spacewander merged 9 commits into
apache:masterfrom
ronething:feat/stream_route_discovery_eureka
Jan 4, 2023
Merged

feat: stream subsystem support eureka service discovery#8583
spacewander merged 9 commits into
apache:masterfrom
ronething:feat/stream_route_discovery_eureka

Conversation

@ronething

@ronething ronething commented Dec 29, 2022

Copy link
Copy Markdown
Member

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 install docker-compose.first.yml server, so i change the yml file of ci.

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 December 29, 2022 09:52
@starsz
starsz requested review from soulbird and spacewander and removed request for spacewander December 30, 2022 02:17
starsz
starsz previously approved these changes Dec 30, 2022
@soulbird

Copy link
Copy Markdown
Contributor

We need to describe in the document that nacos service discovery is available at layer 4.

Comment thread 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"}}}

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done.

@@ -0,0 +1,91 @@
#

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.

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.

Copy link
Copy Markdown
Member 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/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"

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.

Would be better to use "GET ...." directly for plaintext requests so other people can understand what's sent.

Copy link
Copy Markdown
Member Author

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.

Comment thread t/stream-node/discovery/eureka.t Outdated
type: roundrobin

#END
--- stream_enable

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.

As your first test, we can omit --- stream_enable

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done.

Comment thread docs/en/latest/discovery.md Outdated

## Upstream setting

### routes

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.

We should use L4 and L7 instead of routes, as the configuration actually takes effect in the upstream

@ronething ronething Dec 30, 2022

Copy link
Copy Markdown
Member Author

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.

soulbird
soulbird previously approved these changes Dec 30, 2022
tokers
tokers previously approved these changes Jan 2, 2023
Comment thread 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"

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 you can write \r\n directly in the string?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

i had a try, but it seem not work. can you help me?

image

@ronething ronething Jan 3, 2023

Copy link
Copy Markdown
Member Author

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

});

@ronething
ronething dismissed stale reviews from tokers and soulbird via f1a073e January 3, 2023 06:27
@ronething
ronething requested a review from spacewander January 3, 2023 09:02
@spacewander
spacewander merged commit 9345425 into apache:master Jan 4, 2023
@ronething
ronething deleted the feat/stream_route_discovery_eureka branch January 4, 2023 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

5 participants