Merge pull request kubernetes#121283 from tnqn/cleanup-unregistered-group

Unregister group path from apiserver when the group no longer exists

Author: k8s-ci-robot

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/<group> 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.

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"
@@ -560,7 +561,6 @@ func TestSampleAPIServer(ctx context.Context, f *framework.Framework, aggrclient
 	ginkgo.By("Adding a label to the APIService")
 	apiServiceClient := aggrclient.ApiregistrationV1().APIServices()
 	apiServiceLabel := map[string]string{"e2e-apiservice": "patched"}
-	apiServiceLabelSelector := labels.SelectorFromSet(apiServiceLabel).String()
 	apiServicePatch, err := json.Marshal(map[string]interface{}{
 		"metadata": map[string]interface{}{
 			"labels": apiServiceLabel,
@@ -641,7 +641,7 @@ func TestSampleAPIServer(ctx context.Context, f *framework.Framework, aggrclient
 	framework.Logf("Found updated apiService label for %q", apiServiceName)
 
 	// kubectl delete flunder test-flunder
-	ginkgo.By(fmt.Sprintf("Delete APIService %q", flunderName))
+	ginkgo.By(fmt.Sprintf("Delete flunders resource %q", flunderName))
 	err = dynamicClient.Delete(ctx, flunderName, metav1.DeleteOptions{})
 	validateErrorWithDebugInfo(ctx, f, err, pods, "deleting flunders(%v) using dynamic client", unstructuredList.Items)
 
@@ -724,6 +724,7 @@ func TestSampleAPIServer(ctx context.Context, f *framework.Framework, aggrclient
 	}
 	framework.Logf("Found patched status condition for %s", wardle.ObjectMeta.Name)
 
+	apiServiceLabelSelector := labels.SelectorFromSet(updatedApiService.Labels).String()
 	ginkgo.By(fmt.Sprintf("APIService deleteCollection with labelSelector: %q", apiServiceLabelSelector))
 
 	err = aggrclient.ApiregistrationV1().APIServices().DeleteCollection(ctx,
@@ -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)
 }