Fix bug where some ConfigMap and Secret references could be missed #4330
+290
−66
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
matthchr
requested review from
davefellows,
theunrepentantgeek,
babbageclunk and
super-harsh
as code owners
theunrepentantgeek
approved these changes
Oct 13, 2024
Comment on lines
+25
to
+27
| if reflect.PointerTo(rt).Implements(interfaceType) { | ||
| return gvk, nil | ||
| } |
There was a problem hiding this comment.
Why not use the existing NewEmptyVersionedResourceFromGVK() and then test using if _, ok := empty.(conversion.Hub); ok, avoiding the cryptic reflection code?
There was a problem hiding this comment.
Two reasons:
scheme.AllKnownTypesreturns amap[schema.GroupVersionKind]reflect.Type, so it seemed to make some sense to stay in the "reflect world" when scanning it if I don't actually need an instance.- Since this is called in the (somewhat) hot path of the core reconcile loop logic, and can be hit at least a couple times through each loop (because there may be multiple versions of a resource for a given group + version) I was a bit concerned about performance/extra allocations. It probably wouldn't matter but avoiding constructing 8-10 objects that are immediately thrown away when instead we can do a check via reflection that avoids those allocations.
I almost just computed a map of this upfront once and then we could look it up whenever we wanted but that seemed overkill so instead settled on using a less-allocations approach using this reflection method that kept things "simple" (no extra variables to storage cached data that needed to be passed around) while still avoiding excess allocations.
super-harsh
approved these changes
Oct 14, 2024
super-harsh
approved these changes
Oct 14, 2024
The ASO controller uses reflection to look through the resource and find the ConfigMapReference and SecretReference fields. This reflection could miss SecretReference and ConfigMapReference fields that had been serialized into the "PropertyBag" property on the storage type. This was most common when working with preview APIs that had added new ConfigMapReference or SecretReference fields that don't exist in the GA api version. The fix is to use the converted version for reflection-based discovery. Fixes Azure#4298. Fixes Azure#4316.
matthchr
force-pushed
the
feature/fix-pre-send-discovery
branch
from
f209def
to
654df66
Compare
|
/ok-to-test sha=654df66 |
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.
The ASO controller uses reflection to look through the resource and find the ConfigMapReference and SecretReference fields. This reflection could miss SecretReference and ConfigMapReference fields that had been serialized into the "PropertyBag" property on the storage type. This was most common when working with preview APIs that had added new ConfigMapReference or SecretReference fields that don't exist in the GA api version.
The fix is to use the converted version for reflection-based discovery.
Fixes #4298.
Fixes #4316.
If applicable: