Skip to content

Commit

Permalink
Fix panic for lazy dynamicRESTMapper
Browse files Browse the repository at this point in the history
Signed-off-by: FillZpp <FillZpp.pub@gmail.com>
  • Loading branch information
FillZpp committed May 9, 2022
1 parent 51159d3 commit 8bdad41
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 14 deletions.
2 changes: 1 addition & 1 deletion pkg/client/apiutil/apiutil_suite_test.go
Expand Up @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package apiutil_test
package apiutil

import (
"testing"
Expand Down
19 changes: 14 additions & 5 deletions pkg/client/apiutil/dynamicrestmapper.go
Expand Up @@ -19,6 +19,7 @@ package apiutil
import (
"errors"
"sync"
"sync/atomic"

"golang.org/x/time/rate"
"k8s.io/apimachinery/pkg/api/meta"
Expand All @@ -38,7 +39,8 @@ type dynamicRESTMapper struct {

lazy bool
// Used for lazy init.
initOnce sync.Once
inited uint32
initMtx sync.Mutex
}

// DynamicRESTMapperOption is a functional option on the dynamicRESTMapper.
Expand Down Expand Up @@ -125,11 +127,18 @@ func (drm *dynamicRESTMapper) setStaticMapper() error {

// init initializes drm only once if drm is lazy.
func (drm *dynamicRESTMapper) init() (err error) {
drm.initOnce.Do(func() {
if drm.lazy {
err = drm.setStaticMapper()
// skip init if drm is not lazy or has initialized
if !drm.lazy || atomic.LoadUint32(&drm.inited) != 0 {
return nil
}

drm.initMtx.Lock()
defer drm.initMtx.Unlock()
if drm.inited == 0 {
if err = drm.setStaticMapper(); err == nil {
atomic.StoreUint32(&drm.inited, 1)
}
})
}
return err
}

Expand Down
32 changes: 24 additions & 8 deletions pkg/client/apiutil/dynamicrestmapper_test.go
Expand Up @@ -14,9 +14,10 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package apiutil_test
package apiutil

import (
"fmt"
"time"

. "github.com/onsi/ginkgo"
Expand All @@ -26,8 +27,6 @@ import (
"golang.org/x/time/rate"
"k8s.io/apimachinery/pkg/api/meta"
"k8s.io/apimachinery/pkg/runtime/schema"

"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
)

var (
Expand All @@ -52,7 +51,7 @@ var _ = Describe("Dynamic REST Mapper", func() {
}

lim = rate.NewLimiter(rate.Limit(5), 5)
mapper, err = apiutil.NewDynamicRESTMapper(cfg, apiutil.WithLimiter(lim), apiutil.WithCustomMapper(func() (meta.RESTMapper, error) {
mapper, err = NewDynamicRESTMapper(cfg, WithLimiter(lim), WithCustomMapper(func() (meta.RESTMapper, error) {
baseMapper := meta.NewDefaultRESTMapper(nil)
addToMapper(baseMapper)

Expand Down Expand Up @@ -146,11 +145,28 @@ var _ = Describe("Dynamic REST Mapper", func() {
By("ensuring that it was only refreshed once")
Expect(count).To(Equal(1))
})
}

PIt("should lazily initialize if the lazy option is used", func() {

})
It("should lazily initialize if the lazy option is used", func() {
var err error
var failedOnce bool
mockErr := fmt.Errorf("mock failed once")
mapper, err = NewDynamicRESTMapper(cfg, WithLazyDiscovery, WithCustomMapper(func() (meta.RESTMapper, error) {
// Make newMapper failed once
if !failedOnce {
failedOnce = true
return nil, mockErr
}
baseMapper := meta.NewDefaultRESTMapper(nil)
addToMapper(baseMapper)
return baseMapper, nil
}))
Expect(err).NotTo(HaveOccurred())
Expect(mapper.(*dynamicRESTMapper).staticMapper).To(BeNil())

Expect(callWithTarget()).To(MatchError(mockErr))
Expect(callWithTarget()).To(Succeed())
})
}

Describe("KindFor", func() {
mapperTest(func() error {
Expand Down

0 comments on commit 8bdad41

Please sign in to comment.