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

Fallback to legacy discovery on a wider range of conditions #119870

Merged
merged 1 commit into from
Sep 1, 2023
Merged

Fallback to legacy discovery on a wider range of conditions #119870

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
46 changes: 25 additions & 21 deletions staging/src/k8s.io/client-go/discovery/discovery_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@ const (
acceptDiscoveryFormats = AcceptV2Beta1 + "," + AcceptV1
)

// Aggregated discovery content-type GVK.
var v2Beta1GVK = schema.GroupVersionKind{Group: "apidiscovery.k8s.io", Version: "v2beta1", Kind: "APIGroupDiscoveryList"}

// DiscoveryInterface holds the methods that discover server-supported API groups,
// versions and resources.
type DiscoveryInterface interface {
Expand Down Expand Up @@ -261,16 +264,15 @@ func (d *DiscoveryClient) downloadLegacy() (
}

var resourcesByGV map[schema.GroupVersion]*metav1.APIResourceList
// Switch on content-type server responded with: aggregated or unaggregated.
switch {
case isV2Beta1ContentType(responseContentType):
// Based on the content-type server responded with: aggregated or unaggregated.
if isGVK, _ := ContentTypeIsGVK(responseContentType, v2Beta1GVK); isGVK {
var aggregatedDiscovery apidiscovery.APIGroupDiscoveryList
err = json.Unmarshal(body, &aggregatedDiscovery)
if err != nil {
return nil, nil, nil, err
}
apiGroupList, resourcesByGV, failedGVs = SplitGroupsAndResources(aggregatedDiscovery)
default:
} else {
// Default is unaggregated discovery v1.
var v metav1.APIVersions
err = json.Unmarshal(body, &v)
Expand Down Expand Up @@ -314,16 +316,15 @@ func (d *DiscoveryClient) downloadAPIs() (
apiGroupList := &metav1.APIGroupList{}
failedGVs := map[schema.GroupVersion]error{}
var resourcesByGV map[schema.GroupVersion]*metav1.APIResourceList
// Switch on content-type server responded with: aggregated or unaggregated.
switch {
case isV2Beta1ContentType(responseContentType):
// Based on the content-type server responded with: aggregated or unaggregated.
if isGVK, _ := ContentTypeIsGVK(responseContentType, v2Beta1GVK); isGVK {
var aggregatedDiscovery apidiscovery.APIGroupDiscoveryList
err = json.Unmarshal(body, &aggregatedDiscovery)
if err != nil {
return nil, nil, nil, err
}
apiGroupList, resourcesByGV, failedGVs = SplitGroupsAndResources(aggregatedDiscovery)
default:
} else {
// Default is unaggregated discovery v1.
err = json.Unmarshal(body, apiGroupList)
if err != nil {
Expand All @@ -334,26 +335,29 @@ func (d *DiscoveryClient) downloadAPIs() (
return apiGroupList, resourcesByGV, failedGVs, nil
}

// isV2Beta1ContentType checks of the content-type string is both
// "application/json" and contains the v2beta1 content-type params.
// ContentTypeIsGVK checks of the content-type string is both
// "application/json" and matches the provided GVK. An error
// is returned if the content type string is malformed.
// 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 {
// ("application/json; g=apidiscovery.k8s.io;v=v2beta1;as=APIGroupDiscoveryList", {apidiscovery.k8s.io, v2beta1, APIGroupDiscoveryList}) = (true, nil)
// ("application/json; as=APIGroupDiscoveryList;v=v2beta1;g=apidiscovery.k8s.io", {apidiscovery.k8s.io, v2beta1, APIGroupDiscoveryList}) = (true, nil)
// ("application/json; as=APIGroupDiscoveryList;v=v2beta1;g=apidiscovery.k8s.io;charset=utf-8", {apidiscovery.k8s.io, v2beta1, APIGroupDiscoveryList}) = (true, nil)
// ("application/json", any GVK) = (false, nil)
// ("application/json; charset=UTF-8", any GVK) = (false, nil)
// ("malformed content type string", any GVK) = (false, error)
func ContentTypeIsGVK(contentType string, gvk schema.GroupVersionKind) (bool, error) {
base, params, err := mime.ParseMediaType(contentType)
if err != nil {
return false
return false, err
}
return runtime.ContentTypeJSON == base &&
params["g"] == "apidiscovery.k8s.io" &&
params["v"] == "v2beta1" &&
params["as"] == "APIGroupDiscoveryList"
gvkMatch := runtime.ContentTypeJSON == base &&
params["g"] == gvk.Group &&
params["v"] == gvk.Version &&
params["as"] == gvk.Kind
return gvkMatch, nil
}

// ServerGroups returns the supported groups, with information like supported versions and the
Expand Down
46 changes: 34 additions & 12 deletions staging/src/k8s.io/client-go/discovery/discovery_client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2762,54 +2762,76 @@ func TestAggregatedServerPreferredResources(t *testing.T) {
}

func TestDiscoveryContentTypeVersion(t *testing.T) {
v2beta1 := schema.GroupVersionKind{Group: "apidiscovery.k8s.io", Version: "v2beta1", Kind: "APIGroupDiscoveryList"}
tests := []struct {
contentType string
isV2Beta1 bool
gvk schema.GroupVersionKind
match bool
expectErr bool
}{
{
contentType: "application/json; g=apidiscovery.k8s.io;v=v2beta1;as=APIGroupDiscoveryList",
isV2Beta1: true,
gvk: v2beta1,
match: true,
expectErr: false,
},
{
// 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,
gvk: v2beta1,
match: true,
expectErr: false,
},
{
// 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,
gvk: v2beta1,
match: true,
expectErr: false,
},
{
// Ignores extra parameter "charset=utf-8"
contentType: "application/json; g=apidiscovery.k8s.io;v=v2beta1;as=APIGroupDiscoveryList;charset=utf-8",
isV2Beta1: true,
gvk: v2beta1,
match: true,
expectErr: false,
},
{
contentType: "application/json",
isV2Beta1: false,
gvk: v2beta1,
match: false,
expectErr: false,
},
{
contentType: "application/json; charset=UTF-8",
isV2Beta1: false,
gvk: v2beta1,
match: false,
expectErr: false,
},
{
contentType: "text/json",
isV2Beta1: false,
gvk: v2beta1,
match: false,
expectErr: false,
},
{
contentType: "text/html",
isV2Beta1: false,
gvk: v2beta1,
match: false,
expectErr: false,
},
{
contentType: "",
isV2Beta1: false,
gvk: v2beta1,
match: false,
expectErr: true,
},
}

for _, test := range tests {
isV2Beta1 := isV2Beta1ContentType(test.contentType)
assert.Equal(t, test.isV2Beta1, isV2Beta1)
match, err := ContentTypeIsGVK(test.contentType, test.gvk)
assert.Equal(t, test.expectErr, err != nil)
assert.Equal(t, test.match, match)
}
}

Expand Down
106 changes: 57 additions & 49 deletions staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_discovery.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,14 @@ import (
apidiscoveryv2beta1 "k8s.io/api/apidiscovery/v2beta1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/apiserver/pkg/authentication/user"
"k8s.io/apiserver/pkg/endpoints"
discoveryendpoint "k8s.io/apiserver/pkg/endpoints/discovery/aggregated"
"k8s.io/apiserver/pkg/endpoints/request"
scheme "k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/discovery"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/util/workqueue"
"k8s.io/klog/v2"
apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1"
Expand All @@ -44,6 +46,13 @@ var APIRegistrationGroupVersion metav1.GroupVersion = metav1.GroupVersion{Group:
// first (mirrors v1 discovery behavior)
var APIRegistrationGroupPriority int = 20001

// Aggregated discovery content-type GVK.
var v2Beta1GVK = schema.GroupVersionKind{
Group: "apidiscovery.k8s.io",
Version: "v2beta1",
Kind: "APIGroupDiscoveryList",
}

// Given a list of APIServices and proxyHandlers for contacting them,
// DiscoveryManager caches a list of discovery documents for each server

Expand Down Expand Up @@ -204,7 +213,7 @@ func (dm *discoveryManager) fetchFreshDiscoveryForService(gv metav1.GroupVersion
Path: req.URL.Path,
IsResourceRequest: false,
}))
req.Header.Add("Accept", runtime.ContentTypeJSON+";g=apidiscovery.k8s.io;v=v2beta1;as=APIGroupDiscoveryList")
req.Header.Add("Accept", discovery.AcceptV2Beta1)

if exists && len(cached.etag) > 0 {
req.Header.Add("If-None-Match", cached.etag)
Expand All @@ -217,8 +226,9 @@ func (dm *discoveryManager) fetchFreshDiscoveryForService(gv metav1.GroupVersion
writer := newInMemoryResponseWriter()
handler.ServeHTTP(writer, req)

switch writer.respCode {
Copy link
Member

Choose a reason for hiding this comment

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

I think a switch might still be more readable here

switch {
case writer.respCode == http.StatusNotModified:
  // ...cache case

case writer.respCode == http.StatusServiceUnavailable:
  // ...server unavailable

case writer.respCode == http.StatusOK && discovery.ContentTypeIsGVK(writer.Header().Get("Content-Type"), ...):
  // ...aggregated

default:
  // ...fallback to legacy
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Since discovery.ContentTypeIsGVK(...) returns two values, the condition would need to be checked outside the case statement. Eg:

isGVK, err := discovery.ContentTypeIsGVK(...)
switch {
case ...
case writer.respCode == http.StatusOK && isGVK && err == nil:
// ...aggregated
...
}

Wouldn't an if statement be slightly cleaner in this regard?

Copy link
Member

Choose a reason for hiding this comment

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

heh, true

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to a case statement with the GVK check outside of it, lmk if this is readable.

case http.StatusNotModified:
isV2Beta1GVK, _ := discovery.ContentTypeIsGVK(writer.Header().Get("Content-Type"), v2Beta1GVK)
switch {
case writer.respCode == http.StatusNotModified:
// Keep old entry, update timestamp
cached = cachedResult{
discovery: cached.discovery,
Expand All @@ -228,8 +238,47 @@ func (dm *discoveryManager) fetchFreshDiscoveryForService(gv metav1.GroupVersion

dm.setCacheEntryForService(info.service, cached)
return &cached, nil
case http.StatusNotAcceptable:
// Discovery Document is not being served at all.
case writer.respCode == http.StatusServiceUnavailable:
return nil, fmt.Errorf("service %s returned non-success response code: %v",
info.service.String(), writer.respCode)
case writer.respCode == http.StatusOK && isV2Beta1GVK:
parsed := &apidiscoveryv2beta1.APIGroupDiscoveryList{}
if err := runtime.DecodeInto(scheme.Codecs.UniversalDecoder(), writer.data, parsed); err != nil {
return nil, err
}

klog.V(3).Infof("DiscoveryManager: Successfully downloaded discovery for %s", info.service.String())

// Convert discovery info into a map for convenient lookup later
discoMap := map[metav1.GroupVersion]apidiscoveryv2beta1.APIVersionDiscovery{}
for _, g := range parsed.Items {
for _, v := range g.Versions {
discoMap[metav1.GroupVersion{Group: g.Name, Version: v.Version}] = v
for i := range v.Resources {
// avoid nil panics in v0.26.0-v0.26.3 client-go clients
// see https://github.com/kubernetes/kubernetes/issues/118361
if v.Resources[i].ResponseKind == nil {
v.Resources[i].ResponseKind = &metav1.GroupVersionKind{}
}
for j := range v.Resources[i].Subresources {
if v.Resources[i].Subresources[j].ResponseKind == nil {
v.Resources[i].Subresources[j].ResponseKind = &metav1.GroupVersionKind{}
}
}
}
}
}

// Save cached result
cached = cachedResult{
discovery: discoMap,
etag: writer.Header().Get("Etag"),
lastUpdated: now,
}
dm.setCacheEntryForService(info.service, cached)
return &cached, nil
default:
// Could not get acceptable response for Aggregated Discovery.
// Fall back to legacy discovery information
if len(gv.Version) == 0 {
return nil, errors.New("not found")
Expand Down Expand Up @@ -265,7 +314,7 @@ func (dm *discoveryManager) fetchFreshDiscoveryForService(gv metav1.GroupVersion
handler.ServeHTTP(writer, req)

if writer.respCode != http.StatusOK {
return nil, fmt.Errorf("failed to download discovery for %s: %v", path, writer.String())
return nil, fmt.Errorf("failed to download legacy discovery for %s: %v", path, writer.String())
}

parsed := &metav1.APIResourceList{}
Expand All @@ -278,6 +327,7 @@ func (dm *discoveryManager) fetchFreshDiscoveryForService(gv metav1.GroupVersion
if err != nil {
return nil, err
}
klog.V(3).Infof("DiscoveryManager: Successfully downloaded legacy discovery for %s", info.service.String())

discoMap := map[metav1.GroupVersion]apidiscoveryv2beta1.APIVersionDiscovery{
// Convert old-style APIGroupList to new information
Expand All @@ -296,48 +346,6 @@ func (dm *discoveryManager) fetchFreshDiscoveryForService(gv metav1.GroupVersion
// one group version and an API Service may serve multiple
// group versions.
return &cached, nil

case http.StatusOK:
parsed := &apidiscoveryv2beta1.APIGroupDiscoveryList{}
if err := runtime.DecodeInto(scheme.Codecs.UniversalDecoder(), writer.data, parsed); err != nil {
return nil, err
}
klog.V(3).Infof("DiscoveryManager: Successfully downloaded discovery for %s", info.service.String())

// Convert discovery info into a map for convenient lookup later
discoMap := map[metav1.GroupVersion]apidiscoveryv2beta1.APIVersionDiscovery{}
for _, g := range parsed.Items {
for _, v := range g.Versions {
discoMap[metav1.GroupVersion{Group: g.Name, Version: v.Version}] = v
for i := range v.Resources {
// avoid nil panics in v0.26.0-v0.26.3 client-go clients
// see https://github.com/kubernetes/kubernetes/issues/118361
if v.Resources[i].ResponseKind == nil {
v.Resources[i].ResponseKind = &metav1.GroupVersionKind{}
}
for j := range v.Resources[i].Subresources {
if v.Resources[i].Subresources[j].ResponseKind == nil {
v.Resources[i].Subresources[j].ResponseKind = &metav1.GroupVersionKind{}
}
}
}
}
}

// Save cached result
cached = cachedResult{
discovery: discoMap,
etag: writer.Header().Get("Etag"),
lastUpdated: now,
}
dm.setCacheEntryForService(info.service, cached)
return &cached, nil

default:
klog.Infof("DiscoveryManager: Failed to download discovery for %v: %v %s",
info.service.String(), writer.respCode, writer.data)
return nil, fmt.Errorf("service %s returned non-success response code: %v",
info.service.String(), writer.respCode)
}
}

Expand Down