crddiff: Treat field removal as a breaking API change

Signed-off-by: Alper Rifat Ulucinar <[email protected]>

go.mod

@@ -8,6 +8,7 @@ require (
 	github.com/google/go-cmp v0.5.9
 	github.com/pkg/errors v0.9.1
 	github.com/tufin/oasdiff v1.2.6
+	golang.org/x/mod v0.7.0
 	gopkg.in/alecthomas/kingpin.v2 v2.2.6
 	k8s.io/apiextensions-apiserver v0.23.0
 	k8s.io/apimachinery v0.23.0
@@ -36,7 +37,6 @@ require (
 	github.com/modern-go/reflect2 v1.0.2 // indirect
 	github.com/mohae/deepcopy v0.0.0-20170929034955-c48cc78d4826 // indirect
 	github.com/yuin/goldmark v1.5.3 // indirect
-	golang.org/x/mod v0.7.0 // indirect
 	golang.org/x/net v0.0.0-20221014081412-f15817d10f9b // indirect
 	golang.org/x/oauth2 v0.0.0-20221014153046-6fdb5e3db783 // indirect
 	golang.org/x/sys v0.0.0-20220728004956-3c1f35247d10 // indirect

go.sum

@@ -82,6 +82,7 @@ github.com/blang/semver v3.5.1+incompatible/go.mod h1:kRBLl5iJ+tD4TcOOxsy/0fnweb
 github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU=
 github.com/certifi/gocertifi v0.0.0-20191021191039-0944d244cd40/go.mod h1:sGbDF6GwGcLpkNXPUTkMRoywsNa/ol15pxFe6ERfguA=
 github.com/certifi/gocertifi v0.0.0-20200922220541-2c3bb06c6054/go.mod h1:sGbDF6GwGcLpkNXPUTkMRoywsNa/ol15pxFe6ERfguA=
+github.com/cespare/xxhash v1.1.0 h1:a6HrQnmkObjyL+Gs60czilIUGqrzKutQD6XZog3p+ko=
 github.com/cespare/xxhash v1.1.0/go.mod h1:XrSqR1VqqWfGrhpAt58auRo0WTKS1nRRg3ghfAqPWnc=
 github.com/cespare/xxhash/v2 v2.1.1 h1:6MnRN8NT7+YBpUIWxHtefFZOKTAPgGjpQSxqLNn0+qY=
 github.com/cespare/xxhash/v2 v2.1.1/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs=

internal/crdschema/crd.go

@@ -29,6 +29,8 @@ import (
 )
 
 const (
+	contentTypeJSON = "application/json"
+
 	errCRDLoad                        = "failed to load the CustomResourceDefinition"
 	errBreakingRevisionChangesCompute = "failed to compute breaking changes in base and revision CRD schemas"
 	errBreakingSelfVersionsCompute    = "failed to compute breaking changes in the versions of a CRD"
@@ -117,7 +119,7 @@ func getOpenAPIv3Document(crd *v1.CustomResourceDefinition) ([]*openapi3.T, erro
 			},
 		}
 		s := &openapi3.Schema{}
-		c["application/json"] = &openapi3.MediaType{
+		c[contentTypeJSON] = &openapi3.MediaType{
 			Schema: &openapi3.SchemaRef{
 				Value: s,
 			},
@@ -205,13 +207,88 @@ func (d *RevisionDiff) GetBreakingChanges() (map[string]*diff.Diff, error) {
 		}
 		diffMap[versionName] = sd
 	}
-	return diffMap, nil
+	return filterNonBreaking(diffMap), nil
+}
+
+var crdPutEndpoint = diff.Endpoint{
+	Method: "PUT",
+	Path:   "/crd",
+}
+
+func filterNonBreaking(diffMap map[string]*diff.Diff) map[string]*diff.Diff {
+	for v, d := range diffMap {
+		if d.Empty() {
+			continue
+		}
+		sd := d.EndpointsDiff.Modified[crdPutEndpoint].RequestBodyDiff.ContentDiff.MediaTypeModified[contentTypeJSON].SchemaDiff
+		ignoreOptionalNewProperties(sd)
+		if sd != nil && empty(sd.PropertiesDiff) {
+			sd.PropertiesDiff = nil
+		}
+		if sd == nil || sd.Empty() {
+			delete(diffMap, v)
+		}
+	}
+	return diffMap
+}
+
+func ignoreOptionalNewProperties(sd *diff.SchemaDiff) {
+	if sd == nil || sd.Empty() {
+		return
+	}
+	if sd.PropertiesDiff != nil {
+		// optional new fields are non-breaking
+		filteredAddedProps := make(diff.StringList, 0, len(sd.PropertiesDiff.Added))
+		if sd.RequiredDiff != nil {
+			for _, f := range sd.PropertiesDiff.Added {
+				for _, r := range sd.RequiredDiff.Added {
+					if f == r {
+						filteredAddedProps = append(filteredAddedProps, f)
+						break
+					}
+				}
+			}
+		}
+		sd.PropertiesDiff.Added = filteredAddedProps
+		for n, csd := range sd.PropertiesDiff.Modified {
+			ignoreOptionalNewProperties(csd)
+			if csd != nil && empty(csd.PropertiesDiff) {
+				csd.PropertiesDiff = nil
+			}
+			if csd == nil || csd.Empty() {
+				delete(sd.PropertiesDiff.Modified, n)
+			}
+		}
+		if empty(sd.PropertiesDiff) {
+			sd.PropertiesDiff = nil
+		}
+	}
+	ignoreOptionalNewProperties(sd.ItemsDiff)
+	if sd.ItemsDiff != nil && sd.ItemsDiff.Empty() {
+		sd.ItemsDiff = nil
+	}
+}
+
+func empty(sd *diff.SchemasDiff) bool {
+	if sd == nil || sd.Empty() {
+		return true
+	}
+	if len(sd.Added) != 0 || len(sd.Deleted) != 0 {
+		return false
+	}
+	for _, csd := range sd.Modified {
+		if csd != nil && !csd.Empty() {
+			return false
+		}
+	}
+	return true
 }
 
 func schemaDiff(baseDoc, revisionDoc *openapi3.T) (*diff.Diff, error) {
-	config := diff.NewConfig()
-	// currently we only need to detect breaking API changes
-	config.BreakingOnly = true
+	config := &diff.Config{
+		ExcludeExamples:    true,
+		ExcludeDescription: true,
+	}
 	sd, err := diff.Get(config, baseDoc, revisionDoc)
 	return sd, errors.Wrap(err, "failed to compute breaking changes between OpenAPI v3 schemas")
 }

internal/crdschema/crd_test.go

@@ -245,6 +245,96 @@ func Test_GetRevisionBreakingChanges(t *testing.T) {
               - Type changed from 'string' to 'int'`},
 			},
 		},
+		"RemoveRequiredFieldInRevision": {
+			reason: "Removing a required field in the revision is a breaking API change",
+			args: args{
+				basePath: "testdata/base.yaml",
+				revisionModifiers: []crdModifier{
+					func(r *v1.CustomResourceDefinition) {
+						removeSpecForProviderProperty(r, 0, "region")
+					},
+				},
+			},
+			want: want{
+				breakingChanges: map[int]string{0: `
+- Schema changed
+  - Properties changed
+    - Modified property: spec
+      - Properties changed
+        - Modified property: forProvider
+          - Properties changed
+            - Deleted property: region`},
+			},
+		},
+		"RemoveOptionalFieldInRevision": {
+			reason: "Removing an optional field in the revision is a breaking API change",
+			args: args{
+				basePath: "testdata/base.yaml",
+				revisionModifiers: []crdModifier{
+					func(r *v1.CustomResourceDefinition) {
+						removeSpecForProviderProperty(r, 0, "tags")
+					},
+				},
+			},
+			want: want{
+				breakingChanges: map[int]string{0: `
+- Schema changed
+  - Properties changed
+    - Modified property: spec
+      - Properties changed
+        - Modified property: forProvider
+          - Properties changed
+            - Deleted property: tags`},
+			},
+		},
+		"RenameRequiredFieldInRevision": {
+			reason: "Renaming a required field in the revision is a breaking API change",
+			args: args{
+				basePath: "testdata/base.yaml",
+				revisionModifiers: []crdModifier{
+					func(r *v1.CustomResourceDefinition) {
+						renameSpecForProviderProperty(r, 0, "region", "regionRenamed")
+					},
+				},
+			},
+			want: want{
+				breakingChanges: map[int]string{0: `
+- Schema changed
+  - Properties changed
+    - Modified property: spec
+      - Properties changed
+        - Modified property: forProvider
+          - Required changed
+            - New required property: regionRenamed
+            - Deleted required property: region
+          - Properties changed
+            - New property: regionRenamed
+            - Deleted property: region`},
+			},
+		},
+		"RenameOptionalFieldInRevision": {
+			reason: "Renaming an optional field in the revision is a breaking API change",
+			args: args{
+				basePath: "testdata/base.yaml",
+				revisionModifiers: []crdModifier{
+					func(r *v1.CustomResourceDefinition) {
+						renameSpecForProviderProperty(r, 0, "tags", "tagsRenamed")
+					},
+				},
+			},
+			want: want{
+				// this is basically removing an optional field (breaking)
+				// and adding a new optional field (non-breaking)
+				breakingChanges: map[int]string{0: `
+- Schema changed
+  - Properties changed
+    - Modified property: spec
+      - Properties changed
+        - Modified property: forProvider
+          - Properties changed
+            - Deleted property: tags`},
+			},
+		},
 	}
 
 	for name, tt := range tests {
@@ -390,3 +480,24 @@ func addSpecForProviderProperty(crd *v1.CustomResourceDefinition, versionIndex i
 	}
 	crd.Spec.Versions[versionIndex].Schema.OpenAPIV3Schema.Properties["spec"].Properties["forProvider"] = forProvider
 }
+
+func removeSpecForProviderProperty(crd *v1.CustomResourceDefinition, versionIndex int, fieldName string) {
+	forProvider := crd.Spec.Versions[versionIndex].Schema.OpenAPIV3Schema.Properties["spec"].Properties["forProvider"]
+	delete(forProvider.Properties, fieldName)
+	crd.Spec.Versions[versionIndex].Schema.OpenAPIV3Schema.Properties["spec"].Properties["forProvider"] = forProvider
+}
+
+func renameSpecForProviderProperty(crd *v1.CustomResourceDefinition, versionIndex int, fieldOldName, fieldNewName string) {
+	forProvider := crd.Spec.Versions[versionIndex].Schema.OpenAPIV3Schema.Properties["spec"].Properties["forProvider"]
+	p := forProvider.Properties[fieldOldName]
+	removeSpecForProviderProperty(crd, versionIndex, fieldOldName)
+	forProvider.Properties[fieldNewName] = p
+	// check if the field being removed is a required one
+	for i, r := range forProvider.Required {
+		if r == fieldOldName {
+			forProvider.Required[i] = fieldNewName
+			break
+		}
+	}
+	crd.Spec.Versions[versionIndex].Schema.OpenAPIV3Schema.Properties["spec"].Properties["forProvider"] = forProvider
+}