From 8ef1aa2a8cf1938a607bc7e2d11f5d3826b9862b Mon Sep 17 00:00:00 2001 From: Nic Cope Date: Sat, 11 Nov 2023 23:08:51 -0800 Subject: [PATCH 01/21] Add server-side apply merge strategy markers This commit adds SSA merge strategy markers that allow the API server to granularly merge lists, rather than atomically replacing them. Composition functions use SSA, so this change means that a function can return only the list elements it desires (e.g. tags) and those elements will be merged into any existing elements, without replacing them. For the moment I've only covered two cases: * Lists that we know are sets of scalar values (generated from Terraform sets) * Maps of scalar values (generated from Terraform maps) I'm hopeful that in both of these cases it _should_ be possible to allow the map or set to be granularly merged, not atomically replaced. https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy Signed-off-by: Nic Cope --- pkg/types/field.go | 30 ++++++++++++ pkg/types/markers/options.go | 4 +- pkg/types/markers/ssa.go | 87 +++++++++++++++++++++++++++++++++++ pkg/types/markers/ssa_test.go | 49 ++++++++++++++++++++ 4 files changed, 169 insertions(+), 1 deletion(-) create mode 100644 pkg/types/markers/ssa.go create mode 100644 pkg/types/markers/ssa_test.go diff --git a/pkg/types/field.go b/pkg/types/field.go index 838d9073..d2ed2325 100644 --- a/pkg/types/field.go +++ b/pkg/types/field.go @@ -15,6 +15,7 @@ import ( "github.com/crossplane/upjet/pkg" "github.com/crossplane/upjet/pkg/config" "github.com/crossplane/upjet/pkg/types/comments" + "github.com/crossplane/upjet/pkg/types/markers" "github.com/crossplane/upjet/pkg/types/name" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/pkg/errors" @@ -151,9 +152,38 @@ func NewField(g *Builder, cfg *config.Resource, r *resource, sch *schema.Schema, f.FieldType = fieldType f.InitType = initType + AddServerSideApplyMarkers(f) + return f, nil } +// AddServerSideApplyMarkers adds server-side apply comment markers to indicate +// that scalar maps and sets can be merged granularly, not replace atomically. +func AddServerSideApplyMarkers(f *Field) { + switch f.Schema.Type { //nolint:exhaustive + case schema.TypeMap: + // A map should always have an element of type Schema. + if es, ok := f.Schema.Elem.(*schema.Schema); ok { + switch es.Type { //nolint:exhaustive + // We assume scalar types can be granular maps. + case schema.TypeString, schema.TypeBool, schema.TypeInt, schema.TypeFloat: + f.Comment.ServerSideApplyOptions.MapType = ptr.To[markers.MapType](markers.MapTypeGranular) + } + } + case schema.TypeSet: + if es, ok := f.Schema.Elem.(*schema.Schema); ok { + switch es.Type { //nolint:exhaustive + // We assume scalar types can be granular maps. + case schema.TypeString, schema.TypeBool, schema.TypeInt, schema.TypeFloat: + f.Comment.ServerSideApplyOptions.ListType = ptr.To[markers.ListType](markers.ListTypeSet) + } + } + } + // TODO(negz): Can we reliably add SSA markers for lists of objects? Do we + // have cases where we're turning a Terraform map of maps into a list of + // objects with a well-known key that we could merge on? +} + // NewSensitiveField returns a constructed sensitive Field object. func NewSensitiveField(g *Builder, cfg *config.Resource, r *resource, sch *schema.Schema, snakeFieldName string, tfPath, xpPath, names []string, asBlocksMode bool) (*Field, bool, error) { //nolint:gocyclo f, err := NewField(g, cfg, r, sch, snakeFieldName, tfPath, xpPath, names, asBlocksMode) diff --git a/pkg/types/markers/options.go b/pkg/types/markers/options.go index fb5e06ae..c64b5773 100644 --- a/pkg/types/markers/options.go +++ b/pkg/types/markers/options.go @@ -9,11 +9,13 @@ type Options struct { UpjetOptions CrossplaneOptions KubebuilderOptions + ServerSideApplyOptions } // String returns a string representation of this Options object. func (o Options) String() string { return o.UpjetOptions.String() + o.CrossplaneOptions.String() + - o.KubebuilderOptions.String() + o.KubebuilderOptions.String() + + o.ServerSideApplyOptions.String() } diff --git a/pkg/types/markers/ssa.go b/pkg/types/markers/ssa.go new file mode 100644 index 00000000..c6891fc4 --- /dev/null +++ b/pkg/types/markers/ssa.go @@ -0,0 +1,87 @@ +// SPDX-FileCopyrightText: 2023 The Crossplane Authors +// +// SPDX-License-Identifier: Apache-2.0 + +package markers + +import "fmt" + +// A ListType is a type of list. +type ListType string + +// Types of lists. +const ( + // ListTypeAtomic means the entire list is replaced during merge. At any + // point in time, a single manager owns the list. + ListTypeAtomic ListType = "atomic" + + // ListTypeSet can be granularly merged, and different managers can own + // different elements in the list. The list can include only scalar + // elements. + ListTypeSet ListType = "set" + + // ListTypeSet can be granularly merged, and different managers can own + // different elements in the list. The list can include only nested types + // (i.e. objects). + ListTypeMap ListType = "map" +) + +// A MapType is a type of map. +type MapType string + +// Types of maps. +const ( + // MapTypeAtomic means that the map can only be entirely replaced by a + // single manager. + MapTypeAtomic MapType = "atomic" + + // MapTypeGranular means that the map supports separate managers updating + // individual fields. + MapTypeGranular MapType = "granular" +) + +// A StructType is a type of struct. +type StructType string + +// Struct types. +const ( + // StructTypeAtomic means that the struct can only be entirely replaced by a + // single manager. + StructTypeAtomic StructType = "atomic" + + // StructTypeGranular means that the struct supports separate managers + // updating individual fields. + StructTypeGranular StructType = "granular" +) + +// ServerSideApplyOptions represents the server-side apply merge options that +// upjet needs to control. +// https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy +type ServerSideApplyOptions struct { + ListType *ListType + ListMapKey []string + MapType *MapType + StructType *StructType +} + +func (o ServerSideApplyOptions) String() string { + m := "" + + if o.ListType != nil { + m += fmt.Sprintf("+listType:%s\n", *o.ListType) + } + + for _, k := range o.ListMapKey { + m += fmt.Sprintf("+listMapKey:%s\n", k) + } + + if o.MapType != nil { + m += fmt.Sprintf("+mapType:%s\n", *o.MapType) + } + + if o.StructType != nil { + m += fmt.Sprintf("+structType:%s\n", *o.StructType) + } + + return m +} diff --git a/pkg/types/markers/ssa_test.go b/pkg/types/markers/ssa_test.go new file mode 100644 index 00000000..062d808c --- /dev/null +++ b/pkg/types/markers/ssa_test.go @@ -0,0 +1,49 @@ +// SPDX-FileCopyrightText: 2023 The Crossplane Authors +// +// SPDX-License-Identifier: Apache-2.0 + +package markers + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "k8s.io/utils/ptr" +) + +func TestServerSideApplyOptions(t *testing.T) { + cases := map[string]struct { + o ServerSideApplyOptions + want string + }{ + "MapType": { + o: ServerSideApplyOptions{ + MapType: ptr.To[MapType](MapTypeAtomic), + }, + want: "+mapType:atomic\n", + }, + "StructType": { + o: ServerSideApplyOptions{ + StructType: ptr.To[StructType](StructTypeAtomic), + }, + want: "+structType:atomic\n", + }, + "ListType": { + o: ServerSideApplyOptions{ + ListType: ptr.To[ListType](ListTypeMap), + ListMapKey: []string{"name", "coolness"}, + }, + want: "+listType:map\n+listMapKey:name\n+listMapKey:coolness\n", + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + got := tc.o.String() + + if diff := cmp.Diff(tc.want, got); diff != "" { + t.Errorf("o.String(): -want, +got: %s", diff) + } + }) + } +} From bed1fa2fdde426e6beaf61aa51f490e0303b6f17 Mon Sep 17 00:00:00 2001 From: Alper Rifat Ulucinar Date: Thu, 16 Nov 2023 15:23:51 +0300 Subject: [PATCH 02/21] Post release commit after v1.0.0 Signed-off-by: Alper Rifat Ulucinar From 0752be698fe140dadaac961f4ef04fde0d6888b0 Mon Sep 17 00:00:00 2001 From: Mitch Connors Date: Tue, 27 Jun 2023 21:17:09 +0000 Subject: [PATCH 03/21] Make main-tf contents exported Signed-off-by: Mitch Connors --- pkg/terraform/files.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/pkg/terraform/files.go b/pkg/terraform/files.go index 78adac16..c769f62c 100644 --- a/pkg/terraform/files.go +++ b/pkg/terraform/files.go @@ -130,9 +130,7 @@ type FileProducer struct { features *feature.Flags } -// WriteMainTF writes the content main configuration file that has the desired -// state configuration for Terraform. -func (fp *FileProducer) WriteMainTF() (ProviderHandle, error) { +func (fp *FileProducer) BuildMainTF() map[string]any { // If the resource is in a deletion process, we need to remove the deletion // protection. lifecycle := map[string]any{ @@ -153,7 +151,7 @@ func (fp *FileProducer) WriteMainTF() (ProviderHandle, error) { // Note(turkenh): To use third party providers, we need to configure // provider name in required_providers. providerSource := strings.Split(fp.Setup.Requirement.Source, "/") - m := map[string]any{ + return map[string]any{ "terraform": map[string]any{ "required_providers": map[string]any{ providerSource[len(providerSource)-1]: map[string]string{ @@ -171,6 +169,12 @@ func (fp *FileProducer) WriteMainTF() (ProviderHandle, error) { }, }, } +} + +// WriteMainTF writes the content main configuration file that has the desired +// state configuration for Terraform. +func (fp *FileProducer) WriteMainTF() error { + m := fp.BuildMainTF() rawMainTF, err := json.JSParser.Marshal(m) if err != nil { return InvalidProviderHandle, errors.Wrap(err, "cannot marshal main hcl object") From c40331d552a883843b3390608822afd9b69bce84 Mon Sep 17 00:00:00 2001 From: Mitch Connors Date: Mon, 20 Nov 2023 16:23:01 +0000 Subject: [PATCH 04/21] fix signature Signed-off-by: Mitch Connors --- pkg/terraform/files.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/terraform/files.go b/pkg/terraform/files.go index c769f62c..c429c6cb 100644 --- a/pkg/terraform/files.go +++ b/pkg/terraform/files.go @@ -173,7 +173,7 @@ func (fp *FileProducer) BuildMainTF() map[string]any { // WriteMainTF writes the content main configuration file that has the desired // state configuration for Terraform. -func (fp *FileProducer) WriteMainTF() error { +func (fp *FileProducer) WriteMainTF() (ProviderHandle, error) { m := fp.BuildMainTF() rawMainTF, err := json.JSParser.Marshal(m) if err != nil { From 5150500b82b67cf882591ef7ed751a07fb52db4a Mon Sep 17 00:00:00 2001 From: Alper Rifat Ulucinar Date: Fri, 1 Dec 2023 03:01:30 +0300 Subject: [PATCH 05/21] Fix kubebuilder topological markers: use "=" instead of ":" between tokens - Do not add topological markers for sensitive fields Signed-off-by: Alper Rifat Ulucinar --- pkg/types/field.go | 6 ++++-- pkg/types/markers/ssa.go | 10 +++++----- pkg/types/markers/ssa_test.go | 6 +++--- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/pkg/types/field.go b/pkg/types/field.go index d2ed2325..97d6e9c9 100644 --- a/pkg/types/field.go +++ b/pkg/types/field.go @@ -152,7 +152,9 @@ func NewField(g *Builder, cfg *config.Resource, r *resource, sch *schema.Schema, f.FieldType = fieldType f.InitType = initType - AddServerSideApplyMarkers(f) + if !sch.Sensitive { + AddServerSideApplyMarkers(f) + } return f, nil } @@ -173,7 +175,7 @@ func AddServerSideApplyMarkers(f *Field) { case schema.TypeSet: if es, ok := f.Schema.Elem.(*schema.Schema); ok { switch es.Type { //nolint:exhaustive - // We assume scalar types can be granular maps. + // We assume scalar types can be granular sets. case schema.TypeString, schema.TypeBool, schema.TypeInt, schema.TypeFloat: f.Comment.ServerSideApplyOptions.ListType = ptr.To[markers.ListType](markers.ListTypeSet) } diff --git a/pkg/types/markers/ssa.go b/pkg/types/markers/ssa.go index c6891fc4..6a26c875 100644 --- a/pkg/types/markers/ssa.go +++ b/pkg/types/markers/ssa.go @@ -20,7 +20,7 @@ const ( // elements. ListTypeSet ListType = "set" - // ListTypeSet can be granularly merged, and different managers can own + // ListTypeMap can be granularly merged, and different managers can own // different elements in the list. The list can include only nested types // (i.e. objects). ListTypeMap ListType = "map" @@ -68,19 +68,19 @@ func (o ServerSideApplyOptions) String() string { m := "" if o.ListType != nil { - m += fmt.Sprintf("+listType:%s\n", *o.ListType) + m += fmt.Sprintf("+listType=%s\n", *o.ListType) } for _, k := range o.ListMapKey { - m += fmt.Sprintf("+listMapKey:%s\n", k) + m += fmt.Sprintf("+listMapKey=%s\n", k) } if o.MapType != nil { - m += fmt.Sprintf("+mapType:%s\n", *o.MapType) + m += fmt.Sprintf("+mapType=%s\n", *o.MapType) } if o.StructType != nil { - m += fmt.Sprintf("+structType:%s\n", *o.StructType) + m += fmt.Sprintf("+structType=%s\n", *o.StructType) } return m diff --git a/pkg/types/markers/ssa_test.go b/pkg/types/markers/ssa_test.go index 062d808c..dbfa5513 100644 --- a/pkg/types/markers/ssa_test.go +++ b/pkg/types/markers/ssa_test.go @@ -20,20 +20,20 @@ func TestServerSideApplyOptions(t *testing.T) { o: ServerSideApplyOptions{ MapType: ptr.To[MapType](MapTypeAtomic), }, - want: "+mapType:atomic\n", + want: "+mapType=atomic\n", }, "StructType": { o: ServerSideApplyOptions{ StructType: ptr.To[StructType](StructTypeAtomic), }, - want: "+structType:atomic\n", + want: "+structType=atomic\n", }, "ListType": { o: ServerSideApplyOptions{ ListType: ptr.To[ListType](ListTypeMap), ListMapKey: []string{"name", "coolness"}, }, - want: "+listType:map\n+listMapKey:name\n+listMapKey:coolness\n", + want: "+listType=map\n+listMapKey=name\n+listMapKey=coolness\n", }, } From f99dee311532b400cdc81c464fd69c4c7e52a582 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sergen=20Yal=C3=A7=C4=B1n?= Date: Tue, 5 Dec 2023 17:15:44 +0300 Subject: [PATCH 06/21] Pass default and configured timeouts to InstanceState for using the timeouts while Observe calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Sergen Yalçın --- pkg/controller/external_nofork.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/pkg/controller/external_nofork.go b/pkg/controller/external_nofork.go index 3bcd674b..bd4d601f 100644 --- a/pkg/controller/external_nofork.go +++ b/pkg/controller/external_nofork.go @@ -227,7 +227,7 @@ func (c *NoForkConnector) applyStateFuncToParam(sc *schema.Schema, param any) an return param } -func (c *NoForkConnector) Connect(ctx context.Context, mg xpresource.Managed) (managed.ExternalClient, error) { +func (c *NoForkConnector) Connect(ctx context.Context, mg xpresource.Managed) (managed.ExternalClient, error) { //nolint:gocyclo c.metricRecorder.ObserveReconcileDelay(mg.GetObjectKind().GroupVersionKind(), mg.GetName()) logger := c.logger.WithValues("uid", mg.GetUID(), "name", mg.GetName(), "gvk", mg.GetObjectKind().GroupVersionKind().String()) logger.Debug("Connecting to the service provider") @@ -279,6 +279,17 @@ func (c *NoForkConnector) Connect(ctx context.Context, mg xpresource.Managed) (m } s.RawPlan = tfStateCtyValue s.RawConfig = rawConfig + + timeouts := getTimeoutParameters(c.config) + if len(timeouts) > 0 { + if s == nil { + s = &tf.InstanceState{} + } + if s.Meta == nil { + s.Meta = make(map[string]interface{}) + } + s.Meta[schema.TimeoutKey] = timeouts + } opTracker.SetTfState(s) } From 25a4aa59de315e05c0cb86dd0b3b5eb2f2810e5e Mon Sep 17 00:00:00 2001 From: Mitch Connors Date: Wed, 29 Nov 2023 18:47:48 +0000 Subject: [PATCH 07/21] Add exported function description Signed-off-by: Mitch Connors --- pkg/terraform/files.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/terraform/files.go b/pkg/terraform/files.go index c429c6cb..26a8fe1c 100644 --- a/pkg/terraform/files.go +++ b/pkg/terraform/files.go @@ -130,6 +130,8 @@ type FileProducer struct { features *feature.Flags } +// BuildMainTF produces the contents of the mainTF file as a map. This format is conducive to +// inspection for tests. WriteMainTF calls this function an serializes the result to a file as JSON. func (fp *FileProducer) BuildMainTF() map[string]any { // If the resource is in a deletion process, we need to remove the deletion // protection. From 7431179b0bd7f6789e2371221078cf9704081fa3 Mon Sep 17 00:00:00 2001 From: Alper Rifat Ulucinar Date: Tue, 12 Dec 2023 12:13:15 +0300 Subject: [PATCH 08/21] Call the registered schema.CustomizeDiffFunc functions in the Terraform SDK-based external client Signed-off-by: Alper Rifat Ulucinar --- pkg/controller/external_nofork.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/external_nofork.go b/pkg/controller/external_nofork.go index bd4d601f..6197b220 100644 --- a/pkg/controller/external_nofork.go +++ b/pkg/controller/external_nofork.go @@ -416,7 +416,7 @@ func getTimeoutParameters(config *config.Resource) map[string]any { //nolint:goc func (n *noForkExternal) getResourceDataDiff(tr resource.Terraformed, ctx context.Context, s *tf.InstanceState, resourceExists bool) (*tf.InstanceDiff, error) { //nolint:gocyclo resourceConfig := tf.NewResourceConfigRaw(n.params) - instanceDiff, err := schema.InternalMap(n.config.TerraformResource.Schema).Diff(ctx, s, resourceConfig, nil, n.ts.Meta, false) + instanceDiff, err := schema.InternalMap(n.config.TerraformResource.Schema).Diff(ctx, s, resourceConfig, n.config.TerraformResource.CustomizeDiff, n.ts.Meta, false) if err != nil { return nil, errors.Wrap(err, "failed to get *terraform.InstanceDiff") } From 085ff0a262d4b52ab354a893a3320cbd167ed030 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sergen=20Yal=C3=A7=C4=B1n?= Date: Tue, 12 Dec 2023 14:29:14 +0300 Subject: [PATCH 09/21] Ignore specific error that returned by expandWildcard function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Sergen Yalçın --- pkg/resource/sensitive.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/resource/sensitive.go b/pkg/resource/sensitive.go index 229ddd97..54959e9e 100644 --- a/pkg/resource/sensitive.go +++ b/pkg/resource/sensitive.go @@ -107,6 +107,9 @@ func GetSensitiveAttributes(from map[string]any, mapping map[string]string) (map for tf := range mapping { fieldPaths, err := paved.ExpandWildcards(tf) if err != nil { + if fieldpath.IsNotFound(err) { + continue + } return nil, errors.Wrap(err, errCannotExpandWildcards) } From 0fc0a0744754ef36587f7624b0e8a29068fbd5ee Mon Sep 17 00:00:00 2001 From: Alper Rifat Ulucinar Date: Fri, 1 Dec 2023 03:01:30 +0300 Subject: [PATCH 10/21] Add config.Resource.ServerSideApplyMergeStrategies to be able to configure the server-side apply merge strategies for object lists & maps. Signed-off-by: Alper Rifat Ulucinar --- pkg/config/common.go | 23 +++---- pkg/config/common_test.go | 95 ++++++++++++++------------- pkg/config/resource.go | 108 +++++++++++++++++++++++++++++++ pkg/types/builder.go | 48 +++++++++++++- pkg/types/builder_test.go | 6 +- pkg/types/field.go | 100 ++++++++++++++++++++++++---- pkg/types/markers/kubebuilder.go | 4 ++ pkg/types/markers/ssa.go | 56 ++-------------- pkg/types/markers/ssa_test.go | 8 ++- 9 files changed, 323 insertions(+), 125 deletions(-) diff --git a/pkg/config/common.go b/pkg/config/common.go index ee291e39..eb5c0b8d 100644 --- a/pkg/config/common.go +++ b/pkg/config/common.go @@ -77,17 +77,18 @@ func DefaultResource(name string, terraformSchema *schema.Resource, terraformReg } r := &Resource{ - Name: name, - TerraformResource: terraformSchema, - MetaResource: terraformRegistry, - ShortGroup: group, - Kind: kind, - Version: "v1alpha1", - ExternalName: NameAsIdentifier, - References: map[string]Reference{}, - Sensitive: NopSensitive, - UseAsync: true, - SchemaElementOptions: make(map[string]*SchemaElementOption), + Name: name, + TerraformResource: terraformSchema, + MetaResource: terraformRegistry, + ShortGroup: group, + Kind: kind, + Version: "v1alpha1", + ExternalName: NameAsIdentifier, + References: make(References), + Sensitive: NopSensitive, + UseAsync: true, + SchemaElementOptions: make(SchemaElementOptions), + ServerSideApplyMergeStrategies: make(ServerSideApplyMergeStrategies), } for _, f := range opts { f(r) diff --git a/pkg/config/common_test.go b/pkg/config/common_test.go index 3ad62cab..4b8a7207 100644 --- a/pkg/config/common_test.go +++ b/pkg/config/common_test.go @@ -33,15 +33,16 @@ func TestDefaultResource(t *testing.T) { name: "aws_ec2_instance", }, want: &Resource{ - Name: "aws_ec2_instance", - ShortGroup: "ec2", - Kind: "Instance", - Version: "v1alpha1", - ExternalName: NameAsIdentifier, - References: map[string]Reference{}, - Sensitive: NopSensitive, - UseAsync: true, - SchemaElementOptions: SchemaElementOptions{}, + Name: "aws_ec2_instance", + ShortGroup: "ec2", + Kind: "Instance", + Version: "v1alpha1", + ExternalName: NameAsIdentifier, + References: map[string]Reference{}, + Sensitive: NopSensitive, + UseAsync: true, + SchemaElementOptions: SchemaElementOptions{}, + ServerSideApplyMergeStrategies: ServerSideApplyMergeStrategies{}, }, }, "TwoSectionsName": { @@ -50,15 +51,16 @@ func TestDefaultResource(t *testing.T) { name: "aws_instance", }, want: &Resource{ - Name: "aws_instance", - ShortGroup: "aws", - Kind: "Instance", - Version: "v1alpha1", - ExternalName: NameAsIdentifier, - References: map[string]Reference{}, - Sensitive: NopSensitive, - UseAsync: true, - SchemaElementOptions: SchemaElementOptions{}, + Name: "aws_instance", + ShortGroup: "aws", + Kind: "Instance", + Version: "v1alpha1", + ExternalName: NameAsIdentifier, + References: map[string]Reference{}, + Sensitive: NopSensitive, + UseAsync: true, + SchemaElementOptions: SchemaElementOptions{}, + ServerSideApplyMergeStrategies: ServerSideApplyMergeStrategies{}, }, }, "NameWithPrefixAcronym": { @@ -67,15 +69,16 @@ func TestDefaultResource(t *testing.T) { name: "aws_db_sql_server", }, want: &Resource{ - Name: "aws_db_sql_server", - ShortGroup: "db", - Kind: "SQLServer", - Version: "v1alpha1", - ExternalName: NameAsIdentifier, - References: map[string]Reference{}, - Sensitive: NopSensitive, - UseAsync: true, - SchemaElementOptions: SchemaElementOptions{}, + Name: "aws_db_sql_server", + ShortGroup: "db", + Kind: "SQLServer", + Version: "v1alpha1", + ExternalName: NameAsIdentifier, + References: map[string]Reference{}, + Sensitive: NopSensitive, + UseAsync: true, + SchemaElementOptions: SchemaElementOptions{}, + ServerSideApplyMergeStrategies: ServerSideApplyMergeStrategies{}, }, }, "NameWithSuffixAcronym": { @@ -84,15 +87,16 @@ func TestDefaultResource(t *testing.T) { name: "aws_db_server_id", }, want: &Resource{ - Name: "aws_db_server_id", - ShortGroup: "db", - Kind: "ServerID", - Version: "v1alpha1", - ExternalName: NameAsIdentifier, - References: map[string]Reference{}, - Sensitive: NopSensitive, - UseAsync: true, - SchemaElementOptions: SchemaElementOptions{}, + Name: "aws_db_server_id", + ShortGroup: "db", + Kind: "ServerID", + Version: "v1alpha1", + ExternalName: NameAsIdentifier, + References: map[string]Reference{}, + Sensitive: NopSensitive, + UseAsync: true, + SchemaElementOptions: SchemaElementOptions{}, + ServerSideApplyMergeStrategies: ServerSideApplyMergeStrategies{}, }, }, "NameWithMultipleAcronyms": { @@ -101,15 +105,16 @@ func TestDefaultResource(t *testing.T) { name: "aws_db_sql_server_id", }, want: &Resource{ - Name: "aws_db_sql_server_id", - ShortGroup: "db", - Kind: "SQLServerID", - Version: "v1alpha1", - ExternalName: NameAsIdentifier, - References: map[string]Reference{}, - Sensitive: NopSensitive, - UseAsync: true, - SchemaElementOptions: SchemaElementOptions{}, + Name: "aws_db_sql_server_id", + ShortGroup: "db", + Kind: "SQLServerID", + Version: "v1alpha1", + ExternalName: NameAsIdentifier, + References: map[string]Reference{}, + Sensitive: NopSensitive, + UseAsync: true, + SchemaElementOptions: SchemaElementOptions{}, + ServerSideApplyMergeStrategies: ServerSideApplyMergeStrategies{}, }, }, } diff --git a/pkg/config/resource.go b/pkg/config/resource.go index 876ec6f5..1ad1e777 100644 --- a/pkg/config/resource.go +++ b/pkg/config/resource.go @@ -24,6 +24,54 @@ import ( "github.com/crossplane/upjet/pkg/registry" ) +// A ListType is a type of list. +type ListType string + +// Types of lists. +const ( + // ListTypeAtomic means the entire list is replaced during merge. At any + // point in time, a single manager owns the list. + ListTypeAtomic ListType = "atomic" + + // ListTypeSet can be granularly merged, and different managers can own + // different elements in the list. The list can include only scalar + // elements. + ListTypeSet ListType = "set" + + // ListTypeMap can be granularly merged, and different managers can own + // different elements in the list. The list can include only nested types + // (i.e. objects). + ListTypeMap ListType = "map" +) + +// A MapType is a type of map. +type MapType string + +// Types of maps. +const ( + // MapTypeAtomic means that the map can only be entirely replaced by a + // single manager. + MapTypeAtomic MapType = "atomic" + + // MapTypeGranular means that the map supports separate managers updating + // individual fields. + MapTypeGranular MapType = "granular" +) + +// A StructType is a type of struct. +type StructType string + +// Struct types. +const ( + // StructTypeAtomic means that the struct can only be entirely replaced by a + // single manager. + StructTypeAtomic StructType = "atomic" + + // StructTypeGranular means that the struct supports separate managers + // updating individual fields. + StructTypeGranular StructType = "granular" +) + // SetIdentifierArgumentsFn sets the name of the resource in Terraform attributes map, // i.e. Main HCL file. type SetIdentifierArgumentsFn func(base map[string]any, externalName string) @@ -266,6 +314,60 @@ func setExternalTagsWithPaved(externalTags map[string]string, paved *fieldpath.P return pavedByte, nil } +type InjectedKey struct { + Key string + DefaultValue string +} + +// ListMapKeys is the list map keys when the server-side apply merge strategy +// islistType=map. +type ListMapKeys struct { + // InjectedKey can be used to inject the specified index key + // into the generated CRD schema for the list object when + // the SSA merge strategy for the parent list is `map`. + // If a non-zero `InjectedKey` is specified, then a field of type string with + // the specified name is injected into the Terraform schema and used as + // a list map key together with any other existing keys specified in `Keys`. + InjectedKey InjectedKey + // Keys is the set of list map keys to be used while SSA merges list items. + // If InjectedKey is non-zero, then it's automatically put into Keys and + // you must not specify the InjectedKey in Keys explicitly. + Keys []string +} + +// ListMergeStrategy configures the corresponding field as list +// and configures its server-side apply merge strategy. +type ListMergeStrategy struct { + // ListMapKeys is the list map keys when the SSA merge strategy is + // `listType=map`. The keys specified here must be a set of scalar Terraform + // argument names to be used as the list map keys for the object list. + ListMapKeys ListMapKeys + // MergeStrategy is the SSA merge strategy for an object list. Valid values + // are: `atomic`, `set` and `map` + MergeStrategy ListType +} + +// MergeStrategy configures the server-side apply merge strategy for the +// corresponding field. One and only one of the pointer members can be set +// and the specified merge strategy configuration must match the field's +// type, e.g., you cannot set MapMergeStrategy for a field of type list. +type MergeStrategy struct { + ListMergeStrategy ListMergeStrategy + MapMergeStrategy MapType + StructMergeStrategy StructType +} + +// ServerSideApplyMergeStrategies configures the server-side apply merge strategy +// for the field at the specified path as the map key. The key is +// a Terraform configuration argument path such as a.b.c, without any +// index notation (i.e., array/map components do not need indices). +// It's an error to set a configuration option which does not match +// the object type at the specified path or to leave the corresponding +// configuration entry empty. For example, if the field at path a.b.c is +// a list, then ListMergeStrategy must be set and it should be the only +// configuration entry set. +type ServerSideApplyMergeStrategies map[string]MergeStrategy + // Resource is the set of information that you can override at different steps // of the code generation pipeline. type Resource struct { @@ -338,6 +440,12 @@ type Resource struct { // Terraform InstanceDiff is computed during reconciliation. TerraformCustomDiff CustomDiff + // ServerSideApplyMergeStrategies configures the server-side apply merge + // strategy for the fields at the given map keys. The map key is + // a Terraform configuration argument path such as a.b.c, without any + // index notation (i.e., array/map components do not need indices). + ServerSideApplyMergeStrategies ServerSideApplyMergeStrategies + // useNoForkClient indicates that a no-fork external client should // be generated instead of the Terraform CLI-forking client. useNoForkClient bool diff --git a/pkg/types/builder.go b/pkg/types/builder.go index b4dbc3ef..3c476fe8 100644 --- a/pkg/types/builder.go +++ b/pkg/types/builder.go @@ -27,6 +27,9 @@ const ( // ref: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#validation-rules celEscapeSequence = "__%s__" + // description for an injected list map key field in the context of the + // server-side apply object list merging + descriptionInjectedKey = "This is an injected field with a default value for being able to merge items of the parent object list." ) var ( @@ -67,6 +70,10 @@ func NewBuilder(pkg *types.Package) *Builder { // Build returns parameters and observation types built out of Terraform schema. func (g *Builder) Build(cfg *config.Resource) (Generated, error) { + if err := injectServerSideApplyListMergeKeys(cfg); err != nil { + return Generated{}, errors.Wrapf(err, "cannot inject server-side apply merge keys for resource %q", cfg.Name) + } + fp, ap, ip, err := g.buildResource(cfg.TerraformResource, cfg, nil, nil, false, cfg.Kind) return Generated{ Types: g.genTypes, @@ -75,7 +82,46 @@ func (g *Builder) Build(cfg *config.Resource) (Generated, error) { InitProviderType: ip, AtProviderType: ap, ValidationRules: g.validationRules, - }, errors.Wrapf(err, "cannot build the Types") + }, errors.Wrapf(err, "cannot build the Types for resource %q", cfg.Name) +} + +func injectServerSideApplyListMergeKeys(cfg *config.Resource) error { //nolint:gocyclo // Easier to follow the logic in a single function + for f, s := range cfg.ServerSideApplyMergeStrategies { + if s.ListMergeStrategy.MergeStrategy != config.ListTypeMap { + continue + } + if s.ListMergeStrategy.ListMapKeys.InjectedKey.Key == "" && len(s.ListMergeStrategy.ListMapKeys.Keys) == 0 { + return errors.Errorf("list map keys configuration for the object list %q is empty", f) + } + if s.ListMergeStrategy.ListMapKeys.InjectedKey.Key == "" { + continue + } + sch := config.GetSchema(cfg.TerraformResource, f) + if sch == nil { + return errors.Errorf("cannot find the Terraform schema for the argument at the path %q", f) + } + if sch.Type != schema.TypeList && sch.Type != schema.TypeSet { + return errors.Errorf("fieldpath %q is not a Terraform list or set", f) + } + el, ok := sch.Elem.(*schema.Resource) + if !ok { + return errors.Errorf("fieldpath %q is a Terraform list or set but its element type is not a Terraform *schema.Resource", f) + } + for k := range el.Schema { + if k == s.ListMergeStrategy.ListMapKeys.InjectedKey.Key { + return errors.Errorf("element schema for the object list %q already contains the argument key %q", f, k) + } + } + el.Schema[s.ListMergeStrategy.ListMapKeys.InjectedKey.Key] = &schema.Schema{ + Type: schema.TypeString, + Required: true, + Description: descriptionInjectedKey, + } + if s.ListMergeStrategy.ListMapKeys.InjectedKey.DefaultValue != "" { + el.Schema[s.ListMergeStrategy.ListMapKeys.InjectedKey.Key].Default = s.ListMergeStrategy.ListMapKeys.InjectedKey.DefaultValue + } + } + return nil } func (g *Builder) buildResource(res *schema.Resource, cfg *config.Resource, tfPath []string, xpPath []string, asBlocksMode bool, names ...string) (*types.Named, *types.Named, *types.Named, error) { //nolint:gocyclo diff --git a/pkg/types/builder_test.go b/pkg/types/builder_test.go index 0322f2e9..2c559f7b 100644 --- a/pkg/types/builder_test.go +++ b/pkg/types/builder_test.go @@ -314,6 +314,7 @@ func TestBuild(t *testing.T) { "Invalid_Sensitive_Fields": { args: args{ cfg: &config.Resource{ + Name: "test_resource", TerraformResource: &schema.Resource{ Schema: map[string]*schema.Schema{ "key_1": { @@ -325,7 +326,7 @@ func TestBuild(t *testing.T) { }, }, want: want{ - err: errors.Wrapf(fmt.Errorf(`got type %q for field %q, only types "string", "*string", []string, []*string, "map[string]string" and "map[string]*string" supported as sensitive`, "*float64", "Key1"), "cannot build the Types"), + err: errors.Wrapf(fmt.Errorf(`got type %q for field %q, only types "string", "*string", []string, []*string, "map[string]string" and "map[string]*string" supported as sensitive`, "*float64", "Key1"), `cannot build the Types for resource "test_resource"`), }, }, "References": { @@ -361,6 +362,7 @@ func TestBuild(t *testing.T) { "Invalid_Schema_Type": { args: args{ cfg: &config.Resource{ + Name: "test_resource", TerraformResource: &schema.Resource{ Schema: map[string]*schema.Schema{ "name": { @@ -372,7 +374,7 @@ func TestBuild(t *testing.T) { }, }, want: want{ - err: errors.Wrapf(errors.Wrapf(errors.Errorf("invalid schema type %s", "TypeInvalid"), "cannot infer type from schema of field %s", "name"), "cannot build the Types"), + err: errors.Wrapf(errors.Wrapf(errors.Errorf("invalid schema type %s", "TypeInvalid"), "cannot infer type from schema of field %s", "name"), `cannot build the Types for resource "test_resource"`), }, }, "Validation_Rules_With_Keywords": { diff --git a/pkg/types/field.go b/pkg/types/field.go index 81ce87fd..89e2df08 100644 --- a/pkg/types/field.go +++ b/pkg/types/field.go @@ -19,10 +19,15 @@ import ( "github.com/crossplane/upjet/pkg" "github.com/crossplane/upjet/pkg/config" "github.com/crossplane/upjet/pkg/types/comments" - "github.com/crossplane/upjet/pkg/types/markers" "github.com/crossplane/upjet/pkg/types/name" ) +const ( + errFmtInvalidSSAConfiguration = "invalid server-side apply merge strategy configuration: Field schema for %q is of type %q and the specified configuration must only set %q" + errFmtUnsupportedSSAField = "cannot configure the server-side apply merge strategy for %q: Configuration can only be specified for lists, sets or maps" + errFmtMissingListMapKeys = "server-side apply merge strategy configuration for %q belongs to a list of type map but list map keys configuration is missing" +) + var parentheses = regexp.MustCompile(`\(([^)]+)\)`) // Field represents a field that is built from the Terraform schema. @@ -40,6 +45,9 @@ type Field struct { TransformedName string SelectorName string Identifier bool + // Injected is set if this Field is an injected field to the Terraform + // schema as an object list map key for server-side apply merges. + Injected bool } // getDocString tries to extract the documentation string for the specified @@ -153,16 +161,18 @@ func NewField(g *Builder, cfg *config.Resource, r *resource, sch *schema.Schema, f.FieldType = fieldType f.InitType = initType - if !sch.Sensitive { - AddServerSideApplyMarkers(f) - } - - return f, nil + AddServerSideApplyMarkers(f) + return f, errors.Wrapf(AddServerSideApplyMarkersFromConfig(f, cfg), "cannot add the server-side apply merge strategy markers for the field") } // AddServerSideApplyMarkers adds server-side apply comment markers to indicate // that scalar maps and sets can be merged granularly, not replace atomically. func AddServerSideApplyMarkers(f *Field) { + // for sensitive fields, we generate secret or secret key references + if f.Schema.Sensitive { + return + } + switch f.Schema.Type { //nolint:exhaustive case schema.TypeMap: // A map should always have an element of type Schema. @@ -170,7 +180,7 @@ func AddServerSideApplyMarkers(f *Field) { switch es.Type { //nolint:exhaustive // We assume scalar types can be granular maps. case schema.TypeString, schema.TypeBool, schema.TypeInt, schema.TypeFloat: - f.Comment.ServerSideApplyOptions.MapType = ptr.To[markers.MapType](markers.MapTypeGranular) + f.Comment.ServerSideApplyOptions.MapType = ptr.To[config.MapType](config.MapTypeGranular) } } case schema.TypeSet: @@ -178,7 +188,7 @@ func AddServerSideApplyMarkers(f *Field) { switch es.Type { //nolint:exhaustive // We assume scalar types can be granular sets. case schema.TypeString, schema.TypeBool, schema.TypeInt, schema.TypeFloat: - f.Comment.ServerSideApplyOptions.ListType = ptr.To[markers.ListType](markers.ListTypeSet) + f.Comment.ServerSideApplyOptions.ListType = ptr.To[config.ListType](config.ListTypeSet) } } } @@ -187,6 +197,63 @@ func AddServerSideApplyMarkers(f *Field) { // objects with a well-known key that we could merge on? } +func setInjectedField(fp, k string, f *Field, s config.MergeStrategy) bool { + if fp != fmt.Sprintf("%s.%s", k, s.ListMergeStrategy.ListMapKeys.InjectedKey.Key) { + return false + } + + if s.ListMergeStrategy.ListMapKeys.InjectedKey.DefaultValue != "" { + f.Comment.KubebuilderOptions.Default = ptr.To[string](s.ListMergeStrategy.ListMapKeys.InjectedKey.DefaultValue) + } + f.TFTag = "-" // prevent serialization into Terraform configuration + f.Injected = true + return true +} + +func AddServerSideApplyMarkersFromConfig(f *Field, cfg *config.Resource) error { //nolint:gocyclo // Easier to follow the logic in a single function + // for sensitive fields, we generate secret or secret key references + if f.Schema.Sensitive { + return nil + } + fp := strings.ReplaceAll(strings.Join(f.TerraformPaths, "."), ".*.", ".") + fp = strings.TrimSuffix(fp, ".*") + for k, s := range cfg.ServerSideApplyMergeStrategies { + if setInjectedField(fp, k, f, s) || k != fp { + continue + } + switch f.Schema.Type { //nolint:exhaustive + case schema.TypeList, schema.TypeSet: + if s.ListMergeStrategy.MergeStrategy == "" || s.MapMergeStrategy != "" || s.StructMergeStrategy != "" { + return errors.Errorf(errFmtInvalidSSAConfiguration, k, "list", "ListMergeStrategy") + } + f.Comment.ServerSideApplyOptions.ListType = ptr.To[config.ListType](s.ListMergeStrategy.MergeStrategy) + if s.ListMergeStrategy.MergeStrategy != config.ListTypeMap { + continue + } + f.Comment.ServerSideApplyOptions.ListMapKey = make([]string, 0, len(s.ListMergeStrategy.ListMapKeys.Keys)+1) + f.Comment.ServerSideApplyOptions.ListMapKey = append(f.Comment.ServerSideApplyOptions.ListMapKey, s.ListMergeStrategy.ListMapKeys.Keys...) + if s.ListMergeStrategy.ListMapKeys.InjectedKey.Key != "" { + f.Comment.ServerSideApplyOptions.ListMapKey = append(f.Comment.ServerSideApplyOptions.ListMapKey, s.ListMergeStrategy.ListMapKeys.InjectedKey.Key) + } + if len(f.Comment.ServerSideApplyOptions.ListMapKey) == 0 { + return errors.Errorf(errFmtMissingListMapKeys, k) + } + case schema.TypeMap: + if s.MapMergeStrategy == "" || s.ListMergeStrategy.MergeStrategy != "" || s.StructMergeStrategy != "" { + return errors.Errorf(errFmtInvalidSSAConfiguration, k, "map", "MapMergeStrategy") + } + f.Comment.ServerSideApplyOptions.MapType = ptr.To[config.MapType](s.MapMergeStrategy) // better to have a copy of the strategy + default: + // currently the generated APIs do not contain embedded objects, embedded + // objects are represented as lists of max size 1. However, this may + // change in the future, i.e., we may decide to generate HCL lists of max + // size 1 as embedded objects. + return errors.Errorf(errFmtUnsupportedSSAField, k) + } + } + return nil +} + // NewSensitiveField returns a constructed sensitive Field object. func NewSensitiveField(g *Builder, cfg *config.Resource, r *resource, sch *schema.Schema, snakeFieldName string, tfPath, xpPath, names []string, asBlocksMode bool) (*Field, bool, error) { //nolint:gocyclo f, err := NewField(g, cfg, r, sch, snakeFieldName, tfPath, xpPath, names, asBlocksMode) @@ -267,9 +334,15 @@ func (f *Field) AddToResource(g *Builder, r *resource, typeNames *TypeNames, add // parameter field if it's not an observation (only) field, i.e. parameter. // // We do this only if tf tag is not set to "-" because otherwise it won't - // be populated from the tfstate. We typically set tf tag to "-" for - // sensitive fields which were replaced with secretKeyRefs. - if f.TFTag != "-" && !addToObservation { + // be populated from the tfstate. Injected fields are included in the + // observation because an associative-list in the spec should also be + // an associative-list in the observation (status). + // We also make sure that this field has not already been added to the + // observation type via an explicit resource configuration. + // We typically set tf tag to "-" for sensitive fields which were replaced + // with secretKeyRefs, or for injected fields into the CRD schema, + // which do not exist in the Terraform schema. + if (f.TFTag != "-" || f.Injected) && !addToObservation { r.addObservationField(f, field) } @@ -313,7 +386,8 @@ func (f *Field) AddToResource(g *Builder, r *resource, typeNames *TypeNames, add // isInit returns true if the field should be added to initProvider. // We don't add Identifiers, references or fields which tag is set to -// "-". +// "-" unless they are injected object list map keys for server-side apply +// merges. // // Identifiers as they should not be ignorable or part of init due // the fact being created for one identifier and then updated for another @@ -327,7 +401,7 @@ func (f *Field) AddToResource(g *Builder, r *resource, typeNames *TypeNames, add // an earlier step, so they cannot be included as well. Plus probably they // should also not change for Create and Update steps. func (f *Field) isInit() bool { - return !f.Identifier && f.Reference == nil && f.TFTag != "-" + return !f.Identifier && f.Reference == nil && (f.TFTag != "-" || f.Injected) } func getDescription(s string) string { diff --git a/pkg/types/markers/kubebuilder.go b/pkg/types/markers/kubebuilder.go index 6b1c6e15..e1b386a2 100644 --- a/pkg/types/markers/kubebuilder.go +++ b/pkg/types/markers/kubebuilder.go @@ -12,6 +12,7 @@ type KubebuilderOptions struct { Required *bool Minimum *int Maximum *int + Default *string } func (o KubebuilderOptions) String() string { @@ -30,6 +31,9 @@ func (o KubebuilderOptions) String() string { if o.Maximum != nil { m += fmt.Sprintf("+kubebuilder:validation:Maximum=%d\n", *o.Maximum) } + if o.Default != nil { + m += fmt.Sprintf("+kubebuilder:default:=%s\n", *o.Default) + } return m } diff --git a/pkg/types/markers/ssa.go b/pkg/types/markers/ssa.go index 6a26c875..bdd0d02a 100644 --- a/pkg/types/markers/ssa.go +++ b/pkg/types/markers/ssa.go @@ -4,64 +4,20 @@ package markers -import "fmt" +import ( + "fmt" -// A ListType is a type of list. -type ListType string - -// Types of lists. -const ( - // ListTypeAtomic means the entire list is replaced during merge. At any - // point in time, a single manager owns the list. - ListTypeAtomic ListType = "atomic" - - // ListTypeSet can be granularly merged, and different managers can own - // different elements in the list. The list can include only scalar - // elements. - ListTypeSet ListType = "set" - - // ListTypeMap can be granularly merged, and different managers can own - // different elements in the list. The list can include only nested types - // (i.e. objects). - ListTypeMap ListType = "map" -) - -// A MapType is a type of map. -type MapType string - -// Types of maps. -const ( - // MapTypeAtomic means that the map can only be entirely replaced by a - // single manager. - MapTypeAtomic MapType = "atomic" - - // MapTypeGranular means that the map supports separate managers updating - // individual fields. - MapTypeGranular MapType = "granular" -) - -// A StructType is a type of struct. -type StructType string - -// Struct types. -const ( - // StructTypeAtomic means that the struct can only be entirely replaced by a - // single manager. - StructTypeAtomic StructType = "atomic" - - // StructTypeGranular means that the struct supports separate managers - // updating individual fields. - StructTypeGranular StructType = "granular" + "github.com/crossplane/upjet/pkg/config" ) // ServerSideApplyOptions represents the server-side apply merge options that // upjet needs to control. // https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy type ServerSideApplyOptions struct { - ListType *ListType + ListType *config.ListType ListMapKey []string - MapType *MapType - StructType *StructType + MapType *config.MapType + StructType *config.StructType } func (o ServerSideApplyOptions) String() string { diff --git a/pkg/types/markers/ssa_test.go b/pkg/types/markers/ssa_test.go index dbfa5513..92c17eb8 100644 --- a/pkg/types/markers/ssa_test.go +++ b/pkg/types/markers/ssa_test.go @@ -9,6 +9,8 @@ import ( "github.com/google/go-cmp/cmp" "k8s.io/utils/ptr" + + "github.com/crossplane/upjet/pkg/config" ) func TestServerSideApplyOptions(t *testing.T) { @@ -18,19 +20,19 @@ func TestServerSideApplyOptions(t *testing.T) { }{ "MapType": { o: ServerSideApplyOptions{ - MapType: ptr.To[MapType](MapTypeAtomic), + MapType: ptr.To[config.MapType](config.MapTypeAtomic), }, want: "+mapType=atomic\n", }, "StructType": { o: ServerSideApplyOptions{ - StructType: ptr.To[StructType](StructTypeAtomic), + StructType: ptr.To[config.StructType](config.StructTypeAtomic), }, want: "+structType=atomic\n", }, "ListType": { o: ServerSideApplyOptions{ - ListType: ptr.To[ListType](ListTypeMap), + ListType: ptr.To[config.ListType](config.ListTypeMap), ListMapKey: []string{"name", "coolness"}, }, want: "+listType=map\n+listMapKey=name\n+listMapKey=coolness\n", From b674f3dd0c9f9dc238b2bbd5518fade662d12077 Mon Sep 17 00:00:00 2001 From: Alper Rifat Ulucinar Date: Thu, 14 Dec 2023 15:07:37 +0300 Subject: [PATCH 11/21] Cache the Terraform instance state returned from schema.Resource.Apply in external-client's Create even if the returned diagnostics contain errors. - In most cases, the Terraform plugin SDK's create implementation for a resource comprises multiple steps (with the creation of the external resource being the very first step). In case, the creation succeeds but any of the subsequent steps fail, then upjet's TF plugin SDK-based external client will not record this state losing the only opportunity to associate the MR with the newly provisioned external resource in some cases. We now put this initial state into the upjet's in-memory state cache so that it's now available for the external- client's next observe call. Signed-off-by: Alper Rifat Ulucinar --- pkg/controller/external_nofork.go | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/pkg/controller/external_nofork.go b/pkg/controller/external_nofork.go index 6197b220..1ae2e28c 100644 --- a/pkg/controller/external_nofork.go +++ b/pkg/controller/external_nofork.go @@ -564,8 +564,36 @@ func (n *noForkExternal) Create(ctx context.Context, mg xpresource.Managed) (man start := time.Now() newState, diag := n.resourceSchema.Apply(ctx, n.opTracker.GetTfState(), n.instanceDiff, n.ts.Meta) metrics.ExternalAPITime.WithLabelValues("create").Observe(time.Since(start).Seconds()) - // diag := n.resourceSchema.CreateWithoutTimeout(ctx, n.resourceData, n.ts.Meta) if diag != nil && diag.HasError() { + // we need to store the Terraform state from the downstream create call if + // one is available even if the diagnostics has reported errors. + // The downstream create call comprises multiple external API calls such as + // the external resource create call, expected state assertion calls + // (external resource state reads) and external resource state refresh + // calls, etc. Any of these steps can fail and if the initial + // external resource create call succeeds, then the TF plugin SDK makes the + // state (together with the TF ID associated with the external resource) + // available reporting any encountered issues in the returned diagnostics. + // If we don't record the returned state from the successful create call, + // then we may hit issues for resources whose Crossplane identifiers cannot + // be computed solely from spec parameters and provider configs, i.e., + // those that contain a random part generated by the CSP. Please see: + // https://github.com/upbound/provider-aws/issues/1010, or + // https://github.com/upbound/provider-aws/issues/1018, which both involve + // MRs with config.IdentifierFromProvider external-name configurations. + // NOTE: The safe (and thus the proper) thing to do in this situation from + // the Crossplane provider's perspective is to set the MR's + // `crossplane.io/external-create-failed` annotation because the provider + // does not know the exact state the external resource is in and a manual + // intervention may be required. But at the time we are introducing this + // fix, we believe associating the external-resource with the MR will just + // provide a better UX although the external resource may not be in the + // expected/desired state yet. We are also planning for improvements on the + // crossplane-runtime's managed reconciler to better support upjet's async + // operations in this regard. + if !n.opTracker.HasState() { // we do not expect a previous state here but just being defensive + n.opTracker.SetTfState(newState) + } return managed.ExternalCreation{}, errors.Errorf("failed to create the resource: %v", diag) } From eb239f423e39b464d425bd9750d9c6238409fd5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sergen=20Yal=C3=A7=C4=B1n?= Date: Thu, 21 Dec 2023 15:44:56 +0300 Subject: [PATCH 12/21] Add reference fields to the InitProvider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Sergen Yalçın --- pkg/types/builder.go | 21 +++++++++++++++------ pkg/types/field.go | 6 +++--- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/pkg/types/builder.go b/pkg/types/builder.go index 3c476fe8..6b9aa4a8 100644 --- a/pkg/types/builder.go +++ b/pkg/types/builder.go @@ -274,7 +274,7 @@ func (g *Builder) buildSchema(f *Field, cfg *config.Resource, names []string, r if paramType.Underlying().String() != emptyStruct { field := types.NewField(token.NoPos, g.Package, f.Name.Camel, types.NewSlice(paramType), false) r.addParameterField(f, field) - r.addInitField(f, field, g.Package) + r.addInitField(f, field, g, nil) } default: if paramType == nil { @@ -400,7 +400,7 @@ func (r *resource) addParameterField(f *Field, field *types.Var) { r.paramFields = append(r.paramFields, field) } -func (r *resource) addInitField(f *Field, field *types.Var, pkg *types.Package) { +func (r *resource) addInitField(f *Field, field *types.Var, g *Builder, typeNames *types.TypeName) { // If the field is not an init field, we don't add it. if !f.isInit() { return @@ -410,10 +410,14 @@ func (r *resource) addInitField(f *Field, field *types.Var, pkg *types.Package) // If the field is a nested type, we need to add it as the init type. if f.InitType != nil { - field = types.NewField(token.NoPos, pkg, f.Name.Camel, f.InitType, false) + field = types.NewField(token.NoPos, g.Package, f.Name.Camel, f.InitType, false) } r.initFields = append(r.initFields, field) + + if f.Reference != nil { + r.addReferenceFields(g, typeNames, f, true) + } } func (r *resource) addObservationField(f *Field, field *types.Var) { @@ -429,10 +433,15 @@ func (r *resource) addObservationField(f *Field, field *types.Var) { r.obsTags = append(r.obsTags, fmt.Sprintf(`json:"%s" tf:"%s"`, f.JSONTag, f.TFTag)) } -func (r *resource) addReferenceFields(g *Builder, paramName *types.TypeName, field *Field) { +func (r *resource) addReferenceFields(g *Builder, paramName *types.TypeName, field *Field, isInit bool) { refFields, refTags := g.generateReferenceFields(paramName, field) - r.paramTags = append(r.paramTags, refTags...) - r.paramFields = append(r.paramFields, refFields...) + if isInit { + r.initTags = append(r.initTags, refTags...) + r.initFields = append(r.initFields, refFields...) + } else { + r.paramTags = append(r.paramTags, refTags...) + r.paramFields = append(r.paramFields, refFields...) + } } // generateTypeName generates a unique name for the type if its original name diff --git a/pkg/types/field.go b/pkg/types/field.go index 89e2df08..3ea21d86 100644 --- a/pkg/types/field.go +++ b/pkg/types/field.go @@ -351,11 +351,11 @@ func (f *Field) AddToResource(g *Builder, r *resource, typeNames *TypeNames, add f.TFTag = strings.TrimSuffix(f.TFTag, ",omitempty") } r.addParameterField(f, field) - r.addInitField(f, field, g.Package) + r.addInitField(f, field, g, typeNames.InitTypeName) } if f.Reference != nil { - r.addReferenceFields(g, typeNames.ParameterTypeName, f) + r.addReferenceFields(g, typeNames.ParameterTypeName, f, false) } // Note(lsviben): All fields are optional because observation fields are @@ -401,7 +401,7 @@ func (f *Field) AddToResource(g *Builder, r *resource, typeNames *TypeNames, add // an earlier step, so they cannot be included as well. Plus probably they // should also not change for Create and Update steps. func (f *Field) isInit() bool { - return !f.Identifier && f.Reference == nil && (f.TFTag != "-" || f.Injected) + return !f.Identifier && f.TFTag != "-" } func getDescription(s string) string { From ac7c6a1953c20e7f548801e8a0409b5bd5bac4b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sergen=20Yal=C3=A7=C4=B1n?= Date: Thu, 21 Dec 2023 18:17:47 +0300 Subject: [PATCH 13/21] Pass full state to GetExternalNameFn function to access field other than ID MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Sergen Yalçın --- pkg/controller/external_nofork.go | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/pkg/controller/external_nofork.go b/pkg/controller/external_nofork.go index 1ae2e28c..b77a4913 100644 --- a/pkg/controller/external_nofork.go +++ b/pkg/controller/external_nofork.go @@ -526,7 +526,7 @@ func (n *noForkExternal) Observe(ctx context.Context, mg xpresource.Managed) (ma resource.SetUpToDateCondition(mg, noDiff) } // check for an external-name change - if nameChanged, err := n.setExternalName(mg, newState); err != nil { + if nameChanged, err := n.setExternalName(mg, stateValueMap); err != nil { return managed.ExternalObservation{}, errors.Wrapf(err, "failed to set the external-name of the managed resource during observe") } else { specUpdateRequired = specUpdateRequired || nameChanged @@ -543,15 +543,14 @@ func (n *noForkExternal) Observe(ctx context.Context, mg xpresource.Managed) (ma // sets the external-name on the MR. Returns `true` // if the external-name of the MR has changed. -func (n *noForkExternal) setExternalName(mg xpresource.Managed, newState *tf.InstanceState) (bool, error) { - if newState.ID == "" { +func (n *noForkExternal) setExternalName(mg xpresource.Managed, stateValueMap map[string]interface{}) (bool, error) { + id, ok := stateValueMap["id"] + if !ok { return false, nil } - newName, err := n.config.ExternalName.GetExternalNameFn(map[string]any{ - "id": newState.ID, - }) + newName, err := n.config.ExternalName.GetExternalNameFn(stateValueMap) if err != nil { - return false, errors.Wrapf(err, "failed to get the external-name from ID: %s", newState.ID) + return false, errors.Wrapf(err, "failed to get the external-name from ID: %s", id) } oldName := meta.GetExternalName(mg) // we have to make sure the newly set external-name is recorded @@ -602,10 +601,10 @@ func (n *noForkExternal) Create(ctx context.Context, mg xpresource.Managed) (man } n.opTracker.SetTfState(newState) - if _, err := n.setExternalName(mg, newState); err != nil { + stateValueMap, err := n.fromInstanceStateToJSONMap(newState) + if _, err := n.setExternalName(mg, stateValueMap); err != nil { return managed.ExternalCreation{}, errors.Wrapf(err, "failed to set the external-name of the managed resource during create") } - stateValueMap, err := n.fromInstanceStateToJSONMap(newState) if err != nil { return managed.ExternalCreation{}, err } From f548c79f1fd061acec2c1e7d2fc94a7acdd5dc40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sergen=20Yal=C3=A7=C4=B1n?= Date: Fri, 22 Dec 2023 14:14:20 +0300 Subject: [PATCH 14/21] - Check if the id is empty - Move the error check to the correct place MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Sergen Yalçın --- pkg/controller/external_nofork.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/controller/external_nofork.go b/pkg/controller/external_nofork.go index b77a4913..52a9413a 100644 --- a/pkg/controller/external_nofork.go +++ b/pkg/controller/external_nofork.go @@ -545,12 +545,12 @@ func (n *noForkExternal) Observe(ctx context.Context, mg xpresource.Managed) (ma // if the external-name of the MR has changed. func (n *noForkExternal) setExternalName(mg xpresource.Managed, stateValueMap map[string]interface{}) (bool, error) { id, ok := stateValueMap["id"] - if !ok { + if !ok || id.(string) == "" { return false, nil } newName, err := n.config.ExternalName.GetExternalNameFn(stateValueMap) if err != nil { - return false, errors.Wrapf(err, "failed to get the external-name from ID: %s", id) + return false, errors.Wrapf(err, "failed to compute the external-name from the state map of the resource with the ID %s", id) } oldName := meta.GetExternalName(mg) // we have to make sure the newly set external-name is recorded @@ -602,12 +602,12 @@ func (n *noForkExternal) Create(ctx context.Context, mg xpresource.Managed) (man n.opTracker.SetTfState(newState) stateValueMap, err := n.fromInstanceStateToJSONMap(newState) + if err != nil { + return managed.ExternalCreation{}, errors.Wrap(err, "failed to convert instance state to map") + } if _, err := n.setExternalName(mg, stateValueMap); err != nil { return managed.ExternalCreation{}, errors.Wrapf(err, "failed to set the external-name of the managed resource during create") } - if err != nil { - return managed.ExternalCreation{}, err - } err = mg.(resource.Terraformed).SetObservation(stateValueMap) if err != nil { return managed.ExternalCreation{}, errors.Errorf("could not set observation: %v", err) From b15649bdb66996136245765c5ea6d9157a1c37e6 Mon Sep 17 00:00:00 2001 From: Alper Rifat Ulucinar Date: Thu, 21 Dec 2023 23:11:17 +0300 Subject: [PATCH 15/21] Alias diag import Signed-off-by: Alper Rifat Ulucinar --- pkg/controller/external_nofork.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/controller/external_nofork.go b/pkg/controller/external_nofork.go index 52a9413a..36c76f4d 100644 --- a/pkg/controller/external_nofork.go +++ b/pkg/controller/external_nofork.go @@ -17,7 +17,7 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/reconciler/managed" xpresource "github.com/crossplane/crossplane-runtime/pkg/resource" "github.com/hashicorp/go-cty/cty" - "github.com/hashicorp/terraform-plugin-sdk/v2/diag" + tfdiag "github.com/hashicorp/terraform-plugin-sdk/v2/diag" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" tf "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" "github.com/pkg/errors" @@ -104,8 +104,8 @@ func getJSONMap(mg xpresource.Managed) (map[string]any, error) { } type Resource interface { - Apply(ctx context.Context, s *tf.InstanceState, d *tf.InstanceDiff, meta interface{}) (*tf.InstanceState, diag.Diagnostics) - RefreshWithoutUpgrade(ctx context.Context, s *tf.InstanceState, meta interface{}) (*tf.InstanceState, diag.Diagnostics) + Apply(ctx context.Context, s *tf.InstanceState, d *tf.InstanceDiff, meta interface{}) (*tf.InstanceState, tfdiag.Diagnostics) + RefreshWithoutUpgrade(ctx context.Context, s *tf.InstanceState, meta interface{}) (*tf.InstanceState, tfdiag.Diagnostics) } type noForkExternal struct { From 74159f85d129838d6fa355fe5fa106de4e704f40 Mon Sep 17 00:00:00 2001 From: Alper Rifat Ulucinar Date: Thu, 21 Dec 2023 23:13:06 +0300 Subject: [PATCH 16/21] Return the cty.Value of InstanceState from noForkExternal.fromInstanceStateToJSONMap Signed-off-by: Alper Rifat Ulucinar --- pkg/controller/external_nofork.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/controller/external_nofork.go b/pkg/controller/external_nofork.go index 36c76f4d..3b98cdee 100644 --- a/pkg/controller/external_nofork.go +++ b/pkg/controller/external_nofork.go @@ -496,7 +496,7 @@ func (n *noForkExternal) Observe(ctx context.Context, mg xpresource.Managed) (ma addTTR(mg) } mg.SetConditions(xpv1.Available()) - stateValueMap, err := n.fromInstanceStateToJSONMap(newState) + stateValueMap, _, err := n.fromInstanceStateToJSONMap(newState) if err != nil { return managed.ExternalObservation{}, errors.Wrap(err, "cannot convert instance state to JSON map") } @@ -601,12 +601,12 @@ func (n *noForkExternal) Create(ctx context.Context, mg xpresource.Managed) (man } n.opTracker.SetTfState(newState) - stateValueMap, err := n.fromInstanceStateToJSONMap(newState) + stateValueMap, _, err := n.fromInstanceStateToJSONMap(newState) if err != nil { return managed.ExternalCreation{}, errors.Wrap(err, "failed to convert instance state to map") } if _, err := n.setExternalName(mg, stateValueMap); err != nil { - return managed.ExternalCreation{}, errors.Wrapf(err, "failed to set the external-name of the managed resource during create") + return managed.ExternalCreation{}, errors.Wrap(err, "failed to set the external-name of the managed resource during create") } err = mg.(resource.Terraformed).SetObservation(stateValueMap) if err != nil { @@ -655,7 +655,7 @@ func (n *noForkExternal) Update(ctx context.Context, mg xpresource.Managed) (man } n.opTracker.SetTfState(newState) - stateValueMap, err := n.fromInstanceStateToJSONMap(newState) + stateValueMap, _, err := n.fromInstanceStateToJSONMap(newState) if err != nil { return managed.ExternalUpdate{}, err } @@ -686,15 +686,15 @@ func (n *noForkExternal) Delete(ctx context.Context, _ xpresource.Managed) error return nil } -func (n *noForkExternal) fromInstanceStateToJSONMap(newState *tf.InstanceState) (map[string]interface{}, error) { +func (n *noForkExternal) fromInstanceStateToJSONMap(newState *tf.InstanceState) (map[string]interface{}, cty.Value, error) { impliedType := n.config.TerraformResource.CoreConfigSchema().ImpliedType() attrsAsCtyValue, err := newState.AttrsAsObjectValue(impliedType) if err != nil { - return nil, errors.Wrap(err, "could not convert attrs to cty value") + return nil, cty.NilVal, errors.Wrap(err, "could not convert attrs to cty value") } stateValueMap, err := schema.StateValueToJSONMap(attrsAsCtyValue, impliedType) if err != nil { - return nil, errors.Wrap(err, "could not convert instance state value to JSON") + return nil, cty.NilVal, errors.Wrap(err, "could not convert instance state value to JSON") } - return stateValueMap, nil + return stateValueMap, attrsAsCtyValue, nil } From f0de67a3ce046a61a1fe0a030b5657bca9485cc5 Mon Sep 17 00:00:00 2001 From: Alper Rifat Ulucinar Date: Fri, 22 Dec 2023 10:49:59 +0300 Subject: [PATCH 17/21] Set the RawPlan for a new terraform.InstanceState returned from an observation Signed-off-by: Alper Rifat Ulucinar --- pkg/controller/external_nofork.go | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/pkg/controller/external_nofork.go b/pkg/controller/external_nofork.go index 3b98cdee..4bb73c60 100644 --- a/pkg/controller/external_nofork.go +++ b/pkg/controller/external_nofork.go @@ -474,10 +474,21 @@ func (n *noForkExternal) Observe(ctx context.Context, mg xpresource.Managed) (ma if diag != nil && diag.HasError() { return managed.ExternalObservation{}, errors.Errorf("failed to observe the resource: %v", diag) } + diffState := n.opTracker.GetTfState() n.opTracker.SetTfState(newState) // TODO: missing RawConfig & RawPlan here... - resourceExists := newState != nil && newState.ID != "" - instanceDiff, err := n.getResourceDataDiff(mg.(resource.Terraformed), ctx, newState, resourceExists) + + var stateValueMap map[string]any + if resourceExists { + jsonMap, stateValue, err := n.fromInstanceStateToJSONMap(newState) + if err != nil { + return managed.ExternalObservation{}, errors.Wrap(err, "cannot convert instance state to JSON map") + } + stateValueMap = jsonMap + newState.RawPlan = stateValue + diffState = newState + } + instanceDiff, err := n.getResourceDataDiff(mg.(resource.Terraformed), ctx, diffState, resourceExists) if err != nil { return managed.ExternalObservation{}, errors.Wrap(err, "cannot compute the instance diff") } @@ -496,10 +507,6 @@ func (n *noForkExternal) Observe(ctx context.Context, mg xpresource.Managed) (ma addTTR(mg) } mg.SetConditions(xpv1.Available()) - stateValueMap, _, err := n.fromInstanceStateToJSONMap(newState) - if err != nil { - return managed.ExternalObservation{}, errors.Wrap(err, "cannot convert instance state to JSON map") - } buff, err := json.TFParser.Marshal(stateValueMap) if err != nil { From ccb06111c8d1bdaf0c24ed5a52a077e039bac8b0 Mon Sep 17 00:00:00 2001 From: Alper Rifat Ulucinar Date: Fri, 22 Dec 2023 23:47:19 +0300 Subject: [PATCH 18/21] Set diff state's Attributes to nil if the resource does not exist Signed-off-by: Alper Rifat Ulucinar --- pkg/controller/external_nofork.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/controller/external_nofork.go b/pkg/controller/external_nofork.go index 4bb73c60..0e7496dd 100644 --- a/pkg/controller/external_nofork.go +++ b/pkg/controller/external_nofork.go @@ -487,6 +487,8 @@ func (n *noForkExternal) Observe(ctx context.Context, mg xpresource.Managed) (ma stateValueMap = jsonMap newState.RawPlan = stateValue diffState = newState + } else if diffState != nil { + diffState.Attributes = nil } instanceDiff, err := n.getResourceDataDiff(mg.(resource.Terraformed), ctx, diffState, resourceExists) if err != nil { From 6a56e732b189a088444e17f1d5860c0d704ceb97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sergen=20Yal=C3=A7=C4=B1n?= Date: Tue, 26 Dec 2023 11:38:03 +0300 Subject: [PATCH 19/21] Fix nonintentional if case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Sergen Yalçın --- pkg/types/field.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/types/field.go b/pkg/types/field.go index 3ea21d86..2e6fc2dc 100644 --- a/pkg/types/field.go +++ b/pkg/types/field.go @@ -401,7 +401,7 @@ func (f *Field) AddToResource(g *Builder, r *resource, typeNames *TypeNames, add // an earlier step, so they cannot be included as well. Plus probably they // should also not change for Create and Update steps. func (f *Field) isInit() bool { - return !f.Identifier && f.TFTag != "-" + return !f.Identifier && (f.TFTag != "-" || f.Injected) } func getDescription(s string) string { From f33d01b36d90174d0f6ff3f1e2163f90fe6e401a Mon Sep 17 00:00:00 2001 From: Matt Bush Date: Tue, 23 Jan 2024 09:55:41 -0800 Subject: [PATCH 20/21] Revert "Merge pull request #318 from ulucinar/fix-diff-state" This reverts commit 81e262067d2b9e7099ca6a07b15e8d2ca1a08b5e, reversing changes made to 1d547af3ddb6560f7675f793b5f4caa8061295b9. Signed-off-by: Matt Bush --- pkg/controller/external_nofork.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/controller/external_nofork.go b/pkg/controller/external_nofork.go index 0e7496dd..4bb73c60 100644 --- a/pkg/controller/external_nofork.go +++ b/pkg/controller/external_nofork.go @@ -487,8 +487,6 @@ func (n *noForkExternal) Observe(ctx context.Context, mg xpresource.Managed) (ma stateValueMap = jsonMap newState.RawPlan = stateValue diffState = newState - } else if diffState != nil { - diffState.Attributes = nil } instanceDiff, err := n.getResourceDataDiff(mg.(resource.Terraformed), ctx, diffState, resourceExists) if err != nil { From cd53d7a8fbb7e43a5e365cf8892f08aa68697ebd Mon Sep 17 00:00:00 2001 From: Matt Bush Date: Tue, 23 Jan 2024 13:39:39 -0800 Subject: [PATCH 21/21] Revert "Merge pull request #317 from ulucinar/fix-customize-diff" This reverts commit 1d547af3ddb6560f7675f793b5f4caa8061295b9, reversing changes made to 0bc8185b6fa1286204ec3723cdfa75f5978df6f8. Signed-off-by: Matt Bush --- pkg/controller/external_nofork.go | 39 +++++++++++++------------------ 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/pkg/controller/external_nofork.go b/pkg/controller/external_nofork.go index 4bb73c60..52a9413a 100644 --- a/pkg/controller/external_nofork.go +++ b/pkg/controller/external_nofork.go @@ -17,7 +17,7 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/reconciler/managed" xpresource "github.com/crossplane/crossplane-runtime/pkg/resource" "github.com/hashicorp/go-cty/cty" - tfdiag "github.com/hashicorp/terraform-plugin-sdk/v2/diag" + "github.com/hashicorp/terraform-plugin-sdk/v2/diag" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" tf "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" "github.com/pkg/errors" @@ -104,8 +104,8 @@ func getJSONMap(mg xpresource.Managed) (map[string]any, error) { } type Resource interface { - Apply(ctx context.Context, s *tf.InstanceState, d *tf.InstanceDiff, meta interface{}) (*tf.InstanceState, tfdiag.Diagnostics) - RefreshWithoutUpgrade(ctx context.Context, s *tf.InstanceState, meta interface{}) (*tf.InstanceState, tfdiag.Diagnostics) + Apply(ctx context.Context, s *tf.InstanceState, d *tf.InstanceDiff, meta interface{}) (*tf.InstanceState, diag.Diagnostics) + RefreshWithoutUpgrade(ctx context.Context, s *tf.InstanceState, meta interface{}) (*tf.InstanceState, diag.Diagnostics) } type noForkExternal struct { @@ -474,21 +474,10 @@ func (n *noForkExternal) Observe(ctx context.Context, mg xpresource.Managed) (ma if diag != nil && diag.HasError() { return managed.ExternalObservation{}, errors.Errorf("failed to observe the resource: %v", diag) } - diffState := n.opTracker.GetTfState() n.opTracker.SetTfState(newState) // TODO: missing RawConfig & RawPlan here... - resourceExists := newState != nil && newState.ID != "" - var stateValueMap map[string]any - if resourceExists { - jsonMap, stateValue, err := n.fromInstanceStateToJSONMap(newState) - if err != nil { - return managed.ExternalObservation{}, errors.Wrap(err, "cannot convert instance state to JSON map") - } - stateValueMap = jsonMap - newState.RawPlan = stateValue - diffState = newState - } - instanceDiff, err := n.getResourceDataDiff(mg.(resource.Terraformed), ctx, diffState, resourceExists) + resourceExists := newState != nil && newState.ID != "" + instanceDiff, err := n.getResourceDataDiff(mg.(resource.Terraformed), ctx, newState, resourceExists) if err != nil { return managed.ExternalObservation{}, errors.Wrap(err, "cannot compute the instance diff") } @@ -507,6 +496,10 @@ func (n *noForkExternal) Observe(ctx context.Context, mg xpresource.Managed) (ma addTTR(mg) } mg.SetConditions(xpv1.Available()) + stateValueMap, err := n.fromInstanceStateToJSONMap(newState) + if err != nil { + return managed.ExternalObservation{}, errors.Wrap(err, "cannot convert instance state to JSON map") + } buff, err := json.TFParser.Marshal(stateValueMap) if err != nil { @@ -608,12 +601,12 @@ func (n *noForkExternal) Create(ctx context.Context, mg xpresource.Managed) (man } n.opTracker.SetTfState(newState) - stateValueMap, _, err := n.fromInstanceStateToJSONMap(newState) + stateValueMap, err := n.fromInstanceStateToJSONMap(newState) if err != nil { return managed.ExternalCreation{}, errors.Wrap(err, "failed to convert instance state to map") } if _, err := n.setExternalName(mg, stateValueMap); err != nil { - return managed.ExternalCreation{}, errors.Wrap(err, "failed to set the external-name of the managed resource during create") + return managed.ExternalCreation{}, errors.Wrapf(err, "failed to set the external-name of the managed resource during create") } err = mg.(resource.Terraformed).SetObservation(stateValueMap) if err != nil { @@ -662,7 +655,7 @@ func (n *noForkExternal) Update(ctx context.Context, mg xpresource.Managed) (man } n.opTracker.SetTfState(newState) - stateValueMap, _, err := n.fromInstanceStateToJSONMap(newState) + stateValueMap, err := n.fromInstanceStateToJSONMap(newState) if err != nil { return managed.ExternalUpdate{}, err } @@ -693,15 +686,15 @@ func (n *noForkExternal) Delete(ctx context.Context, _ xpresource.Managed) error return nil } -func (n *noForkExternal) fromInstanceStateToJSONMap(newState *tf.InstanceState) (map[string]interface{}, cty.Value, error) { +func (n *noForkExternal) fromInstanceStateToJSONMap(newState *tf.InstanceState) (map[string]interface{}, error) { impliedType := n.config.TerraformResource.CoreConfigSchema().ImpliedType() attrsAsCtyValue, err := newState.AttrsAsObjectValue(impliedType) if err != nil { - return nil, cty.NilVal, errors.Wrap(err, "could not convert attrs to cty value") + return nil, errors.Wrap(err, "could not convert attrs to cty value") } stateValueMap, err := schema.StateValueToJSONMap(attrsAsCtyValue, impliedType) if err != nil { - return nil, cty.NilVal, errors.Wrap(err, "could not convert instance state value to JSON") + return nil, errors.Wrap(err, "could not convert instance state value to JSON") } - return stateValueMap, attrsAsCtyValue, nil + return stateValueMap, nil }