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

Add terraform testing #8734

Merged
merged 2 commits into from
Mar 31, 2020
Merged

Add terraform testing #8734

merged 2 commits into from
Mar 31, 2020

Conversation

rifelpet
Copy link
Member

@rifelpet rifelpet commented Mar 13, 2020

This adds a new verify-terraform make target and hack script that uses the official terraform docker image to validate the .tf files in our integration tests.

This can catch issues like:

  • missing required resource properties
  • typos in resource property names
  • invalid values for resource properties

Unfortunately the test fails unless all referenced external files are present, which means we need to check in the userdata, IAM policies, and SSH public key files used by the terraform code's file() references. I decided to update the regular integration tests to also validate their contents. hack/updated-expected.sh will now also create these files.

Marking WIP because some clusters didn't have all their files created, i think it might be an issue with the expected list of files vs actual list of files. also need to get this connected to a prow job.

Feel free to review the hack script now though. I chose to use docker in order to make it work across platforms more easily. I tried using bazel but only found a terraform rules repo that is two years old so I was a bit hesitant to use it. shelling out from bazel seemed messy too.
ref: #8648

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 13, 2020
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 13, 2020
@rifelpet rifelpet force-pushed the tf-test branch 3 times, most recently from e10844b to 7df478a Compare March 15, 2020 01:34
@rifelpet
Copy link
Member Author

I think this is ready for review/merge.

There are failures with google's terraform setup. The hardcoded provider version requires terraform >= 0.12 but the remainder of the syntax requires terraform <0.12. This will be fixed as 0.12 support is added in a future PR.

#8744 will also "conflict" with this PR in the sense that tests will fail if both are merged without one of them requiring changes (I have no preference which lands first, I'll fix the other). This is because this PR adds empty bastion userdata files in order to pass the integration tests, but #8744 will assert that they don't exist.

Because of the failures, I'm making the script non-blocking: it exits 0 even in the case of failures. I'd rather get this merged early to avoid further issues with other PRs and the new data files now being checked in.

Once #8744 is merged and we either fix the google provider or rollout the 0.12 upgrade we can make the script blocking.

@rifelpet rifelpet changed the title WIP Add terraform testing Add terraform testing Mar 15, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 15, 2020
@rifelpet rifelpet mentioned this pull request Mar 26, 2020
@rifelpet
Copy link
Member Author

@mikesplain you mentioned in office hours yesterday that you use Terraform. Any chance you can take a look at this?

@mikesplain
Copy link
Contributor

Absolutely, I’ll take a look when I’m back at my machine.

Copy link
Contributor

@mikesplain mikesplain left a comment

Choose a reason for hiding this comment

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

This looks awesome. Thanks for your hard work on this, definitely lots of value here.

Lets get this in.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 30, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikesplain, rifelpet

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [mikesplain,rifelpet]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 30, 2020
@rifelpet
Copy link
Member Author

I had to add empty userdata files for bastion launch configs (and launch templates) because we still compare them and ensure they exist. I actually have #8744 open which will negate that, but that requires #8737 which is ready for review/merge whenever.

Lastly theres an issue with our golden compare and line endings. the comparison strips carriage returns before comparing:

//on windows, with git set to autocrlf, the reference files on disk have windows line endings
expected = strings.Replace(expected, "\r\n", "\n", -1)

but the additionalUserData field actually adds carriage returns:

writer.Write([]byte(fmt.Sprintf("Content-Type: multipart/mixed; boundary=\"%s\"\r\n", boundary)))
writer.Write([]byte("MIME-Version: 1.0\r\n\r\n"))

meaning that ./hack/updated-expected.sh wont update the file properly and the subsequent test will continue to fail. I'm still working on how to handle this, so i expect my most recent push to fail some tests.

In the mean time, #8737 is ready to go. If it causes merge conflicts with this or #8744 I'll rebase them asap while I try to work through this newline issue.

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. area/api area/provider/aws Issues or PRs related to aws provider labels Mar 31, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 31, 2020
@rifelpet rifelpet force-pushed the tf-test branch 2 times, most recently from efc75b5 to ba56305 Compare March 31, 2020 19:33
@hakman
Copy link
Member

hakman commented Mar 31, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 31, 2020
@k8s-ci-robot k8s-ci-robot merged commit 882d012 into kubernetes:master Mar 31, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Mar 31, 2020
@rifelpet rifelpet mentioned this pull request Apr 10, 2020
@rifelpet rifelpet deleted the tf-test branch August 6, 2020 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api area/provider/aws Issues or PRs related to aws provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants