-
Notifications
You must be signed in to change notification settings - Fork 51
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
Fix scheme registration in pkg/apis/templates #142
Fix scheme registration in pkg/apis/templates #142
Conversation
PR open-policy-agent#138 introduced a bug: the type conversion functions required by the Scheme for use in the ToVersionless functions of the v1, v1beta1, v1alpha1 packages were missing. This manifested itself as an error in Gatekeeper: "unable to convert template: converting (v1beta1.ConstraintTemplate) to (templates.ConstraintTemplate): unknown conversion" This problem was due to the ordering of init() functions. init functions are called according to the lexicographic order of their containing files. zz_generated.conversion.go registers the conversion functions with the scheme, but did so after the init() function previously held in defaults.go due to the aforementioned ordering. This left the Scheme used in ToVersionless without the conversion functions it was expected to have. Rather than hack this ordering to ensure a correctly populated `localSchemeBuilder`, this PR makes a schemeBuilder with the explicit purpose of using it in the Scheme that's used in ToVersionless. This decouples the scheme from the ordering of init() funcs, resolving the issue. This PR also adds unit tests for each of the ToVersionless funcs, ensuring such a problem won't happen again. Signed-off-by: juliankatz <juliankatz@google.com>
Codecov Report
@@ Coverage Diff @@
## master #142 +/- ##
==========================================
+ Coverage 41.51% 42.02% +0.50%
==========================================
Files 52 55 +3
Lines 3122 3127 +5
==========================================
+ Hits 1296 1314 +18
+ Misses 1435 1419 -16
- Partials 391 394 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
Thank you! LGTM
// registered with the localSchemeBuilder by the time this init() function | ||
// runs. We sidestep this problem by adding RegisterConversions here. | ||
schemeBuilder := runtime.NewSchemeBuilder(localSchemeBuilder...) | ||
schemeBuilder.Register(RegisterConversions) |
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.
We should create a scheme builder that explicitly registers all necessary registrations. Otherwise this function will still be vulnerable to failures due to reordering that changes the contents of localSchemeBuilder
Also, we can move this to the helpers.go
package, since this is the only thing that should depend on the sch
scheme.
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.
Define the schemeBuilder outside of a function as a global var to avoid ordering concerns:
var schemeBuilder = runtime.NewSchemeBuilder(fn1, fn2, fn3, ...)
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.
Also, we can move this to the
helpers.go
package, since this is the only thing that should depend on thesch
scheme.
The defaults.go
file also depends on the Scheme, as the scheme is used when creating the structuralSchema
. I thought the separate file would prevent the assumption that the scheme only belongs to one thing.
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.
Define the schemeBuilder outside of a function as a global var to avoid ordering concerns:
var schemeBuilder = runtime.NewSchemeBuilder(fn1, fn2, fn3, ...)
This actually re-introduces ordering problems. One of the funcs that's included in the schemeBuilder
(SchemeBuilder.AddToScheme
) is declared as a package level variable in register.go
: https://github.com/open-policy-agent/frameworks/blob/master/constraint/pkg/apis/templates/v1/register.go#L39
As @maxsmythe suggests, not using localSchemeBuilder
's contents directly has a possible benefit. The difference between what I have here and writing out SchemeBuilder.AddToScheme, addDefaultingFuncs
is that if localSchemeBuilder
were to change in the future, we would not see those changes in the separate schemebuilder I'm making. That could be good or bad, depending on the scenario.
The key idea here is to take advantage of this ordering:
- All package variables instantiated
- All package
init()
functions run
Basically I can reliably use any package variable, but I can't rely on other init()
funcs. To truly not rely on other init()
functions, I'll actually need to duplicate this line from constrainttemplate_types.go in my own schemebuilder.
I see two problems that we're choosing between:
- Relying on
init()
function ordering - maintaining a separate list of items that should be registered to the scheme from the one that external packages will receive when using our package's external
AddToScheme
function.
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.
My vote is for option 2 since it is explicit and doesn't rely on magic.
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.
It's okay to have our own package-internal scheme,
var toVersionlessSchemeBuilder = runtime.NewSchemeBuilder(...)
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.
The defaults.go file also depends on the Scheme, as the scheme is used when creating the structuralSchema. I thought the separate file would prevent the assumption that the scheme only belongs to one thing.
ack, SGTM
is that if localSchemeBuilder were to change in the future, we would not see those changes in the separate schemebuilder I'm making. That could be good or bad, depending on the scenario.
Fortunately we don't need all changes to the schema builder, just those features that we rely on (defaulting and conversion it looks like).
Also note that we are coupled to the init() functions if we are using anything whose contents may change because of the init functions (such as localSchemeBuilder
).
+1 to option 2
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.
It's okay to have our own package-internal scheme,
var toVersionlessSchemeBuilder = runtime.NewSchemeBuilder(...)
I'd like to avoid doing this as a package-level variable as I step back towards ordering problems. Putting all of the logic inside an init()
function that is decoupled from other init()
functions allows me to take advantage of other package level variables while still isolating my schemeBuilder from side-effects.
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.
LGTM after changes
… fix-versionless-schema Signed-off-by: juliankatz <juliankatz@google.com>
Signed-off-by: juliankatz <juliankatz@google.com>
5dcb771
to
65407b5
Compare
PR #138 introduced a bug: the type conversion functions required by the
Scheme for use in the ToVersionless functions of the v1, v1beta1,
v1alpha1 packages were missing. This manifested itself as an error in
Gatekeeper:
"unable to convert template: converting (v1beta1.ConstraintTemplate) to
(templates.ConstraintTemplate): unknown conversion"
This problem was due to the ordering of init() functions. init
functions are called according to the lexicographic order of their
containing files. zz_generated.conversion.go registers the conversion
functions with the scheme, but did so after the init() function
previously held in defaults.go due to the aforementioned ordering. This
left the Scheme used in ToVersionless without the conversion functions
it was expected to have.
Rather than hack this ordering to ensure a correctly populated
localSchemeBuilder
, this PR makes a schemeBuilder with the explicitpurpose of using it in the Scheme that's used in ToVersionless. This
decouples the scheme from the ordering of init() funcs, resolving the
issue.
This PR also adds unit tests for each of the ToVersionless funcs,
ensuring such a problem won't happen again.
Signed-off-by: juliankatz juliankatz@google.com