Skip to content

Commit f38075d

Browse files
crenshaw-devcrenshaw-dev
authored
fix: deduplicate OpenAPI definitions for GVKParser (argoproj#587)
* fix: deduplicate OpenAPI definitions for GVKParser Signed-off-by: Michael Crenshaw <[email protected]> * do the thing that was the whole point Signed-off-by: Michael Crenshaw <[email protected]> * more logs Signed-off-by: Michael Crenshaw <[email protected]> * don't uniquify models Signed-off-by: Michael Crenshaw <[email protected]> * schema for both Signed-off-by: Michael Crenshaw <[email protected]> * more logs Signed-off-by: Michael Crenshaw <[email protected]> * fix logic Signed-off-by: Michael Crenshaw <[email protected]> * better tainted gvk handling, better docs, update mocks Signed-off-by: Michael Crenshaw <[email protected]> * add a test Signed-off-by: Michael Crenshaw <[email protected]> * improvements from comments Signed-off-by: Michael Crenshaw <[email protected]> --------- Signed-off-by: Michael Crenshaw <[email protected]>
1 parent 4386ff4 commit f38075d

File tree

6 files changed

+832
-33
lines changed

6 files changed

+832
-33
lines changed

‎pkg/cache/cluster.go

+2-10
Original file line numberDiff line numberDiff line change
@@ -707,11 +707,7 @@ func (c *clusterCache) watchEvents(ctx context.Context, api kube.APIResourceInfo
707707
err = runSynced(&c.lock, func() error {
708708
openAPISchema, gvkParser, err := c.kubectl.LoadOpenAPISchema(c.config)
709709
if err != nil {
710-
e, ok := err.(*kube.CreateGVKParserError)
711-
if !ok {
712-
return err
713-
}
714-
c.log.Info("warning: failed to create gvk parser", "error", e.Error())
710+
return fmt.Errorf("failed to load open api schema while handling CRD change: %w", err)
715711
}
716712
if gvkParser != nil {
717713
c.gvkParser = gvkParser
@@ -821,11 +817,7 @@ func (c *clusterCache) sync() error {
821817

822818
openAPISchema, gvkParser, err := c.kubectl.LoadOpenAPISchema(config)
823819
if err != nil {
824-
e, ok := err.(*kube.CreateGVKParserError)
825-
if !ok {
826-
return err
827-
}
828-
c.log.Info("warning: failed to create gvk parser", "error", e.Error())
820+
return fmt.Errorf("failed to load open api schema while syncing cluster cache: %w", err)
829821
}
830822

831823
if gvkParser != nil {

‎pkg/cache/mocks/ClusterCache.go

+67-3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎pkg/utils/kube/ctl.go

+8-20
Original file line numberDiff line numberDiff line change
@@ -118,20 +118,6 @@ func isSupportedVerb(apiResource *metav1.APIResource, verb string) bool {
118118
return false
119119
}
120120

121-
type CreateGVKParserError struct {
122-
err error
123-
}
124-
125-
func NewCreateGVKParserError(err error) *CreateGVKParserError {
126-
return &CreateGVKParserError{
127-
err: err,
128-
}
129-
}
130-
131-
func (e *CreateGVKParserError) Error() string {
132-
return fmt.Sprintf("error creating gvk parser: %s", e.err)
133-
}
134-
135121
// LoadOpenAPISchema will load all existing resource schemas from the cluster
136122
// and return:
137123
// - openapi.Resources: used for getting the proto.Schema from a GVK
@@ -148,17 +134,14 @@ func (k *KubectlCmd) LoadOpenAPISchema(config *rest.Config) (openapi.Resources,
148134
if err != nil {
149135
return nil, nil, fmt.Errorf("error getting openapi resources: %s", err)
150136
}
151-
gvkParser, err := newGVKParser(oapiGetter)
137+
gvkParser, err := k.newGVKParser(oapiGetter)
152138
if err != nil {
153-
// return a specific error type to allow gracefully handle
154-
// creating GVK Parser bug:
155-
// https://github.com/kubernetes/kubernetes/issues/103597
156-
return oapiResources, nil, NewCreateGVKParserError(err)
139+
return oapiResources, nil, fmt.Errorf("error getting gvk parser: %s", err)
157140
}
158141
return oapiResources, gvkParser, nil
159142
}
160143

161-
func newGVKParser(oapiGetter *openapi.CachedOpenAPIGetter) (*managedfields.GvkParser, error) {
144+
func (k *KubectlCmd) newGVKParser(oapiGetter discovery.OpenAPISchemaInterface) (*managedfields.GvkParser, error) {
162145
doc, err := oapiGetter.OpenAPISchema()
163146
if err != nil {
164147
return nil, fmt.Errorf("error getting openapi schema: %s", err)
@@ -167,6 +150,11 @@ func newGVKParser(oapiGetter *openapi.CachedOpenAPIGetter) (*managedfields.GvkPa
167150
if err != nil {
168151
return nil, fmt.Errorf("error getting openapi data: %s", err)
169152
}
153+
var taintedGVKs []schema.GroupVersionKind
154+
models, taintedGVKs = newUniqueModels(models)
155+
if len(taintedGVKs) > 0 {
156+
k.Log.Info("Duplicate GVKs detected in OpenAPI schema. This could cause inaccurate diffs.", "gvks", taintedGVKs)
157+
}
170158
gvkParser, err := managedfields.NewGVKParser(models, false)
171159
if err != nil {
172160
return nil, err

‎pkg/utils/kube/ctl_test.go

+49
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
package kube
22

33
import (
4+
_ "embed"
5+
"encoding/json"
6+
openapi_v2 "github.com/google/gnostic-models/openapiv2"
7+
"github.com/stretchr/testify/require"
48
"testing"
59

610
"github.com/stretchr/testify/assert"
@@ -58,4 +62,49 @@ func TestConvertToVersion(t *testing.T) {
5862
assert.Equal(t, "v1", gvk.Version)
5963
}
6064
})
65+
t.Run("newGVKParser gracefully handles duplicate GVKs", func(t *testing.T) {
66+
client := &fakeOpenAPIClient{}
67+
_, err := kubectl.newGVKParser(client)
68+
require.NoError(t, err)
69+
})
70+
}
71+
72+
/**
73+
Getting the test data here was challenging.
74+
75+
First I needed a Kubernetes cluster with an aggregated API installed which had disagreeing versions of the same
76+
resource. In this case, I used Kubernetes 1.30.1 and metrics-server 0.7.1. They have different versions of
77+
APIResourceList.
78+
79+
So then I got the protobuf representation of the aggregated OpenAPI doc. I used the following command to get it:
80+
curl -k https://localhost:6443/openapi/v2 -H "Authorization: Bearer $token" -H "Accept: application/[email protected]+protobuf" > pkg/utils/kube/testdata/openapi_v2.proto
81+
82+
Then I unmarshaled the protobuf representation into JSON using the following code:
83+
84+
json.Unmarshal(openAPIDoc, document)
85+
jsondata, _ := json.Marshal(document)
86+
ioutil.WriteFile("pkg/utils/kube/testdata/openapi_v2.json", jsondata, 0644)
87+
88+
This step was necessary because it's not possible to unmarshal the json representation of the OpenAPI doc into the
89+
openapi_v2.Document struct. I don't know why they're different.
90+
91+
Then I used this code to post-process the JSON representation of the OpenAPI doc and remove unnecessary information:
92+
jq '.definitions.additional_properties |= map(select(.name | test(".*APIResource.*"))) | {definitions: {additional_properties: .definitions.additional_properties}}' pkg/utils/kube/testdata/openapi_v2.json > pkg/utils/kube/testdata/openapi_v2.json.better
93+
mv pkg/utils/kube/testdata/openapi_v2.json.better pkg/utils/kube/testdata/openapi_v2.json
94+
95+
Hopefully we'll never need to reload the test data, because it was to demonstrate a very specific bug.
96+
*/
97+
98+
//go:embed testdata/openapi_v2.json
99+
var openAPIDoc []byte
100+
101+
type fakeOpenAPIClient struct{}
102+
103+
func (f *fakeOpenAPIClient) OpenAPISchema() (*openapi_v2.Document, error) {
104+
document := &openapi_v2.Document{}
105+
err := json.Unmarshal(openAPIDoc, document)
106+
if err != nil {
107+
return nil, err
108+
}
109+
return document, nil
61110
}

0 commit comments

Comments
 (0)