From 2bb3d4f343b3dde877c919aa16a15af19bcd3450 Mon Sep 17 00:00:00 2001 From: Alper Rifat Ulucinar Date: Thu, 12 Jan 2023 02:56:04 +0300 Subject: [PATCH] crddiff: Treat field removal as a breaking API change Signed-off-by: Alper Rifat Ulucinar --- go.mod | 2 +- go.sum | 1 + internal/crdschema/crd.go | 87 ++++++++++++++++++++++++-- internal/crdschema/crd_test.go | 111 +++++++++++++++++++++++++++++++++ 4 files changed, 195 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index 658a5af..4c9b64f 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 81e5b33..1997cd5 100644 --- a/go.sum +++ b/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= diff --git a/internal/crdschema/crd.go b/internal/crdschema/crd.go index 916f626..91d6edd 100644 --- a/internal/crdschema/crd.go +++ b/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") } diff --git a/internal/crdschema/crd_test.go b/internal/crdschema/crd_test.go index 7d1ff71..6ea097d 100644 --- a/internal/crdschema/crd_test.go +++ b/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 +}