Skip to content

🐛 Avoid creating a new scheme object when combining the options #2237

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

🐛 Avoid creating a new scheme object when combining the options #2237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9ed2ff0
Preserve the inherited scheme object when combining the options
ccojocar Mar 14, 2023
3c8199e
Add checks to pass unit tests
ccojocar Mar 14, 2023
9574853
Add a test case in manager for custom object selector created with Bu…
ccojocar Mar 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 10 additions & 13 deletions pkg/cache/cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,20 +287,17 @@ func (options Options) inheritFrom(inherited Options) (*Options, error) {
return &combined, nil
}

func combineScheme(schemes ...*runtime.Scheme) *runtime.Scheme {
var out *runtime.Scheme
for _, sch := range schemes {
if sch == nil {
continue
}
for gvk, t := range sch.AllKnownTypes() {
if out == nil {
out = runtime.NewScheme()
}
out.AddKnownTypeWithName(gvk, reflect.New(t).Interface().(runtime.Object))
}
func combineScheme(inherited *runtime.Scheme, new *runtime.Scheme) *runtime.Scheme {
if inherited == nil {
return new
}
if new == nil {
return inherited
}
for gvk, t := range new.AllKnownTypes() {
inherited.AddKnownTypeWithName(gvk, reflect.New(t).Interface().(runtime.Object))
}
return out
return inherited
Comment on lines +297 to +300
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this leak new scheme types into the inherited scheme? What happens here if (somehow) a conflicting scheme is required by a different cache, and that cache shares the same inherited scheme? Will we change the scheme out from under the first cache?

}

func selectMapper(def, override meta.RESTMapper) meta.RESTMapper {
Expand Down
41 changes: 41 additions & 0 deletions pkg/manager/manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,13 @@ import (
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/rest"
"k8s.io/client-go/tools/leaderelection/resourcelock"

configv1alpha1 "k8s.io/component-base/config/v1alpha1"
"sigs.k8s.io/controller-runtime/examples/crd/pkg"
"sigs.k8s.io/controller-runtime/pkg/cache"
"sigs.k8s.io/controller-runtime/pkg/cache/informertest"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -1156,6 +1158,45 @@ var _ = Describe("manger.Manager", func() {
)
})

Context("with custom object selector", func() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what exactly this is supposed to test and how this relates to the problem you described in the PR body? It also passes without your codechanges btw. It might be easier to unittest combineScheme directly

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying to test the scenario mentioned above in the description.

The main issue is when a scheme is combined using the BuilderWithOptions function, a new scheme object pointer is created and that pointer is configured into the cache -

controller-runtime/pkg/cache/cache.go

Line 300 in c3c1f05

out = runtime.NewScheme()
.

The issue is that the original scheme object pointer is configured into the manager/controller and not the newly created one by combineScheme, so this means all the schemes added to mgr.GetScheme(), object are no available in the cache. As a result, the controller will fail to start, returning the error "msg"="kind must be registered to the Scheme".

I think just unit-testing the combineScheme doesn't cover this scenario end to end. Also the combineScheme has unittest which already covers this entirely.

It("created with BuilderWithOptions and scheme added to manager", func() {
opts := Options{
NewCache: cache.BuilderWithOptions(cache.Options{
ByObject: map[client.Object]cache.ByObject{
&corev1.Pod{}: {
Label: labels.SelectorFromSet(labels.Set{
"test-label": "true",
}),
},
},
}),
}
m, err := New(cfg, opts)
Expect(err).NotTo(HaveOccurred())

var wgRunnableStarted sync.WaitGroup
wgRunnableStarted.Add(1)
Expect(m.Add(RunnableFunc(func(context.Context) error {
defer GinkgoRecover()
wgRunnableStarted.Done()
return nil
}))).To(Succeed())

Expect(pkg.AddToScheme(m.GetScheme())).NotTo(HaveOccurred())

ctx, cancel := context.WithCancel(context.Background())
defer cancel()
go func() {
defer GinkgoRecover()
Expect(m.Elected()).ShouldNot(BeClosed())
Expect(m.Start(ctx)).NotTo(HaveOccurred())
}()

<-m.Elected()
wgRunnableStarted.Wait()
})
})

Context("should start serving metrics", func() {
var listener net.Listener
var opts Options
Expand Down