Install assets only via Ignition by squeed · Pull Request #3183 · coreos/tectonic-installer

squeed · 2018-04-16T16:49:28Z

This PR does the following:

No longer zips up /generated and uploads it to S3
Installs all needed files via Ignition
Stops writing some files to /generated
Cleans up the ignition "API"

coreosbot · 2018-04-16T16:49:34Z

Can one of the admins verify this patch?

abhinavdahiya · 2018-04-16T17:23:39Z

@squeed

in old method only the bootstrap node pulled the secrets and tried to bootstrap using bootkube. Is this PR changing that behavior, i see init-assets.sh is dropped which enforced that behaviour.
also rm-assets.sh would remove all these secrets from disk after bootstrapping. that also seems to be dropped.
sensitive data will be served through config/master endpoint in s3 bucket which is public.

squeed · 2018-04-16T19:39:22Z

@abhinavdahiya
In the old method, we uploaded both the master ignition file without secrets and a zip file containing secrets to S3. All this PR does is move everything that was in that zip file in to ignition. This was originally done because of size limitations on ignition files. It doesn't otherwise change the bootstrap process.

You are right that the .zip file has a different ACL. I'll change that.

The downloaded secret files are never deleted from the master - perhaps they should be, but that's not anything different in this PR. All "rm-assets.sh" does (and did) is overwrite the uploaded S3 files with empty files.

squeed · 2018-04-17T14:03:17Z

@abhinavdahiya Changed the ignition flow to redirect to a s3:// url. Ignition can fetch private s3:// urls.

enxebre · 2018-04-17T14:22:04Z

ok to test

abhinavdahiya · 2018-04-17T15:04:07Z

steps/assets/resources/rm-assets.sh

            REGION=$(wget -q -O - http://169.254.169.254/latest/meta-data/placement/availability-zone | sed '"'"'s/[a-zA-Z]$//'"'"')
-            /usr/bin/aws --region="$REGION" s3 cp /tmp/assets.zip s3://"$LOCATION/assets.zip"
-            /usr/bin/aws --region="$REGION" s3 cp /tmp/assets.zip s3://"$LOCATION/config/master"
+            /usr/bin/aws --region="$REGION" s3 cp /tmp/empty s3://"$LOCATION/config/bootstrap"


Do we have 'config/bootstrap' ?

Yeah. it's the third step in the redirect chain, and it has a private acl.

Should also include a set -u here, so if $LOCATION is omitted bash will complain, and we don't just get a cryptic S3 API error back.

enxebre · 2018-04-17T15:16:40Z

steps/topology/s3-assets.tf

-  # folder, we write it in the Terraform managed hidden folder `.terraform`.
-  output_path = "./.terraform/generated_${sha1("${var.tectonic_cluster_id}")}.zip"
+# The public ignition configuration
+data "ignition_config" "bootstrap_redirect" {


I’ve been scratching my head to figure out how to make the http s3 endpoint more secure, this is just very nice and super simple, thanks!

squeed · 2018-04-17T15:41:58Z

The tests have failed because it's looking for files in /generated that don't exist anymore.

squeed · 2018-04-17T16:32:39Z

I'm stuck now. All of the tests expect to be able to compare /generated/manifests and /generated/tectonic with the state of the cluster. We don't write those any more (and there's no need to). Do we need to render those to disk just so tests can run?

enxebre · 2018-04-18T06:46:19Z

The test failing is checking that some expected files are written to local disk, as this was part of the bootstrap workflow runtime https://github.com/coreos/tectonic-installer/blob/master/tests/rspec/lib/shared_examples/k8s.rb#L65
https://github.com/coreos/tectonic-installer/blob/master/tests/rspec/lib/operators.rb
Since this won't be part of the workflow anymore we don't need to test it so we can remove it.
Instead with could leverage ssh_exec to test those manifests are generated in the right path inside the machine (this would require the tests to differentiate the bootstrap node ip from the other nodes ips)
@cpanato wdyt?
Also if we eventually move this out of tf, we can also unit test the generated ignition file to match an expected config

squeed · 2018-04-18T08:31:20Z

The easiest answer is probably to enable a test mode that renders some files to disk.

squeed · 2018-04-18T10:35:36Z

all green!

colhom

Great stuff, this really cleans up the provisioning process.

In general, also wondering why the directory structure needed to shuffle around (perhaps I'm missing something.) Would be nice if the generated/ folder had same structure as before, if you think it's possible.

colhom · 2018-04-18T17:38:42Z

modules/bootkube/outputs.tf

+
+output "ignition_file_id_list" {
+  value = [
+    "${data.ignition_file.manifest_file_list.*.id}",


This splat evaluates to a list, which means the ignition_file_id_list will be a list which contains a list element (which I'm assuming is not intended). A pretty robust way to deal with this is and ensure the output is always a flat list of id's:

value = [ "${flatten(list(data.foo.bar.*.id, data.blah.thing.id, data.blah.otherthing.id))}" ]

oh man, thanks!

colhom · 2018-04-18T17:57:41Z

modules/ignition/outputs.tf

+    "${data.ignition_file.root_ca_cert_pem.id}",
+    "${data.ignition_file.ingress_ca_cert_pem.id}",
+    "${data.ignition_file.etcd_ca_cert_pem.id}",
+    "${data.ignition_file.custom_ca_cert_pem.*.id}",


Same thing here, when mixing splat id lists and singelton ids into a single list, use flatten(list(...)) so you get a flat list consistently

colhom · 2018-04-18T18:02:21Z

modules/tectonic/output.tf


+output "ignition_file_id_list" {
+  value = [
+    "${data.ignition_file.tectonic_manifest_list.*.id}",


flatten(list(...)))

colhom · 2018-04-18T18:08:09Z

modules/tls/ca/self-signed/assets.tf

+  path       = "/opt/tectonic/tls/root-ca.crt"
+
+  content {
+    content = "${var.root_ca_cert_pem == "" ? join("", tls_self_signed_cert.root_ca.*.cert_pem) : var.root_ca_cert_pem}"


Ternary expressions conditionally consuming lists can make terraform behave "oddly". Would recommend doing something more generic like:

${join("\n",compact(flatten(list(tls_self_signed_cert.root_ca.*.cert_pem, var.root_ca_cert_pem))))}

Also- I'd think you'd want to join the certificate pem blocks using a newline, instead of empty string?

I just copied this from the line above; while you're probably right, I'd rather not change what "works". I'm not sure that custom certificates work right now anyways.

a good rule of thumb w/ terraform is to only use ternary expressions when all inputs and outputs are primitives. we learned this the hard way in earlier versions of terraform, for all I know this could be perfectly fine with more current versions.

colhom · 2018-04-18T18:08:56Z

modules/tls/ca/self-signed/assets.tf


+data "ignition_file" "aggregator_ca_key" {
+  filesystem = "root"
+  mode       = "0644"


This (and all other private keys) need to be 0600 mode

colhom · 2018-04-18T18:17:09Z

steps/assets/ignition-bootstrap.tf

 }
+
+data "ignition_config" "bootstrap" {
+  files = ["${compact(list(


This will also be a nested list situation, recommend flattening the whole list. (nice use of compact here).

colhom · 2018-04-18T18:17:21Z

steps/assets/ignition-bootstrap.tf

+    "${module.etcd_certs.ignition_file_id_list}",
+  ]
+
+  systemd = ["${compact(list(


flatten here as well

colhom · 2018-04-18T18:18:44Z

steps/assets/local-files.tf

+# some files we want rendered to disk for other use
+resource "local_file" "kubeconfig-kubelet" {
+  content  = "${module.bootkube.kubeconfig-kubelet_rendered}"
+  filename = "./generated/auth/kubeconfig-kubelet"


It would be nice if these generated file paths were canonically defined in a locals block.

colhom · 2018-04-18T18:20:26Z

steps/assets/resources/rm-assets.sh

            REGION=$(wget -q -O - http://169.254.169.254/latest/meta-data/placement/availability-zone | sed '"'"'s/[a-zA-Z]$//'"'"')
-            /usr/bin/aws --region="$REGION" s3 cp /tmp/assets.zip s3://"$LOCATION/assets.zip"
-            /usr/bin/aws --region="$REGION" s3 cp /tmp/assets.zip s3://"$LOCATION/config/master"
+            /usr/bin/aws --region="$REGION" s3 cp /tmp/empty s3://"$LOCATION/config/bootstrap"


Should also include a set -u here, so if $LOCATION is omitted bash will complain, and we don't just get a cryptic S3 API error back.

colhom · 2018-04-18T18:21:52Z

steps/topology/s3-assets.tf

-  output_path = "./.terraform/generated_${sha1("${var.tectonic_cluster_id}")}.zip"
+# The public ignition configuration
+data "ignition_config" "bootstrap_redirect" {
+  replace {


What is the reason this a replace and not an append block?

Basically to keep people honest :-).

In other words, I don't want there any to be any chance that there's insecure ignition information out there.

This PR does the following: - No longer zips up /generated and uploads it to S3 - Installs all needed files via Ignition - Stops writing some files to /generated - Cleans up the ignition API

squeed · 2018-04-20T15:07:01Z

@colhom thanks a ton for the terraform tips! I reverted the directory structure change, there was no real reason for it.

colhom

LGTM!

squat · 2018-04-23T08:12:59Z

🎉

The last consumer was removed in b2e0bcf (Don't upload /generated to S3 - only use Ignition, 2018-04-13, coreos/tectonic-installer#3183).

This variable was added in a2232f3 (platform/aws: create bootstrap step, 2018-02-14, coreos/tectonic-installer#2946), to force VPC creation after the S3 bucket. But that dependency was removed in b2e0bcf (Don't upload /generated to S3 - only use Ignition, 2018-04-13, coreos/tectonic-installer#3183), so we no longer need the variable.

installer: generate the cluster configmaps before asset generation

93fff9b

squeed force-pushed the no-assets branch from f3d1092 to cf42cfe Compare April 16, 2018 16:51

squeed force-pushed the no-assets branch 2 times, most recently from 8064d4c to 0b34564 Compare April 17, 2018 14:01

squeed force-pushed the no-assets branch from 0b34564 to b133a37 Compare April 17, 2018 14:07

enxebre added platform/aws run-smoke-tests labels Apr 17, 2018

squeed force-pushed the no-assets branch from b133a37 to 803440c Compare April 17, 2018 14:51

abhinavdahiya reviewed Apr 17, 2018

View reviewed changes

enxebre reviewed Apr 17, 2018

View reviewed changes

squeed force-pushed the no-assets branch from 803440c to c14216a Compare April 18, 2018 09:41

enxebre previously approved these changes Apr 18, 2018

View reviewed changes

spangenberg previously approved these changes Apr 18, 2018

View reviewed changes

colhom suggested changes Apr 18, 2018

View reviewed changes

squeed dismissed stale reviews from spangenberg and enxebre via 2a26fc7 April 20, 2018 14:48

squeed force-pushed the no-assets branch from c14216a to 2a26fc7 Compare April 20, 2018 14:48

Don't upload /generated to S3 - only use Ignition

b2e0bcf

This PR does the following: - No longer zips up /generated and uploads it to S3 - Installs all needed files via Ignition - Stops writing some files to /generated - Cleans up the ignition API

squeed force-pushed the no-assets branch from 2a26fc7 to b2e0bcf Compare April 20, 2018 15:06

colhom approved these changes Apr 20, 2018

View reviewed changes

enxebre approved these changes Apr 23, 2018

View reviewed changes

squat approved these changes Apr 23, 2018

View reviewed changes

squat merged commit f0c524c into coreos:master Apr 23, 2018

enxebre mentioned this pull request Apr 23, 2018

TLS bootstrap using tnc #3174

Merged

squeed mentioned this pull request Apr 23, 2018

Refactor workflow #3197

Closed

wking added a commit to wking/openshift-installer that referenced this pull request Aug 17, 2018

config: Remove gcloudsdk

24af3e7

The last consumer was removed in b2e0bcf (Don't upload /generated to S3 - only use Ignition, 2018-04-13, coreos/tectonic-installer#3183).

wking mentioned this pull request Aug 17, 2018

config: Remove gcloudsdk openshift/installer#148

Merged

wking mentioned this pull request Nov 18, 2018

data/aws/vpc: Drop unused depends_on variable openshift/installer#699

Merged

Conversation

squeed commented Apr 16, 2018

Uh oh!

coreosbot commented Apr 16, 2018

Uh oh!

abhinavdahiya commented Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

squeed commented Apr 16, 2018

Uh oh!

squeed commented Apr 17, 2018

Uh oh!

enxebre commented Apr 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squeed commented Apr 17, 2018

Uh oh!

squeed commented Apr 17, 2018

Uh oh!

enxebre commented Apr 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

squeed commented Apr 18, 2018

Uh oh!

squeed commented Apr 18, 2018

Uh oh!

colhom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squeed commented Apr 20, 2018

Uh oh!

colhom left a comment

Choose a reason for hiding this comment

Uh oh!

squat commented Apr 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

abhinavdahiya commented Apr 16, 2018 •

edited

Loading

enxebre commented Apr 18, 2018 •

edited

Loading