diff --git a/pkg/config/provider.go b/pkg/config/provider.go index 9ae89ecb..d3642522 100644 --- a/pkg/config/provider.go +++ b/pkg/config/provider.go @@ -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")) } } diff --git a/pkg/config/resource.go b/pkg/config/resource.go index e7002629..f1509135 100644 --- a/pkg/config/resource.go +++ b/pkg/config/resource.go @@ -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) { diff --git a/pkg/config/resource_test.go b/pkg/config/resource_test.go index a94366c7..0494ac77 100644 --- a/pkg/config/resource_test.go +++ b/pkg/config/resource_test.go @@ -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 { @@ -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) + } + }) + } +} diff --git a/pkg/config/schema_conversions.go b/pkg/config/schema_conversions.go index 3258a409..d654092b 100644 --- a/pkg/config/schema_conversions.go +++ b/pkg/config/schema_conversions.go @@ -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" ) @@ -17,7 +18,26 @@ 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 { @@ -25,7 +45,7 @@ func traverseSchemas(tfName string, tfResource *schema.Resource, r *Resource, vi rs.SetResource(r) } } - return traverser.Traverse(tfName, tfResource, visitors...) + return traverser.Traverse(tfName, r.TerraformResource, visitors...) } type resourceContext struct { diff --git a/pkg/config/schema_conversions_test.go b/pkg/config/schema_conversions_test.go index 1cd93c43..785fc6bd 100644 --- a/pkg/config/schema_conversions_test.go +++ b/pkg/config/schema_conversions_test.go @@ -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) } diff --git a/pkg/schema/traverser/access.go b/pkg/schema/traverser/access.go new file mode 100644 index 00000000..99e1731b --- /dev/null +++ b/pkg/schema/traverser/access.go @@ -0,0 +1,58 @@ +// SPDX-FileCopyrightText: 2024 The Crossplane Authors +// +// 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 +} diff --git a/pkg/schema/traverser/maxitemssync.go b/pkg/schema/traverser/maxitemssync.go new file mode 100644 index 00000000..0406a07d --- /dev/null +++ b/pkg/schema/traverser/maxitemssync.go @@ -0,0 +1,47 @@ +// SPDX-FileCopyrightText: 2024 The Crossplane Authors +// +// 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) +} diff --git a/pkg/schema/traverser/maxitemssync_test.go b/pkg/schema/traverser/maxitemssync_test.go new file mode 100644 index 00000000..008e06cc --- /dev/null +++ b/pkg/schema/traverser/maxitemssync_test.go @@ -0,0 +1,142 @@ +// SPDX-FileCopyrightText: 2024 The Crossplane Authors +// +// SPDX-License-Identifier: Apache-2.0 + +package traverser + +import ( + "testing" + + "github.com/crossplane/crossplane-runtime/pkg/test" + "github.com/google/go-cmp/cmp" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" +) + +func TestMaxItemsSync(t *testing.T) { + type args struct { + srcSchema TFResourceSchema + targetSchema TFResourceSchema + } + type want struct { + targetSchema TFResourceSchema + err error + } + cases := map[string]struct { + reason string + args + want + }{ + "SyncMaxItemsConstraints": { + reason: `maxItemsSync traverser can successfully sync the "MaxItems = 1" constraints from the source schema to the target schema.`, + args: args{ + srcSchema: map[string]*schema.Resource{ + "test_resource": { + Schema: map[string]*schema.Schema{ + "argument": { + MaxItems: 1, + Type: schema.TypeList, + Elem: &schema.Resource{}, + }, + }, + }, + }, + targetSchema: map[string]*schema.Resource{ + "test_resource": { + Schema: map[string]*schema.Schema{ + "argument": { + MaxItems: 0, + Type: schema.TypeList, + Elem: &schema.Resource{}, + }, + }, + }, + }, + }, + want: want{ + targetSchema: map[string]*schema.Resource{ + "test_resource": { + Schema: map[string]*schema.Schema{ + "argument": { + MaxItems: 1, + Type: schema.TypeList, + Elem: &schema.Resource{}, + }, + }, + }, + }, + }, + }, + "NoSyncMaxItems": { + reason: "If the MaxItems constraint is greater than 1, then maxItemsSync should not sync the constraint.", + args: args{ + srcSchema: map[string]*schema.Resource{ + "test_resource": { + Schema: map[string]*schema.Schema{ + "argument": { + MaxItems: 2, + Type: schema.TypeList, + Elem: &schema.Resource{}, + }, + }, + }, + }, + targetSchema: map[string]*schema.Resource{ + "test_resource": { + Schema: map[string]*schema.Schema{ + "argument": { + MaxItems: 0, + Type: schema.TypeList, + Elem: &schema.Resource{}, + }, + }, + }, + }, + }, + want: want{ + targetSchema: map[string]*schema.Resource{ + "test_resource": { + Schema: map[string]*schema.Schema{ + "argument": { + MaxItems: 0, + Type: schema.TypeList, + Elem: &schema.Resource{}, + }, + }, + }, + }, + }, + }, + } + for n, tc := range cases { + t.Run(n, func(t *testing.T) { + copySrc := copySchema(tc.args.srcSchema) + got := tc.args.srcSchema.Traverse(NewMaxItemsSync(tc.args.targetSchema)) + if diff := cmp.Diff(tc.want.err, got, test.EquateErrors()); diff != "" { + t.Errorf("%s\nMaxItemsSync: -wantErr, +gotErr: \n%s", tc.reason, diff) + } + if diff := cmp.Diff(tc.want.err, got, test.EquateErrors()); diff != "" { + t.Errorf("%s\nMaxItemsSync: -wantErr, +gotErr: \n%s", tc.reason, diff) + } + if diff := cmp.Diff(copySrc, tc.srcSchema); diff != "" { + t.Errorf("%s\nMaxItemsSync: -wantSourceSchema, +gotSourceSchema: \n%s", tc.reason, diff) + } + if diff := cmp.Diff(tc.want.targetSchema, tc.args.targetSchema); diff != "" { + t.Errorf("%s\nMaxItemsSync: -wantTargetSchema, +gotTargetSchema: \n%s", tc.reason, diff) + } + }) + } +} + +func copySchema(s TFResourceSchema) TFResourceSchema { + result := make(TFResourceSchema) + for k, v := range s { + c := *v + for n, s := range v.Schema { + // not a deep copy but sufficient to check the schema constraints + s := *s + c.Schema[n] = &s + } + result[k] = &c + } + return result +} diff --git a/pkg/schema/traverser/traverse.go b/pkg/schema/traverser/traverse.go index 8846f60f..725bff2c 100644 --- a/pkg/schema/traverser/traverse.go +++ b/pkg/schema/traverser/traverse.go @@ -140,3 +140,17 @@ func (n NoopTraverser) VisitSchema(*SchemaNode) error { func (n NoopTraverser) VisitResource(*ResourceNode) error { return nil } + +// TFResourceSchema represents a provider's Terraform resource schema. +type TFResourceSchema map[string]*schema.Resource + +// Traverse traverses the receiver schema using the specified +// visitors. Reports any errors encountered by the visitors. +func (s TFResourceSchema) Traverse(visitors ...SchemaTraverser) error { + for n, r := range s { + if err := Traverse(n, r, visitors...); err != nil { + return errors.Wrapf(err, "failed to traverse the schema of the Terraform resource with name %q", n) + } + } + return nil +}