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

Create a second Terraform provider for managed files #12322

Merged
merged 7 commits into from
Sep 18, 2021

Conversation

rifelpet
Copy link
Member

@rifelpet rifelpet commented Sep 14, 2021

This adds a second Terraform provider definition for managed files. All managed files now have an alias field that refers to the new provider's alias.

Some notes:

  • We decided over slack to add a second API field for extra configuration of the new provider definition. I'm not satisfied with the new field's name of filesProviderExtraConfig so any suggestions would be appreciated.
  • This functionality requires Terraform 0.15 so I added a release note about upgrade requirements
  • As mentioned in TODO comments, we're use the cloud provider of the cluster rather than of the VFS path, so this may need to be revisited when we begin supporting cross-cloud cluster+VFS combinations. I didn't see a straight forward way in our VFS code to determine a cloud provider based on a path but we could certainly add that later.
  • I use a datasource to lookup the bucket's region and use that in the new provider's configuration. Alternatively if we had a way of using the VFS code to determine the bucket's region, that would be preferable. Also I wasn't sure whether to use the ClusterSpec's ConfigStore or ConfigBase.
  • I also added a release note for the deprecation of the TerraformJSON feature flag as discussed in #kops-dev on slack. Terraform's JSON doesn't support provider aliasing so we can't define two AWS providers. Trying to have the MinimalJSON integration test support -TerraformManagedFiles proved to be too complex given we'll be removing the support in 1.23 so I'm removing the integration test altogether.

@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 Sep 14, 2021
@k8s-ci-robot k8s-ci-robot added area/api area/documentation area/provider/aws Issues or PRs related to aws provider labels Sep 14, 2021
@rifelpet rifelpet force-pushed the managed-files2 branch 2 times, most recently from 275b26f to ea9fb8a Compare September 14, 2021 02:26
@rifelpet
Copy link
Member Author

The fact that we use the same memfs paths in AWS and GCE integration tests is causing us to render the GCS files as aws_s3_bucket_objects with this alias, which is causing the verify-terraform job to fail. Maybe we can somehow use our s3fs and gcsfs code for memfs integration tests somehow.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2021
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2021
@rifelpet rifelpet force-pushed the managed-files2 branch 2 times, most recently from e27b3f3 to f3c9f27 Compare September 17, 2021 12:29
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2021
@rifelpet rifelpet force-pushed the managed-files2 branch 2 times, most recently from 714bd7e to 6b6f930 Compare September 17, 2021 12:37
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2021
@rifelpet
Copy link
Member Author

This is ready for review. I refactored this a bit, now adding the provider definition to the vfs.TerraformPath interface. This allows each VFS provider to define both their terraform resource definitions and terraform provider definitions. I still have a new API field for additional provider configuration that gets merged into what is specified in the VFS code.

This has a nice side effect for our integration tests - because they all use memfs regardless of the cluster's cloud provider, and the memfs terraform resource definitions use aws_s3_bucket_object, I defined memfs' provider to use use aws, so now the GCE integration tests' kubernetes.tf show aws_s3_bucket_objects using an aws provider as if we supported that configuration natively, and all of the verify scripts pass.

In the future I could see vaultfs adding terraform support too.

@rifelpet rifelpet changed the title WIP Create a second Terraform provider for managed files Create a second Terraform provider for managed files Sep 17, 2021
@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 Sep 17, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 18, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 18, 2021
@k8s-ci-robot k8s-ci-robot merged commit 7f7a78a into kubernetes:master Sep 18, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Sep 18, 2021
k8s-ci-robot added a commit that referenced this pull request Sep 19, 2021
…22-origin-release-1.22

Automated cherry pick of #12322: Add support for writing lists of terraform literals
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/documentation area/provider/aws Issues or PRs related to aws provider blocks-next 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

3 participants