Change controller-runtime List option from MatchingFields to ListOpti… #6958
Conversation
blackpiglet
force-pushed
the
5156_list_option_fix
branch
from
27631a9 to
d58761d
Compare
Codecov Report
@@ Coverage Diff @@
## main #6958 +/- ##
==========================================
+ Coverage 61.05% 61.06% +0.01%
==========================================
Files 251 251
Lines 26845 26855 +10
==========================================
+ Hits 16389 16399 +10
Misses 9303 9303
Partials 1153 1153
|
…ons. Signed-off-by: Xun Jiang <[email protected]>
blackpiglet
force-pushed
the
5156_list_option_fix
branch
from
6e4d6d5 to
79c7571
Compare
| @@ -419,7 +419,7 @@ func (s *nodeAgentServer) markDataDownloadsCancel(r *controller.DataDownloadReco | |||
|
|
|||
| func (s *nodeAgentServer) markInProgressPVBsFailed(client ctrlclient.Client) { | |||
| pvbs := &velerov1api.PodVolumeBackupList{} | |||
| if err := client.List(s.ctx, pvbs, &ctrlclient.MatchingFields{"metadata.namespace": s.namespace}); err != nil { | |||
| if err := client.List(s.ctx, pvbs, &ctrlclient.ListOptions{Namespace: s.namespace}); err != nil { | |||
There was a problem hiding this comment.
Why did you think , use ctrlclient.ListOptions{ instead of ctrlclient.MatchingFields{ can do anything better?
Like the docs note client#hdr-Indexing , ctrlclient.MatchingFields will better than ctrlclient.ListOptions{ when it is used in index condition.
There was a problem hiding this comment.
This is used to address the error reported in the Velero with a limited permission scenario.
time="2023-10-09T23:56:44Z" level=error msg="failed to list backups" error="backups.velero.io is forbidden: User \"system:serviceaccount:arthur:velero-server\
" cannot list resource \"backups\" in API group \"velero.io\" at the cluster scope" error.file="/go/src/github.com/vmware-tanzu/velero/pkg/cmd/server/server.g
o:954" error.function=github.com/vmware-tanzu/velero/pkg/cmd/server.markInProgressBackupsFailed logSource="pkg/cmd/server/server.go:954"
I suppose this error is reported, instead of reading from cached, is due to the namespaced cache was not created before use.
https://github.com/kubernetes-sigs/controller-runtime/blob/15d79283523517f52ee30b28d2ae4c1ea3f113da/pkg/cache/cache_test.go#L1928-L1956
There was a problem hiding this comment.
is forbidden: User \"system:serviceaccount:arthur:velero-server , I'm afraid cache have or have not , may not the error's root cause.😀😀
There was a problem hiding this comment.
It's related to the limited permission scenario.
Because the query parameter already has the namespace, I didn't expect the query to be in the cluster scope.
I'm not sure that MatchingFields will definitely query resources in the whole cluster or query according to the parameter setting.
ListOptions doesn't have the error-reported issue.
…ons.
Thank you for contributing to Velero!
Please add a summary of your change
Does your change fix a particular issue?
Fixes #5156
Please indicate you've done the following:
/kind changelog-not-requiredas a comment on this pull request.site/content/docs/main.