From 20b16f23b03639bafa010d9dd54396033ba3dc03 Mon Sep 17 00:00:00 2001 From: Eric Stroczynski Date: Thu, 28 Feb 2019 12:15:37 -0800 Subject: [PATCH 1/5] pkg/scaffold/crd.go: merge CRD validation without overwriting manually modified fields --- pkg/scaffold/crd.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pkg/scaffold/crd.go b/pkg/scaffold/crd.go index a7ee0384c99..7b9dd267677 100644 --- a/pkg/scaffold/crd.go +++ b/pkg/scaffold/crd.go @@ -25,6 +25,7 @@ import ( "github.com/operator-framework/operator-sdk/pkg/scaffold/input" "github.com/ghodss/yaml" + "github.com/imdario/mergo" "github.com/spf13/afero" apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -132,7 +133,12 @@ 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 populate dstCRD + // validation fields unless they were added manually, hence the merge. + if err = mergo.Merge(dstCRD.Spec.Validation, val); err != nil { + return nil, err + } } } // controller-tools does not set ListKind or Singular names. From 3b21120e5f22046a6065f4aa5f7de67b841eae55 Mon Sep 17 00:00:00 2001 From: Eric Stroczynski Date: Thu, 28 Feb 2019 12:15:47 -0800 Subject: [PATCH 2/5] Gopkg.lock: revendor --- Gopkg.lock | 1 + 1 file changed, 1 insertion(+) diff --git a/Gopkg.lock b/Gopkg.lock index cf72455cd4a..95da612ba95 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -1589,6 +1589,7 @@ "github.com/go-logr/logr", "github.com/go-logr/zapr", "github.com/iancoleman/strcase", + "github.com/imdario/mergo", "github.com/markbates/inflect", "github.com/martinlindhe/base36", "github.com/mattbaird/jsonpatch", From ca8aed8491e116dd938fe5318bb9a2fe15375e3f Mon Sep 17 00:00:00 2001 From: Eric Stroczynski Date: Mon, 4 Mar 2019 13:57:17 -0800 Subject: [PATCH 3/5] pkg/scaffold/crd*.go: custom merge CRD fields --- pkg/scaffold/crd.go | 118 +++++++++++++++++++++++-- pkg/scaffold/crd_test.go | 180 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 292 insertions(+), 6 deletions(-) diff --git a/pkg/scaffold/crd.go b/pkg/scaffold/crd.go index 7b9dd267677..e802e4f78a7 100644 --- a/pkg/scaffold/crd.go +++ b/pkg/scaffold/crd.go @@ -19,13 +19,13 @@ import ( "io/ioutil" "os" "path/filepath" + "reflect" "strings" "sync" "github.com/operator-framework/operator-sdk/pkg/scaffold/input" "github.com/ghodss/yaml" - "github.com/imdario/mergo" "github.com/spf13/afero" apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -134,11 +134,9 @@ func (s *CRD) CustomRender() ([]byte, error) { return nil, err } // val contains all validation fields from source code tagged with - // +kubebuilder directives. These fields should populate dstCRD - // validation fields unless they were added manually, hence the merge. - if err = mergo.Merge(dstCRD.Spec.Validation, val); err != nil { - return nil, err - } + // '+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. @@ -233,3 +231,111 @@ func getCRDBytes(crd *apiextv1beta1.CustomResourceDefinition) ([]byte, error) { delete(crdMap, "status") return yaml.Marshal(&crdMap) } + +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 +} diff --git a/pkg/scaffold/crd_test.go b/pkg/scaffold/crd_test.go index f1566115a4c..387ba1c293f 100644 --- a/pkg/scaffold/crd_test.go +++ b/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/util/diffutil" "github.com/operator-framework/operator-sdk/pkg/scaffold/input" + + "github.com/ghodss/yaml" + apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" ) func TestCRDGoProject(t *testing.T) { @@ -148,3 +152,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)) + } +} From a972d598d06d6ef1f02de9855fb14895d253a096 Mon Sep 17 00:00:00 2001 From: Eric Stroczynski Date: Mon, 4 Mar 2019 14:48:21 -0800 Subject: [PATCH 4/5] Gopkg.lock: un-revendor --- Gopkg.lock | 1 - 1 file changed, 1 deletion(-) diff --git a/Gopkg.lock b/Gopkg.lock index 95da612ba95..cf72455cd4a 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -1589,7 +1589,6 @@ "github.com/go-logr/logr", "github.com/go-logr/zapr", "github.com/iancoleman/strcase", - "github.com/imdario/mergo", "github.com/markbates/inflect", "github.com/martinlindhe/base36", "github.com/mattbaird/jsonpatch", From 606ee272a921db363b90df546fc9807c8b66bfb9 Mon Sep 17 00:00:00 2001 From: Eric Stroczynski Date: Tue, 5 Mar 2019 09:13:03 -0800 Subject: [PATCH 5/5] check GetInput error --- pkg/scaffold/crd.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/scaffold/crd.go b/pkg/scaffold/crd.go index fa370f6932e..c6ebc83cdc9 100644 --- a/pkg/scaffold/crd.go +++ b/pkg/scaffold/crd.go @@ -80,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: // __.yaml. path := strings.Replace(filepath.Base(i.Path), "_crd.yaml", ".yaml", 1)