Skip to content
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

Incorrect assumptions about SDK-v2 TypeMap encoding #1828

Open
t0yv0 opened this issue Apr 2, 2024 · 2 comments
Open

Incorrect assumptions about SDK-v2 TypeMap encoding #1828

t0yv0 opened this issue Apr 2, 2024 · 2 comments
Labels
kind/bug Some behavior is incorrect or out of spec

Comments

@t0yv0
Copy link
Member

t0yv0 commented Apr 2, 2024

Randomized conformance testing has led me to an important corner case for encoding object types in SDKv2 that I
previously misunderstood, it is unfortunately possible that the bridge codebase does not do the right thing here and we
need to take a look and refine the assumptions.

The key code is here: https://github.com/hashicorp/terraform-plugin-sdk/blob/main/helper/schema/core_schema_test.go#L220

Previously I assumed that to encode an object type or a single-nested TF block in schemav2 one would use
Type=schema.TypeMap and Elem=*schema.Resource{}.

However it appears that based on the evidence above, TF in this case would DISCARD the schema supplied in
&schema.Resource entirely and assume this type to be a Map[string]. The cited reason is for compatibility with TF 0.11...

It remains unclear to me how SDKv2 providers encode object types then. It seems from e.g. assumeRoleSchema() in AWS that they use Type=schema.TypeList and Elem=*schema.Resource{} and MaxItems: 1. So that MaxItems is required simply to encode object types. This may be the logical conclusion here.

In terms of the way forward:

  • Revise the documentation on schemashim.Elem()
  • Ensure that tfgen decides on Map[string] for this case
  • Ensure tfgen errors if the user supplies a discarded Resource with TypeMap
    • if the upstream provider supplies a Resource, allow the bridge author to ignore the error
    • otherwise fail hard, we should not have this
  • Scan for all runtime usage of the schema for this case
    • tests for makeTerraformInput
    • tests for makeTerraformOutput
    • fix PlanResourceChange code and type inference
    • possibly more

CC @iwahbe @VenelinMartinov

t0yv0 added a commit that referenced this issue Apr 12, 2024
Fixes #1790 by
building a rapid generator for schemas and associated values.

Large-ish problem 1: I do not have it figured out how to test unknown
values. TF literals as unknown values are forbidden and do not make
sense. We might need a helper resource so that testing unknown values
generates references to an output of the helper resource. This is logged
for future work.

Large-ish problem 2: iteration is pretty slow (x-proc). Normal n=100
rapid tests can take up to 10min. Could try batching so several
resources are tried in one shot say 100 resources.

Large-ish problem 3: I'm not sure if no-op Update and Create
implementations are acceptable. There is something to testing Computed
attributes where provider has to set values. Possibly Update also needs
to set values? Possibly not.

Small problems:

- [x] Using TF JSON syntax didn't handle null/empty correctly; that is
now discarded, using actual HCL syntax
- [x] TF representations are difficult to visualize in failing tests and
difficult to assert against
- [x] Lots of lost-in-translation papercuts possible between
representations (cty.Value, resource.PropertyValue, tftypes.Value)
- [x] this requires a change to providertest to abstract from testing.T
so we can pass rapid.T
- [x] it's very hard to disable annoying TF logging, using env vars for
now

We are starting to find bugs and discrepancies from this work:

- #1856 panic
corner-case
- #1852 need to
InternalValidate
- #1828

Future work:

- #1856 
- #1857 
- #1858 
- #1859 
- #1860 
- #1861 
- #1862 
- #1863 
- #1864 
- #1865 
- #1866 
- #1867
@iwahbe iwahbe added the kind/bug Some behavior is incorrect or out of spec label Apr 17, 2024
t0yv0 added a commit that referenced this issue Jul 16, 2024
Toward fixing #1828

Document current findings and add tests for PF representations.
t0yv0 added a commit that referenced this issue Jul 16, 2024
Toward fixing #1828

Document current findings and add tests for PF representations.
@VenelinMartinov
Copy link
Contributor

VenelinMartinov commented Jul 23, 2024

CastToTypeObject in

func CastToTypeObject(tfs shim.Schema) (shim.SchemaMap, bool) {
still suffers from this.

#2180 and #2185 look like consequensces.

Should we revisit this?

#2231 has some more tests around the PF behaviour in the shim layer and the Map type assumption for Objects is not valid everywhere.

@t0yv0
Copy link
Member Author

t0yv0 commented Jul 23, 2024

Yes, M109 perhaps. I did a light pass adjusting docu on Elem() but didn't finish auditing all places that use Elem() to make decisions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Some behavior is incorrect or out of spec
Projects
None yet
Development

No branches or pull requests

3 participants