Routing: Statistics and Service Support #229
Conversation
|
Client test code: package main
import (
"context"
"fmt"
"google.golang.org/grpc"
"v2ray.com/core/app/router/command"
)
func main() {
conn, err := grpc.DialContext(context.Background(), "127.0.0.1:10086", grpc.WithInsecure(), grpc.WithBlock())
if err != nil {
fmt.Println(err)
return
}
client := command.NewRoutingServiceClient(conn)
stream, err := client.SubscribeRoutingStats(context.Background(), &command.SubscribeRoutingStatsRequest{})
if err != nil {
fmt.Println(err)
return
}
for {
route, err := stream.Recv()
if err != nil {
fmt.Println(err)
return
}
fmt.Println(route)
}
}with related configs: "routing": {
"rules": [
{
"type": "field",
"inboundTag": ["api"],
"outboundTag": "api"
},
},
"inbounds": [
{
"tag": "api",
"protocol": "dokodemo-door",
"listen": "127.0.0.1",
"port": 10086,
"settings": {
"address": "127.0.0.1"
}
}
],
"stats": {
"routing": {
"enabled": true
}
},
"api": {
"tag": "api",
"services": ["RoutingService"]
},Result: Client after Killing V2ray Process: Client subscribing after there's already a client subscriber: |
|
@kslr There're too many Config struct exported without a comment in It is impossible to disable exported comment check in golint as per golang/lint#186, golang/lint#389. According to Codacy Configuration File, files with glob pattern could be disabled for some engines, like tool (golint), category (duplication). However, the 'exported' issue is a |
Vigilans
force-pushed
the
vigilans/stats-routing
branch
from
f65e454
to
7fee8c1
Compare
|
We can ignore certain folders |
| if d.routingStats != nil { | ||
| d.routingStats.Publish(route) | ||
| } |
There was a problem hiding this comment.
Routing statistics is published from Dispather instead of Router, so as to:
- By not placing publishing logic in
PickRoute, we can control whether the returned route should be published. - By not placing stats channel in
Router, router can be kept as a more light-weight feature.
infra/conf/stats.go
Outdated
| return &stats.ChannelConfig{ | ||
| SubscriberLimit: 1, | ||
| BufferSize: 16, | ||
| BroadcastTimeout: 100, | ||
| }, nil |
There was a problem hiding this comment.
The default value of ChannelConfig is set in config building process. All Channels exposed to client side are default to have at most 1 subscriber.
There was a problem hiding this comment.
Ref #105:
To regulate the usage of routing stats service:
- When routing stats is enabled, at most 1 client can subscribe to the gRPC stream channel. If one decides to enable routing stats, it should take responsibility to make sure its own client (GUI or user written program) subscribes successfully, so routing stats will not be managed by third-party applications.
There was a problem hiding this comment.
Btw, wouldn't it be unsafe if a gRPC service with these APIs is discovered in the first place? Should api service be implemented with some authentication mechanism like grpc's TLS credentials? This may also help export api service remotely.
There was a problem hiding this comment.
The api can be protected by routing, but we can also add TLS Credentials
There was a problem hiding this comment.
This is a breaking change, now we just need to make it configurable, and the default is 1(?)
There was a problem hiding this comment.
I will tweak the code to find a not ugly implementation, since integer value defaults to 0 and 0 means not limited in 'SubscriberLimit', unlike nil which can be viewed as field not set.
This is a breaking change
If the authentication is implemented, should it be disabled if credential/token not set for backward compatibility, or be required as breaking change to enforce security update?
There was a problem hiding this comment.
If the workload is the same, I suggest compatible. But if needed, this is not a problem.
set the listen and port to solve security problems
There was a problem hiding this comment.
set the listen and port to solve security problems
More hint on how to configure listen and port?
Vigilans
force-pushed
the
vigilans/stats-routing
branch
2 times, most recently
from
db15c22
to
a2c8270
Compare
| // ChannelConfig is the JSON config for app/stats.Channel. | ||
| type ChannelConfig struct { | ||
| Enabled bool `json:"enabled"` | ||
| SubscriberLimit *int32 `json:"subscriberLimit"` | ||
| } |
There was a problem hiding this comment.
subscriberLimit is exposed to user through JSON config now. Its value still defaults to 1, setting it to 0 to disable the limit on subscribers.
enabled field is still kept, to not confuse the user with setting an empty object {} to enable the channel. I also found the existence of enabled field in MuxConfig and SniffingConfig.
| runMultiTestCase(t, []TestCase{ | ||
| { | ||
| Input: `{}`, | ||
| Parser: createParser(), | ||
| Output: (*stats.ChannelConfig)(nil), | ||
| }, | ||
| { | ||
| Input: `{ | ||
| "enabled": true | ||
| }`, | ||
| Parser: createParser(), | ||
| Output: &stats.ChannelConfig{ | ||
| SubscriberLimit: 1, | ||
| BufferSize: 16, | ||
| BroadcastTimeout: 100, | ||
| }, | ||
| }, | ||
| { | ||
| Input: `{ | ||
| "enabled": true, | ||
| "subscriberLimit": 0 | ||
| }`, | ||
| Parser: createParser(), | ||
| Output: &stats.ChannelConfig{ | ||
| SubscriberLimit: 0, | ||
| BufferSize: 16, | ||
| BroadcastTimeout: 100, | ||
| }, | ||
| }, | ||
| { | ||
| Input: `{ | ||
| "subscriberLimit": 0 | ||
| }`, | ||
| Parser: createParser(), | ||
| Output: (*stats.ChannelConfig)(nil), | ||
| }, | ||
| }) |
There was a problem hiding this comment.
A test of parsing Channel JSON config is added.
Vigilans
force-pushed
the
vigilans/stats-routing
branch
from
a2c8270
to
37b561a
Compare
|
发现一个测试错误 === RUN TestSerivceTestRoute
panic: Fail in goroutine after TestServiceSubscribeRoutingStats has completed
goroutine 10 [running]:
testing.(*common).Fail(0xc00017a300)
/Users/runner/hostedtoolcache/go/1.15.2/x64/src/testing/testing.go:680 +0x125
testing.(*common).Error(0xc00017a300, 0xc000149e48, 0x1, 0x1)
/Users/runner/hostedtoolcache/go/1.15.2/x64/src/testing/testing.go:780 +0x78
v2ray.com/core/app/router/command_test.TestServiceSubscribeRoutingStats.func4(0xc00014e440, 0xc000040960, 0xc00000f5c0, 0xc000033b40, 0x8, 0x8, 0xc00017a300, 0xc00007cfc0, 0xc0000409c0)
/Users/runner/work/v2ray-core/v2ray-core/app/router/command/command_test.go:167 +0xd13
created by v2ray.com/core/app/router/command_test.TestServiceSubscribeRoutingStats
/Users/runner/work/v2ray-core/v2ray-core/app/router/command/command_test.go:91 +0x885
FAIL v2ray.com/core/app/router/command 0.243s |
Caused by: I should have exited the goroutine after the error is sent to error channel. Investigating why upstream statistics failed to receive. |
|
@kslr found the reason...the services blocked on (This rarely happens in real scenario since the channel has a buffer, and routing stats are not sent in a moment, so there are more time left for service stream to send messages. But in the test all testcases are published at once.) I am seeking to tweak the implementation of broadcast logic. |
|
@Vigilans are you ok? |
|
It has been open 120 days with no activity. Remove stale label or comment or this will be closed in 5 days |
Closes #105. This PR:
RoutingServiceto user through json config:With this config,
Stats Managerwill registers aStats Channelwith a key specified infeature/stats:Where this specific channel will be retrieved by both publisher and subscriber side:
So a routing statistics push model is established.