Skip to content

add vendor extensions to marshal openapi v2 spec.Header #282

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

Merged
merged 1 commit into from
Mar 10, 2022
Merged

add vendor extensions to marshal openapi v2 spec.Header #282

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion pkg/validation/spec/header.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,11 @@ func (h Header) MarshalJSON() ([]byte, error) {
if err != nil {
return nil, err
}
return swag.ConcatJSON(b1, b2, b3), nil
b4, err := json.Marshal(h.VendorExtensible)
if err != nil {
return nil, err
}
Comment on lines +56 to +59
Copy link
Member

@liggitt liggitt Mar 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

are we using json marshaling of this type anywhere in kubernetes today (in CRD validation, or openapi v2 generation, etc)? just wanting to make sure this isn't an API-facing change in current use.

(if it is, this may still be ok, I just wanted to understand the implications)

Copy link
Member Author

@alexzielenski alexzielenski Mar 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Looking at the instances of Response through https://raw.githubusercontent.com/kubernetes/kubernetes/master/api/openapi-spec/swagger.json none of the default types define headers for their responses.

In the case of CRDs, it may be possible that CRDs populate response headers for their open api spec. I am wholly unfamiliar with that part of the infrastructure, though. @apelisse do you know whether existing CRDs are able to populate response Headers?

Copy link
Member

Choose a reason for hiding this comment

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

CRDs don't have the ability to do anything specific to their response header, if I understand correctly. I don't think it's being used at all.

Also, I'm not sure what this would break, or how this would be an api-facing change.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, I swept k8s.io/kubernetes and couldn't find any places we construct this

Also, I'm not sure what this would break, or how this would be an api-facing change.

just wanted to know if this was going to start including directives places we hadn't before, and think if any known clients would be unable to handle those (think kubectl's fragile handling of openapi v2)

return swag.ConcatJSON(b1, b2, b3, b4), nil
}

// UnmarshalJSON unmarshals this header from JSON
Expand Down
33 changes: 32 additions & 1 deletion pkg/validation/spec/header_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,12 @@ package spec

import (
"encoding/json"
"reflect"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func float64Ptr(f float64) *float64 {
Expand Down Expand Up @@ -80,11 +83,39 @@ const headerJSON = `{
"default": "8"
}`

// cmp.Diff panics when reflecting unexported fields under jsonreference.Ref
// a custom comparator is required
var swaggerDiffOptions = []cmp.Option{cmp.Comparer(func (a Ref, b Ref) bool {
return a.String() == b.String()
})}

func TestIntegrationHeader(t *testing.T) {
var actual Header
if assert.NoError(t, json.Unmarshal([]byte(headerJSON), &actual)) {
assert.EqualValues(t, actual, header)
if !reflect.DeepEqual(header, actual) {
t.Fatal(cmp.Diff(header, actual, swaggerDiffOptions...))
}
}

assertParsesJSON(t, headerJSON, header)
}

// Makes sure that a Header unmarshaled from known good JSON, and one unmarshaled
// from generated JSON are equivalent.
func TestHeaderSerialization(t *testing.T) {
generatedJSON, err := json.Marshal(header)
require.NoError(t, err)


generatedJSONActual := Header{}
require.NoError(t, json.Unmarshal(generatedJSON, &generatedJSONActual))
if !reflect.DeepEqual(header, generatedJSONActual) {
t.Fatal(cmp.Diff(header, generatedJSONActual, swaggerDiffOptions...))
}

goodJSONActual := Header{}
require.NoError(t, json.Unmarshal([]byte(headerJSON), &goodJSONActual))
if !reflect.DeepEqual(header, goodJSONActual) {
t.Fatal(cmp.Diff(header, goodJSONActual, swaggerDiffOptions...))
}
}