Skip to content

Commit

Permalink
fix panic for lazy dynamicRESTMapper
Browse files Browse the repository at this point in the history
  • Loading branch information
airycanon committed Sep 27, 2022
1 parent 67d2efd commit a7a1898
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 30 deletions.
2 changes: 1 addition & 1 deletion pkg/client/apiutil/apiutil_suite_test.go
Original file line number Diff line number Diff line change
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
35 changes: 14 additions & 21 deletions pkg/client/apiutil/dynamicrestmapper.go
Original file line number Diff line number Diff line change
@@ -1,24 +1,9 @@
/*
Copyright 2019 The Kubernetes Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package apiutil

import (
"errors"
"sync"
"sync/atomic"

"golang.org/x/time/rate"
"k8s.io/apimachinery/pkg/api/meta"
Expand All @@ -38,7 +23,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 +111,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
48 changes: 40 additions & 8 deletions pkg/client/apiutil/dynamicrestmapper_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,23 @@
package apiutil_test
/*
Copyright 2021 The Kubernetes Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package apiutil

import (
"fmt"
"time"

. "github.com/onsi/ginkgo"
Expand All @@ -10,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 @@ -36,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 @@ -130,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 fail 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 a7a1898

Please sign in to comment.