Move CRD defaulting helpers outside of pkg/apis #138
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
julianKatz
force-pushed
the
threadsafe-toversionless
branch
from
4436e42 to
e55277f
Compare
Codecov Report
@@ Coverage Diff @@
## master #138 +/- ##
==========================================
- Coverage 42.39% 42.21% -0.19%
==========================================
Files 49 46 -3
Lines 2951 2966 +15
==========================================
+ Hits 1251 1252 +1
- Misses 1321 1330 +9
- Partials 379 384 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
julianKatz
force-pushed
the
threadsafe-toversionless
branch
from
e55277f to
e4875f5
Compare
willbeason
reviewed
Sep 24, 2021
willbeason
approved these changes
Sep 24, 2021
In the building of `gator` github.com/willbeason found that the usage of a shared Scheme amongst multiple sub-packages of pkg/apis/templates was not thread safe. An internal map within the Scheme was experiencing a simultaneous read and write. Instantiating the scheme in a super-package and using it in the sub-packages required the creation of a circular dependency, as the super-package needed the contents of the sub-package to create a functioning scheme that could convert between sub-package types. This was previously solved by adding sub-package contents to the scheme at scheme-usage-time, but this yielded the race condition on the map. This PR moves most of the core logic into a separate package: pkg/schema To prevent a circular dependency, a *runtime.Scheme is passed from consumer packages to this new package's function call. These schemes are instantiated once in the consumer packages and the output of pkg/schema is stored at consumer package init() time to prevent unnecessary re-calculation. Fixes open-policy-agent#137 Signed-off-by: juliankatz <[email protected]>
julianKatz
force-pushed
the
threadsafe-toversionless
branch
from
b087a0d to
a6f18c6
Compare
… threadsafe-toversionless Signed-off-by: juliankatz <[email protected]>
julianKatz
added a commit
to julianKatz/frameworks
that referenced
this pull request
Oct 5, 2021
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 ordering of their containing functions. 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. 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. Signed-off-by: juliankatz <[email protected]>
julianKatz
added a commit
to julianKatz/frameworks
that referenced
this pull request
Oct 5, 2021
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 <[email protected]>
julianKatz
added a commit
that referenced
this pull request
Oct 12, 2021
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 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 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the building of
gatorgithub.com/willbeason found that the usage ofa shared Scheme amongst multiple sub-packages of pkg/apis/templates was
not thread safe. An internal map within the Scheme was experiencing a
simultaneous read and write.
Instantiating the scheme in a super-package and using it in the
sub-packages required the creation of a circular dependency, as the
super-package needed the contents of the sub-package to create a
functioning scheme that could convert between sub-package types. This
was previously solved by adding sub-package contents to the scheme at
scheme-usage-time, but this yielded the race condition on the map.
This PR moves most of the core logic into a separate package: pkg/schema
To prevent a circular dependency, a *runtime.Scheme is passed from
consumer packages to this new package's function call.
These schemes are instantiated once in the consumer packages and the
output of pkg/schema is stored at consumer package init() time to
prevent unnecessary re-calculation.
Fixes #137
Signed-off-by: juliankatz [email protected]