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

feat: TF Language Support - dereference variables in CWD while parsing [CFG-1496] #2752

Merged
merged 1 commit into from
Feb 16, 2022

Conversation

teodora-sandu
Copy link
Contributor

@teodora-sandu teodora-sandu commented Feb 11, 2022

What does this PR do?

This PR introduces a new parsing flow for Terraform files, which supports dereferencing variables. This flow is behind the iacTerraformVarSupport feature flag and currently only supports scanning Terraform files in the current directory.

Scanning nested directories is out of scope for this PR.

Where should the reviewer start?

The changes are split in commits:

  1. the changes in release-scripts]/ are to update the version of the snyk/snyk-iac-parsers library to the latest, https://github.com/snyk/snyk-iac-parsers/releases/tag/v0.2.0
  2. the acceptance tests verify backwards compatibility, optional flag support, and dereferencing
  3. the parseTerraformFiles function will take in the Terraform files, send them to the snyk-iac-parsers library, and modify them into the desired output
  4. the main flow is split into two phases, depending on whether the iacTerraformVarSupport feature flag is enabled or not
    • in src/cli/commands/test/iac-local-execution/file-loader.ts we modify the directory reader to take in an optional validFileTypes, which specifies what kind of files are valid
    • this will be used to separate Terraform files from all other files when the iacTerraformVarSupport feature flag is enabled, thus allowing us to parse Terraform files separately with our parseTerraformFiles function

How should this be manually tested?

  1. npm run build
  2. Clone https://github.com/snyk-labs/infrastructure-as-code-goof
  3. Run snykiac test variables/
  4. Run snyk-dev iac test variables/ and see that there are extra misconfigurations

Any background context you want to provide?

This PR is the first in a line of PRs to enhance Terraform Language Support, as part of this feature. We have delivered a new Terraform parser in https://github.com/snyk/snyk-iac-parsers, which supports extracting variables and dereferencing them inline wherever they're references. The files referencing these variables get converted into JSON, which is used to run our Rego rules (internal or custom) and discover misconfigurations.

What are the relevant tickets?

https://snyksec.atlassian.net/browse/CFG-1498

Screenshots

Before:
Screenshot 2022-02-14 at 12 14 49

After:
Screenshot 2022-02-14 at 12 15 06

@teodora-sandu teodora-sandu requested review from a team, wbeuil, gitphill, jk05 and mika-bar and removed request for a team February 11, 2022 19:41
@teodora-sandu teodora-sandu marked this pull request as draft February 11, 2022 19:41
@teodora-sandu teodora-sandu requested review from wbeuil, gitphill, jk05 and mika-bar and removed request for a team, wbeuil, gitphill, jk05 and mika-bar February 11, 2022 19:42
@teodora-sandu teodora-sandu force-pushed the feat/tf-vars branch 3 times, most recently from 6d46867 to b8c56a4 Compare February 14, 2022 12:29
@teodora-sandu teodora-sandu force-pushed the feat/tf-vars branch 2 times, most recently from 8eff457 to 25dde23 Compare February 14, 2022 12:31
@teodora-sandu teodora-sandu changed the title feat: TF Language Support - dereference variables feat: TF Language Support - dereference variables in CWD while parsing Feb 15, 2022
@teodora-sandu teodora-sandu changed the title feat: TF Language Support - dereference variables in CWD while parsing feat: TF Language Support - dereference variables in CWD while parsing [CFG-1496] Feb 15, 2022
@teodora-sandu teodora-sandu force-pushed the feat/tf-vars branch 2 times, most recently from fd25561 to 8ed7675 Compare February 15, 2022 13:39
@teodora-sandu
Copy link
Contributor Author

I can't reply to your comment but in regards to

just a thought, do you think there might be a chance where we can have duplicate Files in here when we merge them? I am thinking scenarios that might have duplicate files per environment and not using "modules".
Should we worry about this at all?

I'm guessing you mean we'll have two folders, prod and dev, both with a cluster.tf file for example, and we will have to merge their results. I think at that point the files will actually have the file path prod/cluster.tf and dev/cluster.tf, so I don't think there's any chance of running into problems. Did you have a different scenario in mind @ipapast ?

@ipapast
Copy link
Contributor

ipapast commented Feb 15, 2022

I can't reply to your comment but in regards to

just a thought, do you think there might be a chance where we can have duplicate Files in here when we merge them? I am thinking scenarios that might have duplicate files per environment and not using "modules".
Should we worry about this at all?

I'm guessing you mean we'll have two folders, prod and dev, both with a cluster.tf file for example, and we will have to merge their results. I think at that point the files will actually have the file path prod/cluster.tf and dev/cluster.tf, so I don't think there's any chance of running into problems. Did you have a different scenario in mind @ipapast ?

yes, I was thinking something like that, but you're right the paths should be different already.

Copy link
Contributor

@ipapast ipapast left a comment

Choose a reason for hiding this comment

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

Looks great, I unfortunately can't run my CLI locally, but your screenshots show it's working.

If you could fix the acceptance tests to validate that and I'll approve when we get back in the morning. 🙌

The parseTerraformFiles function could also be covered with a unit test as it's exported and has some if flows.

.eslintignore Outdated Show resolved Hide resolved
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.

4 participants