-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
Allow user to separate http and grpc server #15446
Conversation
Making as draft as I still need to implement more tests |
A quick comment: suggest to extract the first commit into a separate PR, so that we can approve & merge it firstly. |
I believe adding a new user facing flag " From users perspective, I think it still makes sense to multiplex both gRPC and HTTP (including 1 and 2) using one port ( gRPC-go has always been a pain point in etcd, which is still depending on some experimental APIs right now (see #15145), one of which is grpcServer.ServeHTTP. I think
We (most etcd contributors) are not network (e.g. gRPC, golang.org/x/net, etc) experts, so it would be great if we can get more feedback from network experts. cc @dfawley @aojea |
cc K8s folks @liggitt @dims @nikhita @neolit123 @wojtek-t |
well, I just was trying to add some drama 😄 , but you can see similar comments from its own author grpc/grpc-go#586 (comment) in this issue that comments about the performance regression of using ServeHTTP
and on the documentation it is still considered Experimental |
@ahrtr - I agree that all changes requiring action from customers are difficult and should go through a deep consideration. I think that this PR is not yet controversial and not introducing the breaking change. It's just adding new |
15f13fb
to
dde34f2
Compare
Hmm cmux test failure ?!
Can't repro locally. |
/cc |
I think I run into this as well. https://github.com/etcd-io/etcd/actions/runs/4439049893/jobs/7790956875. |
@fuweid Thanks for confirming. Looks like a flake :( |
If no one takes it, please assign it to me. Thanks~ 🤓 |
I just added the test :(, so let me take responsibility. |
258ceab
to
39b2dfc
Compare
} | ||
|
||
sctx := sctxs[addr] | ||
if sctx == nil { |
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.
It's strange but it is the status quo...
// net.Listener will rewrite ipv4 0.0.0.0 to ipv6 [::], breaking | ||
// hosts that disable ipv6. So, use the address given by the user. |
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.
Should the comment also be moved up to the appropriate location?
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.
Not sure what this comment refers to. I didn't move it.
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.
It was introduced by #8221 to fix the grpc gateway creating grpc connections using [::] caused by sctx.listener.addr()
when ipv6 is disabled but user configures 0.0.0.0 as listen client urls.
I think we can leave it as it is right now and improve it later.
@ptabor Anyway, I agree this PR is the best choice for 3.4 and 3.5, and overall looks good. Great work! @serathius . (Please resolve the couple of minor comments from @chaochn47 ) |
39b2dfc
to
fb7d0d3
Compare
Overall it's good for simplicity, however I don't think this should apply in this case. Aside http v2 and grpcgateway APIs that should be removed, all remaining http endpoints are administrative and users should not care about them. Please think in terms different user personas, user that connects to etcd in their application and administrator that manages the etcd (healthchecks, upgrades, etc) it could be a human or automated operator. With this in mind it seems not only unnecessary but also wrong to mix client and administrative endpoints. As an administrator I would prefer to bind hide administrative endpoints from external traffic by binding them to local address. I don't think this was discussed before as etcd API is usually already well protected with access requiring mtls. Still I think it's worth to consider starting an effort to clean up etcd API and have those personal in mind. If there is any API (maybe version) that is useful for users, it should have it's alternative in grpc. If there is an administrative API available to user that should require higher privilege, we should consider moving it to separate port. |
server/embed/etcd.go
Outdated
addr = u.Host | ||
if u.Scheme == "unix" || u.Scheme == "unixs" { | ||
addr = u.Host + u.Path | ||
} | ||
secure = u.Scheme == "https" || u.Scheme == "unixs" | ||
network = "tcp" | ||
if u.Scheme == "unix" || u.Scheme == "unixs" { | ||
network = "unix" | ||
} |
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.
addr = u.Host | |
if u.Scheme == "unix" || u.Scheme == "unixs" { | |
addr = u.Host + u.Path | |
} | |
secure = u.Scheme == "https" || u.Scheme == "unixs" | |
network = "tcp" | |
if u.Scheme == "unix" || u.Scheme == "unixs" { | |
network = "unix" | |
} | |
addr = u.Host | |
network = "tcp" | |
if u.Scheme == "unix" || u.Scheme == "unixs" { | |
addr = u.Host + u.Path | |
network = "unix" | |
} | |
secure = u.Scheme == "https" || u.Scheme == "unixs" |
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.
LGTM with a minor comment.
cc @ptabor @chaochn47 @fuweid to double check.
"List of URLs to listen on for client grpc traffic. Http services are available as long --listen-client-http-urls is not specified.", | ||
) | ||
fs.Var( | ||
flags.NewUniqueURLsWithExceptions("", ""), "listen-client-http-urls", |
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.
IMHO, there is a nuance that advertise-client-urls
to include listen-client-http-urls
after the split to allow communicating via http though just in case users rely on this behavior.
I think adding some warning message in the help string of the flag is enough for now.
Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
…er separate from http server Difference in load configuration for watch delay tests show how huge the impact is. Even with random write scheduler grpc under http server can only handle 500 KB with 2 seconds delay. On the other hand, separate grpc server easily hits 10, 100 or even 1000 MB within 100 miliseconds. Priority write scheduler that was used in most previous releases is far worse than random one. Tests configured to only 5 MB to avoid flakes and taking too long to fill etcd. Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
fb7d0d3
to
65add8c
Compare
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.
LGTM. Thank you.
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.
Thanks for the patch!
Left one comment for the defer handle, IIUC.
defer func(u url.URL) { | ||
if err == nil { | ||
defer func(addr string) { | ||
if err == nil || sctx.l == nil { |
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 we should pass sctx in the defer function instead of address.
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.
Nice find!, will send a separate PR with a fix.
Phase 2 of #15402 (comment)
Adding a flag to separate http server. No changes in default so the PR can be backported.
cc @ahrtr @ptabor @spzala @fuweid @aojea