-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
🐛 Avoid creating a new scheme object when combining the options #2237
Conversation
The inherited scheme was already configured in the cluster object, by creating a new scheme object for the cache object when combining the schemes will not allow the scheme registered afterwards into the cluster to be visible into the cache.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ccojocar The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Please provide a testcase for the underlying bug |
I'll try to add one. |
@alvaroaleman I added a test case in manager. I hope that's the right place for this. |
@@ -1156,6 +1158,45 @@ var _ = Describe("manger.Manager", func() { | |||
) | |||
}) | |||
|
|||
Context("with custom object selector", func() { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
for gvk, t := range new.AllKnownTypes() { | ||
inherited.AddKnownTypeWithName(gvk, reflect.New(t).Interface().(runtime.Object)) | ||
} | ||
return out | ||
return inherited |
There was a problem hiding this comment.
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?
This may be an area of the design where we didn't really consider the possibility of schemes changing dynamically. Should adding more types to This seems like it could absolutely be an issue in the case of a multi-cluster controller where the schemes for different clusters may need to have conflicting registrations. I can't personally think of a valid case where the same GVK in two different clusters (or maybe even different contexts within the same cluster?) would need a different go type associated with them, but it doesn't seem like we should preclude that possibility. |
It should probably be a hard limitation that the scheme for the time being has to be global. In terms of this PR, I'm ok allowing the Scheme to be shared, given that's one of those things that's already passed around |
The corresponding core was removed in #2300, so this isn't needed anymore. |
This keeps the inherited scheme object created by the cluster when combining the options instead of creating a new one.
That scheme object can be used later on to register schemes.
The inherited scheme was already configured in the cluster object, by creating a new scheme object
for the cache object when combining the schemes will not allow the schemes registered afterwards into the
cluster scheme object to be visible into the cache, and it will result in an error when the controller is started (see below).
Configure the controller as follows:
This fix will allow the controller to start properly instead of getting this error that the scheme is not registered:
You can see more details in kubernetes-sigs/security-profiles-operator#1543 where we encountered this issue.