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

Use gnostic-models over gnostic #402

Merged
merged 1 commit into from
May 31, 2023

Conversation

Jefftree
Copy link
Member

gnostic is pinned on v0.5.7-v3refs because we did not want to capture the additional dependencies introduced with later versions of gnostic. The OpenAPI component has been moved to gnostic-models so update the library path to reflect that.

/assign @apelisse

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 30, 2023
@k8s-ci-robot k8s-ci-robot requested review from jpbetz and sttts May 30, 2023 18:36
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 30, 2023
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 30, 2023
@apelisse
Copy link
Member

That removes a lot of dependencies, nice.
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2023
@Jefftree
Copy link
Member Author

/assign @sttts

@sttts
Copy link
Contributor

sttts commented May 31, 2023

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, Jefftree, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 31, 2023
@k8s-ci-robot k8s-ci-robot merged commit 9b4dcd3 into kubernetes:master May 31, 2023
@laszlocph
Copy link

laszlocph commented May 31, 2023

Can this be related?

Just updated with go get -u and it is complaining about gnostic structs.

../../go/pkg/mod/sigs.k8s.io/kustomize/kyaml@v0.14.2/openapi/openapi.go:683:33: cannot use doc (variable of type *"github.com/google/gnostic/openapiv2".Document) as *"github.com/google/gnost
ic-models/openapiv2".Document value in argument to swagger.FromGnostic                                                                                                                        
# k8s.io/kubectl/pkg/util/openapi
../../go/pkg/mod/k8s.io/kubectl@v0.27.2/pkg/util/openapi/openapi.go:52:38: cannot use doc (variable of type *"github.com/google/gnostic/openapiv2".Document) as *"github.com/google/gnostic-models/openapiv2".Document value in argument to proto.NewOpenAPIData
# k8s.io/client-go/applyconfigurations/meta/v1
../../go/pkg/mod/k8s.io/client-go@v0.27.2/applyconfigurations/meta/v1/unstructured.go:64:38: cannot use doc (variable of type *"github.com/google/gnostic/openapiv2".Document) as *"github.com/google/gnostic-models/openapiv2".Document value in argument to proto.NewOpenAPIData

Downgrade solved it: go get k8s.io/kube-openapi@2546d827e515dca59571ec245eef2302e11018e1

@apelisse
Copy link
Member

Yeah, this change will break all the things that used to pull the former gnostic model library. That's a fair amount of things.
How do you want to proceed Jeff? We can either give up this change, that's sad, or we can at least update kubectl to use the new models, or maybe we should tag/cut a branch for 1.27?

@Jefftree
Copy link
Member Author

It's unfortunate that we can't do this update atomically. I've opened kubernetes-sigs/kustomize#5186 for kustomize and kubernetes/kubernetes#118340 to track this migration across k/k.

Out of curiosity, what repo is the go get -u done from? We've generally discouraged go get -u in k/k because of issues like these and pin our dependencies appropriately to avoid breaking things.

@stefanprodan
Copy link

stefanprodan commented Jun 20, 2023

The fact that gnostic doesn't import gnostic-models breaks all tools in the Kubernetes ecosystem. For us in Flux means that we need to wait for all our Kubernetes dependencies (apimachinery, apiextensions-apiserver, client-go, cli-utils) and for controller-runtime, Kustomize, Helm, Cosign, etc to switch to gnostic-models, before we can update Flux. We are looking at months of being behind upstream waiting on everyone to make the switch, if any of these tools have a major CVE, this change makes it impossible to patch Flux.

@sttts
Copy link
Contributor

sttts commented Jun 20, 2023

@stefanprodan why is that? Can you elaborate? How is go.mod complaining?

@stefanprodan
Copy link

stefanprodan commented Jun 20, 2023

@sttts any tool that imports kube-openapi and a project using gnostic e.g. controller-runtime will panic with:

panic: proto: file "extensions/extension.proto" is already registered
        previously from: "github.com/google/gnostic-models/extensions"
        currently from:  "github.com/google/gnostic/extensions"

Ref: google/gnostic#397

@sttts
Copy link
Contributor

sttts commented Jun 20, 2023

We have moved the discussion into Slack https://kubernetes.slack.com/archives/C0EG7JC6T/p1687271483899979

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants