From a16d5255060680e8f87acb75c88e2f0982dbbba5 Mon Sep 17 00:00:00 2001 From: Sean Sullivan Date: Mon, 24 Apr 2023 17:40:07 -0700 Subject: [PATCH] Refactors discovery content-type and helper functions Kubernetes-commit: 3ce0c108fe9587be2e5195ad872578877970a7a9 --- discovery/discovery_client.go | 64 +++++++++++++++++--------- discovery/discovery_client_test.go | 72 ++++++++++++++++++++++++++---- 2 files changed, 107 insertions(+), 29 deletions(-) diff --git a/discovery/discovery_client.go b/discovery/discovery_client.go index 641568008b..1253fa1f44 100644 --- a/discovery/discovery_client.go +++ b/discovery/discovery_client.go @@ -20,6 +20,7 @@ import ( "context" "encoding/json" "fmt" + "mime" "net/http" "net/url" "sort" @@ -58,8 +59,9 @@ const ( defaultBurst = 300 AcceptV1 = runtime.ContentTypeJSON - // Aggregated discovery content-type (currently v2beta1). NOTE: Currently, we are assuming the order - // for "g", "v", and "as" from the server. We can only compare this string if we can make that assumption. + // Aggregated discovery content-type (v2beta1). NOTE: content-type parameters + // MUST be ordered (g, v, as) for server in "Accept" header (BUT we are resilient + // to ordering when comparing returned values in "Content-Type" header). AcceptV2Beta1 = runtime.ContentTypeJSON + ";" + "g=apidiscovery.k8s.io;v=v2beta1;as=APIGroupDiscoveryList" // Prioritize aggregated discovery by placing first in the order of discovery accept types. acceptDiscoveryFormats = AcceptV2Beta1 + "," + AcceptV1 @@ -259,8 +261,16 @@ func (d *DiscoveryClient) downloadLegacy() ( var resourcesByGV map[schema.GroupVersion]*metav1.APIResourceList // Switch on content-type server responded with: aggregated or unaggregated. - switch responseContentType { - case AcceptV1: + switch { + case isV2Beta1ContentType(responseContentType): + var aggregatedDiscovery apidiscovery.APIGroupDiscoveryList + err = json.Unmarshal(body, &aggregatedDiscovery) + if err != nil { + return nil, nil, nil, err + } + apiGroupList, resourcesByGV, failedGVs = SplitGroupsAndResources(aggregatedDiscovery) + default: + // Default is unaggregated discovery v1. var v metav1.APIVersions err = json.Unmarshal(body, &v) if err != nil { @@ -271,15 +281,6 @@ func (d *DiscoveryClient) downloadLegacy() ( apiGroup = apiVersionsToAPIGroup(&v) } apiGroupList.Groups = []metav1.APIGroup{apiGroup} - case AcceptV2Beta1: - var aggregatedDiscovery apidiscovery.APIGroupDiscoveryList - err = json.Unmarshal(body, &aggregatedDiscovery) - if err != nil { - return nil, nil, nil, err - } - apiGroupList, resourcesByGV, failedGVs = SplitGroupsAndResources(aggregatedDiscovery) - default: - return nil, nil, nil, fmt.Errorf("Unknown discovery response content-type: %s", responseContentType) } return apiGroupList, resourcesByGV, failedGVs, nil @@ -313,13 +314,8 @@ func (d *DiscoveryClient) downloadAPIs() ( failedGVs := map[schema.GroupVersion]error{} var resourcesByGV map[schema.GroupVersion]*metav1.APIResourceList // Switch on content-type server responded with: aggregated or unaggregated. - switch responseContentType { - case AcceptV1: - err = json.Unmarshal(body, apiGroupList) - if err != nil { - return nil, nil, nil, err - } - case AcceptV2Beta1: + switch { + case isV2Beta1ContentType(responseContentType): var aggregatedDiscovery apidiscovery.APIGroupDiscoveryList err = json.Unmarshal(body, &aggregatedDiscovery) if err != nil { @@ -327,12 +323,38 @@ func (d *DiscoveryClient) downloadAPIs() ( } apiGroupList, resourcesByGV, failedGVs = SplitGroupsAndResources(aggregatedDiscovery) default: - return nil, nil, nil, fmt.Errorf("Unknown discovery response content-type: %s", responseContentType) + // Default is unaggregated discovery v1. + err = json.Unmarshal(body, apiGroupList) + if err != nil { + return nil, nil, nil, err + } } return apiGroupList, resourcesByGV, failedGVs, nil } +// isV2Beta1ContentType checks of the content-type string is both +// "application/json" and contains the v2beta1 content-type params. +// NOTE: This function is resilient to the ordering of the +// content-type parameters, as well as parameters added by +// intermediaries such as proxies or gateways. Examples: +// +// "application/json; g=apidiscovery.k8s.io;v=v2beta1;as=APIGroupDiscoveryList" = true +// "application/json; as=APIGroupDiscoveryList;v=v2beta1;g=apidiscovery.k8s.io" = true +// "application/json; as=APIGroupDiscoveryList;v=v2beta1;g=apidiscovery.k8s.io;charset=utf-8" = true +// "application/json" = false +// "application/json; charset=UTF-8" = false +func isV2Beta1ContentType(contentType string) bool { + base, params, err := mime.ParseMediaType(contentType) + if err != nil { + return false + } + return runtime.ContentTypeJSON == base && + params["g"] == "apidiscovery.k8s.io" && + params["v"] == "v2beta1" && + params["as"] == "APIGroupDiscoveryList" +} + // ServerGroups returns the supported groups, with information like supported versions and the // preferred version. func (d *DiscoveryClient) ServerGroups() (*metav1.APIGroupList, error) { diff --git a/discovery/discovery_client_test.go b/discovery/discovery_client_test.go index dc9a4ffe75..f7a8b09856 100644 --- a/discovery/discovery_client_test.go +++ b/discovery/discovery_client_test.go @@ -1395,8 +1395,9 @@ func TestAggregatedServerGroups(t *testing.T) { } output, err := json.Marshal(agg) require.NoError(t, err) - // Content-type is "aggregated" discovery format. - w.Header().Set("Content-Type", AcceptV2Beta1) + // Content-Type is "aggregated" discovery format. Add extra parameter + // to ensure we are resilient to these extra parameters. + w.Header().Set("Content-Type", AcceptV2Beta1+"; charset=utf-8") w.WriteHeader(http.StatusOK) w.Write(output) })) @@ -1985,8 +1986,9 @@ func TestAggregatedServerGroupsAndResources(t *testing.T) { } output, err := json.Marshal(agg) require.NoError(t, err) - // Content-type is "aggregated" discovery format. - w.Header().Set("Content-Type", AcceptV2Beta1) + // Content-type is "aggregated" discovery format. Add extra parameter + // to ensure we are resilient to these extra parameters. + w.Header().Set("Content-Type", AcceptV2Beta1+"; charset=utf-8") w.WriteHeader(http.StatusOK) w.Write(output) })) @@ -2125,8 +2127,9 @@ func TestAggregatedServerGroupsAndResourcesWithErrors(t *testing.T) { } output, err := json.Marshal(agg) require.NoError(t, err) - // Content-type is "aggregated" discovery format. - w.Header().Set("Content-Type", AcceptV2Beta1) + // Content-type is "aggregated" discovery format. Add extra parameter + // to ensure we are resilient to these extra parameters. + w.Header().Set("Content-Type", AcceptV2Beta1+"; charset=utf-8") w.WriteHeader(status) w.Write(output) })) @@ -2733,8 +2736,9 @@ func TestAggregatedServerPreferredResources(t *testing.T) { } output, err := json.Marshal(agg) require.NoError(t, err) - // Content-type is "aggregated" discovery format. - w.Header().Set("Content-Type", AcceptV2Beta1) + // Content-type is "aggregated" discovery format. Add extra parameter + // to ensure we are resilient to these extra parameters. + w.Header().Set("Content-Type", AcceptV2Beta1+"; charset=utf-8") w.WriteHeader(http.StatusOK) w.Write(output) })) @@ -2758,6 +2762,58 @@ func TestAggregatedServerPreferredResources(t *testing.T) { } } +func TestDiscoveryContentTypeVersion(t *testing.T) { + tests := []struct { + contentType string + isV2Beta1 bool + }{ + { + contentType: "application/json; g=apidiscovery.k8s.io;v=v2beta1;as=APIGroupDiscoveryList", + isV2Beta1: true, + }, + { + // content-type parameters are not in correct order, but comparison ignores order. + contentType: "application/json; v=v2beta1;as=APIGroupDiscoveryList;g=apidiscovery.k8s.io", + isV2Beta1: true, + }, + { + // content-type parameters are not in correct order, but comparison ignores order. + contentType: "application/json; as=APIGroupDiscoveryList;g=apidiscovery.k8s.io;v=v2beta1", + isV2Beta1: true, + }, + { + // Ignores extra parameter "charset=utf-8" + contentType: "application/json; g=apidiscovery.k8s.io;v=v2beta1;as=APIGroupDiscoveryList;charset=utf-8", + isV2Beta1: true, + }, + { + contentType: "application/json", + isV2Beta1: false, + }, + { + contentType: "application/json; charset=UTF-8", + isV2Beta1: false, + }, + { + contentType: "text/json", + isV2Beta1: false, + }, + { + contentType: "text/html", + isV2Beta1: false, + }, + { + contentType: "", + isV2Beta1: false, + }, + } + + for _, test := range tests { + isV2Beta1 := isV2Beta1ContentType(test.contentType) + assert.Equal(t, test.isV2Beta1, isV2Beta1) + } +} + func TestUseLegacyDiscovery(t *testing.T) { // Default client sends aggregated discovery accept format (first) as well as legacy format. server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {