diff --git a/internal/pkg/scaffold/crd.go b/internal/pkg/scaffold/crd.go index a0b0f253fb7..a50974ae553 100644 --- a/internal/pkg/scaffold/crd.go +++ b/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: // __.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 +} diff --git a/internal/pkg/scaffold/crd_test.go b/internal/pkg/scaffold/crd_test.go index 1f7cf958488..36e093aed98 100644 --- a/internal/pkg/scaffold/crd_test.go +++ b/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)) + } +}