Stable release of Go generated protos

[dfawley]

Anyone writing a Go application that wishes to use Envoy protos will need a stable release of this repo. In case you are not aware, proto definitions cannot be generated independently by libraries, since multiple versions of the generated code for a single message that make their way into the same binary will cause a panic at init time. And without a stability guarantee, use of this repo as the official source of the protos is risky. Please create a stable v1.x release of this repo ASAP. Thank you.

Related to cncf/udpa#42 and envoyproxy/go-control-plane#391.

cc @htuch

[htuch]

@dfawley what do we need to do to make this "stable"? Is it just a release tag that is v1.0.0? I'm not sure why Go doesn't work with < v1.0.0 here, but we can probably do a v1.0.0 and iterate.

https://github.com/cncf/xds is now live and the canonical xDS-WG repo.

[dfawley]

@dfawley what do we need to do to make this "stable"? Is it just a release tag that is v1.0.0?

Yes, exactly!

I'm not sure why Go doesn't work with < v1.0.0 here, but we can probably do a v1.0.0 and iterate.

Everything technically works fine without a v1 release. This is purely symbolic -- releasing v1 is a signal that the APIs are stable, and that the owners will not break backward compatibility.

[phlax]

not sure if i follow entirely - but im wondering if a release is created should it not be the version number of the api - ie v3.x

[phlax]

nm - ignore me - this is xds version not envoy's api version

[dfawley]

Exactly. This repo can be at v1 and could eventually have envoy/xds v4-v7 protos in it as well as v3.

[htuch]

@markdroth @mattklein123 @lizan are you happy with cutting v1 from this repo? If so I can push the button.

[markdroth]

Assuming that we're really treating this just as a checkpoint tag for build-dependency purposes, I think this is fine. Before we would do any sort of really public release, I'd want to finish renaming the protos in the udpa tree to the xds tree. But that seems like not an issue here, so let's not block on it.

[mattklein123]

I don't have a strong opinion either, as long as per @markdroth we aren't locking ourselves into anything we can't change. If it's an arbitrary point in time for "reasons" that's fine with me.

[howardjohn]

My understanding is that tagging v1 means we are not going to make any semver incompatible changes. If we do, we would need to release as v2 (or v3, v4, etc..) which has impacts on consumers: https://blog.golang.org/v2-go-modules.

So the ask here is not just tagging it as v1, but also committing to semver backwards compatibility

[markdroth]

If we're really committing to not making any backward-incompatible changes at this point, then I would like to see us move the protos from the udpa tree to the xds tree before tagging v1.

[dfawley]

Technically Go wouldn't care about the protos themselves being moved, but that would affect the generated code. If the generated code moves from its current location, you will break our users. We currently depend upon:

github.com/cncf/udpa/go/udpa/data/orca/v1
github.com/cncf/udpa/go/udpa/type/v1

[htuch]

@markdroth the APIs are already subject to https://github.com/envoyproxy/envoy/blob/main/api/API_VERSIONING.md.

At the very least, https://github.com/cncf/udpa/blob/main/udpa/type/v1/typed_struct.proto can't be moved in a breaking way.

Annotations can be debated, and ORCA we can probably break as nobody is using it in production as far as anyone knows..

[markdroth]

At minimum, it seems like we should copy the protos in the udpa tree into the xds tree and tell people to start using those, even if the ones in the udpa tree need to stick around for backward-compatibility.

It would be nice if there was a reasonable way to maybe have the ones in the udpa tree be auto-generated from the ones in the xds tree, so that we have only one copy to maintain.

[dfawley]

While no one may be using ORCA in production, grpc-go uses the generated protos in several places. C and Java also use ORCA, but they generate their own protos so there is less concern about breakage being caused by a (re)move. Should we remove all our code that depends upon it?

[markdroth]

No, we shouldn't remove the code that uses ORCA -- we are going to need that functionality. But once we have the protos in the new locations, we should update our code to use those new locations.

[dfawley]

We can copy them to a new location here and update our existing code to reference it. Do we ever need to delete the old generated code (within a year or two)? If not, I think we'll be fine with that.

[markdroth]

(Sorry, hit the wrong button.)

Doug, if we copy them to a new location before we tag v1, and we update our current code to use v1 and use the new locations, then why would it matter if we later remove the old locations? Or are you concerned about other users that may be using the old locations?

[dfawley]

If someone is using an old release of gRPC-Go that references the deleted things, their build can break when those things are removed if:

1. we delete the whole UDPA repo, or
2. they reference (directly or indirectly via another dependency) the UDPA repo at a commit after we delete those files. This can be mitigated by making generated code depend upon an updated version of gRPC when we delete them.

[markdroth]

When you say "the UDPA repo", I assume you actually mean cncf/xds.

I don't think we should be worried about deleting the whole repo. I don't think we're going to plan to do that, and if we were, tagging a v1 in the repo probably wouldn't help anyway. :)

Since the second case you mentioned can be mitigated, it sounds like there's no real problem here?

[dfawley]

Since the second case you mentioned can be mitigated, it sounds like there's no real problem here?

If we do things right we should be okay for ORCA, but we will need to coordinate things carefully. I don't think we can move the typed struct proto to xds unless it's done in a major envoy version bump, as @htuch was implying.

[htuch]

Yeah, we can't move at least typed struct, so we can't kill this udpa tree until the next major version bump. ORCA can move. The only discretionary thing is annotations. I think it's safe to just change them in Envoy, in particular if we maintain the same extension IDs, but my intuition is sometimes broken when it comes to the complexities around backwards breaking changes across things like proto language bindings.

Anyway, I don't feel super strongly one way or the other. @markdroth I think you get to set the plan here as long as we don't break typed struct.

[htuch]

Friendly ping @markdroth. What should we do before declaring 1.0? :)

[derekperkins]

Are there any blockers before releasing v1?

[markdroth]

Here's my proposal:

1. In the xds directory, create a duplicate copy of everything under the udpa directory.
2. Everywhere in xDS where we use something from the udpa directory, change it to use the corresponding type from the xds directory instead, with whatever backward-compatibility is required. For annotations, hopefully we can just change them directly to use the new ones without keeping the old ones. For TypedStruct, change both Envoy and gRPC to handle both udpa.type.v1.TypedStruct and xds.type.v1.TypedStruct. For ORCA, hopefully we can just change the code that exists in gRPC to use the new location.
3. In the udpa directory, remove anything that does not actually need to remain there to avoid API breakage. For anything that does need to remain, mark it as deprecated and add very visible comments indicating that any new code should use the copy in the xds directory instead. (Ideally, the only thing we should need to keep here is TypedStruct, but if we also wind up needing to keep the ORCA protos for some reason, we can live with that.)
4. Tag v1.

Thoughts...?

[htuch]

@markdroth sounds like a plan to me. Since we never reference TypedStruct directly, we just need to update code, I think we can break annotations for things like deprecation etc.

[markdroth]

It occurs to me that we probably won't be able to actually completely remove udpa.type.v1.TypedStruct, since there are probably control planes already using it, and clients will need to continue to support it for a while. I suspect that will have to stick around for a while, although we can mark it as deprecated.