Skip to content

Commit

Permalink
Fixes union type conversions to CloudControl
Browse files Browse the repository at this point in the history
This change fixes the conversion of Pulumi input properties to the format expected by the CloudControl API in the case
when Pulumi schema specifies a union type (OneOf) without a discriminator. Previously the first matching case would be
picked which could erroneously send empty data to CloudControl. With this change a heuristic is run instead to pick the
non-error case with the largest map or array.

In the long term it would be better to support discriminators and manage their metadata (#1849).

Fixes #1846
  • Loading branch information
t0yv0 committed Nov 25, 2024
1 parent 763be99 commit 4771f98
Show file tree
Hide file tree
Showing 2 changed files with 110 additions and 4 deletions.
53 changes: 49 additions & 4 deletions provider/pkg/naming/convert.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,19 @@
package naming

import (
"errors"
"fmt"
"reflect"
"slices"
"strings"

"github.com/golang/glog"
"github.com/mattbaird/jsonpatch"
"github.com/pulumi/pulumi-aws-native/provider/pkg/metadata"
"github.com/pulumi/pulumi-go-provider/resourcex"
pschema "github.com/pulumi/pulumi/pkg/v3/codegen/schema"
"github.com/pulumi/pulumi/sdk/v3/go/common/resource"
"github.com/pulumi/pulumi/sdk/v3/go/common/util/contract"
)

// SdkToCfn converts Pulumi-SDK-shaped state to CloudFormation-shaped payload. In particular, SDK properties
Expand Down Expand Up @@ -94,14 +97,56 @@ func (c *sdkToCfnConverter) sdkTypedValueToCfn(spec *pschema.TypeSpec, v interfa
}

if spec.OneOf != nil {
contract.Assertf(len(spec.OneOf) >= 1, "at least one union case is required")

results := []any{}
errs := []error{}

for _, item := range spec.OneOf {
converted, err := c.sdkTypedValueToCfn(&item, v)
// return the first successful conversion
if err == nil {
return converted, nil
if err != nil {
errs = append(errs, err)
} else {
results = append(results, converted)
}
}

switch {
case len(results) == 0:
glog.V(9).Infof("conversion error: all union variants failed: %v", errors.Join(errs...))
return nil, &ConversionError{fmt.Sprintf("%v", *spec), v}
case len(results) == 1:
return results[0], nil
default:
// Properties such as aws-native:datazone:DataSource configuration do not specify a
// discriminator yet. Without a discriminator in case of multiple successful results we could
// instead pick the largest by length.

sizeOf := func(x any) int {
switch x := x.(type) {
case map[string]any:
return len(x)
case []any:
return len(x)
case nil:
return 0
default:
return 1
}
}

var bestResult any
bestResultSize := -1
for _, r := range results {
s := sizeOf(r)
if s > bestResultSize {
bestResult = r
bestResultSize = s
}
}
return bestResult, nil
}
return nil, &ConversionError{fmt.Sprintf("%v", *spec), v}

}

switch spec.Type {
Expand Down
61 changes: 61 additions & 0 deletions provider/pkg/naming/convert_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
pschema "github.com/pulumi/pulumi/pkg/v3/codegen/schema"
"github.com/pulumi/pulumi/sdk/v3/go/common/resource"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestCfnToSdk(t *testing.T) {
Expand Down Expand Up @@ -83,6 +84,66 @@ func TestSdkToCfnOneOf(t *testing.T) {
assert.Equal(t, expected, actual)
}

// Check that ambiguous OneOf without a discriminator will pick the largest match.
func TestSdkToCfnOneOfAmbiguous(t *testing.T) {
res := metadata.CloudAPIResource{
Inputs: map[string]pschema.PropertySpec{
"configuration": {TypeSpec: pschema.TypeSpec{
OneOf: []pschema.TypeSpec{
{Ref: "#/types/aws-native:datazone:DataSourceConfigurationInput0Properties"},
{Ref: "#/types/aws-native:datazone:DataSourceConfigurationInput1Properties"},
},
}},
},
}
types := map[string]metadata.CloudAPIType{
"aws-native:datazone:DataSourceConfigurationInput0Properties": metadata.CloudAPIType{
Type: "object",
Properties: map[string]pschema.PropertySpec{
"glueRunConfiguration": pschema.PropertySpec{
TypeSpec: pschema.TypeSpec{
Ref: "#/types/aws-native:datazone:DataSourceGlueRunConfigurationInput",
},
},
},
},
"aws-native:datazone:DataSourceGlueRunConfigurationInput": metadata.CloudAPIType{
Type: "object",
Properties: map[string]pschema.PropertySpec{
"dataAccessRole": pschema.PropertySpec{TypeSpec: pschema.TypeSpec{Type: "string"}},
},
},
"aws-native:datazone:DataSourceConfigurationInput1Properties": metadata.CloudAPIType{
Type: "object",
Properties: map[string]pschema.PropertySpec{"redshiftRunConfiguration": pschema.PropertySpec{
TypeSpec: pschema.TypeSpec{
Ref: "#/types/aws-native:datazone:DataSourceRedshiftRunConfigurationInput",
},
}},
},
"aws-native:datazone:DataSourceRedshiftRunConfigurationInput": metadata.CloudAPIType{
Type: "object",
Properties: map[string]pschema.PropertySpec{
"dataAccessRole": pschema.PropertySpec{TypeSpec: pschema.TypeSpec{Type: "string"}},
},
},
}

result, err := SdkToCfn(&res, types, map[string]any{
"configuration": map[string]any{
"redshiftRunConfiguration": map[string]any{
"dataAccessRole": "myrole",
},
},
})
require.NoError(t, err)

actualConfig := result["Configuration"].(map[string]any)
actualRedshiftConfig := actualConfig["RedshiftRunConfiguration"].(map[string]any)
actualDataAccessRole := actualRedshiftConfig["DataAccessRole"].(string)
require.Equal(t, "myrole", actualDataAccessRole)
}

func TestDiffToPatch(t *testing.T) {
test := func(t *testing.T, diff resource.ObjectDiff, expected []jsonpatch.JsonPatchOperation) {
res := sampleSchema.Resources["aws-native:ecs:Service"]
Expand Down

0 comments on commit 4771f98

Please sign in to comment.