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

fix: phantom attributes on multi-resources (CLOUD-1843) #279

Merged
merged 1 commit into from
Nov 23, 2023

Conversation

jaspervdj-snyk
Copy link
Collaborator

We were failing to resolve the
bucket = aws_s3_bucket.nativebucket[0].id attribute in the following snippet:

resource "aws_s3_bucket" "nativebucket" {
    count = 1
    bucket = "test"
}

resource "aws_s3_bucket_versioning" "nativebucket" {
    count = 1
    bucket = aws_s3_bucket.nativebucket[0].id
    versioning_configuration {
      status = "Enabled"
    }
}

This is problematic since it is extremely common to use count = var.create ? 1 : 0 in terraform modules.

This is fixed by a number of changes:

  1. We currently reference attributes by LocalName, which is a list of strings. This cannot represent an accessor that also has numbers in it, like aws_s3_bucket.nativebucket[0].id.

    A new accessor is added to take care of this case, including conversion functions to and from LocalName.

  2. We take care to keep the trailing part of the accessors around in any conversion functions; so that aws_s3_bucket.nativebucket[0].id gets split into aws_s3_bucket.nativebucket and [0].id.

  3. We refactor the phantomAttrs type so it receives a way to tell whether or not a resource is a multi-resource (rather than passing this in through an argument in phantomAttrs.add()).

  4. Finally, now that we have all the machinery from 1-3 available, we can adjust phantomAttrs to remove one element from the trailing accessors when applicable.

I added this snippet as a golden test, and included one for the for_each version as well.

We were failing to resolve the
`bucket = aws_s3_bucket.nativebucket[0].id` attribute in the following snippet:

    resource "aws_s3_bucket" "nativebucket" {
        count = 1
        bucket = "test"
    }

    resource "aws_s3_bucket_versioning" "nativebucket" {
        count = 1
        bucket = aws_s3_bucket.nativebucket[0].id
        versioning_configuration {
          status = "Enabled"
        }
    }

This is problematic since it is extremely common to use
`count = var.create ? 1 : 0` in terraform modules.

This is fixed by a number of changes:

1.  We currently reference attributes by `LocalName`, which is a list of
    strings.  This cannot represent an accessor that also has numbers in it,
    like `aws_s3_bucket.nativebucket[0].id`.

    A new `accessor` is added to take care of this case, including conversion
    functions to and from `LocalName`.

2.  We take care to keep the trailing part of the `accessor`s around in any
    conversion functions; so that
    `aws_s3_bucket.nativebucket[0].id` gets split into
    `aws_s3_bucket.nativebucket` and `[0].id`.

3.  We refactor the `phantomAttrs` type so it receives a way to tell whether
    or not a resource is a multi-resource (rather than passing this in through
    an argument in `phantomAttrs.add()`).

4.  Finally, now that we have all the machinery from 1-3 available, we can
    adjust `phantomAttrs` to remove one element from the trailing accessors
    when applicable.

I added this snippet as a golden test, and included one for the `for_each`
version as well.
@jaspervdj-snyk jaspervdj-snyk requested a review from a team as a code owner November 22, 2023 12:48
@jaspervdj-snyk jaspervdj-snyk requested review from a user and removed request for a team November 22, 2023 12:48
@craigfurman craigfurman self-assigned this Nov 23, 2023
Copy link
Contributor

@craigfurman craigfurman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I run the golden tests from this branch against the code from main, it neatly illustrates the issue this pr solves:

❯ go test ./pkg/input
--- FAIL: TestGolden (0.05s)
    --- FAIL: TestGolden/golden_test/tf/count-ref-02 (0.00s)
        utils.go:51:
                Error Trace:    /Users/craig/workspace/github.com/snyk/policy-engine/pkg/input/utils.go:51
                                                        /Users/craig/workspace/github.com/snyk/policy-engine/pkg/input/golden_test.go:97
                Error:          Not equal:
                                expected: "{\n  \"format\": \"\",\n  \"format_version\": \"\",\n  \"input_type\": \"tf_hcl\",\n  \"environment_provider\": \"iac\",\n  \"meta\": {\n    \"filepath\": \"golden_test/tf/count-ref-02/main.tf\"\n  },\n  \"resources\": {\n    \"aws_s3_bucket\": {\n      \"aws_s3_bucket.febucket[one]\": {\n        \"id\": \"aws_s3_bucket.febucket[one]\",\n        \"resource_type\": \"aws_s3_bucket\",\n        \"namespace\": \"golden_test/tf/count-ref-02/main.tf\",\n        \"meta\": {},\n        \"attributes\": {\n          \"bucket\": \"test\"\n        }\n      },\n      \"aws_s3_bucket.nativebucket[0]\": {\n        \"id\": \"aws_s3_bucket.nativebucket[0]\",\n        \"resource_type\": \"aws_s3_bucket\",\n        \"namespace\": \"golden_test/tf/count-ref-02/main.tf\",\n        \"meta\": {},\n        \"attributes\": {\n          \"bucket\": \"test\"\n        }\n      }\n    },\n    \"aws_s3_bucket_versioning\": {\n      \"aws_s3_bucket_versioning.febucket[one]\": {\n        \"id\": \"aws_s3_bucket_versioning.febucket[one]\",\n        \"resource_type\": \"aws_s3_bucket_versioning\",\n        \"namespace\": \"golden_test/tf/count-ref-02/main.tf\",\n        \"meta\": {},\n        \"attributes\": {\n          \"bucket\": \"aws_s3_bucket.febucket[one]\",\n          \"versioning_configuration\": [\n            {\n              \"status\": \"Enabled\"\n            }\n          ]\n        }\n      },\n      \"aws_s3_bucket_versioning.nativebucket[0]\": {\n        \"id\": \"aws_s3_bucket_versioning.nativebucket[0]\",\n        \"resource_type\": \"aws_s3_bucket_versioning\",\n        \"namespace\": \"golden_test/tf/count-ref-02/main.tf\",\n        \"meta\": {},\n        \"attributes\": {\n          \"bucket\": \"aws_s3_bucket.nativebucket[0]\",\n          \"versioning_configuration\": [\n            {\n              \"status\": \"Enabled\"\n            }\n          ]\n        }\n      }\n    }\n  },\n  \"scope\": {\n    \"filepath\": \"golden_test/tf/count-ref-02/main.tf\"\n  }\n}"
                                actual  : "{\n  \"format\": \"\",\n  \"format_version\": \"\",\n  \"input_type\": \"tf_hcl\",\n  \"environment_provider\": \"iac\",\n  \"meta\": {\n    \"filepath\": \"golden_test/tf/count-ref-02/main.tf\"\n  },\n  \"resources\": {\n    \"aws_s3_bucket\": {\n      \"aws_s3_bucket.febucket[one]\": {\n        \"id\": \"aws_s3_bucket.febucket[one]\",\n        \"resource_type\": \"aws_s3_bucket\",\n        \"namespace\": \"golden_test/tf/count-ref-02/main.tf\",\n        \"meta\": {},\n        \"attributes\": {\n          \"bucket\": \"test\"\n        }\n      },\n      \"aws_s3_bucket.nativebucket[0]\": {\n        \"id\": \"aws_s3_bucket.nativebucket[0]\",\n        \"resource_type\": \"aws_s3_bucket\",\n        \"namespace\": \"golden_test/tf/count-ref-02/main.tf\",\n        \"meta\": {},\n        \"attributes\": {\n          \"bucket\": \"test\"\n        }\n      }\n    },\n    \"aws_s3_bucket_versioning\": {\n      \"aws_s3_bucket_versioning.febucket[one]\": {\n        \"id\": \"aws_s3_bucket_versioning.febucket[one]\",\n        \"resource_type\": \"aws_s3_bucket_versioning\",\n        \"namespace\": \"golden_test/tf/count-ref-02/main.tf\",\n        \"meta\": {},\n        \"attributes\": {\n          \"bucket\": null,\n          \"versioning_configuration\": [\n            {\n              \"status\": \"Enabled\"\n            }\n          ]\n        }\n      },\n      \"aws_s3_bucket_versioning.nativebucket[0]\": {\n        \"id\": \"aws_s3_bucket_versioning.nativebucket[0]\",\n        \"resource_type\": \"aws_s3_bucket_versioning\",\n        \"namespace\": \"golden_test/tf/count-ref-02/main.tf\",\n        \"meta\": {},\n        \"attributes\": {\n          \"bucket\": null,\n          \"versioning_configuration\": [\n            {\n              \"status\": \"Enabled\"\n            }\n          ]\n        }\n      }\n    }\n  },\n  \"scope\": {\n    \"filepath\": \"golden_test/tf/count-ref-02/main.tf\"\n  }\n}"

                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -36,3 +36,3 @@
                                         "attributes": {
                                -          "bucket": "aws_s3_bucket.febucket[one]",
                                +          "bucket": null,
                                           "versioning_configuration": [
                                @@ -50,3 +50,3 @@
                                         "attributes": {
                                -          "bucket": "aws_s3_bucket.nativebucket[0]",
                                +          "bucket": null,
                                           "versioning_configuration": [
                Test:           TestGolden/golden_test/tf/count-ref-02
FAIL
FAIL    github.com/snyk/policy-engine/pkg/input 0.857s
FAIL

Thanks for the detailed explanation too. I don't claim to totally grasp this line-by-line (don't get me wrong, I read it, but glossed over many functions as grey boxes) but I get the gist of what it's doing, and the golden tests prove this works and hasn't broken anything.

@jaspervdj-snyk jaspervdj-snyk merged commit fb2c79e into main Nov 23, 2023
12 checks passed
@jaspervdj-snyk jaspervdj-snyk deleted the fix/CLOUD-1843/count-ref branch November 23, 2023 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants