Skip to content

Commit

Permalink
Avoid returning nil responseKind in v1beta1 aggregated discovery
Browse files Browse the repository at this point in the history
  • Loading branch information
liggitt committed Aug 9, 2023
1 parent 1171161 commit fc529b6
Show file tree
Hide file tree
Showing 12 changed files with 352 additions and 45 deletions.
4 changes: 2 additions & 2 deletions pkg/apis/apidiscovery/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ type APIResourceDiscovery struct {
Resource string
// responseKind describes the group, version, and kind of the serialization schema for the object type this endpoint typically returns.
// APIs may return other objects types at their discretion, such as error conditions, requests for alternate representations, or other operation specific behavior.
// This value will be null if an APIService reports subresources but supports no operations on the parent resource
// This value will be null or empty if an APIService reports subresources but supports no operations on the parent resource
ResponseKind *v1.GroupVersionKind
// scope indicates the scope of a resource, either Cluster or Namespaced
Scope ResourceScope
Expand Down Expand Up @@ -134,7 +134,7 @@ type APISubresourceDiscovery struct {
// for this resource across all versions.
Subresource string
// responseKind describes the group, version, and kind of the serialization schema for the object type this endpoint typically returns.
// Some subresources do not return normal resources, these will have null return types.
// Some subresources do not return normal resources, these will have null or empty return types.
ResponseKind *v1.GroupVersionKind
// acceptedTypes describes the kinds that this endpoint accepts.
// Subresources may accept the standard content types or define
Expand Down
4 changes: 2 additions & 2 deletions pkg/generated/openapi/zz_generated.openapi.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions staging/src/k8s.io/api/apidiscovery/v2beta1/generated.proto

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions staging/src/k8s.io/api/apidiscovery/v2beta1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ type APIResourceDiscovery struct {
Resource string `json:"resource" protobuf:"bytes,1,opt,name=resource"`
// responseKind describes the group, version, and kind of the serialization schema for the object type this endpoint typically returns.
// APIs may return other objects types at their discretion, such as error conditions, requests for alternate representations, or other operation specific behavior.
// This value will be null if an APIService reports subresources but supports no operations on the parent resource
// This value will be null or empty if an APIService reports subresources but supports no operations on the parent resource
ResponseKind *v1.GroupVersionKind `json:"responseKind,omitempty" protobuf:"bytes,2,opt,name=responseKind"`
// scope indicates the scope of a resource, either Cluster or Namespaced
Scope ResourceScope `json:"scope" protobuf:"bytes,3,opt,name=scope"`
Expand Down Expand Up @@ -141,7 +141,7 @@ type APISubresourceDiscovery struct {
// for this resource across all versions.
Subresource string `json:"subresource" protobuf:"bytes,1,opt,name=subresource"`
// responseKind describes the group, version, and kind of the serialization schema for the object type this endpoint typically returns.
// Some subresources do not return normal resources, these will have null return types.
// Some subresources do not return normal resources, these will have null or empty return types.
ResponseKind *v1.GroupVersionKind `json:"responseKind,omitempty" protobuf:"bytes,2,opt,name=responseKind"`
// acceptedTypes describes the kinds that this endpoint accepts.
// Subresources may accept the standard content types or define
Expand Down
6 changes: 6 additions & 0 deletions staging/src/k8s.io/apiserver/pkg/endpoints/installer.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,9 @@ func ConvertGroupVersionIntoToDiscovery(list []metav1.APIResource) ([]apidiscove
apiResourceList = append(apiResourceList, apidiscoveryv2beta1.APIResourceDiscovery{
Resource: split[0],
Scope: scope,
// avoid nil panics in v0.26.0-v0.26.3 client-go clients
// see https://github.com/kubernetes/kubernetes/issues/118361
ResponseKind: &metav1.GroupVersionKind{},
})
parentidx = len(apiResourceList) - 1
parentResources[split[0]] = parentidx
Expand All @@ -140,6 +143,9 @@ func ConvertGroupVersionIntoToDiscovery(list []metav1.APIResource) ([]apidiscove
subresource := apidiscoveryv2beta1.APISubresourceDiscovery{
Subresource: split[1],
Verbs: r.Verbs,
// avoid nil panics in v0.26.0-v0.26.3 client-go clients
// see https://github.com/kubernetes/kubernetes/issues/118361
ResponseKind: &metav1.GroupVersionKind{},
}
if r.Kind != "" {
subresource.ResponseKind = &metav1.GroupVersionKind{
Expand Down
28 changes: 28 additions & 0 deletions staging/src/k8s.io/apiserver/pkg/endpoints/installer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,8 @@ func TestConvertAPIResourceToDiscovery(t *testing.T) {
{
Resource: "cronjobs",
Scope: apidiscoveryv2beta1.ScopeNamespace,
// populated to avoid nil panics
ResponseKind: &metav1.GroupVersionKind{},
Subresources: []apidiscoveryv2beta1.APISubresourceDiscovery{{
Subresource: "status",
ResponseKind: &metav1.GroupVersionKind{
Expand All @@ -314,6 +316,32 @@ func TestConvertAPIResourceToDiscovery(t *testing.T) {
},
},
},
{
name: "Test with subresource with missing kind",
resources: []metav1.APIResource{
{
Name: "cronjobs/status",
Namespaced: true,
Group: "batch",
Version: "v1",
Verbs: []string{"create", "delete", "deletecollection", "get", "list", "patch", "update", "watch"},
},
},
wantAPIResourceDiscovery: []apidiscoveryv2beta1.APIResourceDiscovery{
{
Resource: "cronjobs",
Scope: apidiscoveryv2beta1.ScopeNamespace,
// populated to avoid nil panics
ResponseKind: &metav1.GroupVersionKind{},
Subresources: []apidiscoveryv2beta1.APISubresourceDiscovery{{
Subresource: "status",
// populated to avoid nil panics
ResponseKind: &metav1.GroupVersionKind{},
Verbs: []string{"create", "delete", "deletecollection", "get", "list", "patch", "update", "watch"},
}},
},
},
},
{
name: "Test with mismatch parent and subresource scope",
resources: []metav1.APIResource{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ func convertAPIGroup(g apidiscovery.APIGroupDiscovery) (
return group, gvResources, failedGVs
}

var emptyKind = metav1.GroupVersionKind{}

// convertAPIResource tranforms a APIResourceDiscovery to an APIResource. We are
// resilient to missing GVK, since this resource might be the parent resource
// for a subresource. If the parent is missing a GVK, it is not returned in
Expand All @@ -125,7 +127,7 @@ func convertAPIResource(in apidiscovery.APIResourceDiscovery) (metav1.APIResourc
Categories: in.Categories,
}
var err error
if in.ResponseKind != nil {
if in.ResponseKind != nil && (*in.ResponseKind) != emptyKind {
result.Group = in.ResponseKind.Group
result.Version = in.ResponseKind.Version
result.Kind = in.ResponseKind.Kind
Expand All @@ -140,7 +142,7 @@ func convertAPIResource(in apidiscovery.APIResourceDiscovery) (metav1.APIResourc
// convertAPISubresource tranforms a APISubresourceDiscovery to an APIResource.
func convertAPISubresource(parent metav1.APIResource, in apidiscovery.APISubresourceDiscovery) (metav1.APIResource, error) {
result := metav1.APIResource{}
if in.ResponseKind == nil {
if in.ResponseKind == nil || (*in.ResponseKind) == emptyKind {
return result, fmt.Errorf("subresource %s/%s missing GVK", parent.Name, in.Subresource)
}
result.Name = fmt.Sprintf("%s/%s", parent.Name, in.Subresource)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,76 @@ func TestSplitGroupsAndResources(t *testing.T) {
},
expectedFailedGVs: map[schema.GroupVersion]error{},
},
{
name: "Aggregated discovery with single subresource and parent empty GVK",
agg: apidiscovery.APIGroupDiscoveryList{
Items: []apidiscovery.APIGroupDiscovery{
{
ObjectMeta: metav1.ObjectMeta{
Name: "external.metrics.k8s.io",
},
Versions: []apidiscovery.APIVersionDiscovery{
{
Version: "v1beta1",
Resources: []apidiscovery.APIResourceDiscovery{
{
// resilient to empty GVK for parent
Resource: "*",
Scope: apidiscovery.ScopeNamespace,
SingularResource: "",
ResponseKind: &metav1.GroupVersionKind{},
Subresources: []apidiscovery.APISubresourceDiscovery{
{
Subresource: "other-external-metric",
ResponseKind: &metav1.GroupVersionKind{
Kind: "MetricValueList",
},
Verbs: []string{"get"},
},
},
},
},
},
},
},
},
},
expectedGroups: metav1.APIGroupList{
Groups: []metav1.APIGroup{
{
Name: "external.metrics.k8s.io",
Versions: []metav1.GroupVersionForDiscovery{
{
GroupVersion: "external.metrics.k8s.io/v1beta1",
Version: "v1beta1",
},
},
PreferredVersion: metav1.GroupVersionForDiscovery{
GroupVersion: "external.metrics.k8s.io/v1beta1",
Version: "v1beta1",
},
},
},
},
expectedGVResources: map[schema.GroupVersion]*metav1.APIResourceList{
{Group: "external.metrics.k8s.io", Version: "v1beta1"}: {
GroupVersion: "external.metrics.k8s.io/v1beta1",
APIResources: []metav1.APIResource{
// Since parent GVK was nil, it is NOT returned--only the subresource.
{
Name: "*/other-external-metric",
SingularName: "",
Namespaced: true,
Group: "",
Version: "",
Kind: "MetricValueList",
Verbs: []string{"get"},
},
},
},
},
expectedFailedGVs: map[schema.GroupVersion]error{},
},
{
name: "Aggregated discovery with multiple subresources",
agg: apidiscovery.APIGroupDiscoveryList{
Expand Down
2 changes: 1 addition & 1 deletion staging/src/k8s.io/kube-aggregator/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ require (
github.com/davecgh/go-spew v1.1.1
github.com/emicklei/go-restful/v3 v3.9.0
github.com/gogo/protobuf v1.3.2
github.com/google/go-cmp v0.5.9
github.com/google/gofuzz v1.1.0
github.com/spf13/cobra v1.6.0
github.com/spf13/pflag v1.0.5
Expand Down Expand Up @@ -46,7 +47,6 @@ require (
github.com/golang/protobuf v1.5.2 // indirect
github.com/google/cel-go v0.12.6 // indirect
github.com/google/gnostic v0.5.7-v3refs // indirect
github.com/google/go-cmp v0.5.9 // indirect
github.com/google/uuid v1.3.0 // indirect
github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 // indirect
github.com/grpc-ecosystem/grpc-gateway/v2 v2.7.0 // indirect
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,18 @@ func (dm *discoveryManager) fetchFreshDiscoveryForService(gv metav1.GroupVersion
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{}
}
}
}
}
}

Expand Down

0 comments on commit fc529b6

Please sign in to comment.