Migrate from SDKv2 => Framework #249

bendbennett · 2022-05-03T14:47:43Z

Objectives

Migrate provider from SDKv2 to Framework to align with RFC TF-483 Utility Provider Upgrade.
~~Use muxing so the changes will be compatible with tf5 protocol and TF >= 0.12.~~
Replicate the current behaviour of the provider (i.e., ~~v3.1.3~~ v3.2.0).

Items Requiring Resolving

~~Currently, there is a failing test which is the result of a rounding issue that is currently under investigation. This should be fixed by terraform-plugin-go/pull/190.~~ This has been fixed in terraform-plugin-go and v0.9.1 is now being used.

Breaking Changes

As described in the CHANGELOG:

NOTES:

* Provider has been re-written using the new [`terraform-plugin-framework`](https://www.terraform.io/plugin/framework) ([#177](https://github.com/hashicorp/terraform-provider-random/pull/177)).

BREAKING CHANGES:

* [Terraform `>=1.0`](https://www.terraform.io/language/upgrade-guides/1-0) is now required to use this provider.

* resource/random_password: Deprecated attribute `number` has been removed ([266](https://github.com/hashicorp/terraform-provider-random/issues/266)).
* resource/random_string: Deprecated attribute `number` has been removed ([266](https://github.com/hashicorp/terraform-provider-random/issues/266)).

Closes #177

internal/provider_fm/resource_id.go

detro

Left a few comments and nitpicks. In general this is looking really good.

Aside from those minor things, I do have a question (I might have asked this before already, so I apologise if I did): what's the plan in terms of Protocol 5 -> 6 support?

As part of the "Utility Providers RFC" we established we would be moving to 6 also as a way to "nudge" the ecosystem.

Is the intention here to do one last Protocol 5 release and then upgrade to 6, by removing mux? Or do you have a different plan?

.github/workflows/test.yml

internal/provider/provider.go

internal/provider/resource_id.go

detro · 2022-05-09T10:37:04Z

internal/provider/string.go

+func validatorMinInt(min int64) tfsdk.AttributeValidator {
+	return minIntValidator{min}
+}
+
+type minIntValidator struct {
+	val int64
+}
+
+func (m minIntValidator) Description(context.Context) string {
+	return "MinInt validator ensures that attribute is at least val"
+}
+
+func (m minIntValidator) MarkdownDescription(context.Context) string {
+	return "MinInt validator ensures that attribute is at least `val`"
+}
+
+func (m minIntValidator) Validate(ctx context.Context, req tfsdk.ValidateAttributeRequest, resp *tfsdk.ValidateAttributeResponse) {
+	t := req.AttributeConfig.(types.Int64)
+
+	if t.Value < m.val {
+		resp.Diagnostics.AddError(
+			fmt.Sprintf("expected attribute to be at least %d, got %d", m.val, t.Value),
+			fmt.Sprintf("expected attribute to be at least %d, got %d", m.val, t.Value),
 		)
+	}
+}


Nitpick: Would it make sense to have the Validators and PlanModifiers into their own files?
Like a validators.go and a plan_modifiers.go?

I might be wrong but they feel a bit "out of their context" in here, given that some are also used in other contexts (not just random_string and random_password).

Personally, I agree with the above. It'll also help us as we consider making a corpus of "common" validators in the near future.

Makes sense to me. Have moved validators and plan modifiers to separate files.

main.go

bflad

I'd also like to discuss the merits of trying to do this in a minor release versus major release, since the migration testing is currently manual and there could be unexpected value behaviors when upgrading.

bflad · 2022-05-09T14:37:58Z

internal/provider/resource_id.go

+		resp.Diagnostics.AddError(
+			"generated insufficient random bytes: %s",
+			fmt.Sprintf("generated insufficient random bytes: %s", err),
+		)


Nit: Diagnostic summaries should generally be a "title" of sorts, e.g. what's the higher level issue?, leaving the details to explain the actual problem, offer troubleshooting advice, and/or next steps. Similar comment applies to any other place that's copying contents between summary and details.

For something like this, it'd be great to say something like:

Suggested change

resp.Diagnostics.AddError(

"generated insufficient random bytes: %s",

fmt.Sprintf("generated insufficient random bytes: %s", err),

)

resp.Diagnostics.AddError(

"Randomness Generation Error",

"While attempting to generate a random value for this resource, an insufficient number of random bytes were generated. Most commonly, this is a hardware or operating system issue where their random number generator does not provide a sufficient randomness source. Otherwise, it may represent an issue in the randomness handling of the provider.\n\n"+

"Retry the Terraform operation. If the error still occurs or happens regularly, please contact the provider developer with hardware and operating system information.\n\n"+

fmt.Sprintf("Original Error: %s", err)

)

If desired, this diagnostic can then be wrapped in a quick function and reused for other resources.

Reference: https://www.terraform.io/plugin/framework/diagnostics

I've used your wording and expanded the summary and detail messages for other diagnostics errors.

bflad · 2022-05-09T14:41:35Z

internal/provider/resource_id.go

@@ -4,17 +4,19 @@ import (
 	"context"
 	"crypto/rand"
 	"encoding/base64"
-	"encoding/hex"
+	hex2 "encoding/hex"


In general with Go, it is better to avoid renaming imports, especially those from standard library. Instead I'd recommend renaming the variable that's trying to use hex instead. 👍

Reference: https://github.com/golang/go/wiki/CodeReviewComments#imports

Ooops. My bad. Fixed.

internal/provider/resource_integer.go

bflad · 2022-05-09T14:52:31Z

internal/provider/string.go

+func validatorMinInt(min int64) tfsdk.AttributeValidator {
+	return minIntValidator{min}
+}
+
+type minIntValidator struct {
+	val int64
+}
+
+func (m minIntValidator) Description(context.Context) string {
+	return "MinInt validator ensures that attribute is at least val"
+}
+
+func (m minIntValidator) MarkdownDescription(context.Context) string {
+	return "MinInt validator ensures that attribute is at least `val`"
+}
+
+func (m minIntValidator) Validate(ctx context.Context, req tfsdk.ValidateAttributeRequest, resp *tfsdk.ValidateAttributeResponse) {
+	t := req.AttributeConfig.(types.Int64)
+
+	if t.Value < m.val {
+		resp.Diagnostics.AddError(
+			fmt.Sprintf("expected attribute to be at least %d, got %d", m.val, t.Value),
+			fmt.Sprintf("expected attribute to be at least %d, got %d", m.val, t.Value),
 		)
+	}
+}


Personally, I agree with the above. It'll also help us as we consider making a corpus of "common" validators in the near future.

bflad · 2022-05-09T14:57:57Z

internal/provider/string.go

+}
+
+func (m minIntValidator) Validate(ctx context.Context, req tfsdk.ValidateAttributeRequest, resp *tfsdk.ValidateAttributeResponse) {
+	t := req.AttributeConfig.(types.Int64)


I'd strongly encourage avoiding a direct type assertion in validators. This can cause a panic if it receives an unexpected type. My recommendation would be to at least use the two-value form of type assertion to return an error on an unexpected type and return early, e.g.

t, ok := req.AttributeConfig.(types.Int64) if !ok { resp.Diagnostics.AddError() return }

Or use something similar to the terraform.io documentation here with tfsdk.ValueAs():

var t types.Int64 diags := tfsdk.ValueAs(ctx, req.AttributeConfig, &t) resp.Diagnostics.Append(diags...) if diags.HasError() { return }

Reference: https://go.dev/doc/effective_go#interface_conversions
Reference: https://www.terraform.io/plugin/framework/validation#creating-attribute-validators

Have amended using the latter of your suggestions.

bendbennett · 2022-05-11T10:31:22Z

Left a few comments and nitpicks. In general this is looking really good.

Aside from those minor things, I do have a question (I might have asked this before already, so I apologise if I did): what's the plan in terms of Protocol 5 -> 6 support?

As part of the "Utility Providers RFC" we established we would be moving to 6 also as a way to "nudge" the ecosystem.

Is the intention here to do one last Protocol 5 release and then upgrade to 6, by removing mux? Or do you have a different plan?

TBH the mux is just present as I was using this whilst migrating one resource at a time from SDKv2 to Framework. As @bflad indicates, probably best to remove the muxing and do a breaking change release.

bendbennett · 2022-05-11T10:32:16Z

I'd also like to discuss the merits of trying to do this in a minor release versus major release, since the migration testing is currently manual and there could be unexpected value behaviors when upgrading.

Agree, that it's probably best to remove the muxing and have a breaking change major release.

…gger unexpected replace (#177)

bendbennett · 2022-06-27T12:56:23Z

I have separated out the resources into their own packages as it seems like there are details that are specific to the resources that shouldn’t be exposed, for instance, models and unexported funcs (e.g., upgradePasswordStateV1toV2(), generateHash() in password), but I'm happy to refactor if people would rather we kept the resources with the provider.

detro

A suggestion and a nitpick/question about the code restructuring.

detro · 2022-06-27T13:01:17Z

main.go

@@ -17,5 +20,10 @@ import (
 //go:generate go run github.com/hashicorp/terraform-plugin-docs/cmd/tfplugindocs

 func main() {
-	plugin.Serve(&plugin.ServeOpts{ProviderFunc: provider.New})
+	err := providerserver.Serve(context.Background(), provider.New, providerserver.ServeOpts{


Would you mind adding the -debug flag here, similarly to https://github.com/hashicorp/terraform-provider-tls/blob/main/main.go#L16 ?

This is not strictly in topic with this PR, but it seems small enough that I thought I'd mention it.

Good shout, have added.

detro · 2022-06-27T13:03:08Z

internal/provider/provider_resource_id_test.go

@@ -0,0 +1,98 @@
+package provider


Nit: General about the new code structure: what motivated moving resources into their own separate sub-packages, and away from their tests?

Asking just because it looks a bit unconventional, but there is nothing bad per se.

Maybe one thing that this causes, is that it removes the possibility to just see at the diff within the same files.

I have separated out the resources into their own packages as it seems like there are details that are specific to the resources that shouldn’t be exposed, for instance, models and unexported funcs (e.g., upgradePasswordStateV1toV2(), generateHash() in password), but I'm happy to refactor if people would rather we kept the resources with the provider.

In terms of the tests, I must admit I don't really see them as "resource" tests, we're exercising the provider and a resource it exposes (e.g., password, string etc) through the interaction with TF core , so I see them as provider tests. The tests that are specific to resources themselves (e.g., TestUpgradePasswordStateV0toV2) still live with the resource code, but there's an argument for removing these sorts of tests as they're essentially testing "private" (unexported) functions which is generally frowned upon.

Another, indirectly related aspect of the refactoring has to do with import cycles. Perhaps one of the reasons a provider and its resources normally reside within the same package (e.g., package provider) is to avoid the issue of import cycles when a resource needs to use the API client that is typically configured in the Configure() func, for instance:

func (p *provider) Configure(ctx context.Context, req tfsdk.ConfigureProviderRequest, resp *tfsdk.ConfigureProviderResponse) { p.client = client.NewClient() }

Access to the client is then usually achieved in the resource by using something like the following in the NewResource() func:

func (r resource) NewResource(_ context.Context, p tfsdk.Provider) (tfsdk.Resource, diag.Diagnostics) { return resource{ p: *(p.(*provider)), }, nil }

This seems a slightly peculiar pattern to me, as we have a provider that has resources but each resource also contains the provider and accesses "private" (unexported) fields on the provider in order to get the client for instance.

Although a client is not being used by the random provider, I'd be keen to encourage a pattern whereby the client can be obtained from the provider by checking to see if it implements an interface that allows access to the client without exposing any of the underlying "private" (unexported) fields of the provider. Perhaps something along the following lines:

package client type Client interface { Client() API } type API interface { GetString() string } func NewClient() *client { return &client{ msg: "hello", } } type client struct { msg string } func (c *client) GetString() string { return c.msg }

package provider func NewProvider() tfsdk.Provider { return &provider{} } var _ tfsdk.Provider = (*provider)(nil) type provider struct { client client.API } func (p *provider) Client() client.API { return p.client } func (p *provider) Configure(ctx context.Context, req tfsdk.ConfigureProviderRequest, resp *tfsdk.ConfigureProviderResponse) { p.client = client.NewClient() }

package resource func (r *resourceType) NewResource(_ context.Context, p tfsdk.Provider) (tfsdk.Resource, diag.Diagnostics) { c, ok := p.(client.Client) if !ok { panic("not ok") } return &resource{ client: c.Client(), }, nil } var ( _ tfsdk.Resource = (*resource)(nil) _ tfsdk.ResourceWithImportState = (*resource)(nil) ) type resource struct { client client.API } func (r *resource) Create(ctx context.Context, req tfsdk.CreateResourceRequest, resp *tfsdk.CreateResourceResponse) { fmt.Println(r.client.GetString())

So this is a very long-winded way of saying that the refactoring also encourages decoupling the provider and resources.

👍 I can't argue against incapsulation: I frankly dislike how Go doesn't support controlling visibility more granularly within the same package, so I guess this is the most idiomatic way to go.

In the future I might copy this approach.

Hey folks 👋 I'd like to chat about this more, but it is probably easier to do over a higher bandwidth medium.

In Go terms, the only "publicly facing" things in a provider codebase would be anything outside the internal/ directory. As long as things abide by that, you can export all types, methods, etc. without suggesting any sort of exported API that needs to worry about compatibility. Historically, most small providers are implemented in a "flat" package structure, e.g. everything terraform-plugin-sdk/framework related in internal/provider, to make things easier for contributors and avoid overcomplicating things with extra interfaces, etc.

@bflad I've gone ahead and refactored to have all of the resources and provider in package provider as I think the case for making things easier for contributors is a fair one. If you can take another look and let me know what you think. Thanks.

bflad

Looks good to me 🚀 Excellent work

bflad · 2022-07-01T13:52:06Z

internal/planmodifiers/attribute.go

+func (d *defaultValueAttributePlanModifier) Modify(ctx context.Context, req tfsdk.ModifyAttributePlanRequest, resp *tfsdk.ModifyAttributePlanResponse) {
+	// Do not set default if the attribute configuration has been set.
+	if !req.AttributeConfig.IsNull() {
+		return
+	}
+
+	resp.AttributePlan = d.val
+}


This implementation is okay for an attribute with RequiresReplace (as its used in this provider), however we should be careful to check whether the plan already has a known value if this code is moved to other implementations. Refer also to https://github.com/hashicorp/terraform-provider-tls/blob/27e3a082e712f81eb7f5bc126b3debdbedab550e/internal/provider/attribute_plan_modification/attribute_plan_modifiers.go#L37-L41

bflad · 2022-07-01T13:53:50Z

internal/provider/provider_test.go

+		"random": func() (tfprotov6.ProviderServer, error) {
+			return providerserver.NewProtocol6WithError(New())()
+		},


providerserver.NewProtocol6WithError() should already return the right thing to prevent the wrapping function 👍

Suggested change

"random": func() (tfprotov6.ProviderServer, error) {

return providerserver.NewProtocol6WithError(New())()

},

"random": providerserver.NewProtocol6WithError(New()),

bflad · 2022-07-01T13:55:52Z

internal/provider/resource_id.go

+		resp.Diagnostics.AddError(
+			"Randomness Generation Error",
+			"While attempting to generate a random value for this resource, an insufficient number of random bytes were generated. Most commonly, this is a hardware or operating system issue where their random number generator does not provide a sufficient randomness source. Otherwise, it may represent an issue in the randomness handling of the provider.\n\n"+
+				"Retry the Terraform operation. If the error still occurs or happens regularly, please contact the provider developer with hardware and operating system information.\n\n"+
+				fmt.Sprintf("Original Error: %s", err),
+		)


Nit: should this be using (or be added to) internal/diagnostics?

👍 moved.

bflad · 2022-07-01T14:01:17Z

internal/provider/resource_integer_test.go

 				Check: resource.ComposeTestCheckFunc(
-					testAccResourceIntegerBasic("random_integer.integer_1"),


Nice work on removing these 👍

github-actions bot added dependencies size/XL labels May 3, 2022

detro reviewed May 3, 2022

View reviewed changes

internal/provider_fm/resource_id.go Outdated Show resolved Hide resolved

detro reviewed May 3, 2022

View reviewed changes

internal/provider_fm/resource_id.go Outdated Show resolved Hide resolved

detro reviewed May 3, 2022

View reviewed changes

internal/provider_fm/resource_id.go Outdated Show resolved Hide resolved

bendbennett force-pushed the bendbennett/issues-177 branch 3 times, most recently from 550bbcb to 97ed28f Compare May 6, 2022 08:48

github-actions bot added the documentation label May 6, 2022

bendbennett marked this pull request as ready for review May 9, 2022 08:04

bendbennett requested a review from a team as a code owner May 9, 2022 08:04

bendbennett requested review from detro and bflad May 9, 2022 08:04

detro reviewed May 9, 2022

View reviewed changes

bflad reviewed May 9, 2022

View reviewed changes

bendbennett added 13 commits May 12, 2022 16:18

Configuring muxing (#177)

e302d7b

Migrating uuid resource to framework (#177)

b8cc057

Adding keepers into uuid resource schema (#177)

05aabad

Incorporating keepers for uuid resource (#177)

5668eb6

Migrating id resource to framework (#177)

1f6fcd9

Removing dead code (#177)

f537277

Migrate integer resource to framework (#177)

de8217e

Removing dead code and unnecessary type conversions (#177)

bf645bd

Updating types and adding empty validators and plan modifiers (#177)

0ea5717

Migrating password and string resources from SDKv2 to Framework (#177)

69c2547

Fixing description (#177)

4c0a6cf

Adding UpgradeState to string resource (#177)

ccc05d7

go fmt (#177)

1085fc6

bendbennett and others added 14 commits June 13, 2022 14:34

Refactoring tests to use helper functions (#177)

74ec49f

Refactoring tests to use helper functions (#177)

561a325

Using modified version of RequiresReplace (#177)

a1aaabf

Merge main (#177)

1877941

Using terraform-plugin-framework-validators (#177)

01a1558

Fixing test (#177)

1251d07

Porting setting default values during import password and string (#177)

818a27b

Removing deprecated number attribute (#177)

643151b

Updating CHANGELOG.md (#177)

8a35a9e

Merge branch 'main' into bendbennett/issues-177

872d90e

Updating CHANGELOG.md (#177)

aaeab93

Updating docs (#177)

d12f1ad

Refactoring (#177)

f888917

Adding acceptance tests to verify upgrading to framework does not tri…

f29402f

…gger unexpected replace (#177)

bendbennett force-pushed the bendbennett/issues-177 branch from bf97f8f to f29402f Compare June 27, 2022 10:45

Renaming files (#177)

6c94c96

bendbennett requested review from detro and bflad June 27, 2022 12:56

detro approved these changes Jun 27, 2022

View reviewed changes

bendbennett added 6 commits June 28, 2022 11:36

Adding support for debug flag (#177)

c75de56

Bumping protocol version (#177)

8565799

Merge remote-tracking branch 'origin/main' into bendbennett/issues-177

81c6699

Refactoring (#177)

6272933

Merge remote-tracking branch 'origin/main' into bendbennett/issues-177

20d678f

Updating tests to use NewProtocol6WithError (#177)

a53a450

bflad approved these changes Jul 1, 2022

View reviewed changes

Updates following code review (#177)

dd331c5

bendbennett merged commit 6af97c9 into main Jul 1, 2022

bendbennett deleted the bendbennett/issues-177 branch July 1, 2022 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate from SDKv2 => Framework #249

Migrate from SDKv2 => Framework #249

bendbennett commented May 3, 2022 •

edited

detro left a comment

detro May 9, 2022

bflad May 9, 2022

bendbennett May 10, 2022

bflad left a comment

bflad May 9, 2022

bendbennett May 10, 2022

bflad May 9, 2022

bendbennett May 10, 2022

bflad May 9, 2022

bflad May 9, 2022

bendbennett May 10, 2022

bendbennett commented May 11, 2022

bendbennett commented May 11, 2022

bendbennett commented Jun 27, 2022

detro left a comment

detro Jun 27, 2022

bendbennett Jun 28, 2022

detro Jun 27, 2022

detro Jun 27, 2022

bendbennett Jun 28, 2022

detro Jun 28, 2022

bflad Jun 29, 2022

bendbennett Jun 30, 2022

bflad left a comment

bflad Jul 1, 2022

bflad Jul 1, 2022

bflad Jul 1, 2022

bendbennett Jul 1, 2022

bflad Jul 1, 2022

		Check: resource.ComposeTestCheckFunc(
		testAccResourceIntegerBasic("random_integer.integer_1"),

Migrate from SDKv2 => Framework #249

Migrate from SDKv2 => Framework #249

Conversation

bendbennett commented May 3, 2022 • edited

Objectives

Items Requiring Resolving

Breaking Changes

detro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendbennett commented May 11, 2022

bendbennett commented May 11, 2022

bendbennett commented Jun 27, 2022

detro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendbennett commented May 3, 2022 •

edited