From 72af07d8ddb7bc15896e3b21d5f4f9666ba94fe0 Mon Sep 17 00:00:00 2001 From: Matt Bush Date: Fri, 4 Apr 2025 23:03:30 -0700 Subject: [PATCH 1/2] Improve tests of singleton list conversions Signed-off-by: Matt Bush --- pkg/config/conversion/conversions_test.go | 184 ++++++++++++++++++ pkg/config/conversion/list_conversion_test.go | 3 +- pkg/config/provider.go | 2 +- 3 files changed, 187 insertions(+), 2 deletions(-) diff --git a/pkg/config/conversion/conversions_test.go b/pkg/config/conversion/conversions_test.go index 532b3ef5..a0a89cb2 100644 --- a/pkg/config/conversion/conversions_test.go +++ b/pkg/config/conversion/conversions_test.go @@ -411,6 +411,98 @@ func TestSingletonListConversion(t *testing.T) { }, }, }, + "SuccessfulNestedToEmbeddedObjectConversion": { + reason: "Successful conversion from a nested singleton list to an embedded object.", + args: args{ + sourceVersion: AllVersions, + sourceMap: map[string]any{ + "spec": map[string]any{ + "initProvider": map[string]any{ + "o": []map[string]any{ + { + "n": []map[string]any{ + { + "k": "v", + }, + }, + }, + }, + }, + }, + }, + targetVersion: AllVersions, + targetMap: map[string]any{}, + crdPaths: []string{"o", "o[*].n"}, + mode: ToEmbeddedObject, + }, + want: want{ + converted: true, + targetMap: map[string]any{ + "spec": map[string]any{ + "initProvider": map[string]any{ + "o": map[string]any{ + "n": map[string]any{ + "k": "v", + }, + }, + }, + }, + }, + }, + }, + "SuccessfulNestedToEmbeddedObjectConversionPartial": { + reason: "Successful conversion from an singleton list nested in a non-singleton list to an embedded object nested in a list.", + args: args{ + sourceVersion: AllVersions, + sourceMap: map[string]any{ + "spec": map[string]any{ + "initProvider": map[string]any{ + "o": []map[string]any{ + { + "n": []map[string]any{ + { + "k": "v", + }, + }, + }, + { + "n": []map[string]any{ + { + "k": "v2", + }, + }, + }, + }, + }, + }, + }, + targetVersion: AllVersions, + targetMap: map[string]any{}, + crdPaths: []string{"o[*].n"}, + mode: ToEmbeddedObject, + }, + want: want{ + converted: true, + targetMap: map[string]any{ + "spec": map[string]any{ + "initProvider": map[string]any{ + "o": []map[string]any{ + { + "n": map[string]any{ + "k": "v", + }, + }, + { + "n": map[string]any{ + "k": "v2", + }, + }, + }, + }, + }, + }, + }, + }, "SuccessfulToSingletonListConversion": { reason: "Successful conversion from an embedded object to a singleton list.", args: args{ @@ -444,6 +536,98 @@ func TestSingletonListConversion(t *testing.T) { }, }, }, + "SuccessfulNestedToSingletonListConversion": { + reason: "Successful conversion from a nested embedded object to a singleton list.", + args: args{ + sourceVersion: AllVersions, + sourceMap: map[string]any{ + "spec": map[string]any{ + "initProvider": map[string]any{ + "o": map[string]any{ + "n": map[string]any{ + "k": "v", + }, + }, + }, + }, + }, + targetVersion: AllVersions, + targetMap: map[string]any{}, + crdPaths: []string{"o", "o[*].n"}, + mode: ToSingletonList, + }, + want: want{ + converted: true, + targetMap: map[string]any{ + "spec": map[string]any{ + "initProvider": map[string]any{ + "o": []map[string]any{ + { + "n": []map[string]any{ + { + "k": "v", + }, + }, + }, + }, + }, + }, + }, + }, + }, + "SuccessfulNestedToSingletonListConversionPartial": { + reason: "Successful conversion from an embedded object nested in a non-singleton list to a nested list.", + args: args{ + sourceVersion: AllVersions, + sourceMap: map[string]any{ + "spec": map[string]any{ + "initProvider": map[string]any{ + "o": []map[string]any{ + { + "n": map[string]any{ + "k": "v", + }, + }, + { + "n": map[string]any{ + "k": "v2", + }, + }, + }, + }, + }, + }, + targetVersion: AllVersions, + targetMap: map[string]any{}, + crdPaths: []string{"o[*].n"}, + mode: ToSingletonList, + }, + want: want{ + converted: true, + targetMap: map[string]any{ + "spec": map[string]any{ + "initProvider": map[string]any{ + "o": []map[string]any{ + { + "n": []map[string]any{ + { + "k": "v", + }, + }, + }, + { + "n": []map[string]any{ + { + "k": "v2", + }, + }, + }, + }, + }, + }, + }, + }, + }, "NoCRDPath": { reason: "No conversion when the specified CRD paths is empty.", args: args{ diff --git a/pkg/config/conversion/list_conversion_test.go b/pkg/config/conversion/list_conversion_test.go index e1644e59..d066dd3a 100644 --- a/pkg/config/conversion/list_conversion_test.go +++ b/pkg/config/conversion/list_conversion_test.go @@ -12,6 +12,7 @@ import ( "github.com/google/go-cmp/cmp" jsoniter "github.com/json-iterator/go" "github.com/pkg/errors" + kjson "sigs.k8s.io/json" ) func TestConvert(t *testing.T) { @@ -470,5 +471,5 @@ func roundTrip(m map[string]any) (map[string]any, error) { return nil, err } var r map[string]any - return r, jsoniter.ConfigCompatibleWithStandardLibrary.Unmarshal(buff, &r) + return r, kjson.UnmarshalCaseSensitivePreserveInts(buff, &r) } diff --git a/pkg/config/provider.go b/pkg/config/provider.go index d3642522..e21190be 100644 --- a/pkg/config/provider.go +++ b/pkg/config/provider.go @@ -337,7 +337,7 @@ func NewProvider(schema []byte, prefix string, modulePath string, metadata []byt if (isTerraformPluginSDK && isPluginFrameworkResource) || (isTerraformPluginSDK && isCLIResource) || (isPluginFrameworkResource && isCLIResource) { panic(errors.Errorf(`resource %q is specified in more than one include list. It should appear in at most one of the lists "IncludeList", "TerraformPluginSDKIncludeList" or "TerraformPluginFrameworkIncludeList"`, name)) } - if len(terraformResource.Schema) == 0 || matches(name, p.SkipList) || (!matches(name, p.IncludeList) && !isTerraformPluginSDK && !isPluginFrameworkResource) { + if len(terraformResource.Schema) == 0 || matches(name, p.SkipList) || (!isCLIResource && !isTerraformPluginSDK && !isPluginFrameworkResource) { p.skippedResourceNames = append(p.skippedResourceNames, name) continue } From 4341c68646414419f1c0acd2e05791ba4fde8593 Mon Sep 17 00:00:00 2001 From: Matt Bush Date: Fri, 4 Apr 2025 23:07:10 -0700 Subject: [PATCH 2/2] Conversions to change types between float and string --- pkg/config/conversion/conversions.go | 99 ++++++ pkg/config/conversion/conversions_test.go | 355 ++++++++++++++++++++++ 2 files changed, 454 insertions(+) diff --git a/pkg/config/conversion/conversions.go b/pkg/config/conversion/conversions.go index fe33a68e..1058fd27 100644 --- a/pkg/config/conversion/conversions.go +++ b/pkg/config/conversion/conversions.go @@ -7,6 +7,7 @@ package conversion import ( "fmt" "slices" + "strconv" "github.com/crossplane/crossplane-runtime/pkg/fieldpath" "github.com/crossplane/crossplane-runtime/pkg/resource" @@ -151,6 +152,104 @@ func NewFieldRenameConversion(sourceVersion, sourceField, targetVersion, targetF } } +type TypeChangingMode = int + +const ( + // FloatToString converts a *float64 to a string, using an empty string for nil + FloatToString = iota + // StringToFloat converts a string to a *float64, using nil for an empty string. + // I don't know what I should have it do if the string is not a number. Panic? + // Return empty string? + StringToFloat +) + +type typeChangingFieldCopy struct { + baseConversion + paths []string + mode TypeChangingMode +} + +func (f *typeChangingFieldCopy) ConvertPaved(src, target *fieldpath.Paved) (bool, error) { + // TODO maybe refactor to instantiate these so I can extract their gvk for error messages + if !f.Applicable(&unstructured.Unstructured{Object: src.UnstructuredContent()}, + &unstructured.Unstructured{Object: target.UnstructuredContent()}) { + return false, nil + } + + modified := false + for _, p := range f.paths { + exp, err := src.ExpandWildcards(p) + if err != nil { + return modified, errors.Wrapf(err, "cannot expand wildcards for the field path expression %s", p) + } + for _, e := range exp { + v, err := src.GetValue(e) + if fieldpath.IsNotFound(err) { + continue + } + if err != nil { + return modified, errors.Wrapf(err, "failed to get the field %q from the %s conversion source object", e, f.sourceVersion) + } + switch f.mode { + case FloatToString: + // While the go stdlibrary unmarshalls all json numbers as float64, crossplane-runtime (perhaps accidentally?) uses + // the kubernetes json library, which delegates to UnmarshallCaseSensitivePreserveInts, which sometimes unmarshalls numbers + // to float64 and sometimes to int64. + // In order to be compatible with both types of deserialization, this can handle either float or int types. + strVal := fmt.Sprintf("%v", v) + if _, err := strconv.ParseFloat(strVal, 64); err != nil { + return modified, errors.Errorf("expected number at field %q with value %s in %s, got %T", e, f.sourceVersion, strVal, v) + } + err = target.SetValue(e, strVal) + if err != nil { + return modified, errors.Wrapf(err, "failed to set the field %q of the %s conversion target object", e, f.targetVersion) + } + modified = true + case StringToFloat: + strVal, ok := v.(string) + if !ok { + return modified, errors.Errorf("expected string at field %q in %s, got %T", e, f.sourceVersion, v) + } + if strVal != "" { + parsed, err := strconv.ParseFloat(strVal, 64) + if err != nil { + return modified, errors.Wrapf(err, "converting %s from %s to %s: failed to parse string %q as float64", e, f.sourceVersion, f.targetVersion, strVal) + } + // SetValue does a round trip json serialization/deserialization step using the kubernetes JSON library, + // which converts floats which are whole numbers to integers + err = target.SetValue(e, parsed) + if err != nil { + return modified, errors.Wrapf(err, "failed to set the field %q of the %s conversion target object", e, f.targetVersion) + } + modified = true + } else { + // Special behavior for if a string field is defined and set to "" to remove the existing contents of the field. + // I believe this is necessary because otherwise we would be unable to unset anything, but I need to think about it + // some more. + err := target.DeleteField(e) + if err != nil { + return modified, errors.Wrapf(err, "failed to unset the field %q of the %s conversion target object", e, f.targetVersion) + } + modified = true + } + } + } + + } + return modified, nil +} + +// NewTypeChangeConversion returns a new Conversion that implements a +// conversion from the specified `sourceVersion` to the specified +// `targetVersion` of an API that also changes the type of the field. +func NewTypeChangeConversion(sourceVersion, targetVersion string, paths []string, mode TypeChangingMode) Conversion { + return &typeChangingFieldCopy{ + baseConversion: newBaseConversion(sourceVersion, targetVersion), + paths: paths, + mode: mode, + } +} + type customConverter func(src, target resource.Managed) error type customConversion struct { diff --git a/pkg/config/conversion/conversions_test.go b/pkg/config/conversion/conversions_test.go index a0a89cb2..38e98d2d 100644 --- a/pkg/config/conversion/conversions_test.go +++ b/pkg/config/conversion/conversions_test.go @@ -346,6 +346,361 @@ func TestIdentityConversion(t *testing.T) { } } +func TestTypeConversion(t *testing.T) { + type args struct { + sourceVersion string + sourceMap map[string]any + targetVersion string + targetMap map[string]any + paths []string + mode TypeChangingMode + } + type want struct { + converted bool + err error + targetMap map[string]any + } + tests := map[string]struct { + reason string + args args + want want + }{ + "SuccessfulConversionWholeNumberFloatToStringTopLevel": { + reason: "Successfully convert a top-level integer-valued float to a string.", + args: args{ + sourceVersion: AllVersions, + sourceMap: map[string]any{ + "k1": 1, + "k2": "v2", + "k3": map[string]any{ + "nk1": "nv1", + }, + }, + targetVersion: AllVersions, + targetMap: map[string]any{ + "k2": "v2", + "k1": "wrong", + }, + paths: []string{ + "k1", + }, + mode: FloatToString, + }, + want: want{ + converted: true, + targetMap: map[string]any{ + "k1": "1", + "k2": "v2", + }, + }, + }, + "SuccessfulConversionFloatToStringTopLevel": { + reason: "Successfully convert a top-level float to a string.", + args: args{ + sourceVersion: AllVersions, + sourceMap: map[string]any{ + "k1": 1.23, + "k2": "v2", + "k3": map[string]any{ + "nk1": "nv1", + }, + }, + targetVersion: AllVersions, + targetMap: map[string]any{ + "k2": "v2", + "k1": 12, + }, + paths: []string{ + "k1", + }, + mode: FloatToString, + }, + want: want{ + converted: true, + targetMap: map[string]any{ + "k1": "1.23", + "k2": "v2", + }, + }, + }, + "SuccessfulConversionFloatToStringMultipleValuesComplexPath": { + reason: "Successfully convert multiple floats to strings with complex path expressions", + args: args{ + sourceVersion: AllVersions, + sourceMap: map[string]any{ + "l1": []map[string]any{ + { + "a": 48, + }, + { + "a": 12, + }, + }, + "l2": []map[string]any{ + { + "c": 13, + }, + }, + "o1": map[string]any{ + "b": 14, + }, + }, + targetVersion: AllVersions, + targetMap: map[string]any{}, + paths: []string{ + "l1[*].a", + "l2[0].c", + "o1.b", + }, + mode: FloatToString, + }, + want: want{ + converted: true, + targetMap: map[string]any{ + "l1": []map[string]any{ + { + "a": "48", + }, + { + "a": "12", + }, + }, + "l2": []map[string]any{ + { + "c": "13", + }, + }, + "o1": map[string]any{ + "b": "14", + }, + }, + }, + }, + "SuccessfulConversionWholeNumberStringToFloatTopLevel": { + reason: "Successfully convert a top-level integer-valued string to a float.", + args: args{ + sourceVersion: AllVersions, + sourceMap: map[string]any{ + "k1": "1", + "k2": "v2", + "k3": map[string]any{ + "nk1": "nv1", + }, + }, + targetVersion: AllVersions, + targetMap: map[string]any{ + "k2": "v2", + "k1": "wrong", + }, + paths: []string{ + "k1", + }, + mode: StringToFloat, + }, + want: want{ + converted: true, + targetMap: map[string]any{ + "k1": 1, + "k2": "v2", + }, + }, + }, + "SuccessfulConversionStringToFloatTopLevel": { + reason: "Successfully convert a top-level string to a float.", + args: args{ + sourceVersion: AllVersions, + sourceMap: map[string]any{ + "k1": "1.23", + "k2": "v2", + "k3": map[string]any{ + "nk1": "nv1", + }, + }, + targetVersion: AllVersions, + targetMap: map[string]any{ + "k2": "v2", + "k1": "wrong", + }, + paths: []string{ + "k1", + }, + mode: StringToFloat, + }, + want: want{ + converted: true, + targetMap: map[string]any{ + "k1": 1.23, + "k2": "v2", + }, + }, + }, + "SuccessfulConversionStringToFloatMultipleValuesComplexPath": { + reason: "Successfully convert multiple strings to floats with complex path expressions", + args: args{ + sourceVersion: AllVersions, + sourceMap: map[string]any{ + "l1": []map[string]any{ + { + "a": "48", + }, + { + "a": "12", + }, + }, + "l2": []map[string]any{ + { + "c": "13", + }, + }, + "o1": map[string]any{ + "b": "14", + }, + }, + targetVersion: AllVersions, + targetMap: map[string]any{}, + paths: []string{ + "l1[*].a", + "l2[0].c", + "o1.b", + }, + mode: StringToFloat, + }, + want: want{ + converted: true, + targetMap: map[string]any{ + "l1": []map[string]any{ + { + "a": 48, + }, + { + "a": 12, + }, + }, + "l2": []map[string]any{ + { + "c": 13, + }, + }, + "o1": map[string]any{ + "b": 14, + }, + }, + }, + }, + "StringToFloatWithEmptyInputShouldNullOutExistingValue": { + reason: "Successfully remove a value from the target map when the source value is an empty string", + args: args{ + sourceVersion: AllVersions, + sourceMap: map[string]any{ + "k1": "", + "k2": "v2", + "k3": map[string]any{ + "nk1": "nv1", + }, + }, + targetVersion: AllVersions, + targetMap: map[string]any{ + "k2": "v2", + "k1": "wrong", + }, + paths: []string{ + "k1", + }, + mode: StringToFloat, + }, + want: want{ + converted: true, + targetMap: map[string]any{ + "k2": "v2", + }, + }, + }, + "FloatToStringWithNullInputShouldBeSkipped": { + reason: "Leaves the target unchanged if the field path is not present", + args: args{ + sourceVersion: AllVersions, + sourceMap: map[string]any{ + "k2": "v2", + "k3": map[string]any{ + "nk1": "nv1", + }, + }, + targetVersion: AllVersions, + targetMap: map[string]any{ + "k2": "v2", + "k1": "wrong", + }, + paths: []string{ + "k1", + }, + mode: FloatToString, + }, + want: want{ + converted: false, + targetMap: map[string]any{ + "k1": "wrong", + "k2": "v2", + }, + }, + }, + "StringToFloatWithNullInputShouldBeSkipped": { + reason: "Leaves the target unchanged if the field path is not present", + args: args{ + sourceVersion: AllVersions, + sourceMap: map[string]any{ + "k2": "v2", + "k3": map[string]any{ + "nk1": "nv1", + }, + }, + targetVersion: AllVersions, + targetMap: map[string]any{ + "k2": "v2", + "k1": "wrong", + }, + paths: []string{ + "k1", + }, + mode: StringToFloat, + }, + want: want{ + converted: false, + targetMap: map[string]any{ + "k1": "wrong", + "k2": "v2", + }, + }, + }, + } + for n, tc := range tests { + t.Run(n, func(t *testing.T) { + c := NewTypeChangeConversion(tc.args.sourceVersion, tc.args.targetVersion, tc.args.paths, tc.args.mode) + sourceMap, err := roundTrip(tc.args.sourceMap) + if err != nil { + t.Fatalf("Failed to preprocess tc.args.sourceMap: %v", err) + } + targetMap, err := roundTrip(tc.args.targetMap) + if err != nil { + t.Fatalf("Failed to preprocess tc.args.targetMap: %v", err) + } + converted, err := c.(*typeChangingFieldCopy).ConvertPaved(fieldpath.Pave(sourceMap), fieldpath.Pave(targetMap)) + if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + t.Fatalf("\n%s\nConvertPaved(source, target): -wantErr, +gotErr:\n%s", tc.reason, diff) + } + if diff := cmp.Diff(tc.want.converted, converted); diff != "" { + t.Errorf("\n%s\nConvertPaved(source, target): -wantConverted, +gotConverted:\n%s", tc.reason, diff) + } + m, err := roundTrip(tc.want.targetMap) + if err != nil { + t.Fatalf("Failed to preprocess tc.want.targetMap: %v", err) + } + if diff := cmp.Diff(m, targetMap); diff != "" { + t.Errorf("\n%s\nConvertPaved(source, target): -wantTarget, +gotTarget:\n%s", tc.reason, diff) + } + }) + } +} + func TestDefaultPathPrefixes(t *testing.T) { // no need for a table-driven test here as we assert all the parameter roots // in the MR schema are asserted.