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

OpenAPI downloads protobuf rather than Json #46803

Merged
merged 1 commit into from
Jun 30, 2017
Merged

OpenAPI downloads protobuf rather than Json #46803

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions pkg/kubectl/cmd/util/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ go_library(
"//pkg/version:go_default_library",
"//vendor/github.com/emicklei/go-restful-swagger12:go_default_library",
"//vendor/github.com/evanphx/json-patch:go_default_library",
"//vendor/github.com/go-openapi/spec:go_default_library",
"//vendor/github.com/golang/glog:go_default_library",
"//vendor/github.com/googleapis/gnostic/OpenAPIv2:go_default_library",
"//vendor/github.com/spf13/cobra:go_default_library",
"//vendor/github.com/spf13/pflag:go_default_library",
"//vendor/k8s.io/api/core/v1:go_default_library",
Expand Down Expand Up @@ -108,7 +108,7 @@ go_test(
"//pkg/kubectl/resource:go_default_library",
"//pkg/util/exec:go_default_library",
"//vendor/github.com/emicklei/go-restful-swagger12:go_default_library",
"//vendor/github.com/go-openapi/spec:go_default_library",
"//vendor/github.com/googleapis/gnostic/OpenAPIv2:go_default_library",
"//vendor/github.com/stretchr/testify/assert:go_default_library",
"//vendor/k8s.io/api/core/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/api/equality:go_default_library",
Expand Down
4 changes: 2 additions & 2 deletions pkg/kubectl/cmd/util/cached_discovery.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ import (
"time"

"github.com/emicklei/go-restful-swagger12"
"github.com/go-openapi/spec"
"github.com/golang/glog"
"github.com/googleapis/gnostic/OpenAPIv2"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
Expand Down Expand Up @@ -237,7 +237,7 @@ func (d *CachedDiscoveryClient) SwaggerSchema(version schema.GroupVersion) (*swa
return d.delegate.SwaggerSchema(version)
}

func (d *CachedDiscoveryClient) OpenAPISchema() (*spec.Swagger, error) {
func (d *CachedDiscoveryClient) OpenAPISchema() (*openapi_v2.Document, error) {
return d.delegate.OpenAPISchema()
}

Expand Down
6 changes: 3 additions & 3 deletions pkg/kubectl/cmd/util/cached_discovery_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (
"time"

"github.com/emicklei/go-restful-swagger12"
"github.com/go-openapi/spec"
"github.com/googleapis/gnostic/OpenAPIv2"
"github.com/stretchr/testify/assert"

"k8s.io/apimachinery/pkg/api/errors"
Expand Down Expand Up @@ -171,7 +171,7 @@ func (c *fakeDiscoveryClient) SwaggerSchema(version schema.GroupVersion) (*swagg
return &swagger.ApiDeclaration{}, nil
}

func (c *fakeDiscoveryClient) OpenAPISchema() (*spec.Swagger, error) {
func (c *fakeDiscoveryClient) OpenAPISchema() (*openapi_v2.Document, error) {
c.openAPICalls = c.openAPICalls + 1
return &spec.Swagger{}, nil
return &openapi_v2.Document{}, nil
}
6 changes: 5 additions & 1 deletion pkg/kubectl/cmd/util/openapi/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ go_library(
"//pkg/version:go_default_library",
"//vendor/github.com/go-openapi/spec:go_default_library",
"//vendor/github.com/golang/glog:go_default_library",
"//vendor/github.com/googleapis/gnostic/OpenAPIv2:go_default_library",
"//vendor/gopkg.in/yaml.v2:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library",
"//vendor/k8s.io/client-go/discovery:go_default_library",
Expand All @@ -41,12 +43,14 @@ go_test(
tags = ["automanaged"],
deps = [
"//pkg/kubectl/cmd/util/openapi:go_default_library",
"//vendor/github.com/go-openapi/loads:go_default_library",
"//vendor/github.com/go-openapi/spec:go_default_library",
"//vendor/github.com/googleapis/gnostic/OpenAPIv2:go_default_library",
"//vendor/github.com/googleapis/gnostic/compiler:go_default_library",
"//vendor/github.com/onsi/ginkgo:go_default_library",
"//vendor/github.com/onsi/ginkgo/config:go_default_library",
"//vendor/github.com/onsi/ginkgo/types:go_default_library",
"//vendor/github.com/onsi/gomega:go_default_library",
"//vendor/gopkg.in/yaml.v2:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
],
)
Expand Down
124 changes: 75 additions & 49 deletions pkg/kubectl/cmd/util/openapi/openapi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@ import (
"fmt"
"strings"

"github.com/go-openapi/spec"
"gopkg.in/yaml.v2"

"github.com/golang/glog"
"github.com/googleapis/gnostic/OpenAPIv2"

"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/sets"
Expand Down Expand Up @@ -98,7 +100,7 @@ type Kind struct {
PrimitiveType string

// Extensions are openapi extensions for the object definition.
Copy link
Member

Choose a reason for hiding this comment

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

Update the comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe not. They are the same the type under the cover.

Extensions spec.Extensions
Extensions map[string]interface{}

// Fields are the fields defined for this Kind
Fields map[string]Type
Expand Down Expand Up @@ -130,21 +132,45 @@ type Type struct {
// Extensions are extensions for this field and may contain
// metadata from the types.go struct field tags.
// e.g. contains patchStrategy, patchMergeKey, etc
Extensions spec.Extensions
Extensions map[string]interface{}
}

func vendorExtensionToMap(e []*openapi_v2.NamedAny) map[string]interface{} {
var values map[string]interface{}
Copy link
Member

Choose a reason for hiding this comment

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

You can values := map[string]interface{}{}
and then remove

if values == nil {
  values = make(map[string]interface{})
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's exactly what I had done initially. Problem is that it broke many tests. Many tests build an expected Kind/Type with the default Extension value nil. Problem is, this function will always initialize Extension with an empty Extension, which doesn't compare as equal to nil in reflect.DeepEqual.

IOW, the goal of this if is to return a nil map rather than an empty map, so that the tests don't have to worry about initializing an empty map ...


for _, na := range e {
if na.GetName() == "" || na.GetValue() == nil {
continue
}
if na.GetValue().GetYaml() == "" {
continue
}
var value interface{}
err := yaml.Unmarshal([]byte(na.GetValue().GetYaml()), &value)
if err != nil {
continue
}
if values == nil {
values = make(map[string]interface{})
}
values[na.GetName()] = value
}

return values
}

// NewOpenAPIData parses the resource definitions in openapi data by groupversionkind and name
func NewOpenAPIData(s *spec.Swagger) (*Resources, error) {
func NewOpenAPIData(doc *openapi_v2.Document) (*Resources, error) {
o := &Resources{
GroupVersionKindToName: map[schema.GroupVersionKind]string{},
NameToDefinition: map[string]Kind{},
}
// Parse and index definitions by name
for name, d := range s.Definitions {
definition := o.parseDefinition(name, d)
o.NameToDefinition[name] = definition
for _, ns := range doc.GetDefinitions().GetAdditionalProperties() {
definition := o.parseDefinition(ns.GetName(), ns.GetValue())
o.NameToDefinition[ns.GetName()] = definition
if len(definition.GroupVersionKind.Kind) > 0 {
o.GroupVersionKindToName[definition.GroupVersionKind] = name
o.GroupVersionKindToName[definition.GroupVersionKind] = ns.GetName()
}
}

Expand Down Expand Up @@ -185,12 +211,12 @@ func (o *Resources) getTypeNames(elem Type) []string {
return t
}

func (o *Resources) parseDefinition(name string, s spec.Schema) Kind {
func (o *Resources) parseDefinition(name string, s *openapi_v2.Schema) Kind {
gvk, err := o.getGroupVersionKind(s)
value := Kind{
Name: name,
GroupVersionKind: gvk,
Extensions: s.Extensions,
Extensions: vendorExtensionToMap(s.GetVendorExtension()),
Fields: map[string]Type{},
}
if err != nil {
Expand All @@ -202,13 +228,13 @@ func (o *Resources) parseDefinition(name string, s spec.Schema) Kind {
if o.isPrimitive(s) {
value.PrimitiveType = o.getTypeNameForField(s)
}
for fieldname, property := range s.Properties {
value.Fields[fieldname] = o.parseField(property)
for _, ns := range s.GetProperties().GetAdditionalProperties() {
value.Fields[ns.GetName()] = o.parseField(ns.GetValue())
}
return value
}

func (o *Resources) parseField(s spec.Schema) Type {
func (o *Resources) parseField(s *openapi_v2.Schema) Type {
def := Type{
TypeName: o.getTypeNameForField(s),
IsPrimitive: o.isPrimitive(s),
Expand All @@ -225,14 +251,14 @@ func (o *Resources) parseField(s spec.Schema) Type {
def.ElementType = &d
}

def.Extensions = s.Extensions
def.Extensions = vendorExtensionToMap(s.GetVendorExtension())

return def
}

// isArray returns true if s is an array type.
func (o *Resources) isArray(s spec.Schema) bool {
if len(s.Properties) > 0 {
func (o *Resources) isArray(s *openapi_v2.Schema) bool {
if len(s.GetProperties().GetAdditionalProperties()) > 0 {
// Open API can have embedded type definitions, but Kubernetes doesn't generate these.
// This should just be a sanity check against changing the format.
return false
Expand All @@ -241,8 +267,8 @@ func (o *Resources) isArray(s spec.Schema) bool {
}

// isMap returns true if s is a map type.
func (o *Resources) isMap(s spec.Schema) bool {
if len(s.Properties) > 0 {
func (o *Resources) isMap(s *openapi_v2.Schema) bool {
if len(s.GetProperties().GetAdditionalProperties()) > 0 {
// Open API can have embedded type definitions, but Kubernetes doesn't generate these.
// This should just be a sanity check against changing the format.
return false
Expand All @@ -253,8 +279,8 @@ func (o *Resources) isMap(s spec.Schema) bool {
// isPrimitive returns true if s is a primitive type
// Note: For object references that represent primitive types - e.g. IntOrString - this will
// be false, and the referenced Kind will have a non-empty "PrimitiveType".
func (o *Resources) isPrimitive(s spec.Schema) bool {
if len(s.Properties) > 0 {
func (o *Resources) isPrimitive(s *openapi_v2.Schema) bool {
if len(s.GetProperties().GetAdditionalProperties()) > 0 {
// Open API can have embedded type definitions, but Kubernetes doesn't generate these.
// This should just be a sanity check against changing the format.
return false
Expand All @@ -266,96 +292,96 @@ func (o *Resources) isPrimitive(s spec.Schema) bool {
return false
}

func (*Resources) getType(s spec.Schema) string {
if len(s.Type) != 1 {
func (*Resources) getType(s *openapi_v2.Schema) string {
if len(s.GetType().GetValue()) != 1 {
return ""
}
return strings.ToLower(s.Type[0])
return strings.ToLower(s.GetType().GetValue()[0])
}

func (o *Resources) getTypeNameForField(s spec.Schema) string {
func (o *Resources) getTypeNameForField(s *openapi_v2.Schema) string {
// Get the reference for complex types
if o.isDefinitionReference(s) {
return o.nameForDefinitionField(s)
}
// Recurse if type is array
if o.isArray(s) {
return fmt.Sprintf("%s array", o.getTypeNameForField(*s.Items.Schema))
return fmt.Sprintf("%s array", o.getTypeNameForField(s.GetItems().GetSchema()[0]))
Copy link
Member

Choose a reason for hiding this comment

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

Is it guaranteed to have at least one item?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, but I think it falls under the same logic as above: "we assume we have tested this code against the swagger file before".

}
if o.isMap(s) {
return fmt.Sprintf("%s map", o.getTypeNameForField(*s.AdditionalProperties.Schema))
return fmt.Sprintf("%s map", o.getTypeNameForField(s.GetAdditionalProperties().GetSchema()))
}

// Get the value for primitive types
if o.isPrimitive(s) {
return fmt.Sprintf("%s", s.Type[0])
return fmt.Sprintf("%s", s.GetType().GetValue()[0])
}
return ""
}

// isDefinitionReference returns true s is a complex type that should have a Kind.
func (o *Resources) isDefinitionReference(s spec.Schema) bool {
if len(s.Properties) > 0 {
func (o *Resources) isDefinitionReference(s *openapi_v2.Schema) bool {
if len(s.GetProperties().GetAdditionalProperties()) > 0 {
// Open API can have embedded type definitions, but Kubernetes doesn't generate these.
// This should just be a sanity check against changing the format.
return false
}
if len(s.Type) > 0 {
if len(s.GetType().GetValue()) > 0 {
// Definition references won't have a type
return false
}

p := s.SchemaProps.Ref.GetPointer().String()
return len(p) > 0 && strings.HasPrefix(p, "/definitions/")
p := s.GetXRef()
return len(p) > 0 && strings.HasPrefix(p, "#/definitions/")
}

// getElementType returns the type of an element for arrays
// returns an error if s is not an array.
func (o *Resources) getElementType(s spec.Schema) (spec.Schema, error) {
func (o *Resources) getElementType(s *openapi_v2.Schema) (*openapi_v2.Schema, error) {
if !o.isArray(s) {
return spec.Schema{}, fmt.Errorf("%v is not an array type", s.Type)
return &openapi_v2.Schema{}, fmt.Errorf("%v is not an array type", o.getTypeNameForField(s))
}
return *s.Items.Schema, nil
return s.GetItems().GetSchema()[0], nil
Copy link
Member

Choose a reason for hiding this comment

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

Same question here. Should we check the length of the schema slice?

}

// getElementType returns the type of an element for maps
// getValueType returns the type of an element for maps
// returns an error if s is not a map.
func (o *Resources) getValueType(s spec.Schema) (spec.Schema, error) {
func (o *Resources) getValueType(s *openapi_v2.Schema) (*openapi_v2.Schema, error) {
if !o.isMap(s) {
return spec.Schema{}, fmt.Errorf("%v is not an map type", s.Type)
return &openapi_v2.Schema{}, fmt.Errorf("%v is not an map type", o.getTypeNameForField(s))
}
return *s.AdditionalProperties.Schema, nil
return s.GetAdditionalProperties().GetSchema(), nil
}

// nameForDefinitionField returns the definition name for the schema (field) if it is a complex type
func (o *Resources) nameForDefinitionField(s spec.Schema) string {
p := s.SchemaProps.Ref.GetPointer().String()
func (o *Resources) nameForDefinitionField(s *openapi_v2.Schema) string {
p := s.GetXRef()
if len(p) == 0 {
return ""
}

// Strip the "definitions/" pieces of the reference
return strings.Replace(p, "/definitions/", "", -1)
return strings.Replace(p, "#/definitions/", "", -1)
}

// getGroupVersionKind implements OpenAPIData
// getGVK parses the gropuversionkind for a resource definition from the x-kubernetes
// extensions
// Expected format for s.Extensions: map[string][]map[string]string
// map[x-kubernetes-group-version-kind:[map[Group:authentication.k8s.io Version:v1 Kind:TokenReview]]]
func (o *Resources) getGroupVersionKind(s spec.Schema) (schema.GroupVersionKind, error) {
func (o *Resources) getGroupVersionKind(s *openapi_v2.Schema) (schema.GroupVersionKind, error) {
empty := schema.GroupVersionKind{}

extensionMap := vendorExtensionToMap(s.GetVendorExtension())
// Get the extensions
extList, f := s.Extensions[groupVersionKindExtensionKey]
extList, f := extensionMap[groupVersionKindExtensionKey]
if !f {
return empty, fmt.Errorf("No %s extension present in %v", groupVersionKindExtensionKey, s.Extensions)
return empty, fmt.Errorf("No %s extension present in %v", groupVersionKindExtensionKey, extensionMap)
}

// Expect a empty of a list with 1 element
extListCasted, ok := extList.([]interface{})
if !ok {
return empty, fmt.Errorf("%s extension has unexpected type %T in %s", groupVersionKindExtensionKey, extListCasted, s.Extensions)
return empty, fmt.Errorf("%s extension has unexpected type %T in %s", groupVersionKindExtensionKey, extListCasted, extensionMap)
}
if len(extListCasted) == 0 {
return empty, fmt.Errorf("No Group Version Kind found in %v", extListCasted)
Expand All @@ -366,9 +392,9 @@ func (o *Resources) getGroupVersionKind(s spec.Schema) (schema.GroupVersionKind,
gvk := extListCasted[0]

// Expect a empty of a map with 3 entries
gvkMap, ok := gvk.(map[string]interface{})
gvkMap, ok := gvk.(map[interface{}]interface{})
Copy link
Member

Choose a reason for hiding this comment

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

Interesting. It is not a map[string]interface{} any more?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, seems like the new type uses a different format.

if !ok {
return empty, fmt.Errorf("%s extension has unexpected type %T in %s", groupVersionKindExtensionKey, gvk, s.Extensions)
return empty, fmt.Errorf("%s extension has unexpected type %T in %s", groupVersionKindExtensionKey, gvk, extList)
}
group, ok := gvkMap["group"].(string)
if !ok {
Expand Down
3 changes: 1 addition & 2 deletions pkg/kubectl/cmd/util/openapi/openapi_cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,6 @@ func linkFiles(old, new string) error {

// registerBinaryEncodingTypes registers the types so they can be binary encoded by gob
func registerBinaryEncodingTypes() {
gob.Register(map[string]interface{}{})
gob.Register(map[interface{}]interface{}{})
gob.Register([]interface{}{})
gob.Register(Resources{})
}