Skip to content

Commit 43c7bd9

Browse files
everettraveneverettraven
committed
warn but allow CRD upgrade when validations fail but new CRD has conversion strategy
Signed-off-by: everettraven <everettraven@gmail.com>
1 parent be20100 commit 43c7bd9

File tree

3 files changed

+39
-21
lines changed

3 files changed

+39
-21
lines changed

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

+6-6
Original file line numberDiff line numberDiff line change
@@ -1585,9 +1585,7 @@ func (o *Operator) ensureSubscriptionCSVState(logger *logrus.Entry, sub *v1alpha
15851585
}
15861586

15871587
func (o *Operator) setIPReference(subs []*v1alpha1.Subscription, gen int, installPlanRef *corev1.ObjectReference) []*v1alpha1.Subscription {
1588-
var (
1589-
lastUpdated = o.now()
1590-
)
1588+
lastUpdated := o.now()
15911589
for _, sub := range subs {
15921590
sub.Status.LastUpdated = lastUpdated
15931591
if installPlanRef != nil {
@@ -2204,6 +2202,10 @@ func validateV1Beta1CRDCompatibility(dynamicClient dynamic.Interface, oldCRD *ap
22042202
return validateExistingCRs(dynamicClient, gr, validationsMap)
22052203
}
22062204

2205+
type ValidationError struct {
2206+
error
2207+
}
2208+
22072209
// validateExistingCRs lists all CRs for each version entry in validationsMap, then validates each using the paired validation.
22082210
func validateExistingCRs(dynamicClient dynamic.Interface, gr schema.GroupResource, validationsMap map[string]*apiextensions.CustomResourceValidation) error {
22092211
for version, schemaValidation := range validationsMap {
@@ -2212,7 +2214,6 @@ func validateExistingCRs(dynamicClient dynamic.Interface, gr schema.GroupResourc
22122214
if err != nil {
22132215
return fmt.Errorf("error creating validator for schema version %s: %s", version, err)
22142216
}
2215-
22162217
gvr := schema.GroupVersionResource{Group: gr.Group, Version: version, Resource: gr.Resource}
22172218
crList, err := dynamicClient.Resource(gvr).List(context.TODO(), metav1.ListOptions{})
22182219
if err != nil {
@@ -2229,7 +2230,7 @@ func validateExistingCRs(dynamicClient dynamic.Interface, gr schema.GroupResourc
22292230
} else {
22302231
namespacedName = fmt.Sprintf("%s/%s", cr.GetNamespace(), cr.GetName())
22312232
}
2232-
return fmt.Errorf("error validating %s %q: updated validation is too restrictive: %v", cr.GroupVersionKind(), namespacedName, err)
2233+
return ValidationError{fmt.Errorf("error validating %s %q: updated validation is too restrictive: %v", cr.GroupVersionKind(), namespacedName, err)}
22332234
}
22342235
}
22352236
}
@@ -2782,7 +2783,6 @@ func (o *Operator) ExecutePlan(plan *v1alpha1.InstallPlan) error {
27822783
func (o *Operator) getExistingAPIOwners(namespace string) (map[string][]string, error) {
27832784
// Get a list of CSVs in the namespace
27842785
csvList, err := o.client.OperatorsV1alpha1().ClusterServiceVersions(namespace).List(context.TODO(), metav1.ListOptions{})
2785-
27862786
if err != nil {
27872787
return nil, err
27882788
}

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

+17-11
Original file line numberDiff line numberDiff line change
@@ -251,8 +251,10 @@ func TestSyncInstallPlanUnhappy(t *testing.T) {
251251
testName: "HasSteps/NoOperatorGroup",
252252
err: fmt.Errorf("attenuated service account query failed - no operator group found that is managing this namespace"),
253253
expectedPhase: v1alpha1.InstallPlanPhaseInstalling,
254-
expectedCondition: &v1alpha1.InstallPlanCondition{Type: v1alpha1.InstallPlanInstalled, Status: corev1.ConditionFalse, Reason: v1alpha1.InstallPlanReasonInstallCheckFailed,
255-
Message: "no operator group found that is managing this namespace"},
254+
expectedCondition: &v1alpha1.InstallPlanCondition{
255+
Type: v1alpha1.InstallPlanInstalled, Status: corev1.ConditionFalse, Reason: v1alpha1.InstallPlanReasonInstallCheckFailed,
256+
Message: "no operator group found that is managing this namespace",
257+
},
256258
in: ipWithSteps,
257259
},
258260
{
@@ -261,8 +263,10 @@ func TestSyncInstallPlanUnhappy(t *testing.T) {
261263
err: fmt.Errorf("attenuated service account query failed - more than one operator group(s) are managing this namespace count=2"),
262264
expectedPhase: v1alpha1.InstallPlanPhaseInstalling,
263265
in: ipWithSteps,
264-
expectedCondition: &v1alpha1.InstallPlanCondition{Type: v1alpha1.InstallPlanInstalled, Status: corev1.ConditionFalse, Reason: v1alpha1.InstallPlanReasonInstallCheckFailed,
265-
Message: "more than one operator group(s) are managing this namespace count=2"},
266+
expectedCondition: &v1alpha1.InstallPlanCondition{
267+
Type: v1alpha1.InstallPlanInstalled, Status: corev1.ConditionFalse, Reason: v1alpha1.InstallPlanReasonInstallCheckFailed,
268+
Message: "more than one operator group(s) are managing this namespace count=2",
269+
},
266270
clientObjs: []runtime.Object{
267271
operatorGroup("og1", "sa", namespace,
268272
&corev1.ObjectReference{
@@ -283,8 +287,10 @@ func TestSyncInstallPlanUnhappy(t *testing.T) {
283287
testName: "HasSteps/NonExistentServiceAccount",
284288
err: fmt.Errorf("attenuated service account query failed - please make sure the service account exists. sa=sa1 operatorgroup=ns/og"),
285289
expectedPhase: v1alpha1.InstallPlanPhaseInstalling,
286-
expectedCondition: &v1alpha1.InstallPlanCondition{Type: v1alpha1.InstallPlanInstalled, Status: corev1.ConditionFalse, Reason: v1alpha1.InstallPlanReasonInstallCheckFailed,
287-
Message: "please make sure the service account exists. sa=sa1 operatorgroup=ns/og"},
290+
expectedCondition: &v1alpha1.InstallPlanCondition{
291+
Type: v1alpha1.InstallPlanInstalled, Status: corev1.ConditionFalse, Reason: v1alpha1.InstallPlanReasonInstallCheckFailed,
292+
Message: "please make sure the service account exists. sa=sa1 operatorgroup=ns/og",
293+
},
288294
in: ipWithSteps,
289295
clientObjs: []runtime.Object{
290296
operatorGroup("og", "sa1", namespace, nil),
@@ -1623,7 +1629,7 @@ func TestValidateV1Beta1CRDCompatibility(t *testing.T) {
16231629
},
16241630
oldCRD: unversionedCRDForV1beta1File("testdata/hivebug/crd.yaml"),
16251631
newCRD: unversionedCRDForV1beta1File("testdata/hivebug/crd.yaml"),
1626-
want: fmt.Errorf("error validating hive.openshift.io/v1, Kind=MachinePool \"test\": updated validation is too restrictive: [[].spec.clusterDeploymentRef: Invalid value: \"null\": spec.clusterDeploymentRef in body must be of type object: \"null\", [].spec.name: Required value, [].spec.platform: Required value]"),
1632+
want: ValidationError{fmt.Errorf("error validating hive.openshift.io/v1, Kind=MachinePool \"test\": updated validation is too restrictive: [[].spec.clusterDeploymentRef: Invalid value: \"null\": spec.clusterDeploymentRef in body must be of type object: \"null\", [].spec.name: Required value, [].spec.platform: Required value]")},
16271633
},
16281634
{
16291635
name: "backwards incompatible change",
@@ -1637,7 +1643,7 @@ func TestValidateV1Beta1CRDCompatibility(t *testing.T) {
16371643
},
16381644
oldCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.old.yaml"),
16391645
newCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.yaml"),
1640-
want: fmt.Errorf("error validating cluster.com/v1alpha1, Kind=testcrd \"my-cr-1\": updated validation is too restrictive: [].spec.scalar: Invalid value: 2: spec.scalar in body should be greater than or equal to 3"),
1646+
want: ValidationError{fmt.Errorf("error validating cluster.com/v1alpha1, Kind=testcrd \"my-cr-1\": updated validation is too restrictive: [].spec.scalar: Invalid value: 2: spec.scalar in body should be greater than or equal to 3")},
16411647
},
16421648
{
16431649
name: "unserved version",
@@ -1670,7 +1676,7 @@ func TestValidateV1Beta1CRDCompatibility(t *testing.T) {
16701676
},
16711677
oldCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.no-versions-list.old.yaml"),
16721678
newCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.no-versions-list.yaml"),
1673-
want: fmt.Errorf("error validating cluster.com/v1alpha1, Kind=testcrd \"my-cr-1\": updated validation is too restrictive: [].spec.scalar: Invalid value: 2: spec.scalar in body should be greater than or equal to 3"),
1679+
want: ValidationError{fmt.Errorf("error validating cluster.com/v1alpha1, Kind=testcrd \"my-cr-1\": updated validation is too restrictive: [].spec.scalar: Invalid value: 2: spec.scalar in body should be greater than or equal to 3")},
16741680
},
16751681
}
16761682
for _, tt := range tests {
@@ -1725,7 +1731,7 @@ func TestValidateV1CRDCompatibility(t *testing.T) {
17251731
},
17261732
oldCRD: unversionedCRDForV1File("testdata/apiextensionsv1/crontabs.crd.old.yaml"),
17271733
newCRD: unversionedCRDForV1File("testdata/apiextensionsv1/crontabs.crd.yaml"),
1728-
want: fmt.Errorf("error validating stable.example.com/v2, Kind=CronTab \"my-crontab\": updated validation is too restrictive: [].spec.replicas: Invalid value: 10: spec.replicas in body should be less than or equal to 9"),
1734+
want: ValidationError{fmt.Errorf("error validating stable.example.com/v2, Kind=CronTab \"my-crontab\": updated validation is too restrictive: [].spec.replicas: Invalid value: 10: spec.replicas in body should be less than or equal to 9")},
17291735
},
17301736
{
17311737
name: "cr not invalidated by unserved version",
@@ -1752,7 +1758,7 @@ func TestValidateV1CRDCompatibility(t *testing.T) {
17521758
},
17531759
oldCRD: unversionedCRDForV1File("testdata/apiextensionsv1/single-version-crd.old.yaml"),
17541760
newCRD: unversionedCRDForV1File("testdata/apiextensionsv1/single-version-crd.yaml"),
1755-
want: fmt.Errorf("error validating cluster.com/v1alpha1, Kind=testcrd \"my-cr-1\": updated validation is too restrictive: [].spec.scalar: Invalid value: 100: spec.scalar in body should be less than or equal to 50"),
1761+
want: ValidationError{fmt.Errorf("error validating cluster.com/v1alpha1, Kind=testcrd \"my-cr-1\": updated validation is too restrictive: [].spec.scalar: Invalid value: 100: spec.scalar in body should be less than or equal to 50")},
17561762
},
17571763
}
17581764
for _, tt := range tests {

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

+16-4
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,21 @@ func (b *builder) NewCRDV1Step(client apiextensionsv1client.ApiextensionsV1Inter
140140
currentCRD, _ := client.CustomResourceDefinitions().Get(context.TODO(), crd.GetName(), metav1.GetOptions{})
141141
crd.SetResourceVersion(currentCRD.GetResourceVersion())
142142
if err = validateV1CRDCompatibility(b.dynamicClient, currentCRD, crd); err != nil {
143-
return fmt.Errorf("error validating existing CRs against new CRD's schema for %q: %w", step.Resource.Name, err)
143+
vErr := &ValidationError{}
144+
// if the conversion strategy in the new CRD is "None" OR the error is not a ValidationError
145+
// return an error. This will catch and return any errors that occur unrelated to actual validation.
146+
// For example, the API server returning an error when performing a list operation
147+
if crd.Spec.Conversion == nil || crd.Spec.Conversion.Strategy == apiextensionsv1.NoneConverter || !errors.As(err, vErr) {
148+
return fmt.Errorf("error validating existing CRs against new CRD's schema for %q: %w", step.Resource.Name, err)
149+
}
150+
// If the conversion strategy in the new CRD is not "None" and the error that occurred
151+
// is an error related to validation, warn that validation failed but that we are trusting
152+
// that the conversion strategy specified by the author will successfully convert to a format
153+
// that passes validation and allow the upgrade to continue
154+
b.logger.Warnf("trusting conversion strategy detected in new CRD, but failed validation of existing CRs against new CRD's schema for %q: %s",
155+
step.Resource.Name,
156+
err.Error(),
157+
)
144158
}
145159

146160
// check to see if stored versions changed and whether the upgrade could cause potential data loss
@@ -267,9 +281,7 @@ func (b *builder) NewCRDV1Beta1Step(client apiextensionsv1beta1client.Apiextensi
267281
}
268282

269283
func setInstalledAlongsideAnnotation(a alongside.Annotator, dst metav1.Object, namespace string, name string, lister listersv1alpha1.ClusterServiceVersionLister, srcs ...metav1.Object) {
270-
var (
271-
nns []alongside.NamespacedName
272-
)
284+
var nns []alongside.NamespacedName
273285

274286
// Only keep references to existing and non-copied CSVs to
275287
// avoid unbounded growth.

0 commit comments

Comments
 (0)