Skip to content

Commit

Permalink
Merge pull request crossplane#411 from ulucinar/fix-embedded-conversion
Browse files Browse the repository at this point in the history
Add config.Resource.RemoveSingletonListConversion
  • Loading branch information
ulucinar authored Jun 12, 2024
2 parents 58f4ba3 + 361331e commit 37c7f4e
Show file tree
Hide file tree
Showing 9 changed files with 495 additions and 5 deletions.
2 changes: 1 addition & 1 deletion pkg/config/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,7 @@ func NewProvider(schema []byte, prefix string, modulePath string, metadata []byt
p.Resources[name].useTerraformPluginFrameworkClient = isPluginFrameworkResource
// traverse the Terraform resource schema to initialize the upjet Resource
// configurations
if err := traverseSchemas(name, terraformResource, p.Resources[name], p.schemaTraversers...); err != nil {
if err := TraverseSchemas(name, p.Resources[name], p.schemaTraversers...); err != nil {
panic(errors.Wrap(err, "failed to execute the Terraform schema traverser chain"))
}
}
Expand Down
22 changes: 22 additions & 0 deletions pkg/config/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -693,6 +693,28 @@ func (r *Resource) AddSingletonListConversion(tfPath, crdPath string) {
r.listConversionPaths[tfPath] = crdPath
}

// RemoveSingletonListConversion removes the singleton list conversion
// for the specified Terraform configuration path. Also unsets the path's
// embedding mode. The specified fieldpath expression must be a Terraform
// field path with or without the wildcard segments. Returns true if
// the path has already been registered for singleton list conversion.
func (r *Resource) RemoveSingletonListConversion(tfPath string) bool {
nPath := strings.ReplaceAll(tfPath, "[*]", "")
nPath = strings.ReplaceAll(nPath, "[0]", "")
for p := range r.listConversionPaths {
n := strings.ReplaceAll(p, "[*]", "")
n = strings.ReplaceAll(n, "[0]", "")
if n == nPath {
delete(r.listConversionPaths, p)
if r.SchemaElementOptions[n] != nil {
r.SchemaElementOptions[n].EmbeddedObject = false
}
return true
}
}
return false
}

// 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
186 changes: 185 additions & 1 deletion pkg/config/resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ const (
provider = "ACoolProvider"
)

func TestTagger_Initialize(t *testing.T) {
func TestTaggerInitialize(t *testing.T) {
errBoom := errors.New("boom")

type args struct {
Expand Down Expand Up @@ -112,3 +112,187 @@ func TestSetExternalTagsWithPaved(t *testing.T) {
})
}
}

func TestAddSingletonListConversion(t *testing.T) {
type args struct {
r func() *Resource
tfPath string
crdPath string
}
type want struct {
r func() *Resource
}
cases := map[string]struct {
reason string
args
want
}{
"AddNonWildcardTFPath": {
reason: "A non-wildcard TF path of a singleton list should successfully be configured to be converted into an embedded object.",
args: args{
tfPath: "singleton_list",
crdPath: "singletonList",
r: func() *Resource {
r := DefaultResource("test_resource", nil, nil, nil)
r.AddSingletonListConversion("singleton_list", "singletonList")
return r
},
},
want: want{
r: func() *Resource {
r := DefaultResource("test_resource", nil, nil, nil)
r.SchemaElementOptions = SchemaElementOptions{}
r.SchemaElementOptions["singleton_list"] = &SchemaElementOption{
EmbeddedObject: true,
}
r.listConversionPaths["singleton_list"] = "singletonList"
return r
},
},
},
"AddWildcardTFPath": {
reason: "A wildcard TF path of a singleton list should successfully be configured to be converted into an embedded object.",
args: args{
tfPath: "parent[*].singleton_list",
crdPath: "parent[*].singletonList",
r: func() *Resource {
r := DefaultResource("test_resource", nil, nil, nil)
r.AddSingletonListConversion("parent[*].singleton_list", "parent[*].singletonList")
return r
},
},
want: want{
r: func() *Resource {
r := DefaultResource("test_resource", nil, nil, nil)
r.SchemaElementOptions = SchemaElementOptions{}
r.SchemaElementOptions["parent.singleton_list"] = &SchemaElementOption{
EmbeddedObject: true,
}
r.listConversionPaths["parent[*].singleton_list"] = "parent[*].singletonList"
return r
},
},
},
"AddIndexedTFPath": {
reason: "An indexed TF path of a singleton list should successfully be configured to be converted into an embedded object.",
args: args{
tfPath: "parent[0].singleton_list",
crdPath: "parent[0].singletonList",
r: func() *Resource {
r := DefaultResource("test_resource", nil, nil, nil)
r.AddSingletonListConversion("parent[0].singleton_list", "parent[0].singletonList")
return r
},
},
want: want{
r: func() *Resource {
r := DefaultResource("test_resource", nil, nil, nil)
r.SchemaElementOptions = SchemaElementOptions{}
r.SchemaElementOptions["parent.singleton_list"] = &SchemaElementOption{
EmbeddedObject: true,
}
r.listConversionPaths["parent[0].singleton_list"] = "parent[0].singletonList"
return r
},
},
},
}
for n, tc := range cases {
t.Run(n, func(t *testing.T) {
r := tc.args.r()
r.AddSingletonListConversion(tc.args.tfPath, tc.args.crdPath)
wantR := tc.want.r()
if diff := cmp.Diff(wantR.listConversionPaths, r.listConversionPaths); diff != "" {
t.Errorf("%s\nAddSingletonListConversion(tfPath): -wantConversionPaths, +gotConversionPaths: \n%s", tc.reason, diff)
}
if diff := cmp.Diff(wantR.SchemaElementOptions, r.SchemaElementOptions); diff != "" {
t.Errorf("%s\nAddSingletonListConversion(tfPath): -wantSchemaElementOptions, +gotSchemaElementOptions: \n%s", tc.reason, diff)
}
})
}
}

func TestRemoveSingletonListConversion(t *testing.T) {
type args struct {
r func() *Resource
tfPath string
}
type want struct {
removed bool
r func() *Resource
}
cases := map[string]struct {
reason string
args
want
}{
"RemoveWildcardListConversion": {
reason: "An existing wildcard list conversion can successfully be removed.",
args: args{
tfPath: "parent[*].singleton_list",
r: func() *Resource {
r := DefaultResource("test_resource", nil, nil, nil)
r.AddSingletonListConversion("parent[*].singleton_list", "parent[*].singletonList")
return r
},
},
want: want{
removed: true,
r: func() *Resource {
r := DefaultResource("test_resource", nil, nil, nil)
return r
},
},
},
"RemoveIndexedListConversion": {
reason: "An existing indexed list conversion can successfully be removed.",
args: args{
tfPath: "parent[0].singleton_list",
r: func() *Resource {
r := DefaultResource("test_resource", nil, nil, nil)
r.AddSingletonListConversion("parent[0].singleton_list", "parent[0].singletonList")
return r
},
},
want: want{
removed: true,
r: func() *Resource {
r := DefaultResource("test_resource", nil, nil, nil)
return r
},
},
},
"NonExistingListConversion": {
reason: "A list conversion path that does not exist cannot be removed.",
args: args{
tfPath: "non-existent",
r: func() *Resource {
r := DefaultResource("test_resource", nil, nil, nil)
r.AddSingletonListConversion("parent[*].singleton_list", "parent[*].singletonList")
return r
},
},
want: want{
removed: false,
r: func() *Resource {
r := DefaultResource("test_resource", nil, nil, nil)
r.AddSingletonListConversion("parent[*].singleton_list", "parent[*].singletonList")
return r
},
},
},
}
for n, tc := range cases {
t.Run(n, func(t *testing.T) {
r := tc.args.r()
got := r.RemoveSingletonListConversion(tc.args.tfPath)
if diff := cmp.Diff(tc.want.removed, got); diff != "" {
t.Errorf("%s\nRemoveSingletonListConversion(tfPath): -wantRemoved, +gotRemoved: \n%s", tc.reason, diff)
}

if diff := cmp.Diff(tc.want.r().listConversionPaths, r.listConversionPaths); diff != "" {
t.Errorf("%s\nRemoveSingletonListConversion(tfPath): -wantConversionPaths, +gotConversionPaths: \n%s", tc.reason, diff)
}
})
}
}
24 changes: 22 additions & 2 deletions pkg/config/schema_conversions.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package config

import (
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/pkg/errors"

"github.com/crossplane/upjet/pkg/schema/traverser"
)
Expand All @@ -17,15 +18,34 @@ type ResourceSetter interface {
SetResource(r *Resource)
}

func traverseSchemas(tfName string, tfResource *schema.Resource, r *Resource, visitors ...traverser.SchemaTraverser) error {
// ResourceSchema represents a provider's resource schema.
type ResourceSchema map[string]*Resource

// TraverseTFSchemas traverses the Terraform schemas of all the resources of
// the Provider `p` using the specified visitors. Reports any errors
// encountered.
func (s ResourceSchema) TraverseTFSchemas(visitors ...traverser.SchemaTraverser) error {
for name, cfg := range s {
if err := TraverseSchemas(name, cfg, visitors...); err != nil {
return errors.Wrapf(err, "failed to traverse the schema of the Terraform resource with name %q", name)
}
}
return nil
}

// TraverseSchemas visits the specified schema belonging to the Terraform
// resource with the given name and given upjet resource configuration using
// the specified visitors. If any visitors report an error, traversal is
// stopped and the error is reported to the caller.
func TraverseSchemas(tfName string, r *Resource, visitors ...traverser.SchemaTraverser) error {
// set the upjet Resource configuration as context for the visitors that
// satisfy the ResourceSetter interface.
for _, v := range visitors {
if rs, ok := v.(ResourceSetter); ok {
rs.SetResource(r)
}
}
return traverser.Traverse(tfName, tfResource, visitors...)
return traverser.Traverse(tfName, r.TerraformResource, visitors...)
}

type resourceContext struct {
Expand Down
5 changes: 4 additions & 1 deletion pkg/config/schema_conversions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,10 @@ func TestSingletonListEmbedder(t *testing.T) {
t.Run(n, func(t *testing.T) {
e := &SingletonListEmbedder{}
r := DefaultResource(tt.args.name, tt.args.resource, nil, nil)
err := traverseSchemas(tt.args.name, tt.args.resource, r, e)
s := ResourceSchema{
tt.args.name: r,
}
err := s.TraverseTFSchemas(e)
if diff := cmp.Diff(tt.want.err, err, test.EquateErrors()); diff != "" {
t.Fatalf("\n%s\ntraverseSchemas(name, schema, ...): -wantErr, +gotErr:\n%s", tt.reason, diff)
}
Expand Down
58 changes: 58 additions & 0 deletions pkg/schema/traverser/access.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
// SPDX-FileCopyrightText: 2024 The Crossplane Authors <https://crossplane.io>
//
// SPDX-License-Identifier: Apache-2.0

package traverser

import (
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/pkg/errors"
)

// SchemaAccessor is a function that accesses and potentially modifies a
// Terraform schema.
type SchemaAccessor func(*schema.Schema) error

// AccessSchema accesses the schema element at the specified path and calls
// the given schema accessors in order. Reports any errors encountered by
// the accessors. The terminal node at the end of the specified path must be
// a *schema.Resource or an error will be reported. The specified path must
// have at least one component.
func AccessSchema(sch any, path []string, accessors ...SchemaAccessor) error { //nolint:gocyclo // easier to follow the flow
if len(path) == 0 {
return errors.New("empty path specified while accessing the Terraform resource schema")
}
k := path[0]
path = path[1:]
switch s := sch.(type) {
case *schema.Schema:
if len(path) == 0 {
return errors.Errorf("terminal node at key %q is a *schema.Schema", k)
}
if k != wildcard {
return errors.Errorf("expecting a wildcard key but encountered the key %q", k)
}
if err := AccessSchema(s.Elem, path, accessors...); err != nil {
return err
}
case *schema.Resource:
c := s.Schema[k]
if c == nil {
return errors.Errorf("schema element for key %q is nil", k)
}
if len(path) == 0 {
for _, a := range accessors {
if err := a(c); err != nil {
return errors.Wrapf(err, "failed to call the accessor function on the schema element at key %q", k)
}
}
return nil
}
if err := AccessSchema(c, path, accessors...); err != nil {
return err
}
default:
return errors.Errorf("unknown schema element type %T at key %q", s, k)
}
return nil
}
47 changes: 47 additions & 0 deletions pkg/schema/traverser/maxitemssync.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// SPDX-FileCopyrightText: 2024 The Crossplane Authors <https://crossplane.io>
//
// SPDX-License-Identifier: Apache-2.0

package traverser

import (
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/pkg/errors"
)

// maxItemsSync is a visitor to sync the MaxItems constraints from the
// Go schema to the JSON schema. We've observed that some MaxItems constraints
// in the JSON schemas are not set where the corresponding MaxItems constraints
// in the Go schemas are set to 1. This inconsistency results in some singleton
// lists not being properly converted in the MR API whereas at runtime we may
// try to invoke the corresponding Terraform conversion functions. This
// traverser can mitigate such inconsistencies by syncing the MaxItems
// constraints from the Go schema to the JSON schema.
type maxItemsSync struct {
NoopTraverser

jsonSchema TFResourceSchema
}

// NewMaxItemsSync returns a new schema traverser capable of
// syncing the MaxItems constraints from the Go schema to the specified JSON
// schema. This will ensure the generation time and the runtime schema MaxItems
// constraints will be consistent. This is needed only if the generation time
// and runtime schemas differ.
func NewMaxItemsSync(s TFResourceSchema) SchemaTraverser {
return &maxItemsSync{
jsonSchema: s,
}
}

func (m *maxItemsSync) VisitResource(r *ResourceNode) error {
// this visitor only works on singleton lists
if (r.Schema.Type != schema.TypeList && r.Schema.Type != schema.TypeSet) || r.Schema.MaxItems != 1 {
return nil
}
return errors.Wrapf(AccessSchema(m.jsonSchema[r.TFName], r.TFPath,
func(s *schema.Schema) error {
s.MaxItems = 1
return nil
}), "failed to access the schema element at path %v for resource %q", r.TFPath, r.TFName)
}
Loading

0 comments on commit 37c7f4e

Please sign in to comment.