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.

Sign up for GitHub

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

Jump to bottom

apiextensions: builder for v3 schemas #81480

Merged
merged 2 commits into from
Aug 22, 2019
Merged

apiextensions: builder for v3 schemas #81480

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e710fb7
apiextensions: move openapi schema builder and merge code into package
sttts Jun 27, 2019
c0594d2
apiextensions: builder for OpenAPI v3
sttts Aug 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ go_library(
"complete.go",
"convert.go",
"goopenapi.go",
"skeleton.go",
"structural.go",
"unfold.go",
"validation.go",
Expand Down
51 changes: 51 additions & 0 deletions staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/skeleton.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
Copyright 2019 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 schema
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move Unflod method (func (s *Structural) Unfold() *Structural {}) to this file and then remove unfold.go file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it's not about removing anything


// StripDefaults returns a copy without defaults.
func (s *Structural) StripDefaults() *Structural {
s = s.DeepCopy()
v := Visitor{
Structural: func(s *Structural) bool {
changed := false
if s.Default.Object != nil {
s.Default.Object = nil
changed = true
}
return changed
},
}
v.Visit(s)
return s
}

// StripValueValidations returns a copy without value validations.
func (s *Structural) StripValueValidations() *Structural {
s = s.DeepCopy()
v := Visitor{
Structural: func(s *Structural) bool {
changed := false
if s.ValueValidation != nil {
s.ValueValidation = nil
changed = true
}
return changed
},
}
v.Visit(s)
return s
}
62 changes: 8 additions & 54 deletions staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,75 +1,25 @@
load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
load("@io_bazel_rules_go//go:def.bzl", "go_library")

go_library(
name = "go_default_library",
srcs = [
"aggregator.go",
"builder.go",
"controller.go",
"conversion.go",
],
srcs = ["controller.go"],
importmap = "k8s.io/kubernetes/vendor/k8s.io/apiextensions-apiserver/pkg/controller/openapi",
importpath = "k8s.io/apiextensions-apiserver/pkg/controller/openapi",
visibility = ["//visibility:public"],
deps = [
"//staging/src/k8s.io/api/autoscaling/v1:go_default_library",
"//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions:go_default_library",
"//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation:go_default_library",
"//staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema:go_default_library",
"//staging/src/k8s.io/apiextensions-apiserver/pkg/client/informers/internalversion/apiextensions/internalversion:go_default_library",
"//staging/src/k8s.io/apiextensions-apiserver/pkg/client/listers/apiextensions/internalversion:go_default_library",
"//staging/src/k8s.io/apiextensions-apiserver/pkg/generated/openapi:go_default_library",
"//staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1beta1:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/types:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/runtime:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/endpoints:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/endpoints/openapi:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/features:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
"//staging/src/k8s.io/client-go/tools/cache:go_default_library",
"//staging/src/k8s.io/client-go/util/workqueue:go_default_library",
"//vendor/github.com/emicklei/go-restful:go_default_library",
"//vendor/github.com/go-openapi/spec:go_default_library",
"//vendor/k8s.io/klog:go_default_library",
"//vendor/k8s.io/kube-openapi/pkg/builder:go_default_library",
"//vendor/k8s.io/kube-openapi/pkg/common:go_default_library",
"//vendor/k8s.io/kube-openapi/pkg/handler:go_default_library",
"//vendor/k8s.io/kube-openapi/pkg/util:go_default_library",
],
)

go_test(
name = "go_default_test",
srcs = [
"builder_test.go",
"conversion_test.go",
],
embed = [":go_default_library"],
deps = [
"//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions:go_default_library",
"//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1:go_default_library",
"//staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/json:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/endpoints:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/features:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
"//vendor/github.com/go-openapi/spec:go_default_library",
"//vendor/github.com/google/go-cmp/cmp:go_default_library",
"//vendor/github.com/google/gofuzz:go_default_library",
"//vendor/github.com/googleapis/gnostic/OpenAPIv2:go_default_library",
"//vendor/github.com/googleapis/gnostic/compiler:go_default_library",
"//vendor/github.com/stretchr/testify/assert:go_default_library",
"//vendor/github.com/stretchr/testify/require:go_default_library",
"//vendor/gopkg.in/yaml.v2:go_default_library",
"//vendor/k8s.io/kube-openapi/pkg/util/proto:go_default_library",
],
)

Expand All @@ -82,7 +32,11 @@ filegroup(

filegroup(
name = "all-srcs",
srcs = [":package-srcs"],
srcs = [
":package-srcs",
"//staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder:all-srcs",
"//staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/v2:all-srcs",
],
tags = ["automanaged"],
visibility = ["//visibility:public"],
)
68 changes: 68 additions & 0 deletions staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")

go_library(
name = "go_default_library",
srcs = [
"builder.go",
"merge.go",
],
importmap = "k8s.io/kubernetes/vendor/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder",
importpath = "k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder",
visibility = ["//visibility:public"],
deps = [
"//staging/src/k8s.io/api/autoscaling/v1:go_default_library",
"//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions:go_default_library",
"//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation:go_default_library",
"//staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema:go_default_library",
"//staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/v2:go_default_library",
"//staging/src/k8s.io/apiextensions-apiserver/pkg/generated/openapi:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1beta1:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/types:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/endpoints:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/endpoints/openapi:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/features:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
"//vendor/github.com/emicklei/go-restful:go_default_library",
"//vendor/github.com/go-openapi/spec:go_default_library",
"//vendor/k8s.io/kube-openapi/pkg/builder:go_default_library",
"//vendor/k8s.io/kube-openapi/pkg/common:go_default_library",
"//vendor/k8s.io/kube-openapi/pkg/util:go_default_library",
],
)

go_test(
name = "go_default_test",
srcs = ["builder_test.go"],
embed = [":go_default_library"],
deps = [
"//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions:go_default_library",
"//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1:go_default_library",
"//staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/json:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/endpoints:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/features:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
"//vendor/github.com/go-openapi/spec:go_default_library",
"//vendor/github.com/stretchr/testify/assert:go_default_library",
"//vendor/github.com/stretchr/testify/require:go_default_library",
],
)

filegroup(
name = "package-srcs",
srcs = glob(["**"]),
tags = ["automanaged"],
visibility = ["//visibility:private"],
)

filegroup(
name = "all-srcs",
srcs = [":package-srcs"],
tags = ["automanaged"],
visibility = ["//visibility:public"],
)
33 changes: 28 additions & 5 deletions ...iserver/pkg/controller/openapi/builder.go → ...pkg/controller/openapi/builder/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package openapi
package builder

import (
"fmt"
Expand All @@ -29,6 +29,7 @@ import (
"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation"
structuralschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema"
openapiv2 "k8s.io/apiextensions-apiserver/pkg/controller/openapi/v2"
generatedopenapi "k8s.io/apiextensions-apiserver/pkg/generated/openapi"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
metav1beta1 "k8s.io/apimachinery/pkg/apis/meta/v1beta1"
Expand Down Expand Up @@ -64,8 +65,20 @@ var definitions map[string]common.OpenAPIDefinition
var buildDefinitions sync.Once
var namer *openapi.DefinitionNamer

// Options contains builder options.
type Options struct {
// Convert to OpenAPI v2.
V2 bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This builder always returns the spec in v2, the new builder will return the spec in v3. I'm not sure if we need to declare this field for this builder unless the Options struct will be shared with the new builder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the spec is already partly v3. This skips the filtering we do for that (and for kubectl).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new builder will be probably independent. But that's nothing we have to decide now, but can do when we add the v3 builder. This is not meant as the final state of v3 publishing, but best effort v3 output to be used for server side apply.


// Strip defaults.
StripDefaults bool

// Strip value validation.
StripValueValidation bool
}

// BuildSwagger builds swagger for the given crd in the given version
func BuildSwagger(crd *apiextensions.CustomResourceDefinition, version string) (*spec.Swagger, error) {
func BuildSwagger(crd *apiextensions.CustomResourceDefinition, version string, opts Options) (*spec.Swagger, error) {
var schema *structuralschema.Structural
s, err := apiextensions.GetSchemaForVersion(crd, version)
if err != nil {
Expand All @@ -75,7 +88,17 @@ func BuildSwagger(crd *apiextensions.CustomResourceDefinition, version string) (
if s != nil && s.OpenAPIV3Schema != nil {
if !validation.SchemaHasInvalidTypes(s.OpenAPIV3Schema) {
if ss, err := structuralschema.NewStructural(s.OpenAPIV3Schema); err == nil {
schema = ss.Unfold()
// skip non-structural schemas
schema = ss

if opts.StripDefaults {
schema = schema.StripDefaults()
}
if opts.StripValueValidation {
schema = schema.StripValueValidations()
}

schema = schema.Unfold()
}
}
}
Expand All @@ -84,7 +107,7 @@ func BuildSwagger(crd *apiextensions.CustomResourceDefinition, version string) (
// comes from function registerResourceHandlers() in k8s.io/apiserver.
// Alternatives are either (ideally) refactoring registerResourceHandlers() to
// reuse the code, or faking an APIInstaller for CR to feed to registerResourceHandlers().
b := newBuilder(crd, version, schema, true)
b := newBuilder(crd, version, schema, opts.V2)

// Sample response types for building web service
sample := &CRDCanonicalTypeNamer{
Expand Down Expand Up @@ -324,7 +347,7 @@ func (b *builder) buildKubeNative(schema *structuralschema.Structural, v2 bool)
// unknown fields for anything else.
} else {
if v2 {
schema = ToStructuralOpenAPIV2(schema)
schema = openapiv2.ToStructuralOpenAPIV2(schema)
}
ret = schema.ToGoOpenAPI()
ret.SetProperty("metadata", *spec.RefSchema(objectMetaSchemaRef).
Expand Down
34 changes: 31 additions & 3 deletions ...er/pkg/controller/openapi/builder_test.go → ...ontroller/openapi/builder/builder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package openapi
package builder
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could it be builder_test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it could. But we never do that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? I think that "black-box" testing is better because the tests have access only to the exported methods/fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's better. Just not common in this repo. Might be a simple rename here though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have whitebox tests in there. So it's more than a rename and out of scope for this PR.


import (
"reflect"
Expand Down Expand Up @@ -485,7 +485,7 @@ func TestCRDRouteParameterBuilder(t *testing.T) {
},
},
}
swagger, err := BuildSwagger(testNamespacedCRD, testCRDVersion)
swagger, err := BuildSwagger(testNamespacedCRD, testCRDVersion, Options{V2: true, StripDefaults: true})
require.NoError(t, err)
require.Equal(t, len(testCase.paths), len(swagger.Paths.Paths), testCase.scope)
for path, expected := range testCase.paths {
Expand Down Expand Up @@ -557,21 +557,49 @@ func TestBuildSwagger(t *testing.T) {
name string
schema string
wantedSchema string
opts Options
}{
{
"nil",
"",
`{"type":"object","x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`,
Options{V2: true, StripDefaults: true},
},
{
"with properties",
`{"type":"object","properties":{"spec":{"type":"object"},"status":{"type":"object"}}}`,
`{"type":"object","properties":{"apiVersion":{"type":"string"},"kind":{"type":"string"},"metadata":{"$ref":"#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta"},"spec":{"type":"object"},"status":{"type":"object"}},"x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`,
Options{V2: true, StripDefaults: true},
},
{
"with invalid-typed properties",
`{"type":"object","properties":{"spec":{"type":"bug"},"status":{"type":"object"}}}`,
`{"type":"object","x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`,
Options{V2: true, StripDefaults: true},
},
{
"with stripped defaults",
`{"type":"object","properties":{"foo":{"type":"string","default":"bar"}}}`,
`{"type":"object","properties":{"apiVersion":{"type":"string"},"kind":{"type":"string"},"metadata":{"$ref":"#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta"},"foo":{"type":"string"}},"x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`,
Options{V2: true, StripDefaults: true},
},
{
"with stripped defaults",
`{"type":"object","properties":{"foo":{"type":"string","default":"bar"}}}`,
`{"type":"object","properties":{"apiVersion":{"type":"string"},"kind":{"type":"string"},"metadata":{"$ref":"#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta"},"foo":{"type":"string"}},"x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`,
Options{V2: true, StripDefaults: true},
},
{
"v2",
`{"type":"object","properties":{"foo":{"type":"string","oneOf":[{"pattern":"a"},{"pattern":"b"}]}}}`,
`{"type":"object","properties":{"apiVersion":{"type":"string"},"kind":{"type":"string"},"metadata":{"$ref":"#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta"},"foo":{"type":"string"}},"x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`,
Options{V2: true, StripDefaults: true},
},
{
"v3",
`{"type":"object","properties":{"foo":{"type":"string","oneOf":[{"pattern":"a"},{"pattern":"b"}]}}}`,
`{"type":"object","properties":{"apiVersion":{"type":"string"},"kind":{"type":"string"},"metadata":{"$ref":"#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta"},"foo":{"type":"string","oneOf":[{"pattern":"a"},{"pattern":"b"}]}},"x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`,
Options{V2: false, StripDefaults: true},
},
}
for _, tt := range tests {
Expand Down Expand Up @@ -603,7 +631,7 @@ func TestBuildSwagger(t *testing.T) {
Scope: apiextensions.NamespaceScoped,
Validation: validation,
},
}, "v1")
}, "v1", tt.opts)
if err != nil {
t.Fatal(err)
}
Expand Down
8 changes: 5 additions & 3 deletions ...rver/pkg/controller/openapi/aggregator.go → ...r/pkg/controller/openapi/builder/merge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,16 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package openapi
package builder

import (
"github.com/go-openapi/spec"
)

// mergeSpecs aggregates all OpenAPI specs, reusing the metadata of the first, static spec as the basis.
func mergeSpecs(staticSpec *spec.Swagger, crdSpecs ...*spec.Swagger) *spec.Swagger {
// MergeSpecs aggregates all OpenAPI specs, reusing the metadata of the first, static spec as the basis.
// Later paths and definitions override earlier ones. None of the input is mutated, but input
// and output share data structures.
func MergeSpecs(staticSpec *spec.Swagger, crdSpecs ...*spec.Swagger) *spec.Swagger {
// create shallow copy of staticSpec, but replace paths and definitions because we modify them.
specToReturn := *staticSpec
if staticSpec.Definitions != nil {
Expand Down