generate openapi: preserve manually modified/added CRD validation

internal/pkg/scaffold/crd.go

@@ -19,6 +19,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"reflect"
 	"strings"
 	"sync"
 
@@ -79,7 +80,10 @@ func initCache() {
 func (s *CRD) SetFS(_ afero.Fs) {}
 
 func (s *CRD) CustomRender() ([]byte, error) {
-	i, _ := s.GetInput()
+	i, err := s.GetInput()
+	if err != nil {
+		return nil, err
+	}
 	// controller-tools generates crd file names with no _crd.yaml suffix:
 	// <group>_<version>_<kind>.yaml.
 	path := strings.Replace(filepath.Base(i.Path), "_crd.yaml", ".yaml", 1)
@@ -132,7 +136,10 @@ func (s *CRD) CustomRender() ([]byte, error) {
 				if err = yaml.Unmarshal(cb, dstCRD); err != nil {
 					return nil, err
 				}
-				dstCRD.Spec.Validation = val
+				// val contains all validation fields from source code tagged with
+				// '+kubebuilder' directives. These fields should overwrite dstCRD
+				// validation fields except for those added manually, hence the merge.
+				mergeValidations(dstCRD.Spec.Validation, val)
 			}
 		}
 		// controller-tools does not set ListKind or Singular names.
@@ -216,3 +223,111 @@ func addCRDVersions(crd *apiextv1beta1.CustomResourceDefinition) {
 		crd.Spec.Versions = crdVersions
 	}
 }
+
+func mergeValidations(dstv, srcv *apiextv1beta1.CustomResourceValidation) {
+	mergeJSONSchemaProps(dstv.OpenAPIV3Schema, srcv.OpenAPIV3Schema)
+	return
+}
+
+func mergeJSONSchemaProps(dstp, srcp *apiextv1beta1.JSONSchemaProps) {
+	if dstp == nil || srcp == nil {
+		if srcp != nil {
+			dstp = srcp
+		}
+		return
+	}
+	if reflect.DeepEqual(dstp, srcp) {
+		return
+	}
+
+	prop := srcp.DeepCopy()
+	// Overwrite fields not writeable by kubebuilder directives with old values.
+	if prop.ID == "" {
+		prop.ID = dstp.ID
+	}
+	if prop.Schema == "" {
+		prop.Schema = dstp.Schema
+	}
+	if prop.Ref == nil {
+		prop.Ref = dstp.Ref
+	}
+	if prop.Description == "" {
+		prop.Description = dstp.Description
+	}
+	if prop.Title == "" {
+		prop.Title = dstp.Title
+	}
+	if prop.Default == nil {
+		prop.Default = dstp.Default
+	}
+	if prop.MaxProperties == nil {
+		prop.MaxProperties = dstp.MaxProperties
+	}
+	if prop.MinProperties == nil {
+		prop.MinProperties = dstp.MinProperties
+	}
+	if prop.ExternalDocs == nil {
+		prop.ExternalDocs = dstp.ExternalDocs
+	}
+	if prop.Example == nil {
+		prop.Example = dstp.Example
+	}
+	if prop.Items == nil {
+		prop.Items = dstp.Items
+	}
+	if prop.Not == nil {
+		prop.Not = dstp.Not
+	}
+	if prop.AdditionalProperties == nil {
+		prop.AdditionalProperties = dstp.AdditionalProperties
+	}
+	if prop.AdditionalItems == nil {
+		prop.AdditionalItems = dstp.AdditionalItems
+	}
+	if len(prop.Required) == 0 {
+		prop.Required = dstp.Required
+	}
+	if len(prop.AllOf) == 0 {
+		prop.AllOf = dstp.AllOf
+	}
+	if len(prop.OneOf) == 0 {
+		prop.OneOf = dstp.OneOf
+	}
+	if len(prop.AnyOf) == 0 {
+		prop.AnyOf = dstp.AnyOf
+	}
+	// Recursively merge the following fields until no sub schema props exist.
+	for dk, dv := range dstp.Properties {
+		if pv, ok := prop.Properties[dk]; ok {
+			mergeJSONSchemaProps(&dv, &pv)
+			prop.Properties[dk] = dv
+		}
+	}
+	for dk, dv := range dstp.PatternProperties {
+		if pv, ok := prop.PatternProperties[dk]; ok {
+			mergeJSONSchemaProps(&dv, &pv)
+			prop.PatternProperties[dk] = dv
+		}
+	}
+	for dk, dv := range dstp.Definitions {
+		if pv, ok := prop.Definitions[dk]; ok {
+			mergeJSONSchemaProps(&dv, &pv)
+			prop.Definitions[dk] = dv
+		}
+	}
+	for dk, dv := range dstp.Dependencies {
+		if pv, ok := prop.Dependencies[dk]; ok {
+			if dv.Schema != nil {
+				if pv.Schema != nil {
+					mergeJSONSchemaProps(dv.Schema, pv.Schema)
+				}
+				prop.Dependencies[dk] = dv
+			} else if len(dv.Property) != 0 && len(pv.Property) == 0 && pv.Schema == nil {
+				prop.Dependencies[dk] = dv
+			}
+		}
+	}
+
+	*dstp = *prop
+	return
+}

internal/pkg/scaffold/crd_test.go

@@ -17,11 +17,15 @@ package scaffold
 import (
 	"os"
 	"path/filepath"
+	"reflect"
 	"strings"
 	"testing"
 
 	"github.com/operator-framework/operator-sdk/internal/pkg/scaffold/input"
 	"github.com/operator-framework/operator-sdk/internal/util/diffutil"
+
+	"github.com/ghodss/yaml"
+	apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 )
 
 func TestCRDGoProject(t *testing.T) {
@@ -154,3 +158,179 @@ spec:
     served: true
     storage: true
 `
+
+func marshalAndGetStrings(a, b interface{}) (string, string, error) {
+	var (
+		ab, bb []byte
+		err    error
+	)
+	if ab, err = yaml.Marshal(a); err != nil {
+		return "", "", err
+	}
+	if bb, err = yaml.Marshal(b); err != nil {
+		return "", "", err
+	}
+	return string(ab), string(bb), nil
+}
+
+var baseCRDVal = &apiextv1beta1.CustomResourceValidation{
+	OpenAPIV3Schema: &apiextv1beta1.JSONSchemaProps{
+		Properties: map[string]apiextv1beta1.JSONSchemaProps{
+			"apiVersion": {
+				Description: `APIVersion defines the versioned schema of this representation of an object.`,
+				Type:        "string",
+			},
+			"kind": {
+				Description: `Kind is a string value representing the REST resource this object represents.`,
+				Type:        "string",
+			},
+			"metadata": {Type: "object"},
+			"spec": {
+				Properties: map[string]apiextv1beta1.JSONSchemaProps{
+					"size": {
+						Description: `Desired state of cluster.`,
+						Format:      "int32",
+						Type:        "integer",
+					},
+				},
+				Required: []string{"size"},
+				Type:     "object",
+			},
+			"status": {
+				Properties: map[string]apiextv1beta1.JSONSchemaProps{
+					"nodes": {
+						Description: `Define observed state of cluster.`,
+						Items:       &apiextv1beta1.JSONSchemaPropsOrArray{Schema: &apiextv1beta1.JSONSchemaProps{Type: "string"}},
+						Type:        "array",
+					},
+				},
+				Required: []string{"nodes"},
+				Type:     "object",
+			},
+		},
+	},
+}
+
+func TestMergeValidationsNoChange(t *testing.T) {
+	// Both should be the same.
+	dstCRDVal, srcCRDVal := baseCRDVal.DeepCopy(), baseCRDVal.DeepCopy()
+	mergeValidations(dstCRDVal, srcCRDVal)
+	if !reflect.DeepEqual(dstCRDVal, srcCRDVal) {
+		ts, bs, err := marshalAndGetStrings(dstCRDVal, srcCRDVal)
+		if err != nil {
+			t.Fatal(err)
+		}
+		t.Errorf("Expected vs actual differs.\n%v", diffutil.Diff(bs, ts))
+	}
+}
+
+func TestMergeValidationsUpdateDescription(t *testing.T) {
+	// apiVersion description should be that in src val.
+	dstCRDVal, srcCRDVal := baseCRDVal.DeepCopy(), baseCRDVal.DeepCopy()
+	apiv := dstCRDVal.OpenAPIV3Schema.Properties["apiVersion"]
+	apiv.Description = "foo bar"
+	dstCRDVal.OpenAPIV3Schema.Properties["apiVersion"] = apiv
+
+	mergeValidations(dstCRDVal, srcCRDVal)
+	if !reflect.DeepEqual(dstCRDVal, srcCRDVal) {
+		ds, ss, err := marshalAndGetStrings(dstCRDVal, srcCRDVal)
+		if err != nil {
+			t.Fatal(err)
+		}
+		t.Errorf("Expected vs actual differs.\n%v", diffutil.Diff(ss, ds))
+	}
+}
+
+func TestMergeValidationsUpdateMultiple(t *testing.T) {
+	// specs' size description should be that in src val.
+	// sizes' required should be that in dst val.
+	dstCRDVal, srcCRDVal := baseCRDVal.DeepCopy(), baseCRDVal.DeepCopy()
+	spec := dstCRDVal.OpenAPIV3Schema.Properties["spec"]
+	specSize := spec.Properties["size"]
+	oldSpecDesc := specSize.Description
+	specSize.Description = "new description"
+	newRequired := []string{"some prop"}
+	specSize.Required = newRequired
+	spec.Properties["size"] = specSize
+	dstCRDVal.OpenAPIV3Schema.Properties["spec"] = spec
+
+	mergeValidations(dstCRDVal, srcCRDVal)
+	if reflect.DeepEqual(dstCRDVal, srcCRDVal) {
+		t.Error("Expected vs actual should differ but do not.")
+	}
+	specSize = dstCRDVal.OpenAPIV3Schema.Properties["spec"].Properties["size"]
+	if specSize.Description != oldSpecDesc {
+		t.Errorf("Expected old description %s, got %s", oldSpecDesc, specSize.Description)
+	}
+	if !reflect.DeepEqual(specSize.Required, newRequired) {
+		t.Errorf("Expected new required %v, got %v", newRequired, specSize.Required)
+	}
+}
+
+func TestMergeValidationsAddNewDeleteOld(t *testing.T) {
+	// specs' size description should be that in src val.
+	// sizes' required should be that in dst val.
+	dstCRDVal, srcCRDVal := baseCRDVal.DeepCopy(), baseCRDVal.DeepCopy()
+
+	updatePropSpec := apiextv1beta1.JSONSchemaProps{
+		Properties: map[string]apiextv1beta1.JSONSchemaProps{
+			"shape": {
+				Description: `Desired shape of cluster.`,
+				Format:      "bool",
+				Type:        "boolean",
+			},
+			"curve": {
+				Title: "Some curve thing.",
+			},
+		},
+		Required: []string{"shape"},
+		Type:     "object",
+	}
+	newPropHammer := apiextv1beta1.JSONSchemaProps{
+		Properties: map[string]apiextv1beta1.JSONSchemaProps{
+			"nail": {
+				Type: "object",
+			},
+		},
+		Items: &apiextv1beta1.JSONSchemaPropsOrArray{Schema: &apiextv1beta1.JSONSchemaProps{Type: "string"}},
+		Type:  "object",
+	}
+
+	srcCRDVal.OpenAPIV3Schema.Properties["spec"] = updatePropSpec
+	srcCRDVal.OpenAPIV3Schema.Properties["hammer"] = newPropHammer
+	delete(srcCRDVal.OpenAPIV3Schema.Properties, "status")
+
+	mergeValidations(dstCRDVal, srcCRDVal)
+	if !reflect.DeepEqual(dstCRDVal, srcCRDVal) {
+		ds, ss, err := marshalAndGetStrings(dstCRDVal, srcCRDVal)
+		if err != nil {
+			t.Fatal(err)
+		}
+		t.Errorf("Expected vs actual differs.\n%v", diffutil.Diff(ss, ds))
+	}
+
+	spec := dstCRDVal.OpenAPIV3Schema.Properties["spec"]
+	if !reflect.DeepEqual(spec, updatePropSpec) {
+		ss, us, err := marshalAndGetStrings(spec, updatePropSpec)
+		if err != nil {
+			t.Fatal(err)
+		}
+		t.Errorf("Expected vs actual differs.\n%v", diffutil.Diff(us, ss))
+	}
+
+	if _, ok := dstCRDVal.OpenAPIV3Schema.Properties["status"]; ok {
+		t.Error("Expected no status field but still exists")
+	}
+
+	h, ok := dstCRDVal.OpenAPIV3Schema.Properties["hammer"]
+	if !ok {
+		t.Error("Expected hammer field but does not exist")
+	}
+	if !reflect.DeepEqual(h, newPropHammer) {
+		hs, us, err := marshalAndGetStrings(h, newPropHammer)
+		if err != nil {
+			t.Fatal(err)
+		}
+		t.Errorf("Expected vs actual differs.\n%v", diffutil.Diff(us, hs))
+	}
+}