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

Automate Terraform Platform Detection for Lockfile Hashes #4905

Merged
merged 12 commits into from Apr 19, 2022

Conversation

Nishnha
Copy link
Member

@Nishnha Nishnha commented Mar 24, 2022

Terraform supports a few different architectures and, depending on the one(s) being used in the project, will save a hash for each in the .terraform.lock.hcl lockfile.

Previously, Dependabot would use the default architecture linux_amd64 for every Terraform update, which caused issues for users who were not using that platform (see #4784).

This PR automates Terraform architecture detection by comparing the existing lockfile hashes against the possible platforms. Once the architecture(s) are determined, Dependabot will update the hashes for the actual architectures in that project to the new version.

The architectures that we check are (in order):

  • linux_amd64
  • darwin_amd64
  • windows_amd64
  • darwin_arm64
  • linux_arm64

@Nishnha Nishnha changed the title Automate Terraform Platform Detection for Lockfile ashes Automate Terraform Platform Detection for Lockfile Hashes Mar 24, 2022
Change the order in which we look for platforms to be the most common to the least common

Optimized regex to grab the h1 hashes only
Currently failing test since we exit early before we do the comparisons...
The h1 provider hashes in a lockfile can be used to look up architecure the
provider is built for.

We do this by running `terraform provider lock` with a `-platform=`
which could be linux_amd64, darin_amd64, etc.
We compare the hash we get from those platforms to the one we see in the
original lockfile. If they match then we know the user who checked in
this lockfile was using that architecture.

We stop chekcing platforms early if we determine that all of the h1 hashes in
the original lockfile are accounted for.

When we go to actually update the lockfile, we can download the provider
hashes for all the necessary architectures in one go by chaining
multiple `-platform=` flags.

We also sort the hashes in an order that we assume is the most to least
common.
@Nishnha Nishnha marked this pull request as ready for review April 15, 2022 05:56
@Nishnha Nishnha requested a review from a team as a code owner April 15, 2022 05:56
Disabled Metrics/PerceivedComplexity for lookup_hash_architecture and update_lockfile_declaration
@Nishnha Nishnha requested review from jurre and pavera April 15, 2022 22:26
Copy link
Contributor

@pavera pavera left a comment

Choose a reason for hiding this comment

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

I tried to think through any way to further optimize this, but I think this is as good as it gets with the tools TF currently provides.

@Nishnha Nishnha self-assigned this Apr 19, 2022
[content, provider_source, declaration_regex]
end

def lookup_hash_architecture # rubocop:disable Metrics/AbcSize, Metrics/MethodLength, Metrics/PerceivedComplexity
Copy link
Member

Choose a reason for hiding this comment

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

Whew that's a big one 😄 are there maybe any methods that we can extract out of here to make this a little easier to grok? Happy to take a stab at it (tomorrow) also

Copy link
Member

@jurre jurre left a comment

Choose a reason for hiding this comment

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

Overall the change looks good to me, I think it might be worth trying to extract some logic from #lookup_hash_architecture, rubocop also complains about this and we need to disable 3 linting rules in order to satisfy it, but don't want to block on it. We can either tackle it in this PR or follow up later, lmk if I can help with this!

@Nishnha
Copy link
Member Author

Nishnha commented Apr 19, 2022

Overall the change looks good to me, I think it might be worth trying to extract some logic from #lookup_hash_architecture, rubocop also complains about this and we need to disable 3 linting rules in order to satisfy it, but don't want to block on it. We can either tackle it in this PR or follow up later, lmk if I can help with this!

Thanks for the review! 🎉

I think it would be best for the community to get this fix merged in since it seems better than the status quo described in #4784

I decided to open #5013 as a refactor pass on this PR. Maybe we can pair up on it together sometime this week?

@Nishnha Nishnha merged commit c30e1e7 into main Apr 19, 2022
@Nishnha Nishnha deleted the nishnha/automate-terraform-hashes branch April 19, 2022 20:48
@Nishnha Nishnha mentioned this pull request Apr 19, 2022
@Nishnha
Copy link
Member Author

Nishnha commented Apr 21, 2022

This PR was shipped in #5014

@billinghamj
Copy link

This has instantly removed the need for the action, and made it immediately do exactly the right thing for virtually everyone using Terraform - thank you for getting this sorted :)

@billinghamj
Copy link

Just to gently flag, it seems like this has stopped working... not quite sure why

Screenshot 2022-05-03 at 19 07 36

@Nishnha
Copy link
Member Author

Nishnha commented May 10, 2022

We had a brief period where we had to roll back this PR.
Commenting @dependabot recreate on a Terraform PR that showed the old behavior will recreate it with the updated behavior.

Sorry about that!

@Flydiverny
Copy link
Contributor

Thank you very much for this! It enabled us to finally start using dependabot for our terraform dependencies ❤️
We are in some cases still seeing PRs removing hashes, I've opened a related issue here #5225 with a reproduction repo, recreating does not solve it :(

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.

None yet

5 participants