Introduce equality checking of PreparedConfig in PrepareProviderConfig and ValidateProviderConfig #54

bflad · 2022-02-15T20:33:15Z

Closes #51
Reference: hashicorp/terraform-provider-corner#44

Previously in real world usage:


    provider_test.go:11: Step 1/1 error: Error running pre-apply refresh: exit status 1

        Error: Invalid provider configuration

        Provider "registry.terraform.io/hashicorp/tf5muxprovider" requires explicit
        configuration. Add a provider block to the root module and configure the
        provider's required arguments as described in the provider documentation.

        Error: Plugin error

          with provider["registry.terraform.io/hashicorp/tf5muxprovider"],
          on <empty> line 0:
          (source code not available)

        The plugin returned an unexpected error from
        plugin.(*GRPCProvider).ValidateProviderConfig: rpc error: code = Unknown desc
        = got a PrepareProviderConfig PreparedConfig response from multiple servers,
        not sure which to use

    provider_test.go:11: Step 1/1 error: Error running pre-apply refresh: exit status 1

        Error: Plugin error

        The plugin returned an unexpected error from
        plugin6.(*GRPCProvider).ValidateProviderConfig: rpc error: code = Unknown
        desc = got a ValidateProviderConfig PreparedConfig response from multiple
        servers, not sure which to use

        Error: Invalid provider configuration

        Provider "registry.terraform.io/hashicorp/tf6muxprovider" requires explicit
        configuration. Add a provider block to the root module and configure the
        provider's required arguments as described in the provider documentation.

Updated unit testing before code changes:

--- FAIL: TestMuxServerPrepareProviderConfig (0.00s)
    --- FAIL: TestMuxServerPrepareProviderConfig/PreparedConfig-multiple-equal (0.00s)
        mux_server_PrepareProviderConfig_test.go:365: wanted no error, got error: got a PrepareProviderConfig PreparedConfig response from multiple servers, not sure which to use
FAIL
FAIL    github.com/hashicorp/terraform-plugin-mux/tf5muxserver  1.244s

--- FAIL: TestMuxServerValidateProviderConfig (0.00s)
    --- FAIL: TestMuxServerValidateProviderConfig/PreparedConfig-multiple-equal (0.00s)
        mux_server_ValidateProviderConfig_test.go:365: wanted no error, got error: got a ValidateProviderConfig PreparedConfig response from multiple servers, not sure which to use
FAIL
FAIL    github.com/hashicorp/terraform-plugin-mux/tf6muxserver  0.665s

Verified those integration tests are now passing by replacing the mux dependency with this branch. Once those tests are published, we can introduce the integration tests into this project's testing workflow.

…g and ValidateProviderConfig Reference: #51 Previously in real world usage: ``` provider_test.go:11: Step 1/1 error: Error running pre-apply refresh: exit status 1 Error: Invalid provider configuration Provider "registry.terraform.io/hashicorp/tf5muxprovider" requires explicit configuration. Add a provider block to the root module and configure the provider's required arguments as described in the provider documentation. Error: Plugin error with provider["registry.terraform.io/hashicorp/tf5muxprovider"], on <empty> line 0: (source code not available) The plugin returned an unexpected error from plugin.(*GRPCProvider).ValidateProviderConfig: rpc error: code = Unknown desc = got a PrepareProviderConfig PreparedConfig response from multiple servers, not sure which to use provider_test.go:11: Step 1/1 error: Error running pre-apply refresh: exit status 1 Error: Plugin error The plugin returned an unexpected error from plugin6.(*GRPCProvider).ValidateProviderConfig: rpc error: code = Unknown desc = got a ValidateProviderConfig PreparedConfig response from multiple servers, not sure which to use Error: Invalid provider configuration Provider "registry.terraform.io/hashicorp/tf6muxprovider" requires explicit configuration. Add a provider block to the root module and configure the provider's required arguments as described in the provider documentation. ``` Updated unit testing before code changes: ``` --- FAIL: TestMuxServerPrepareProviderConfig (0.00s) --- FAIL: TestMuxServerPrepareProviderConfig/PreparedConfig-multiple-equal (0.00s) mux_server_PrepareProviderConfig_test.go:365: wanted no error, got error: got a PrepareProviderConfig PreparedConfig response from multiple servers, not sure which to use FAIL FAIL github.com/hashicorp/terraform-plugin-mux/tf5muxserver 1.244s --- FAIL: TestMuxServerValidateProviderConfig (0.00s) --- FAIL: TestMuxServerValidateProviderConfig/PreparedConfig-multiple-equal (0.00s) mux_server_ValidateProviderConfig_test.go:365: wanted no error, got error: got a ValidateProviderConfig PreparedConfig response from multiple servers, not sure which to use FAIL FAIL github.com/hashicorp/terraform-plugin-mux/tf6muxserver 0.665s ``` Verified those integration tests are now passing by replacing the mux dependency with this branch. Once those tests are published, we can introduce the integration tests into this project's testing workflow.

detro

OK, I think to properly understand this PR I need to understand this IF:

if !equal {
    return nil, fmt.Errorf("got different PrepareProviderConfig PreparedConfig response from multiple servers, not sure which to use")
}

Not because it's unclear, but it's because I was expecting the opposite.

Don't servers provide a Schema? And if so, and if we are mux-ing them, should those schemas be "complementary" so that this library can correctly dispatch requests to the right server? Shouldn't then our expectation be that those servers configuration should differ instead of converge?

detro · 2022-02-17T11:52:01Z

tf5muxserver/mux_server_PrepareProviderConfig.go

+		if !equal {
+			return nil, fmt.Errorf("got different PrepareProviderConfig PreparedConfig response from multiple servers, not sure which to use")
+		}


So, if 2 servers expose the same configuration schema, doesn't that mean that we are creating a situation where we don't know which of the 2 (or more) servers should be handed over a request?

What I was expecting, is to see a logic that enforced that each server would cover different resources/data-sources. What am I missing?

You are correct that upfront, the server creation will verify that all servers have the same schema, or otherwise return an error.

PreparedConfig is a data structure that's implemented from that schema and has an interesting history in Terraform. It allowed providers to modify the incoming configuration value of a provider and return a different result (e.g. marking a schema attribute as required, receiving no configuration value, then filling the configuration value via environment variable value, which Terraform would then "allow" in the required sense).

As with other parts of Terraform, this modification possibility was left for backwards compatibility (e.g. with terraform-plugin-sdk/v2), however going forward this capability is being discouraged in implementations. For example, terraform-plugin-framework uses the read-only tfsdk.Config type when handling this data. Since both terraform-plugin-sdk/v2 and terraform-plugin-framework do however copy the configuration into PreparedConfig, we need to allow equal return values, but still want to prevent unequal values given our more recent implementation goals.

OK so, I reached out to @bflad and he clarified things for me. What we are verifying here is that the configuration returned by the muxed servers, after PrepareProviderConfig (protocol version 5) and ValidateProviderConfig (protocol version 6), is STILL consistent.

While the HCL configuration fed to the provider is one and only one, because we are muxing, each server might be implementing transformations of the configuration, and the result would be incompatible.

This whole check is to avoid this: if one creates a provider muxing servers, and one (or more) of them end up creating different configurations, this library will detect it and return an error.

detro · 2022-02-17T11:52:57Z

tf5muxserver/mux_server_PrepareProviderConfig.go


-			resp.PreparedConfig = res.PreparedConfig
+		equal, err := dynamicValueEquals(schemaType(s.providerSchema), res.PreparedConfig, resp.PreparedConfig)


With this we are moving from just "fail if both res.PrepareConfig and resp.PrepareConfig are set" to actually checking for equality based on type.

Correct. My guess is that the previous implementation assumed that terraform-plugin-sdk/v2 (since this came from the older SchemaServerFactory implementation) did not automatically fill in the PreparedConfig value, however it does.

detro · 2022-02-17T12:04:24Z

tf6muxserver/mux_server_ValidateProviderConfig.go

+			return nil, fmt.Errorf("got different PrepareProviderConfig PreparedConfig response from multiple servers, not sure which to use")
+		}
+
+		resp.PreparedConfig = res.PreparedConfig


In this loop, at the first chance that we get back a res and resp is still nil, we assign resp = res (line 36 above).
Than later on we keep looping, and if we reach line 61, it means we found a res.PrepareConfig identical to resp.PrepareConfig, so we set resp.PrepareConfig again. Doesn't that risk creating a resp that is based on 2 difference "sources"?

Maybe that's precisely what you are after, and I just haven't learned enough of mux yet, but it feels... odd (for lack of better words).

I suspect this is the same above for protocol 5.0.

Doesn't that risk creating a resp that is based on 2 difference "sources"?

Technically, yes, but the data is equal by this point, so it shouldn't matter. We could wrap this with if resp.PreparedConfig == nil { resp.PreparedConfig = res.PreparedConfig } conditional or prepend a if resp.PreparedConfig != nil { continue } before here, but both felt extraneous when the final result isn't changing. Willing to change it for clarity if it helps though.

bflad

Don't servers provide a Schema? And if so, and if we are mux-ing them, should those schemas be "complementary" so that this library can correctly dispatch requests to the right server? Shouldn't then our expectation be that those servers configuration should differ instead of converge?

Hopefully my longer comment about PreparedConfig explains this, but please reach out if it doesn't. Two providers returning different results would be unexpected and the practice of changing PreparedConfig (which is generally copied from the incoming configuration) is discouraged going forward.

bflad · 2022-02-17T14:30:18Z

tf5muxserver/mux_server_PrepareProviderConfig.go


-			resp.PreparedConfig = res.PreparedConfig
+		equal, err := dynamicValueEquals(schemaType(s.providerSchema), res.PreparedConfig, resp.PreparedConfig)


Correct. My guess is that the previous implementation assumed that terraform-plugin-sdk/v2 (since this came from the older SchemaServerFactory implementation) did not automatically fill in the PreparedConfig value, however it does.

bflad · 2022-02-17T14:39:31Z

tf5muxserver/mux_server_PrepareProviderConfig.go

+		if !equal {
+			return nil, fmt.Errorf("got different PrepareProviderConfig PreparedConfig response from multiple servers, not sure which to use")
+		}


You are correct that upfront, the server creation will verify that all servers have the same schema, or otherwise return an error.

PreparedConfig is a data structure that's implemented from that schema and has an interesting history in Terraform. It allowed providers to modify the incoming configuration value of a provider and return a different result (e.g. marking a schema attribute as required, receiving no configuration value, then filling the configuration value via environment variable value, which Terraform would then "allow" in the required sense).

As with other parts of Terraform, this modification possibility was left for backwards compatibility (e.g. with terraform-plugin-sdk/v2), however going forward this capability is being discouraged in implementations. For example, terraform-plugin-framework uses the read-only tfsdk.Config type when handling this data. Since both terraform-plugin-sdk/v2 and terraform-plugin-framework do however copy the configuration into PreparedConfig, we need to allow equal return values, but still want to prevent unequal values given our more recent implementation goals.

bflad · 2022-02-17T14:43:26Z

tf6muxserver/mux_server_ValidateProviderConfig.go

+			return nil, fmt.Errorf("got different PrepareProviderConfig PreparedConfig response from multiple servers, not sure which to use")
+		}
+
+		resp.PreparedConfig = res.PreparedConfig


Doesn't that risk creating a resp that is based on 2 difference "sources"?

Technically, yes, but the data is equal by this point, so it shouldn't matter. We could wrap this with if resp.PreparedConfig == nil { resp.PreparedConfig = res.PreparedConfig } conditional or prepend a if resp.PreparedConfig != nil { continue } before here, but both felt extraneous when the final result isn't changing. Willing to change it for clarity if it helps though.

detro

Brian helped me grasp better what he is implementing here, so it's 💚

github-actions · 2024-05-28T12:05:12Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

bflad added the bug Something isn't working label Feb 15, 2022

bflad added this to the v0.5.1 milestone Feb 15, 2022

bflad requested a review from a team as a code owner February 15, 2022 20:33

Update CHANGELOG for #54

d8fdec8

This was referenced Feb 15, 2022

Add terraform-plugin-mux@v0.5.0 testing providers hashicorp/terraform-provider-corner#44

Merged

Consider Adding tfprotov5/tfprotov6 Schema* Methods for tftypes.Type Conversion hashicorp/terraform-plugin-go#157

Closed

detro reviewed Feb 17, 2022

View reviewed changes

bflad commented Feb 17, 2022

View reviewed changes

detro approved these changes Feb 17, 2022

View reviewed changes

bflad merged commit 82e4ba5 into main Feb 17, 2022

bflad deleted the bflad-PreparedConfig branch February 17, 2022 17:52

bflad mentioned this pull request Feb 17, 2022

tf5muxserver+tf6muxserver: Replace Schema tftypes.Type Functions With terraform-plugin-go ValueType() Methods #58

Closed

github-actions bot locked as resolved and limited conversation to collaborators May 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce equality checking of PreparedConfig in PrepareProviderConfig and ValidateProviderConfig #54

Introduce equality checking of PreparedConfig in PrepareProviderConfig and ValidateProviderConfig #54

bflad commented Feb 15, 2022

detro left a comment

detro Feb 17, 2022

bflad Feb 17, 2022

detro Feb 17, 2022

detro Feb 17, 2022

bflad Feb 17, 2022

detro Feb 17, 2022

bflad Feb 17, 2022

bflad left a comment

bflad Feb 17, 2022

bflad Feb 17, 2022

bflad Feb 17, 2022

detro left a comment

github-actions bot commented May 28, 2024


		resp.PreparedConfig = res.PreparedConfig
		equal, err := dynamicValueEquals(schemaType(s.providerSchema), res.PreparedConfig, resp.PreparedConfig)

Introduce equality checking of PreparedConfig in PrepareProviderConfig and ValidateProviderConfig #54

Introduce equality checking of PreparedConfig in PrepareProviderConfig and ValidateProviderConfig #54

Conversation

bflad commented Feb 15, 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

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

detro left a comment

Choose a reason for hiding this comment

github-actions bot commented May 28, 2024