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

Dont use terraform's file() for singleline strings in GCE metadata #9084

Merged
merged 2 commits into from
May 8, 2020

Conversation

rifelpet
Copy link
Member

@rifelpet rifelpet commented May 7, 2020

Currently the instance_template's metadata field contains the startup script, ssh keys, cluster name, and IG name. rather than storing redundant separate files containing just the cluster and IG names, this will store them as string literals within the .tf file.

The motivation for this is that a new version of hcl2 adds support for writing multiline maps. Using the library's function for writing maps will allow us to get rid of our own hcl2.go's writeMap function. The only blocker is that the library doesn't support values being literals like foo = file(). GCE's instance_template is the only such use of maps in kops and this fixes 2 of the 4 fields.

The instance_template terraform resource actually has a separate metadata_startup_script field that we can use instead of having the script in the metadata field (I'll update that in a followup PR). That leaves the ssh key as the last file() in the metadata field. We could inline it as another hardcoded string, but it seems better to use ssh keys at the project level rather than instance level. I dont have a gce account to test that though so I'd need assistance in that migration.

/assign @geojaz

This also removes a redundant terraform literal function LiteralExpression that was only used in one place, with the identical LiteralFromStringValue being more commonly used.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 7, 2020
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 7, 2020
Copy link
Member

@geojaz geojaz left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning out those file()s that reference a single word. Those are kinda silly.

I agree that we should move the actual startup script to the metadata_startup_script field in a follow-on PR.

For the project-wide SSH keys, i think you may have incorrectly linked to the docs for os-login. google_compute_project_metadata_item would manage the SSH key at the project-level. Mislink or not, os-login is probably a feature that we should be supporting in kops since it's the generally accepted way to manage SSH access to instances in GCE.

I would vote to just move the SSH key to the project-metadata for now and tackle alternative access models a different day. I'm happy to take a look at that PR when it's ready. I'll write an issue for os-login....

/approve
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: geojaz, 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:

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 merged commit 8768178 into kubernetes:master May 8, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone May 8, 2020
@rifelpet rifelpet deleted the gce-tf-metadata branch October 29, 2020 02:03
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. 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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants