Merge pull request #36444 from soltysh/issue31481

Automatic merge from submit-queue

Read all resources for finalization and gc, not just preferred

Fixes #31481. 

Currently when starting namespace controller or garbage collector we only gather preferred version resources, which in case of multiple versions (you guessed it `batch/v2alpha1.CronJobs` again) isn't sufficient. This PR adds additional method which actually retrieves all resources from all versions and works on them.

@kubernetes/sig-api-machinery ptal

pkg/client/typed/discovery/discovery_client.go

@@ -213,9 +213,11 @@ func (d *DiscoveryClient) serverPreferredResources(namespaced bool) ([]unversion
 	const maxRetries = 2
 	var failedGroups map[unversioned.GroupVersion]error
 	var results []unversioned.GroupVersionResource
+	var resources map[unversioned.GroupResource]string
 RetrieveGroups:
 	for i := 0; i < maxRetries; i++ {
 		results = []unversioned.GroupVersionResource{}
+		resources = map[unversioned.GroupResource]string{}
 		failedGroups = make(map[unversioned.GroupVersion]error)
 		serverGroupList, err := d.ServerGroups()
 		if err != nil {
@@ -223,25 +225,40 @@ RetrieveGroups:
 		}
 
 		for _, apiGroup := range serverGroupList.Groups {
-			preferredVersion := apiGroup.PreferredVersion
-			groupVersion := unversioned.GroupVersion{Group: apiGroup.Name, Version: preferredVersion.Version}
-			apiResourceList, err := d.ServerResourcesForGroupVersion(preferredVersion.GroupVersion)
-			if err != nil {
-				if i < maxRetries-1 {
-					continue RetrieveGroups
-				}
-				failedGroups[groupVersion] = err
-				continue
-			}
-			for _, apiResource := range apiResourceList.APIResources {
-				// ignore the root scoped resources if "namespaced" is true.
-				if namespaced && !apiResource.Namespaced {
+			versions := apiGroup.Versions
+			for _, version := range versions {
+				groupVersion := unversioned.GroupVersion{Group: apiGroup.Name, Version: version.Version}
+				apiResourceList, err := d.ServerResourcesForGroupVersion(version.GroupVersion)
+				if err != nil {
+					if i < maxRetries-1 {
+						continue RetrieveGroups
+					}
+					failedGroups[groupVersion] = err
 					continue
 				}
-				if strings.Contains(apiResource.Name, "/") {
-					continue
+				for _, apiResource := range apiResourceList.APIResources {
+					// ignore the root scoped resources if "namespaced" is true.
+					if namespaced && !apiResource.Namespaced {
+						continue
+					}
+					if strings.Contains(apiResource.Name, "/") {
+						continue
+					}
+					gvr := groupVersion.WithResource(apiResource.Name)
+					if _, ok := resources[gvr.GroupResource()]; ok {
+						if gvr.Version != apiGroup.PreferredVersion.Version {
+							continue
+						}
+						// remove previous entry, because it will be replaced with a preferred one
+						for i := range results {
+							if results[i].GroupResource() == gvr.GroupResource() {
+								results = append(results[:i], results[i+1:]...)
+							}
+						}
+					}
+					resources[gvr.GroupResource()] = gvr.Version
+					results = append(results, gvr)
 				}
-				results = append(results, groupVersion.WithResource(apiResource.Name))
 			}
 		}
 		if len(failedGroups) == 0 {

pkg/client/typed/discovery/discovery_client_test.go

@@ -321,7 +321,7 @@ func TestGetSwaggerSchemaFail(t *testing.T) {
 	}
 }
 
-func TestGetServerPreferredResources(t *testing.T) {
+func TestServerPreferredResources(t *testing.T) {
 	stable := unversioned.APIResourceList{
 		GroupVersion: "v1",
 		APIResources: []unversioned.APIResource{
@@ -330,14 +330,6 @@ func TestGetServerPreferredResources(t *testing.T) {
 			{Name: "namespaces", Namespaced: false, Kind: "Namespace"},
 		},
 	}
-	/*beta := unversioned.APIResourceList{
-		GroupVersion: "extensions/v1",
-		APIResources: []unversioned.APIResource{
-			{Name: "deployments", Namespaced: true, Kind: "Deployment"},
-			{Name: "ingresses", Namespaced: true, Kind: "Ingress"},
-			{Name: "jobs", Namespaced: true, Kind: "Job"},
-		},
-	}*/
 	tests := []struct {
 		resourcesList *unversioned.APIResourceList
 		response      func(w http.ResponseWriter, req *http.Request)
@@ -427,9 +419,6 @@ func TestGetServerPreferredResources(t *testing.T) {
 				w.Write(output)
 			},
 		},
-		/*{
-			resourcesList: &stable,
-		},*/
 	}
 	for _, test := range tests {
 		server := httptest.NewServer(http.HandlerFunc(test.response))
@@ -455,7 +444,7 @@ func TestGetServerPreferredResources(t *testing.T) {
 	}
 }
 
-func TestGetServerPreferredResourcesRetries(t *testing.T) {
+func TestServerPreferredResourcesRetries(t *testing.T) {
 	stable := unversioned.APIResourceList{
 		GroupVersion: "v1",
 		APIResources: []unversioned.APIResource{
@@ -553,3 +542,173 @@ func TestGetServerPreferredResourcesRetries(t *testing.T) {
 		server.Close()
 	}
 }
+
+func TestServerPreferredNamespacedResources(t *testing.T) {
+	stable := unversioned.APIResourceList{
+		GroupVersion: "v1",
+		APIResources: []unversioned.APIResource{
+			{Name: "pods", Namespaced: true, Kind: "Pod"},
+			{Name: "services", Namespaced: true, Kind: "Service"},
+			{Name: "namespaces", Namespaced: false, Kind: "Namespace"},
+		},
+	}
+	batchv1 := unversioned.APIResourceList{
+		GroupVersion: "batch/v1",
+		APIResources: []unversioned.APIResource{
+			{Name: "jobs", Namespaced: true, Kind: "Job"},
+		},
+	}
+	batchv2alpha1 := unversioned.APIResourceList{
+		GroupVersion: "batch/v2alpha1",
+		APIResources: []unversioned.APIResource{
+			{Name: "jobs", Namespaced: true, Kind: "Job"},
+			{Name: "cronjobs", Namespaced: true, Kind: "CronJob"},
+		},
+	}
+	batchv3alpha1 := unversioned.APIResourceList{
+		GroupVersion: "batch/v3alpha1",
+		APIResources: []unversioned.APIResource{
+			{Name: "jobs", Namespaced: true, Kind: "Job"},
+			{Name: "cronjobs", Namespaced: true, Kind: "CronJob"},
+		},
+	}
+	tests := []struct {
+		response func(w http.ResponseWriter, req *http.Request)
+		expected []unversioned.GroupVersionResource
+	}{
+		{
+			response: func(w http.ResponseWriter, req *http.Request) {
+				var list interface{}
+				switch req.URL.Path {
+				case "/api/v1":
+					list = &stable
+				case "/api":
+					list = &unversioned.APIVersions{
+						Versions: []string{
+							"v1",
+						},
+					}
+				default:
+					t.Logf("unexpected request: %s", req.URL.Path)
+					w.WriteHeader(http.StatusNotFound)
+					return
+				}
+				output, err := json.Marshal(list)
+				if err != nil {
+					t.Errorf("unexpected encoding error: %v", err)
+					return
+				}
+				w.Header().Set("Content-Type", "application/json")
+				w.WriteHeader(http.StatusOK)
+				w.Write(output)
+			},
+			expected: []unversioned.GroupVersionResource{
+				{Group: "", Version: "v1", Resource: "pods"},
+				{Group: "", Version: "v1", Resource: "services"},
+			},
+		},
+		{
+			response: func(w http.ResponseWriter, req *http.Request) {
+				var list interface{}
+				switch req.URL.Path {
+				case "/apis":
+					list = &unversioned.APIGroupList{
+						Groups: []unversioned.APIGroup{
+							{
+								Name: "batch",
+								Versions: []unversioned.GroupVersionForDiscovery{
+									{GroupVersion: "batch/v1", Version: "v1"},
+									{GroupVersion: "batch/v2alpha1", Version: "v2alpha1"},
+									{GroupVersion: "batch/v3alpha1", Version: "v3alpha1"},
+								},
+								PreferredVersion: unversioned.GroupVersionForDiscovery{GroupVersion: "batch/v1", Version: "v1"},
+							},
+						},
+					}
+				case "/apis/batch/v1":
+					list = &batchv1
+				case "/apis/batch/v2alpha1":
+					list = &batchv2alpha1
+				case "/apis/batch/v3alpha1":
+					list = &batchv3alpha1
+				default:
+					t.Logf("unexpected request: %s", req.URL.Path)
+					w.WriteHeader(http.StatusNotFound)
+					return
+				}
+				output, err := json.Marshal(list)
+				if err != nil {
+					t.Errorf("unexpected encoding error: %v", err)
+					return
+				}
+				w.Header().Set("Content-Type", "application/json")
+				w.WriteHeader(http.StatusOK)
+				w.Write(output)
+			},
+			expected: []unversioned.GroupVersionResource{
+				{Group: "batch", Version: "v1", Resource: "jobs"},
+				{Group: "batch", Version: "v2alpha1", Resource: "cronjobs"},
+			},
+		},
+		{
+			response: func(w http.ResponseWriter, req *http.Request) {
+				var list interface{}
+				switch req.URL.Path {
+				case "/apis":
+					list = &unversioned.APIGroupList{
+						Groups: []unversioned.APIGroup{
+							{
+								Name: "batch",
+								Versions: []unversioned.GroupVersionForDiscovery{
+									{GroupVersion: "batch/v1", Version: "v1"},
+									{GroupVersion: "batch/v2alpha1", Version: "v2alpha1"},
+									{GroupVersion: "batch/v3alpha1", Version: "v3alpha1"},
+								},
+								PreferredVersion: unversioned.GroupVersionForDiscovery{GroupVersion: "batch/v2alpha", Version: "v2alpha1"},
+							},
+						},
+					}
+				case "/apis/batch/v1":
+					list = &batchv1
+				case "/apis/batch/v2alpha1":
+					list = &batchv2alpha1
+				case "/apis/batch/v3alpha1":
+					list = &batchv3alpha1
+				default:
+					t.Logf("unexpected request: %s", req.URL.Path)
+					w.WriteHeader(http.StatusNotFound)
+					return
+				}
+				output, err := json.Marshal(list)
+				if err != nil {
+					t.Errorf("unexpected encoding error: %v", err)
+					return
+				}
+				w.Header().Set("Content-Type", "application/json")
+				w.WriteHeader(http.StatusOK)
+				w.Write(output)
+			},
+			expected: []unversioned.GroupVersionResource{
+				{Group: "batch", Version: "v2alpha1", Resource: "jobs"},
+				{Group: "batch", Version: "v2alpha1", Resource: "cronjobs"},
+			},
+		},
+	}
+	for _, test := range tests {
+		server := httptest.NewServer(http.HandlerFunc(test.response))
+		defer server.Close()
+
+		client := NewDiscoveryClientForConfigOrDie(&restclient.Config{Host: server.URL})
+		got, err := client.ServerPreferredNamespacedResources()
+		if err != nil {
+			t.Errorf("unexpected error: %v", err)
+			continue
+		}
+		// we need deterministic order and since during processing in ServerPreferredNamespacedResources
+		// a map comes into play the result needs sorting
+		if !reflect.DeepEqual(got, test.expected) {
+			t.Errorf("expected:\n%v\ngot:\n%v\n", test.expected, got)
+		}
+		server.Close()
+	}
+}