Check for empty ns list before checking nslist[0] #5236
Conversation
sseago
force-pushed
the
no-namespaces-fix
branch
from
a8f7b20
to
5ef9628
Compare
sseago
force-pushed
the
no-namespaces-fix
branch
from
5ef9628
to
337359b
Compare
| @@ -226,7 +226,7 @@ func (r *itemCollector) getResourceItems(log logrus.FieldLogger, gv schema.Group | |||
| namespacesToList := getNamespacesToList(r.backupRequest.NamespaceIncludesExcludes) | |||
|
|
|||
| // Check if we're backing up namespaces, and only certain ones | |||
| if gr == kuberesource.Namespaces && namespacesToList[0] != "" { | |||
| if gr == kuberesource.Namespaces && (len(namespacesToList) == 0 || namespacesToList[0] != "") { | |||
There was a problem hiding this comment.
Would it be more explicit if it's like gr == kuberesource.Namespaces && len(namespacesToList) > 0 && namespacesToList[0] != "" ?
There was a problem hiding this comment.
@reasonerjt No, that would introduce incorrect logic. This block should be executed when the resource is "Namespaces" and "we're not backing up the whole cluster". a slice value of [""] is what we're looking for to skip. This means "All namespaces", so we skip the block. If it's an empty slice, this means that the includes minus the excludes leaves us with zero namespaces, meaning this is a less-than-whole-cluster backup, so we run the block. If it's non-empty with values other than "", this is also a less-than-whole-cluster backup and we run the block -- that's why I have the embedded or clause. The alternative suggestion would have us skipping the block and going on where there are no namespaces, which could result in stuff being done that we don't want done here.
There was a problem hiding this comment.
Could you please update the comment in line 228, to explain the case when the first item is an empty string， the phrase only certain ones makes people think the the block after if should not be executed if the namespaceToList is empty.
In determining whether a backup includes all namespaces, item_collector checks for an empty string in the first element of the ns list. If processing includes+excludes results in an empty list, treat this as another case of a not-all-namespaces backup rather than crashing velero. Signed-off-by: Scott Seago <sseago@redhat.com>
sseago
force-pushed
the
no-namespaces-fix
branch
from
337359b
to
55bf2de
Compare
In determining whether a backup includes all namespaces, item_collector
checks for an empty string in the first element of the ns list. If processing
includes+excludes results in an empty list, treat this as another case
of a not-all-namespaces backup rather than crashing velero.
Signed-off-by: Scott Seago sseago@redhat.com
Thank you for contributing to Velero!
Please add a summary of your change
In determining whether a backup includes all namespaces, item_collector
checks for an empty string in the first element of the ns list. If processing
includes+excludes results in an empty list, treat this as another case
of a not-all-namespaces backup rather than crashing velero.
Does your change fix a particular issue?
Fixes #5235
Please indicate you've done the following:
/kind changelog-not-requiredas a comment on this pull request.site/content/docs/main.