Dynamic RESTMapper returns ErrRateLimited instead of expected NoKindMatchError

[timebertt]

/kind bug

In our code we have some client.Delete calls that should be ignored, when the respective resources are not available on the API server (e.g. additional CRDs not deployed, because some feature gate is disabled).
Therefore we use meta.IsNoMatchError and ignore the error, if it returns true.

So basically something like this:

if err := client.Delete(ctx, obj); client.IgnoreNotFound(err) != nil {
	if meta.IsNoMatchError(err) {
		return nil
	}
	return err
}

When under high load, we have a lot of these calls in parallel, which might end up with NoKindMatchErrors.

After switching to the dynamic RESTMapper in our clients, meta.IsNoMatchError can no longer catch this, as the dynamic RESTMapper returns ErrRateLimited instead of NoKindMatchError, which is (implicitly) expected from implementations of the meta.RESTMapper interface.

I went back and forth about how this can be improved and gave these options a thought:

1. returning the NoKindMatchError directly
   • is not backwards-compatible but would implement the expected behaviour
   • how would folks then detect that they are hitting the rate limit?
2. wrapping either the underlying NoKindMatchError or the ErrRateLimited with fmt.Errorf("%w... or errors.Wrap
   • you could then check the error with errors.As(err, &NoKindMatchError)
   • still kind of differing from the expected error and you will not be able to use meta.IsNoMatchError as well

I can do it in a PR, if we agree on one approach.

WDYT?
@estroz @DirectXMan12

[alvaroaleman]

meta.IsNoMatchError can no longer catch this, as the dynamic RESTMapper returns ErrRateLimited instead of NoKindMatchError, which is (implicitly) expected from implementations of the meta.RESTMapper interface.

Hm, that sounds wrong. I had a quick look at the code and it seems its basically built with the assumption that when we hit a NoKindMatch error its because we are stale, not because it actually doesn't exist on the server.

I think this here needs to be updated to return meta.NoKindMatchError if checkNeedsReload returned ErrRateLimited :

controller-runtime/pkg/client/apiutil/dynamicrestmapper.go

Line 212 in 0f6c191

return checkNeedsReload()

[timebertt]

it seems its basically built with the assumption that when we hit a NoKindMatch error its because we are stale, not because it actually doesn't exist on the server.

Yes, indeed. And this assumptions leads to rate limiting, when the rest mapper is heavily used, like in our case.

I think this here needs to be updated to return meta.NoKindMatchError if checkNeedsReload returned ErrRateLimited :

Hmm, do you mean this part here? I think, checkNeedsReload doesn't return ErrRateLimited but limiter.checkRate does.

controller-runtime/pkg/client/apiutil/dynamicrestmapper.go

Lines 202 to 204 in 0f6c191

	if err := drm.limiter.checkRate(); err != nil {
	return err
	}

The problem with returning NoKindMatchError here directly is that DelayIfRateLimited can't detect the rate limiting anymore :/
I think one option to solve this, would be to wrap NoKindMatchError in ErrRateLimited and add a second implementation for IsNoMatchErrror which tries to unwrap the error instead of checking for the type directly.
WDYT?

[alvaroaleman]

@timebertt sorry, this fell off my radar. Without going into the implementation, the behavior I would expect in the end is:

• We have a test that verifies that when using a new type that's not in the mapper but on the apiserver, the dynamicRestMapper will do a discovery and everything will work - I assume we already have this test today
• We have a test that verifies that when using a new type thats not in the mapper and not in the api, the restmapper will do a discovery, not find the type and return meta.NoKindMatchError

[timebertt]

/assign