Skip to content

Commit

Permalink
CRD upgrade existing CR validation fix (#3442)
Browse files Browse the repository at this point in the history
* 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]>
  • Loading branch information
grokspawn authored Dec 9, 2024
1 parent 8e39847 commit 1cfabfe
Show file tree
Hide file tree
Showing 4 changed files with 1,953 additions and 4 deletions.
8 changes: 4 additions & 4 deletions pkg/controller/operators/catalog/operator.go
Original file line number Diff line number Diff line change
Expand Up @@ -2246,11 +2246,11 @@ func validateExistingCRs(dynamicClient dynamic.Interface, gr schema.GroupResourc
return dynamicClient.Resource(gvr).List(context.TODO(), opts)
}))
validationFn := func(obj runtime.Object) error {
err = validation.ValidateCustomResource(field.NewPath(""), obj, validator).ToAggregate()
// lister will only provide unstructured objects as runtime.Object, so this should never fail to convert
// if it does, it's a programming error
cr := obj.(*unstructured.Unstructured)
err = validation.ValidateCustomResource(field.NewPath(""), cr.UnstructuredContent(), validator).ToAggregate()
if err != nil {
// lister will only provide unstructured objects as runtime.Object, so this should never fail to convert
// if it does, it's a programming error
cr := obj.(*unstructured.Unstructured)
var namespacedName string
if cr.GetNamespace() == "" {
namespacedName = cr.GetName()
Expand Down
8 changes: 8 additions & 0 deletions pkg/controller/operators/catalog/operator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1835,6 +1835,14 @@ func TestValidateV1Beta1CRDCompatibility(t *testing.T) {
newCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.no-versions-list.yaml"),
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")},
},
{
name: "crd with incorrect comparison",
existingObjects: []runtime.Object{
unstructuredForFile("testdata/postgrestolerations/pgadmin.cr.yaml"),
},
oldCRD: unversionedCRDForV1beta1File("testdata/postgrestolerations/crd.yaml"),
newCRD: unversionedCRDForV1beta1File("testdata/postgrestolerations/crd.yaml"),
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down
Loading

0 comments on commit 1cfabfe

Please sign in to comment.