[3.5] Bump grpc-go to1.47 (and fix the connection-string format)

[chaochn47]

Split gRPC-go dependency update in release-3.5 into multiple PRs based on #16591 (comment).

Backport #14125

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

[chaochn47]

@ahrtr @serathius @jmhbnz @fuweid

[fuweid]

LGTM

[serathius]

Is there any security issue we are trying to fix by backporting?

3 people complaining about dependency issue doesn't seem like a sensible reason for backport. #15877 (comment)

cc @liggitt

[ahrtr]

Is there any security issue we are trying to fix by backporting?

Just spending 2 minutes to search "CVE" or "security" in grpc-go/releases. You will get the "YES" answer.

3 people complaining about dependency issue doesn't seem like a sensible reason for backport. #15877 (comment)

Why not? It's prevent some applications from integrating with etcd!

[ahrtr]

LGTM

thx @chaochn47

[serathius]

Is there any security issue we are trying to fix by backporting?

Just spending 2 minutes to search "CVE" or "security" in grpc-go/releases. You will get the "YES" answer.

Spent 10 seconds, didn't find anything worth backporting. Those "security" or "cve" are not related to grpc are unrelated to etcd security posture. For example a feature adding min/max TLS, or bump grpc dependency to address CVE 2022-32149.

3 people complaining about dependency issue doesn't seem like a sensible reason for backport. #15877 (comment)

Why not? It's prevent some applications from integrating with etcd!

What's a weak reason, you are comparing 3 applications vs all the remaining applications.

[ahrtr]

you are comparing 3 applications vs all the remaining applications.

It is not about 3 applications. It's about all applications which depend on both a newer grpc version (e.g. 1.52+) and etcd client 3.5 SDK.

[liggitt]

from #16591 (comment)

In order to use Outlier Detection, release-3.5 should be at least gRPC-go v1.50.0 onward. Working on it..

is it a goal to take new grpc features back to etcd release branches? propagating this many grpc updates to all etcd clients on patch updates is surprising to me

[liggitt]

when is etcd 1.6.0 targeted? major bumps in grpc lib versions that pull in breaking changes like package renames seem better to push to consumers in a minor release

[ahrtr]

when is etcd 1.6.0 targeted? major bumps in grpc lib versions that pull in breaking changes like package renames seem better to push to consumers in a minor release

Yes, that hit the point. Usually it isn't recommended to add any feature to a stable release (etcd 3.5 in this case), and users are supposed to integrate with etcd 3.6 to resolve such grpc incompatible issue. But unfortunately, based on the 3.6 roadmap, I don't think we will release 3.6.0 in near future (e.g. at least > 0.5 year).

[serathius]

when is etcd 1.6.0 targeted? major bumps in grpc lib versions that pull in breaking changes like package renames seem better to push to consumers in a minor release

Yes, that hit the point. Usually it isn't recommended to add any feature to a stable release (etcd 3.5 in this case), and users are supposed to integrate with etcd 3.6 to resolve such grpc incompatible issue. But unfortunately, based on the 3.6 roadmap, I don't think we will release 3.6.0 in near future (e.g. at least > 0.5 year).

Agree that it will be hard to release v3.6 in near future, however we should avoid the vicious cycle of backport anything because next release will not come.

[ahrtr]

is it a goal to take new grpc features back to etcd release branches?

The goal is to resolve users' pain points that any applications depending on a new grpc version are not able to integrate with etcd 3.5 sdk. gRPC is a very basic lib, so I imagine it will impact lots of applications which depend on etcd SDK as well.

however we should avoid the vicious cycle of backport anything because next release will not come.

See my comment above. We should handle it from pragmatic propective and case by case.

[ahrtr]

cc @dfawley

[chaochn47]

is it a goal to take new grpc features back to etcd release branches? propagating this many grpc updates to all etcd clients on patch updates is surprising to me.

No, etcd clients does not use this gRPC feature while consumer of etcd client could use this experimental feature on demand.

[rsafonseca]

@liggitt the goal is not to introduce new features, it's to maintain interoperability support for the latest stable branch (3.5)
There is no date for the release of 3.6 and this point has come up over and over again.

To be clear, the proposed changes should not be breaking, and the goal is not to add new features, simply to maintain the 3.5 branch which is the latest branch currently stable in ETCD, which is currently causing trouble to anyone who integrates with ETCD client and have other updated dependencies which don't use an old version of GRPC.

[mhutchinson]

Hi all, we're actively looking for this fix to be merged for the google/trillian project. We use dependabot to ensure our dependencies are kept up to date. Until a tagged version of etcd is released that uses a newer version of grpc, many of our dependency updates are blocked. We're not looking for any of the newer features in grpc that this backport may introduce.

Trillian's dependency graph will continue to become increasingly out of date until this PR is merged or the 3.6 version of etcd is released. This is a cost for us as maintainers to manage dependabot (which is normal running is a very automatic process), and more importantly, it means that our project and any projects that depend on it (e.g. Certificate Transparency, Sigstore) will be stuck on old/obsolete/vulnerable versions of a growing number of libraries.

[ahrtr]

@rsafonseca @mhutchinson Please feel free to bring the topic to tonight's etcd community meeting [I will not be able to join the meeting :(].

You need to subscribe etcd-dev@googlegroups.com (https://groups.google.com/g/etcd-dev) to gain permission to add topic to the google doc,

https://docs.google.com/document/d/16XEGyPBisZvmmoIHSZzv__LoyOeluC5a4x353CX0SIM/edit#heading=h.eu3cetgrd3ii

[jmhbnz]

On balance, I support this pull request merging, thanks @chaochn47 for raising.

As mentioned in our dependency management guidelines we generally don't bump dependencies for stable releases unless there is a security driver.

With that said, given how far away 3.6.0 is, and the input from the community on how not proceeding with the backport will negatively affect projects that depend on etcd stable releases (and how this will get worse over time). Then on the understanding that this backport is non breaking / transparent to clients my vote would be that we proceed.

[ahrtr]

Part of #16740

[ahrtr]

@chaochn47 please rebase this PR.

[ahrtr]

@chaochn47 can you please also bump grpc for 3.4?

[chaochn47]

Security is the top priority, I will work on this today. Thanks for the heads up! @ahrtr

[rsafonseca]

Just a reminder that this PR alone won't solve the dependency issues, it's just the first bump in a series that needs to happen to get to a version with the problem fixed (1.55+)

[ahrtr]

Just a reminder that this PR alone won't solve the dependency issues, it's just the first bump in a series that needs to happen to get to a version with the problem fixed (1.55+)

We are well aware of it, we will bump grpc-go step by step. FYI. #16740

[ahrtr]

Looks much better now. Should be ready to go. ping @serathius

[ahrtr]

LGTM

Thanks @chaochn47 for the quick response.

[chaochn47]

@chaochn47 can you please also bump grpc for 3.4?

I am on it. 3.4 google.golang.org/grpc is still on v1.26.0 :p

[ahrtr]

ping @wenjiaswe @serathius @fuweid @jmhbnz

[fuweid]

LGTM on new change

[wenjiaswe]

Agree the security issue is super important for all, us included. But also want to be extra cautious given this is a stable version. I took a quick look at the diff between grpc-go, there are quite a bit of new features, API changes, behavior changes and stuff.

@chaochn47 would you please kindly check the diff between grpc-go 1.41 and 1.47, and make sure there won't be any breaking changes? Let's just try to be extra careful, so many people are waiting for this patch, let's not break anyone.

[chaochn47]

@chaochn47 would you please kindly check the diff between grpc-go 1.41 and 1.47, and make sure there won't be any breaking changes? Let's just try to be extra careful, so many people are waiting for this patch, let's not break anyone.

Sure.

After reviewing change log of v1.42.0 and grpc/grpc-go@v1.41.0...v1.42.0, there are potentially three behavior changes.

• 1. grpc: Dial("unix://relative-path") no longer works (grpc: better RFC 3986 compliant target parsing grpc/grpc-go#4817). Use "unix://absolute-path" or "unix:relative-path" instead in accordance with our documentation.
  • 3.5 etcd binary does not allow UDS in the listen client urls flag since [release-3.5]pkg/types: Support Unix sockets in NewURLS #15940 has not yet merged.
• 2. transport: server transport will reject requests with connection header (transport: logic specified in A41 to support RBAC xDS HTTP Filter grpc/grpc-go#4803).
  • gRPC server transport will reject with END_STREAM when user uses HTTP1.1 request with connection header set dialing gRPC-gateway. This behavior is aligned with https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2.2 that prohibits such request with connection header. Existing user depends on the previous wrong behavior should change its usage.
• 3. grpc: support grpc.WithAuthority when secure credentials are used (grpc: better RFC 3986 compliant target parsing grpc/grpc-go#4817)
  • This is a new feature. Existing user of etcd 3.5 does not depend on this.

In conclusion: we should be good upgrading to v1.42.0. I will continue with v1.43.0 auditing..

[chaochn47]

Behavior Changes

• gRPC v1.43.0 change log

  • status: support wrapped errors in FromContextError grpc/grpc-go#4977
    • etcd does not uses this exported function.
  • config: remove retry disable via environment variable grpc/grpc-go#4922
    • etcd does not use gRPC retry option in the service config. We implement by the gRPC interceptor instead. So this is no-op for us. See more in Improving client call options flexibility and replace retry interceptor #16216

• gRPC v1.44.0 change log

  • no behavior changes. But it reveals that we should replace all grpc.WithInsecure() with grpc.WithTransportCredentials(insecure.NewCredentials()) based on grpc: minor improvement on WithInsecure() document grpc/grpc-go#5068 in 3.4/3.5/main

• gRPC v1.45.0 change log

  • server: handle context errors returned by service handler grpc/grpc-go#5156
    • Somehow etcd server already relies on this behavior in togRPCError "let gRPC server convert to codes.Canceled, codes.DeadlineExceeded"

• gRPC v1.46.0 change log

  • server: set TCP_USER_TIMEOUT socket option for linux grpc/grpc-go#5219
    • This will effectively change the default TCP user timeout as 20s in gRPC server transport layer. This is the same as the h2 keep alive ping timeout default value though. We should be good since client has implemented the same semantic in client: set TCP_USER_TIMEOUT socket option for linux grpc/grpc-go#2307.

• gRPC v1.47.0 change log

  • transport: add HTTP status and grpc status to response if POST check fails grpc/grpc-go#5364
    • etcd is not subject to this behavior change. It is aligned with https://grpc.github.io/grpc/core/md_doc__p_r_o_t_o_c_o_l-_h_t_t_p2.html..

[chaochn47]

Done all the auditing.. No outstanding breaking changes to etcd AFAIK.

[ahrtr]

Done all the auditing.. No outstanding breaking changes to etcd AFAIK.

thx @chaochn47 for the investigation. It aligns with my understanding. There is no obvious differences between 3.5 and 3.6 in term of grpc usage. If a grpc version works for 3.6, there is no reason that it doesn't work for 3.5.

But 3.4 is another story. see #16740 (comment). We can have more discussion in your following PR of bumping grpc for 3.4.

[wenjiaswe]

Thanks @chaochn47 !

1. grpc: Dial("unix://relative-path") no longer works (grpc: better RFC 3986 compliant target parsing grpc/grpc-go#4817). Use "unix://absolute-path" or "unix:relative-path" instead in accordance with our documentation. 3.5 etcd binary does not allow UDS in the listen client urls flag since [release-3.5]pkg/types: Support Unix sockets in NewURLS #15940 has not yet merged.

Do we need to change #15940 accordingly? Have we changed #12469 since main branch already updated grpc-go? If so, could you please open an issue for tracking, to the next 3.5 cut is blocked by it?

2. transport: server transport will reject requests with connection header (transport: logic specified in A41 to support RBAC xDS HTTP Filter grpc/grpc-go#4803).
   gRPC server transport will reject with END_STREAM when user uses HTTP1.1 request with connection header set dialing gRPC-gateway. This behavior is aligned with https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2.2 that prohibits such request with connection header. Existing user depends on the previous wrong behavior should change its usage.

This looks like a breaking change? @chaochn47 could you please make sure this is reflected in the CHANGELOG?

gRPC v1.44.0 change log, no behavior changes. But it reveals that we should replace all grpc.WithInsecure() with grpc.WithTransportCredentials(insecure.NewCredentials()) based on grpc/grpc-go#5068 in 3.4/3.5/main

Could you open a ticket as 3.5 release blocking issue for tracking?

cc @jmhbnz @serathius @ahrtr @fuweid could you please help to check if there is anything missing?

[ahrtr]

Do we need to change #15940 accordingly? Have we changed #12469 since main branch already updated grpc-go? If so, could you please open an issue for tracking, to the next 3.5 cut is blocked by it?

• I don't think 3.5 patch release is blocked by it;
• I don't see strong reason to reject the minor & safe feature for 3.5. Of course, since it's a feature (usually we don't backport feature to stable release), so I won't insist on it if others do not agree.

[chaochn47]

@wenjiaswe

Do we need to change #15940 accordingly? Have we changed #12469 since main branch already updated grpc-go? If so, could you please open an issue for tracking, to the next 3.5 cut is blocked by it?

No, etcd 3.5 never supports unix socket domain in the listen-client-urls. So I think it's not a blocker of this PR to resolve CVE. Does that make sense to you?

I just moved the discussion to the right place #15940 (comment).

This looks like a breaking change? @chaochn47 could you please make sure this is reflected in the CHANGELOG?

#16762 is drafted since CHANGELOG-3.5.md is in main branch.

Could you open a ticket as 3.5 release blocking issue for tracking?

Deprecated does not mean removed. 3.5 release is not blocked by this issue. Tracked in #15145 (comment) though.

[chaochn47]

Hi @wenjiaswe, @serathius, is it in a good shape to be merged and unblock the CVE fix?

[wenjiaswe]

Looks like everyone approved. I will go ahead and merge.