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(misconf): fix caching of modules in subdirectories #6814

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

nikpivkin
Copy link
Contributor

@nikpivkin nikpivkin commented May 29, 2024

Description

Modules are saved and retrieved from the cache by a key, which is built from the module source and version. But when caching a module from a package subdirectory, the relative path of that module is added to the key, which is not taken into account when loading the module from the cache. This leads to two problems:

  • The cache is bloated because a separate directory is created for each module in the package that contains the entire package. For example, two directories would be created for terraform-aws-modules/s3-bucket/aws//modules/object and terraform-aws-modules/s3-bucket/aws//modules/notification, each containing a terraform-aws-modules/s3-bucket/aws module.
  • Trivy does not load modules from the cache that are in subdirectories, such as terraform-aws-modules/s3-bucket/aws//modules/object.

This PR removes subdirectories from the module source when creating a key, since the package is cached completely.

Doc:

Terraform will still extract the entire package to local disk, but will read the module from the subdirectory. As a result, it is safe for a module in a sub-directory of a package to use a local path to another module as long as it is in the same package.

Steps to reproduce:

❯ rm -rf $TMPDIR/.aqua
❯ cat main.tf
module "object" {
  source = "terraform-aws-modules/s3-bucket/aws//modules/object"
  version = "4.1.2"
}

module "notification" {
  source = "terraform-aws-modules/s3-bucket/aws//modules/notification"
  version = "4.1.2"
}% 
❯ trivy conf -d .
...
2024-05-29T18:41:04+07:00       DEBUG    [misconf] 41:04.727053000 terraform.parser.<root>.evaluator.resolver Downloading git::https://github.com/terraform-aws-modules/terraform-aws-s3-bucket?ref=8a0b697adfbc673e6135c70246cff7f8052ad95a...
2024-05-29T18:41:06+07:00       DEBUG    [misconf] 41:06.590482000 terraform.parser.<root>.evaluator.resolver Incrementing the download counter
2024-05-29T18:41:06+07:00       DEBUG    [misconf] 41:06.590545000 terraform.parser.<root>.evaluator.resolver Download counter is now 1
2024-05-29T18:41:06+07:00       DEBUG    [misconf] 41:06.590557000 terraform.parser.<root>.evaluator.resolver Successfully downloaded module.notification from git::https://github.com/terraform-aws-modules/terraform-aws-s3-bucket?ref=8a0b697adfbc673e6135c70246cff7f8052ad95a
...
2024-05-29T18:41:06+07:00       DEBUG    [misconf] 41:06.815826000 terraform.parser.<root>.evaluator.resolver Downloading git::https://github.com/terraform-aws-modules/terraform-aws-s3-bucket?ref=8a0b697adfbc673e6135c70246cff7f8052ad95a...
2024-05-29T18:41:08+07:00       DEBUG    [misconf] 41:08.477748000 terraform.parser.<root>.evaluator.resolver Incrementing the download counter
2024-05-29T18:41:08+07:00       DEBUG    [misconf] 41:08.477798000 terraform.parser.<root>.evaluator.resolver Download counter is now 2
2024-05-29T18:41:08+07:00       DEBUG    [misconf] 41:08.477811000 terraform.parser.<root>.evaluator.resolver Successfully downloaded module.object from git::https://github.com/terraform-aws-modules/terraform-aws-s3-bucket?ref=8a0b697adfbc673e6135c70246cff7f8052ad95a
...
❯ ll $TMPDIR/.aqua/cache/
total 0
drwxr-xr-x  19 tososomaru  staff   608B 29 май 18:41 979bacd596feccb476850d8603ea9fdb
drwxr-xr-x  19 tososomaru  staff   608B 29 май 18:41 b61a3243ae53f2bd060603f7a03bb76d
❯ cat $TMPDIR/.aqua/cache/979bacd596feccb476850d8603ea9fdb/README.md |grep "AWS S3 bucket Terraform module"
# AWS S3 bucket Terraform module
❯ cat $TMPDIR/.aqua/cache/b61a3243ae53f2bd060603f7a03bb76d/README.md |grep "AWS S3 bucket Terraform module"
# AWS S3 bucket Terraform module

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

@nikpivkin nikpivkin marked this pull request as ready for review June 3, 2024 06:58
@nikpivkin nikpivkin requested a review from simar7 as a code owner June 3, 2024 06:58
Comment on lines +71 to +72
hash := md5.Sum([]byte(source + ":" + version)) // #nosec
return hex.EncodeToString(hash[:])
Copy link
Member

Choose a reason for hiding this comment

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

do we need to keep hash? Can we inline it?

Suggested change
hash := md5.Sum([]byte(source + ":" + version)) // #nosec
return hex.EncodeToString(hash[:])
return hex.EncodeToString(md5.Sum([]byte(source + ":" + version))[:]) // #nosec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

md5.Sum returns an array of bytes, hex.EncodeToString takes a slice of bytes. In order to convert an array to a slice, it must be addressable, so an intermediate variable is needed.

Copy link
Member

@simar7 simar7 left a comment

Choose a reason for hiding this comment

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

one small comment, but lgtm!

@nikpivkin nikpivkin requested a review from simar7 June 4, 2024 05:47
@simar7 simar7 added this pull request to the merge queue Jun 5, 2024
Merged via the queue into aquasecurity:main with commit 0bcfedb Jun 5, 2024
12 checks passed
@aqua-bot aqua-bot mentioned this pull request Jun 5, 2024
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