From 1dabdf0dcd5ab48bf706177da8ffeda89e323dc2 Mon Sep 17 00:00:00 2001 From: Katy Moe Date: Sun, 14 Nov 2021 21:51:53 +0000 Subject: [PATCH 1/6] normalise test names --- internal/configs/moved_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/configs/moved_test.go b/internal/configs/moved_test.go index 4e2666a27d..433525d28c 100644 --- a/internal/configs/moved_test.go +++ b/internal/configs/moved_test.go @@ -9,7 +9,7 @@ import ( "github.com/hashicorp/terraform/internal/addrs" ) -func TestDecodeMovedBlock(t *testing.T) { +func TestMovedBlock_decode(t *testing.T) { blockRange := hcl.Range{ Filename: "mock.tf", Start: hcl.Pos{Line: 3, Column: 12, Byte: 27}, @@ -169,7 +169,7 @@ func TestDecodeMovedBlock(t *testing.T) { } } -func TestMovedBlocksInModule(t *testing.T) { +func TestMovedBlock_inModule(t *testing.T) { parser := NewParser(nil) mod, diags := parser.LoadConfigDir("testdata/valid-modules/moved-blocks") if diags.HasErrors() { From 4305271cff59ce6abb3bd7734acf5a223d2cf3dc Mon Sep 17 00:00:00 2001 From: Katy Moe Date: Mon, 15 Nov 2021 10:46:06 +0000 Subject: [PATCH 2/6] remove occurrences of AbsMovable --- internal/addrs/move_endpoint.go | 10 +++++----- internal/addrs/move_endpoint_module.go | 2 +- internal/addrs/move_endpoint_test.go | 2 +- internal/addrs/moveable.go | 4 ++-- internal/refactoring/move_validate.go | 6 +++--- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/internal/addrs/move_endpoint.go b/internal/addrs/move_endpoint.go index 2b44c5cd1d..765b66f5ee 100644 --- a/internal/addrs/move_endpoint.go +++ b/internal/addrs/move_endpoint.go @@ -19,7 +19,7 @@ import ( // prompt creating a different plan than Terraform would by default. // // To obtain a full address from a MoveEndpoint you must use -// either the package function UnifyMoveEndpoints (to get an AbsMovable) or +// either the package function UnifyMoveEndpoints (to get an AbsMoveable) or // the method ConfigMoveable (to get a ConfigMoveable). type MoveEndpoint struct { // SourceRange is the location of the physical endpoint address @@ -27,15 +27,15 @@ type MoveEndpoint struct { // configuration expresson. SourceRange tfdiags.SourceRange - // Internally we (ab)use AbsMovable as the representation of our + // Internally we (ab)use AbsMoveable as the representation of our // relative address, even though everywhere else in Terraform - // AbsMovable always represents a fully-absolute address. + // AbsMoveable always represents a fully-absolute address. // In practice, due to the implementation of ParseMoveEndpoint, // this is always either a ModuleInstance or an AbsResourceInstance, // and we only consider the possibility of interpreting it as // a AbsModuleCall or an AbsResource in UnifyMoveEndpoints. // This is intentionally unexported to encapsulate this unusual - // meaning of AbsMovable. + // meaning of AbsMoveable. relSubject AbsMoveable } @@ -44,7 +44,7 @@ func (e *MoveEndpoint) ObjectKind() MoveEndpointKind { } func (e *MoveEndpoint) String() string { - // Our internal pseudo-AbsMovable representing the relative + // Our internal pseudo-AbsMoveable representing the relative // address (either ModuleInstance or AbsResourceInstance) is // a good enough proxy for the relative move endpoint address // serialization. diff --git a/internal/addrs/move_endpoint_module.go b/internal/addrs/move_endpoint_module.go index 8bbbe83e7d..e2180f25a1 100644 --- a/internal/addrs/move_endpoint_module.go +++ b/internal/addrs/move_endpoint_module.go @@ -131,7 +131,7 @@ func (e *MoveEndpointInModule) Module() Module { return e.module } -// InModuleInstance returns an AbsMovable address which concatenates the +// InModuleInstance returns an AbsMoveable address which concatenates the // given module instance address with the receiver's relative object selection // to produce one example of an instance that might be affected by this // move statement. diff --git a/internal/addrs/move_endpoint_test.go b/internal/addrs/move_endpoint_test.go index 6e117139b7..602c7e9719 100644 --- a/internal/addrs/move_endpoint_test.go +++ b/internal/addrs/move_endpoint_test.go @@ -12,7 +12,7 @@ import ( func TestParseMoveEndpoint(t *testing.T) { tests := []struct { Input string - WantRel AbsMoveable // funny intermediate subset of AbsMovable + WantRel AbsMoveable // funny intermediate subset of AbsMoveable WantErr string }{ { diff --git a/internal/addrs/moveable.go b/internal/addrs/moveable.go index a85fe72c1c..dcbd8c03e7 100644 --- a/internal/addrs/moveable.go +++ b/internal/addrs/moveable.go @@ -5,7 +5,7 @@ package addrs // with any other similar cross-module state refactoring statements we might // allow. // -// Note that AbsMovable represents an absolute address relative to the root +// Note that AbsMoveable represents an absolute address relative to the root // of the configuration, which is different than the direct representation // of these in configuration where the author gives an address relative to // the current module where the address is defined. The type MoveEndpoint @@ -16,7 +16,7 @@ type AbsMoveable interface { String() string } -// The following are all of the possible AbsMovable address types: +// The following are all of the possible AbsMoveable address types: var ( _ AbsMoveable = AbsResource{} _ AbsMoveable = AbsResourceInstance{} diff --git a/internal/refactoring/move_validate.go b/internal/refactoring/move_validate.go index aad356cb2b..2c38ba756d 100644 --- a/internal/refactoring/move_validate.go +++ b/internal/refactoring/move_validate.go @@ -110,7 +110,7 @@ func ValidateMoves(stmts []MoveStatement, rootCfg *configs.Config, declaredInsts shortNoun = "resource" default: // The above cases should cover all of the AbsMoveable types - panic("unsupported AbsMovable address type") + panic("unsupported AbsMoveable address type") } // It's invalid to have a move statement whose "from" address @@ -234,7 +234,7 @@ func moveableObjectExists(addr addrs.AbsMoveable, in instances.Set) bool { return in.HasResource(addr) default: // The above cases should cover all of the AbsMoveable types - panic("unsupported AbsMovable address type") + panic("unsupported AbsMoveable address type") } } @@ -309,7 +309,7 @@ func movableObjectDeclRange(addr addrs.AbsMoveable, cfg *configs.Config) (tfdiag return tfdiags.SourceRangeFromHCL(rc.DeclRange), true default: // The above cases should cover all of the AbsMoveable types - panic("unsupported AbsMovable address type") + panic("unsupported AbsMoveable address type") } } From dc3ed6271c4111021b92ca9be39a9d6598d3c8a0 Mon Sep 17 00:00:00 2001 From: Katy Moe Date: Mon, 15 Nov 2021 10:42:08 +0000 Subject: [PATCH 3/6] add failing test for resource type mismatch --- internal/refactoring/move_validate_test.go | 18 ++++++++++++++++++ .../move-validate-zoo/move-validate-root.tf | 3 +++ 2 files changed, 21 insertions(+) diff --git a/internal/refactoring/move_validate_test.go b/internal/refactoring/move_validate_test.go index 986c1d5db3..53bbbe6c2e 100644 --- a/internal/refactoring/move_validate_test.go +++ b/internal/refactoring/move_validate_test.go @@ -386,6 +386,24 @@ Each resource can have moved from only one source resource.`, }, WantError: ``, // This is okay because the call itself is not considered to be inside the package it refers to }, + "resource type mismatch": { + Statements: []MoveStatement{ + makeTestMoveStmt(t, ``, + `test.nonexist1`, + `other.single`, + ), + }, + WantError: `Resource type mismatch: This statement declares a move from test.nonexist1 to other.single, which is a resource of a different type.`, + }, + "resource instance type mismatch": { + Statements: []MoveStatement{ + makeTestMoveStmt(t, ``, + `test.nonexist1[0]`, + `other.single`, + ), + }, + WantError: `Resource type mismatch: This statement declares a move from test.nonexist1[0] to other.single, which is a resource instance of a different type.`, + }, } for name, test := range tests { diff --git a/internal/refactoring/testdata/move-validate-zoo/move-validate-root.tf b/internal/refactoring/testdata/move-validate-zoo/move-validate-root.tf index 492d671dbc..3cc8504f9e 100644 --- a/internal/refactoring/testdata/move-validate-zoo/move-validate-root.tf +++ b/internal/refactoring/testdata/move-validate-zoo/move-validate-root.tf @@ -41,6 +41,9 @@ resource "test" "for_each" { } } +resource "other" "single" { +} + module "fake_external" { # Our configuration fixture loader has a special case for a module call # named "fake_external" where it will mutate the source address after From 7162e79fdf8fc24be62371eb1d800d24e837cf19 Mon Sep 17 00:00:00 2001 From: Katy Moe Date: Mon, 15 Nov 2021 10:43:06 +0000 Subject: [PATCH 4/6] return error on resource type mismatch A resource move is invalid if the two resource( instance)s are of different resource types. Check for this during move validation. --- internal/refactoring/move_validate.go | 36 +++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/internal/refactoring/move_validate.go b/internal/refactoring/move_validate.go index 2c38ba756d..b59bc315ef 100644 --- a/internal/refactoring/move_validate.go +++ b/internal/refactoring/move_validate.go @@ -178,6 +178,16 @@ func ValidateMoves(stmts []MoveStatement, rootCfg *configs.Config, declaredInsts } } + // Resource types must match. + if resourceTypesDiffer(absFrom, absTo) { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Resource type mismatch", + Detail: fmt.Sprintf( + "This statement declares a move from %s to %s, which is a %s of a different type.", absFrom, absTo, noun, + ), + }) + } } } @@ -238,6 +248,32 @@ func moveableObjectExists(addr addrs.AbsMoveable, in instances.Set) bool { } } +func resourceTypesDiffer(absFrom, absTo addrs.AbsMoveable) bool { + switch absFrom.(type) { + case addrs.AbsResourceInstance, addrs.AbsResource: + // addrs.UnifyMoveEndpoints guarantees that both addresses are of the + // same kind, so at this point we can assume that absTo is an + // addrs.AbsResourceInstance or addrs.AbsResource. + return absMoveableResourceType(absFrom) != absMoveableResourceType(absTo) + default: + return false + } +} + +// absMoveableResourceType returns the type of the supplied +// addrs.AbsResourceInstance or addrs.AbsResource, or "" for other AbsMoveable +// types. +func absMoveableResourceType(addr addrs.AbsMoveable) string { + switch addr := addr.(type) { + case addrs.AbsResourceInstance: + return addr.Resource.Resource.Type + case addrs.AbsResource: + return addr.Resource.Type + default: + return "" + } +} + func movableObjectDeclRange(addr addrs.AbsMoveable, cfg *configs.Config) (tfdiags.SourceRange, bool) { switch addr := addr.(type) { case addrs.ModuleInstance: From 11566b1d11eb6a76cc3d5fe664dd197fe021bbdf Mon Sep 17 00:00:00 2001 From: Katy Moe Date: Mon, 15 Nov 2021 11:03:33 +0000 Subject: [PATCH 5/6] introduce AbsMoveableResource AbsMoveableResource is an AbsMoveable that is either a resource or a resource instance. This interface represents a moveable that has a resource type. --- internal/addrs/moveable.go | 13 +++++++++++++ internal/addrs/resource.go | 8 ++++++++ internal/refactoring/move_validate.go | 20 ++++---------------- 3 files changed, 25 insertions(+), 16 deletions(-) diff --git a/internal/addrs/moveable.go b/internal/addrs/moveable.go index dcbd8c03e7..083fcfb27f 100644 --- a/internal/addrs/moveable.go +++ b/internal/addrs/moveable.go @@ -24,6 +24,19 @@ var ( _ AbsMoveable = AbsModuleCall{} ) +// AbsMoveableResource is an AbsMoveable that is either a resource or a resource +// instance. +type AbsMoveableResource interface { + AbsMoveable + Type() string +} + +// The following are all of the possible AbsMoveableResource types: +var ( + _ AbsMoveableResource = AbsResource{} + _ AbsMoveableResource = AbsResourceInstance{} +) + // ConfigMoveable is similar to AbsMoveable but represents a static object in // the configuration, rather than an instance of that object created by // module expansion. diff --git a/internal/addrs/resource.go b/internal/addrs/resource.go index 0ebf89d999..9c81c0eddd 100644 --- a/internal/addrs/resource.go +++ b/internal/addrs/resource.go @@ -186,6 +186,10 @@ func (r AbsResource) String() string { return fmt.Sprintf("%s.%s", r.Module.String(), r.Resource.String()) } +func (r AbsResource) Type() string { + return r.Resource.Type +} + func (r AbsResource) Equal(o AbsResource) bool { return r.Module.Equal(o.Module) && r.Resource.Equal(o.Resource) } @@ -267,6 +271,10 @@ func (r AbsResourceInstance) String() string { return fmt.Sprintf("%s.%s", r.Module.String(), r.Resource.String()) } +func (r AbsResourceInstance) Type() string { + return r.Resource.Resource.Type +} + func (r AbsResourceInstance) Equal(o AbsResourceInstance) bool { return r.Module.Equal(o.Module) && r.Resource.Equal(o.Resource) } diff --git a/internal/refactoring/move_validate.go b/internal/refactoring/move_validate.go index b59bc315ef..b393c0a0b0 100644 --- a/internal/refactoring/move_validate.go +++ b/internal/refactoring/move_validate.go @@ -251,29 +251,17 @@ func moveableObjectExists(addr addrs.AbsMoveable, in instances.Set) bool { func resourceTypesDiffer(absFrom, absTo addrs.AbsMoveable) bool { switch absFrom.(type) { case addrs.AbsResourceInstance, addrs.AbsResource: + absFrom := absFrom.(addrs.AbsMoveableResource) // addrs.UnifyMoveEndpoints guarantees that both addresses are of the - // same kind, so at this point we can assume that absTo is an + // same kind, so at this point we can assume that absTo is also an // addrs.AbsResourceInstance or addrs.AbsResource. - return absMoveableResourceType(absFrom) != absMoveableResourceType(absTo) + absTo := absTo.(addrs.AbsMoveableResource) + return absFrom.Type() != absTo.Type() default: return false } } -// absMoveableResourceType returns the type of the supplied -// addrs.AbsResourceInstance or addrs.AbsResource, or "" for other AbsMoveable -// types. -func absMoveableResourceType(addr addrs.AbsMoveable) string { - switch addr := addr.(type) { - case addrs.AbsResourceInstance: - return addr.Resource.Resource.Type - case addrs.AbsResource: - return addr.Resource.Type - default: - return "" - } -} - func movableObjectDeclRange(addr addrs.AbsMoveable, cfg *configs.Config) (tfdiags.SourceRange, bool) { switch addr := addr.(type) { case addrs.ModuleInstance: From aeecc6a627030ac2fdecf12b6cf59dfccb44ff5b Mon Sep 17 00:00:00 2001 From: Katy Moe Date: Tue, 16 Nov 2021 18:19:11 +0000 Subject: [PATCH 6/6] add AffectedAbsResource to interface --- internal/addrs/moveable.go | 2 +- internal/addrs/resource.go | 13 +++++++++---- internal/refactoring/move_validate.go | 7 +++---- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/internal/addrs/moveable.go b/internal/addrs/moveable.go index 083fcfb27f..b9d2f87f07 100644 --- a/internal/addrs/moveable.go +++ b/internal/addrs/moveable.go @@ -28,7 +28,7 @@ var ( // instance. type AbsMoveableResource interface { AbsMoveable - Type() string + AffectedAbsResource() AbsResource } // The following are all of the possible AbsMoveableResource types: diff --git a/internal/addrs/resource.go b/internal/addrs/resource.go index 9c81c0eddd..d8596237ff 100644 --- a/internal/addrs/resource.go +++ b/internal/addrs/resource.go @@ -186,8 +186,9 @@ func (r AbsResource) String() string { return fmt.Sprintf("%s.%s", r.Module.String(), r.Resource.String()) } -func (r AbsResource) Type() string { - return r.Resource.Type +// AffectedAbsResource returns the AbsResource. +func (r AbsResource) AffectedAbsResource() AbsResource { + return r } func (r AbsResource) Equal(o AbsResource) bool { @@ -271,8 +272,12 @@ func (r AbsResourceInstance) String() string { return fmt.Sprintf("%s.%s", r.Module.String(), r.Resource.String()) } -func (r AbsResourceInstance) Type() string { - return r.Resource.Resource.Type +// AffectedAbsResource returns the AbsResource for the instance. +func (r AbsResourceInstance) AffectedAbsResource() AbsResource { + return AbsResource{ + Module: r.Module, + Resource: r.Resource.Resource, + } } func (r AbsResourceInstance) Equal(o AbsResourceInstance) bool { diff --git a/internal/refactoring/move_validate.go b/internal/refactoring/move_validate.go index b393c0a0b0..1336986083 100644 --- a/internal/refactoring/move_validate.go +++ b/internal/refactoring/move_validate.go @@ -249,14 +249,13 @@ func moveableObjectExists(addr addrs.AbsMoveable, in instances.Set) bool { } func resourceTypesDiffer(absFrom, absTo addrs.AbsMoveable) bool { - switch absFrom.(type) { - case addrs.AbsResourceInstance, addrs.AbsResource: - absFrom := absFrom.(addrs.AbsMoveableResource) + switch absFrom := absFrom.(type) { + case addrs.AbsMoveableResource: // addrs.UnifyMoveEndpoints guarantees that both addresses are of the // same kind, so at this point we can assume that absTo is also an // addrs.AbsResourceInstance or addrs.AbsResource. absTo := absTo.(addrs.AbsMoveableResource) - return absFrom.Type() != absTo.Type() + return absFrom.AffectedAbsResource().Resource.Type != absTo.AffectedAbsResource().Resource.Type default: return false }