Skip to content

Commit 92aac5f

Browse files
exdxexdx
committed
fix: add check for storage version changes when installing CRDs. This
prevents data loss when a CRD is upgraded and no longer supports existing stored versions of CRs on the cluster.
1 parent 1849f65 commit 92aac5f

File tree

4 files changed

+564
-20
lines changed

4 files changed

+564
-20
lines changed

‎pkg/controller/operators/catalog/step.go

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,16 @@ func (b *builder) NewCRDV1Step(client apiextensionsv1client.ApiextensionsV1Inter
139139
}
140140
}
141141

142-
// TODO ensure stored version compatibility
142+
// check to see if stored versions changed and whether the upgrade could cause potential data loss
143+
safe, err := crdlib.SafeStorageVersionUpgrade(currentCRD, crd)
144+
if !safe {
145+
b.logger.Errorf("risk of data loss updating %s: %s", step.Resource.Name, err)
146+
return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(err, "risk of data loss updating %s", step.Resource.Name)
147+
}
148+
if err != nil {
149+
return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(err, "checking CRD for potential data loss updating %s", step.Resource.Name)
150+
}
151+
143152
// Update CRD to new version
144153
_, err = client.CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{})
145154
if err != nil {
@@ -219,7 +228,16 @@ func (b *builder) NewCRDV1Beta1Step(client apiextensionsv1beta1client.Apiextensi
219228
return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(err, "error validating existing CRs agains new CRD's schema: %s", step.Resource.Name)
220229
}
221230
}
222-
// TODO ensure stored version compatibility
231+
232+
// check to see if stored versions changed and whether the upgrade could cause potential data loss
233+
safe, err := crdlib.SafeStorageVersionUpgrade(currentCRD, crd)
234+
if !safe {
235+
b.logger.Errorf("risk of data loss updating %s: %s", step.Resource.Name, err)
236+
return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(err, "risk of data loss updating %s", step.Resource.Name)
237+
}
238+
if err != nil {
239+
return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(err, "checking CRD for potential data loss updating %s", step.Resource.Name)
240+
}
223241

224242
// Update CRD to new version
225243
_, err = client.CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{})

‎pkg/lib/crd/storage.go

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
package crd
2+
3+
import (
4+
"fmt"
5+
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
6+
apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
7+
"k8s.io/apimachinery/pkg/runtime"
8+
)
9+
10+
// SafeStorageVersionUpgrade determines whether the new CRD spec includes all the storage versions of the existing on-cluster CRD.
11+
// For each stored version in the status of the CRD on the cluster (there will be at least one) - each version must exist in the spec of the new CRD that is being installed.
12+
// See https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#upgrade-existing-objects-to-a-new-stored-version.
13+
func SafeStorageVersionUpgrade(existingCRD runtime.Object, newCRD runtime.Object) (bool, error) {
14+
newSpecVersions, existingStatusVersions := getStoredVersions(existingCRD, newCRD)
15+
if newSpecVersions == nil {
16+
return true, fmt.Errorf("could not find any versions in the new CRD")
17+
}
18+
if existingStatusVersions == nil {
19+
// every on-cluster CRD should have at least one stored version in its status
20+
// in the case where there are no existing stored versions, checking against new versions is not relevant
21+
return true, nil
22+
}
23+
24+
for name := range existingStatusVersions {
25+
if _, ok := newSpecVersions[name]; !ok {
26+
// a storage version in the status of the old CRD is not present in the spec of the new CRD
27+
// potential data loss of CRs without a storage migration - throw error and block the CRD upgrade
28+
return false, fmt.Errorf("new CRD removes version %s that is listed as a stored version on the existing CRD", name)
29+
}
30+
}
31+
32+
return true, nil
33+
}
34+
35+
// getStoredVersions returns the storage versions listed in the status of the old on-cluster CRD
36+
// and all the versions listed in the spec of the new CRD.
37+
func getStoredVersions(oldCRD runtime.Object, newCRD runtime.Object) (newSpecVersions map[string]struct{}, existingStatusVersions map[string]struct{}) {
38+
existingStatusVersions = make(map[string]struct{})
39+
newSpecVersions = make(map[string]struct{})
40+
41+
// find old storage versions by inspect the status field of the existing on-cluster CRD
42+
switch crd := oldCRD.(type) {
43+
case *apiextensionsv1.CustomResourceDefinition:
44+
for _, version := range crd.Status.StoredVersions {
45+
existingStatusVersions[version] = struct{}{}
46+
}
47+
case *apiextensionsv1beta1.CustomResourceDefinition:
48+
for _, version := range crd.Status.StoredVersions {
49+
existingStatusVersions[version] = struct{}{}
50+
}
51+
}
52+
53+
switch crd := newCRD.(type) {
54+
case *apiextensionsv1.CustomResourceDefinition:
55+
for _, version := range crd.Spec.Versions {
56+
newSpecVersions[version.Name] = struct{}{}
57+
}
58+
case *apiextensionsv1beta1.CustomResourceDefinition:
59+
if crd.Spec.Version != "" {
60+
newSpecVersions[crd.Spec.Version] = struct{}{}
61+
}
62+
for _, version := range crd.Spec.Versions {
63+
newSpecVersions[version.Name] = struct{}{}
64+
}
65+
}
66+
67+
return newSpecVersions, existingStatusVersions
68+
}

‎pkg/lib/crd/storage_test.go

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
package crd
2+
3+
import (
4+
"github.com/stretchr/testify/require"
5+
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
6+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
7+
"testing"
8+
)
9+
10+
const crdName = "test"
11+
12+
func TestSafeStorageVersionUpgradeFailure(t *testing.T) {
13+
existingCRD := &apiextensionsv1.CustomResourceDefinition{
14+
ObjectMeta: metav1.ObjectMeta{
15+
Name: crdName,
16+
},
17+
Spec: apiextensionsv1.CustomResourceDefinitionSpec{
18+
Group: "cluster.com",
19+
Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
20+
{
21+
Name: "v1alpha2",
22+
Served: true,
23+
Storage: true,
24+
Schema: &apiextensionsv1.CustomResourceValidation{
25+
OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{
26+
Type: "object",
27+
Description: "my crd schema",
28+
},
29+
},
30+
},
31+
},
32+
},
33+
Status: apiextensionsv1.CustomResourceDefinitionStatus{
34+
StoredVersions: []string{"v1alpha2"},
35+
},
36+
}
37+
38+
newCRD := &apiextensionsv1.CustomResourceDefinition{
39+
ObjectMeta: metav1.ObjectMeta{
40+
Name: crdName,
41+
},
42+
Spec: apiextensionsv1.CustomResourceDefinitionSpec{
43+
Group: "cluster.com",
44+
Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
45+
{
46+
Name: "v1alpha3",
47+
Served: true,
48+
Storage: true,
49+
Schema: &apiextensionsv1.CustomResourceValidation{
50+
OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{
51+
Type: "object",
52+
Description: "my crd schema",
53+
},
54+
},
55+
},
56+
},
57+
},
58+
}
59+
60+
safe, err := SafeStorageVersionUpgrade(existingCRD, newCRD)
61+
// expect safe to be false, since crd upgrade is not safe
62+
require.False(t, safe)
63+
// expect error, since crd upgrade is not safe
64+
require.Error(t, err, "expected error for unsafe CRD upgrade")
65+
// error should be related to the storage upgrade removing a version
66+
require.Contains(t, err.Error(), "new CRD removes version", "expected storage version error")
67+
}
68+
69+
func TestSafeStorageVersionUpgradeSuccess(t *testing.T) {
70+
existingCRD := &apiextensionsv1.CustomResourceDefinition{
71+
ObjectMeta: metav1.ObjectMeta{
72+
Name: crdName,
73+
},
74+
Spec: apiextensionsv1.CustomResourceDefinitionSpec{
75+
Group: "cluster.com",
76+
Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
77+
{
78+
Name: "v1alpha2",
79+
Served: true,
80+
Storage: false,
81+
Schema: &apiextensionsv1.CustomResourceValidation{
82+
OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{
83+
Type: "object",
84+
Description: "my crd schema",
85+
},
86+
},
87+
},
88+
{
89+
Name: "v1alpha3",
90+
Served: true,
91+
Storage: true,
92+
Schema: &apiextensionsv1.CustomResourceValidation{
93+
OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{
94+
Type: "object",
95+
Description: "my crd schema",
96+
},
97+
},
98+
},
99+
},
100+
},
101+
Status: apiextensionsv1.CustomResourceDefinitionStatus{
102+
StoredVersions: []string{"v1alpha2", "v1alpha3"},
103+
},
104+
}
105+
106+
newCRD := &apiextensionsv1.CustomResourceDefinition{
107+
ObjectMeta: metav1.ObjectMeta{
108+
Name: crdName,
109+
},
110+
Spec: apiextensionsv1.CustomResourceDefinitionSpec{
111+
Group: "cluster.com",
112+
Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
113+
{
114+
Name: "v1alpha2",
115+
Served: true,
116+
Storage: false,
117+
Schema: &apiextensionsv1.CustomResourceValidation{
118+
OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{
119+
Type: "object",
120+
Description: "my crd schema",
121+
},
122+
},
123+
},
124+
{
125+
Name: "v1alpha3",
126+
Served: true,
127+
Storage: true,
128+
Schema: &apiextensionsv1.CustomResourceValidation{
129+
OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{
130+
Type: "object",
131+
Description: "my crd schema",
132+
},
133+
},
134+
},
135+
},
136+
},
137+
}
138+
139+
safe, err := SafeStorageVersionUpgrade(existingCRD, newCRD)
140+
// expect safe to be true, since crd upgrade is safe
141+
require.True(t, safe)
142+
// expect no error, since crd upgrade is safe
143+
require.NoError(t, err, "did not expect error for safe CRD upgrade")
144+
}

0 commit comments

Comments
 (0)