google / gnostic

A compiler for APIs described by the OpenAPI Specification with plugins for code generation and other API support tasks.
Apache License 2.0
2.03k stars 241 forks source link

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

Closed pkwarren closed 10 months ago

pkwarren commented 1 year ago

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 commented 1 year ago

For example, k8s.io/kube-openapi recently updated to switch to gnostic-models in https://github.com/kubernetes/kube-openapi/pull/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 commented 1 year ago

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 commented 1 year ago

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.

pkwarren commented 11 months ago

Related: https://github.com/kubernetes/kube-openapi/issues/404

pkwarren commented 11 months ago

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 commented 11 months ago

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

pkwarren commented 11 months ago

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

Opened #400 with a proposed change.

Jefftree commented 11 months ago

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 https://github.com/kubernetes/client-go/issues/1269#issuecomment-1613565035 and https://github.com/kubernetes/client-go/issues/1269#issuecomment-1603511589.

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.