Skip to content

Commit c3e1774

Browse files
everettraveneverettraven
authored
OCPBUGS-31522: Warn and allow CRD upgrade if validation fails but new CRD specifies a conversion strategy (#3209)
* warn but allow CRD upgrade when validations fail but new CRD has conversion strategy Signed-off-by: everettraven <[email protected]> * unexport validationError Signed-off-by: everettraven <[email protected]> * updating warning message to be more descriptive, add warning event on installplan Signed-off-by: everettraven <[email protected]> * update warning templ per review comments Signed-off-by: everettraven <[email protected]> --------- Signed-off-by: everettraven <[email protected]>
1 parent 9b28021 commit c3e1774

File tree

3 files changed

+50
-23
lines changed

3 files changed

+50
-23
lines changed

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

Lines changed: 7 additions & 7 deletions
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
}
@@ -2321,7 +2322,7 @@ func (o *Operator) ExecutePlan(plan *v1alpha1.InstallPlan) error {
23212322
o.logger.Errorf("failed to get a client for plan execution- %v", err)
23222323
return err
23232324
}
2324-
b := newBuilder(plan, o.lister.OperatorsV1alpha1().ClusterServiceVersionLister(), builderKubeClient, builderDynamicClient, r, o.logger)
2325+
b := newBuilder(plan, o.lister.OperatorsV1alpha1().ClusterServiceVersionLister(), builderKubeClient, builderDynamicClient, r, o.logger, o.recorder)
23252326

23262327
for i, step := range plan.Status.Plan {
23272328
if err := func(i int, step *v1alpha1.Step) error {
@@ -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

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -254,8 +254,10 @@ func TestSyncInstallPlanUnhappy(t *testing.T) {
254254
testName: "HasSteps/NoOperatorGroup",
255255
err: fmt.Errorf("attenuated service account query failed - no operator group found that is managing this namespace"),
256256
expectedPhase: v1alpha1.InstallPlanPhaseInstalling,
257-
expectedCondition: &v1alpha1.InstallPlanCondition{Type: v1alpha1.InstallPlanInstalled, Status: corev1.ConditionFalse, Reason: v1alpha1.InstallPlanReasonInstallCheckFailed,
258-
Message: "no operator group found that is managing this namespace"},
257+
expectedCondition: &v1alpha1.InstallPlanCondition{
258+
Type: v1alpha1.InstallPlanInstalled, Status: corev1.ConditionFalse, Reason: v1alpha1.InstallPlanReasonInstallCheckFailed,
259+
Message: "no operator group found that is managing this namespace",
260+
},
259261
in: ipWithSteps,
260262
},
261263
{
@@ -264,8 +266,10 @@ func TestSyncInstallPlanUnhappy(t *testing.T) {
264266
err: fmt.Errorf("attenuated service account query failed - more than one operator group(s) are managing this namespace count=2"),
265267
expectedPhase: v1alpha1.InstallPlanPhaseInstalling,
266268
in: ipWithSteps,
267-
expectedCondition: &v1alpha1.InstallPlanCondition{Type: v1alpha1.InstallPlanInstalled, Status: corev1.ConditionFalse, Reason: v1alpha1.InstallPlanReasonInstallCheckFailed,
268-
Message: "more than one operator group(s) are managing this namespace count=2"},
269+
expectedCondition: &v1alpha1.InstallPlanCondition{
270+
Type: v1alpha1.InstallPlanInstalled, Status: corev1.ConditionFalse, Reason: v1alpha1.InstallPlanReasonInstallCheckFailed,
271+
Message: "more than one operator group(s) are managing this namespace count=2",
272+
},
269273
clientObjs: []runtime.Object{
270274
operatorGroup("og1", "sa", namespace,
271275
&corev1.ObjectReference{
@@ -286,8 +290,10 @@ func TestSyncInstallPlanUnhappy(t *testing.T) {
286290
testName: "HasSteps/NonExistentServiceAccount",
287291
err: fmt.Errorf("attenuated service account query failed - please make sure the service account exists. sa=sa1 operatorgroup=ns/og"),
288292
expectedPhase: v1alpha1.InstallPlanPhaseInstalling,
289-
expectedCondition: &v1alpha1.InstallPlanCondition{Type: v1alpha1.InstallPlanInstalled, Status: corev1.ConditionFalse, Reason: v1alpha1.InstallPlanReasonInstallCheckFailed,
290-
Message: "please make sure the service account exists. sa=sa1 operatorgroup=ns/og"},
293+
expectedCondition: &v1alpha1.InstallPlanCondition{
294+
Type: v1alpha1.InstallPlanInstalled, Status: corev1.ConditionFalse, Reason: v1alpha1.InstallPlanReasonInstallCheckFailed,
295+
Message: "please make sure the service account exists. sa=sa1 operatorgroup=ns/og",
296+
},
291297
in: ipWithSteps,
292298
clientObjs: []runtime.Object{
293299
operatorGroup("og", "sa1", namespace, nil),
@@ -1780,7 +1786,7 @@ func TestValidateV1Beta1CRDCompatibility(t *testing.T) {
17801786
},
17811787
oldCRD: unversionedCRDForV1beta1File("testdata/hivebug/crd.yaml"),
17821788
newCRD: unversionedCRDForV1beta1File("testdata/hivebug/crd.yaml"),
1783-
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]"),
1789+
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]")},
17841790
},
17851791
{
17861792
name: "backwards incompatible change",
@@ -1794,7 +1800,7 @@ func TestValidateV1Beta1CRDCompatibility(t *testing.T) {
17941800
},
17951801
oldCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.old.yaml"),
17961802
newCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.yaml"),
1797-
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"),
1803+
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")},
17981804
},
17991805
{
18001806
name: "unserved version",
@@ -1827,7 +1833,7 @@ func TestValidateV1Beta1CRDCompatibility(t *testing.T) {
18271833
},
18281834
oldCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.no-versions-list.old.yaml"),
18291835
newCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.no-versions-list.yaml"),
1830-
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"),
1836+
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")},
18311837
},
18321838
}
18331839
for _, tt := range tests {
@@ -1882,7 +1888,7 @@ func TestValidateV1CRDCompatibility(t *testing.T) {
18821888
},
18831889
oldCRD: unversionedCRDForV1File("testdata/apiextensionsv1/crontabs.crd.old.yaml"),
18841890
newCRD: unversionedCRDForV1File("testdata/apiextensionsv1/crontabs.crd.yaml"),
1885-
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"),
1891+
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")},
18861892
},
18871893
{
18881894
name: "cr not invalidated by unserved version",
@@ -1909,7 +1915,7 @@ func TestValidateV1CRDCompatibility(t *testing.T) {
19091915
},
19101916
oldCRD: unversionedCRDForV1File("testdata/apiextensionsv1/single-version-crd.old.yaml"),
19111917
newCRD: unversionedCRDForV1File("testdata/apiextensionsv1/single-version-crd.yaml"),
1912-
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"),
1918+
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")},
19131919
},
19141920
}
19151921
for _, tt := range tests {

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

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,15 @@ import (
77
"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install"
88
"github.com/pkg/errors"
99
"github.com/sirupsen/logrus"
10+
corev1 "k8s.io/api/core/v1"
1011
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
1112
apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
1213
apiextensionsv1client "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1"
1314
apiextensionsv1beta1client "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1beta1"
1415
apierrors "k8s.io/apimachinery/pkg/api/errors"
1516
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1617
"k8s.io/client-go/dynamic"
18+
"k8s.io/client-go/tools/record"
1719
"k8s.io/client-go/util/retry"
1820

1921
"github.com/operator-framework/api/pkg/operators/v1alpha1"
@@ -44,18 +46,20 @@ type builder struct {
4446
dynamicClient dynamic.Interface
4547
manifestResolver ManifestResolver
4648
logger logrus.FieldLogger
49+
eventRecorder record.EventRecorder
4750

4851
annotator alongside.Annotator
4952
}
5053

51-
func newBuilder(plan *v1alpha1.InstallPlan, csvLister listersv1alpha1.ClusterServiceVersionLister, opclient operatorclient.ClientInterface, dynamicClient dynamic.Interface, manifestResolver ManifestResolver, logger logrus.FieldLogger) *builder {
54+
func newBuilder(plan *v1alpha1.InstallPlan, csvLister listersv1alpha1.ClusterServiceVersionLister, opclient operatorclient.ClientInterface, dynamicClient dynamic.Interface, manifestResolver ManifestResolver, logger logrus.FieldLogger, er record.EventRecorder) *builder {
5255
return &builder{
5356
plan: plan,
5457
csvLister: csvLister,
5558
opclient: opclient,
5659
dynamicClient: dynamicClient,
5760
manifestResolver: manifestResolver,
5861
logger: logger,
62+
eventRecorder: er,
5963
}
6064
}
6165

@@ -140,7 +144,26 @@ func (b *builder) NewCRDV1Step(client apiextensionsv1client.ApiextensionsV1Inter
140144
currentCRD, _ := client.CustomResourceDefinitions().Get(context.TODO(), crd.GetName(), metav1.GetOptions{})
141145
crd.SetResourceVersion(currentCRD.GetResourceVersion())
142146
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)
147+
vErr := &validationError{}
148+
// if the conversion strategy in the new CRD is not "Webhook" OR the error is not a ValidationError
149+
// return an error. This will catch and return any errors that occur unrelated to actual validation.
150+
// For example, the API server returning an error when performing a list operation
151+
if crd.Spec.Conversion == nil || crd.Spec.Conversion.Strategy != apiextensionsv1.WebhookConverter || !errors.As(err, vErr) {
152+
return fmt.Errorf("error validating existing CRs against new CRD's schema for %q: %w", step.Resource.Name, err)
153+
}
154+
// If the conversion strategy in the new CRD is "Webhook" and the error that occurred
155+
// is an error related to validation, warn that validation failed but that we are trusting
156+
// that the conversion strategy specified by the author will successfully convert to a format
157+
// that passes validation and allow the upgrade to continue
158+
warnTempl := `Validation of existing CRs against the new CRD's schema failed, but a webhook conversion strategy was specified in the new CRD.
159+
The new webhook will only start after the bundle is upgraded, so we must assume that it will successfully convert existing CRs to a format that would have passed validation.
160+
161+
CRD: %q
162+
Validation Error: %s
163+
`
164+
warnString := fmt.Sprintf(warnTempl, step.Resource.Name, err.Error())
165+
b.logger.Warn(warnString)
166+
b.eventRecorder.Event(b.plan, corev1.EventTypeWarning, "CRDValidation", warnString)
144167
}
145168

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

269292
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-
)
293+
var nns []alongside.NamespacedName
273294

274295
// Only keep references to existing and non-copied CSVs to
275296
// avoid unbounded growth.

0 commit comments

Comments
 (0)