Refactor how we're serving providers #215
Comments
paddycarver
added
the
tech-debt
|
Yes, please! This writeup matches some fleeting thoughts I have had as well when working with this area of the code and it is great to have this captured so well in an issue. |
|
I think this pairs well with #172 👍 |
Closed
bflad
added a commit
that referenced
this issue
Apr 27, 2022
Reference: #215 Reference: #294 Reference: #296 This change represents the first half of the work necessary to extract the `tfprotov6.ProviderServer` implementation and helper functions out of the `tfsdk` package and into separate packages. The `providerserver` package will be the provider developer facing functionality, while the next iteration of this refactoring will move the actual server implementation into a separate internal package. Once in that separate internal package, efforts can be made to make that code handle terraform-plugin-go type conversions "at the edge" better.
bflad
added a commit
that referenced
this issue
Apr 28, 2022
#308) Reference: #215 Reference: #294 Reference: #296 This change represents the first half of the work necessary to extract the `tfprotov6.ProviderServer` implementation and helper functions out of the `tfsdk` package and into separate packages. The `providerserver` package will be the provider developer facing functionality, while the next iteration of this refactoring will move the actual server implementation into a separate internal package. Once in that separate internal package, efforts can be made to make that code handle terraform-plugin-go type conversions "at the edge" better.
bflad
added a commit
that referenced
this issue
Apr 29, 2022
Reference: #215 This change was accomplished by: - `mkdir internal/proto6server` - `git mv tfsdk/serve* internal/proto6server/` - Replacing `package tfsdk` with `package proto6server` in those moved files - Removing `NewProtocol6Server()`, `Serve()`, and `ServeOpts` in moved files - Adding necessary `tfsdk.` scoping to the now external `tfsdk` package references - Moved necessary unexported methods (e.g. `Schema.tfprotov6Schema()`) to `internal/toproto6` (this package will be expanded further in the future) - Moved attribute plan modification testing (including test plan modifiers) to `internal/proto6server/attribute_plan_modification_test.go` - Moved attribute validation testing (including test validators) to `internal/proto6server/attribute_validation_test.go` - Moved block plan modification testing (including test plan modifiers) to `internal/proto6server/block_plan_modification_test.go` - Moved block validation testing (including test validators) to `internal/proto6server/block_validation_test.go` - Copied tfsdk Config/Plan/State.getAttributeValue methods to internal/proto6server (Config/Plan/State)GetAttributeValue functions temporarily to not export existing methods There are some other planned refactorings in the future for the `internal/proto6server` and `tfsdk` code including: - Creating a shared implementation for the underlying `Config`/`Plan`/`State` details - Creating a `internal/fromproto6` package with protocol version 6 type to framework type conversions - Migrating additional framework type to protocol version 6 code to `internal/toproto6` - Migrating RPC handling into individual code and test files These will be handled separately to reduce review burden, as this change was already very large.
bflad
added a commit
that referenced
this issue
May 3, 2022
Reference: #215 This change was accomplished by: - `mkdir internal/proto6server` - `git mv tfsdk/serve* internal/proto6server/` - Replacing `package tfsdk` with `package proto6server` in those moved files - Removing `NewProtocol6Server()`, `Serve()`, and `ServeOpts` in moved files - Adding necessary `tfsdk.` scoping to the now external `tfsdk` package references - Moved necessary unexported methods (e.g. `Schema.tfprotov6Schema()`) to `internal/toproto6` (this package will be expanded further in the future) - Moved attribute plan modification testing (including test plan modifiers) to `internal/proto6server/attribute_plan_modification_test.go` - Moved attribute validation testing (including test validators) to `internal/proto6server/attribute_validation_test.go` - Moved block plan modification testing (including test plan modifiers) to `internal/proto6server/block_plan_modification_test.go` - Moved block validation testing (including test validators) to `internal/proto6server/block_validation_test.go` - Copied tfsdk Config/Plan/State.getAttributeValue methods to internal/proto6server (Config/Plan/State)GetAttributeValue functions temporarily to not export existing methods There are some other planned refactorings in the future for the `internal/proto6server` and `tfsdk` code including: - Creating a shared implementation for the underlying `Config`/`Plan`/`State` details - Creating a `internal/fromproto6` package with protocol version 6 type to framework type conversions - Migrating additional framework type to protocol version 6 code to `internal/toproto6` - Migrating RPC handling into individual code and test files These will be handled separately to reduce review burden, as this change was already very large.
bflad
added a commit
that referenced
this issue
May 6, 2022
Reference: #215 This step of the provider server refactoring introduces a new `internal/fwserver` package that will contain the provider server implementation for any protocol version, there only being a version 6 implementation at the moment. This package will contain only framework-native types that aren't already defined by the `tfsdk` package that are used by provider developers today, which is necessary to prevent import cycles. The protocol specific implementations will then wrap this framework server implementation and handle any necessary conversion of protocol specific types to framework specific types and back. Eventually, the existing `internal/proto6server` implementation will make no reference to any framework native types except the framework server and protocol type conversions. For now, only the `GetProviderSchema` RPC is migrated to show what this refactoring will look like for the rest of the RPCs. This has a few benefits which can be seen here including clear abstractions for protocol specific handling versus generic framework handling and unit testing at those abstraction boundaries.
bflad
added a commit
that referenced
this issue
May 6, 2022
Reference: #215 This step of the provider server refactoring introduces a new `internal/fwserver` package that will contain the provider server implementation for any protocol version, there only being a version 6 implementation at the moment. This package will contain only framework-native types that aren't already defined by the `tfsdk` package that are used by provider developers today, which is necessary to prevent import cycles. The protocol specific implementations will then wrap this framework server implementation and handle any necessary conversion of protocol specific types to framework specific types and back. Eventually, the existing `internal/proto6server` implementation will make no reference to any framework native types except the framework server and protocol type conversions. For now, only the `GetProviderSchema` RPC is migrated to show what this refactoring will look like for the rest of the RPCs. This has a few benefits which can be seen here including clear abstractions for protocol specific handling versus generic framework handling and unit testing at those abstraction boundaries.
bflad
added a commit
that referenced
this issue
May 9, 2022
… and toproto6 Reference: #215 One particular quirk that this implementation shows is that it is not entirely possible to avoid working with `tfsdk`/`fwserver` bits in `proto6server` as the current data types within the framework (`Config`, `Plan`, `State`) require the `Schema` so data can be appropriately converted from the protocol to the framework. To support this effort and prevent repetitively executing the provider defined `GetSchema` method, a `fwserver` method is introduced that caches the `Schema` and `Diagnostics` results on first use. This methodology can also be applied in the future to resolve #299.
bflad
added a commit
that referenced
this issue
May 11, 2022
… and toproto6 (#320) Reference: #215 One particular quirk that this implementation shows is that it is not entirely possible to avoid working with `tfsdk`/`fwserver` bits in `proto6server` as the current data types within the framework (`Config`, `Plan`, `State`) require the `Schema` so data can be appropriately converted from the protocol to the framework. To support this effort and prevent repetitively executing the provider defined `GetSchema` method, a `fwserver` method is introduced that caches the `Schema` and `Diagnostics` results on first use. This methodology can also be applied in the future to resolve #299.
bflad
added a commit
that referenced
this issue
May 11, 2022
bflad
added a commit
that referenced
this issue
May 11, 2022
…r, and toproto6 Reference: #215 Reference: #299 This introduces framework server caching for data source types and schemas, similar to the provider schema cache, which is needed with other data source RPCs. The same methodology will be applied for resource types and schemas, when the `ValidateResourceConfig` RPC is migrated.
bflad
added a commit
that referenced
this issue
May 27, 2022
…eConfig testing and update proto6server testing Reference: #215
bflad
added a commit
that referenced
this issue
Jun 2, 2022
…server testing Reference: #215 This also changes a potential panic into an error where the incoming Config is missing, which was discovered via the new unit testing. Terraform always fills in this data currently, so it does not warrant any CHANGELOG entry, but is good from a defensive coding standpoint.
bflad
added a commit
that referenced
this issue
Jun 2, 2022
…server testing Reference: #215 This also changes a potential panic into an error where the incoming Config is missing, which was discovered via the new unit testing. Terraform always fills in this data currently, so it does not warrant any CHANGELOG entry, but is good from a defensive coding standpoint.
bflad
added a commit
that referenced
this issue
Jun 6, 2022
…server testing (#359) Reference: #215 This also changes a potential panic into an error where the incoming Config is missing, which was discovered via the new unit testing. Terraform always fills in this data currently, so it does not warrant any CHANGELOG entry, but is good from a defensive coding standpoint.
This was referenced Jun 8, 2022
bflad
added a commit
that referenced
this issue
Jun 8, 2022
…oto6server testing Reference: #215 Also removes the placeholder `internal/testing/emptyprovider` implementation in preference of the fully built out declarative one in `internal/testing/testprovider`. At this point we should hopefully feel pretty confident in the framework and protocol version 6 server unit testing focusing on their logic and data handoffs to other functions/methods. This also means we should be in a good spot to create a protocol version 5 implementation, very similar to the protocol version 6 implementation.
bflad
added a commit
that referenced
this issue
Jun 13, 2022
…oto6server testing (#367) Reference: #215 Also removes the placeholder `internal/testing/emptyprovider` implementation in preference of the fully built out declarative one in `internal/testing/testprovider`. At this point we should hopefully feel pretty confident in the framework and protocol version 6 server unit testing focusing on their logic and data handoffs to other functions/methods. This also means we should be in a good spot to create a protocol version 5 implementation, very similar to the protocol version 6 implementation.
bflad
added a commit
that referenced
this issue
Jun 22, 2022
Reference: #81 Reference: #161 Reference: #215 Reference: #365 This introduces a native abstraction over terraform-plugin-go's `tftypes.AttributePath`, allowing the framework to own the implementation details and extend the functionality further. Provider developers will be closer to removing a direct dependency on terraform-plugin-go. This is a major breaking change, however it is necessary before 1.0 to prevent compatibility issues in the future. This functionality is only intended to replace `tftypes.AttributePath` usage and not mess with the `tfsdk.Config`, `tfsdk.Plan`, and `tfsdk.State` type `tftypes.Value` data storage fields or their underlying data manipulation logic. This does leave the framework in an awkward half-state until those are further refactored (likely towards native `attr.Value`), but is done to try and reduce the review complexity of this initial migration. Additional followup changes will introduce the concept of path expressions, which will allow provider developers to match against multiple paths or reference parent paths in schema-based plan modifier and validation functionality. To prevent import cycles between `attr` and `diag` packages due changing the `attr.TypeWithValidate` type `Validate` method signature from `Validate(context.Context, tftypes.Value, *tftypes.AttributePath) diag.Diagnostics` to `Validate(context.Context, tftypes.Value, path.Path) diag.Diagnostics`, the `TypeWithValidation` interface is moved a new `xattr` package underneath `attr` with Go and website documentation to direct provider developers to the other package. This will also solve a prior issue with trying to implement `TypeWithModifyPlan` support. Naming and location of anything in this initial implementation can be adjusted as necessary. Provider developers can migrate to the new path handling by replacing: ```go tftypes.NewAttributePath().WithAttributeName("example") ``` With the equivalent: ```go path.RootPath("example") ``` Then using the `(Path).At*` methods to extend the path definition instead of `(*tftypes.AttributePath).With*` methods: | Current | New | | ------------------------ | --------------- | | `WithAttributeName()` | `AtName()` | | `WithElementKeyInt()` | `AtListIndex()` | | `WithElementKeyString()` | `AtMapKey()` | | `WithElementKeyValue()` | `AtSetValue()` |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Currently, a lot of logic is living in the
tfsdk/serve.gofile. It's basically gluing together all the abstractions and helpers in the framework, and it's where the magic happens. It also is what surfaces a gRPC server that Terraform can connect to.I think having those two concerns colocated is probably a bit of technical debt. Doing so means that testing the magic needs to be tested at the gRPC level. It also means that code to mutate values and call helpers is living right next to code that converts between the framework's types and the gRPC server's.
I think we could make the methods on the server thin wrappers around calls with more narrow function signatures and requirements, and that would allow us to write more tests more easily, because we wouldn't need to implement an entire provider to integration test against. The server methods would just be responsible for converting from gRPC types into framework types, calling these functions, and converting back. The functions would just be responsible for the mutation and calling other helpers/calling into the framework injection points.
I think this would make it a little easier to navigate, test, and understand the codebase.
The text was updated successfully, but these errors were encountered: