diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/merge.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/merge.go index 36c4118207b..be8b571ee90 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/merge.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/merge.go @@ -17,12 +17,17 @@ limitations under the License. package builder import ( - "k8s.io/klog/v2" + "fmt" + "strings" + "k8s.io/kube-openapi/pkg/aggregator" "k8s.io/kube-openapi/pkg/spec3" "k8s.io/kube-openapi/pkg/validation/spec" ) +const metadataGV = "io.k8s.apimachinery.pkg.apis.meta.v1" +const autoscalingGV = "io.k8s.api.autoscaling.v1" + // MergeSpecs aggregates all OpenAPI specs, reusing the metadata of the first, static spec as the basis. // The static spec has the highest priority, and its paths and definitions won't get overlapped by // user-defined CRDs. None of the input is mutated, but input and output share data structures. @@ -83,28 +88,29 @@ func mergeSpec(dest, source *spec.Swagger) { } // MergeSpecsV3 merges OpenAPI v3 specs for CRDs -// For V3, the static spec is never merged with the individual CRD specs so no conflict resolution is necessary +// Conflicts belonging to the meta.v1 or autoscaling.v1 group versions are skipped as all CRDs reference those types +// Other conflicts will result in an error func MergeSpecsV3(crdSpecs ...*spec3.OpenAPI) (*spec3.OpenAPI, error) { - // create shallow copy of staticSpec, but replace paths and definitions because we modify them. crdSpec := &spec3.OpenAPI{} if len(crdSpecs) > 0 { crdSpec.Version = crdSpecs[0].Version crdSpec.Info = crdSpecs[0].Info } for _, s := range crdSpecs { - // merge specs without checking conflicts, since the naming controller prevents - // conflicts between user-defined CRDs - mergeSpecV3(crdSpec, s) + err := mergeSpecV3(crdSpec, s) + if err != nil { + return nil, err + } } - return crdSpec, nil } // mergeSpecV3 copies paths and definitions from source to dest, mutating dest, but not source. -// We assume that conflicts do not matter. -func mergeSpecV3(dest, source *spec3.OpenAPI) { +// Conflicts belonging to the meta.v1 or autoscaling.v1 group versions are skipped as all CRDs reference those types +// Other conflicts will result in an error +func mergeSpecV3(dest, source *spec3.OpenAPI) error { if source == nil || source.Paths == nil { - return + return nil } if dest.Paths == nil { dest.Paths = &spec3.Paths{} @@ -118,7 +124,10 @@ func mergeSpecV3(dest, source *spec3.OpenAPI) { dest.Components.Schemas = map[string]*spec.Schema{} } if _, exists := dest.Components.Schemas[k]; exists { - klog.Warningf("Should not happen: OpenAPI V3 merge schema conflict on %s", k) + if strings.HasPrefix(k, metadataGV) || strings.HasPrefix(k, autoscalingGV) { + continue + } + return fmt.Errorf("OpenAPI V3 merge schema conflict on %s", k) } dest.Components.Schemas[k] = v } @@ -128,4 +137,5 @@ func mergeSpecV3(dest, source *spec3.OpenAPI) { } dest.Paths.Paths[k] = v } + return nil } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/merge_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/merge_test.go new file mode 100644 index 00000000000..8364ea87764 --- /dev/null +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/merge_test.go @@ -0,0 +1,170 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package builder + +import ( + "reflect" + "testing" + + "k8s.io/kube-openapi/pkg/spec3" + "k8s.io/kube-openapi/pkg/validation/spec" +) + +func TestMergeSpecV3(t *testing.T) { + tests := []struct { + name string + specs []*spec3.OpenAPI + expected *spec3.OpenAPI + }{ + { + name: "oneCRD", + specs: []*spec3.OpenAPI{{ + Paths: &spec3.Paths{ + Paths: map[string]*spec3.Path{ + "/apis/stable.example.com/v1/crd1": {}, + }, + }, + Components: &spec3.Components{ + Schemas: map[string]*spec.Schema{ + "io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta": {}, + }, + }, + }, + }, + expected: &spec3.OpenAPI{ + Paths: &spec3.Paths{ + Paths: map[string]*spec3.Path{ + "/apis/stable.example.com/v1/crd1": {}, + }, + }, + Components: &spec3.Components{ + Schemas: map[string]*spec.Schema{ + "io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta": {}, + }, + }, + }, + }, + { + name: "two CRDs same gv", + specs: []*spec3.OpenAPI{{ + Paths: &spec3.Paths{ + Paths: map[string]*spec3.Path{ + "/apis/stable.example.com/v1/crd1": {}, + }, + }, + Components: &spec3.Components{ + Schemas: map[string]*spec.Schema{ + "com.example.stable.v1.CRD1": {}, + + "io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta": {}, + }, + }, + }, + { + Paths: &spec3.Paths{ + Paths: map[string]*spec3.Path{ + "/apis/stable.example.com/v1/crd2": {}, + }, + }, + Components: &spec3.Components{ + Schemas: map[string]*spec.Schema{ + "com.example.stable.v1.CRD2": {}, + "io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta": {}, + }, + }, + }, + }, + expected: &spec3.OpenAPI{ + Paths: &spec3.Paths{ + Paths: map[string]*spec3.Path{ + "/apis/stable.example.com/v1/crd1": {}, + "/apis/stable.example.com/v1/crd2": {}, + }, + }, + Components: &spec3.Components{ + Schemas: map[string]*spec.Schema{ + "io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta": {}, + "com.example.stable.v1.CRD1": {}, + "com.example.stable.v1.CRD2": {}, + }, + }, + }, + }, + { + name: "two CRDs with scale", + specs: []*spec3.OpenAPI{{ + Paths: &spec3.Paths{ + Paths: map[string]*spec3.Path{ + "/apis/stable.example.com/v1/crd1": {}, + }, + }, + Components: &spec3.Components{ + Schemas: map[string]*spec.Schema{ + "com.example.stable.v1.CRD1": {}, + + "io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta": {}, + "io.k8s.api.autoscaling.v1.Scale": {}, + }, + }, + }, + { + Paths: &spec3.Paths{ + Paths: map[string]*spec3.Path{ + "/apis/stable.example.com/v1/crd2": {}, + }, + }, + Components: &spec3.Components{ + Schemas: map[string]*spec.Schema{ + "com.example.stable.v1.CRD2": {}, + "io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta": {}, + "io.k8s.api.autoscaling.v1.Scale": {}, + }, + }, + }, + }, + expected: &spec3.OpenAPI{ + Paths: &spec3.Paths{ + Paths: map[string]*spec3.Path{ + "/apis/stable.example.com/v1/crd1": {}, + "/apis/stable.example.com/v1/crd2": {}, + }, + }, + Components: &spec3.Components{ + Schemas: map[string]*spec.Schema{ + "io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta": {}, + "com.example.stable.v1.CRD1": {}, + "com.example.stable.v1.CRD2": {}, + "io.k8s.api.autoscaling.v1.Scale": {}, + }, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + merged, err := MergeSpecsV3(tt.specs...) + if err != nil { + t.Error(err) + } + if !reflect.DeepEqual(merged, tt.expected) { + t.Error("Merged spec is different from expected spec") + } + + }) + } +} diff --git a/test/integration/apiserver/openapi/openapi_merge_test.go b/test/integration/apiserver/openapi/openapi_merge_test.go new file mode 100644 index 00000000000..2bc43d95072 --- /dev/null +++ b/test/integration/apiserver/openapi/openapi_merge_test.go @@ -0,0 +1,199 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package openapi + +import ( + "context" + "encoding/json" + "testing" + "time" + + "github.com/stretchr/testify/assert" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" + "k8s.io/apiextensions-apiserver/test/integration/fixtures" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + genericfeatures "k8s.io/apiserver/pkg/features" + utilfeature "k8s.io/apiserver/pkg/util/feature" + "k8s.io/client-go/dynamic" + kubernetes "k8s.io/client-go/kubernetes" + featuregatetesting "k8s.io/component-base/featuregate/testing" + "k8s.io/kube-openapi/pkg/spec3" + apiservertesting "k8s.io/kubernetes/cmd/kube-apiserver/app/testing" + "k8s.io/kubernetes/test/integration/framework" +) + +const scaleSchemaName = "io.k8s.api.autoscaling.v1.Scale" +const statusSchemaName = "io.k8s.apimachinery.pkg.apis.meta.v1.Status" +const objectMetaSchemaName = "io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta" + +func TestOpenAPIV3MultipleCRDsSameGV(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, genericfeatures.OpenAPIV3, true)() + server, err := apiservertesting.StartTestServer(t, apiservertesting.NewDefaultTestServerOptions(), nil, framework.SharedEtcd()) + if err != nil { + t.Fatal(err) + } + defer server.TearDownFn() + config := server.ClientConfig + + apiExtensionClient, err := clientset.NewForConfig(config) + if err != nil { + t.Fatal(err) + } + dynamicClient, err := dynamic.NewForConfig(config) + if err != nil { + t.Fatal(err) + } + clientset, err := kubernetes.NewForConfig(config) + if err != nil { + t.Fatal(err) + } + + fooWithSubresourceCRD := &apiextensionsv1.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foosubs.cr.bar.com", + }, + Spec: apiextensionsv1.CustomResourceDefinitionSpec{ + Group: "cr.bar.com", + Scope: apiextensionsv1.NamespaceScoped, + Names: apiextensionsv1.CustomResourceDefinitionNames{ + Plural: "foosubs", + Kind: "FooSub", + }, + Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ + { + Name: "v1", + Served: true, + Storage: true, + Schema: &apiextensionsv1.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "spec": { + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "replicas": { + Type: "integer", + }, + }, + }, + "status": { + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "replicas": { + Type: "integer", + }, + }, + }, + }, + }, + }, + Subresources: &apiextensionsv1.CustomResourceSubresources{ + Status: &apiextensionsv1.CustomResourceSubresourceStatus{}, + Scale: &apiextensionsv1.CustomResourceSubresourceScale{ + SpecReplicasPath: ".spec.replicas", + StatusReplicasPath: ".status.replicas", + }, + }, + }, + }, + }, + } + + bazWithSubresourceCRD := &apiextensionsv1.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bazsubs.cr.bar.com", + }, + Spec: apiextensionsv1.CustomResourceDefinitionSpec{ + Group: "cr.bar.com", + Scope: apiextensionsv1.NamespaceScoped, + Names: apiextensionsv1.CustomResourceDefinitionNames{ + Plural: "bazsubs", + Kind: "BazSub", + }, + Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ + { + Name: "v1", + Served: true, + Storage: true, + Schema: &apiextensionsv1.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "spec": { + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "replicas": { + Type: "integer", + }, + }, + }, + "status": { + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "replicas": { + Type: "integer", + }, + }, + }, + }, + }, + }, + Subresources: &apiextensionsv1.CustomResourceSubresources{ + Status: &apiextensionsv1.CustomResourceSubresourceStatus{}, + Scale: &apiextensionsv1.CustomResourceSubresourceScale{ + SpecReplicasPath: ".spec.replicas", + StatusReplicasPath: ".status.replicas", + }, + }, + }, + }, + }, + } + + _, err = fixtures.CreateNewV1CustomResourceDefinition(fooWithSubresourceCRD, apiExtensionClient, dynamicClient) + if err != nil { + t.Fatal(err) + } + + _, err = fixtures.CreateNewV1CustomResourceDefinition(bazWithSubresourceCRD, apiExtensionClient, dynamicClient) + if err != nil { + t.Fatal(err) + } + + // It takes a second for CRD specs to propagate to the aggregator + time.Sleep(4 * time.Second) + + jsonData, err := clientset.RESTClient().Get().AbsPath("/openapi/v3/apis/cr.bar.com/v1").Do(context.TODO()).Raw() + if err != nil { + t.Fatal(err) + } + + openAPISpec := spec3.OpenAPI{} + err = json.Unmarshal(jsonData, &openAPISpec) + if err != nil { + t.Fatal(err) + } + + // Ensure both CRD schemas are present in the OpenAPI + // Scale, status, and metadata dependencies must also be present + assert.Contains(t, openAPISpec.Components.Schemas, scaleSchemaName) + assert.Contains(t, openAPISpec.Components.Schemas, statusSchemaName) + assert.Contains(t, openAPISpec.Components.Schemas, objectMetaSchemaName) + assert.Contains(t, openAPISpec.Components.Schemas, "com.bar.cr.v1.BazSub") + assert.Contains(t, openAPISpec.Components.Schemas, "com.bar.cr.v1.FooSub") +}