Terraform: Add lockfile update support

[Nishnha]

Overview

Lockfiles were added to Terraform v0.14 and only track Terraform providers. The lockfile entry for a provider will contain the version, the version constraints, and the dependency hash.

This pull request adds in Dependabot updater support for lockfiles.

What it does

When a provider dependency is updated by Dependabot, we update the mainfest vile (main.tf or versions.tf) dependency version and update the lockfile entry.

The implementation for terraform lockfile updates is based on bundler lockfile updates.

Known issues

** These will be addressed in future PRs **

• Lockfiles will not update if the provider dependency version is pinned.
  • This is because lockfile entries are updated independently of the manifest file. When the lockfile update it is not aware of the version change in the manifest file.
• Rspec tests for the lockfile currently match against the latest at the time dependency version. If there is a new version for a dependency after the test was updated, the test will fail.
  • Fix might require stubbing the network call for terraform providers lock

Testing the Pull Request

Currrently testing against Terraform rspec tests and 3 GitHub internal repositories. From inside the docker-dev-shell with LOCAL_GITHUB_ACCESS_TOKEN set:

cd terraform && rspec spec

bin/dry-run.rb terraform dsp-testing/dependabot-terraform-v0.15-test --cache=files --dir="/." 

bin/dry-run.rb terraform dsp-testing/dependabot-terraform-lockfile-test --cache=files --dir="/."

bin/dry-run.rb terraform dsp-testing/dependabot-terraform-lockfile-test --cache=files --dir="/." --branch=pinned-versions

Update log

• Add lockfiles as a dependency source
• Update lockfile dependencies individually via providers lock update
• Refactor: remove lockfile dependencies, update lockfile when provider dependency matches lockfile entry.
• Fix: lockfile detected changes when dependency hash opportunistically updated. Only update when lockfile version changes.
• Delete "dead" commits from PR
• Rebase onto main
• Memoize network calls from terraform provider lock
• Update lockfile when updating from a pinned version
• Pin rspec tests dependency versions! (stub network call?)
• Merge PR, cut release

[feelepxyz]

I wonder if we should refactor these methods to return the updated content instead of mutating the content argument, this might make it easier to reason about update_lockfile_declaration, e.g.

when "provider"
  content = update_registry_declaration(new_req, old_req, content)
when "lockfile"
  content = update_lockfile_declaration(new_req, old_req, content)
...

[jurre]

I think we can get rid of this as these files are already parsed and pull out the resolved version (the only thing we care about in this step) here. That means we can also get rid of build_lockfile_dependency. It'd be sweet to keep def lock_file in the file fetcher, so we'd get rid of that method in the file_parser.

[Nishnha]

Initially the lockfile didn't build dependencies, but then FileUpdater would skip over the lockfile since no dependencies changed:

[*terraform_files, *terragrunt_files, *lock_file].each do |file|
      next unless file_changed?(file)

Where file_changed? calls the FileUpdaters/base#requirements_changed? function on each dependency of the file

def requirement_changed?(file, dependency)
    changed_requirements = dependency.requirements - dependency.previous_requirements
    changed_requirements.any? { |f| f[:file] == file.name }
end

If we get rid of lockfile dependencies we would need to either remove the file_changed?(file) check and fix the test suite or create exceptions for when the file being updated is a lockfile.

[jurre]

I would not expect any of the dependency_files to be this directory, when does this happen?

[Nishnha]

Dependency files are copied to a temporary directory to provide context for the terraform providers lock command. The files are copied over in the FileUpdater#update_lockfile_declaration function but I'm not sure when the dependency_files array itself is populated.

[jurre]

It's what we grab in the FileFetcher, I don't think we ever pull in those files so we probably don't need to worry about it?

[feelepxyz]

Nice cleanup! I'm still wondering if this should return the content instead of the updated_file(file..)? That way you could only append it to updated_files if the content has changed.

Something like

updated_lockfile_content = update_lockfile_declaration
if (updated_lockfile_content && lock_file.content != update_lockfile_content)
  updated_files << updated_file(file: lock_file, content: update_lockfile_content)
end

[feelepxyz]

It would be good to add a test case to make sure updated_dependency_files doesn't return a lockfile if it doesn't change.

[Nishnha]

Implemented @feelepxyz's suggestion and created a test/fixture to make sure updated_dependency_files doesn't return a lockfile if it doesn't change.

The test uses a terraform project fixture that is fully up-to-date. However, since no files change after running dependabot on the fixture, it raises a "No files changed!" error.

The error makes the function quit early and prevents the test from directly checking whether the lockfile is unmodified or appended to the updated_files array.

So instead, I am testing for the error, which indicates that the lockfile didn't change.

[feelepxyz]

This is looking very close to done! Only have one comment remaining about checking the lockfile content has changed 🙌

[feelepxyz]

🎉 Thanks for incorporating the feedback and for adding the test case! This looks good to go to me

[jurre]

Should we pin the version here?

[jurre]

Meh, whatevs let's do it in a follow-up PR

[Nishnha]

Pinned to 1.0.0 81a424e