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

RFC: Sync Envoy protobufs to a nested proto module. #723

Closed
wants to merge 1 commit into from

Conversation

jpeach
Copy link
Contributor

@jpeach jpeach commented Jun 14, 2023

Create a new "proto" directory for publishing the Envoy API protobufs. This lets core and contrib protobuf APIs be packaged in the same module, which is versioned independently of the main Go control plane.

This PR is an RFC to show people what the combined envoy+contrib module would look like. The GitHub envoy sync action ran on my fork, and you can see the results here.

Per the discussion in #714, this approach retains compatibility for existing users of go-control-plane, since we do no remove any existing code (we just stop updating it). Note that if everyone like this approach, we will need to update all the option go_package specifications in the Envoy protobufs. This means that when projects do adopt the new module, they will need to edit all their import paths.

Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

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

+1 to this though I think it also needs a change (can maybe be a follow up to this PR) to switch the root module and code to import and reference this nested module, especially if we stop syncing the protos to the original package

if we want to be more conservative we could sync to both locations until the next release, and then stop syncing to the original after that; not sure how that will work with the actual generation bits since I haven't tried it (maybe change the go_package to the package path containing protos but also do a run where we do an ad-hoc sed replace on that option?), so maybe more headache than we want to deal with

@jpeach
Copy link
Contributor Author

jpeach commented Jun 14, 2023

+1 to this though I think it also needs a change (can maybe be a follow up to this PR) to switch the root module and code to import and reference this nested module, especially if we stop syncing the protos to the original package

Yup, I was assuming that we would make that change in #714.

@jpeach
Copy link
Contributor Author

jpeach commented Jun 15, 2023

I tested the Envoy build with the import path change, and AFAICT Bazel doesn't do anything with the go_package option. We would need to update those, but also figure out how to make Bazel do what we want.

For example:

diff --git api/envoy/api/v2/core/base.proto api/envoy/api/v2/core/base.proto
index 94b346bc3e..9c76576bb2 100644
--- api/envoy/api/v2/core/base.proto
+++ api/envoy/api/v2/core/base.proto
@@ -21,7 +21,7 @@ import public "envoy/api/v2/core/socket_option.proto";
 option java_package = "io.envoyproxy.envoy.api.v2.core";
 option java_outer_classname = "BaseProto";
 option java_multiple_files = true;
-option go_package = "github.com/envoyproxy/go-control-plane/envoy/api/v2/core";
+option go_package = "github.com/envoyproxy/go-control-plane/proto/envoy/api/v2/core";
 option (udpa.annotations.file_migrate).move_to_package = "envoy.config.core.v3";
 option (udpa.annotations.file_status).package_version_status = FROZEN;

Doesn't change the output path:

$ find build_go | grep base.pb
build_go/envoy/config/core/v3/base.pb.go
build_go/envoy/config/core/v3/base.pb.validate.go
build_go/envoy/api/v2/core/base.pb.go
build_go/envoy/api/v2/core/base.pb.validate.go

Doesn't change the import path:

$ grep -r envoy/config/core/v3 build_go | head
build_go/envoy/config/metrics/v3/metrics_service.pb.validate.go:	v3 "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
build_go/envoy/config/metrics/v3/stats.pb.go:	v31 "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
build_go/envoy/config/metrics/v3/metrics_service.pb.go:	v3 "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"

@jpeach
Copy link
Contributor Author

jpeach commented Jun 15, 2023

OK, this diff seems to do what we want:

$ git diff api/bazel ci/
diff --git api/bazel/api_build_system.bzl api/bazel/api_build_system.bzl
index 0e73ceb8e1..195f199d23 100644
--- api/bazel/api_build_system.bzl
+++ api/bazel/api_build_system.bzl
@@ -17,7 +17,7 @@ _PY_PROTO_SUFFIX = "_py_proto"
 _CC_PROTO_SUFFIX = "_cc_proto"
 _CC_GRPC_SUFFIX = "_cc_grpc"
 _GO_PROTO_SUFFIX = "_go_proto"
-_GO_IMPORTPATH_PREFIX = "github.com/envoyproxy/go-control-plane/"
+_GO_IMPORTPATH_PREFIX = "github.com/envoyproxy/go-control-plane/proto/"

 _COMMON_PROTO_DEPS = [
     "@com_google_protobuf//:any_proto",
diff --git ci/do_ci.sh ci/do_ci.sh
index c320a7a29e..35d0a5e77f 100755
--- ci/do_ci.sh
+++ ci/do_ci.sh
@@ -220,7 +220,7 @@ case $CI_TARGET in
         if [[ -z "$NO_BUILD_SETUP" ]]; then
             setup_clang_toolchain
         fi
-        GO_IMPORT_BASE="github.com/envoyproxy/go-control-plane"
+        GO_IMPORT_BASE="github.com/envoyproxy/go-control-plane/proto"
         GO_TARGETS=(@envoy_api//...)
         read -r -a GO_PROTOS <<< "$(bazel query "${BAZEL_GLOBAL_OPTIONS[@]}" "kind('go_proto_library', ${GO_TARGETS[*]})" | tr '\n' ' ')"
         echo "${GO_PROTOS[@]}" | grep -q envoy_api || echo "No go proto targets found"

Create a new "proto" directory for publishing the Envoy API protobufs.
This lets core and contrib protobuf APIs be packaged in the same module,
which is versioned independently of the main Go control plane.

Signed-off-by: James Peach <[email protected]>
@jpeach
Copy link
Contributor Author

jpeach commented Jun 20, 2023

I updated this to assume the changes in envoyproxy/envoy#28041 (i.e. the Envoy scripts generate Go code to the "proto" subdirectory).

@howardjohn
Copy link
Contributor

What happens when we import the new version of go-control-plane, but also import something like prometheus, grpc, etc that depend on an old version. Will we get duplicate protobufs which will cause panic?

@jpeach
Copy link
Contributor Author

jpeach commented Jun 28, 2023

What happens when we import the new version of go-control-plane, but also import something like prometheus, grpc, etc that depend on an old version. Will we get duplicate protobufs which will cause panic?

Yeh, the way I read it, if you imported the old and the new packages, the init functions in both would run and try to register the types. This would error with "foo is already registered", and the caller would panic (actually it would panic in ignoreConflict before that). You'd have to set the conflict knob in your build.

If I'm reading it right, prometheus gets go-control-plane via grpc, so I bet that basically every consumer of go-control-plane would be the same.

That said, the local experiments @sunjayBhatia tried with Contour didn't panic, so maybe it's OK. Definitely need to do some more experiments and understand what behavior to expect though. It probably depends on having a specific code path in the app that imports the same message via grpx and go-control-plane.

@jpeach
Copy link
Contributor Author

jpeach commented Jun 28, 2023

What happens when we import the new version of go-control-plane, but also import something like prometheus, grpc, etc that depend on an old version. Will we get duplicate protobufs which will cause panic?

Yeh, the way I read it, if you imported the old and the new packages, the init functions in both would run and try to register the types. This would error with "foo is already registered", and the caller would panic (actually it would panic in ignoreConflict before that). You'd have to set the conflict knob in your build.

If I'm reading it right, prometheus gets go-control-plane via grpc, so I bet that basically every consumer of go-control-plane would be the same.

That said, the local experiments @sunjayBhatia tried with Contour didn't panic, so maybe it's OK. Definitely need to do some more experiments and understand what behavior to expect though. It probably depends on having a specific code path in the app that imports the same message via grpx and go-control-plane.

But I'm not sure whether there is a robust way around this? Even if we move to new major versions of all the modules, the protobuf registry is still global and we can't update dependencies in lock-step.

@jpeach
Copy link
Contributor Author

jpeach commented Jun 30, 2023

@howardjohn I verified that the protobuf registry does panic if you mix grpc with envoy APIs from a different package"

package main

import (
        "fmt"

        _ "github.com/envoyproxy/go-control-plane/proto/envoy/config/core/v3"
        _ "google.golang.org/grpc/xds"

        "google.golang.org/protobuf/reflect/protoreflect"
        "google.golang.org/protobuf/reflect/protoregistry"
)

func main() {

        protoregistry.GlobalTypes.RangeMessages(
                func(m protoreflect.MessageType) bool {
                        fmt.Println(m.Descriptor().FullName())
                        return true
                },
        )
}

This results in:

$ go run ./main.go
main.go:8:2: no required module provides package github.com/envoyproxy/go-control-plane/proto; to add it:
	go get github.com/envoyproxy/go-control-plane/proto
smelt:xds-proto-test jpeach$ go run ./main.go
panic: proto: file "envoy/annotations/deprecation.proto" is already registered
	previously from: "github.com/envoyproxy/go-control-plane/envoy/annotations"
	currently from:  "github.com/envoyproxy/go-control-plane/proto/envoy/annotations"
See https://protobuf.dev/reference/go/faq#namespace-conflict


goroutine 1 [running]:
google.golang.org/protobuf/reflect/protoregistry.glob..func1({0x1d85e20?, 0xc0002eb520?}, {0x1d85e20?, 0xc0002eb560})
	/Users/jpeach/go/pkg/mod/google.golang.org/[email protected]/reflect/protoregistry/registry.go:56 +0x1ee
google.golang.org/protobuf/reflect/protoregistry.(*Files).RegisterFile(0xc0001aa000, {0x1d9a158?, 0xc000305180?})
	/Users/jpeach/go/pkg/mod/google.golang.org/[email protected]/reflect/protoregistry/registry.go:130 +0x39f
google.golang.org/protobuf/internal/filedesc.Builder.Build({{0x1a1bd88, 0x3e}, {0x257b180, 0x22c, 0x22c}, 0x0, 0x0, 0x4, 0x0, {0x1d917a0, ...}, ...})
	/Users/jpeach/go/pkg/mod/google.golang.org/[email protected]/internal/filedesc/build.go:112 +0x1d6
google.golang.org/protobuf/internal/filetype.Builder.Build({{{0x1a1bd88, 0x3e}, {0x257b180, 0x22c, 0x22c}, 0x0, 0x0, 0x4, 0x0, {0x0, ...}, ...}, ...})
	/Users/jpeach/go/pkg/mod/google.golang.org/[email protected]/internal/filetype/build.go:138 +0x1b8
github.com/envoyproxy/go-control-plane/proto/envoy/annotations.file_envoy_annotations_deprecation_proto_init()
	/Users/jpeach/upstream/go-control-plane/proto/envoy/annotations/deprecation.pb.go:154 +0x198
github.com/envoyproxy/go-control-plane/proto/envoy/annotations.init.0()
	/Users/jpeach/upstream/go-control-plane/proto/envoy/annotations/deprecation.pb.go:136 +0x17
exit status 2
smelt:xds-proto-test jpeach$ go run ./main.go
panic: proto: file "envoy/type/v3/hash_policy.proto" is already registered
	previously from: "github.com/envoyproxy/go-control-plane/proto/envoy/type/v3"
	currently from:  "github.com/envoyproxy/go-control-plane/envoy/type/v3"
See https://protobuf.dev/reference/go/faq#namespace-conflict


goroutine 1 [running]:
google.golang.org/protobuf/reflect/protoregistry.glob..func1({0x1d85e40?, 0xc0001c4830?}, {0x1d85e40?, 0xc0001c4870})
	/Users/jpeach/go/pkg/mod/google.golang.org/[email protected]/reflect/protoregistry/registry.go:56 +0x1ee
google.golang.org/protobuf/reflect/protoregistry.(*Files).RegisterFile(0xc000010030, {0x1d9a178?, 0xc0001c6380?})
	/Users/jpeach/go/pkg/mod/google.golang.org/[email protected]/reflect/protoregistry/registry.go:130 +0x39f
google.golang.org/protobuf/internal/filedesc.Builder.Build({{0x19f29b5, 0x34}, {0x257ba60, 0x23c, 0x23c}, 0x0, 0x3, 0x0, 0x0, {0x1d917c0, ...}, ...})
	/Users/jpeach/go/pkg/mod/google.golang.org/[email protected]/internal/filedesc/build.go:112 +0x1d6
google.golang.org/protobuf/internal/filetype.Builder.Build({{{0x19f29b5, 0x34}, {0x257ba60, 0x23c, 0x23c}, 0x0, 0x3, 0x0, 0x0, {0x0, ...}, ...}, ...})
	/Users/jpeach/go/pkg/mod/google.golang.org/[email protected]/internal/filetype/build.go:138 +0x1b8
github.com/envoyproxy/go-control-plane/envoy/type/v3.file_envoy_type_v3_hash_policy_proto_init()
	/Users/jpeach/go/pkg/mod/github.com/envoyproxy/[email protected]/envoy/type/v3/hash_policy.pb.go:327 +0x218
github.com/envoyproxy/go-control-plane/envoy/type/v3.init.0()
	/Users/jpeach/go/pkg/mod/github.com/envoyproxy/[email protected]/envoy/type/v3/hash_policy.pb.go:267 +0x17
exit status 2

@jpeach jpeach mentioned this pull request Jun 30, 2023
@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 won't work.

@jpeach jpeach deleted the sync-to-protos-module branch June 30, 2023 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants