Skip to content
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

api: generate Go protobufs to a module subdirectory #28041

Closed
wants to merge 1 commit into from

Conversation

jpeach
Copy link
Contributor

@jpeach jpeach commented Jun 20, 2023

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 jpeach requested a review from alyssawilk as a code owner June 20, 2023 05:10
@jpeach
Copy link
Contributor Author

jpeach commented Jun 20, 2023

cc @phlax @alecholmez @sunjayBhatia

@jpeach
Copy link
Contributor Author

jpeach commented Jun 20, 2023

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
Copy link
Member

phlax commented Jun 20, 2023

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
Copy link
Member

phlax commented Jun 21, 2023

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

@jpeach
Copy link
Contributor Author

jpeach commented Jun 21, 2023

@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
Copy link
Contributor Author

jpeach commented Jun 22, 2023

@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 😬

@jpeach jpeach force-pushed the sync-go-proto-to-module branch 2 times, most recently from e0dd8a0 to 44fab6d Compare June 22, 2023 04:15
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.

Signed-off-by: James Peach <[email protected]>
@htuch
Copy link
Member

htuch commented Jun 22, 2023

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
Copy link
Contributor Author

jpeach commented Jun 23, 2023

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

@phlax
Copy link
Member

phlax commented Jun 23, 2023

/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
Copy link
Contributor

@htuch ping

@htuch
Copy link
Member

htuch commented Jun 27, 2023

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?

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing approval (see comment).

@markdroth
Copy link
Contributor

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

@jpeach
Copy link
Contributor Author

jpeach commented Jun 28, 2023

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
Copy link
Member

htuch commented Jun 29, 2023

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
Copy link
Member

htuch commented Jun 29, 2023

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

@phlax
Copy link
Member

phlax commented Jun 29, 2023

@phlax could you help?

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

@markdroth
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

sunjayBhatia commented Jun 29, 2023

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
Copy link
Contributor

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
Copy link
Member

dfawley commented Jun 29, 2023

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
Copy link
Member

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
Copy link
Member

dfawley commented Jun 29, 2023

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
Copy link
Member

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
Copy link
Member

dfawley commented Jun 29, 2023

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
Copy link
Contributor Author

jpeach commented Jun 30, 2023

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
Copy link
Contributor

markdroth commented Jun 30, 2023 via email

@dfawley
Copy link
Member

dfawley commented Jun 30, 2023

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 jpeach closed this Jun 30, 2023
@jpeach
Copy link
Contributor Author

jpeach commented Jun 30, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants