From 0cda42af7ab8b9b63d3b6ce1bf1c3766f15a2b00 Mon Sep 17 00:00:00 2001 From: Quan Tian Date: Tue, 17 Oct 2023 16:40:34 +0800 Subject: [PATCH] Unregister group path from apiserver when the group no longer exists After a CRD or an APIService was deleted, the corresponding group was never unregistered. It caused a stale entry to remain in the root path and could potentially lead to memory leak as the groupDiscoveryHandler was never released and the handledGroups was never cleaned up. The commit implements the cleanup. It tracks each group's usage and unregister the a group when there is no version for this group. Signed-off-by: Quan Tian --- .../pkg/apiserver/apiserver.go | 27 ++++++++++++++----- test/e2e/apimachinery/aggregator.go | 19 +++++++++++++ 2 files changed, 39 insertions(+), 7 deletions(-) diff --git a/staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go b/staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go index 88775f4b098..3df617aad87 100644 --- a/staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go +++ b/staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go @@ -144,8 +144,9 @@ type APIAggregator struct { // proxyHandlers are the proxy handlers that are currently registered, keyed by apiservice.name proxyHandlers map[string]*proxyHandler - // handledGroups are the groups that already have routes - handledGroups sets.String + // handledGroupVersions contain the groups that already have routes. The key is the name of the group and the value + // is the versions for the group. + handledGroupVersions map[string]sets.Set[string] // lister is used to add group handling for /apis/ aggregator lookups based on // controller state @@ -235,7 +236,7 @@ func (c completedConfig) NewWithDelegate(delegationTarget genericapiserver.Deleg delegateHandler: delegationTarget.UnprotectedHandler(), proxyTransportDial: proxyTransportDial, proxyHandlers: map[string]*proxyHandler{}, - handledGroups: sets.String{}, + handledGroupVersions: map[string]sets.Set[string]{}, lister: informerFactory.Apiregistration().V1().APIServices().Lister(), APIRegistrationInformers: informerFactory, serviceResolver: c.ExtraConfig.ServiceResolver, @@ -524,7 +525,9 @@ func (s *APIAggregator) AddAPIService(apiService *v1.APIService) error { } // if we've already registered the path with the handler, we don't want to do it again. - if s.handledGroups.Has(apiService.Spec.Group) { + versions, exist := s.handledGroupVersions[apiService.Spec.Group] + if exist { + versions.Insert(apiService.Spec.Version) return nil } @@ -539,7 +542,7 @@ func (s *APIAggregator) AddAPIService(apiService *v1.APIService) error { // aggregation is protected s.GenericAPIServer.Handler.NonGoRestfulMux.Handle(groupPath, groupDiscoveryHandler) s.GenericAPIServer.Handler.NonGoRestfulMux.UnlistedHandle(groupPath+"/", groupDiscoveryHandler) - s.handledGroups.Insert(apiService.Spec.Group) + s.handledGroupVersions[apiService.Spec.Group] = sets.New[string](apiService.Spec.Version) return nil } @@ -568,8 +571,18 @@ func (s *APIAggregator) RemoveAPIService(apiServiceName string) { } delete(s.proxyHandlers, apiServiceName) - // TODO unregister group level discovery when there are no more versions for the group - // We don't need this right away because the handler properly delegates when no versions are present + versions, exist := s.handledGroupVersions[version.Group] + if !exist { + return + } + versions.Delete(version.Version) + if versions.Len() > 0 { + return + } + delete(s.handledGroupVersions, version.Group) + groupPath := "/apis/" + version.Group + s.GenericAPIServer.Handler.NonGoRestfulMux.Unregister(groupPath) + s.GenericAPIServer.Handler.NonGoRestfulMux.Unregister(groupPath + "/") } // DefaultAPIResourceConfigSource returns default configuration for an APIResource. diff --git a/test/e2e/apimachinery/aggregator.go b/test/e2e/apimachinery/aggregator.go index 529c3353228..b7f12162dc6 100644 --- a/test/e2e/apimachinery/aggregator.go +++ b/test/e2e/apimachinery/aggregator.go @@ -52,6 +52,7 @@ import ( admissionapi "k8s.io/pod-security-admission/api" samplev1alpha1 "k8s.io/sample-apiserver/pkg/apis/wardle/v1alpha1" "k8s.io/utils/pointer" + "k8s.io/utils/strings/slices" "github.com/onsi/ginkgo/v2" "github.com/onsi/gomega" @@ -736,6 +737,24 @@ func TestSampleAPIServer(ctx context.Context, f *framework.Framework, aggrclient framework.ExpectNoError(err, "failed to count the required APIServices") framework.Logf("APIService %s has been deleted.", apiServiceName) + ginkgo.By("Confirm that the group path of " + apiServiceName + " was removed from root paths") + groupPath := "/apis/" + apiServiceGroupName + err = wait.PollUntilContextTimeout(ctx, apiServiceRetryPeriod, apiServiceRetryTimeout, true, func(ctx context.Context) (done bool, err error) { + rootPaths := metav1.RootPaths{} + statusContent, err = restClient.Get(). + AbsPath("/"). + SetHeader("Accept", "application/json").DoRaw(ctx) + if err != nil { + return false, err + } + err = json.Unmarshal(statusContent, &rootPaths) + if err != nil { + return false, err + } + return !slices.Contains(rootPaths.Paths, groupPath), nil + }) + framework.ExpectNoError(err, "Expected to not find %s from root paths", groupPath) + cleanupSampleAPIServer(ctx, client, aggrclient, n, apiServiceName) }