Merge pull request #1803 from openshift-cherrypick-robot/cherry-pick-…

…1802-to-release-0.7 [release-0.7] Fix schemacompat npe
kcp-dev · Aug 19, 2022 · 55b06a4 · 55b06a4
2 parents c81a760 + bbf5ac7
commit 55b06a4
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 5 deletions.
diff --git a/pkg/schemacompat/schemacompat.go b/pkg/schemacompat/schemacompat.go
@@ -24,7 +24,7 @@ import (
 
 	"go.uber.org/multierr"
 
-	apiextensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
+	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	"k8s.io/apiextensions-apiserver/pkg/apiserver/schema"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -361,14 +361,14 @@ func lcdForObject(fldPath *field.Path, existing, new *schema.Structural, lcd *sc
 				delete(lcd.Properties, removedProperty)
 			}
 
-		} else if new.AdditionalProperties.Structural != nil {
+		} else if new.AdditionalProperties != nil && new.AdditionalProperties.Structural != nil {
 			for _, key := range sets.StringKeySet(existing.Properties).List() {
 				existingPropertySchema := existing.Properties[key]
 				lcdPropertySchema := lcd.Properties[key]
 				multierr.AppendInto(&err, lcdForStructural(fldPath.Child("properties").Key(key), &existingPropertySchema, new.AdditionalProperties.Structural, &lcdPropertySchema, narrowExisting))
 				lcd.Properties[key] = lcdPropertySchema
 			}
-		} else if new.AdditionalProperties.Bool {
+		} else if new.AdditionalProperties != nil && new.AdditionalProperties.Bool {
 			// that allows named properties only.
 			// => Keep the existing schemas as the lcd.
 		} else {

diff --git a/pkg/schemacompat/schemacompat_test.go b/pkg/schemacompat/schemacompat_test.go
@@ -20,6 +20,7 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"go.uber.org/multierr"
 
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	"k8s.io/apimachinery/pkg/util/validation/field"
@@ -440,12 +441,52 @@ func TestCompatibility(t *testing.T) {
 				},
 			},
 		},
+	}, {
+		desc: "existing has properties, new has neither properties or additionalProperties",
+		existing: &apiextensionsv1.JSONSchemaProps{
+			Type: "object",
+			Properties: map[string]apiextensionsv1.JSONSchemaProps{
+				"existing": {Type: "boolean"},
+			},
+		},
+		new: &apiextensionsv1.JSONSchemaProps{
+			Type: "array",
+			Items: &apiextensionsv1.JSONSchemaPropsOrArray{
+				Schema: &apiextensionsv1.JSONSchemaProps{
+					Type: "object",
+					Properties: map[string]apiextensionsv1.JSONSchemaProps{
+						"existing": {Type: "integer"},
+					},
+				},
+			},
+		},
+		wantErr: multierr.Append(
+			field.Invalid(
+				field.NewPath("schema", "openAPISchema").Child("type"),
+				"array",
+				`The type changed (was "object", now "array")`,
+			),
+			field.Invalid(
+				field.NewPath("schema", "openAPISchema").Child("properties"),
+				[]string{"existing"},
+				"properties value has been completely cleared in an incompatible way",
+			),
+		),
 	}} {
 		t.Run(c.desc, func(t *testing.T) {
 			gotLCD, err := EnsureStructuralSchemaCompatibility(field.NewPath("schema", "openAPISchema"), c.existing, c.new, c.narrowExisting)
-			if d := cmp.Diff(c.wantErr, err); d != "" {
-				t.Errorf("Error Diff(-want,+got): %s", d)
+			if c.wantErr != nil {
+				if err == nil {
+					t.Fatalf("expected err %v but got nil", c.wantErr)
+				}
+
+				if d := cmp.Diff(c.wantErr.Error(), err.Error()); d != "" {
+					t.Errorf("Error Diff(-want,+got): %s", d)
+				}
+			} else if err != nil {
+				t.Fatalf("unexpected err %v", err)
 			}
+
 			if d := cmp.Diff(c.wantLCD, gotLCD); d != "" {
 				t.Errorf("LCD Diff(-want,+got): %s", d)
 			}