diff --git a/changelogs/unreleased/10665-abd-ulbasit b/changelogs/unreleased/10665-abd-ulbasit new file mode 100644 index 000000000..de8de8a74 --- /dev/null +++ b/changelogs/unreleased/10665-abd-ulbasit @@ -0,0 +1 @@ +Fix restore resource priorities aliasing that can skip restoring resources diff --git a/pkg/restore/priority_aliasing_test.go b/pkg/restore/priority_aliasing_test.go new file mode 100644 index 000000000..e9335193c --- /dev/null +++ b/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") + } +} diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index b32300661..34bb703ad 100644 --- a/pkg/restore/restore.go +++ b/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...) } diff --git a/pkg/types/priority.go b/pkg/types/priority.go index 4ed03efa2..98e6d643f 100644 --- a/pkg/types/priority.go +++ b/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 diff --git a/pkg/types/priority_test.go b/pkg/types/priority_test.go index ad34afce1..eb99221aa 100644 --- a/pkg/types/priority_test.go +++ b/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()) + }) + } +}