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.

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

Avoid double decoding all client responses #36001

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 cmd/kubeadm/app/apis/kubeadm/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ func addKnownTypes(scheme *runtime.Scheme) error {
&ClusterInfo{},
&api.ListOptions{},
&api.DeleteOptions{},
&api.ExportOptions{},
)
return nil
}
Expand Down
1 change: 1 addition & 0 deletions cmd/kubeadm/app/apis/kubeadm/v1alpha1/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ func addKnownTypes(scheme *runtime.Scheme) error {
&ClusterInfo{},
&v1.ListOptions{},
&v1.DeleteOptions{},
&v1.ExportOptions{},
)
return nil
}
Expand Down
1 change: 1 addition & 0 deletions federation/apis/federation/v1beta1/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ func addKnownTypes(scheme *runtime.Scheme) error {
&ClusterList{},
&v1.ListOptions{},
&v1.DeleteOptions{},
&v1.ExportOptions{},
)
versionedwatch.AddToGroupVersion(scheme, SchemeGroupVersion)
return nil
Expand Down
28 changes: 25 additions & 3 deletions pkg/api/serialization_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -244,16 +244,38 @@ var nonRoundTrippableTypes = sets.NewString(
"WatchEvent",
)

var commonKinds = []string{"ListOptions", "DeleteOptions"}
var commonKinds = []string{"Status", "ListOptions", "DeleteOptions", "ExportOptions"}

// verify all external group/versions have the common kinds like the ListOptions, DeleteOptions are registered.
// verify all external group/versions have the common kinds registered.
func TestCommonKindsRegistered(t *testing.T) {
for _, kind := range commonKinds {
for _, group := range testapi.Groups {
gv := group.GroupVersion()
if _, err := api.Scheme.New(gv.WithKind(kind)); err != nil {
gvk := gv.WithKind(kind)
obj, err := api.Scheme.New(gvk)
if err != nil {
t.Error(err)
}
defaults := gv.WithKind("")
if _, got, err := api.Codecs.LegacyCodec().Decode([]byte(`{"kind":"`+kind+`"}`), &defaults, nil); err != nil || gvk != *got {
t.Errorf("expected %v: %v %v", gvk, got, err)
}
data, err := runtime.Encode(api.Codecs.LegacyCodec(*gv), obj)
if err != nil {
t.Errorf("expected %v: %v\n%s", gvk, err, string(data))
continue
}
if !bytes.Contains(data, []byte(`"kind":"`+kind+`","apiVersion":"`+gv.String()+`"`)) {
if kind != "Status" {
t.Errorf("expected %v: %v\n%s", gvk, err, string(data))
continue
}
// TODO: this is wrong, but legacy clients expect it
if !bytes.Contains(data, []byte(`"kind":"`+kind+`","apiVersion":"v1"`)) {
t.Errorf("expected %v: %v\n%s", gvk, err, string(data))
continue
}
}
}
}
}
Expand Down
1 change: 1 addition & 0 deletions pkg/api/testapi/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ go_library(
"//pkg/apis/apps:go_default_library",
"//pkg/apis/apps/install:go_default_library",
"//pkg/apis/authentication/install:go_default_library",
"//pkg/apis/authorization:go_default_library",
"//pkg/apis/authorization/install:go_default_library",
"//pkg/apis/autoscaling:go_default_library",
"//pkg/apis/autoscaling/install:go_default_library",
Expand Down
36 changes: 24 additions & 12 deletions pkg/api/testapi/testapi.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import (
"k8s.io/kubernetes/pkg/api/unversioned"
"k8s.io/kubernetes/pkg/apimachinery/registered"
"k8s.io/kubernetes/pkg/apis/apps"
"k8s.io/kubernetes/pkg/apis/authorization"
"k8s.io/kubernetes/pkg/apis/autoscaling"
"k8s.io/kubernetes/pkg/apis/batch"
"k8s.io/kubernetes/pkg/apis/certificates"
Expand Down Expand Up @@ -66,18 +67,19 @@ import (
)

var (
Groups = make(map[string]TestGroup)
Default TestGroup
Autoscaling TestGroup
Batch TestGroup
Extensions TestGroup
Apps TestGroup
Policy TestGroup
Federation TestGroup
Rbac TestGroup
Certificates TestGroup
Storage TestGroup
ImagePolicy TestGroup
Groups = make(map[string]TestGroup)
Default TestGroup
Authorization TestGroup
Autoscaling TestGroup
Batch TestGroup
Extensions TestGroup
Apps TestGroup
Policy TestGroup
Federation TestGroup
Rbac TestGroup
Certificates TestGroup
Storage TestGroup
ImagePolicy TestGroup

serializer runtime.SerializerInfo
storageSerializer runtime.SerializerInfo
Expand Down Expand Up @@ -258,6 +260,15 @@ func init() {
externalTypes: api.Scheme.KnownTypes(externalGroupVersion),
}
}
if _, ok := Groups[authorization.GroupName]; !ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be nice to have a unit test somewhere that makes sure this parallel registration stays in sync.

externalGroupVersion := unversioned.GroupVersion{Group: authorization.GroupName, Version: registered.GroupOrDie(authorization.GroupName).GroupVersion.Version}
Groups[authorization.GroupName] = TestGroup{
externalGroupVersion: externalGroupVersion,
internalGroupVersion: authorization.SchemeGroupVersion,
internalTypes: api.Scheme.KnownTypes(authorization.SchemeGroupVersion),
externalTypes: api.Scheme.KnownTypes(externalGroupVersion),
}
}
if _, ok := Groups[kubeadm.GroupName]; !ok {
externalGroupVersion := unversioned.GroupVersion{Group: kubeadm.GroupName, Version: registered.GroupOrDie(kubeadm.GroupName).GroupVersion.Version}
Groups[kubeadm.GroupName] = TestGroup{
Expand All @@ -279,6 +290,7 @@ func init() {
Rbac = Groups[rbac.GroupName]
Storage = Groups[storage.GroupName]
ImagePolicy = Groups[imagepolicy.GroupName]
Authorization = Groups[authorization.GroupName]
Copy link
Contributor

Choose a reason for hiding this comment

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

If nobody uses this, don't add it. We want less code to touch for each group you add, not more.

}

func (g TestGroup) ContentConfig() (string, *unversioned.GroupVersion, runtime.Codec) {
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/apps/v1alpha1/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ func addKnownTypes(scheme *runtime.Scheme) error {
&StatefulSetList{},
&v1.ListOptions{},
&v1.DeleteOptions{},
&v1.ExportOptions{},
)
versionedwatch.AddToGroupVersion(scheme, SchemeGroupVersion)
return nil
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/authorization/v1beta1/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ func addKnownTypes(scheme *runtime.Scheme) error {
scheme.AddKnownTypes(SchemeGroupVersion,
&v1.ListOptions{},
&v1.DeleteOptions{},
&v1.ExportOptions{},

&SelfSubjectAccessReview{},
&SubjectAccessReview{},
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/autoscaling/v1/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ func addKnownTypes(scheme *runtime.Scheme) error {
&Scale{},
&v1.ListOptions{},
&v1.DeleteOptions{},
&v1.ExportOptions{},
)
versionedwatch.AddToGroupVersion(scheme, SchemeGroupVersion)
return nil
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/batch/v1/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ func addKnownTypes(scheme *runtime.Scheme) error {
&JobList{},
&v1.ListOptions{},
&v1.DeleteOptions{},
&v1.ExportOptions{},
)
versionedwatch.AddToGroupVersion(scheme, SchemeGroupVersion)
return nil
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/certificates/v1alpha1/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ func addKnownTypes(scheme *runtime.Scheme) error {
&CertificateSigningRequestList{},
&v1.ListOptions{},
&v1.DeleteOptions{},
&v1.ExportOptions{},
)

// Add the watch version that applies
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/extensions/v1beta1/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ func addKnownTypes(scheme *runtime.Scheme) error {
&IngressList{},
&v1.ListOptions{},
&v1.DeleteOptions{},
&v1.ExportOptions{},
&ReplicaSet{},
&ReplicaSetList{},
&PodSecurityPolicy{},
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/policy/v1alpha1/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ func addKnownTypes(scheme *runtime.Scheme) error {
&Eviction{},
&v1.ListOptions{},
&v1.DeleteOptions{},
&v1.ExportOptions{},
)
// Add the watch version that applies
versionedwatch.AddToGroupVersion(scheme, SchemeGroupVersion)
Expand Down
49 changes: 42 additions & 7 deletions pkg/client/restclient/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,26 +79,61 @@ func TestDoRequestFailed(t *testing.T) {
testServer := httptest.NewServer(&fakeHandler)
defer testServer.Close()

c, err := restClient(testServer)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
err = c.Get().Do().Error()
if err == nil {
t.Errorf("unexpected non-error")
}
ss, ok := err.(errors.APIStatus)
if !ok {
t.Errorf("unexpected error type %v", err)
}
actual := ss.Status()
if !reflect.DeepEqual(status, &actual) {
t.Errorf("Unexpected mis-match: %s", diff.ObjectReflectDiff(status, &actual))
}
}

func TestDoRawRequestFailed(t *testing.T) {
status := &unversioned.Status{
Code: http.StatusNotFound,
Status: unversioned.StatusFailure,
Reason: unversioned.StatusReasonNotFound,
Message: "the server could not find the requested resource",
Details: &unversioned.StatusDetails{
Causes: []unversioned.StatusCause{
{Type: unversioned.CauseTypeUnexpectedServerResponse, Message: "unknown"},
},
},
}
expectedBody, _ := runtime.Encode(testapi.Default.Codec(), status)
fakeHandler := utiltesting.FakeHandler{
StatusCode: 404,
ResponseBody: string(expectedBody),
T: t,
}
testServer := httptest.NewServer(&fakeHandler)
defer testServer.Close()

c, err := restClient(testServer)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
body, err := c.Get().Do().Raw()

if err == nil || body != nil {
if err == nil || body == nil {
t.Errorf("unexpected non-error: %#v", body)
}
ss, ok := err.(errors.APIStatus)
if !ok {
t.Errorf("unexpected error type %v", err)
}
actual := ss.Status()
expected := *status
// The decoder will apply the default Version and Kind to the Status.
expected.APIVersion = "v1"
expected.Kind = "Status"
if !reflect.DeepEqual(&expected, &actual) {
t.Errorf("Unexpected mis-match: %s", diff.ObjectDiff(status, &actual))
if !reflect.DeepEqual(status, &actual) {
t.Errorf("Unexpected mis-match: %s", diff.ObjectReflectDiff(status, &actual))
}
}

Expand Down