From 351ba8bb1c50284672b3cd5d78a7e3433b867f6b Mon Sep 17 00:00:00 2001 From: Jefftree Date: Mon, 30 Oct 2023 15:42:22 -0400 Subject: [PATCH] Lazy load OpenAPIV2 Kubernetes-commit: eb32969ab8f3d1455c33a595bba5b3ce4d8a03b8 --- pkg/cmd/apply/apply.go | 17 +++--- pkg/cmd/apply/apply_test.go | 79 +++++++++++++++++++++------ pkg/cmd/apply/patcher.go | 68 ++++++++++++----------- pkg/cmd/diff/diff.go | 24 ++++---- pkg/cmd/util/factory_client_access.go | 2 +- pkg/cmd/util/helpers.go | 2 +- 6 files changed, 119 insertions(+), 73 deletions(-) diff --git a/pkg/cmd/apply/apply.go b/pkg/cmd/apply/apply.go index b3eeb21a..88fe07c2 100644 --- a/pkg/cmd/apply/apply.go +++ b/pkg/cmd/apply/apply.go @@ -39,7 +39,6 @@ import ( "k8s.io/cli-runtime/pkg/printers" "k8s.io/cli-runtime/pkg/resource" "k8s.io/client-go/dynamic" - cachedopenapi "k8s.io/client-go/openapi/cached" "k8s.io/client-go/openapi3" "k8s.io/client-go/util/csaupgrade" "k8s.io/component-base/version" @@ -107,7 +106,7 @@ type ApplyOptions struct { Builder *resource.Builder Mapper meta.RESTMapper DynamicClient dynamic.Interface - OpenAPISchema openapi.Resources + OpenAPIGetter openapi.OpenAPIResourcesGetter OpenAPIV3Root openapi3.Root Namespace string @@ -285,12 +284,14 @@ func (flags *ApplyFlags) ToOptions(f cmdutil.Factory, cmd *cobra.Command, baseNa return nil, err } - openAPISchema, _ := f.OpenAPISchema() var openAPIV3Root openapi3.Root - openAPIV3Client, err := f.OpenAPIV3Client() - if err == nil && !cmdutil.OpenAPIV3Apply.IsDisabled() { - cachedOpenAPIV3Client := cachedopenapi.NewClient(openAPIV3Client) - openAPIV3Root = openapi3.NewRoot(cachedOpenAPIV3Client) + if !cmdutil.OpenAPIV3Patch.IsDisabled() { + openAPIV3Client, err := f.OpenAPIV3Client() + if err == nil { + openAPIV3Root = openapi3.NewRoot(openAPIV3Client) + } else { + klog.V(4).Infof("warning: OpenAPI V3 Patch is enabled but is unable to be loaded. Will fall back to OpenAPI V2") + } } validationDirective, err := cmdutil.GetValidationDirective(cmd) @@ -369,7 +370,7 @@ func (flags *ApplyFlags) ToOptions(f cmdutil.Factory, cmd *cobra.Command, baseNa Builder: builder, Mapper: mapper, DynamicClient: dynamicClient, - OpenAPISchema: openAPISchema, + OpenAPIGetter: f, OpenAPIV3Root: openAPIV3Root, IOStreams: flags.IOStreams, diff --git a/pkg/cmd/apply/apply_test.go b/pkg/cmd/apply/apply_test.go index 7cd2ad53..d9de4826 100644 --- a/pkg/cmd/apply/apply_test.go +++ b/pkg/cmd/apply/apply_test.go @@ -65,10 +65,9 @@ import ( ) var ( - fakeSchema = sptest.Fake{Path: filepath.Join("..", "..", "..", "testdata", "openapi", "swagger.json")} - fakeOpenAPIV3Legacy = sptest.OpenAPIV3Getter{Path: filepath.Join("..", "..", "..", "testdata", "openapi", "v3", "api", "v1.json")} - fakeOpenAPIV3AppsV1 = sptest.OpenAPIV3Getter{Path: filepath.Join("..", "..", "..", "testdata", "openapi", "v3", "apis", "apps", "v1.json")} - // testingOpenAPISchemas = []testOpenAPISchema{FakeOpenAPISchema} + fakeSchema = sptest.Fake{Path: filepath.Join("..", "..", "..", "testdata", "openapi", "swagger.json")} + fakeOpenAPIV3Legacy = sptest.OpenAPIV3Getter{Path: filepath.Join("..", "..", "..", "testdata", "openapi", "v3", "api", "v1.json")} + fakeOpenAPIV3AppsV1 = sptest.OpenAPIV3Getter{Path: filepath.Join("..", "..", "..", "testdata", "openapi", "v3", "apis", "apps", "v1.json")} testingOpenAPISchemas = []testOpenAPISchema{AlwaysErrorsOpenAPISchema, FakeOpenAPISchema} AlwaysErrorsOpenAPISchema = testOpenAPISchema{ @@ -94,13 +93,9 @@ var ( return c, nil }, } - OpenAPIV3PanicSchema = testOpenAPISchema{ + AlwaysPanicSchema = testOpenAPISchema{ OpenAPISchemaFn: func() (openapi.Resources, error) { - s, err := fakeSchema.OpenAPISchema() - if err != nil { - return nil, err - } - return openapi.NewOpenAPIData(s) + panic("error, openAPIV2 should not be called") }, OpenAPIV3ClientFunc: func() (openapiclient.Client, error) { return &OpenAPIV3ClientAlwaysPanic{}, nil @@ -116,14 +111,14 @@ func (o *OpenAPIV3ClientAlwaysPanic) Paths() (map[string]openapiclient.GroupVers panic("Cannot get paths") } -func noopOpenAPIV3Apply(t *testing.T, f func(t *testing.T)) { +func noopOpenAPIV3Patch(t *testing.T, f func(t *testing.T)) { f(t) } -func disableOpenAPIV3Apply(t *testing.T, f func(t *testing.T)) { - cmdtesting.WithAlphaEnvsDisabled([]cmdutil.FeatureGate{cmdutil.OpenAPIV3Apply}, t, f) +func disableOpenAPIV3Patch(t *testing.T, f func(t *testing.T)) { + cmdtesting.WithAlphaEnvsDisabled([]cmdutil.FeatureGate{cmdutil.OpenAPIV3Patch}, t, f) } -var applyFeatureToggles = []func(*testing.T, func(t *testing.T)){noopOpenAPIV3Apply, disableOpenAPIV3Apply} +var applyFeatureToggles = []func(*testing.T, func(t *testing.T)){noopOpenAPIV3Patch, disableOpenAPIV3Patch} type testOpenAPISchema struct { OpenAPISchemaFn func() (openapi.Resources, error) @@ -747,7 +742,7 @@ func TestApplyObjectWithoutAnnotation(t *testing.T) { } } -func TestOpenAPIV3ApplyFeatureFlag(t *testing.T) { +func TestOpenAPIV3PatchFeatureFlag(t *testing.T) { // OpenAPIV3 smp apply is on by default. // Test that users can disable it to use OpenAPI V2 smp // An OpenAPI V3 root that always panics is used to ensure @@ -757,7 +752,7 @@ func TestOpenAPIV3ApplyFeatureFlag(t *testing.T) { pathRC := "/namespaces/test/replicationcontrollers/" + nameRC t.Run("test apply when a local object is specified - openapi v2 smp", func(t *testing.T) { - disableOpenAPIV3Apply(t, func(t *testing.T) { + disableOpenAPIV3Patch(t, func(t *testing.T) { tf := cmdtesting.NewTestFactory().WithNamespace("test") defer tf.Cleanup() @@ -778,8 +773,8 @@ func TestOpenAPIV3ApplyFeatureFlag(t *testing.T) { } }), } - tf.OpenAPISchemaFunc = OpenAPIV3PanicSchema.OpenAPISchemaFn - tf.OpenAPIV3ClientFunc = OpenAPIV3PanicSchema.OpenAPIV3ClientFunc + tf.OpenAPISchemaFunc = FakeOpenAPISchema.OpenAPISchemaFn + tf.OpenAPIV3ClientFunc = AlwaysPanicSchema.OpenAPIV3ClientFunc tf.ClientConfigVal = cmdtesting.DefaultClientConfig() ioStreams, _, buf, errBuf := genericiooptions.NewTestIOStreams() @@ -801,6 +796,54 @@ func TestOpenAPIV3ApplyFeatureFlag(t *testing.T) { } +func TestOpenAPIV3DoesNotLoadV2(t *testing.T) { + cmdtesting.InitTestErrorHandler(t) + nameRC, currentRC := readAndAnnotateReplicationController(t, filenameRC) + pathRC := "/namespaces/test/replicationcontrollers/" + nameRC + + t.Run("test apply when a local object is specified - openapi v3 smp", func(t *testing.T) { + tf := cmdtesting.NewTestFactory().WithNamespace("test") + defer tf.Cleanup() + + tf.UnstructuredClient = &fake.RESTClient{ + NegotiatedSerializer: resource.UnstructuredPlusDefaultContentConfig().NegotiatedSerializer, + Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) { + switch p, m := req.URL.Path, req.Method; { + case p == pathRC && m == "GET": + bodyRC := io.NopCloser(bytes.NewReader(currentRC)) + return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: bodyRC}, nil + case p == pathRC && m == "PATCH": + validatePatchApplication(t, req, types.StrategicMergePatchType) + bodyRC := io.NopCloser(bytes.NewReader(currentRC)) + return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: bodyRC}, nil + default: + t.Fatalf("unexpected request: %#v\n%#v", req.URL, req) + return nil, nil + } + }), + } + tf.OpenAPISchemaFunc = AlwaysPanicSchema.OpenAPISchemaFn + tf.OpenAPIV3ClientFunc = FakeOpenAPISchema.OpenAPIV3ClientFunc + tf.ClientConfigVal = cmdtesting.DefaultClientConfig() + + ioStreams, _, buf, errBuf := genericiooptions.NewTestIOStreams() + cmd := NewCmdApply("kubectl", tf, ioStreams) + cmd.Flags().Set("filename", filenameRC) + cmd.Flags().Set("output", "name") + cmd.Run(cmd, []string{}) + + // uses the name from the file, not the response + expectRC := "replicationcontroller/" + nameRC + "\n" + if buf.String() != expectRC { + t.Fatalf("unexpected output: %s\nexpected: %s", buf.String(), expectRC) + } + if errBuf.String() != "" { + t.Fatalf("unexpected error output: %s", errBuf.String()) + } + }) + +} + func TestApplyObject(t *testing.T) { cmdtesting.InitTestErrorHandler(t) nameRC, currentRC := readAndAnnotateReplicationController(t, filenameRC) diff --git a/pkg/cmd/apply/patcher.go b/pkg/cmd/apply/patcher.go index 912e95e4..111cf57e 100644 --- a/pkg/cmd/apply/patcher.go +++ b/pkg/cmd/apply/patcher.go @@ -80,14 +80,17 @@ type Patcher struct { // Number of retries to make if the patch fails with conflict Retries int - OpenapiSchema openapi.Resources + OpenAPIGetter openapi.OpenAPIResourcesGetter OpenAPIV3Root openapi3.Root } func newPatcher(o *ApplyOptions, info *resource.Info, helper *resource.Helper) (*Patcher, error) { - var openapiSchema openapi.Resources + var openAPIGetter openapi.OpenAPIResourcesGetter + var openAPIV3Root openapi3.Root + if o.OpenAPIPatch { - openapiSchema = o.OpenAPISchema + openAPIGetter = o.OpenAPIGetter + openAPIV3Root = o.OpenAPIV3Root } return &Patcher{ @@ -99,8 +102,8 @@ func newPatcher(o *ApplyOptions, info *resource.Info, helper *resource.Helper) ( CascadingStrategy: o.DeleteOptions.CascadingStrategy, Timeout: o.DeleteOptions.Timeout, GracePeriod: o.DeleteOptions.GracePeriod, - OpenapiSchema: openapiSchema, - OpenAPIV3Root: o.OpenAPIV3Root, + OpenAPIGetter: openAPIGetter, + OpenAPIV3Root: openAPIV3Root, Retries: maxPatchRetry, }, nil } @@ -135,34 +138,34 @@ func (p *Patcher) patchSimple(obj runtime.Object, modified []byte, namespace, na // with OpenAPI V3 not present in V2. klog.V(5).Infof("warning: OpenAPI V3 path does not exist - group: %s, version %s, kind %s\n", p.Mapping.GroupVersionKind.Group, p.Mapping.GroupVersionKind.Version, p.Mapping.GroupVersionKind.Kind) - } else { - if gvkSupported { - patch, err = p.buildStrategicMergePatchFromOpenAPIV3(original, modified, current) - if err != nil { - // Fall back to OpenAPI V2 if there is a problem - // We should remove the fallback in the future, - // but for the first release it might be beneficial - // to fall back to OpenAPI V2 while logging the error - // and seeing if we get any bug reports. - fmt.Fprintf(errOut, "warning: error calculating patch from openapi v3 spec: %v\n", err) - } else { - patchType = types.StrategicMergePatchType - } + } else if gvkSupported { + patch, err = p.buildStrategicMergePatchFromOpenAPIV3(original, modified, current) + if err != nil { + // Fall back to OpenAPI V2 if there is a problem + // We should remove the fallback in the future, + // but for the first release it might be beneficial + // to fall back to OpenAPI V2 while logging the error + // and seeing if we get any bug reports. + fmt.Fprintf(errOut, "warning: error calculating patch from openapi v3 spec: %v\n", err) } else { - klog.V(5).Infof("warning: OpenAPI V3 path does not support strategic merge patch - group: %s, version %s, kind %s\n", - p.Mapping.GroupVersionKind.Group, p.Mapping.GroupVersionKind.Version, p.Mapping.GroupVersionKind.Kind) + patchType = types.StrategicMergePatchType } + } else { + klog.V(5).Infof("warning: OpenAPI V3 path does not support strategic merge patch - group: %s, version %s, kind %s\n", + p.Mapping.GroupVersionKind.Group, p.Mapping.GroupVersionKind.Version, p.Mapping.GroupVersionKind.Kind) } } - if patch == nil && p.OpenapiSchema != nil { - // if openapischema is used, we'll try to get required patch type for this GVK from Open API. - // if it fails or could not find any patch type, fall back to baked-in patch type determination. - if patchType, err = p.getPatchTypeFromOpenAPI(p.Mapping.GroupVersionKind); err == nil && patchType == types.StrategicMergePatchType { - patch, err = p.buildStrategicMergeFromOpenAPI(original, modified, current) - if err != nil { - // Warn user about problem and continue strategic merge patching using builtin types. - fmt.Fprintf(errOut, "warning: error calculating patch from openapi spec: %v\n", err) + if patch == nil { + if openAPISchema, err := p.OpenAPIGetter.OpenAPISchema(); err == nil && openAPISchema != nil { + // if openapischema is used, we'll try to get required patch type for this GVK from Open API. + // if it fails or could not find any patch type, fall back to baked-in patch type determination. + if patchType, err = p.getPatchTypeFromOpenAPI(openAPISchema, p.Mapping.GroupVersionKind); err == nil && patchType == types.StrategicMergePatchType { + patch, err = p.buildStrategicMergeFromOpenAPI(openAPISchema, original, modified, current) + if err != nil { + // Warn user about problem and continue strategic merge patching using builtin types. + fmt.Fprintf(errOut, "warning: error calculating patch from openapi spec: %v\n", err) + } } } } @@ -222,7 +225,6 @@ func (p *Patcher) buildMergePatch(original, modified, current []byte) ([]byte, e // gvkSupportsPatchOpenAPIV3 checks if a particular GVK supports the patch operation. // It returns an error if the OpenAPI V3 could not be downloaded. func (p *Patcher) gvkSupportsPatchOpenAPIV3(gvk schema.GroupVersionKind) (bool, error) { - // Bypassing root to save apiserver memory. gvSpec, err := p.OpenAPIV3Root.GVSpec(schema.GroupVersion{ Group: p.Mapping.GroupVersionKind.Group, Version: p.Mapping.GroupVersionKind.Version, @@ -306,8 +308,8 @@ func (p *Patcher) buildStrategicMergePatchFromOpenAPIV3(original, modified, curr // buildStrategicMergeFromOpenAPI builds patch from OpenAPI if it is enabled. // This is used for core types which is published in openapi. -func (p *Patcher) buildStrategicMergeFromOpenAPI(original, modified, current []byte) ([]byte, error) { - schema := p.OpenapiSchema.LookupResource(p.Mapping.GroupVersionKind) +func (p *Patcher) buildStrategicMergeFromOpenAPI(openAPISchema openapi.Resources, original, modified, current []byte) ([]byte, error) { + schema := openAPISchema.LookupResource(p.Mapping.GroupVersionKind) if schema == nil { // Missing schema returns nil patch; also no error. return nil, nil @@ -321,8 +323,8 @@ func (p *Patcher) buildStrategicMergeFromOpenAPI(original, modified, current []b } // getPatchTypeFromOpenAPI looks up patch types supported by given GroupVersionKind in Open API. -func (p *Patcher) getPatchTypeFromOpenAPI(gvk schema.GroupVersionKind) (types.PatchType, error) { - if pc := p.OpenapiSchema.GetConsumes(p.Mapping.GroupVersionKind, "PATCH"); pc != nil { +func (p *Patcher) getPatchTypeFromOpenAPI(openAPISchema openapi.Resources, gvk schema.GroupVersionKind) (types.PatchType, error) { + if pc := openAPISchema.GetConsumes(p.Mapping.GroupVersionKind, "PATCH"); pc != nil { for _, c := range pc { if c == string(types.StrategicMergePatchType) { return types.StrategicMergePatchType, nil diff --git a/pkg/cmd/diff/diff.go b/pkg/cmd/diff/diff.go index b08da5a4..e22c6fa9 100644 --- a/pkg/cmd/diff/diff.go +++ b/pkg/cmd/diff/diff.go @@ -110,7 +110,7 @@ type DiffOptions struct { Concurrency int Selector string - OpenAPISchema openapi.Resources + OpenAPIGetter openapi.OpenAPIResourcesGetter OpenAPIV3Root openapi3.Root DynamicClient dynamic.Interface CmdNamespace string @@ -325,7 +325,7 @@ type InfoObject struct { LocalObj runtime.Object Info *resource.Info Encoder runtime.Encoder - OpenAPI openapi.Resources + OpenAPIGetter openapi.OpenAPIResourcesGetter OpenAPIV3Root openapi3.Root Force bool ServerSideApply bool @@ -398,7 +398,7 @@ func (obj InfoObject) Merged() (runtime.Object, error) { Helper: helper, Overwrite: true, BackOff: clockwork.NewRealClock(), - OpenapiSchema: obj.OpenAPI, + OpenAPIGetter: obj.OpenAPIGetter, OpenAPIV3Root: obj.OpenAPIV3Root, ResourceVersion: resourceVersion, } @@ -641,15 +641,15 @@ func (o *DiffOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []str } if !o.ServerSideApply { - o.OpenAPISchema, err = f.OpenAPISchema() - if err != nil { - return err + o.OpenAPIGetter = f + if !cmdutil.OpenAPIV3Patch.IsDisabled() { + openAPIV3Client, err := f.OpenAPIV3Client() + if err == nil { + o.OpenAPIV3Root = openapi3.NewRoot(openAPIV3Client) + } else { + klog.V(4).Infof("warning: OpenAPI V3 Patch is enabled but is unable to be loaded. Will fall back to OpenAPI V2") + } } - openAPIV3Client, err := f.OpenAPIV3Client() - if err != nil { - return err - } - o.OpenAPIV3Root = openapi3.NewRoot(openAPIV3Client) } o.DynamicClient, err = f.DynamicClient() @@ -730,7 +730,7 @@ func (o *DiffOptions) Run() error { LocalObj: local, Info: info, Encoder: scheme.DefaultJSONEncoder(), - OpenAPI: o.OpenAPISchema, + OpenAPIGetter: o.OpenAPIGetter, OpenAPIV3Root: o.OpenAPIV3Root, Force: force, ServerSideApply: o.ServerSideApply, diff --git a/pkg/cmd/util/factory_client_access.go b/pkg/cmd/util/factory_client_access.go index 4b47d5a3..6a1646b8 100644 --- a/pkg/cmd/util/factory_client_access.go +++ b/pkg/cmd/util/factory_client_access.go @@ -214,5 +214,5 @@ func (f *factoryImpl) OpenAPIV3Client() (openapiclient.Client, error) { return nil, err } - return discovery.OpenAPIV3(), nil + return cached.NewClient(discovery.OpenAPIV3()), nil } diff --git a/pkg/cmd/util/helpers.go b/pkg/cmd/util/helpers.go index b9e1ac19..6d38fade 100644 --- a/pkg/cmd/util/helpers.go +++ b/pkg/cmd/util/helpers.go @@ -428,7 +428,7 @@ const ( ApplySet FeatureGate = "KUBECTL_APPLYSET" CmdPluginAsSubcommand FeatureGate = "KUBECTL_ENABLE_CMD_SHADOW" InteractiveDelete FeatureGate = "KUBECTL_INTERACTIVE_DELETE" - OpenAPIV3Apply FeatureGate = "KUBECTL_OPENAPIV3_APPLY" + OpenAPIV3Patch FeatureGate = "KUBECTL_OPENAPIV3_PATCH" RemoteCommandWebsockets FeatureGate = "KUBECTL_REMOTE_COMMAND_WEBSOCKETS" )