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

Import of gnostic-models and gnostic generated code leads to panic #397

Closed
pkwarren opened this issue Jun 6, 2023 · 8 comments · Fixed by #400
Closed

Import of gnostic-models and gnostic generated code leads to panic #397

pkwarren opened this issue Jun 6, 2023 · 8 comments · Fixed by #400

Comments

@pkwarren
Copy link
Contributor

pkwarren commented Jun 6, 2023

We've encountered an issue in recent versions of upstream libraries that mix dependencies - sometimes pulling in gnostic and other times gnostic-models. This leads to a panic at runtime, and it causes golangci-lint to fail with an internal error.

Here's a small example program illustrating the panic:

package main

import (
	"log"

	_ "github.com/google/gnostic-models/openapiv2"
	_ "github.com/google/gnostic/openapiv2"
)

func main() {
	log.Println("hello world")
}

This leads to a panic:

panic: proto: file "extensions/extension.proto" is already registered
        previously from: "github.com/google/gnostic-models/extensions"
        currently from:  "github.com/google/gnostic/extensions"
See https://protobuf.dev/reference/go/faq#namespace-conflict


goroutine 1 [running]:
google.golang.org/protobuf/reflect/protoregistry.glob..func1({0x102932c68?, 0x1400010b180?}, {0x102932c68?, 0x1400010b1c0})
        /Users/pkw/go/pkg/mod/google.golang.org/protobuf@v1.30.0/reflect/protoregistry/registry.go:56 +0x200
google.golang.org/protobuf/reflect/protoregistry.(*Files).RegisterFile(0x14000114060, {0x1029396a0?, 0x14000119340?})
        /Users/pkw/go/pkg/mod/google.golang.org/protobuf@v1.30.0/reflect/protoregistry/registry.go:130 +0x314
google.golang.org/protobuf/internal/filedesc.Builder.Build({{0x102872ea6, 0x24}, {0x102bf5aa0, 0x282, 0x282}, 0x0, 0x4, 0x0, 0x0, {0x102934758, ...}, ...})
        /Users/pkw/go/pkg/mod/google.golang.org/protobuf@v1.30.0/internal/filedesc/build.go:112 +0x1a8
google.golang.org/protobuf/internal/filetype.Builder.Build({{{0x102872ea6, 0x24}, {0x102bf5aa0, 0x282, 0x282}, 0x0, 0x4, 0x0, 0x0, {0x0, ...}, ...}, ...})
        /Users/pkw/go/pkg/mod/google.golang.org/protobuf@v1.30.0/internal/filetype/build.go:138 +0x17c
github.com/google/gnostic/extensions.file_extensions_extension_proto_init()
        /Users/pkw/go/pkg/mod/github.com/google/gnostic@v0.6.9/extensions/extension.pb.go:456 +0x178
github.com/google/gnostic/extensions.init.0()

Generated code should ideally only live in a single place - is it possible to update gnostic to pull generated code from gnostic-models so that it only exists in one go module?

@pkwarren
Copy link
Contributor Author

pkwarren commented Jun 6, 2023

For example, k8s.io/kube-openapi recently updated to switch to gnostic-models in kubernetes/kube-openapi#402. The latest release of https://github.com/kubernetes/apimachinery/releases/tag/v0.27.2 and https://github.com/kubernetes/apiserver/releases/tag/v0.27.2 are still using gnostic (although unreleased versions look like they're switching to use gnostic-models).

@Jefftree
Copy link
Contributor

Jefftree commented Jun 7, 2023

This is a known issue. The latest releases of apimachinery v0.27.2 uses a corresponding version of kube-openapi that also uses gnostic instead of gnostic-models: https://github.com/kubernetes/apimachinery/blob/v0.27.2/go.mod#L26. Please stick to the pinned dependency versions to prevent drift in the modules.

@pkwarren
Copy link
Contributor Author

pkwarren commented Jun 7, 2023

Ok - we hit this with an upgrade to kube-openapi (which has no tags so it picked up the latest commit). Hopefully over the next few releases of libraries things will stabilize to only depend on gnostic-models instead of a combination of gnostic-models and gnostic.

Kyrremann added a commit to nais/cli that referenced this issue Jul 7, 2023
apimachinery/api/client-go v0.27.3 dro inn gnostic-models, men vi avhenger av
gnostic i client-go, så må vente til de spiller på lag.

```
panic: proto: file "extensions/extension.proto" is already registered
        previously from: "github.com/google/gnostic-models/extensions"
        currently from:  "github.com/google/gnostic/extensions"
See https://protobuf.dev/reference/go/faq#namespace-conflict
```

google/gnostic#397
@pkwarren
Copy link
Contributor Author

Related: kubernetes/kube-openapi#404

@pkwarren
Copy link
Contributor Author

Would you be open to a PR which updates the generated proto code to come from gnostic-models and provides type aliases for compatibility with existing code, i.e. replace discovery/discovery.pb.go with:

package discovery_v1

import (
	models "github.com/google/gnostic-models/discovery"
)

type Annotations = models.Annotations
type Any = models.Any
type Auth = models.Auth
...
type Simple = models.Simple
type StringArray = models.StringArray

var File_discovery_discovery_proto = models.File_discovery_discovery_proto

This should allow for a Go module to depend on code both from gnostic and gnostic-models without panicing.

@Jefftree
Copy link
Contributor

Sounds like a great idea, do you want to go ahead with the PR? cc @timburks

@pkwarren
Copy link
Contributor Author

Sounds like a great idea, do you want to go ahead with the PR? cc @timburks

Opened #400 with a proposed change.

@Jefftree
Copy link
Contributor

Summary of the issue:
gnostic was renamed to gnostic-models in kubernetes which affects mainly importers of client-go and kube-openapi. Some of the workarounds have been described in kubernetes/client-go#1269 (comment) and kubernetes/client-go#1269 (comment).

These issues should be fully resolved once kubernetes 1.28 is released and a workaround is not needed to use an alpha client. In the meantime, #400 is a generalized way of solving this for projects that do not have any kubernetes dependencies.

xrstf added a commit to xrstf/stalk that referenced this issue Aug 10, 2023
reneleonhardt added a commit to reneleonhardt/mesh that referenced this issue Oct 14, 2023
kube-openapi pinned before the migration to google/gnostic
kubernetes/kube-openapi#285
google/gnostic#397
reneleonhardt added a commit to reneleonhardt/mesh that referenced this issue Oct 14, 2023
kube-openapi pinned before the migration to google/gnostic
kubernetes/kube-openapi#285
google/gnostic#397
reneleonhardt added a commit to reneleonhardt/mesh that referenced this issue Oct 14, 2023
kube-openapi pinned before the migration to google/gnostic
kubernetes/kube-openapi#285
google/gnostic#397
enrichman added a commit to enrichman/cli that referenced this issue Apr 10, 2024
import of gnostic-models along with gnostic was causing a panic due to protobuf namespace conflicts.
See google/gnostic#397 for more info
enrichman added a commit to enrichman/cli that referenced this issue Apr 11, 2024
import of gnostic-models along with gnostic was causing a panic due to protobuf namespace conflicts.
See google/gnostic#397 for more info
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 a pull request may close this issue.

2 participants