Panic from dynamicRESTMapper when using lazy initialization

[siliconbrain]

When dynamicRESTMapper lazy initialization fails, any subsequent calls to it will panic (runtime error: invalid memory address or nil pointer dereference) because staticMapper is uninitialized.

I ran into this when my cluster could not be reached, so I'll illustrate the problem this way, but any error returned by setStaticMapper during init will result in the same issue.

How to reproduce

import (
	"k8s.io/apimachinery/pkg/runtime/schema"
	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
	"sigs.k8s.io/controller-runtime/pkg/client/config"
)

func main() {
	// get a client config; this should succeed
	cfg := config.GetConfigOrDie()
	// to simulate a refused connection, set cfg.Host to some URL that won't answer
	cfg.Host = "https://127.0.0.1:12345"
	// create a new dynamicRESTMapper instance with lazy initialization; this should succeed
	mapper, _ := apiutil.NewDynamicRESTMapper(cfg, apiutil.WithLazyDiscovery)
	// call any method on mapper; this should result in an error
	_, err := mapper.KindFor(schema.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"})
	if err != nil {
		fmt.Println("failed to get kind for pods")
	}
	// later on (or from another place/goroutine in your code), call any method on mapper; this should panic
	_, err := mapper.KindFor(schema.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"})
	if err != nil {
		fmt.Println("failed to get kind for pods")
	}
}

Suggested fix

sync.Once is not suitable for guarding the lazy initialization code because the initialization should be retried until successful, not just tried once. I couldn't find an existing solution, so I propose to create a custom version of sync.Once that is done only when the function given to Do returns with true, although, other alternative implementations might be fine too.

type Once struct {
	done uint32
	m    Mutex
}

func (o *Once) Do(f func() bool) {
	if atomic.LoadUint32(&o.done) == 0 {
		o.doSlow(f)
	}
}

func (o *Once) doSlow(f func() bool) {
	o.m.Lock()
	defer o.m.Unlock()
	if o.done == 0 {
		if f() {
			atomic.StoreUint32(&o.done, 1)
		}
	}
}

This should be used instead of sync.Once, updating the inline function passed to Do with a return err == nil statement.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[siliconbrain]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[siliconbrain]

/remove-lifecycle stale