Add `block_types` support in terraform resource schema #35

dorkamotorka · 2023-01-13T19:33:05Z

SUMMARY

If you run terraform providers schema -json each terraform resource can have either the attributes or the block_types keys. At least this is how it is for the terraform-lxd-provider. Currently this collection only handle the attributes key.
This PR adds support to read and sanitize block_types inside this collection.

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

plugins/modules/terraform.py
plugins/module_utils/models.py

ADDITIONAL INFORMATION

Consider the lxd-profile resource where e.g. the name is under attributes key, but the device is under block_types.

Example resource from terraform-lxd-provider:

resource "lxd_profile" "profile" {
  name     = "some-name"

  device {
    type = "disk"
    name = "root"

    properties = {
      pool = "default"
      path = "/"
    }
  }
}

Before applying this patch, the device attribute in the lxd-profile resource was not recognized by this collection and this ansible collection fails with an error.

dorkamotorka · 2023-01-13T19:40:15Z

This should be considered an initial draft and if someone can have a look at the proposed changes and give an opinion I'm happy to apply changes further.

dorkamotorka · 2023-01-13T19:41:56Z

plugins/module_utils/terraform_commands.py

@@ -240,6 +240,7 @@ def workspace_list(self) -> TerraformWorkspaceContext:
                continue
            elif stripped_item.startswith("* "):
                current_workspace = stripped_item.replace("* ", "")
+                all_workspaces.append(current_workspace)


This change is unrelated to the PR, but I noticed that the current_workspace doesn't get added to all_workspaces.

Thanks for catching this. It would be nice to at least have a unit test for this.

softwarefactory-project-zuul · 2023-01-13T19:47:23Z

Build failed.

✔️ build-ansible-collection SUCCESS in 4m 05s
❌ ansible-test-sanity-docker-devel FAILURE in 8m 05s (non-voting)
❌ ansible-test-sanity-docker-milestone FAILURE in 8m 00s (non-voting)
❌ ansible-test-sanity-docker-stable-2.13 FAILURE in 8m 18s
❌ ansible-test-sanity-docker-stable-2.14 FAILURE in 7m 07s
❌ ansible-test-units-cloud-terraform-python38 FAILURE in 5m 37s
❌ ansible-test-changelog FAILURE in 2m 35s
✔️ ansible-galaxy-importer SUCCESS in 4m 29s

softwarefactory-project-zuul · 2023-01-14T10:35:02Z

Build failed.

✔️ build-ansible-collection SUCCESS in 3m 25s
✔️ ansible-test-sanity-docker-devel SUCCESS in 9m 09s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 7m 18s (non-voting)
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 9m 12s
✔️ ansible-test-sanity-docker-stable-2.14 SUCCESS in 7m 27s
❌ ansible-test-units-cloud-terraform-python38 FAILURE in 6m 26s
❌ ansible-test-changelog FAILURE in 2m 25s
✔️ ansible-galaxy-importer SUCCESS in 4m 51s

softwarefactory-project-zuul · 2023-01-14T13:08:59Z

Build succeeded.

✔️ build-ansible-collection SUCCESS in 3m 43s
✔️ ansible-test-sanity-docker-devel SUCCESS in 7m 09s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 6m 51s (non-voting)
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 7m 09s
✔️ ansible-test-sanity-docker-stable-2.14 SUCCESS in 7m 20s
✔️ ansible-test-units-cloud-terraform-python38 SUCCESS in 5m 21s
✔️ ansible-test-changelog SUCCESS in 2m 10s
✔️ ansible-galaxy-importer SUCCESS in 4m 26s

dorkamotorka · 2023-01-14T13:10:39Z

I fixed the unit-tests as well but I think they're not sufficient at the moment since the chosen provider registry.terraform.io/hashicorp/local omits key block_types to test it accurately. The question for me is why in the first place this provider was chosen (I assume just randomly?) and if I should modify the unit-tests to utilize the e.g. terraform-lxd-provider or some else that also includes the block_types key?

dorkamotorka · 2023-04-11T09:33:02Z

@sstanovnik @PolonaM Any thoughts on this?

dorkamotorka · 2023-09-11T08:27:21Z

Hey, been a long time since this had any movement. Is someone willing to check this and merge, before there are many more conflict going on?

gravesm

Sorry for the delay. I'm happy to work on getting this reviewed and merged. Overall, I think this looks fine. I highlighted a bug in the existing codebase that's replicated in this PR. Though the original bug wasn't your problem, I'd prefer to not add more code that just expands the scope of that bug.

In terms of testing, I don't think the unit tests are actually running any terraform commands, so there's no real significance to using local_file other than it was easy. It would be nice to have an integration test that covers this behavior, though. The easiest way to do that would probably be to use the aws provider, because CI is already set up to run integration tests in aws. I can't think of an aws resource with a nested block marked as sensitive off the top of my head, though.

gravesm · 2023-09-11T18:27:05Z

plugins/modules/terraform.py

+    for provider_schema in schemas.provider_schemas:
+        resource_schemas = schemas.provider_schemas[provider_schema].resource_schemas
+        for resource_schema_name, resource_schema in resource_schemas.items():
+            if resource_schema_name == resource.type:


I'm looking at this and realizing there's a bug in the existing code that is replicated here. The logic in this function and in is_attribute_sensitive_in_providers_schema() don't consider which provider a resource comes from. For example, provider_a.attribute_foo would also match provider_b.attribute_foo if both providers are used in the same config. Whichever provider schema showed up first would win. This should generally be pretty rare, since most providers namespace their resource names already, but it's possible.

I'm not sure there's a need to loop through provider schemas anyways, as the resource already has the provider schema name. The same goes for resource schemas. You should be able to just do:

resource_schemas = schemas.provider_schemas[resource.provider_name].resource_schemas resource_schema = resource_schemas[resource.type]

gravesm · 2023-09-11T18:38:56Z

plugins/module_utils/terraform_commands.py

@@ -240,6 +240,7 @@ def workspace_list(self) -> TerraformWorkspaceContext:
                continue
            elif stripped_item.startswith("* "):
                current_workspace = stripped_item.replace("* ", "")
+                all_workspaces.append(current_workspace)


Thanks for catching this. It would be nice to at least have a unit test for this.

dorkamotorka · 2023-09-15T13:20:54Z

ATM I don't have the capacity to move this forward, feel free to take it over. I'll probably find some more time at the end of this year to finish this but until then..

justinc1 · 2023-11-28T06:31:00Z

I wanted to continue this PR, but looking at https://github.com/ansible-collections/cloud.terraform/blob/main/plugins/module_utils/models.py#L226 (main at 1790eb3) I see

        for block_name, block_value in json.get("block", {}).get("block_types", {}).items():

Looks like block_types is now parsed. I was also not able to crash collection using resource "lxd_profile" "profile" { ....

dorkamotorka added 3 commits January 13, 2023 13:32

feat: add block_type attributes to terraform resource sanitization

4cace0e

Merge remote-tracking branch 'upstream/main' into main

b284801

feat: add block_types attribute in resource schema

bddca22

dorkamotorka commented Jan 13, 2023

View reviewed changes

feat: black formatting

6bf5f75

dorkamotorka force-pushed the main branch from a9ee0d4 to 6bf5f75 Compare January 14, 2023 10:18

fix: unit tests

26d3eaf

dorkamotorka force-pushed the main branch from 1088837 to 26d3eaf Compare January 14, 2023 12:46

feat: add changelog

38a734c

gravesm reviewed Sep 11, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `block_types` support in terraform resource schema #35

Add `block_types` support in terraform resource schema #35

dorkamotorka commented Jan 13, 2023

dorkamotorka commented Jan 13, 2023

dorkamotorka Jan 13, 2023

gravesm Sep 11, 2023

softwarefactory-project-zuul bot commented Jan 13, 2023

softwarefactory-project-zuul bot commented Jan 14, 2023

softwarefactory-project-zuul bot commented Jan 14, 2023

dorkamotorka commented Jan 14, 2023

dorkamotorka commented Apr 11, 2023

dorkamotorka commented Sep 11, 2023

gravesm left a comment

gravesm Sep 11, 2023

gravesm Sep 11, 2023

dorkamotorka commented Sep 15, 2023

justinc1 commented Nov 28, 2023

Add block_types support in terraform resource schema #35

Are you sure you want to change the base?

Add block_types support in terraform resource schema #35

Conversation

dorkamotorka commented Jan 13, 2023

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

dorkamotorka commented Jan 13, 2023

dorkamotorka Jan 13, 2023

Choose a reason for hiding this comment

gravesm Sep 11, 2023

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Jan 13, 2023

softwarefactory-project-zuul bot commented Jan 14, 2023

softwarefactory-project-zuul bot commented Jan 14, 2023

dorkamotorka commented Jan 14, 2023

dorkamotorka commented Apr 11, 2023

dorkamotorka commented Sep 11, 2023

gravesm left a comment

Choose a reason for hiding this comment

gravesm Sep 11, 2023

Choose a reason for hiding this comment

gravesm Sep 11, 2023

Choose a reason for hiding this comment

dorkamotorka commented Sep 15, 2023

justinc1 commented Nov 28, 2023

Add `block_types` support in terraform resource schema #35

Add `block_types` support in terraform resource schema #35