Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions pkg/config/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package config

import (
"go/types"
"strings"

fwresource "github.com/hashicorp/terraform-plugin-framework/resource"
Expand Down Expand Up @@ -95,6 +96,7 @@ func DefaultResource(name string, terraformSchema *schema.Resource, terraformPlu
Conversions: []conversion.Conversion{conversion.NewIdentityConversionExpandPaths(conversion.AllVersions, conversion.AllVersions, nil)},
OverrideFieldNames: map[string]string{},
listConversionPaths: make(map[string]string),
overrideGeneratedFieldType: map[string]types.Type{},
}
for _, f := range opts {
f(r)
Expand Down Expand Up @@ -151,8 +153,12 @@ func MarkAsRequired(sch *schema.Resource, fieldpaths ...string) {
}

// GetSchema returns the schema of the field whose fieldpath is given.
// Returns nil if Schema is not found at the specified path.
// Returns nil if the given resource schema is nil or if a Schema is
// not found at the specified path or subpath.
func GetSchema(sch *schema.Resource, fieldpath string) *schema.Schema {
if sch == nil {
return nil
}
current := sch
fields := strings.Split(fieldpath, ".")
final := fields[len(fields)-1]
Expand All @@ -166,7 +172,7 @@ func GetSchema(sch *schema.Resource, fieldpath string) *schema.Schema {
return nil
}
res, rok := s.Elem.(*schema.Resource)
if !rok {
if !rok || res == nil {
return nil
}
current = res
Expand Down
32 changes: 32 additions & 0 deletions pkg/config/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,38 @@ func TestGetSchema(t *testing.T) {
sch: nil,
},
},
"MiddleFieldIsNilResource": {
reason: "A nil element resource in the middle of the path is reported as not found instead of panicking.",
args: args{
fieldpath: "topA.topB.topC",
sch: &schema.Resource{
Schema: map[string]*schema.Schema{
"topA": {
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"topB": {
Elem: (*schema.Resource)(nil),
},
},
},
},
},
},
},
want: want{
sch: nil,
},
},
"NilResourceSchema": {
reason: "A nil Terraform resource schema is reported as not found instead of panicking.",
args: args{
fieldpath: "topA",
sch: nil,
},
want: want{
sch: nil,
},
},
}

for name, tc := range cases {
Expand Down
48 changes: 47 additions & 1 deletion pkg/config/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@ package config
import (
"context"
"fmt"
"go/types"
"strings"
"time"

"github.com/crossplane/crossplane-runtime/v2/pkg/errors"
"github.com/crossplane/crossplane-runtime/v2/pkg/fieldpath"
"github.com/crossplane/crossplane-runtime/v2/pkg/reconciler/managed"
xpresource "github.com/crossplane/crossplane-runtime/v2/pkg/resource"
Expand All @@ -20,7 +22,6 @@ import (
"github.com/hashicorp/terraform-plugin-go/tftypes"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
"github.com/pkg/errors"
"k8s.io/apimachinery/pkg/util/json"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/utils/ptr"
Expand Down Expand Up @@ -707,6 +708,11 @@ type Resource struct {
// the Terraform Plugin SDKv2 client.
useTerraformPluginFrameworkClient bool

// overrideGeneratedFieldType allows to manually override the type for the
// generated field of a Resource at the specified Terraform path.
// We only support type overrides for scalar fields currently.
overrideGeneratedFieldType map[string]types.Type

// OverrideFieldNames allows to manually override the relevant field name to
// avoid possible Go struct name conflicts that may occur after Multiversion
// CRDs support. During field generation, there may be fields with the same
Expand Down Expand Up @@ -1180,6 +1186,46 @@ func (r *Resource) RemoveSingletonListConversion(tfPath string) bool {
return false
}

// OverrideScalarFieldType allows to manually override the type for the
// generated scalar field of a Resource at the specified Terraform path.
// The path is a Terraform field path without the wildcard segments, e.g.,
// "x.y", even if "x" is a collection type.
// We only support overriding types for scalar fields as of now.
// Trying to override the type generated for a non-scalar path will result in
// an error.
func (r *Resource) OverrideScalarFieldType(path string, t types.Type) error {
if r.TerraformResource == nil {
return errors.Errorf("resource %q does not have a valid Terraform resource schema", r.Name)
}
s := GetSchema(r.TerraformResource, path)
if s == nil {
return errors.Errorf("path %s is not valid for the Terraform resource schema of %q", path, r.Name)
}
switch s.Type { //nolint:exhaustive // The default case handles the error cases (non-scalar paths) already.
case schema.TypeBool, schema.TypeFloat, schema.TypeInt, schema.TypeString:
r.overrideGeneratedFieldType[path] = t
default:
return errors.Errorf("field at path %q with Terraform type %s is not scalar, only scalar field types can be overridden", path, s.Type.String())
}
return nil
}

// FieldTypeOverrideConfiguration represents a configuration for a set of type
// overrides at a specific Terraform path.
type FieldTypeOverrideConfiguration struct {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One thing I thought about is exposing FieldTypeOverrideConfiguration as part of the public API. The The doc of the FieldTypeOverride field is says that it is only to be used by the code generator, but the returned type is exported. With this state, I am not sure this is just a internal plumbing point. It seems a general use API. What do you think?

I didn't think the alternative for this but still wanted to discuss do we really want to export them for general use instead of really generation internal?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I was initially thinking exposing the config.Resource.overrideGeneratedFieldType (as config.Resource.OverrideGeneratedFieldType) so that we would not need a separate config.Resource.OverrideScalarFieldType configuration method. However, in the future, we may need to support overriding types of non-scalar (complex/collection) fields and if we simply expose overrideGeneratedFieldType as our configuration API, it also directly exposes upjet code generation pipeline's implementation details.

Currently, the only "intended" and exposed configuration API is the config.Resource.OverrideScalarFieldType, which is what a provider needs to specify to be able to override the generated type for a scalar field, the field's path and the type to be used for the override. Supporting overrides for complex types or collection types in more involved because such fields can differ across the forProvider, initProvider and atProvider API trees. For scalars, they always have the same exact type (and that's the reason the OverrideScalarFieldType method accepts a single type parameter, not separate types for the 3 API trees we have). If we had exposed overrideGeneratedFieldType directly, it would not be future proof but the OverrideScalarFieldType is future proof because it's not a generic override API, i.e., it's not expected to handle non-scalar field type overrides. Also having the configured performed via OverrideScalarFieldType (instead of directly setting the configuration data structures) would allow us to do some validation in the future.

The cost of hiding the implementation detail from the provider authors is then we need to expose that configuration via an accessor because the code generation pipeline that consumes this configuration lives in the package pkg/types. Go does not have something like a "friend package" concept and an internal package won't help because overrideGeneratedFieldType is not exported. FieldTypeOverrideConfiguration and FieldTypeOverride should be of no use to the provider authors but we cannot also hide them from the provider authors. That's why I just added a Go doc that tells FieldTypeOverride is meant to be used by the code generator.

ParameterTypeOverride types.Type
Comment thread
ulucinar marked this conversation as resolved.
}

// FieldTypeOverride returns the type override configuration for the specified
// path. The path is a Terraform field path without the wildcard segments,
// e.g., "x.y", even if "x" is a collection type.
// Note: This accessor is meant to be only used by the code generator.
func (r *Resource) FieldTypeOverride(path string) FieldTypeOverrideConfiguration {
return FieldTypeOverrideConfiguration{
ParameterTypeOverride: r.overrideGeneratedFieldType[path],
}
}

// SetEmbeddedObject sets the EmbeddedObject for the specified key.
// The key is a Terraform field path without the wildcard segments.
func (m SchemaElementOptions) SetEmbeddedObject(el string) {
Expand Down
201 changes: 201 additions & 0 deletions pkg/config/resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package config
import (
"context"
"fmt"
"go/types"
"testing"

"github.com/crossplane/crossplane-runtime/v2/pkg/errors"
Expand All @@ -15,6 +16,7 @@ import (
"github.com/crossplane/crossplane-runtime/v2/pkg/resource/fake"
"github.com/crossplane/crossplane-runtime/v2/pkg/test"
"github.com/google/go-cmp/cmp"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"sigs.k8s.io/controller-runtime/pkg/client"
)

Expand Down Expand Up @@ -296,3 +298,202 @@ func TestRemoveSingletonListConversion(t *testing.T) {
})
}
}

// scalarAtXY returns a Terraform resource schema where x is
// a collection type (list) and x.y is a scalar (int).
func scalarAtXY() *schema.Resource {
return &schema.Resource{
Schema: map[string]*schema.Schema{
"x": {
Type: schema.TypeList,
Optional: true,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"y": {
Type: schema.TypeInt,
Optional: true,
},
},
},
},
},
}
}

// scalarAtXYZ returns a Terraform resource schema where x & x.y are
// a collection types (list) and x.y.z is a scalar (int).
func scalarAtXYZ() *schema.Resource {
return &schema.Resource{
Schema: map[string]*schema.Schema{
"x": {
Type: schema.TypeList,
Optional: true,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"y": {
Type: schema.TypeList,
Optional: true,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"z": {
Type: schema.TypeInt,
Optional: true,
},
},
},
},
},
},
},
},
}
}

func TestFieldTypeOverride(t *testing.T) {
strType := types.Typ[types.String]
type args struct {
r func() *Resource
// overridePath is the path passed to Resource.OverrideScalarFieldType.
overridePath string
// overrideType is the type passed to Resource.OverrideScalarFieldType.
overrideType types.Type
// path is the path queried via Resource.FieldTypeOverride.
path string
}
type want struct {
// parameterType is the expected ParameterTypeOverride.String(),
// or the empty string when there's no override configured.
parameterType string
// err is the expected error from Resource.OverrideScalarFieldType.
err error
}
cases := map[string]struct {
reason string
args
want
}{
"OverrideConfigured": {
reason: "A scalar type override set for a path is returned for that path.",
args: args{
path: "x.y",
overridePath: "x.y",
overrideType: strType,
r: func() *Resource {
return DefaultResource("test_resource", scalarAtXY(), nil, nil)
},
},
want: want{parameterType: "string"},
},
"DifferentPath": {
reason: "A path without a configured override has no parameter type override.",
args: args{
path: "a.b",
overridePath: "x.y",
overrideType: strType,
r: func() *Resource {
return DefaultResource("test_resource", scalarAtXY(), nil, nil)
},
},
want: want{parameterType: ""},
},
"NoOverrides": {
reason: "A resource with no configured overrides has no parameter type override for any path.",
args: args{
path: "x.y",
r: func() *Resource {
return DefaultResource("test_resource", scalarAtXY(), nil, nil)
},
},
want: want{parameterType: ""},
},
"OverrideCollectionPath": {
reason: "A scalar type override configured with a path without the wildcard segments is returned for that path, even if an intermediate path segment is a collection type.",
args: args{
path: "x.y.z",
overridePath: "x.y.z",
overrideType: strType,
r: func() *Resource {
return DefaultResource("test_resource", scalarAtXYZ(), nil, nil)
},
},
want: want{parameterType: "string"},
},
"NilOverrideIgnored": {
reason: "A nil type override is indistinguishable from no override for the path.",
args: args{
path: "x.y",
overridePath: "x.y",
overrideType: nil,
r: func() *Resource {
return DefaultResource("test_resource", scalarAtXY(), nil, nil)
},
},
want: want{parameterType: ""},
},
"PathNotInSchema": {
reason: "A path that does not exist in the Terraform resource schema cannot be configured with a type override.",
args: args{
path: "a.b",
overridePath: "a.b",
overrideType: strType,
r: func() *Resource {
return DefaultResource("test_resource", scalarAtXY(), nil, nil)
},
},
want: want{
parameterType: "",
err: errors.Errorf("path a.b is not valid for the Terraform resource schema of %q", "test_resource"),
},
},
"NoTerraformResourceSchema": {
reason: "A type override cannot be configured for a resource without a Terraform resource schema to validate the path against.",
args: args{
path: "x.y",
overridePath: "x.y",
overrideType: strType,
r: func() *Resource {
return DefaultResource("test_resource", nil, nil, nil)
},
},
want: want{
parameterType: "",
err: errors.Errorf("resource %q does not have a valid Terraform resource schema", "test_resource"),
},
},
"PathNotScalar": {
reason: "A path whose Terraform type is not scalar cannot be configured with a type override.",
args: args{
path: "x.y",
overridePath: "x.y",
overrideType: strType,
r: func() *Resource {
return DefaultResource("test_resource", scalarAtXYZ(), nil, nil)
},
},
want: want{
parameterType: "",
err: errors.Errorf("field at path %q with Terraform type %s is not scalar, only scalar field types can be overridden", "x.y", schema.TypeList.String()),
},
},
}
for n, tc := range cases {
t.Run(n, func(t *testing.T) {
r := tc.args.r()
var gotErr error
if tc.args.overridePath != "" {
gotErr = r.OverrideScalarFieldType(tc.args.overridePath, tc.args.overrideType)
}
if diff := cmp.Diff(tc.want.err, gotErr, test.EquateErrors()); diff != "" {
t.Errorf("%s\nOverrideScalarFieldType(%q): -want error, +got error:\n%s", tc.reason, tc.args.overridePath, diff)
}
got := r.FieldTypeOverride(tc.args.path).ParameterTypeOverride
gotStr := ""
if got != nil {
gotStr = got.String()
}
if diff := cmp.Diff(tc.want.parameterType, gotStr); diff != "" {
t.Errorf("%s\nFieldTypeOverride(%q): -want, +got:\n%s", tc.reason, tc.args.path, diff)
}
})
}
}
Loading
Loading