diff --git a/pkg/controlplane/BUILD b/pkg/controlplane/BUILD index 1c3a039be34..ff22c01111e 100644 --- a/pkg/controlplane/BUILD +++ b/pkg/controlplane/BUILD @@ -5,6 +5,7 @@ go_library( srcs = [ "client_util.go", "controller.go", + "deleted_kinds.go", "doc.go", "import_known_versions.go", "instance.go", @@ -115,10 +116,13 @@ go_library( "//staging/src/k8s.io/apimachinery/pkg/util/intstr:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/net:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/runtime:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/version:go_default_library", "//staging/src/k8s.io/apiserver/pkg/endpoints/discovery:go_default_library", "//staging/src/k8s.io/apiserver/pkg/features:go_default_library", "//staging/src/k8s.io/apiserver/pkg/registry/generic:go_default_library", + "//staging/src/k8s.io/apiserver/pkg/registry/rest:go_default_library", "//staging/src/k8s.io/apiserver/pkg/server:go_default_library", "//staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates:go_default_library", "//staging/src/k8s.io/apiserver/pkg/server/healthz:go_default_library", @@ -130,6 +134,7 @@ go_library( "//staging/src/k8s.io/client-go/kubernetes/typed/core/v1:go_default_library", "//staging/src/k8s.io/client-go/kubernetes/typed/discovery/v1beta1:go_default_library", "//staging/src/k8s.io/client-go/rest:go_default_library", + "//staging/src/k8s.io/component-base/version:go_default_library", "//staging/src/k8s.io/component-helpers/apimachinery/lease:go_default_library", "//vendor/k8s.io/klog/v2:go_default_library", "//vendor/k8s.io/utils/integer:go_default_library", @@ -143,6 +148,7 @@ go_test( timeout = "long", srcs = [ "controller_test.go", + "deleted_kinds_test.go", "import_known_versions_test.go", "instance_openapi_test.go", "instance_test.go", @@ -166,6 +172,7 @@ go_test( "//staging/src/k8s.io/api/discovery/v1beta1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/apitesting/naming:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/intstr:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/net:go_default_library", @@ -173,6 +180,7 @@ go_test( "//staging/src/k8s.io/apimachinery/pkg/version:go_default_library", "//staging/src/k8s.io/apiserver/pkg/authorization/authorizerfactory:go_default_library", "//staging/src/k8s.io/apiserver/pkg/endpoints/openapi:go_default_library", + "//staging/src/k8s.io/apiserver/pkg/registry/rest:go_default_library", "//staging/src/k8s.io/apiserver/pkg/server:go_default_library", "//staging/src/k8s.io/apiserver/pkg/server/options:go_default_library", "//staging/src/k8s.io/apiserver/pkg/server/resourceconfig:go_default_library", @@ -185,6 +193,7 @@ go_test( "//staging/src/k8s.io/client-go/rest:go_default_library", "//staging/src/k8s.io/client-go/testing:go_default_library", "//staging/src/k8s.io/component-base/version:go_default_library", + "//vendor/github.com/davecgh/go-spew/spew:go_default_library", "//vendor/github.com/go-openapi/loads:go_default_library", "//vendor/github.com/go-openapi/spec:go_default_library", "//vendor/github.com/go-openapi/strfmt:go_default_library", diff --git a/pkg/controlplane/deleted_kinds.go b/pkg/controlplane/deleted_kinds.go new file mode 100644 index 00000000000..f9604c8761f --- /dev/null +++ b/pkg/controlplane/deleted_kinds.go @@ -0,0 +1,167 @@ +/* +Copyright 2020 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controlplane + +import ( + "os" + "strconv" + "strings" + + "k8s.io/apimachinery/pkg/util/sets" + apimachineryversion "k8s.io/apimachinery/pkg/version" + "k8s.io/apiserver/pkg/registry/rest" + "k8s.io/klog/v2" +) + +// resourceExpirationEvaluator holds info for deciding if a particular rest.Storage needs to excluded from the API +type resourceExpirationEvaluator struct { + currentMajor int + currentMinor int + isAlpha bool + // This is usually set for testing for which tests need to be removed. This prevent insta-failing CI. + // Set KUBE_APISERVER_STRICT_REMOVED_API_HANDLING_IN_ALPHA to see what will be removed when we tag beta + strictRemovedHandlingInAlpha bool + // This is usually set by a cluster-admin looking for a short-term escape hatch after something bad happened. + // This should be made a flag before merge + // Set KUBE_APISERVER_SERVE_REMOVED_APIS_FOR_ONE_RELEASE to prevent removing APIs for one more release. + serveRemovedAPIsOneMoreRelease bool +} + +func newResourceExpirationEvaluator(currentVersion apimachineryversion.Info) (*resourceExpirationEvaluator, error) { + ret := &resourceExpirationEvaluator{} + if len(currentVersion.Major) > 0 { + currentMajor64, err := strconv.ParseInt(currentVersion.Major, 10, 32) + if err != nil { + return nil, err + } + ret.currentMajor = int(currentMajor64) + } + if len(currentVersion.Minor) > 0 { + // split the "normal" + and - for semver stuff + minorString := strings.Split(currentVersion.Minor, "+")[0] + minorString = strings.Split(minorString, "-")[0] + minorString = strings.Split(minorString, ".")[0] + currentMinor64, err := strconv.ParseInt(minorString, 10, 32) + if err != nil { + return nil, err + } + ret.currentMinor = int(currentMinor64) + } + + ret.isAlpha = strings.Contains(currentVersion.GitVersion, "alpha") + + if envString, ok := os.LookupEnv("KUBE_APISERVER_STRICT_REMOVED_API_HANDLING_IN_ALPHA"); !ok { + // do nothing + } else if envBool, err := strconv.ParseBool(envString); err != nil { + return nil, err + } else { + ret.strictRemovedHandlingInAlpha = envBool + } + if envString, ok := os.LookupEnv("KUBE_APISERVER_SERVE_REMOVED_APIS_FOR_ONE_RELEASE"); !ok { + // do nothing + } else if envBool, err := strconv.ParseBool(envString); err != nil { + return nil, err + } else { + ret.serveRemovedAPIsOneMoreRelease = envBool + } + + return ret, nil +} + +func (e *resourceExpirationEvaluator) shouldServe(resourceServingInfo rest.Storage) bool { + versionedPtr := resourceServingInfo.New() + removed, ok := versionedPtr.(removedInterface) + if !ok { + return true + } + majorRemoved, minorRemoved := removed.APILifecycleRemoved() + if e.currentMajor < majorRemoved { + return true + } + if e.currentMajor > majorRemoved { + return false + } + if e.currentMinor < minorRemoved { + return true + } + if e.currentMinor > minorRemoved { + return false + } + // at this point major and minor are equal, so this API should be removed when the current release GAs. + // If this is an alpha tag, don't remove by default, but allow the option. + // If the cluster-admin has requested serving one more release, allow it. + if e.isAlpha && e.strictRemovedHandlingInAlpha { // don't serve in alpha if we want strict handling + return false + } + if e.isAlpha { // alphas are allowed to continue serving expired betas while we clean up the test + return true + } + if e.serveRemovedAPIsOneMoreRelease { // cluster-admins are allowed to kick the can one release down the road + return true + } + return false +} + +type removedInterface interface { + APILifecycleRemoved() (major, minor int) +} + +// removeDeletedKinds inspects the storage map and modifies it in place by removing storage for kinds that have been deleted. +// versionedResourcesStorageMap mirrors the field on APIGroupInfo, it's a map from version to resource to the storage. +func (e *resourceExpirationEvaluator) removeDeletedKinds(groupName string, versionedResourcesStorageMap map[string]map[string]rest.Storage) { + versionsToRemove := sets.NewString() + for apiVersion, versionToResource := range versionedResourcesStorageMap { + resourcesToRemove := sets.NewString() + for resourceName, resourceServingInfo := range versionToResource { + if !e.shouldServe(resourceServingInfo) { + resourcesToRemove.Insert(resourceName) + } + } + + for resourceName := range versionedResourcesStorageMap[apiVersion] { + if !shouldRemoveResourceAndSubresources(resourcesToRemove, resourceName) { + continue + } + + klog.V(1).Infof("Removing resource %v.%v.%v because it is time to stop serving it per APILifecycle.", resourceName, apiVersion, groupName) + delete(versionedResourcesStorageMap[apiVersion], resourceName) + } + + if len(versionedResourcesStorageMap[apiVersion]) == 0 { + versionsToRemove.Insert(apiVersion) + } + } + + for _, apiVersion := range versionsToRemove.List() { + klog.V(1).Infof("Removing version %v.%v because it is time to stop serving it because it has no resources per APILifecycle.", apiVersion, groupName) + delete(versionedResourcesStorageMap, apiVersion) + } +} + +func shouldRemoveResourceAndSubresources(resourcesToRemove sets.String, resourceName string) bool { + for _, resourceToRemove := range resourcesToRemove.List() { + if resourceName == resourceToRemove { + return true + } + // our API works on nesting, so you can have deployments, deployments/status, and deployments/scale. Not all subresources + // serve the parent type, but if the parent type (deployments in this case), has been removed, it's subresources should be removed too. + if strings.HasPrefix(resourceName, resourceToRemove+"/") { + return true + } + } + return false +} diff --git a/pkg/controlplane/deleted_kinds_test.go b/pkg/controlplane/deleted_kinds_test.go new file mode 100644 index 00000000000..b920e8533ac --- /dev/null +++ b/pkg/controlplane/deleted_kinds_test.go @@ -0,0 +1,353 @@ +/* +Copyright 2020 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controlplane + +import ( + "reflect" + "strings" + "testing" + + "github.com/davecgh/go-spew/spew" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apimachinery/pkg/version" + "k8s.io/apiserver/pkg/registry/rest" +) + +func Test_newResourceExpirationEvaluator(t *testing.T) { + tests := []struct { + name string + currentVersion version.Info + expected resourceExpirationEvaluator + expectedErr string + }{ + { + name: "beta", + currentVersion: version.Info{ + Major: "1", + Minor: "20+", + GitVersion: "v1.20.0-beta.0.62+a5d22854a2ac21", + }, + expected: resourceExpirationEvaluator{currentMajor: 1, currentMinor: 20}, + }, + { + name: "alpha", + currentVersion: version.Info{ + Major: "1", + Minor: "20+", + GitVersion: "v1.20.0-alpha.0.62+a5d22854a2ac21", + }, + expected: resourceExpirationEvaluator{currentMajor: 1, currentMinor: 20, isAlpha: true}, + }, + { + name: "maintenance", + currentVersion: version.Info{ + Major: "1", + Minor: "20+", + GitVersion: "v1.20.1", + }, + expected: resourceExpirationEvaluator{currentMajor: 1, currentMinor: 20}, + }, + { + name: "bad", + currentVersion: version.Info{ + Major: "1", + Minor: "20something+", + GitVersion: "v1.20.1", + }, + expectedErr: `strconv.ParseInt: parsing "20something": invalid syntax`, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + actual, actualErr := newResourceExpirationEvaluator(tt.currentVersion) + + checkErr(t, actualErr, tt.expectedErr) + if actualErr != nil { + return + } + + if !reflect.DeepEqual(tt.expected, *actual) { + t.Fatal(actual) + } + }) + } +} + +func storageRemovedIn(major, minor int) removedInStorage { + return removedInStorage{major: major, minor: minor} +} + +func storageNeverRemoved() removedInStorage { + return removedInStorage{neverRemoved: true} +} + +type removedInStorage struct { + major, minor int + neverRemoved bool +} + +func (r removedInStorage) New() runtime.Object { + if r.neverRemoved { + return neverRemovedObj{} + } + return removedInObj{major: r.major, minor: r.minor} +} + +type neverRemovedObj struct { +} + +func (r neverRemovedObj) GetObjectKind() schema.ObjectKind { + panic("don't do this") +} +func (r neverRemovedObj) DeepCopyObject() runtime.Object { + panic("don't do this either") +} + +type removedInObj struct { + major, minor int +} + +func (r removedInObj) GetObjectKind() schema.ObjectKind { + panic("don't do this") +} +func (r removedInObj) DeepCopyObject() runtime.Object { + panic("don't do this either") +} +func (r removedInObj) APILifecycleRemoved() (major, minor int) { + return r.major, r.minor +} + +func Test_resourceExpirationEvaluator_shouldServe(t *testing.T) { + tests := []struct { + name string + resourceExpirationEvaluator resourceExpirationEvaluator + restStorage rest.Storage + expected bool + }{ + { + name: "removed-in-curr", + resourceExpirationEvaluator: resourceExpirationEvaluator{ + currentMajor: 1, + currentMinor: 20, + }, + restStorage: storageRemovedIn(1, 20), + expected: false, + }, + { + name: "removed-in-curr-but-deferred", + resourceExpirationEvaluator: resourceExpirationEvaluator{ + currentMajor: 1, + currentMinor: 20, + serveRemovedAPIsOneMoreRelease: true, + }, + restStorage: storageRemovedIn(1, 20), + expected: true, + }, + { + name: "removed-in-curr-but-alpha", + resourceExpirationEvaluator: resourceExpirationEvaluator{ + currentMajor: 1, + currentMinor: 20, + isAlpha: true, + }, + restStorage: storageRemovedIn(1, 20), + expected: true, + }, + { + name: "removed-in-curr-but-alpha-but-strict", + resourceExpirationEvaluator: resourceExpirationEvaluator{ + currentMajor: 1, + currentMinor: 20, + isAlpha: true, + strictRemovedHandlingInAlpha: true, + }, + restStorage: storageRemovedIn(1, 20), + expected: false, + }, + { + name: "removed-in-prev-deferral-does-not-help", + resourceExpirationEvaluator: resourceExpirationEvaluator{ + currentMajor: 1, + currentMinor: 21, + serveRemovedAPIsOneMoreRelease: true, + }, + restStorage: storageRemovedIn(1, 20), + expected: false, + }, + { + name: "removed-in-prev-major", + resourceExpirationEvaluator: resourceExpirationEvaluator{ + currentMajor: 2, + currentMinor: 20, + serveRemovedAPIsOneMoreRelease: true, + }, + restStorage: storageRemovedIn(1, 20), + expected: false, + }, + { + name: "removed-in-future", + resourceExpirationEvaluator: resourceExpirationEvaluator{ + currentMajor: 1, + currentMinor: 20, + }, + restStorage: storageRemovedIn(1, 21), + expected: true, + }, + { + name: "never-removed", + resourceExpirationEvaluator: resourceExpirationEvaluator{ + currentMajor: 1, + currentMinor: 20, + }, + restStorage: storageNeverRemoved(), + expected: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if actual := tt.resourceExpirationEvaluator.shouldServe(tt.restStorage); actual != tt.expected { + t.Errorf("shouldServe() = %v, want %v", actual, tt.expected) + } + }) + } +} + +func checkErr(t *testing.T, actual error, expected string) { + t.Helper() + switch { + case len(expected) == 0 && actual == nil: + case len(expected) == 0 && actual != nil: + t.Fatal(actual) + case len(expected) != 0 && actual == nil: + t.Fatalf("missing %q, ", expected) + case len(expected) != 0 && actual != nil && !strings.Contains(actual.Error(), expected): + t.Fatalf("missing %q, %v", expected, actual) + } +} + +func Test_removeDeletedKinds(t *testing.T) { + tests := []struct { + name string + resourceExpirationEvaluator resourceExpirationEvaluator + versionedResourcesStorageMap map[string]map[string]rest.Storage + expectedStorage map[string]map[string]rest.Storage + }{ + { + name: "remove-one-of-two", + resourceExpirationEvaluator: resourceExpirationEvaluator{ + currentMajor: 1, + currentMinor: 20, + }, + versionedResourcesStorageMap: map[string]map[string]rest.Storage{ + "v1": { + "twenty": storageRemovedIn(1, 20), + "twentyone": storageRemovedIn(1, 21), + }, + }, + expectedStorage: map[string]map[string]rest.Storage{ + "v1": { + "twentyone": storageRemovedIn(1, 21), + }, + }, + }, + { + name: "remove-nested-not-expired", + resourceExpirationEvaluator: resourceExpirationEvaluator{ + currentMajor: 1, + currentMinor: 20, + }, + versionedResourcesStorageMap: map[string]map[string]rest.Storage{ + "v1": { + "twenty": storageRemovedIn(1, 20), + "twenty/scale": storageRemovedIn(1, 21), + "twentyone": storageRemovedIn(1, 21), + }, + }, + expectedStorage: map[string]map[string]rest.Storage{ + "v1": { + "twentyone": storageRemovedIn(1, 21), + }, + }, + }, + { + name: "remove-all-of-version", + resourceExpirationEvaluator: resourceExpirationEvaluator{ + currentMajor: 1, + currentMinor: 20, + }, + versionedResourcesStorageMap: map[string]map[string]rest.Storage{ + "v1": { + "twenty": storageRemovedIn(1, 20), + }, + "v2": { + "twenty": storageRemovedIn(1, 20), + "twentyone": storageRemovedIn(1, 21), + }, + }, + expectedStorage: map[string]map[string]rest.Storage{ + "v2": { + "twentyone": storageRemovedIn(1, 21), + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt.resourceExpirationEvaluator.removeDeletedKinds("group.name", tt.versionedResourcesStorageMap) + if !reflect.DeepEqual(tt.expectedStorage, tt.versionedResourcesStorageMap) { + t.Fatal(spew.Sdump(tt.versionedResourcesStorageMap)) + } + }) + } +} + +func Test_shouldRemoveResource(t *testing.T) { + tests := []struct { + name string + resourcesToRemove sets.String + resourceName string + want bool + }{ + { + name: "prefix-matches", + resourcesToRemove: sets.NewString("foo"), + resourceName: "foo/scale", + want: true, + }, + { + name: "exact-matches", + resourcesToRemove: sets.NewString("foo"), + resourceName: "foo", + want: true, + }, + { + name: "no-match", + resourcesToRemove: sets.NewString("foo"), + resourceName: "bar", + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if actual := shouldRemoveResourceAndSubresources(tt.resourcesToRemove, tt.resourceName); actual != tt.want { + t.Errorf("shouldRemoveResourceAndSubresources() = %v, want %v", actual, tt.want) + } + }) + } +} diff --git a/pkg/controlplane/instance.go b/pkg/controlplane/instance.go index 28810732ba7..1b5e901d1af 100644 --- a/pkg/controlplane/instance.go +++ b/pkg/controlplane/instance.go @@ -82,7 +82,9 @@ import ( "k8s.io/client-go/kubernetes" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" discoveryclient "k8s.io/client-go/kubernetes/typed/discovery/v1beta1" + "k8s.io/component-base/version" "k8s.io/component-helpers/apimachinery/lease" + "k8s.io/klog/v2" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/controlplane/controller/apiserverleasegc" "k8s.io/kubernetes/pkg/controlplane/controller/clusterauthenticationtrust" @@ -95,8 +97,6 @@ import ( "k8s.io/kubernetes/pkg/serviceaccount" nodeutil "k8s.io/kubernetes/pkg/util/node" - "k8s.io/klog/v2" - // RESTStorage installers admissionregistrationrest "k8s.io/kubernetes/pkg/registry/admissionregistration/rest" apiserverinternalrest "k8s.io/kubernetes/pkg/registry/apiserverinternal/rest" @@ -581,6 +581,12 @@ type RESTStorageProvider interface { func (m *Instance) InstallAPIs(apiResourceConfigSource serverstorage.APIResourceConfigSource, restOptionsGetter generic.RESTOptionsGetter, restStorageProviders ...RESTStorageProvider) error { apiGroupsInfo := []*genericapiserver.APIGroupInfo{} + // used later in the loop to filter the served resource by those that have expired. + resourceExpirationEvaluator, err := newResourceExpirationEvaluator(version.Get()) + if err != nil { + return err + } + for _, restStorageBuilder := range restStorageProviders { groupName := restStorageBuilder.GroupName() if !apiResourceConfigSource.AnyVersionForGroupEnabled(groupName) { @@ -595,6 +601,16 @@ func (m *Instance) InstallAPIs(apiResourceConfigSource serverstorage.APIResource klog.Warningf("API group %q is not enabled, skipping.", groupName) continue } + + // Remove resources that serving kinds that are removed. + // We do this here so that we don't accidentally serve versions without resources or openapi information that for kinds we don't serve. + // This is a spot above the construction of individual storage handlers so that no sig accidentally forgets to check. + resourceExpirationEvaluator.removeDeletedKinds(groupName, apiGroupInfo.VersionedResourcesStorageMap) + if len(apiGroupInfo.VersionedResourcesStorageMap) == 0 { + klog.V(1).Infof("Removing API group %v because it is time to stop serving it because it has no versions per APILifecycle.", groupName) + continue + } + klog.V(1).Infof("Enabling API group %q.", groupName) if postHookProvider, ok := restStorageBuilder.(genericapiserver.PostStartHookProvider); ok {