Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiversion CRDs & Conversion Webhooks #321

Merged
merged 11 commits into from
Jan 29, 2024
143 changes: 143 additions & 0 deletions pkg/config/conversion/conversions.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
// SPDX-FileCopyrightText: 2023 The Crossplane Authors <https://crossplane.io>
//
// 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"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
)

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`
// 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)
}

// 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 ManagedConversion can do type assertions to
// specific source and target types, and so, they are expected to be
// strongly typed.
type ManagedConversion interface {
Conversion
// 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.
ConvertManaged(src, target resource.Managed) (bool, error)
}

type baseConversion struct {
sourceVersion string
targetVersion string
}

func newBaseConversion(sourceVersion, targetVersion string) baseConversion {
return baseConversion{
sourceVersion: sourceVersion,
targetVersion: 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 {
baseConversion
sourceField string
targetField string
}

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)
// 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
}
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)
}

// 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),
sourceField: sourceField,
targetField: targetField,
}
}

type customConverter func(src, target resource.Managed) error

type customConversion struct {
baseConversion
customConverter customConverter
}

func (cc *customConversion) ConvertManaged(src, target resource.Managed) (bool, error) {
if !cc.Applicable(src, target) || cc.customConverter == nil {
return false, 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),
customConverter: converter,
}
}
148 changes: 148 additions & 0 deletions pkg/config/conversion/conversions_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
// SPDX-FileCopyrightText: 2023 The Crossplane Authors <https://crossplane.io>
//
// SPDX-License-Identifier: Apache-2.0

package conversion

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"
)

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)
}
3 changes: 3 additions & 0 deletions pkg/config/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,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"
)

Expand Down Expand Up @@ -446,6 +447,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
Expand Down
67 changes: 67 additions & 0 deletions pkg/controller/conversion/functions.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
// SPDX-FileCopyrightText: 2023 The Crossplane Authors <https://crossplane.io>
//
// SPDX-License-Identifier: Apache-2.0

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 and applies the registered webhook
// 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")
}
gvk := dst.GetObjectKind().GroupVersionKind()
if err := runtime.DefaultUnstructuredConverter.FromUnstructured(srcMap, dst); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Only for documenting, we may consider adding a filtering option for deciding whether to copy all possible fields to the destination. This can also be handled in converters.

Copy link
Collaborator Author

@ulucinar ulucinar Jan 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Let's see the frequency of the need for that functionality and if we frequently need to do this, then we can add a filtering API. Currently, as discussed, we can just let the RoundTrip copy everything and in the conversion function, we can unset the unwanted fields. This is comparable effort to preparing a filter, i.e., instead of passing a filter to RoundTrip, unsetting a field in a custom converter. Here, the assumption is we are dealing with a custom converter, which can be strongly typed.

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 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())
}
}
}
// 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 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())
}
}
}

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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this indirection because we don't have a registry object on the provider side? Now, we consume the universal variable instance in the upjet side, right? Is there any specific reason for this? Is creating and consuming a registry object on the provider side an option?

Copy link
Collaborator Author

@ulucinar ulucinar Jan 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the question. We need to invoke the registered conversion functions from the conversion webhooks. So, the conversion webhook needs to know the chain of the conversion functions available for a (spoke or hub) version. Unfortunately, unlike the reconcilers where the reconciliations for a given GVK happen under the context of a reconciler object that we register with the controller-runtime manager, a similar context variable that's directly owned (initialized) by the provider does not exist. Hence, we use a global registry that can convert a given managed resource kind to the conversion functions associated with it. We can initialize this global registry in the provider codebase but I've preferred to do this in upjet so that we will not need to repeat this for different providers.

The conversion.Convertible implementation that upjet generates currently relies on this global registry that upjet initializes using the conversion function configuration provided by the provider. We could extend upjet provider configuration and let the provider specify the global conversion registry to upjet and then, upjet could use that registry instead of the one that it directly owns (in the generated conversion.Convertible implementations). For now, I've avoided this indirection by letting upjet own this global registry.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to this discussion, we had better get rid of this global registry (whether it's owned by upjet or the provider) if we can find a way to set the context for the conversion webhooks. To the best of my knowledge, it's currently not possible with how the conversion.Convertible implementations are hooked into the controller runtime's webhook server. Of course, if we just replace the webhook server implementation itself and use a custom machinery instead of the conversion.Convertible, we can achieve this but I don't believe it would be worth the effort.

return instance.RoundTrip(dst, src)
}
Loading
Loading