-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce equality checking of PreparedConfig in PrepareProviderConfig and ValidateProviderConfig #54
Introduce equality checking of PreparedConfig in PrepareProviderConfig and ValidateProviderConfig #54
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
```release-note:bug | ||
tf5muxserver: Prevent `PrepareProviderConfig` RPC error for multiple `PreparedConfig` responses when combining terraform-plugin-sdk/v2 providers | ||
``` | ||
|
||
```release-note:bug | ||
tf6muxserver: Prevent `ValidateProviderConfig` RPC error for multiple `PreparedConfig` responses when combining terraform-plugin-framework providers | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
package tf5muxserver | ||
|
||
import ( | ||
"fmt" | ||
|
||
"github.com/hashicorp/terraform-plugin-go/tfprotov5" | ||
"github.com/hashicorp/terraform-plugin-go/tftypes" | ||
) | ||
|
||
// dynamicValueEquals performs equality checking of DynamicValue. | ||
func dynamicValueEquals(schemaType tftypes.Type, i *tfprotov5.DynamicValue, j *tfprotov5.DynamicValue) (bool, error) { | ||
if i == nil { | ||
return j == nil, nil | ||
} | ||
|
||
if j == nil { | ||
return false, nil | ||
} | ||
|
||
// Upstream will panic on DynamicValue.Unmarshal with nil Type | ||
if schemaType == nil { | ||
return false, fmt.Errorf("unable to unmarshal DynamicValue: missing Type") | ||
} | ||
|
||
iValue, err := i.Unmarshal(schemaType) | ||
|
||
if err != nil { | ||
return false, fmt.Errorf("unable to unmarshal DynamicValue: %w", err) | ||
} | ||
|
||
jValue, err := j.Unmarshal(schemaType) | ||
|
||
if err != nil { | ||
return false, fmt.Errorf("unable to unmarshal DynamicValue: %w", err) | ||
} | ||
|
||
return iValue.Equal(jValue), nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,283 @@ | ||
package tf5muxserver | ||
|
||
import ( | ||
"fmt" | ||
"strings" | ||
"testing" | ||
|
||
"github.com/hashicorp/terraform-plugin-go/tfprotov5" | ||
"github.com/hashicorp/terraform-plugin-go/tftypes" | ||
) | ||
|
||
func TestDynamicValueEquals(t *testing.T) { | ||
t.Parallel() | ||
|
||
testCases := map[string]struct { | ||
schemaType tftypes.Type | ||
dynamicValue1 func() (*tfprotov5.DynamicValue, error) | ||
dynamicValue2 func() (*tfprotov5.DynamicValue, error) | ||
expected bool | ||
expectedError error | ||
}{ | ||
"all-missing": { | ||
schemaType: nil, | ||
dynamicValue1: func() (*tfprotov5.DynamicValue, error) { | ||
return nil, nil | ||
}, | ||
dynamicValue2: func() (*tfprotov5.DynamicValue, error) { | ||
return nil, nil | ||
}, | ||
expected: true, | ||
}, | ||
"first-missing": { | ||
schemaType: nil, | ||
dynamicValue1: func() (*tfprotov5.DynamicValue, error) { | ||
return nil, nil | ||
}, | ||
dynamicValue2: func() (*tfprotov5.DynamicValue, error) { | ||
return &tfprotov5.DynamicValue{}, nil | ||
}, | ||
expected: false, | ||
}, | ||
"second-missing": { | ||
schemaType: nil, | ||
dynamicValue1: func() (*tfprotov5.DynamicValue, error) { | ||
return &tfprotov5.DynamicValue{}, nil | ||
}, | ||
dynamicValue2: func() (*tfprotov5.DynamicValue, error) { | ||
return nil, nil | ||
}, | ||
expected: false, | ||
}, | ||
"missing-type": { | ||
schemaType: nil, | ||
dynamicValue1: func() (*tfprotov5.DynamicValue, error) { | ||
dv, err := tfprotov5.NewDynamicValue( | ||
tftypes.Object{ | ||
AttributeTypes: map[string]tftypes.Type{ | ||
"test_string_attribute": tftypes.String, | ||
}, | ||
}, | ||
tftypes.NewValue( | ||
tftypes.Object{ | ||
AttributeTypes: map[string]tftypes.Type{ | ||
"test_string_attribute": tftypes.String, | ||
}, | ||
}, | ||
map[string]tftypes.Value{ | ||
"test_string_attribute": tftypes.NewValue(tftypes.String, "test-value"), | ||
}, | ||
), | ||
) | ||
return &dv, err | ||
}, | ||
dynamicValue2: func() (*tfprotov5.DynamicValue, error) { | ||
dv, err := tfprotov5.NewDynamicValue( | ||
tftypes.Object{ | ||
AttributeTypes: map[string]tftypes.Type{ | ||
"test_string_attribute": tftypes.String, | ||
}, | ||
}, | ||
tftypes.NewValue( | ||
tftypes.Object{ | ||
AttributeTypes: map[string]tftypes.Type{ | ||
"test_string_attribute": tftypes.String, | ||
}, | ||
}, | ||
map[string]tftypes.Value{ | ||
"test_string_attribute": tftypes.NewValue(tftypes.String, "test-value"), | ||
}, | ||
), | ||
) | ||
return &dv, err | ||
}, | ||
expected: false, | ||
expectedError: fmt.Errorf("unable to unmarshal DynamicValue: missing Type"), | ||
}, | ||
"mismatched-type": { | ||
schemaType: tftypes.Object{ | ||
AttributeTypes: map[string]tftypes.Type{ | ||
"test_bool_attribute": tftypes.Bool, | ||
}, | ||
}, | ||
dynamicValue1: func() (*tfprotov5.DynamicValue, error) { | ||
dv, err := tfprotov5.NewDynamicValue( | ||
tftypes.Object{ | ||
AttributeTypes: map[string]tftypes.Type{ | ||
"test_string_attribute": tftypes.String, | ||
}, | ||
}, | ||
tftypes.NewValue( | ||
tftypes.Object{ | ||
AttributeTypes: map[string]tftypes.Type{ | ||
"test_string_attribute": tftypes.String, | ||
}, | ||
}, | ||
map[string]tftypes.Value{ | ||
"test_string_attribute": tftypes.NewValue(tftypes.String, "test-value"), | ||
}, | ||
), | ||
) | ||
return &dv, err | ||
}, | ||
dynamicValue2: func() (*tfprotov5.DynamicValue, error) { | ||
dv, err := tfprotov5.NewDynamicValue( | ||
tftypes.Object{ | ||
AttributeTypes: map[string]tftypes.Type{ | ||
"test_string_attribute": tftypes.String, | ||
}, | ||
}, | ||
tftypes.NewValue( | ||
tftypes.Object{ | ||
AttributeTypes: map[string]tftypes.Type{ | ||
"test_string_attribute": tftypes.String, | ||
}, | ||
}, | ||
map[string]tftypes.Value{ | ||
"test_string_attribute": tftypes.NewValue(tftypes.String, "test-value"), | ||
}, | ||
), | ||
) | ||
return &dv, err | ||
}, | ||
expected: false, | ||
expectedError: fmt.Errorf("unable to unmarshal DynamicValue: unknown attribute \"test_string_attribute\""), | ||
}, | ||
"String-different-value": { | ||
schemaType: tftypes.Object{ | ||
AttributeTypes: map[string]tftypes.Type{ | ||
"test_string_attribute": tftypes.String, | ||
}, | ||
}, | ||
dynamicValue1: func() (*tfprotov5.DynamicValue, error) { | ||
dv, err := tfprotov5.NewDynamicValue( | ||
tftypes.Object{ | ||
AttributeTypes: map[string]tftypes.Type{ | ||
"test_string_attribute": tftypes.String, | ||
}, | ||
}, | ||
tftypes.NewValue( | ||
tftypes.Object{ | ||
AttributeTypes: map[string]tftypes.Type{ | ||
"test_string_attribute": tftypes.String, | ||
}, | ||
}, | ||
map[string]tftypes.Value{ | ||
"test_string_attribute": tftypes.NewValue(tftypes.String, "test-value-1"), | ||
}, | ||
), | ||
) | ||
return &dv, err | ||
}, | ||
dynamicValue2: func() (*tfprotov5.DynamicValue, error) { | ||
dv, err := tfprotov5.NewDynamicValue( | ||
tftypes.Object{ | ||
AttributeTypes: map[string]tftypes.Type{ | ||
"test_string_attribute": tftypes.String, | ||
}, | ||
}, | ||
tftypes.NewValue( | ||
tftypes.Object{ | ||
AttributeTypes: map[string]tftypes.Type{ | ||
"test_string_attribute": tftypes.String, | ||
}, | ||
}, | ||
map[string]tftypes.Value{ | ||
"test_string_attribute": tftypes.NewValue(tftypes.String, "test-value-2"), | ||
}, | ||
), | ||
) | ||
return &dv, err | ||
}, | ||
expected: false, | ||
}, | ||
"String-equal-value": { | ||
schemaType: tftypes.Object{ | ||
AttributeTypes: map[string]tftypes.Type{ | ||
"test_string_attribute": tftypes.String, | ||
}, | ||
}, | ||
dynamicValue1: func() (*tfprotov5.DynamicValue, error) { | ||
dv, err := tfprotov5.NewDynamicValue( | ||
tftypes.Object{ | ||
AttributeTypes: map[string]tftypes.Type{ | ||
"test_string_attribute": tftypes.String, | ||
}, | ||
}, | ||
tftypes.NewValue( | ||
tftypes.Object{ | ||
AttributeTypes: map[string]tftypes.Type{ | ||
"test_string_attribute": tftypes.String, | ||
}, | ||
}, | ||
map[string]tftypes.Value{ | ||
"test_string_attribute": tftypes.NewValue(tftypes.String, "test-value"), | ||
}, | ||
), | ||
) | ||
return &dv, err | ||
}, | ||
dynamicValue2: func() (*tfprotov5.DynamicValue, error) { | ||
dv, err := tfprotov5.NewDynamicValue( | ||
tftypes.Object{ | ||
AttributeTypes: map[string]tftypes.Type{ | ||
"test_string_attribute": tftypes.String, | ||
}, | ||
}, | ||
tftypes.NewValue( | ||
tftypes.Object{ | ||
AttributeTypes: map[string]tftypes.Type{ | ||
"test_string_attribute": tftypes.String, | ||
}, | ||
}, | ||
map[string]tftypes.Value{ | ||
"test_string_attribute": tftypes.NewValue(tftypes.String, "test-value"), | ||
}, | ||
), | ||
) | ||
return &dv, err | ||
}, | ||
expected: true, | ||
}, | ||
} | ||
|
||
for name, testCase := range testCases { | ||
name, testCase := name, testCase | ||
|
||
t.Run(name, func(t *testing.T) { | ||
t.Parallel() | ||
|
||
dynamicValue1, err := testCase.dynamicValue1() | ||
|
||
if err != nil { | ||
t.Fatalf("unable to create first DynamicValue: %s", err) | ||
} | ||
|
||
dynamicValue2, err := testCase.dynamicValue2() | ||
|
||
if err != nil { | ||
t.Fatalf("unable to create second DynamicValue: %s", err) | ||
} | ||
|
||
got, err := dynamicValueEquals(testCase.schemaType, dynamicValue1, dynamicValue2) | ||
|
||
if err != nil { | ||
if testCase.expectedError == nil { | ||
t.Fatalf("wanted no error, got error: %s", err) | ||
} | ||
|
||
if !strings.Contains(err.Error(), testCase.expectedError.Error()) { | ||
t.Fatalf("wanted error %q, got error: %s", testCase.expectedError.Error(), err.Error()) | ||
} | ||
} | ||
|
||
if err == nil && testCase.expectedError != nil { | ||
t.Fatalf("got no error, wanted err: %s", testCase.expectedError) | ||
} | ||
|
||
if got != testCase.expected { | ||
t.Errorf("expected %t, got %t", testCase.expected, got) | ||
} | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,8 +9,9 @@ import ( | |
) | ||
|
||
// PrepareProviderConfig calls the PrepareProviderConfig method on each server | ||
// in order, passing `req`. Only one may respond with a non-nil PreparedConfig | ||
// or a non-empty Diagnostics. | ||
// in order, passing `req`. Response diagnostics are appended from all servers. | ||
// Response PreparedConfig must be equal across all servers with nil values | ||
// skipped. | ||
func (s muxServer) PrepareProviderConfig(ctx context.Context, req *tfprotov5.PrepareProviderConfigRequest) (*tfprotov5.PrepareProviderConfigResponse, error) { | ||
rpc := "PrepareProviderConfig" | ||
ctx = logging.InitContext(ctx) | ||
|
@@ -42,16 +43,22 @@ func (s muxServer) PrepareProviderConfig(ctx context.Context, req *tfprotov5.Pre | |
resp.Diagnostics = append(resp.Diagnostics, res.Diagnostics...) | ||
} | ||
|
||
if res.PreparedConfig != nil { | ||
// This could check equality to bypass the error, however | ||
// DynamicValue does not implement Equals() and previous mux server | ||
// implementations have not requested the enhancement. | ||
if resp.PreparedConfig != nil { | ||
return nil, fmt.Errorf("got a PrepareProviderConfig PreparedConfig response from multiple servers, not sure which to use") | ||
} | ||
// Do not check equality on missing PreparedConfig or unset PreparedConfig | ||
if res.PreparedConfig == nil { | ||
continue | ||
} | ||
|
||
resp.PreparedConfig = res.PreparedConfig | ||
equal, err := dynamicValueEquals(schemaType(s.providerSchema), res.PreparedConfig, resp.PreparedConfig) | ||
|
||
if err != nil { | ||
return nil, fmt.Errorf("unable to compare PrepareProviderConfig PreparedConfig responses: %w", err) | ||
} | ||
|
||
if !equal { | ||
return nil, fmt.Errorf("got different PrepareProviderConfig PreparedConfig response from multiple servers, not sure which to use") | ||
} | ||
Comment on lines
+57
to
+59
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are correct that upfront, the server creation will verify that all servers have the same schema, or otherwise return an error.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. |
||
|
||
resp.PreparedConfig = res.PreparedConfig | ||
} | ||
|
||
return resp, nil | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this we are moving from just "fail if both
res.PrepareConfig
andresp.PrepareConfig
are set" to actually checking for equality based on type.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.