clusterCache: lock namespacedResources, don't miss finding live obj if obj is cluster-scoped and namespacedResources is in transition #597
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
When Reconcile performs its logic to compare the desired state (target objects) against the actual state (live objects), it looks up each live object based on a key comprised of data from the target object: API group, API kind, namespace, and name. While group, kind, and name will always be accurate, there is a chance that the value for namespace is not. If a cluster-scoped target object has a namespace (because it incorrectly has a namespace from its source) or the namespace parameter passed into the Reconcile method has a non-empty value (indicating a default value to use on namespace-scoped objects that don't have it set in the source), AND the resInfo ResourceInfoProvider has incomplete or missing API discovery data, the call to IsNamespacedOrUnknown will return true when the information is unknown. This leads to the key being incorrect - it will have a value for namespace when it shouldn't. As a result, indexing into liveObjByKey will fail. This failure results in the reconciliation containing incorrect data: there will be a nil entry appended to targetObjs when there shouldn't be. Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>
namespacedResources is accessed by multiple goroutines simultaneously and therefore must be guarded by a mutex. Because this field is read significantly more often than it's written, use an RWMutex. Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>
ncdc
force-pushed
the
fix-incorrect-pruning
branch
from
fa96340
to
a7ddaf7
Compare
This was referenced Jul 1, 2024
|
@ncdc awesome work! have you posted this on the Argo CD slack or plan to represent it at one of the community/contributor meetings? |
|
For argoproj/argo-cd#15058, we have namespace on the Argo CD application because we deploy namespace-scoped resources as well so we definitely hit the condition. |
|
@rumstead yes, posted in
Not sure about my ability to attend a community meeting; will see. |
|
/cc @ash2k |
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.
namespacedResources is accessed by multiple goroutines simultaneously
and therefore must be guarded by a mutex. Because this field is read
significantly more often than it's written, use an RWMutex.
When Reconcile performs its logic to compare the desired state (target
objects) against the actual state (live objects), it looks up each live
object based on a key comprised of data from the target object: API
group, API kind, namespace, and name. While group, kind, and name will
always be accurate, there is a chance that the value for namespace is
not. If a cluster-scoped target object has a namespace (because it
incorrectly has a namespace from its source) or the namespace parameter
passed into the Reconcile method has a non-empty value (indicating a
default value to use on namespace-scoped objects that don't have it set
in the source), AND the resInfo ResourceInfoProvider has incomplete or
missing API discovery data, the call to IsNamespacedOrUnknown will
return true when the information is unknown. This leads to the key being
incorrect - it will have a value for namespace when it shouldn't. As a
result, indexing into liveObjByKey will fail. This failure results in
the reconciliation containing incorrect data: there will be a nil entry
appended to targetObjs when there shouldn't be.
Fixes #171
Most likely addresses argoproj/argo-cd#17440.
Most likely addresses at least 1 user who had cluster-scoped resources get wiped out in argoproj/argo-cd#15058