Skip to content

vendor all the required terraform plugins #919

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.

Sign up for GitHub

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

Jump to bottom
Merged
openshift-merge-robot merged 13 commits into from
Dec 19, 2018
Merged

vendor all the required terraform plugins #919

openshift-merge-robot merged 13 commits into from
Dec 19, 2018

Conversation

@abhinavdahiya
Copy link
Contributor

@abhinavdahiya abhinavdahiya commented Dec 15, 2018

@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 15, 2018
@wking
Copy link
Member

wking commented Dec 15, 2018

I'm just starting into this, and we can certainly punt to future work, but it would be great if we could put libvirt support behind a build tag so we could compile releases without it.

wking
wking reviewed Dec 15, 2018
View reviewed changes
pkg/terraform/exec/plugins/libvirt.go Outdated
Copy link
Member

@wking wking Dec 15, 2018

Choose a reason for hiding this comment

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

Ah, love it 👍

wking
wking reviewed Dec 15, 2018
View reviewed changes
wking
wking reviewed Dec 15, 2018
View reviewed changes
wking
wking reviewed Dec 15, 2018
View reviewed changes
crawford
crawford reviewed Dec 15, 2018
View reviewed changes
pkg/terraform/terraform.go Outdated
Copy link
Contributor

@crawford crawford Dec 15, 2018

Choose a reason for hiding this comment

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

NIT: Can we move this to trace?

Copy link
Member

@wking wking Dec 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I'm fine with debug or trace, but if we go with trace we should list it here.

wking
wking reviewed Dec 15, 2018
View reviewed changes
crawford
crawford reviewed Dec 15, 2018
View reviewed changes
@crawford
Copy link
Contributor

crawford commented Dec 15, 2018

Approach looks solid. Just a few NITs.

@abhinavdahiya
Copy link
Contributor Author

abhinavdahiya commented Dec 15, 2018 

time="2018-12-15T01:49:13Z" level=debug msg="Initializing provider plugins..."
time="2018-12-15T01:49:13Z" level=debug msg="- Checking for available provider plugins on https://releases.hashicorp.com..."
time="2018-12-15T01:49:13Z" level=debug msg="- Downloading plugin for provider \"aws\" (1.39.0)..."
time="2018-12-15T01:49:15Z" level=debug
time="2018-12-15T01:49:15Z" level=debug msg="Terraform has been successfully initialized!"

Saw this on CI, need to see why it is downloading AWS plugin. Maybe because we set AWS provider version to 1.39.0 and all local plugins get version 0.0.0 ...

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 15, 2018
@wking
Copy link
Member

wking commented Dec 15, 2018

Maybe because we set AWS provider version to 1.39.0 and all local plugins get version 0.0.0...

Yeah we should probably drop versions from the Terraform.

wking
wking reviewed Dec 15, 2018
View reviewed changes
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2018
@abhinavdahiya
terraform/exec: add plugins dir in the terraform workspace to list …
7b1b3dc 
…of plugin dir

so terraform now looks for plugins in
- `.`
- `binary location`
- `$HOME/terraform.d/plugins`
- `$HOME/terraform.d/plugins/GOOS_GOARCH`
- `<tmp terraform workspace>/plugins`
@abhinavdahiya
terraform/plugins: add libvirt, aws, ignition, openstack to K…
3fb4400 
…nownPlugins

`KnownPlugins` maps the plugin name (`terraform-provider-<name>`) to a function that runs the plugin
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2018
@abhinavdahiya abhinavdahiya force-pushed the terraform_vendor branch 2 times, most recently from 6417b29 to 7a3f625 Compare December 17, 2018 18:48
@abhinavdahiya
Copy link
Contributor Author

abhinavdahiya commented Dec 17, 2018

So from the ci

level=debug msg="Initializing modules..."
level=debug msg="- module.bootstrap"
level=debug msg="  Getting source \"./bootstrap\""
level=debug msg="- module.masters"
level=debug msg="  Getting source \"./master\""
level=debug msg="- module.iam"
level=debug msg="  Getting source \"./iam\""
level=debug msg="- module.dns"
level=debug msg="  Getting source \"./route53\""
level=debug msg="- module.vpc"
level=debug msg="  Getting source \"./vpc\""
level=debug
level=debug msg="Initializing provider plugins..."

terraform is not downloading aws plugin.

failure https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/openshift_installer/919/pull-ci-openshift-installer-master-e2e-aws/2365

level=debug msg="module.dns.aws_route53_record.api_internal: Creation complete after 6m47s (ID: Z1JMC5Q1D03G57_ci-op-ti5ttwsz-1d3f3-api.origin-ci-int-aws.dev.rhcloud.com_A)"
level=debug msg="module.dns.aws_route53_record.api_external: Creation complete after 6m49s (ID: Z328TAU66YDJH7_ci-op-ti5ttwsz-1d3f3-api.origin-ci-int-aws.dev.rhcloud.com_A)"
level=error
level=error msg="Error: Error applying plan:"
level=error
level=error msg="1 error occurred:"
level=error msg="\t* module.vpc.aws_vpc_endpoint.s3: 1 error occurred:"
level=error msg="\t* aws_vpc_endpoint.s3: Error creating VPC Endpoint: VpcEndpointLimitExceeded: The maximum number of VPC endpoints has been reached."
level=error msg="\tstatus code: 400, request id: 4edeb332-2df1-4648-b4de-7093e9cedb5c"
level=error
level=error
level=error
level=error
level=error
level=error msg="Terraform does not automatically rollback in the face of errors."
level=error msg="Instead, your Terraform state file has been partially updated with"
level=error msg="any resources that successfully completed. Please address the error"
level=error msg="above and apply again to incrementally change your infrastructure."
level=error
level=error
level=fatal msg="failed to fetch Cluster: failed to generate asset \"Cluster\": failed to create cluster: failed to apply using Terraform"

^^ @wking

@abhinavdahiya
Copy link
Contributor Author

abhinavdahiya commented Dec 17, 2018

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 17, 2018
@abhinavdahiya
Copy link
Contributor Author

abhinavdahiya commented Dec 17, 2018

/retest

@abhinavdahiya
Copy link
Contributor Author

abhinavdahiya commented Dec 18, 2018 •
edited
Loading

@crawford @wking the tests are green https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/openshift_installer/919/pull-ci-openshift-installer-master-e2e-aws/2373

[EDIT]: Oops has to push the commits that change docs for libvirt terraform plugins

wking
wking reviewed Dec 18, 2018
View reviewed changes
@abhinavdahiya
terraform: create symlinks so that terraform can exec embedded plugins
8f49463 
we create symlinks to `<tmp terraform workspace>/plugins` such that terraform calls the installer binary
with the `terraform-provider-<plugin name>` as executable name
@abhinavdahiya
cmd: exec to terraform plugins' run functions when os.Args[0] is se…
10b4cc7 
…t to plugins' name

If the os.Args[0] has the terraform plugin's name, we run the corresponding plugin's exec function.

By default we always run installer's main.
@abhinavdahiya
terraform/plugins: add vendor for terraform plugins
896fe52 
Added `Gopkg.toml` have specific ignores..
```console
ignored = [
  "github.com/libvirt/libvirt-go",
  "github.com/hashicorp/terraform/*"
]
```

* `github.com/libvirt/libvirt-go` as CGO complains if two vendr directories have same c functions.
* `github.com/hashicorp/terraform/*` so that all plugins use the already vendored code in `terraform/exec/vendor`.
@abhinavdahiya
terraform/exec: add ignore for plugins
06aae34
@abhinavdahiya
vendor: updates from dep ensure
0448b57
@abhinavdahiya
pkg/terraform: use Terraform in logs and erorrs
f354c0d
@abhinavdahiya abhinavdahiya force-pushed the terraform_vendor branch 3 times, most recently from 1446e59 to 41b4238 Compare December 18, 2018 18:54
@abhinavdahiya
data/data: unset version from terraform providers
3e7e15f 
Terraform loads the on disk plugins to `0.0.0` version. unsettting allows us to use the local plugins
and prevents terraform from trying to fetch the provider from internet.
@abhinavdahiya
docs: remove sections that ask to install libvirt plugins
cef24b6
@abhinavdahiya
.github: remove libvirt plugin version invocation
9ee7399
@abhinavdahiya
terraform: drop Version from API
0502c5c 
The last consumers of the api was removed in openshift@41dd728

The version of the terraform is now tied to the version of the installer, so this is no longer required.
@abhinavdahiya
pkg/terraform: terraform init never downloads plugins
1ee74db 
setting the `-get-plugins=false` instructs terraform to not fetch any plugins from the ineternet.
All the plugins required by installer have to be embedded. This allows us to ensure that all the plugins required are
never downloaded from the internet.
@wking
Copy link
Member

wking commented Dec 18, 2018

/lgtm

🎉

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 18, 2018
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Dec 18, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, wking

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

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [abhinavdahiya,wking]

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

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 18, 2018

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit ead9f4b into openshift:master Dec 19, 2018
@wking wking mentioned this pull request Dec 19, 2018
Merged
@wking wking mentioned this pull request Dec 20, 2018
Merged
wking added a commit to wking/openshift-installer that referenced this pull request Dec 21, 2018
@wking
*: Make libvirt support completely conditional (behind TAGS=libvirt)
432f283 
Previously, destroy support was behind TAGS=libvirt_destroy and create
support was always built in.  But since 3fb4400 (terraform/plugins:
add `libvirt`, `aws`, `ignition`, `openstack` to KnownPlugins,
2018-12-14, openshift#919), the bundled libvirt Terraform provider has also
been behind libvirt_destroy.  That leads to cluster creation failing
with:

  $ openshift-install create cluster
  ...
  ERROR Missing required providers.
  ERROR
  ERROR The following provider constraints are not met by the currently-installed
  ERROR provider plugins:
  ERROR
  ERROR * libvirt (any version)
  ERROR
  ERROR Terraform can automatically download and install plugins to meet the given
  ERROR constraints, but this step was skipped due to the use of -get-plugins=false
  ERROR and/or -plugin-dir on the command line.
  ...

With this commit, folks trying to 'create cluster' without libvirt
compiled in will get:

  FATAL failed to fetch Common Manifests: failed to load asset "Install Config": invalid "install-config.yaml" file: platform: Invalid value: types.Platform{AWS:(*aws.Platform)(nil), Libvirt:(*libvirt.Platform)(0xc4209511f0), OpenStack:(*openstack.Platform)(nil)}: platform must be one of: aws, openstack

before we get to Terraform.

Now that the build tag guards both creation and deletion, I've renamed
it from 'libvirt_destroy' to the unqualified 'libvirt'.

I've also adjusted the install-config validation testing to use
regular expressions so we can distinguish between failures because
libvirt was not compiled in as a valid platform and failures because
some portion of the libvirt configuration was broken.  In order to get
stable error messages for comparison, I've added some strings.Sort
calls for various allowed-value string-slice computations.
@wking wking mentioned this pull request Jan 18, 2019
Closed
@wking wking mentioned this pull request Mar 26, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wking wking wking left review comments

@eparis eparis Awaiting requested review from eparis

+1 more reviewer

@crawford crawford crawford left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@wking wking

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@abhinavdahiya @wking @crawford @openshift-ci-robot @openshift-bot @openshift-merge-robot