Controller-runtime: RESTMapper doesn't update to reflect new CRDs

[danwinship]

Currently, if you use a controller-runtime Client to first create a CRD, and then create an instance of that CRD, you get an error about, eg, no matches for kind "NetNamespace" in version "network.openshift.io/v1". Because the client's RESTMapper is initialized at startup and then never updated with any new information about newly-available resource kinds, so it is only able to work with the kinds that existed before it was created.

Our workaround for now is a hacked-up wrapper RESTMapper that watches for that error and reloads its cached data and tries again if it sees it. If you assume that that error will never occur other than in this case, then that seems like a plausible way to solve the problem. I could rework this into a PR for controller-runtime if you want. The only question would be whether it should replace the existing default RESTMapper or if it should be an alternative MapperProvider function and users have to pick whether they want static vs dynamic.

Or OTOH maybe the real fix should be at a lower level? I came across DeferredDiscoveryRESTMapper which is similar to my wrapper, but doesn't auto-invalidate on cache miss. But we could make it do that, and then change controller-runtime to use that?

[danwinship]

oh, meant to link to our workaround, https://github.com/openshift/cluster-network-operator/pull/95

[DirectXMan12]

Yeah, I'd been meaning to port some of the code I wrote over to deal with resources added later. We'd probably want rate-limiting on the cache invalidation, but otherwise I'd be open to making that the default implementation. Watchable discovery would also be nice at some point :-/.

Thus far, the problem is generally mitigated by the fact your pod can just fail and restart, but there are other reasons that updating discovery information is nice.

Feel free to send a PR.

[DirectXMan12]

/kind feature
/priority important-longterm

[JoelSpeed]

We came across this problem recently and for the moment have created a RestMapper using the LazyRestMapperLoader and a FirstHitRESTMapper something like below:

drm, err := apiutil.NewDiscoveryRESTMapper(config)
if err != nil {
    return nil, err
}           
lrm := meta.NewLazyRESTMapperLoader(func() (meta.RESTMapper, error) {
    return apiutil.NewDiscoveryRESTMapper(config)
})
 options.Mapper = meta.FirstHitRESTMapper{MultiRESTMapper: meta.MultiRESTMapper{drm, lrm}}

The idea being that if any CRDs are loaded in after the first discovery, the LazyRESTMapperLoader should pick those up, but for the most part we use the original DiscoveryRESTMapper. Would this be worth doing in for Controller-Runtime or is there a better way that invalidates the Discovery cache somehow?

[DirectXMan12]

I was thinking something like a lazy discovery rest mapper, with a wrapper that invalidates on cache misses, but in a rate-limited way. You can use the DeferredDiscoveryRESTMapper with some custom wrapper around it to do the invalidation and rate limiting (since the deferredsicoveryrestmapper won't actually invalidate).

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[kron4eg]

/remove-lifecycle stale

[DirectXMan12]

/help-wanted

[chrisplo]

I ran into a similar issue when starting a controller with a "Kind" Source that I know was installed after the Manager client was created, thought it was going to be a race condition since it seems to be pulling from cache: (controller-runtime/pkg/source/source.go: Start):

    // Lookup the Informer from the Cache and add an EventHandler which populates the Queue
    i, err := ks.cache.GetInformer(ks.Type)
    if err != nil {
        if kindMatchErr, ok := err.(*meta.NoKindMatchError); ok {
            log.Error(err, "if kind is a CRD, it should be installed before calling Start",
                "kind", kindMatchErr.GroupKind)
        }
        return err
    }

~But sounds like I need to work on the Manager Client?~

looks like @danwinship workaround is working for me,seems straightforward enough

[vincepri]

@DirectXMan12 I've just hit this issue in Cluster API, I added a new Cached (and rate limited) RESTMapper here https://github.com/kubernetes-sigs/cluster-api/pull/1280/commits/ee96c31eea0f9b1b2f4cffae5c7da2c274722e76. If you feel like that's a reasonable implementation, I'm happy to PR against controller-runtime as well.

[DirectXMan12]

We're just about ready to get #554 merged , that should solve your problem. Can you try that?

[vincepri]

@DirectXMan12 Thanks, after a quick glance it looks very similar, it should work as well. I'm happy to switch once it'll be merged in controller-runtime.