api: generate Go protobufs to a module subdirectory

[jpeach]

Commit Message:

Per discussion with the go-control-plane maintainers, there is a desire to move all the Envoy API protobufs into a new module that is distinct from the go-control-plane xDS APIs. This is intended to eventually allow users to update prodobuf definitions without requiring new releases of go-control-plane.

This change updates the Go module path for API protobufs to github.com/envoyproxy/go-control-plane/proto, which will be an independently versioned Go module.

Note that this needs a corresponding CI machinery change in the go-control-plane respository.

Additional Description:

The go-control-plane buddy is envoyproxy/go-control-plane#723. Please do not merge this PR until we have consensus between the Envoy and go-control-plane projects.

Risk Level: Low
Testing: Manually ran the do_ci.sh script
Docs Changes:
Release Notes: I'll write a release note once we have consensus.
Platform Specific Features: N/A

[jpeach]

cc @phlax @alecholmez @sunjayBhatia

[jpeach]

I think the checks fail because I ended up using a different clang-format version:

-  map<string, JwtRequirement> requires = 3;
+  map<string, JwtRequirement>
+  requires = 3;

[phlax]

the proto formatter creates an "ideal" version and compares that - the easiest is just to apply the created diff

the dependencies fail is a network transient and seems to have passed

[phlax]

@jpeach i think the api fail is real - seems like the test protos need some kinda update/sync to match change

[jpeach]

@jpeach i think the api fail is real - seems like the test protos need some kinda update/sync to match change

Thanks, I'll take a look tomorrow or the day after. I was waiting to see whether any maintainers had comments :)

[jpeach]

@jpeach i think the api fail is real - seems like the test protos need some kinda update/sync to match change

Looks like I need to regenerate the golden output files somehow - can't find any indication of how to do that 😬

[htuch]

Generally with golden proto diffs, you just need to copy the value being compared against in the test to the checked-in proto. In your case, you should just be able to hand edit the golden protos with the new path structure and that should work.

[jpeach]

The remaining test failures looks like it it a timeout in docker image publishing?

[phlax]

/retest envoy

yeah - we have an underlying docker issue that we workaround imperfectly - its pretty infrequent now thankfully

@htuch some context re this PR (at least iiuc) ...

currently the published go namespaces in proto files dont match what is being actually published

a workaround has been proposed/added downstream in g-c-p but that still leaves the envoy api.go target ~incorrect, and that is what is being addressed here

essentially its a breaking change to fix the namespaces

(deferring to @jpeach if this is not a correct explanation)

[jmarantz]

@htuch ping

[htuch]

Actually, can we take a step back before merging? The changes seem mechanical and fine, but I want to understand why we have go-control-plane embedded into the xDS API protos in the first place? Surely the Go ecosystem use of xDS APIs is not only limited to go-control-plane. For example, we have the Go gRPC xDS client / server @dfawley @markdroth. Can we discuss this a little bit here?

[htuch]

Removing approval (see comment).

[markdroth]

I don't have any significant context here. @dfawley can probably advise when he gets back from vacation.

[jpeach]

Actually, can we take a step back before merging? The changes seem mechanical and fine, but I want to understand why we have go-control-plane embedded into the xDS API protos in the first place? Surely the Go ecosystem use of xDS APIs is not only limited to go-control-plane. For example, we have the Go gRPC xDS client / server @dfawley @markdroth.

Overall, the goal with this change is to decouple the protobuf versioning from the go-control-plane versioning. Projects often want to take updated Envoy protobuf changes without any go-control-plane changes.

I imagine that the reason protobufs are in the go-control-plane repository is historical. If people have an appetite for creating a different repository that just contains the generated Go protobufs, that's great and solves the problem too.

Can we discuss this a little bit here?

👍

[htuch]

Overall, the goal with this change is to decouple the protobuf versioning from the go-control-plane versioning. Projects often want to take updated Envoy protobuf changes without any go-control-plane changes.

I think I would recommend this. @phlax could you help?

[htuch]

FWIW, this is what the data-plane-api repo could have been used for, but I'm not sure about its future.

[phlax]

@phlax could you help?

happy to help if i can - i have few blind spots here tho so not sure iiuc proposals

[markdroth]

I think the long-term plan is to move the authoritative source of the xDS protos to the cncf/xds repo. I believe we already have automation in place in that repo to update the go protos with each PR, so I think this will be taken care of in the long term. I think the question here is just what we do in the interim.

[sunjayBhatia]

another goal that maybe was not explicitly stated in addition to the simple module split between xDS server code and protos was to tag/release the protos when an Envoy release is cut, which seems like it should be achievable regardless of where the code lands, but something to be aware of if we want to move the protos to another existing library

[markdroth]

It's not clear to me that we necessarily want to make a new release of the xDS protos whenever there's a new Envoy release. The xDS protos are not Envoy-specific.

[sunjayBhatia]

It's not clear to me that we necessarily want to make a new release of the xDS protos whenever there's a new Envoy release. The xDS protos are not Envoy-specific.

This isn't necessarily about xDS in the general case, but rather for compatibility with Envoy-specific xDS resources that an Envoy control plane might be using.

If new Bootstrap (not xDS but in the same package at the moment)/Listener/other xDS resource/etc. config options are added/deprecated/etc. and as a consumer I am bumping the Envoy version I am using/bundling/supporting, ideally I want to match what version of the library I am importing to get access to new features/compatibility.

We have this right now in Contour, we support a particular Envoy version and Contour version together and try to bump the imported xDS types when we bump Envoy so we get access to the newly deprecated/added config fields. We do this manually at the moment by finding the latest mirrored commit from Envoy in go-control-plane for a given release. If we lag behind the APIs for too long we can end up missing certain features are deprecated and then removed which makes upgrades cause downtime.

[markdroth]

Sure, I understand that use-case, and I agree that we should look at some mechanism to support that. But I think we need to do this in a way that will scale properly. We already have multiple data planes that support xDS (Envoy and gRPC, and there are 4 different xDS-aware gRPC implementations), and there will be more in the future, so I don't think it's going to scale to release a new version of the xDS protos every time any one of its data planes releases a new version. But maybe we can come up with some automated way for individual data planes to just add a tag or something as part of their release process.

In any case, I think that's a separate problem than the one described in this issue, so let's discuss it separately.

[dfawley]

I don't have an opinion on where exactly the protos should live long-term, but they should never be removed from their current location, or that will break gRPC and its users. Even if they aren't ever updated in their current location, they should remain there for a very long time (1yr+), if not forever.

Per discussion with the go-control-plane maintainers, there is a desire to move all the Envoy API protobufs into a new module that is distinct from the go-control-plane xDS APIs. This is intended to eventually allow users to update prodobuf definitions without requiring new releases of go-control-plane.

This change updates the Go module path for API protobufs to github.com/envoyproxy/go-control-plane/proto, which will be an independently versioned Go module.

Are all the protos in the go-control-plane repot not already under the envoy directory? That was my assumption.

[sunjayBhatia]

I don't have an opinion on where exactly the protos should live long-term, but they should never be removed from their current location, or that will break gRPC and its users. Even if they aren't ever updated in their current location, they should remain there for a very long time (1yr+), if not forever.

yeah I think the current proposal is to either leave the existing protos mirrored in their current package location for a few releases of go-control-plane, whether they are continually updated or not is still up for discussion

Are all the protos in the go-control-plane repot not already under the envoy directory? That was my assumption.

Adding a new module with the same import path as an existing package has some complications/friction so we were looking at adding the new module under a new import path to avoid any issues (so that existing import paths continue to work without any new module dependencies, and "upgrading" to using the new module is an opt-in process), more context in the threads of this PR of course: envoyproxy/go-control-plane#714

[dfawley]

Adding a new module with the same import path as an existing package has some complications/friction

IIUC there's some minor inconveniences you have to go through when first creating the new module, but it's not all that difficult as long as you do things correctly by following the steps outlined by the official Go documentation. It seems worthwhile to go through a little trouble if it means you don't break existing users and don't need to have two copies of all the protos. I'll read through the other PR you linked, though, because maybe I'm missing something in the context there.

[sunjayBhatia]

Adding a new module with the same import path as an existing package has some complications/friction

IIUC there's some minor inconveniences you have to go through when first creating the new module, but it's not all that difficult as long as you do things correctly by following the steps outlined by the official Go documentation. It seems worthwhile to go through a little trouble if it means you don't break existing users and don't need to have two copies of all the protos. I'll read through the other PR you linked, though, because maybe I'm missing something in the context there.

yeah if there is a way around the "ambiguous" package imports from switching an existing package to a module that would be great, I think this arises when you include the new module but have another dependency that still depends on the package from an older version of the library (e.g. contour imports the new go-control-plane and go-control-plane/envoy modules, but prometheus or grpc still depend on an older go-control-plane module with go-control-plane/envoy as a package, so resolving that import to a module dependency becomes ambiguous)

didn't try too hard to get past that myself when I was trying it out, but if there is a way that would be great

[dfawley]

didn't try too hard to get past that myself when I was trying it out, but if there is a way that would be great

This is a step-by-step process for splitting an existing package into a new module: https://github.com/golang/go/wiki/Modules#is-it-possible-to-add-a-module-to-a-multi-module-repository

[jpeach]

Are all the protos in the go-control-plane repot not already under the envoy directory? That was my assumption.

The go-control-plane repo has Envoy API protobufs in the "/envoy" and "/contrib" directories. These are synced from the Envoy repo and ought to both correspond to the same Envoy version. This is why we have this proposal to bump them down a directory level into the "/protos" directory.

go-control-plane also contains https://github.com/envoyproxy/ratelimit protobufs in "/ratelimit". I guess there's some history there too, but it seems to me that the same issues around module separate apply to that too.

[markdroth]

On Thu, Jun 29, 2023 at 6:28 PM James Peach ***@***.***> wrote: It's not clear to me that we necessarily want to make a new release of the xDS protos whenever there's a new Envoy release. The xDS protos are not Envoy-specific. I'm not sure about this. The Envoy repository is the source of truth for the protobufs and they change when Envoy versions are released. I've always thought of them as Envoy-specific. They pretty fundamentally embody Envoy concepts (even though you can clearly write systems that embody the same concepts). There's no versioning of these protobufs that is independent of Envoy versions AFAIK.

The xDS protos are *not* Envoy-specific. They started out that way, but with the introduction of other xDS-enabled data planes like gRPC, that is no longer the case. The authoritative source of truth of these protos will eventually move to the cncf/xds repo, as per xDS Proto Repo Migration <https://docs.google.com/document/d/1dJZXIqqs0fFJlk8YdLmm0Q-cNefxXhAh-UIf0KyYyR4/edit>. The only reason that hasn't happened already is that we haven't yet found someone to work on the Envoy-side tooling changes.

[dfawley]

This is why we have this proposal to bump them down a directory level into the "/protos" directory.

Another option could be to have multiple modules: envoy, contrib and ratelimit. It may not be ideal, but this would avoid the churn (and build-time breakages) from moving them. If we decide to move them, using a different repo instead might be better, but note that if multiple generated versions of a proto file are imported into the same binary, it will panic at init time due to https://protobuf.dev/reference/go/faq/#namespace-conflict -- this might be enough justification to leave them where they are.

[jpeach]

Closing this PR since it's clear that this approach wont work.