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 ability to append Container Linux snippets #22

Merged
merged 1 commit into from
Apr 9, 2018
Merged

Add ability to append Container Linux snippets #22

merged 1 commit into from
Apr 9, 2018

Conversation

dghubble
Copy link
Member

@dghubble dghubble commented Jan 23, 2018

Allow users to supply a list of Container Linux Configs that should be appended. This means base
configs can be additively extended with custom snippets of Container Linux configs. This is a long-awaited feature to unlock end-user customizations.

Testing

I'm putting this up for comment, but still working on validating the usage in real-world scenarios.

data "ct_config" "worker" {
  content      = "${file("base.yaml")}"
  platform     = "ec2"
  pretty_print = false

  snippets = [
    "${file("units.yaml")}",
    "${file("storage.yaml")}",
  ]
}

resource "aws_instance" "worker" {
  user_data = "${data.ct_config.worker.rendered}"
}

rel: https://github.com/coreos/matchbox/issues/622

@dghubble dghubble changed the title Add ability to append Container Linux snippets DRAFT: Add ability to append Container Linux snippets Jan 23, 2018
glide.yaml Outdated
@@ -49,9 +49,11 @@ import:
subpackages:
- semver
- package: github.com/coreos/ignition
version: v0.19.0
version: 28aea78861d53a9b3a9c2b5cf51305e00f36fa14
Copy link

Choose a reason for hiding this comment

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

Is there a reason to not vendor a release of Ignition?

Copy link
Member Author

Choose a reason for hiding this comment

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

I required coreos/ignition#501, otherwise, we have to copy paste the Append code out of Ignition, because none of the packages had appropriate types. Once there is a release, definitely planning to bump to that.

Copy link

Choose a reason for hiding this comment

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

Gotcha. There should be v0.21.0 this week.

Copy link
Member Author

Choose a reason for hiding this comment

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

This definitely won't be ready until after then. I'm kinda nudging it along in spare time. I'll plan to bump to that version.

platform := d.Get("platform").(string)
snippetsIface := d.Get("snippets").([]interface{})
Copy link

Choose a reason for hiding this comment

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

I'm not familiar with terraform libraries, but why not assert this to be a []string? It feels like if the input doesn't match that it would be an error.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did try that originally. The underlying data Terraform Get gives here is actually a slice of interfaces so the assertion to []string fails. I'll look into this again, the rampant use of assertions is saddening.

Copy link

Choose a reason for hiding this comment

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

Oh right, Go doesn't let you assert a []interface{} to a []string, but it does let you iterate over a []interface and assert each thing individually to a string. What a great type system.

@crawford
Copy link

I didn't read the code, but does this continually shove appended configs as dataurls into the parent config?

@dghubble
Copy link
Member Author

What do you mean by "continually"? Describing it that way makes me think there is a misunderstanding.

The tool combines one or more Container Linux configs into a single config. If you change the input configs, yes you get a new output config. All manner of things can be done with that config (e.g. write it to a local file, upload it to a cloud provider, post it somewhere, etc).

No, not dataurls. The test case makes it a little clearer - it appends (like a merge, but not quite a merge) configs as if they had been written as one document.

@cgonyeo
Copy link

cgonyeo commented Jan 24, 2018

@crawford no, this calls the Append function from the Ignition codebase to merge the configs here. It doesn't chain together configs via the append field holding dataurls.

This happens here: https://github.com/coreos/terraform-provider-ct/pull/22/files#diff-ce3114c3cfc8a252dcbdb551d95e066bR86.

Copy link

@arithx arithx left a comment

Choose a reason for hiding this comment

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

Overall LGTM

* Allow users to supply a list of Container Linux
Configs that should be appended. This means base
configs can be additively extended with custom bits
of Container Linux configs
@dghubble dghubble changed the title DRAFT: Add ability to append Container Linux snippets Add ability to append Container Linux snippets Feb 23, 2018
dghubble added a commit that referenced this pull request Feb 23, 2018
@dghubble
Copy link
Member Author

dghubble commented Feb 23, 2018

tldr; I'm fully behind this feature now and vouch for it. Caveats about cutting the v0.3.0 release.

I had some initial problems in practical usage (turns out unrelated). Since this Terraform provider isn't an official provider, it is installed in a global location (~/.terraformrc, rather than each module defining the version it would like). Using a build of terraform-provider-ct meant running terraform plan on any existing managed clusters (i.e. those made with terraform-provider-ct v0.2.0) produced a large diff. This turns out to be caused by the switch from Ignition v2.0.0 spec to Ignition v2.1.0 in #14 - clusters created with terraform-provider-ct v2.0.0 have user data with Ignition v2.0.0, so switching to the latest terraform-provider-ct will try to replace the node (which would destroy clusters). This is unrelated to this PR, but came up in real usage.

We need to be careful. Essentially, terraform-provider-ct releases are tied to the Ignition Config version they produce. It is not safe to create a cluster with an old plugin and upgrade to a new plugin, not until terraform-provider-ct is accepted as an official provider to allow per-module dependency pinning (or Hasicorp relaxes the requirement that dependencies only work with official providers).

  • terraform-provider-ct v0.2.x -> always produce Ignition v2.0.0
  • terraform-provider-ct v0.3.x -> always produce Ignition v2.1.0
  • etc.

Action:

  • When we release v0.3.0, include a warning that upgrading from v0.2.x has repercussions on existing managed clusters.

I've created a protected v0.2.x branch to continue supporting Ignition v2.0.0 series clusters.

@dghubble
Copy link
Member Author

I'd like to cut the v0.3.0 release with this feature, bearing in mind the above caveats.

@schu
Copy link

schu commented Apr 9, 2018

Friendly ping :) Any news here? An up-to-date v0.3.0 with this in would be great - to have both Ignition 2.1 and snippets.

@dghubble
Copy link
Member Author

dghubble commented Apr 9, 2018

Should be ready for merge. As long as release notes describe the different release series and correspond to a particular Ignition version - upgrading terraform-provider-ct (global) would affect existing clusters if you re-run terraform apply.

I no longer have merge ability though. cc @sdemos

@sdemos
Copy link
Contributor

sdemos commented Apr 9, 2018

I'll release this at some point later today.

@sdemos sdemos merged commit 0dc99cc into poseidon:master Apr 9, 2018
@sdemos
Copy link
Contributor

sdemos commented Apr 26, 2018

Sorry about that, I started to do the release then something came up. It's released now though! https://github.com/coreos/terraform-provider-ct/releases/tag/v0.3.0

Also, @schu for what it's worth, snippets are also cherry-picked into the v0.2.1 release. It still outputs Ignition 2.0, though, so definitely worth updating for the newer spec if possible.

dghubble added a commit that referenced this pull request Mar 23, 2020
* Add support for Fedora CoreOS `snippets`
* Add a Fedora CoreOS example to the README
* Move example to examples for project consistency

Related: #22
dghubble added a commit that referenced this pull request Mar 23, 2020
* Add support for Fedora CoreOS `snippets`
* Add a Fedora CoreOS example to the README
* Move example to examples for project consistency

Related: #22
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.

6 participants