Skip to content

Commit 6c467af

Browse files
camilamacedo86camilamacedo86
committed
(fix): unhandle changes for crd upgrade safety
- Skip diffs under ^.status during upgrade checks - Keep unhandled spec changes as errors; message: "unhandled changes found" - Add Tests: allow status-only changes; ensure unhandled spec changes fail with concise message Assisted-by: Cursor
1 parent c56a811 commit 6c467af

File tree

6 files changed

+236
-4
lines changed

6 files changed

+236
-4
lines changed

‎internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go‎

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -108,8 +108,9 @@ func (p *Preflight) runPreflight(ctx context.Context, rel *release.Release) erro
108108
resultErrs := crdWideErrors(results)
109109
resultErrs = append(resultErrs, sameVersionErrors(results)...)
110110
resultErrs = append(resultErrs, servedVersionErrors(results)...)
111-
112-
validateErrors = append(validateErrors, fmt.Errorf("validating upgrade for CRD %q: %w", newCrd.Name, errors.Join(resultErrs...)))
111+
if len(resultErrs) > 0 {
112+
validateErrors = append(validateErrors, fmt.Errorf("validating upgrade for CRD %q: %w", newCrd.Name, errors.Join(resultErrs...)))
113+
}
113114
}
114115
}
115116

@@ -169,9 +170,16 @@ func sameVersionErrors(results *runner.Results) []error {
169170
errs := []error{}
170171
for version, propertyResults := range results.SameVersionValidation {
171172
for property, comparisonResults := range propertyResults {
173+
if strings.HasPrefix(property, "^.status") {
174+
continue
175+
}
172176
for _, result := range comparisonResults {
173177
for _, err := range result.Errors {
174-
errs = append(errs, fmt.Errorf("%s: %s: %s: %s", version, property, result.Name, err))
178+
msg := err
179+
if result.Name == "unhandled" {
180+
msg = "unhandled changes found"
181+
}
182+
errs = append(errs, fmt.Errorf("%s: %s: %s: %s", version, property, result.Name, msg))
175183
}
176184
}
177185
}
@@ -188,9 +196,16 @@ func servedVersionErrors(results *runner.Results) []error {
188196
errs := []error{}
189197
for version, propertyResults := range results.ServedVersionValidation {
190198
for property, comparisonResults := range propertyResults {
199+
if strings.HasPrefix(property, "^.status") {
200+
continue
201+
}
191202
for _, result := range comparisonResults {
192203
for _, err := range result.Errors {
193-
errs = append(errs, fmt.Errorf("%s: %s: %s: %s", version, property, result.Name, err))
204+
msg := err
205+
if result.Name == "unhandled" {
206+
msg = "unhandled changes found"
207+
}
208+
errs = append(errs, fmt.Errorf("%s: %s: %s: %s", version, property, result.Name, msg))
194209
}
195210
}
196211
}

‎internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1616
"k8s.io/apimachinery/pkg/runtime"
1717
"k8s.io/apimachinery/pkg/runtime/schema"
18+
crdifyconfig "sigs.k8s.io/crdify/pkg/config"
1819

1920
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/preflights/crdupgradesafety"
2021
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/util"
@@ -379,3 +380,49 @@ func TestUpgrade(t *testing.T) {
379380
})
380381
}
381382
}
383+
384+
func TestUpgrade_StatusChanges_Ignored(t *testing.T) {
385+
t.Run("status-only changes are ignored", func(t *testing.T) {
386+
preflight := newMockPreflight(getCrdFromManifestFile(t, "crd-status-change-old.json"), nil)
387+
rel := &release.Release{
388+
Name: "test-release",
389+
Manifest: getManifestString(t, "crd-status-change-new.json"),
390+
}
391+
err := preflight.Upgrade(context.Background(), rel)
392+
require.NoError(t, err)
393+
})
394+
}
395+
396+
func TestUpgrade_UnhandledChanges_InSpec_DefaultPolicy(t *testing.T) {
397+
t.Run("unhandled spec changes cause error by default", func(t *testing.T) {
398+
preflight := newMockPreflight(getCrdFromManifestFile(t, "crd-unhandled-old.json"), nil)
399+
rel := &release.Release{
400+
Name: "test-release",
401+
Manifest: getManifestString(t, "crd-unhandled-new.json"),
402+
}
403+
err := preflight.Upgrade(context.Background(), rel)
404+
require.Error(t, err)
405+
require.ErrorContains(t, err, "unhandled changes found")
406+
require.NotContains(t, err.Error(), ".status.", "status changes should be ignored")
407+
require.NotContains(t, err.Error(), "v1.JSONSchemaProps", "error message should be concise without raw diff")
408+
})
409+
}
410+
411+
func TestUpgrade_UnhandledChanges_PolicyError(t *testing.T) {
412+
t.Run("unhandled changes error when policy is Error", func(t *testing.T) {
413+
oldCrd := getCrdFromManifestFile(t, "crd-unhandled-old.json")
414+
preflight := crdupgradesafety.NewPreflight(&MockCRDGetter{oldCrd: oldCrd}, crdupgradesafety.WithConfig(&crdifyconfig.Config{
415+
Conversion: crdifyconfig.ConversionPolicyIgnore,
416+
UnhandledEnforcement: crdifyconfig.EnforcementPolicyError,
417+
}))
418+
419+
rel := &release.Release{
420+
Name: "test-release",
421+
Manifest: getManifestString(t, "crd-unhandled-new.json"),
422+
}
423+
424+
err := preflight.Upgrade(context.Background(), rel)
425+
require.Error(t, err)
426+
require.ErrorContains(t, err, "unhandled changes found")
427+
})
428+
}

‎internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-status-change-new.json‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
{
2+
"apiVersion": "apiextensions.k8s.io/v1",
3+
"kind": "CustomResourceDefinition",
4+
"metadata": {"name": "apps.example.com"},
5+
"spec": {
6+
"group": "example.com",
7+
"versions": [
8+
{
9+
"name": "v1alpha1",
10+
"served": true,
11+
"storage": true,
12+
"schema": {
13+
"openAPIV3Schema": {
14+
"type": "object",
15+
"properties": {
16+
"spec": {
17+
"type": "object",
18+
"properties": {
19+
"name": {"type": "string"}
20+
}
21+
},
22+
"status": {
23+
"type": "object",
24+
"properties": {
25+
"resources": {
26+
"type": "array",
27+
"items": {
28+
"type": "object",
29+
"properties": {
30+
"status": {"type": "string"},
31+
"syncWave": {"type": "integer", "format": "int64"}
32+
}
33+
}
34+
}
35+
}
36+
}
37+
}
38+
}
39+
}
40+
}
41+
],
42+
"scope": "Namespaced",
43+
"names": {"plural": "apps", "singular": "app", "kind": "App"}
44+
}
45+
}
46+

‎internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-status-change-old.json‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
{
2+
"apiVersion": "apiextensions.k8s.io/v1",
3+
"kind": "CustomResourceDefinition",
4+
"metadata": {"name": "apps.example.com"},
5+
"spec": {
6+
"group": "example.com",
7+
"versions": [
8+
{
9+
"name": "v1alpha1",
10+
"served": true,
11+
"storage": true,
12+
"schema": {
13+
"openAPIV3Schema": {
14+
"type": "object",
15+
"properties": {
16+
"spec": {
17+
"type": "object",
18+
"properties": {
19+
"name": {"type": "string"}
20+
}
21+
},
22+
"status": {
23+
"type": "object",
24+
"properties": {
25+
"resources": {
26+
"type": "array",
27+
"items": {
28+
"type": "object",
29+
"properties": {
30+
"status": {"type": "string"}
31+
}
32+
}
33+
}
34+
}
35+
}
36+
}
37+
}
38+
}
39+
}
40+
],
41+
"scope": "Namespaced",
42+
"names": {"plural": "apps", "singular": "app", "kind": "App"}
43+
}
44+
}
45+

‎internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-unhandled-new.json‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
{
2+
"apiVersion": "apiextensions.k8s.io/v1",
3+
"kind": "CustomResourceDefinition",
4+
"metadata": {
5+
"name": "widgets.example.com"
6+
},
7+
"spec": {
8+
"group": "example.com",
9+
"versions": [
10+
{
11+
"name": "v1alpha1",
12+
"served": true,
13+
"storage": true,
14+
"schema": {
15+
"openAPIV3Schema": {
16+
"type": "object",
17+
"properties": {
18+
"spec": {
19+
"type": "object",
20+
"properties": {
21+
"field": {
22+
"type": "string",
23+
"format": "email"
24+
}
25+
}
26+
}
27+
}
28+
}
29+
}
30+
}
31+
],
32+
"scope": "Namespaced",
33+
"names": {
34+
"plural": "widgets",
35+
"singular": "widget",
36+
"kind": "Widget"
37+
}
38+
}
39+
}
40+

‎internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-unhandled-old.json‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
{
2+
"apiVersion": "apiextensions.k8s.io/v1",
3+
"kind": "CustomResourceDefinition",
4+
"metadata": {
5+
"name": "widgets.example.com"
6+
},
7+
"spec": {
8+
"group": "example.com",
9+
"versions": [
10+
{
11+
"name": "v1alpha1",
12+
"served": true,
13+
"storage": true,
14+
"schema": {
15+
"openAPIV3Schema": {
16+
"type": "object",
17+
"properties": {
18+
"spec": {
19+
"type": "object",
20+
"properties": {
21+
"field": {
22+
"type": "string"
23+
}
24+
}
25+
}
26+
}
27+
}
28+
}
29+
}
30+
],
31+
"scope": "Namespaced",
32+
"names": {
33+
"plural": "widgets",
34+
"singular": "widget",
35+
"kind": "Widget"
36+
}
37+
}
38+
}
39+

0 commit comments

Comments
 (0)