Skip to content

Commit 40b1627

Browse files
committed
CRD upgrade existing CR validation fix (#3442)
* fixed to pass map[string]interface instead of unstructured.Unstructured We started seeing some issues with folks who had spurious CRD incompatibility claims when updating operators. It is a failure in OLM code which validates existing CRs against incoming CRDs, recently updated in #3387. This manifested in `InstallPlan` `.status.Message` something like: ``` retrying execution due to error: error validating existing CRs against new CRD's schema for \"pgadmins.postgres-operator.crunchydata.com\": error validating postgres-operator.crunchydata.com/v1beta1, Kind=PGAdmin \"openshift-operators/example-pgadmin\": updated validation is too restrictive: [].spec.tolerations[0].tolerationSeconds: Invalid value: \"number\": spec.tolerations[0].tolerationSeconds in body must be of type integer: \"number\" ``` The difference between the predecessor calling convention and the one introduced in #3387 appears to be that one is a pointer and the other is concrete. old ```golang unstructured.Unstructured{Object:map[string]interface... ``` new ```golang &unstructured.Unstructured{Object:map[string]interface... ``` so it would seem that merely type-asserting the value and de-referencing it would yield the appropriate result, but it appears instead that it effectively disables all CR vs CRD reconciliation checks (evidenced by the fact that the unit tests multiply fail). But k8s already dereferences pointer parameters [here](https://github.com/kubernetes/kube-openapi/blob/master/pkg/validation/validate/schema.go#L139-L141) during validation. So that isn't it. And the `validate.ValidateCustomResource` interface is terrifyingly permissive in allowing `customResource` as `interface{}` [here](https://pkg.go.dev/k8s.io/[email protected]/pkg/apiserver/validation#ValidateCustomResource). So we cannot derive guidance from it. Taking a page from k8s' use of the validation API, which uses `unstructured.UnstructuredContent()` to convert the `unstructured.Unstructured` into a `map[string]interface{}` [here](https://github.com/kubernetes/kubernetes/blob/1504f10e7946f95a8b1da35e28e4c7453ff62775/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/validator.go#L54) then we achieve the desired results. Signed-off-by: Jordan Keister <[email protected]> * adding tests Signed-off-by: Jordan Keister <[email protected]> --------- Signed-off-by: Jordan Keister <[email protected]> Upstream-repository: operator-lifecycle-manager Upstream-commit: 1cfabfe5a495fe3cb276fce93255cdfed7d60783
1 parent 82dc3f9 commit 40b1627

File tree

5 files changed

+1961
-8
lines changed

5 files changed

+1961
-8
lines changed

staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1918,15 +1918,15 @@ func validateExistingCRs(dynamicClient dynamic.Interface, gvr schema.GroupVersio
19181918
return dynamicClient.Resource(gvr).List(context.TODO(), opts)
19191919
}))
19201920
validationFn := func(obj runtime.Object) error {
1921+
// lister will only provide unstructured objects as runtime.Object, so this should never fail to convert
1922+
// if it does, it's a programming error
1923+
cr := obj.(*unstructured.Unstructured)
19211924
validator, _, err := validation.NewSchemaValidator(newCRD.Spec.Validation)
19221925
if err != nil {
19231926
return fmt.Errorf("error creating validator for schema %#v: %s", newCRD.Spec.Validation, err)
19241927
}
1925-
err = validation.ValidateCustomResource(field.NewPath(""), obj, validator).ToAggregate()
1928+
err = validation.ValidateCustomResource(field.NewPath(""), cr.UnstructuredContent(), validator).ToAggregate()
19261929
if err != nil {
1927-
// lister will only provide unstructured objects as runtime.Object, so this should never fail to convert
1928-
// if it does, it's a programming error
1929-
cr := obj.(*unstructured.Unstructured)
19301930
var namespacedName string
19311931
if cr.GetNamespace() == "" {
19321932
namespacedName = cr.GetName()

staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator_test.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1616,6 +1616,18 @@ func TestValidateExistingCRs(t *testing.T) {
16161616
newCRD: unversionedCRDForV1beta1File("testdata/hivebug/crd.yaml"),
16171617
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]"),
16181618
},
1619+
{
1620+
name: "crd with incorrect comparison",
1621+
existingObjects: []runtime.Object{
1622+
unstructuredForFile("testdata/postgrestolerations/pgadmin.cr.yaml"),
1623+
},
1624+
gvr: schema.GroupVersionResource{
1625+
Group: "postgres-operator.crunchydata.com",
1626+
Version: "v1beta1",
1627+
Resource: "pgadmins",
1628+
},
1629+
newCRD: unversionedCRDForV1beta1File("testdata/postgrestolerations/crd.yaml"),
1630+
},
16191631
}
16201632
for _, tt := range tests {
16211633
t.Run(tt.name, func(t *testing.T) {

0 commit comments

Comments
 (0)