Fix restore resource priorities aliasing that can skip restoring resources (#10665)

* Fix restore resource priorities aliasing that can skip restoring resources

Priorities.Set returned HighPriorities and LowPriorities as slices of
one backing array when the value had a "-" in the middle, leaving
HighPriorities with spare capacity over the separator and the low list.
getOrderedResources appended to HighPriorities, so a backup with between
2 and 1+len(low) unprioritized resource types overwrote the low list in
place. That could drop a backed-up resource type from the restore order,
and because the server's priorities are shared by every restore, the
corrupted order persisted for later restores.

- Set: cap HighPriorities with full slice expressions.
- getOrderedResources: build a new slice instead of appending to the
  caller's.
- String: build a fresh slice instead of appending to HighPriorities.

Signed-off-by: abd-ulbasit <basit@basit.engineer>

* Add changelog for #10665

Signed-off-by: abd-ulbasit <basit@basit.engineer>

---------

Signed-off-by: abd-ulbasit <basit@basit.engineer>

changelogs/unreleased/10665-abd-ulbasit

@@ -0,0 +1 @@
+Fix restore resource priorities aliasing that can skip restoring resources

pkg/restore/priority_aliasing_test.go

@@ -0,0 +1,119 @@
+/*
+Copyright the Velero contributors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package restore
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/velero-io/velero/pkg/archive"
+	"github.com/velero-io/velero/pkg/types"
+)
+
+const (
+	mutatingWebhooks   = "mutatingwebhookconfigurations.admissionregistration.k8s.io"
+	validatingWebhooks = "validatingwebhookconfigurations.admissionregistration.k8s.io"
+)
+
+// flagPriorities parses priorities the way the --restore-resource-priorities
+// server flag does. With a "-" in the middle, Set slices HighPriorities and
+// LowPriorities from one backing array, so HighPriorities has spare capacity.
+func flagPriorities(t *testing.T) types.Priorities {
+	t.Helper()
+	var p types.Priorities
+	require.NoError(t, p.Set("namespaces,pods,-,"+mutatingWebhooks+","+validatingWebhooks))
+	return p
+}
+
+func backupWith(resources ...string) map[string]*archive.ResourceItems {
+	m := make(map[string]*archive.ResourceItems, len(resources))
+	for _, r := range resources {
+		m[r] = &archive.ResourceItems{}
+	}
+	return m
+}
+
+// getOrderedResources must not write into the caller's priority slices,
+// whatever the number of unprioritized resources in the backup.
+func TestGetOrderedResourcesDoesNotMutateFlagPriorities(t *testing.T) {
+	tests := []struct {
+		name   string
+		backup map[string]*archive.ResourceItems
+		want   []string
+	}{
+		{
+			name:   "one unprioritized resource",
+			backup: backupWith("configmaps", "pods"),
+			want:   []string{"namespaces", "pods", "configmaps", mutatingWebhooks, validatingWebhooks},
+		},
+		{
+			name:   "two unprioritized resources",
+			backup: backupWith("configmaps", "deployments.apps", "pods"),
+			want:   []string{"namespaces", "pods", "configmaps", "deployments.apps", mutatingWebhooks, validatingWebhooks},
+		},
+		{
+			name:   "three unprioritized resources",
+			backup: backupWith("configmaps", "deployments.apps", "secrets"),
+			want:   []string{"namespaces", "pods", "configmaps", "deployments.apps", "secrets", mutatingWebhooks, validatingWebhooks},
+		},
+		{
+			name:   "four unprioritized resources",
+			backup: backupWith("configmaps", "deployments.apps", "secrets", "services"),
+			want:   []string{"namespaces", "pods", "configmaps", "deployments.apps", "secrets", "services", mutatingWebhooks, validatingWebhooks},
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			p := flagPriorities(t)
+			wantHigh := append([]string(nil), p.HighPriorities...)
+			wantLow := append([]string(nil), p.LowPriorities...)
+
+			assert.Equal(t, tc.want, getOrderedResources(p, tc.backup))
+			assert.Equal(t, wantHigh, p.HighPriorities, "HighPriorities changed")
+			assert.Equal(t, wantLow, p.LowPriorities, "LowPriorities changed")
+		})
+	}
+}
+
+// The restorer copies the server's priorities into every restore by value
+// (restore.go: resourcePriorities: kr.resourcePriorities), so the slices are
+// shared across restores. A later restore must still see the configured order.
+func TestGetOrderedResourcesRepeatedRestoresKeepServerOrder(t *testing.T) {
+	serverPriorities := flagPriorities(t)
+
+	first := getOrderedResources(serverPriorities, backupWith("configmaps", "deployments.apps", "pods"))
+	assert.Equal(t, []string{"namespaces", "pods", "configmaps", "deployments.apps", mutatingWebhooks, validatingWebhooks}, first)
+
+	second := getOrderedResources(serverPriorities, backupWith("configmaps", mutatingWebhooks, "secrets"))
+	assert.Equal(t, []string{"namespaces", "pods", "configmaps", "secrets", mutatingWebhooks, validatingWebhooks}, second,
+		"mutating webhooks must stay low priority (restored after the workloads they guard)")
+}
+
+// Every resource type in the backup must appear in the restore order. The
+// "restore everything else" pass walks this list once, so a type missing from
+// it is silently not restored.
+func TestGetOrderedResourcesKeepsEveryBackedUpResource(t *testing.T) {
+	p := flagPriorities(t)
+	backup := backupWith("configmaps", "deployments.apps", mutatingWebhooks)
+
+	got := getOrderedResources(p, backup)
+	for resource := range backup {
+		assert.Contains(t, got, resource, "backed-up resource missing from restore order")
+	}
+}

pkg/restore/restore.go

@@ -706,7 +706,11 @@ func getOrderedResources(resourcePriorities types.Priorities, backupResources ma
 	// alphabetize resources in the backup
 	sort.Strings(orderedBackupResources)
 
-	list := append(resourcePriorities.HighPriorities, orderedBackupResources...)
+	// Build a new slice: appending to HighPriorities can write into a backing
+	// array it shares with LowPriorities (see types.Priorities.Set).
+	list := make([]string, 0, len(resourcePriorities.HighPriorities)+len(orderedBackupResources)+len(resourcePriorities.LowPriorities))
+	list = append(list, resourcePriorities.HighPriorities...)
+	list = append(list, orderedBackupResources...)
 	return append(list, resourcePriorities.LowPriorities...)
 }
 

pkg/types/priority.go

@@ -36,7 +36,8 @@ type Priorities struct {
 
 // String returns a string representation of Priority.
 func (p *Priorities) String() string {
-	priorities := p.HighPriorities
+	priorities := make([]string, 0, len(p.HighPriorities)+1+len(p.LowPriorities))
+	priorities = append(priorities, p.HighPriorities...)
 	if len(p.LowPriorities) > 0 {
 		priorities = append(priorities, prioritySeparator)
 		priorities = append(priorities, p.LowPriorities...)
@@ -75,12 +76,14 @@ func (p *Priorities) Set(s string) error {
 	}
 	// end with separator
 	if separatorIndex == len(strs)-1 {
-		p.HighPriorities = strs[:len(strs)-1]
+		p.HighPriorities = strs[: len(strs)-1 : len(strs)-1]
 		return nil
 	}
 
-	// separator in the middle
-	p.HighPriorities = strs[:separatorIndex]
+	// separator in the middle. Cap HighPriorities at its length so that
+	// appending to it cannot overwrite the separator or LowPriorities, which
+	// share the same backing array.
+	p.HighPriorities = strs[:separatorIndex:separatorIndex]
 	p.LowPriorities = strs[separatorIndex+1:]
 
 	return nil

pkg/types/priority_test.go

@@ -108,3 +108,18 @@ func TestSetOfPriority(t *testing.T) {
 		})
 	}
 }
+
+func TestSetDoesNotLeaveSpareCapacity(t *testing.T) {
+	for input, wantString := range map[string]string{"p0,p1,-,p8,p9": "p0,p1,-,p8,p9", "p0,-": "p0"} {
+		t.Run(input, func(t *testing.T) {
+			p := Priorities{}
+			require.NoError(t, p.Set(input))
+			assert.Equal(t, len(p.HighPriorities), cap(p.HighPriorities))
+
+			low := append([]string(nil), p.LowPriorities...)
+			_ = append(p.HighPriorities, "x", "y", "z")
+			assert.Equal(t, low, p.LowPriorities, "appending to HighPriorities changed LowPriorities")
+			assert.Equal(t, wantString, p.String())
+		})
+	}
+}