DynamicRESTMapper behavior changes, can't do mapping when only resource is set in gvr.

[xuezhaojun]

Recently, we have updated controller-runtime from v0.14.6 to the latest version, and found a behavior change of DynamicRESTMapper.

This is the sample code, it uses latest version and v0.14.6 to call KindFor:

package main

import (
	"fmt"

	"k8s.io/apimachinery/pkg/runtime/schema"
	"k8s.io/client-go/tools/clientcmd"
	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
)

func main() {
	kubeconfigPath := "./kubeconfig.yaml"

	kubeconfig, err := clientcmd.BuildConfigFromFlags("", kubeconfigPath)
	if err != nil {
		fmt.Println(err)
		return
	}

	// latest version
	// httpClient, err := rest.HTTPClientFor(kubeconfig)
	// if err != nil {
	// 	fmt.Println(err, "Unable to create http client.")
	// 	return
	// }

	// restMapper, err := apiutil.NewDynamicRESTMapper(kubeconfig, httpClient)
	// if err != nil {
	// 	fmt.Println(err, "Unable to create restmapper.")
	// 	return
	// }

	// v0.14.6
	restMapper, err := apiutil.NewDynamicRESTMapper(kubeconfig, apiutil.WithLazyDiscovery)
	if err != nil {
		fmt.Println(err, "Unable to create restmapper.")
		return
	}

	// Call restMapper on resource "deployment"
	gvk, err := restMapper.KindFor(schema.GroupVersionResource{
		Resource: "deployments",
	})
	if err != nil {
		fmt.Println(err, "Unable to get gvk.")
		return
	}

	fmt.Println(gvk)
}

With v0.14.6, it returns no error.

But with the latest code, it will return the error:

no matches for /, Resource=deployments

Is there any workaround for this? Do we consider this as a compatibility issue?

[troy0820]

/kind support

[troy0820]

These are from the release notes of v0.15.0 which is a breaking change:

- client/apiutil.NewDynamicRESTMapper signature has changed and now requires an *http.Client as parameter.

- Use and offer a single dynamic lazy RESTMapper (https://github.com/kubernetes-sigs/controller-runtime/pull/2116, https://github.com/kubernetes-sigs/controller-runtime/pull/2296)
   - The following options have been removed: WithCustomMapper, WithExperimentalLazyMapper, WithLazyDiscovery, WithLimiter.
   - The DynamicRESTMapperOption type has been removed.

From the testing it looks like it should error if the group doesn't exist,
https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/client/apiutil/restmapper_test.go#L292

I'm not sure if you are also hitting that when you put in the group.

[xuezhaojun]

Putting in the group could be a workaround for us, but we used to provide an API that allows user just use "resource" to do mapping and we need to keep this compatibility for at least another round.

Just to make sure whether this change would be fixed.

If this breaking change meant not be fixed in the furture, we may need to plan changes in our API as well to catch up.

[troy0820]

Putting in the group could be a workaround for us, but we used to provide an API that allows user just use "resource" to do mapping and we need to keep this compatibility for at least another round.

I think the workaround is the new defined schema for the contract going forward in controller-runtime and not sure if they intend to go back to what used to be the behavior. What you are saying is that the new intended behavior doesn't match what used to be, but that work was the result of the breaking change moving from v0.14.6 to v0.15.x.

This seems to be an enhancement honestly to make sure that the group is specified with the resource to prevent mapping multiple groups to a single "resource" and prevent naming collisions, etc.

Just to make sure whether this change would be fixed.

I can't speak for the maintainers, but I don't believe that this change will go back to what used to be in older versions.

[xuezhaojun]

Thanks for explaining @troy0820, it makes sense to me now to see it as an enhancement. We will plan a catch-up soon.

[alvaroaleman]

The basic issue with the current restmapper is that it was purely build to be lazy in the context of how controller-runtime uses it. The upstream restmapper which we used with a thin wrapping before has all kinds of logic for what to do when it is called with incomplete input. We don't really want to duplicate that and it would be very difficult to still keep it lazy.

The best suggestion I can offer at the moment is to directly use a discovery restmapper if you need it to work with less information than the full GVK.

[xuezhaojun]

The basic issue with the current restmapper is that it was purely build to be lazy in the context of how controller-runtime uses it. The upstream restmapper which we used with a thin wrapping before has all kinds of logic for what to do when it is called with incomplete input. We don't really want to duplicate that and it would be very difficult to still keep it lazy.

The best suggestion I can offer at the moment is to directly use a discovery restmapper if you need it to work with less information than the full GVK.

Thanks for the advice!

[troy0820]

@xuezhaojun can we close this issue?

[troy0820]

Closing as issue has been addressed with advice/workaround.

If you need to reopen it for more clarity, please feel free.

/close

[k8s-ci-robot]

@troy0820: Closing this issue.

In response to this:

Closing as issue has been addressed with advice/workaround.

If you need to reopen it for more clarity, please feel free.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.