fix(alpha): schema aware validation properly handling ToJson string t…

…ransform Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
turkenh · Aug 25, 2023 · 66e02e4 · 66e02e4
1 parent 4f9d85a
commit 66e02e4
Show file tree

Hide file tree

Showing 7 changed files with 162 additions and 7 deletions.
diff --git a/apis/apiextensions/v1/composition_transforms.go b/apis/apiextensions/v1/composition_transforms.go
@@ -446,12 +446,14 @@ const (
 	TransformIOTypeInt     TransformIOType = "int"
 	TransformIOTypeInt64   TransformIOType = "int64"
 	TransformIOTypeFloat64 TransformIOType = "float64"
+
+	TransformIOTypeObject TransformIOType = "object"
 )
 
 // IsValid checks if the given TransformIOType is valid.
 func (c TransformIOType) IsValid() bool {
 	switch c {
-	case TransformIOTypeString, TransformIOTypeBool, TransformIOTypeInt, TransformIOTypeInt64, TransformIOTypeFloat64:
+	case TransformIOTypeString, TransformIOTypeBool, TransformIOTypeInt, TransformIOTypeInt64, TransformIOTypeFloat64, TransformIOTypeObject:
 		return true
 	}
 	return false

diff --git a/apis/apiextensions/v1beta1/zz_generated.composition_transforms.go b/apis/apiextensions/v1beta1/zz_generated.composition_transforms.go
@@ -448,12 +448,14 @@ const (
 	TransformIOTypeInt     TransformIOType = "int"
 	TransformIOTypeInt64   TransformIOType = "int64"
 	TransformIOTypeFloat64 TransformIOType = "float64"
+
+	TransformIOTypeObject TransformIOType = "object"
 )
 
 // IsValid checks if the given TransformIOType is valid.
 func (c TransformIOType) IsValid() bool {
 	switch c {
-	case TransformIOTypeString, TransformIOTypeBool, TransformIOTypeInt, TransformIOTypeInt64, TransformIOTypeFloat64:
+	case TransformIOTypeString, TransformIOTypeBool, TransformIOTypeInt, TransformIOTypeInt64, TransformIOTypeFloat64, TransformIOTypeObject:
 		return true
 	}
 	return false

diff --git a/pkg/validation/apiextensions/v1/composition/patches.go b/pkg/validation/apiextensions/v1/composition/patches.go
@@ -322,10 +322,10 @@ func validateIOTypesWithTransforms(transforms []v1.Transform, fromType, toType x
 	return field.Invalid(field.NewPath("transforms"), transforms, fmt.Sprintf("the provided transforms do not output a type compatible with the toFieldPath according to the schema: %s != %s", fromType, toType))
 }
 
-func validateTransformsChainIOTypes(transforms []v1.Transform, fromType xpschema.KnownJSONType) (outputType v1.TransformIOType, fErr *field.Error) {
+func validateTransformsChainIOTypes(transforms []v1.Transform, fromType xpschema.KnownJSONType) (v1.TransformIOType, *field.Error) {
 	inputType, err := xpschema.FromKnownJSONType(fromType)
 	if err != nil && fromType != "" {
-		return "", field.InternalError(field.NewPath("transforms"), fErr)
+		return "", field.InternalError(field.NewPath("transforms"), err)
 	}
 	for i, transform := range transforms {
 		transform := transform
@@ -479,8 +479,29 @@ func IsValidInputForTransform(t *v1.Transform, fromType v1.TransformIOType) erro
 			return errors.Errorf("match transform can only be used with string input types, got %s", fromType)
 		}
 	case v1.TransformTypeString:
-		if fromType != v1.TransformIOTypeString {
-			return errors.Errorf("string transform can only be used with string input types, got %s", fromType)
+		switch t.String.Type {
+		case v1.StringTransformTypeRegexp, v1.StringTransformTypeTrimSuffix, v1.StringTransformTypeTrimPrefix:
+			if fromType != v1.TransformIOTypeString {
+				return errors.Errorf("string transform can only be used with string input types, got %s", fromType)
+			}
+		case v1.StringTransformTypeFormat:
+			// any input type is valid
+		case v1.StringTransformTypeConvert:
+			if t.String.Convert == nil {
+				return errors.Errorf("string transform convert type is required for convert transform")
+			}
+			switch *t.String.Convert {
+			case v1.StringConversionTypeToLower, v1.StringConversionTypeToUpper, v1.StringConversionTypeFromBase64, v1.StringConversionTypeToBase64:
+				if fromType != v1.TransformIOTypeString {
+					return errors.Errorf("string transform can only be used with string input types, got %s", fromType)
+				}
+			case v1.StringConversionTypeToJSON, v1.StringConversionTypeToAdler32, v1.StringConversionTypeToSHA1, v1.StringConversionTypeToSHA256, v1.StringConversionTypeToSHA512:
+				// any input type is valid
+			default:
+				return errors.Errorf("unknown string conversion type %s", *t.String.Convert)
+			}
+		default:
+			return errors.Errorf("unknown string transform type %s", t.String.Type)
 		}
 	case v1.TransformTypeConvert:
 		if _, err := composite.GetConversionFunc(t.Convert, fromType); err != nil {

diff --git a/pkg/validation/apiextensions/v1/composition/patches_test.go b/pkg/validation/apiextensions/v1/composition/patches_test.go
@@ -313,6 +313,23 @@ func TestValidateTransforms(t *testing.T) {
 				toType:   "string",
 			},
 		},
+		"AcceptObjectInputTypeToJsonStringTransform": {
+			reason: "Should accept object input type with json string transform",
+			want:   want{err: nil},
+			args: args{
+				transforms: []v1.Transform{
+					{
+						Type: v1.TransformTypeString,
+						String: &v1.StringTransform{
+							Type:    v1.StringTransformTypeConvert,
+							Convert: &[]v1.StringConversionType{v1.StringConversionTypeToJSON}[0],
+						},
+					},
+				},
+				fromType: "object",
+				toType:   "string",
+			},
+		},
 	}
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
@@ -943,3 +960,70 @@ func TestComposedTemplateGetBaseObject(t *testing.T) {
 		})
 	}
 }
+
+func TestIsValidInputForTransform(t *testing.T) {
+	type args struct {
+		t        *v1.Transform
+		fromType v1.TransformIOType
+	}
+	type want struct {
+		err bool
+	}
+	tests := map[string]struct {
+		reason string
+		args   args
+		want   want
+	}{
+		"ValidStringTransformInputString": {
+			reason: "Valid String transformType should not return an error with input string",
+			args: args{
+				fromType: v1.TransformIOTypeString,
+				t: &v1.Transform{
+					Type: v1.TransformTypeString,
+					String: &v1.StringTransform{
+						Type:    v1.StringTransformTypeConvert,
+						Convert: toPointer(v1.StringConversionTypeToUpper),
+					},
+				},
+			},
+		},
+		"ValidStringTransformInputObjectToJson": {
+			reason: "Valid String transformType should not return an error with input object if toJson",
+			args: args{
+				fromType: v1.TransformIOTypeObject,
+				t: &v1.Transform{
+					Type: v1.TransformTypeString,
+					String: &v1.StringTransform{
+						Type:    v1.StringTransformTypeConvert,
+						Convert: toPointer(v1.StringConversionTypeToJSON),
+					},
+				},
+			},
+		},
+		"InValidStringTransformInputObjectToUpper": {
+			reason: "Valid String transformType should not return an error with input string",
+			args: args{
+				fromType: v1.TransformIOTypeObject,
+				t: &v1.Transform{
+					Type: v1.TransformTypeString,
+					String: &v1.StringTransform{
+						Type:    v1.StringTransformTypeConvert,
+						Convert: toPointer(v1.StringConversionTypeToUpper),
+					},
+				},
+			},
+			want: want{
+				err: true,
+			},
+		},
+	}
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			err := IsValidInputForTransform(tc.args.t, tc.args.fromType)
+			if tc.want.err && err == nil {
+				t.Errorf("\n%s\nIsValidInputForTransform(...): -want error, +got error: \n%s", tc.reason, err)
+				return
+			}
+		})
+	}
+}
diff --git a/pkg/validation/internal/schema/schema.go b/pkg/validation/internal/schema/schema.go
@@ -66,6 +66,8 @@ func FromTransformIOType(c v1.TransformIOType) KnownJSONType {
 		return KnownJSONTypeInteger
 	case v1.TransformIOTypeFloat64:
 		return KnownJSONTypeNumber
+	case v1.TransformIOTypeObject:
+		return KnownJSONTypeObject
 	}
 	// should never happen
 	return ""
@@ -82,7 +84,9 @@ func FromKnownJSONType(t KnownJSONType) (v1.TransformIOType, error) {
 		return v1.TransformIOTypeInt64, nil
 	case KnownJSONTypeNumber:
 		return v1.TransformIOTypeFloat64, nil
-	case KnownJSONTypeObject, KnownJSONTypeArray, KnownJSONTypeNull:
+	case KnownJSONTypeObject:
+		return v1.TransformIOTypeObject, nil
+	case KnownJSONTypeArray, KnownJSONTypeNull:
 		return "", errors.Errorf(errFmtUnsupportedJSONType, t)
 	default:
 		return "", errors.Errorf(errFmtUnknownJSONType, t)

diff --git a/test/e2e/comp_schema_validation_test.go b/test/e2e/comp_schema_validation_test.go
@@ -44,6 +44,15 @@ func TestCompositionValidation(t *testing.T) {
 				funcs.ResourcesCreatedWithin(30*time.Second, manifests, "composition-valid.yaml"),
 			),
 		},
+		{
+			// A valid Composition should be created when validated in strict mode.
+			Name:        "ValidCompositionWithAToJsonTransformIsAcceptedStrictMode",
+			Description: "A valid Composition defining a valid ToJson String transform should be created when validated in strict mode.",
+			Assessment: funcs.AllOf(
+				funcs.ApplyResources(FieldManager, manifests, "composition-transform-tojson-valid.yaml"),
+				funcs.ResourcesCreatedWithin(30*time.Second, manifests, "composition-transform-tojson-valid.yaml"),
+			),
+		},
 		{
 			// An invalid Composition should be rejected when validated in strict mode.
 			Name:       "InvalidCompositionIsRejectedStrictMode",

diff --git a/...2e/manifests/apiextensions/composition/validation/composition-transform-tojson-valid.yaml b/...2e/manifests/apiextensions/composition/validation/composition-transform-tojson-valid.yaml
@@ -0,0 +1,33 @@
+apiVersion: apiextensions.crossplane.io/v1
+kind: Composition
+metadata:
+  name: valid-tojson-patch
+  annotations:
+    crossplane.io/composition-validation-mode: strict
+spec:
+  compositeTypeRef:
+    apiVersion: nop.example.org/v1alpha1
+    kind: XNopResource
+  resources:
+  - name: nop-resource-1
+    base:
+     apiVersion: nop.crossplane.io/v1alpha1
+     kind: NopResource
+     spec:
+      forProvider:
+        conditionAfter:
+        - conditionType: Ready
+          conditionStatus: "False"
+          time: 0s
+        - conditionType: Ready
+          conditionStatus: "True"
+          time: 10s
+    patches:
+    - type: FromCompositeFieldPath
+      fromFieldPath: spec
+      toFieldPath: metadata.annotations[spec]
+      transforms:
+      - type: string
+        string:
+         type: Convert
+         convert: ToJson