`GetListLen` check is flawed for new resources

[goakley]

tl;dr the use of GetListLen makes it impossible to import resources.

setSet, setList, and setMap all have the following sort of check:

  // We must not construct new maps in the state. That would cause state drift.                                                                                             
  l, err := GetListLen(path, data)
  if err != nil {
    return nil, err
  }
  if l == 0 {                                                                                                                                                             
    return nil, nil                                                                                                                                                         
  }

Unfortunately, this check causes the import of new resources to fail. GetListLen checks the data object (*schema.ResourceData), which will always be empty/nil for a new resource. Not performing this check allows the logic to do the correct thing. I suspect that the "We must not..." comment is misinformed, but I don't completely understand what it's trying to say either.

For example, when importing a teleport_role, the logic will see that spec is nil/empty in the state (which of course it is - the state is empty), and will stop before processing the spec object that we are trying to import, causing the imported state to be empty (no spec). But removing the check will allow the code to create a slice with one element (the length of the spec object slice) and import the resource successfully.

Here is an example of importing a role before the check is removed (current master) and after it is removed: terraform.tfstate.new.txt terraform.tfstate.old.txt

[goakley]

Related to https://github.com/gravitational/teleport/issues/42451

[gzigzigzeo]

@goakley Hey! Thank you for the deep investigation. Let me elaborate on this issue a bit.

Nested objects in Terraform are represented as a single-element lists. The logic here is implemented this way because Terraform does not support computed lists and maps.

Imagine the following scenario (issue gravitational/teleport-plugins#330): a user adds SAML connector without SigningKey in the configuration. Teleport generates default SigningKey and returns it after creation. Terraform saves key pair to the state. On the next apply, this generates the state drift: Terraform will try to remove previously generated key pair from the state because it was not described in the .tf file, but would be present in the state.

This will lead to an infinite state drift: on every apply Terraform would try to remove a list element which is not specified in the config, but it won't be removed from the state because right after apply Teleport will regenerate the default key pair and it would be saved to the state again.

The obvious way to avoid this situation would be to check the .tf file itself to determine if the key pair was physically specified, or was it generated - but unfortunately Terraform SDK does not provide any tools to perform such checks.

I had to implement the workaround. ResourceData always contains object from the configuration file. If a list is empty in the config - it should stay untouched, regardless of any data received from Teleport side: this is the purpose of the check in question. I have not thought that it would fail imports. I should have implemented import tests.

This issue is solved in Terraform Framework (which, in my oppinion, was created to solve issues like this in the first place). I have the branch in this repo, which has been completely rewritten to use Terraform Framework. It looks very promising and should solve all such problems by design.

Unfortunately, Terraform Framework was not working properly with Terraform versions prior to 1.1: I experienced random crashes with 1.0.x and had to use RC for testing. That was the reason why I put this work on pause. Since 1.1 is finally out on Dec 15, I plan to switch back to that branch soon.

Meanwhile, thank you again for determination of the roots of the issue. I think, I'll find a way to implement a workaround here, while I would be working on the new version which uses Terraform Framework.

[goakley]

Thank you for the detailed explanation of the issue. I see that the way deeply-nested properties are defined in the provider is not well supported by the old hashicorp sdk. I've not For the immediate future, we can get away with a local build of the provider that works around this issue in a pretty ugly way, but an official fix for imports would be great. Currently it's just myself and another engineer importing the existing roles, but we will have multiple engineers maintaining our company's teleport deployment before long, at which point the need for importing will be small but will definitely exist.