From 41e83d85dd7fd4d43b012df688e8579555c89edd Mon Sep 17 00:00:00 2001 From: Alper Rifat Ulucinar Date: Wed, 27 Dec 2023 14:50:21 +0300 Subject: [PATCH 01/11] Support for multiversion CRDs & conversion webhooks Signed-off-by: Alper Rifat Ulucinar --- pkg/config/conversion/conversions.go | 84 +++++++++++++ pkg/config/resource.go | 4 + pkg/examples/example.go | 2 +- pkg/pipeline/conversion_convertible.go | 112 ++++++++++++++++++ pkg/pipeline/conversion_hub.go | 62 ++++++++++ pkg/pipeline/run.go | 22 +++- pkg/pipeline/templates/controller.go.tmpl | 9 ++ .../templates/conversion_convertible.go.tmpl | 25 ++++ pkg/pipeline/templates/conversion_hub.go.tmpl | 14 +++ pkg/pipeline/templates/crd_types.go.tmpl | 3 +- pkg/pipeline/templates/embed.go | 12 ++ 11 files changed, 346 insertions(+), 3 deletions(-) create mode 100644 pkg/config/conversion/conversions.go create mode 100644 pkg/pipeline/conversion_convertible.go create mode 100644 pkg/pipeline/conversion_hub.go create mode 100644 pkg/pipeline/templates/conversion_convertible.go.tmpl create mode 100644 pkg/pipeline/templates/conversion_hub.go.tmpl diff --git a/pkg/config/conversion/conversions.go b/pkg/config/conversion/conversions.go new file mode 100644 index 00000000..d57fc1a9 --- /dev/null +++ b/pkg/config/conversion/conversions.go @@ -0,0 +1,84 @@ +// SPDX-FileCopyrightText: 2023 The Crossplane Authors +// +// SPDX-License-Identifier: Apache-2.0 + +package conversion + +import ( + "github.com/crossplane/crossplane-runtime/pkg/fieldpath" + "github.com/crossplane/crossplane-runtime/pkg/resource" + "github.com/pkg/errors" +) + +const ( + AllVersions = "*" +) + +const ( + pathObjectMeta = "ObjectMeta" +) + +type Conversion interface { + GetSourceVersion() string + GetTargetVersion() string +} + +type PavedConversion interface { + Conversion + ConvertPaved(src, target fieldpath.Paved) error +} + +type ManagedConversion interface { + Conversion + ConvertTerraformed(src, target resource.Managed) +} + +type baseConversion struct { + sourceVersion string + targetVersion string +} + +func newBaseConversion(sourceVersion, targetVersion string) baseConversion { + return baseConversion{ + sourceVersion: sourceVersion, + targetVersion: targetVersion, + } +} + +func (c *baseConversion) GetSourceVersion() string { + return c.sourceVersion +} + +func (c *baseConversion) GetTargetVersion() string { + return c.targetVersion +} + +type fieldCopy struct { + baseConversion + sourceField string + targetField string +} + +func (f *fieldCopy) ConvertPaved(src, target fieldpath.Paved) error { + v, err := src.GetValue(f.sourceField) + if err != nil { + return errors.Wrapf(err, "failed to get the field %q from the conversion source object", f.sourceField) + } + return errors.Wrapf(target.SetValue(f.targetField, v), "failed to set the field %q of the conversion target object", f.targetField) +} + +func NewObjectMetaConversion() Conversion { + return &fieldCopy{ + baseConversion: newBaseConversion(AllVersions, AllVersions), + sourceField: pathObjectMeta, + targetField: pathObjectMeta, + } +} + +func NewFieldRenameConversion(sourceVersion, sourceField, targetVersion, targetField string) Conversion { + return &fieldCopy{ + baseConversion: newBaseConversion(sourceVersion, targetVersion), + sourceField: sourceField, + targetField: targetField, + } +} diff --git a/pkg/config/resource.go b/pkg/config/resource.go index 1ad1e777..204d3fdd 100644 --- a/pkg/config/resource.go +++ b/pkg/config/resource.go @@ -9,6 +9,8 @@ import ( "fmt" "time" + "github.com/crossplane/upjet/pkg/config/conversion" + xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1" "github.com/crossplane/crossplane-runtime/pkg/fieldpath" "github.com/crossplane/crossplane-runtime/pkg/reconciler/managed" @@ -446,6 +448,8 @@ type Resource struct { // index notation (i.e., array/map components do not need indices). ServerSideApplyMergeStrategies ServerSideApplyMergeStrategies + Conversions []conversion.Conversion + // useNoForkClient indicates that a no-fork external client should // be generated instead of the Terraform CLI-forking client. useNoForkClient bool diff --git a/pkg/examples/example.go b/pkg/examples/example.go index 8c6981e6..363a3f46 100644 --- a/pkg/examples/example.go +++ b/pkg/examples/example.go @@ -185,7 +185,7 @@ func (eg *Generator) Generate(group, version string, r *config.Resource) error { // e.g. gvk = ec2/v1beta1/instance gvk := fmt.Sprintf("%s/%s/%s", groupPrefix, version, strings.ToLower(r.Kind)) pm := paveCRManifest(rm.Examples[0].Paved.UnstructuredContent(), r, rm.Examples[0].Name, group, version, gvk) - manifestDir := filepath.Join(eg.rootDir, "examples-generated", groupPrefix) + manifestDir := filepath.Join(eg.rootDir, "examples-generated", groupPrefix, r.Version) pm.ManifestPath = filepath.Join(manifestDir, fmt.Sprintf("%s.yaml", strings.ToLower(r.Kind))) eg.resources[fmt.Sprintf("%s.%s", r.Name, reference.Wildcard)] = pm return nil diff --git a/pkg/pipeline/conversion_convertible.go b/pkg/pipeline/conversion_convertible.go new file mode 100644 index 00000000..671a8c7d --- /dev/null +++ b/pkg/pipeline/conversion_convertible.go @@ -0,0 +1,112 @@ +// SPDX-FileCopyrightText: 2023 The Crossplane Authors +// +// SPDX-License-Identifier: Apache-2.0 + +package pipeline + +import ( + "fmt" + "go/types" + "os" + "path/filepath" + "regexp" + "strings" + + "github.com/muvaf/typewriter/pkg/wrapper" + "github.com/pkg/errors" + + "github.com/crossplane/upjet/pkg/pipeline/templates" +) + +var ( + regexTypeFile = regexp.MustCompile(`zz_(.+)_types.go`) +) + +// NewConversionConvertibleGenerator returns a new ConversionConvertibleGenerator. +func NewConversionConvertibleGenerator(pkg *types.Package, rootDir, group, version string) *ConversionConvertibleGenerator { + return &ConversionConvertibleGenerator{ + LocalDirectoryPath: filepath.Join(rootDir, "apis", strings.ToLower(strings.Split(group, ".")[0])), + LicenseHeaderPath: filepath.Join(rootDir, "hack", "boilerplate.go.txt"), + SpokeVersionsMap: make(map[string][]string), + pkg: pkg, + version: version, + } +} + +// ConversionConvertibleGenerator generates conversion methods implementing the +// conversion.Convertible interface on the CRD structs. +type ConversionConvertibleGenerator struct { + LocalDirectoryPath string + LicenseHeaderPath string + SpokeVersionsMap map[string][]string + + pkg *types.Package + version string +} + +// Generate writes generated conversion.Convertible interface functions +func (cg *ConversionConvertibleGenerator) Generate(cfgs []*terraformedInput) error { + entries, err := os.ReadDir(cg.LocalDirectoryPath) + if err != nil { + return errors.Wrapf(err, "cannot list the directory entries for the source folder %s while generating the conversion.Convertible interface functions", cg.LocalDirectoryPath) + } + + for _, e := range entries { + if !e.IsDir() || e.Name() == cg.version { + // we skip spoke generation for the current version as the assumption is + // the current CRD version is the hub version. + continue + } + trFile := wrapper.NewFile(cg.pkg.Path(), cg.pkg.Name(), templates.ConversionConvertibleTemplate, + wrapper.WithGenStatement(GenStatement), + wrapper.WithHeaderPath(cg.LicenseHeaderPath), + ) + filePath := filepath.Join(cg.LocalDirectoryPath, e.Name(), "zz_generated.conversion.go") + vars := map[string]any{ + "APIVersion": e.Name(), + } + + var resources []map[string]any + versionDir := filepath.Join(cg.LocalDirectoryPath, e.Name()) + files, err := os.ReadDir(versionDir) + if err != nil { + return errors.Wrapf(err, "cannot list the directory entries for the source folder %s while looking for the generated types", versionDir) + } + for _, f := range files { + if f.IsDir() { + continue + } + m := regexTypeFile.FindStringSubmatch(f.Name()) + if len(m) < 2 { + continue + } + c := findKindTerraformedInput(cfgs, m[1]) + if c == nil { + // type may not be available in the new version => + // no conversion is possible. + continue + } + resources = append(resources, map[string]any{ + "CRD": map[string]string{ + "Kind": c.Kind, + }, + }) + cg.SpokeVersionsMap[fmt.Sprintf("%s.%s", c.ShortGroup, c.Kind)] = append(cg.SpokeVersionsMap[c.Kind], filepath.Base(versionDir)) + } + + vars["Resources"] = resources + if err := trFile.Write(filePath, vars, os.ModePerm); err != nil { + return errors.Wrapf(err, "cannot write the generated conversion Hub functions file %s", filePath) + } + } + return nil +} + +func findKindTerraformedInput(cfgs []*terraformedInput, name string) *terraformedInput { + for _, c := range cfgs { + if name == strings.ToLower(c.Kind) { + return c + } + } + return nil +} diff --git a/pkg/pipeline/conversion_hub.go b/pkg/pipeline/conversion_hub.go new file mode 100644 index 00000000..1097bf73 --- /dev/null +++ b/pkg/pipeline/conversion_hub.go @@ -0,0 +1,62 @@ +// SPDX-FileCopyrightText: 2023 The Crossplane Authors +// +// SPDX-License-Identifier: Apache-2.0 + +package pipeline + +import ( + "go/types" + "os" + "path/filepath" + "strings" + + "github.com/muvaf/typewriter/pkg/wrapper" + "github.com/pkg/errors" + + "github.com/crossplane/upjet/pkg/pipeline/templates" +) + +// NewConversionHubGenerator returns a new ConversionHubGenerator. +func NewConversionHubGenerator(pkg *types.Package, rootDir, group, version string) *ConversionHubGenerator { + return &ConversionHubGenerator{ + LocalDirectoryPath: filepath.Join(rootDir, "apis", strings.ToLower(strings.Split(group, ".")[0]), version), + LicenseHeaderPath: filepath.Join(rootDir, "hack", "boilerplate.go.txt"), + pkg: pkg, + } +} + +// ConversionHubGenerator generates conversion methods implementing the +// conversion.Hub interface on the CRD structs. +type ConversionHubGenerator struct { + LocalDirectoryPath string + LicenseHeaderPath string + + pkg *types.Package +} + +// Generate writes generated conversion.Hub interface functions +func (cg *ConversionHubGenerator) Generate(cfgs []*terraformedInput, apiVersion string) error { + trFile := wrapper.NewFile(cg.pkg.Path(), cg.pkg.Name(), templates.ConversionHubTemplate, + wrapper.WithGenStatement(GenStatement), + wrapper.WithHeaderPath(cg.LicenseHeaderPath), + ) + filePath := filepath.Join(cg.LocalDirectoryPath, "zz_generated.conversion.go") + vars := map[string]any{ + "APIVersion": apiVersion, + } + resources := make([]map[string]any, len(cfgs)) + index := 0 + for _, cfg := range cfgs { + resources[index] = map[string]any{ + "CRD": map[string]string{ + "Kind": cfg.Kind, + }, + } + index++ + } + vars["Resources"] = resources + return errors.Wrapf( + trFile.Write(filePath, vars, os.ModePerm), + "cannot write the generated conversion Hub functions file %s", filePath, + ) +} diff --git a/pkg/pipeline/run.go b/pkg/pipeline/run.go index 4d707984..9a7f1519 100644 --- a/pkg/pipeline/run.go +++ b/pkg/pipeline/run.go @@ -94,6 +94,8 @@ func Run(pc *config.Provider, rootDir string) { //nolint:gocyclo versionGen := NewVersionGenerator(rootDir, pc.ModulePath, group, version) crdGen := NewCRDGenerator(versionGen.Package(), rootDir, pc.ShortName, group, version) tfGen := NewTerraformedGenerator(versionGen.Package(), rootDir, group, version) + conversionHubGen := NewConversionHubGenerator(versionGen.Package(), rootDir, group, version) + conversionConvertibleGen := NewConversionConvertibleGenerator(versionGen.Package(), rootDir, group, version) ctrlGen := NewControllerGenerator(rootDir, pc.ModulePath, group) for _, name := range sortedResources(resources) { @@ -127,10 +129,28 @@ func Run(pc *config.Provider, rootDir string) { //nolint:gocyclo panic(errors.Wrapf(err, "cannot generate terraformed for resource %s", group)) } + if err := conversionHubGen.Generate(tfResources, version); err != nil { + panic(errors.Wrapf(err, "cannot generate the conversion.Hub function for the resource group %q", group)) + } + + if err := conversionConvertibleGen.Generate(tfResources); err != nil { + panic(errors.Wrapf(err, "cannot generate the conversion.Convertible functions for the resource group %q", group)) + } + if err := versionGen.Generate(); err != nil { panic(errors.Wrap(err, "cannot generate version files")) } - apiVersionPkgList = append(apiVersionPkgList, versionGen.Package().Path()) + p := versionGen.Package().Path() + apiVersionPkgList = append(apiVersionPkgList, p) + for _, r := range resources { + // if there are spoke versions for the given group.Kind + if spokeVersions := conversionConvertibleGen.SpokeVersionsMap[fmt.Sprintf("%s.%s", r.ShortGroup, r.Kind)]; spokeVersions != nil { + base := filepath.Dir(p) + for _, sv := range spokeVersions { + apiVersionPkgList = append(apiVersionPkgList, filepath.Join(base, sv)) + } + } + } } } diff --git a/pkg/pipeline/templates/controller.go.tmpl b/pkg/pipeline/templates/controller.go.tmpl index e75d3714..8e526825 100644 --- a/pkg/pipeline/templates/controller.go.tmpl +++ b/pkg/pipeline/templates/controller.go.tmpl @@ -19,6 +19,7 @@ import ( "github.com/crossplane/upjet/pkg/controller/handler" tjcontroller "github.com/crossplane/upjet/pkg/controller" "github.com/crossplane/upjet/pkg/terraform" + "github.com/pkg/errors" ctrl "sigs.k8s.io/controller-runtime" {{ .Imports }} @@ -96,6 +97,14 @@ func Setup(mgr ctrl.Manager, o tjcontroller.Options) error { opts = append(opts, managed.WithManagementPolicies()) } {{- end}} + + // register webhooks for the kind {{ .TypePackageAlias }}{{ .CRD.Kind }} + if err := ctrl.NewWebhookManagedBy(mgr). + For(&{{ .TypePackageAlias }}{{ .CRD.Kind }}{}). + Complete(); err != nil { + return errors.Wrap(err, "cannot register webhook for the kind {{ .TypePackageAlias }}{{ .CRD.Kind }}") + } + r := managed.NewReconciler(mgr, xpresource.ManagedKind({{ .TypePackageAlias }}{{ .CRD.Kind }}_GroupVersionKind), opts...) return ctrl.NewControllerManagedBy(mgr). diff --git a/pkg/pipeline/templates/conversion_convertible.go.tmpl b/pkg/pipeline/templates/conversion_convertible.go.tmpl new file mode 100644 index 00000000..e635afb7 --- /dev/null +++ b/pkg/pipeline/templates/conversion_convertible.go.tmpl @@ -0,0 +1,25 @@ +// SPDX-FileCopyrightText: 2023 The Crossplane Authors +// +// SPDX-License-Identifier: Apache-2.0 + +{{ .Header }} + +{{ .GenStatement }} + +package {{ .APIVersion }} + +import ( + "sigs.k8s.io/controller-runtime/pkg/conversion" +) + +{{ range .Resources }} + // ConvertTo converts this {{ .CRD.Kind }} to the hub type. + func (tr *{{ .CRD.Kind }}) ConvertTo(dstRaw conversion.Hub) error { + return nil + } + + // ConvertFrom converts from the hub type to the {{ .CRD.Kind }} type. + func (tr *{{ .CRD.Kind }}) ConvertFrom(srcRaw conversion.Hub) error { + return nil + } +{{ end }} diff --git a/pkg/pipeline/templates/conversion_hub.go.tmpl b/pkg/pipeline/templates/conversion_hub.go.tmpl new file mode 100644 index 00000000..5747ebd5 --- /dev/null +++ b/pkg/pipeline/templates/conversion_hub.go.tmpl @@ -0,0 +1,14 @@ +// SPDX-FileCopyrightText: 2023 The Crossplane Authors +// +// SPDX-License-Identifier: Apache-2.0 + +{{ .Header }} + +{{ .GenStatement }} + +package {{ .APIVersion }} + +{{ range .Resources }} + // Hub marks this type as a conversion hub. + func (tr *{{ .CRD.Kind }}) Hub() {} +{{ end }} diff --git a/pkg/pipeline/templates/crd_types.go.tmpl b/pkg/pipeline/templates/crd_types.go.tmpl index a3f9adba..f5a7084b 100644 --- a/pkg/pipeline/templates/crd_types.go.tmpl +++ b/pkg/pipeline/templates/crd_types.go.tmpl @@ -41,13 +41,14 @@ type {{ .CRD.Kind }}Status struct { } // +kubebuilder:object:root=true +// +kubebuilder:subresource:status +// +kubebuilder:storageversion // {{ .CRD.Kind }} is the Schema for the {{ .CRD.Kind }}s API. {{ .CRD.Description }} // +kubebuilder:printcolumn:name="READY",type="string",JSONPath=".status.conditions[?(@.type=='Ready')].status" // +kubebuilder:printcolumn:name="SYNCED",type="string",JSONPath=".status.conditions[?(@.type=='Synced')].status" // +kubebuilder:printcolumn:name="EXTERNAL-NAME",type="string",JSONPath=".metadata.annotations.crossplane\\.io/external-name" // +kubebuilder:printcolumn:name="AGE",type="date",JSONPath=".metadata.creationTimestamp" -// +kubebuilder:subresource:status // +kubebuilder:resource:scope=Cluster,categories={crossplane,managed,{{ .Provider.ShortName }}}{{ if .CRD.Path }},path={{ .CRD.Path }}{{ end }} type {{ .CRD.Kind }} struct { metav1.TypeMeta `json:",inline"` diff --git a/pkg/pipeline/templates/embed.go b/pkg/pipeline/templates/embed.go index c809057a..0262fafe 100644 --- a/pkg/pipeline/templates/embed.go +++ b/pkg/pipeline/templates/embed.go @@ -36,3 +36,15 @@ var RegisterTemplate string // //go:embed setup.go.tmpl var SetupTemplate string + +// ConversionHubTemplate is populated with the CRD API versions +// conversion.Hub implementation template string. +// +//go:embed conversion_hub.go.tmpl +var ConversionHubTemplate string + +// ConversionConvertibleTemplate is populated with the CRD API versions +// conversion.Convertible implementation template string. +// +//go:embed conversion_convertible.go.tmpl +var ConversionConvertibleTemplate string From 97359b87185475c4ef7bc7023564ccc6aa1001c4 Mon Sep 17 00:00:00 2001 From: Alper Rifat Ulucinar Date: Fri, 5 Jan 2024 03:17:17 +0300 Subject: [PATCH 02/11] Rename ConversionConvertable as ConversionSpoke Signed-off-by: Alper Rifat Ulucinar --- ...nversion_convertible.go => conversion_spoke.go} | 14 +++++++------- pkg/pipeline/run.go | 6 +++--- ...onvertible.go.tmpl => conversion_spoke.go.tmpl} | 0 pkg/pipeline/templates/embed.go | 6 +++--- 4 files changed, 13 insertions(+), 13 deletions(-) rename pkg/pipeline/{conversion_convertible.go => conversion_spoke.go} (85%) rename pkg/pipeline/templates/{conversion_convertible.go.tmpl => conversion_spoke.go.tmpl} (100%) diff --git a/pkg/pipeline/conversion_convertible.go b/pkg/pipeline/conversion_spoke.go similarity index 85% rename from pkg/pipeline/conversion_convertible.go rename to pkg/pipeline/conversion_spoke.go index 671a8c7d..7bb1697e 100644 --- a/pkg/pipeline/conversion_convertible.go +++ b/pkg/pipeline/conversion_spoke.go @@ -22,9 +22,9 @@ var ( regexTypeFile = regexp.MustCompile(`zz_(.+)_types.go`) ) -// NewConversionConvertibleGenerator returns a new ConversionConvertibleGenerator. -func NewConversionConvertibleGenerator(pkg *types.Package, rootDir, group, version string) *ConversionConvertibleGenerator { - return &ConversionConvertibleGenerator{ +// NewConversionSpokeGenerator returns a new ConversionSpokeGenerator. +func NewConversionSpokeGenerator(pkg *types.Package, rootDir, group, version string) *ConversionSpokeGenerator { + return &ConversionSpokeGenerator{ LocalDirectoryPath: filepath.Join(rootDir, "apis", strings.ToLower(strings.Split(group, ".")[0])), LicenseHeaderPath: filepath.Join(rootDir, "hack", "boilerplate.go.txt"), SpokeVersionsMap: make(map[string][]string), @@ -33,9 +33,9 @@ func NewConversionConvertibleGenerator(pkg *types.Package, rootDir, group, versi } } -// ConversionConvertibleGenerator generates conversion methods implementing the +// ConversionSpokeGenerator generates conversion methods implementing the // conversion.Convertible interface on the CRD structs. -type ConversionConvertibleGenerator struct { +type ConversionSpokeGenerator struct { LocalDirectoryPath string LicenseHeaderPath string SpokeVersionsMap map[string][]string @@ -45,7 +45,7 @@ type ConversionConvertibleGenerator struct { } // Generate writes generated conversion.Convertible interface functions -func (cg *ConversionConvertibleGenerator) Generate(cfgs []*terraformedInput) error { +func (cg *ConversionSpokeGenerator) Generate(cfgs []*terraformedInput) error { entries, err := os.ReadDir(cg.LocalDirectoryPath) if err != nil { return errors.Wrapf(err, "cannot list the directory entries for the source folder %s while generating the conversion.Convertible interface functions", cg.LocalDirectoryPath) @@ -57,7 +57,7 @@ func (cg *ConversionConvertibleGenerator) Generate(cfgs []*terraformedInput) err // the current CRD version is the hub version. continue } - trFile := wrapper.NewFile(cg.pkg.Path(), cg.pkg.Name(), templates.ConversionConvertibleTemplate, + trFile := wrapper.NewFile(cg.pkg.Path(), cg.pkg.Name(), templates.ConversionSpokeTemplate, wrapper.WithGenStatement(GenStatement), wrapper.WithHeaderPath(cg.LicenseHeaderPath), ) diff --git a/pkg/pipeline/run.go b/pkg/pipeline/run.go index 9a7f1519..bc82d99b 100644 --- a/pkg/pipeline/run.go +++ b/pkg/pipeline/run.go @@ -95,7 +95,7 @@ func Run(pc *config.Provider, rootDir string) { //nolint:gocyclo crdGen := NewCRDGenerator(versionGen.Package(), rootDir, pc.ShortName, group, version) tfGen := NewTerraformedGenerator(versionGen.Package(), rootDir, group, version) conversionHubGen := NewConversionHubGenerator(versionGen.Package(), rootDir, group, version) - conversionConvertibleGen := NewConversionConvertibleGenerator(versionGen.Package(), rootDir, group, version) + conversionSpokeGen := NewConversionSpokeGenerator(versionGen.Package(), rootDir, group, version) ctrlGen := NewControllerGenerator(rootDir, pc.ModulePath, group) for _, name := range sortedResources(resources) { @@ -133,7 +133,7 @@ func Run(pc *config.Provider, rootDir string) { //nolint:gocyclo panic(errors.Wrapf(err, "cannot generate the conversion.Hub function for the resource group %q", group)) } - if err := conversionConvertibleGen.Generate(tfResources); err != nil { + if err := conversionSpokeGen.Generate(tfResources); err != nil { panic(errors.Wrapf(err, "cannot generate the conversion.Convertible functions for the resource group %q", group)) } @@ -144,7 +144,7 @@ func Run(pc *config.Provider, rootDir string) { //nolint:gocyclo apiVersionPkgList = append(apiVersionPkgList, p) for _, r := range resources { // if there are spoke versions for the given group.Kind - if spokeVersions := conversionConvertibleGen.SpokeVersionsMap[fmt.Sprintf("%s.%s", r.ShortGroup, r.Kind)]; spokeVersions != nil { + if spokeVersions := conversionSpokeGen.SpokeVersionsMap[fmt.Sprintf("%s.%s", r.ShortGroup, r.Kind)]; spokeVersions != nil { base := filepath.Dir(p) for _, sv := range spokeVersions { apiVersionPkgList = append(apiVersionPkgList, filepath.Join(base, sv)) diff --git a/pkg/pipeline/templates/conversion_convertible.go.tmpl b/pkg/pipeline/templates/conversion_spoke.go.tmpl similarity index 100% rename from pkg/pipeline/templates/conversion_convertible.go.tmpl rename to pkg/pipeline/templates/conversion_spoke.go.tmpl diff --git a/pkg/pipeline/templates/embed.go b/pkg/pipeline/templates/embed.go index 0262fafe..00cdb169 100644 --- a/pkg/pipeline/templates/embed.go +++ b/pkg/pipeline/templates/embed.go @@ -43,8 +43,8 @@ var SetupTemplate string //go:embed conversion_hub.go.tmpl var ConversionHubTemplate string -// ConversionConvertibleTemplate is populated with the CRD API versions +// ConversionSpokeTemplate is populated with the CRD API versions // conversion.Convertible implementation template string. // -//go:embed conversion_convertible.go.tmpl -var ConversionConvertibleTemplate string +//go:embed conversion_spoke.go.tmpl +var ConversionSpokeTemplate string From 3030fb0e3ed48323c92bf1199ee750360244654c Mon Sep 17 00:00:00 2001 From: Alper Rifat Ulucinar Date: Fri, 5 Jan 2024 03:35:46 +0300 Subject: [PATCH 03/11] Add round-trip version converter Signed-off-by: Alper Rifat Ulucinar --- pkg/controller/conversion/functions.go | 26 +++++++++++++++++++ .../templates/conversion_spoke.go.tmpl | 24 +++++++++++------ 2 files changed, 42 insertions(+), 8 deletions(-) create mode 100644 pkg/controller/conversion/functions.go diff --git a/pkg/controller/conversion/functions.go b/pkg/controller/conversion/functions.go new file mode 100644 index 00000000..44446522 --- /dev/null +++ b/pkg/controller/conversion/functions.go @@ -0,0 +1,26 @@ +// SPDX-FileCopyrightText: 2023 The Crossplane Authors +// +// SPDX-License-Identifier: Apache-2.0 + +package conversion + +import ( + "github.com/pkg/errors" + "k8s.io/apimachinery/pkg/runtime" +) + +// RoundTrip round-trips from `src` to `dst` via an unstructured map[string]any +// representation of the `src` object. +func RoundTrip(dst, src runtime.Object) error { + srcMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(src) + if err != nil { + return errors.Wrap(err, "cannot convert the conversion source object into the map[string]interface{} representation") + } + gvk := dst.GetObjectKind().GroupVersionKind() + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(srcMap, dst); err != nil { + return errors.Wrap(err, "cannot convert the map[string]interface{} representation to the conversion target object") + } + // restore the original GVK for the conversion destination + dst.GetObjectKind().SetGroupVersionKind(gvk) + return nil +} diff --git a/pkg/pipeline/templates/conversion_spoke.go.tmpl b/pkg/pipeline/templates/conversion_spoke.go.tmpl index e635afb7..2dfbabb1 100644 --- a/pkg/pipeline/templates/conversion_spoke.go.tmpl +++ b/pkg/pipeline/templates/conversion_spoke.go.tmpl @@ -9,17 +9,25 @@ package {{ .APIVersion }} import ( + ujconversion "github.com/crossplane/upjet/pkg/controller/conversion" + "github.com/pkg/errors" "sigs.k8s.io/controller-runtime/pkg/conversion" ) {{ range .Resources }} - // ConvertTo converts this {{ .CRD.Kind }} to the hub type. - func (tr *{{ .CRD.Kind }}) ConvertTo(dstRaw conversion.Hub) error { - return nil - } + // ConvertTo converts this {{ .CRD.Kind }} to the hub type. + func (tr *{{ .CRD.Kind }}) ConvertTo(dstRaw conversion.Hub) error { + if err := ujconversion.RoundTrip(dstRaw, tr); err != nil { + return errors.Wrap(err, "cannot convert a spoke version to the hub version") + } + return nil + } - // ConvertFrom converts from the hub type to the {{ .CRD.Kind }} type. - func (tr *{{ .CRD.Kind }}) ConvertFrom(srcRaw conversion.Hub) error { - return nil - } + // ConvertFrom converts from the hub type to the {{ .CRD.Kind }} type. + func (tr *{{ .CRD.Kind }}) ConvertFrom(srcRaw conversion.Hub) error { + if err := ujconversion.RoundTrip(tr, srcRaw); err != nil { + return errors.Wrap(err, "cannot convert the hub version to a spoke version") + } + return nil + } {{ end }} From e58e4b81d01a52a7c721145cc679a849a9d0289f Mon Sep 17 00:00:00 2001 From: Alper Rifat Ulucinar Date: Fri, 5 Jan 2024 05:51:19 +0300 Subject: [PATCH 04/11] Add webhook conversion registry Signed-off-by: Alper Rifat Ulucinar --- pkg/config/conversion/conversions.go | 54 ++++++++++--------- pkg/controller/conversion/functions.go | 43 +++++++++++++-- pkg/controller/conversion/registry.go | 46 ++++++++++++++++ .../templates/conversion_spoke.go.tmpl | 9 ++-- 4 files changed, 118 insertions(+), 34 deletions(-) create mode 100644 pkg/controller/conversion/registry.go diff --git a/pkg/config/conversion/conversions.go b/pkg/config/conversion/conversions.go index d57fc1a9..f1b03edc 100644 --- a/pkg/config/conversion/conversions.go +++ b/pkg/config/conversion/conversions.go @@ -8,29 +8,32 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/fieldpath" "github.com/crossplane/crossplane-runtime/pkg/resource" "github.com/pkg/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" ) const ( AllVersions = "*" ) -const ( - pathObjectMeta = "ObjectMeta" -) - type Conversion interface { - GetSourceVersion() string - GetTargetVersion() string + Applicable(src, dst runtime.Object) bool } type PavedConversion interface { Conversion - ConvertPaved(src, target fieldpath.Paved) error + // ConvertPaved converts from the `src` paved object to the `dst` + // paved object and returns `true` if the conversion has been done, + // `false` otherwise, together with any errors encountered. + ConvertPaved(src, target *fieldpath.Paved) (bool, error) } -type ManagedConversion interface { +type TerraformedConversion interface { Conversion - ConvertTerraformed(src, target resource.Managed) + // ConvertTerraformed converts from the `src` managed resource to the `dst` + // managed resource and returns `true` if the conversion has been done, + // `false` otherwise, together with any errors encountered. + ConvertTerraformed(src, target resource.Managed) (bool, error) } type baseConversion struct { @@ -45,12 +48,9 @@ func newBaseConversion(sourceVersion, targetVersion string) baseConversion { } } -func (c *baseConversion) GetSourceVersion() string { - return c.sourceVersion -} - -func (c *baseConversion) GetTargetVersion() string { - return c.targetVersion +func (c *baseConversion) Applicable(src, dst runtime.Object) bool { + return (c.sourceVersion == AllVersions || c.sourceVersion == src.GetObjectKind().GroupVersionKind().Version) && + (c.targetVersion == AllVersions || c.targetVersion == dst.GetObjectKind().GroupVersionKind().Version) } type fieldCopy struct { @@ -59,20 +59,22 @@ type fieldCopy struct { targetField string } -func (f *fieldCopy) ConvertPaved(src, target fieldpath.Paved) error { +func (f *fieldCopy) ConvertPaved(src, target *fieldpath.Paved) (bool, error) { + if !f.Applicable(&unstructured.Unstructured{Object: src.UnstructuredContent()}, + &unstructured.Unstructured{Object: target.UnstructuredContent()}) { + return false, nil + } v, err := src.GetValue(f.sourceField) - if err != nil { - return errors.Wrapf(err, "failed to get the field %q from the conversion source object", f.sourceField) + // TODO: the field might actually exist in the schema and + // missing in the object. Or, it may not exist in the schema. + // For a field that does not exist in the schema, we had better error. + if fieldpath.IsNotFound(err) { + return false, nil } - return errors.Wrapf(target.SetValue(f.targetField, v), "failed to set the field %q of the conversion target object", f.targetField) -} - -func NewObjectMetaConversion() Conversion { - return &fieldCopy{ - baseConversion: newBaseConversion(AllVersions, AllVersions), - sourceField: pathObjectMeta, - targetField: pathObjectMeta, + if err != nil { + return false, errors.Wrapf(err, "failed to get the field %q from the conversion source object", f.sourceField) } + return true, errors.Wrapf(target.SetValue(f.targetField, v), "failed to set the field %q of the conversion target object", f.targetField) } func NewFieldRenameConversion(sourceVersion, sourceField, targetVersion, targetField string) Conversion { diff --git a/pkg/controller/conversion/functions.go b/pkg/controller/conversion/functions.go index 44446522..0bdea0eb 100644 --- a/pkg/controller/conversion/functions.go +++ b/pkg/controller/conversion/functions.go @@ -5,22 +5,57 @@ package conversion import ( + "github.com/crossplane/crossplane-runtime/pkg/fieldpath" "github.com/pkg/errors" "k8s.io/apimachinery/pkg/runtime" + + "github.com/crossplane/upjet/pkg/config/conversion" + + "github.com/crossplane/upjet/pkg/resource" ) // RoundTrip round-trips from `src` to `dst` via an unstructured map[string]any -// representation of the `src` object. -func RoundTrip(dst, src runtime.Object) error { +// representation of the `src` object and applies the registered webhook +// conversion functions. +func RoundTrip(dst, src resource.Terraformed) error { srcMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(src) if err != nil { - return errors.Wrap(err, "cannot convert the conversion source object into the map[string]interface{} representation") + return errors.Wrap(err, "cannot convert the conversion source object into the map[string]any representation") } gvk := dst.GetObjectKind().GroupVersionKind() if err := runtime.DefaultUnstructuredConverter.FromUnstructured(srcMap, dst); err != nil { - return errors.Wrap(err, "cannot convert the map[string]interface{} representation to the conversion target object") + return errors.Wrap(err, "cannot convert the map[string]any representation of the source object to the conversion target object") } // restore the original GVK for the conversion destination dst.GetObjectKind().SetGroupVersionKind(gvk) + + // now we will try to run the registered webhook conversions + dstMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(dst) + if err != nil { + return errors.Wrap(err, "cannot convert the conversion destination object into the map[string]any representation") + } + srcPaved := fieldpath.Pave(srcMap) + dstPaved := fieldpath.Pave(dstMap) + for _, c := range GetConversions(dst) { + if pc, ok := c.(conversion.PavedConversion); ok { + if _, err := pc.ConvertPaved(srcPaved, dstPaved); err != nil { + return errors.Wrapf(err, "cannot apply the PavedConversion for the %q object", dst.GetTerraformResourceType()) + } + } + } + // convert the map[string]any representation of the conversion target back to + // the original type. + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(dstMap, dst); err != nil { + return errors.Wrap(err, "cannot convert the map[string]any representation of the conversion target object to the target object") + } + + for _, c := range GetConversions(dst) { + if tc, ok := c.(conversion.TerraformedConversion); ok { + if _, err := tc.ConvertTerraformed(src, dst); err != nil { + return errors.Wrapf(err, "cannot apply the TerraformedConversion for the %q object", dst.GetTerraformResourceType()) + } + } + } + return nil } diff --git a/pkg/controller/conversion/registry.go b/pkg/controller/conversion/registry.go new file mode 100644 index 00000000..6412c082 --- /dev/null +++ b/pkg/controller/conversion/registry.go @@ -0,0 +1,46 @@ +// SPDX-FileCopyrightText: 2023 The Crossplane Authors +// +// SPDX-License-Identifier: Apache-2.0 + +package conversion + +import ( + "github.com/pkg/errors" + + "github.com/crossplane/upjet/pkg/config" + "github.com/crossplane/upjet/pkg/config/conversion" + "github.com/crossplane/upjet/pkg/resource" +) + +const ( + errAlreadyRegistered = "conversion functions are already registered" +) + +var instance *registry + +// registry represents the conversion hook registry for a provider. +type registry struct { + provider *config.Provider +} + +// RegisterConversions registers the API version conversions from the specified +// provider configuration. +func RegisterConversions(provider *config.Provider) error { + if instance != nil { + return errors.New(errAlreadyRegistered) + } + instance = ®istry{ + provider: provider, + } + return nil +} + +// GetConversions returns the conversion.Conversions registered for the +// Terraformed resource. +func GetConversions(tr resource.Terraformed) []conversion.Conversion { + t := tr.GetTerraformResourceType() + if instance == nil || instance.provider == nil || instance.provider.Resources[t] == nil { + return nil + } + return instance.provider.Resources[t].Conversions +} diff --git a/pkg/pipeline/templates/conversion_spoke.go.tmpl b/pkg/pipeline/templates/conversion_spoke.go.tmpl index 2dfbabb1..c76577a3 100644 --- a/pkg/pipeline/templates/conversion_spoke.go.tmpl +++ b/pkg/pipeline/templates/conversion_spoke.go.tmpl @@ -10,6 +10,7 @@ package {{ .APIVersion }} import ( ujconversion "github.com/crossplane/upjet/pkg/controller/conversion" + "github.com/crossplane/upjet/pkg/resource" "github.com/pkg/errors" "sigs.k8s.io/controller-runtime/pkg/conversion" ) @@ -17,16 +18,16 @@ import ( {{ range .Resources }} // ConvertTo converts this {{ .CRD.Kind }} to the hub type. func (tr *{{ .CRD.Kind }}) ConvertTo(dstRaw conversion.Hub) error { - if err := ujconversion.RoundTrip(dstRaw, tr); err != nil { - return errors.Wrap(err, "cannot convert a spoke version to the hub version") + if err := ujconversion.RoundTrip(dstRaw.(resource.Terraformed), tr); err != nil { + return errors.Wrapf(err, "cannot convert from the spoke version %q to the hub version %q", tr.GetObjectKind().GroupVersionKind().Version, dstRaw.GetObjectKind().GroupVersionKind().Version) } return nil } // ConvertFrom converts from the hub type to the {{ .CRD.Kind }} type. func (tr *{{ .CRD.Kind }}) ConvertFrom(srcRaw conversion.Hub) error { - if err := ujconversion.RoundTrip(tr, srcRaw); err != nil { - return errors.Wrap(err, "cannot convert the hub version to a spoke version") + if err := ujconversion.RoundTrip(tr, srcRaw.(resource.Terraformed)); err != nil { + return errors.Wrapf(err, "cannot convert from the hub version %q to the spoke version %q", srcRaw.GetObjectKind().GroupVersionKind().Version, tr.GetObjectKind().GroupVersionKind().Version) } return nil } From 584a442e1517f8e858a8fa9d244a23d700f05834 Mon Sep 17 00:00:00 2001 From: Alper Rifat Ulucinar Date: Fri, 5 Jan 2024 14:36:41 +0300 Subject: [PATCH 05/11] Fix linter issues Signed-off-by: Alper Rifat Ulucinar --- pkg/config/resource.go | 3 +-- pkg/controller/conversion/functions.go | 3 +-- pkg/pipeline/conversion_spoke.go | 7 ++++--- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/pkg/config/resource.go b/pkg/config/resource.go index 204d3fdd..d6f9fa2c 100644 --- a/pkg/config/resource.go +++ b/pkg/config/resource.go @@ -9,8 +9,6 @@ import ( "fmt" "time" - "github.com/crossplane/upjet/pkg/config/conversion" - xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1" "github.com/crossplane/crossplane-runtime/pkg/fieldpath" "github.com/crossplane/crossplane-runtime/pkg/reconciler/managed" @@ -23,6 +21,7 @@ import ( "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/crossplane/upjet/pkg/config/conversion" "github.com/crossplane/upjet/pkg/registry" ) diff --git a/pkg/controller/conversion/functions.go b/pkg/controller/conversion/functions.go index 0bdea0eb..0b6c30a9 100644 --- a/pkg/controller/conversion/functions.go +++ b/pkg/controller/conversion/functions.go @@ -10,14 +10,13 @@ import ( "k8s.io/apimachinery/pkg/runtime" "github.com/crossplane/upjet/pkg/config/conversion" - "github.com/crossplane/upjet/pkg/resource" ) // RoundTrip round-trips from `src` to `dst` via an unstructured map[string]any // representation of the `src` object and applies the registered webhook // conversion functions. -func RoundTrip(dst, src resource.Terraformed) error { +func RoundTrip(dst, src resource.Terraformed) error { //nolint:gocyclo // considered breaking this according to the converters and I did not like it srcMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(src) if err != nil { return errors.Wrap(err, "cannot convert the conversion source object into the map[string]any representation") diff --git a/pkg/pipeline/conversion_spoke.go b/pkg/pipeline/conversion_spoke.go index 7bb1697e..8bb9dc38 100644 --- a/pkg/pipeline/conversion_spoke.go +++ b/pkg/pipeline/conversion_spoke.go @@ -45,7 +45,7 @@ type ConversionSpokeGenerator struct { } // Generate writes generated conversion.Convertible interface functions -func (cg *ConversionSpokeGenerator) Generate(cfgs []*terraformedInput) error { +func (cg *ConversionSpokeGenerator) Generate(cfgs []*terraformedInput) error { //nolint:gocyclo entries, err := os.ReadDir(cg.LocalDirectoryPath) if err != nil { return errors.Wrapf(err, "cannot list the directory entries for the source folder %s while generating the conversion.Convertible interface functions", cg.LocalDirectoryPath) @@ -91,7 +91,8 @@ func (cg *ConversionSpokeGenerator) Generate(cfgs []*terraformedInput) error { "Kind": c.Kind, }, }) - cg.SpokeVersionsMap[fmt.Sprintf("%s.%s", c.ShortGroup, c.Kind)] = append(cg.SpokeVersionsMap[c.Kind], filepath.Base(versionDir)) + sk := fmt.Sprintf("%s.%s", c.ShortGroup, c.Kind) + cg.SpokeVersionsMap[sk] = append(cg.SpokeVersionsMap[sk], filepath.Base(versionDir)) } vars["Resources"] = resources @@ -104,7 +105,7 @@ func (cg *ConversionSpokeGenerator) Generate(cfgs []*terraformedInput) error { func findKindTerraformedInput(cfgs []*terraformedInput, name string) *terraformedInput { for _, c := range cfgs { - if name == strings.ToLower(c.Kind) { + if strings.EqualFold(name, c.Kind) { return c } } From a5a315e118360912b51aec1ecfa927b808d89f78 Mon Sep 17 00:00:00 2001 From: Alper Rifat Ulucinar Date: Tue, 9 Jan 2024 23:56:10 +0300 Subject: [PATCH 06/11] Differentiate the names of the generated conversion hub and spoke files - Do not generate empty conversion hub or spoke files. Signed-off-by: Alper Rifat Ulucinar --- pkg/pipeline/conversion_hub.go | 5 ++++- pkg/pipeline/conversion_spoke.go | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/pkg/pipeline/conversion_hub.go b/pkg/pipeline/conversion_hub.go index 1097bf73..f04372b8 100644 --- a/pkg/pipeline/conversion_hub.go +++ b/pkg/pipeline/conversion_hub.go @@ -40,7 +40,7 @@ func (cg *ConversionHubGenerator) Generate(cfgs []*terraformedInput, apiVersion wrapper.WithGenStatement(GenStatement), wrapper.WithHeaderPath(cg.LicenseHeaderPath), ) - filePath := filepath.Join(cg.LocalDirectoryPath, "zz_generated.conversion.go") + filePath := filepath.Join(cg.LocalDirectoryPath, "zz_generated.conversion_hubs.go") vars := map[string]any{ "APIVersion": apiVersion, } @@ -55,6 +55,9 @@ func (cg *ConversionHubGenerator) Generate(cfgs []*terraformedInput, apiVersion index++ } vars["Resources"] = resources + if len(resources) == 0 { + return nil + } return errors.Wrapf( trFile.Write(filePath, vars, os.ModePerm), "cannot write the generated conversion Hub functions file %s", filePath, diff --git a/pkg/pipeline/conversion_spoke.go b/pkg/pipeline/conversion_spoke.go index 8bb9dc38..1811b111 100644 --- a/pkg/pipeline/conversion_spoke.go +++ b/pkg/pipeline/conversion_spoke.go @@ -61,7 +61,7 @@ func (cg *ConversionSpokeGenerator) Generate(cfgs []*terraformedInput) error { / wrapper.WithGenStatement(GenStatement), wrapper.WithHeaderPath(cg.LicenseHeaderPath), ) - filePath := filepath.Join(cg.LocalDirectoryPath, e.Name(), "zz_generated.conversion.go") + filePath := filepath.Join(cg.LocalDirectoryPath, e.Name(), "zz_generated.conversion_spokes.go") vars := map[string]any{ "APIVersion": e.Name(), } @@ -96,6 +96,9 @@ func (cg *ConversionSpokeGenerator) Generate(cfgs []*terraformedInput) error { / } vars["Resources"] = resources + if len(resources) == 0 { + continue + } if err := trFile.Write(filePath, vars, os.ModePerm); err != nil { return errors.Wrapf(err, "cannot write the generated conversion Hub functions file %s", filePath) } From 97ce3ac9a81542fa0df6a4d382f86bbc8c9b949a Mon Sep 17 00:00:00 2001 From: Alper Rifat Ulucinar Date: Tue, 23 Jan 2024 17:04:34 +0300 Subject: [PATCH 07/11] Only start the conversion webhooks if they are enabled via the controller.Options.StartWebhooks Signed-off-by: Alper Rifat Ulucinar --- pkg/controller/options.go | 4 ++++ pkg/pipeline/templates/controller.go.tmpl | 7 +++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/pkg/controller/options.go b/pkg/controller/options.go index 353ef866..1bba55cb 100644 --- a/pkg/controller/options.go +++ b/pkg/controller/options.go @@ -45,6 +45,10 @@ type Options struct { // PollJitter adds the specified jitter to the configured reconcile period // of the up-to-date resources in managed.Reconciler. PollJitter time.Duration + + // StartWebhooks enables starting of the conversion webhooks by the + // provider's controllerruntime.Manager. + StartWebhooks bool } // ESSOptions for External Secret Stores. diff --git a/pkg/pipeline/templates/controller.go.tmpl b/pkg/pipeline/templates/controller.go.tmpl index 8e526825..5b584dc2 100644 --- a/pkg/pipeline/templates/controller.go.tmpl +++ b/pkg/pipeline/templates/controller.go.tmpl @@ -99,10 +99,13 @@ func Setup(mgr ctrl.Manager, o tjcontroller.Options) error { {{- end}} // register webhooks for the kind {{ .TypePackageAlias }}{{ .CRD.Kind }} - if err := ctrl.NewWebhookManagedBy(mgr). + // if they're enabled. + if o.StartWebhooks { + if err := ctrl.NewWebhookManagedBy(mgr). For(&{{ .TypePackageAlias }}{{ .CRD.Kind }}{}). Complete(); err != nil { - return errors.Wrap(err, "cannot register webhook for the kind {{ .TypePackageAlias }}{{ .CRD.Kind }}") + return errors.Wrap(err, "cannot register webhook for the kind {{ .TypePackageAlias }}{{ .CRD.Kind }}") + } } r := managed.NewReconciler(mgr, xpresource.ManagedKind({{ .TypePackageAlias }}{{ .CRD.Kind }}_GroupVersionKind), opts...) From 77fba4711900c85b3ddc21ffe12b0a636b01ce3c Mon Sep 17 00:00:00 2001 From: Alper Rifat Ulucinar Date: Thu, 25 Jan 2024 22:04:01 +0300 Subject: [PATCH 08/11] Add unit tests for the conversion package Signed-off-by: Alper Rifat Ulucinar --- pkg/config/conversion/conversions.go | 32 +++++ pkg/config/conversion/conversions_test.go | 149 ++++++++++++++++++++++ 2 files changed, 181 insertions(+) create mode 100644 pkg/config/conversion/conversions_test.go diff --git a/pkg/config/conversion/conversions.go b/pkg/config/conversion/conversions.go index f1b03edc..2b22ea09 100644 --- a/pkg/config/conversion/conversions.go +++ b/pkg/config/conversion/conversions.go @@ -13,13 +13,34 @@ import ( ) const ( + // AllVersions denotes that a Conversion is applicable for all versions + // of an API with which the Conversion is registered. It can be used for + // both the conversion source or target API versions. AllVersions = "*" ) +// Conversion is the interface for the API version converters. +// Conversion implementations registered for a source, target +// pair are called in chain so Conversion implementations can be modular, e.g., +// a Conversion implementation registered for a specific source and target +// versions does not have to contain all the needed API conversions between +// these two versions. type Conversion interface { + // Applicable should return true if this Conversion is applicable while + // converting the API of the `src` object to the API of the `dst` object. Applicable(src, dst runtime.Object) bool } +// PavedConversion is an optimized Conversion between two fieldpath.Paved +// objects. PavedConversion implementations for a specific source and target +// version pair are chained together and the source and the destination objects +// are paved once at the beginning of the chained PavedConversion.ConvertPaved +// calls. The target fieldpath.Paved object is then converted into the original +// resource.Terraformed object at the end of the chained calls. This prevents +// the intermediate conversions between fieldpath.Paved and +// the resource.Terraformed representations of the same object, and the +// fieldpath.Paved representation is convenient for writing generic +// Conversion implementations not bound to a specific type. type PavedConversion interface { Conversion // ConvertPaved converts from the `src` paved object to the `dst` @@ -28,6 +49,12 @@ type PavedConversion interface { ConvertPaved(src, target *fieldpath.Paved) (bool, error) } +// TerraformedConversion defines a Conversion from a specific source +// resource.Terraformed type to a target one. Generic Conversion +// implementations may prefer to implement the PavedConversion interface. +// Implementations of TerraformedConversion can do type assertions to +// specific source and target types and so they are expected to be +// strongly typed. type TerraformedConversion interface { Conversion // ConvertTerraformed converts from the `src` managed resource to the `dst` @@ -77,6 +104,11 @@ func (f *fieldCopy) ConvertPaved(src, target *fieldpath.Paved) (bool, error) { return true, errors.Wrapf(target.SetValue(f.targetField, v), "failed to set the field %q of the conversion target object", f.targetField) } +// NewFieldRenameConversion returns a new Conversion that implements a +// field renaming conversion from the specified `sourceVersion` to the specified +// `targetVersion` of an API. The field's name in the `sourceVersion` is given +// with the `sourceField` parameter and its name in the `targetVersion` is +// given with `targetField` parameter. func NewFieldRenameConversion(sourceVersion, sourceField, targetVersion, targetField string) Conversion { return &fieldCopy{ baseConversion: newBaseConversion(sourceVersion, targetVersion), diff --git a/pkg/config/conversion/conversions_test.go b/pkg/config/conversion/conversions_test.go new file mode 100644 index 00000000..33f09a70 --- /dev/null +++ b/pkg/config/conversion/conversions_test.go @@ -0,0 +1,149 @@ +// SPDX-FileCopyrightText: 2023 The Crossplane Authors +// +// SPDX-License-Identifier: Apache-2.0 + +package conversion + +import ( + "fmt" + "testing" + + "github.com/crossplane/crossplane-runtime/pkg/test" + "github.com/google/go-cmp/cmp" + "k8s.io/utils/ptr" + + "github.com/crossplane/crossplane-runtime/pkg/fieldpath" +) + +const ( + sourceVersion = "v1beta1" + sourceField = "testSourceField" + targetVersion = "v1beta2" + targetField = "testTargetField" +) + +func TestConvertPaved(t *testing.T) { + type args struct { + sourceVersion string + sourceField string + targetVersion string + targetField string + sourceObj *fieldpath.Paved + targetObj *fieldpath.Paved + } + type want struct { + converted bool + err error + targetObj *fieldpath.Paved + } + tests := map[string]struct { + reason string + args args + want want + }{ + "SuccessfulConversion": { + reason: "Source field in source version is successfully converted to the target field in target version.", + args: args{ + sourceVersion: sourceVersion, + sourceField: sourceField, + targetVersion: targetVersion, + targetField: targetField, + sourceObj: getPaved(sourceVersion, sourceField, ptr.To("testValue")), + targetObj: getPaved(targetVersion, targetField, nil), + }, + want: want{ + converted: true, + targetObj: getPaved(targetVersion, targetField, ptr.To("testValue")), + }, + }, + "SuccessfulConversionAllVersions": { + reason: "Source field in source version is successfully converted to the target field in target version when the conversion specifies wildcard version for both of the source and the target.", + args: args{ + sourceVersion: AllVersions, + sourceField: sourceField, + targetVersion: AllVersions, + targetField: targetField, + sourceObj: getPaved(sourceVersion, sourceField, ptr.To("testValue")), + targetObj: getPaved(targetVersion, targetField, nil), + }, + want: want{ + converted: true, + targetObj: getPaved(targetVersion, targetField, ptr.To("testValue")), + }, + }, + "SourceVersionMismatch": { + reason: "Conversion is not done if the source version of the object does not match the conversion's source version.", + args: args{ + sourceVersion: "mismatch", + sourceField: sourceField, + targetVersion: AllVersions, + targetField: targetField, + sourceObj: getPaved(sourceVersion, sourceField, ptr.To("testValue")), + targetObj: getPaved(targetVersion, targetField, nil), + }, + want: want{ + converted: false, + targetObj: getPaved(targetVersion, targetField, nil), + }, + }, + "TargetVersionMismatch": { + reason: "Conversion is not done if the target version of the object does not match the conversion's target version.", + args: args{ + sourceVersion: AllVersions, + sourceField: sourceField, + targetVersion: "mismatch", + targetField: targetField, + sourceObj: getPaved(sourceVersion, sourceField, ptr.To("testValue")), + targetObj: getPaved(targetVersion, targetField, nil), + }, + want: want{ + converted: false, + targetObj: getPaved(targetVersion, targetField, nil), + }, + }, + "SourceFieldNotFound": { + reason: "Conversion is not done if the source field is not found in the source object.", + args: args{ + sourceVersion: sourceVersion, + sourceField: sourceField, + targetVersion: targetVersion, + targetField: targetField, + sourceObj: getPaved(sourceVersion, sourceField, nil), + targetObj: getPaved(targetVersion, targetField, ptr.To("test")), + }, + want: want{ + converted: false, + targetObj: getPaved(targetVersion, targetField, ptr.To("test")), + }, + }, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + c := NewFieldRenameConversion(tc.args.sourceVersion, tc.args.sourceField, tc.args.targetVersion, tc.args.targetField) + converted, err := c.(*fieldCopy).ConvertPaved(tc.args.sourceObj, tc.args.targetObj) + if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + t.Errorf("\n%s\nConvertPaved(sourceObj, targetObj): -wantErr, +gotErr:\n%s", tc.reason, diff) + } + if tc.want.err != nil { + return + } + if diff := cmp.Diff(tc.want.converted, converted); diff != "" { + t.Errorf("\n%s\nConvertPaved(sourceObj, targetObj): -wantConverted, +gotConverted:\n%s", tc.reason, diff) + } + if diff := cmp.Diff(tc.want.targetObj.UnstructuredContent(), tc.args.targetObj.UnstructuredContent()); diff != "" { + t.Errorf("\n%s\nConvertPaved(sourceObj, targetObj): -wantTargetObj, +gotTargetObj:\n%s", tc.reason, diff) + } + }) + } +} + +func getPaved(version, field string, value *string) *fieldpath.Paved { + m := map[string]any{ + "apiVersion": fmt.Sprintf("mockgroup/%s", version), + "kind": "mockkind", + } + if value != nil { + m[field] = *value + } + return fieldpath.Pave(m) +} From b9d2b305d7cdd853acb50ffce5e001f6cbbb0ab6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sergen=20Yal=C3=A7=C4=B1n?= Date: Thu, 25 Jan 2024 15:00:10 +0300 Subject: [PATCH 09/11] Add customConverter struct for supporting the Custom Converters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Sergen Yalçın --- pkg/config/conversion/conversions.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/pkg/config/conversion/conversions.go b/pkg/config/conversion/conversions.go index 2b22ea09..0b327ffd 100644 --- a/pkg/config/conversion/conversions.go +++ b/pkg/config/conversion/conversions.go @@ -116,3 +116,27 @@ func NewFieldRenameConversion(sourceVersion, sourceField, targetVersion, targetF targetField: targetField, } } + +type customConverter func(src, target resource.Managed) error + +type customConversion struct { + baseConversion + customConverter customConverter +} + +func (cc *customConversion) ConvertTerraformed(src, target resource.Managed) (bool, error) { + if !cc.Applicable(src, target) { + return false, nil + } + if err := cc.customConverter(src, target); err != nil { + return false, err + } + return true, nil +} + +func NewCustomConverter(sourceVersion, targetVersion string, converter func(src, target resource.Managed) error) Conversion { + return &customConversion{ + baseConversion: newBaseConversion(sourceVersion, targetVersion), + customConverter: converter, + } +} From a85dded45958447682ca189c80e0b06d06ce93da Mon Sep 17 00:00:00 2001 From: Alper Rifat Ulucinar Date: Fri, 26 Jan 2024 00:18:19 +0300 Subject: [PATCH 10/11] Rename conversion.TerraformedConversion to conversion.ManagedConversion - Add unit tests for conversion.RoundTrip Signed-off-by: Alper Rifat Ulucinar --- pkg/config/conversion/conversions.go | 27 ++--- pkg/config/conversion/conversions_test.go | 3 +- pkg/controller/conversion/functions.go | 19 ++-- pkg/controller/conversion/functions_test.go | 103 ++++++++++++++++++++ pkg/controller/conversion/registry.go | 36 ++++--- pkg/resource/fake/terraformed.go | 30 ++++++ 6 files changed, 186 insertions(+), 32 deletions(-) create mode 100644 pkg/controller/conversion/functions_test.go diff --git a/pkg/config/conversion/conversions.go b/pkg/config/conversion/conversions.go index 0b327ffd..ba5a2da1 100644 --- a/pkg/config/conversion/conversions.go +++ b/pkg/config/conversion/conversions.go @@ -49,18 +49,18 @@ type PavedConversion interface { ConvertPaved(src, target *fieldpath.Paved) (bool, error) } -// TerraformedConversion defines a Conversion from a specific source -// resource.Terraformed type to a target one. Generic Conversion +// ManagedConversion defines a Conversion from a specific source +// resource.Managed type to a target one. Generic Conversion // implementations may prefer to implement the PavedConversion interface. -// Implementations of TerraformedConversion can do type assertions to -// specific source and target types and so they are expected to be +// Implementations of ManagedConversion can do type assertions to +// specific source and target types, and so, they are expected to be // strongly typed. -type TerraformedConversion interface { +type ManagedConversion interface { Conversion - // ConvertTerraformed converts from the `src` managed resource to the `dst` + // ConvertManaged converts from the `src` managed resource to the `dst` // managed resource and returns `true` if the conversion has been done, // `false` otherwise, together with any errors encountered. - ConvertTerraformed(src, target resource.Managed) (bool, error) + ConvertManaged(src, target resource.Managed) (bool, error) } type baseConversion struct { @@ -124,16 +124,17 @@ type customConversion struct { customConverter customConverter } -func (cc *customConversion) ConvertTerraformed(src, target resource.Managed) (bool, error) { - if !cc.Applicable(src, target) { +func (cc *customConversion) ConvertManaged(src, target resource.Managed) (bool, error) { + if !cc.Applicable(src, target) || cc.customConverter == nil { return false, nil } - if err := cc.customConverter(src, target); err != nil { - return false, err - } - return true, nil + return true, errors.Wrap(cc.customConverter(src, target), "failed to apply the converter function") } +// NewCustomConverter returns a new Conversion from the specified +// `sourceVersion` of an API to the specified `targetVersion` and invokes +// the specified converter function to perform the conversion on the +// managed resources. func NewCustomConverter(sourceVersion, targetVersion string, converter func(src, target resource.Managed) error) Conversion { return &customConversion{ baseConversion: newBaseConversion(sourceVersion, targetVersion), diff --git a/pkg/config/conversion/conversions_test.go b/pkg/config/conversion/conversions_test.go index 33f09a70..03de9080 100644 --- a/pkg/config/conversion/conversions_test.go +++ b/pkg/config/conversion/conversions_test.go @@ -8,11 +8,10 @@ import ( "fmt" "testing" + "github.com/crossplane/crossplane-runtime/pkg/fieldpath" "github.com/crossplane/crossplane-runtime/pkg/test" "github.com/google/go-cmp/cmp" "k8s.io/utils/ptr" - - "github.com/crossplane/crossplane-runtime/pkg/fieldpath" ) const ( diff --git a/pkg/controller/conversion/functions.go b/pkg/controller/conversion/functions.go index 0b6c30a9..33619094 100644 --- a/pkg/controller/conversion/functions.go +++ b/pkg/controller/conversion/functions.go @@ -15,8 +15,8 @@ import ( // RoundTrip round-trips from `src` to `dst` via an unstructured map[string]any // representation of the `src` object and applies the registered webhook -// conversion functions. -func RoundTrip(dst, src resource.Terraformed) error { //nolint:gocyclo // considered breaking this according to the converters and I did not like it +// conversion functions of this registry. +func (r *registry) RoundTrip(dst, src resource.Terraformed) error { //nolint:gocyclo // considered breaking this according to the converters and I did not like it srcMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(src) if err != nil { return errors.Wrap(err, "cannot convert the conversion source object into the map[string]any representation") @@ -35,7 +35,7 @@ func RoundTrip(dst, src resource.Terraformed) error { //nolint:gocyclo // consid } srcPaved := fieldpath.Pave(srcMap) dstPaved := fieldpath.Pave(dstMap) - for _, c := range GetConversions(dst) { + for _, c := range r.GetConversions(dst) { if pc, ok := c.(conversion.PavedConversion); ok { if _, err := pc.ConvertPaved(srcPaved, dstPaved); err != nil { return errors.Wrapf(err, "cannot apply the PavedConversion for the %q object", dst.GetTerraformResourceType()) @@ -48,9 +48,9 @@ func RoundTrip(dst, src resource.Terraformed) error { //nolint:gocyclo // consid return errors.Wrap(err, "cannot convert the map[string]any representation of the conversion target object to the target object") } - for _, c := range GetConversions(dst) { - if tc, ok := c.(conversion.TerraformedConversion); ok { - if _, err := tc.ConvertTerraformed(src, dst); err != nil { + for _, c := range r.GetConversions(dst) { + if tc, ok := c.(conversion.ManagedConversion); ok { + if _, err := tc.ConvertManaged(src, dst); err != nil { return errors.Wrapf(err, "cannot apply the TerraformedConversion for the %q object", dst.GetTerraformResourceType()) } } @@ -58,3 +58,10 @@ func RoundTrip(dst, src resource.Terraformed) error { //nolint:gocyclo // consid return nil } + +// RoundTrip round-trips from `src` to `dst` via an unstructured map[string]any +// representation of the `src` object and applies the registered webhook +// conversion functions. +func RoundTrip(dst, src resource.Terraformed) error { + return instance.RoundTrip(dst, src) +} diff --git a/pkg/controller/conversion/functions_test.go b/pkg/controller/conversion/functions_test.go new file mode 100644 index 00000000..1e7ebd4d --- /dev/null +++ b/pkg/controller/conversion/functions_test.go @@ -0,0 +1,103 @@ +// SPDX-FileCopyrightText: 2023 The Crossplane Authors +// +// SPDX-License-Identifier: Apache-2.0 + +package conversion + +import ( + "fmt" + "testing" + + xpresource "github.com/crossplane/crossplane-runtime/pkg/resource" + "github.com/crossplane/crossplane-runtime/pkg/test" + "github.com/google/go-cmp/cmp" + + "github.com/crossplane/upjet/pkg/config" + "github.com/crossplane/upjet/pkg/config/conversion" + "github.com/crossplane/upjet/pkg/resource" + "github.com/crossplane/upjet/pkg/resource/fake" +) + +const ( + key1 = "key1" + val1 = "val1" + key2 = "key2" + val2 = "val2" + commonKey = "commonKey" + commonVal = "commonVal" +) + +func TestRoundTrip(t *testing.T) { + type args struct { + dst resource.Terraformed + src resource.Terraformed + conversions []conversion.Conversion + } + type want struct { + err error + dst resource.Terraformed + } + tests := map[string]struct { + reason string + args args + want want + }{ + "SuccessfulRoundTrip": { + reason: "Source object is successfully copied into the target object.", + args: args{ + dst: fake.NewTerraformed(), + src: fake.NewTerraformed(fake.WithParameters(fake.NewMap(key1, val1))), + }, + want: want{ + dst: fake.NewTerraformed(fake.WithParameters(fake.NewMap(key1, val1))), + }, + }, + "SuccessfulRoundTripWithConversions": { + reason: "Source object is successfully converted into the target object with a set of conversions.", + args: args{ + dst: fake.NewTerraformed(), + src: fake.NewTerraformed(fake.WithParameters(fake.NewMap(commonKey, commonVal, key1, val1))), + conversions: []conversion.Conversion{ + // Because the parameters of the fake.Terraformed is an unstructured + // map, all the fields of source (including key1) are successfully + // copied into dst by registry.RoundTrip. + // This conversion deletes the copied key "key1". + conversion.NewCustomConverter(conversion.AllVersions, conversion.AllVersions, func(_, target xpresource.Managed) error { + tr := target.(*fake.Terraformed) + delete(tr.Parameters, key1) + return nil + }), + conversion.NewFieldRenameConversion(conversion.AllVersions, fmt.Sprintf("parameterizable.parameters.%s", key1), conversion.AllVersions, fmt.Sprintf("parameterizable.parameters.%s", key2)), + }, + }, + want: want{ + dst: fake.NewTerraformed(fake.WithParameters(fake.NewMap(commonKey, commonVal, key2, val1))), + }, + }, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + p := &config.Provider{ + Resources: map[string]*config.Resource{ + tc.args.dst.GetTerraformResourceType(): { + Conversions: tc.args.conversions, + }, + }, + } + r := ®istry{} + if err := r.RegisterConversions(p); err != nil { + t.Fatalf("\n%s\nRegisterConversions(p): Failed to register the conversions with the registry.\n", tc.reason) + } + err := r.RoundTrip(tc.args.dst, tc.args.src) + if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + t.Errorf("\n%s\nRoundTrip(dst, src): -wantErr, +gotErr:\n%s", tc.reason, diff) + } + if tc.want.err != nil { + return + } + if diff := cmp.Diff(tc.want.dst, tc.args.dst); diff != "" { + t.Errorf("\n%s\nRoundTrip(dst, src): -wantDst, +gotDst:\n%s", tc.reason, diff) + } + }) + } +} diff --git a/pkg/controller/conversion/registry.go b/pkg/controller/conversion/registry.go index 6412c082..ab1fac5b 100644 --- a/pkg/controller/conversion/registry.go +++ b/pkg/controller/conversion/registry.go @@ -24,23 +24,37 @@ type registry struct { } // RegisterConversions registers the API version conversions from the specified -// provider configuration. -func RegisterConversions(provider *config.Provider) error { - if instance != nil { +// provider configuration with this registry. +func (r *registry) RegisterConversions(provider *config.Provider) error { + if r.provider != nil { return errors.New(errAlreadyRegistered) } - instance = ®istry{ - provider: provider, - } + r.provider = provider return nil } -// GetConversions returns the conversion.Conversions registered for the -// Terraformed resource. -func GetConversions(tr resource.Terraformed) []conversion.Conversion { +// GetConversions returns the conversion.Conversions registered in this +// registry for the specified Terraformed resource. +func (r *registry) GetConversions(tr resource.Terraformed) []conversion.Conversion { t := tr.GetTerraformResourceType() - if instance == nil || instance.provider == nil || instance.provider.Resources[t] == nil { + if r == nil || r.provider == nil || r.provider.Resources[t] == nil { return nil } - return instance.provider.Resources[t].Conversions + return r.provider.Resources[t].Conversions +} + +// GetConversions returns the conversion.Conversions registered for the +// specified Terraformed resource. +func GetConversions(tr resource.Terraformed) []conversion.Conversion { + return instance.GetConversions(tr) +} + +// RegisterConversions registers the API version conversions from the specified +// provider configuration. +func RegisterConversions(provider *config.Provider) error { + if instance != nil { + return errors.New(errAlreadyRegistered) + } + instance = ®istry{} + return instance.RegisterConversions(provider) } diff --git a/pkg/resource/fake/terraformed.go b/pkg/resource/fake/terraformed.go index ed71b356..ed891bb4 100644 --- a/pkg/resource/fake/terraformed.go +++ b/pkg/resource/fake/terraformed.go @@ -122,3 +122,33 @@ func (t *Terraformed) DeepCopyObject() runtime.Object { _ = json.Unmarshal(j, out) return out } + +// Option is an option to modify the properties of a Terraformed object. +type Option func(terraformed *Terraformed) + +// WithParameters sets the parameters of a Terraformed. +func WithParameters(params map[string]any) Option { + return func(tr *Terraformed) { + tr.Parameters = params + } +} + +// NewTerraformed initializes a new Terraformed with the given options. +func NewTerraformed(opts ...Option) *Terraformed { + tr := &Terraformed{} + for _, o := range opts { + o(tr) + } + return tr +} + +// NewMap prepares a map from the supplied key value parameters. +// The parameters slice must be a sequence of key, value pairs and must have +// an even length. The function will panic otherwise. +func NewMap(keyValue ...string) map[string]any { + m := make(map[string]any, len(keyValue)/2) + for i := 0; i < len(keyValue)-1; i += 2 { + m[keyValue[i]] = keyValue[i+1] + } + return m +} From fa398ebc5b39f69769861e7136c893eabc2de5d8 Mon Sep 17 00:00:00 2001 From: Alper Rifat Ulucinar Date: Mon, 29 Jan 2024 23:23:23 +0300 Subject: [PATCH 11/11] Do not set the latest version as the storage version for smoother downgrades Signed-off-by: Alper Rifat Ulucinar --- pkg/pipeline/templates/crd_types.go.tmpl | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/pipeline/templates/crd_types.go.tmpl b/pkg/pipeline/templates/crd_types.go.tmpl index f5a7084b..4b7a5186 100644 --- a/pkg/pipeline/templates/crd_types.go.tmpl +++ b/pkg/pipeline/templates/crd_types.go.tmpl @@ -42,7 +42,6 @@ type {{ .CRD.Kind }}Status struct { // +kubebuilder:object:root=true // +kubebuilder:subresource:status -// +kubebuilder:storageversion // {{ .CRD.Kind }} is the Schema for the {{ .CRD.Kind }}s API. {{ .CRD.Description }} // +kubebuilder:printcolumn:name="READY",type="string",JSONPath=".status.conditions[?(@.type=='Ready')].status"