Skip to content
This repository was archived by the owner on Mar 16, 2024. It is now read-only.

Commit e7fb158

Browse files
thedadamsthedadams
authored
Fix app and project region validation (#1542) (#1581)
* Fix app and project region validation Previously, when a user removed a supported region from a project, the apps in that region would be in various states. After these changes, a user will be required to ensure that there are no apps running in a region before removing that region as a supported region for the project. Additionally, if a user tried to change the region of a running app, then the CLI command would silently ignore this. After these changes, an error will be produced. Signed-off-by: Donnie Adams <[email protected]> --------- Signed-off-by: Donnie Adams <[email protected]>
1 parent eab33d4 commit e7fb158

File tree

8 files changed

+223
-14
lines changed

8 files changed

+223
-14
lines changed

‎pkg/apis/api.acorn.io/v1/types.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -460,7 +460,10 @@ func (in *Project) NamespaceScoped() bool {
460460
}
461461

462462
func (in *Project) HasRegion(region string) bool {
463-
return region == "" || in.Status.DefaultRegion == region || slices.Contains(in.Spec.SupportedRegions, region)
463+
if region == "" || slices.Contains(in.Spec.SupportedRegions, region) {
464+
return true
465+
}
466+
return in.Spec.DefaultRegion == "" && in.Status.DefaultRegion == region
464467
}
465468

466469
func (in *Project) GetRegion() string {

‎pkg/apis/internal.acorn.io/v1/appinstance.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,9 @@ func (in *AppInstance) GetRegion() string {
5959

6060
func (in *AppInstance) SetDefaultRegion(region string) {
6161
if in.Spec.Region == "" {
62-
in.Status.Defaults.Region = region
62+
if in.Status.Defaults.Region == "" {
63+
in.Status.Defaults.Region = region
64+
}
6365
} else {
6466
in.Status.Defaults.Region = ""
6567
}

‎pkg/client/app.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,9 @@ func ToAppUpdate(ctx context.Context, c Client, name string, opts *AppUpdateOpti
146146
if len(opts.ComputeClasses) != 0 {
147147
app.Spec.ComputeClasses = opts.ComputeClasses
148148
}
149+
if opts.Region != "" {
150+
app.Spec.Region = opts.Region
151+
}
149152

150153
return app, nil
151154
}

‎pkg/client/client.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ type AppUpdateOptions struct {
9898
AutoUpgradeInterval string
9999
Memory v1.MemoryMap
100100
ComputeClasses v1.ComputeClassMap
101+
Region string
101102
}
102103

103104
type LogOptions apiv1.LogOptions
@@ -145,6 +146,7 @@ func (a AppRunOptions) ToUpdate() AppUpdateOptions {
145146
AutoUpgradeInterval: a.AutoUpgradeInterval,
146147
Memory: a.Memory,
147148
ComputeClasses: a.ComputeClasses,
149+
Region: a.Region,
148150
}
149151
}
150152

‎pkg/install/namespace.yaml‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,5 +17,4 @@ metadata:
1717
labels:
1818
acorn.io/project: "true"
1919
annotations:
20-
acorn.io/project-default-region: "local"
21-
acorn.io/project-supported-regions: "local"
20+
acorn.io/calculated-project-default-region: "local"

‎pkg/server/registry/apigroups/acorn/projects/storage.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,8 @@ func NewStorage(c kclient.WithWatch) rest.Storage {
2121
updater: remoteResource,
2222
deleter: remoteResource,
2323
}
24-
validator := &Validator{apiv1.LocalRegion}
24+
25+
validator := &Validator{DefaultRegion: apiv1.LocalRegion, Client: c}
2526
return stores.NewBuilder(c.Scheme(), &apiv1.Project{}).
2627
WithCreate(strategy).
2728
WithUpdate(strategy).

‎pkg/server/registry/apigroups/acorn/projects/validator.go‎

Lines changed: 48 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,17 +2,22 @@ package projects
22

33
import (
44
"context"
5+
"fmt"
6+
"strings"
57

68
apiv1 "github.com/acorn-io/acorn/pkg/apis/api.acorn.io/v1"
79
"k8s.io/apimachinery/pkg/runtime"
810
"k8s.io/apimachinery/pkg/util/validation/field"
11+
"k8s.io/utils/strings/slices"
12+
kclient "sigs.k8s.io/controller-runtime/pkg/client"
913
)
1014

1115
type Validator struct {
1216
DefaultRegion string
17+
Client kclient.Client
1318
}
1419

15-
func (v *Validator) Validate(ctx context.Context, obj runtime.Object) field.ErrorList {
20+
func (v *Validator) Validate(_ context.Context, obj runtime.Object) field.ErrorList {
1621
var result field.ErrorList
1722
project := obj.(*apiv1.Project)
1823
if project.Spec.DefaultRegion == "" && len(project.Spec.SupportedRegions) == 0 {
@@ -31,6 +36,46 @@ func (v *Validator) Validate(ctx context.Context, obj runtime.Object) field.Erro
3136
return nil
3237
}
3338

34-
func (v *Validator) ValidateUpdate(ctx context.Context, obj, _ runtime.Object) field.ErrorList {
35-
return v.Validate(ctx, obj)
39+
func (v *Validator) ValidateUpdate(ctx context.Context, new, old runtime.Object) field.ErrorList {
40+
// Ensure that default region and supported regions are valid.
41+
if err := v.Validate(ctx, new); err != nil {
42+
return err
43+
}
44+
45+
// If the user is removing a supported region, ensure that there are no apps in that region.
46+
oldProject, newProject := old.(*apiv1.Project), new.(*apiv1.Project)
47+
var removedRegions []string
48+
for _, region := range append(oldProject.Spec.SupportedRegions, oldProject.Status.DefaultRegion) {
49+
if !newProject.HasRegion(region) {
50+
removedRegions = append(removedRegions, region)
51+
}
52+
}
53+
54+
if len(removedRegions) > 0 {
55+
var (
56+
appList apiv1.AppList
57+
appsInRemovedRegions []string
58+
)
59+
if err := v.Client.List(ctx, &appList, kclient.InNamespace(newProject.Name)); err != nil {
60+
return field.ErrorList{field.InternalError(field.NewPath("spec", "supportedRegions"), err)}
61+
}
62+
63+
for _, app := range appList.Items {
64+
if slices.Contains(removedRegions, app.GetRegion()) {
65+
appsInRemovedRegions = append(appsInRemovedRegions, app.Name)
66+
}
67+
}
68+
69+
if len(appsInRemovedRegions) > 0 {
70+
return field.ErrorList{
71+
field.Invalid(
72+
field.NewPath("spec", "supportedRegions"),
73+
newProject.GetSupportedRegions(),
74+
fmt.Sprintf("cannot remove regions %v that have apps: %v", removedRegions, strings.Join(appsInRemovedRegions, ", ")),
75+
),
76+
}
77+
}
78+
}
79+
80+
return nil
3681
}

‎pkg/server/registry/apigroups/acorn/projects/validator_test.go‎

Lines changed: 160 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,16 @@ import (
55
"testing"
66

77
apiv1 "github.com/acorn-io/acorn/pkg/apis/api.acorn.io/v1"
8+
v1 "github.com/acorn-io/acorn/pkg/apis/internal.acorn.io/v1"
9+
"github.com/acorn-io/acorn/pkg/scheme"
810
"github.com/stretchr/testify/assert"
11+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
12+
kclient "sigs.k8s.io/controller-runtime/pkg/client"
13+
"sigs.k8s.io/controller-runtime/pkg/client/fake"
914
)
1015

1116
func TestProjectCreateValidation(t *testing.T) {
12-
validator := &Validator{apiv1.LocalRegion}
17+
validator := &Validator{DefaultRegion: apiv1.LocalRegion}
1318

1419
tests := []struct {
1520
name string
@@ -82,11 +87,12 @@ func TestProjectCreateValidation(t *testing.T) {
8287
}
8388

8489
func TestProjectUpdateValidation(t *testing.T) {
85-
validator := &Validator{apiv1.LocalRegion}
90+
validator := &Validator{DefaultRegion: apiv1.LocalRegion}
8691
tests := []struct {
87-
name string
88-
newProject apiv1.Project
89-
wantError bool
92+
name string
93+
newProject, oldProject apiv1.Project
94+
client kclient.Client
95+
wantError bool
9096
}{
9197
{
9298
name: "Update project to have default region, no supported regions",
@@ -99,6 +105,11 @@ func TestProjectUpdateValidation(t *testing.T) {
99105
},
100106
{
101107
name: "Update project to have default region and supported region",
108+
oldProject: apiv1.Project{
109+
Status: apiv1.ProjectStatus{
110+
DefaultRegion: "my-region",
111+
},
112+
},
102113
newProject: apiv1.Project{
103114
Spec: apiv1.ProjectSpec{
104115
DefaultRegion: "my-region",
@@ -108,6 +119,11 @@ func TestProjectUpdateValidation(t *testing.T) {
108119
},
109120
{
110121
name: "Update project to have default region and non-existent supported regions",
122+
oldProject: apiv1.Project{
123+
Status: apiv1.ProjectStatus{
124+
DefaultRegion: "my-region",
125+
},
126+
},
111127
newProject: apiv1.Project{
112128
Spec: apiv1.ProjectSpec{
113129
DefaultRegion: "my-region",
@@ -125,11 +141,149 @@ func TestProjectUpdateValidation(t *testing.T) {
125141
},
126142
},
127143
},
144+
{
145+
name: "Update project remove a supported region, no apps",
146+
oldProject: apiv1.Project{
147+
Spec: apiv1.ProjectSpec{
148+
DefaultRegion: "my-region",
149+
SupportedRegions: []string{"my-region", "my-other-region"},
150+
},
151+
},
152+
newProject: apiv1.Project{
153+
Spec: apiv1.ProjectSpec{
154+
DefaultRegion: "my-region",
155+
SupportedRegions: []string{"my-region"},
156+
},
157+
},
158+
client: fake.NewClientBuilder().WithScheme(scheme.Scheme).Build(),
159+
},
160+
{
161+
name: "Update project remove a supported region, no apps in project",
162+
oldProject: apiv1.Project{
163+
Spec: apiv1.ProjectSpec{
164+
DefaultRegion: "my-region",
165+
SupportedRegions: []string{"my-region", "my-other-region"},
166+
},
167+
},
168+
newProject: apiv1.Project{
169+
Spec: apiv1.ProjectSpec{
170+
DefaultRegion: "my-region",
171+
SupportedRegions: []string{"my-region"},
172+
},
173+
},
174+
client: fake.NewClientBuilder().WithScheme(scheme.Scheme).WithLists(
175+
&apiv1.AppList{
176+
Items: []apiv1.App{
177+
{
178+
ObjectMeta: metav1.ObjectMeta{
179+
Name: "my-app",
180+
Namespace: "other-project",
181+
},
182+
Spec: v1.AppInstanceSpec{
183+
Region: "my-region",
184+
},
185+
},
186+
},
187+
},
188+
).Build(),
189+
},
190+
{
191+
name: "Update project remove a supported region, no apps in removed region",
192+
oldProject: apiv1.Project{
193+
Spec: apiv1.ProjectSpec{
194+
DefaultRegion: "my-region",
195+
SupportedRegions: []string{"my-region", "my-other-region"},
196+
},
197+
},
198+
newProject: apiv1.Project{
199+
Spec: apiv1.ProjectSpec{
200+
DefaultRegion: "my-region",
201+
SupportedRegions: []string{"my-region"},
202+
},
203+
},
204+
client: fake.NewClientBuilder().WithScheme(scheme.Scheme).WithLists(
205+
&apiv1.AppList{
206+
Items: []apiv1.App{
207+
{
208+
ObjectMeta: metav1.ObjectMeta{
209+
Name: "my-app",
210+
},
211+
Spec: v1.AppInstanceSpec{
212+
Region: "my-region",
213+
},
214+
},
215+
},
216+
},
217+
).Build(),
218+
},
219+
{
220+
name: "Update project remove a supported region with apps in removed region",
221+
wantError: true,
222+
oldProject: apiv1.Project{
223+
Spec: apiv1.ProjectSpec{
224+
DefaultRegion: "my-region",
225+
SupportedRegions: []string{"my-region", "my-other-region"},
226+
},
227+
},
228+
newProject: apiv1.Project{
229+
Spec: apiv1.ProjectSpec{
230+
DefaultRegion: "my-region",
231+
SupportedRegions: []string{"my-region"},
232+
},
233+
},
234+
client: fake.NewClientBuilder().WithScheme(scheme.Scheme).WithLists(
235+
&apiv1.AppList{
236+
Items: []apiv1.App{
237+
{
238+
ObjectMeta: metav1.ObjectMeta{
239+
Name: "my-app",
240+
},
241+
Spec: v1.AppInstanceSpec{
242+
Region: "my-other-region",
243+
},
244+
},
245+
},
246+
},
247+
).Build(),
248+
},
249+
{
250+
name: "Update project remove a supported region with apps defaulted to removed region",
251+
wantError: true,
252+
oldProject: apiv1.Project{
253+
Spec: apiv1.ProjectSpec{
254+
DefaultRegion: "my-region",
255+
SupportedRegions: []string{"my-region", "my-other-region"},
256+
},
257+
},
258+
newProject: apiv1.Project{
259+
Spec: apiv1.ProjectSpec{
260+
DefaultRegion: "my-region",
261+
SupportedRegions: []string{"my-region"},
262+
},
263+
},
264+
client: fake.NewClientBuilder().WithScheme(scheme.Scheme).WithLists(
265+
&apiv1.AppList{
266+
Items: []apiv1.App{
267+
{
268+
ObjectMeta: metav1.ObjectMeta{
269+
Name: "my-app",
270+
},
271+
Status: v1.AppInstanceStatus{
272+
Defaults: v1.Defaults{
273+
Region: "my-other-region",
274+
},
275+
},
276+
},
277+
},
278+
},
279+
).Build(),
280+
},
128281
}
129282

130283
for _, tt := range tests {
131284
t.Run(tt.name, func(t *testing.T) {
132-
err := validator.ValidateUpdate(context.Background(), &tt.newProject, nil)
285+
validator.Client = tt.client
286+
err := validator.ValidateUpdate(context.Background(), &tt.newProject, &tt.oldProject)
133287
if !tt.wantError {
134288
if err != nil {
135289
t.Fatal(err)

0 commit comments

Comments
 (0)