🐛 Fix gap in APIExport virtual workspace queues #2720
Conversation
Fix a gap in the APIExport virtual workspace queueing where it was possible to incorrectly return a "not found" error when trying to list a claimed resource. The gap was that we weren't queueing the dependent APIExports; i.e., if APIExport A gets updated, and APIExport B has a permission claim on A's identity, we need to make sure to queue B in this situation. Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>
ncdc
force-pushed
the
fix-apiexport-vw-queue-gap
branch
from
aa661f2 to
c359931
Compare
|
/approve |
openshift-ci
bot
added
the
approved
| apiExport, ok := obj.(*apisv1alpha1.APIExport) | ||
| if !ok { | ||
| return []string{}, fmt.Errorf("obj %T is not an APIExport", obj) | ||
| } | ||
|
|
There was a problem hiding this comment.
Noticed we have these type checks present across all indexers -- are we considering dropping them everywhere?
There was a problem hiding this comment.
Could we also strictly type this stuff? Why obj interface{} ?
| @@ -131,11 +135,16 @@ func (c *APIReconciler) reconcile(ctx context.Context, apiExport *apisv1alpha1.A | |||
| continue | |||
| } | |||
|
|
|||
| logger = logger.WithValues("identity", pc.IdentityHash) | |||
|
|
|||
| logger.V(4).Info("getting APIExports by identity") | |||
There was a problem hiding this comment.
Is project policy on logging that v=4 is effectively "anything goes"? Just notice a lot of messages being propagated throughout here 👍🏻
There was a problem hiding this comment.
We can lower to 5 or 6 later, or remove them entirely, but this was critical in determining what was going on.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hasheddan, ncdc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Summary
Fix a gap in the APIExport virtual workspace queueing where it was possible to incorrectly return a "not found" error when trying to list a claimed resource. The gap was that we weren't queueing the dependent APIExports; i.e., if APIExport A gets updated, and APIExport B has a permission claim on A's identity, we need to make sure to queue B in this situation.
Related issue(s)
Fixes #2713