Merge pull request #52227 from liggitt/non-preferred-version-priority

Automatic merge from submit-queue (batch tested with PRs 52227, 52120)

Fix discovery restmapper finding resources in non-preferred versions

Fixes: #52219

Also reverts behavioral changes to tests that version-qualified cronjobs to work around this issue.

The discovery rest mapper was only populating the priority rest mapper's search list with preferred groupversions.

That meant that if a resource existed in multiple non-preferred versions, AND did not exist in the preferred version (like cronjob, which only exists in v1beta2.batch and v2alpha1.batch, but not v1.batch), the priority restmapper would not find it in its group/version priority list, and would return an error.

```release-note
Fixed an issue looking up cronjobs when they existed in more than one API version
```

hack/make-rules/test-cmd-util.sh

@@ -1249,13 +1249,13 @@ run_kubectl_run_tests() {
   kubectl delete deployment nginx-apps "${kube_flags[@]}"
 
   # Pre-Condition: no Job exists
-  kube::test::get_object_assert cronjob.v1beta1.batch "{{range.items}}{{$id_field}}:{{end}}" ''
+  kube::test::get_object_assert cronjobs "{{range.items}}{{$id_field}}:{{end}}" ''
   # Command
   kubectl run pi --schedule="*/5 * * * *" --generator=cronjob/v1beta1 "--image=$IMAGE_PERL" --restart=OnFailure -- perl -Mbignum=bpi -wle 'print bpi(20)' "${kube_flags[@]}"
   # Post-Condition: CronJob "pi" is created
-  kube::test::get_object_assert cronjob.v1beta1.batch "{{range.items}}{{$id_field}}:{{end}}" 'pi:'
+  kube::test::get_object_assert cronjobs "{{range.items}}{{$id_field}}:{{end}}" 'pi:'
   # Clean up
-  kubectl delete cronjob.v1beta1.batch pi "${kube_flags[@]}"
+  kubectl delete cronjobs pi "${kube_flags[@]}"
 
   set +o nounset
   set +o errexit

staging/src/k8s.io/client-go/discovery/restmapper.go

@@ -49,6 +49,7 @@ func NewRESTMapper(groupResources []*APIGroupResources, versionInterfaces meta.V
 	for _, group := range groupResources {
 		groupPriority = append(groupPriority, group.Group.Name)
 
+		// Make sure the preferred version comes first
 		if len(group.Group.PreferredVersion.Version) != 0 {
 			preferred := group.Group.PreferredVersion.Version
 			if _, ok := group.VersionedResources[preferred]; ok {
@@ -72,6 +73,21 @@ func NewRESTMapper(groupResources []*APIGroupResources, versionInterfaces meta.V
 				continue
 			}
 
+			// Add non-preferred versions after the preferred version, in case there are resources that only exist in those versions
+			if discoveryVersion.Version != group.Group.PreferredVersion.Version {
+				resourcePriority = append(resourcePriority, schema.GroupVersionResource{
+					Group:    group.Group.Name,
+					Version:  discoveryVersion.Version,
+					Resource: meta.AnyResource,
+				})
+
+				kindPriority = append(kindPriority, schema.GroupVersionKind{
+					Group:   group.Group.Name,
+					Version: discoveryVersion.Version,
+					Kind:    meta.AnyKind,
+				})
+			}
+
 			gv := schema.GroupVersion{Group: group.Group.Name, Version: discoveryVersion.Version}
 			versionMapper := meta.NewDefaultRESTMapper([]schema.GroupVersion{gv}, versionInterfaces)
 

staging/src/k8s.io/client-go/discovery/restmapper_test.go

@@ -67,6 +67,32 @@ func TestRESTMapper(t *testing.T) {
 				},
 			},
 		},
+
+		// This group tests finding and prioritizing resources that only exist in non-preferred versions
+		{
+			Group: metav1.APIGroup{
+				Name: "unpreferred",
+				Versions: []metav1.GroupVersionForDiscovery{
+					{Version: "v1"},
+					{Version: "v2beta1"},
+					{Version: "v2alpha1"},
+				},
+				PreferredVersion: metav1.GroupVersionForDiscovery{Version: "v1"},
+			},
+			VersionedResources: map[string][]metav1.APIResource{
+				"v1": {
+					{Name: "broccoli", Namespaced: true, Kind: "Broccoli"},
+				},
+				"v2beta1": {
+					{Name: "broccoli", Namespaced: true, Kind: "Broccoli"},
+					{Name: "peas", Namespaced: true, Kind: "Pea"},
+				},
+				"v2alpha1": {
+					{Name: "broccoli", Namespaced: true, Kind: "Broccoli"},
+					{Name: "peas", Namespaced: true, Kind: "Pea"},
+				},
+			},
+		},
 	}
 
 	restMapper := NewRESTMapper(resources, nil)
@@ -123,6 +149,16 @@ func TestRESTMapper(t *testing.T) {
 				Kind:    "Job",
 			},
 		},
+		{
+			input: schema.GroupVersionResource{
+				Resource: "peas",
+			},
+			want: schema.GroupVersionKind{
+				Group:   "unpreferred",
+				Version: "v2beta1",
+				Kind:    "Pea",
+			},
+		},
 	}
 
 	for _, tc := range kindTCs {

test/e2e/kubectl/kubectl.go

@@ -234,7 +234,7 @@ var _ = SIGDescribe("Kubectl alpha client", func() {
 		})
 
 		AfterEach(func() {
-			framework.RunKubectlOrDie("delete", "cronjob.v2alpha1.batch", cjName, nsFlag)
+			framework.RunKubectlOrDie("delete", "cronjobs", cjName, nsFlag)
 		})
 
 		It("should create a CronJob", func() {
@@ -1380,7 +1380,7 @@ metadata:
 		})
 
 		AfterEach(func() {
-			framework.RunKubectlOrDie("delete", "cronjob.v1beta1.batch", cjName, nsFlag)
+			framework.RunKubectlOrDie("delete", "cronjobs", cjName, nsFlag)
 		})
 
 		It("should create a CronJob", func() {