data/bootstrap: Pull content out of pkg/asset/ignition/bootstrap

[wking]

It's easier for humans and linters to find this content if it's not hidden in Go variables.

Since we're effectively pulling these files from Git now (either at build time or at run-time depending on release vs. dev mode in hack/build.sh), I'm being a bit more relaxed about file modes than the previous implementation. Files are now either 0555 (if they were executable) or 0600 (if they weren't). This is a change for files like manifests.Manifests, which had previously been 0644.

I've flattened the manifest overrides into a single directly, because the filenames are sufficient for sorting them by operator. And all of the override manifests now have their own comment explaining their target and eventual location.

I've also reverted #370 (no need to worry about Glide.yaml since #380), and adjusted some of YAML to keep yamllint happy. Linter coverage like that is part of the appeal of using data.Assets for these.

[abhinavdahiya]

I don't think it's more helpful to move these template files out of the go package to terraform directory.

[wking]

I don't think it's more helpful to move these template files out of the go package to terraform directory.

data/ doesn't have to be just about Terraform. For both the Terraform modules and these bootstrap files, it just provides programmatic access to files that the installer needs to access.

Did you have complexity/performance concerns? I think this simplifies finding this content for folks who are not living in this code base. And 74f997a1a8 drops 70 lines from the repo, which is a coarse measure for "simpler" ;).

[wking]

@abhinavdahiya pefers the Go-literal approach. He's given a few reasons, including files like the kubelet kubeconfig which need tge same conten in multiple locations. I still prefer the data.Assets approach as explained above, but we'll role with the status quo.

/close

[crawford]

I quite liked this approach (should have kept up with my email). This gets us YAML linting, syntax highlighting, and no more of Go's poorly-designed multi-line strings.

Can we reopen this?

[wking]

I'll let you sort this out wIth @abhinavdahiya ;).

[abhinavdahiya]

Few things i don't like about this PR

1. this makes it such that the locations of a file on bootstrap node is controlled by the relative location to data/data making the ignition asset relinquish control.
2. This also makes it difficult to model cases where

   installer/pkg/asset/ignition/bootstrap/bootstrap.go

   Lines 177 to 178 in edcd705

   	ignition.FileFromBytes("/etc/kubernetes/kubeconfig", 0600, kubeletKubeconfig.Files()[0].Data),
   	ignition.FileFromBytes("/var/lib/kubelet/kubeconfig", 0600, kubeletKubeconfig.Files()[0].Data),

   same file might need to be present in multiple location.
3. We cannot control what ends up on disk for a platform. It will become a problem with base, aws, openstack etc.. mess again

I think we want to do something like this for @rajatchopra 's manifest-template stuff but such that the asset can ask for the source of the template and then decide the destination.

[crawford]

Ah, I agree with @abhinavdahiya's points. What I actually want is the include_str! macro from Rust - read a file in from disk at compilation and make it available in a variable for runtime.

[wking]

1. this makes it such that the locations of a file on bootstrap node is controlled by the relative location to data/data making the ignition asset relinquish control.

Both the Go package and data/data are under our control, so I'm not worried about "someone put something in the wrong place in data/data and we have no way to override their location".

2. This also makes it difficult to model cases where... same file might need to be present in multiple location.

Agreed. But we don't have any of those cases in this PR, and we can always use Go templates if we hit more of them in the future. Ideally, we could configure the bootstrap node so that we wouldn't need the same content present in multiple locations there either, so I don't expect there will be a ton of these cases.

3. We cannot control what ends up on disk for a platform. It will become a problem with base, aws, openstack etc.. mess again

Again, I don't expect a ton of these cases. Do we expect many of the bootstrap files to be platform-specific? There aren't any such files in this PR, anyway.

What I actually want is the include_str! macro from Rust - read a file in from disk at compilation and make it available in a variable for runtime.

That's effectively what this is, right? You just use the URI into data/data/ to retrieve the value.

[crawford]

@wking Can you rebase this? I spoke with @abhinavdahiya and we want to pull this in.

[rajatchopra]

The kube-dns files are not needed since #548 is merged.

[wking]

Ok, rebased onto master with 3434c2f -> f45763d. I think I got all the upstream changes in; and Git's blame suggests I'm not adding much on my own in the hard-to-audit commit:

$ find data/data/bootstrap -type f -exec git --no-pager blame -CC 775f799 {} \; | grep Trevor
775f7991 (W. Trevor King 2018-10-21 23:46:09 -0700 1) apiVersion: kubecontrolplane.config.openshift.io/v1
775f7991 (W. Trevor King 2018-10-21 23:46:09 -0700 2) kind: KubeControllerManagerConfig
775f7991 data/data/bootstrap/files/opt/tectonic/manifest-overrides/kube-proxy-daemonset.yaml (W. Trevor King        2018-10-21 23:46:09 -0700  1) # This is needed by kube-proxy.
775f7991 data/data/bootstrap/files/opt/tectonic/manifest-overrides/kube-proxy-daemonset.yaml (W. Trevor King        2018-10-21 23:46:09 -0700  2) # TODO: move to the networking operator renderer.
775f7991 data/data/bootstrap/files/opt/tectonic/manifest-overrides/kube-proxy-service-account.yaml (W. Trevor King 2018-10-21 23:46:09 -0700 1) # This is needed by kube-proxy.
775f7991 data/data/bootstrap/files/opt/tectonic/manifest-overrides/kube-proxy-service-account.yaml (W. Trevor King 2018-10-21 23:46:09 -0700 2) # TODO: move to the networking operator renderer.
775f7991 data/data/bootstrap/files/opt/tectonic/manifest-overrides/kube-proxy-kube-system-rbac-role-binding.yaml (W. Trevor King 2018-10-21 23:46:09 -0700  1) # This is needed by kube-proxy.
775f7991 data/data/bootstrap/files/opt/tectonic/manifest-overrides/kube-proxy-kube-system-rbac-role-binding.yaml (W. Trevor King 2018-10-21 23:46:09 -0700  2) # TODO: move to the networking operator renderer.
775f7991 data/data/bootstrap/files/opt/tectonic/manifest-overrides/kube-proxy-role-binding.yaml (W. Trevor King 2018-10-21 23:46:09 -0700  1) # This is needed by kube-proxy.
775f7991 data/data/bootstrap/files/opt/tectonic/manifest-overrides/kube-proxy-role-binding.yaml (W. Trevor King 2018-10-21 23:46:09 -0700  2) # TODO: move to the networking operator renderer.
775f7991 data/data/bootstrap/files/usr/local/bin/bootkube.sh.template (W. Trevor King        2018-10-21 23:46:09 -0700   1) #!/usr/bin/env bash
775f7991 data/data/bootstrap/files/usr/local/bin/bootkube.sh.template (W. Trevor King        2018-10-21 23:46:09 -0700  97) # TODO: Remove this when manifest-overrides is empty.
775f7991 data/data/bootstrap/files/usr/local/bin/bootkube.sh.template (W. Trevor King        2018-10-21 23:46:09 -0700  99) cp manifest-overrides/* manifests/
775f7991 data/data/bootstrap/files/usr/local/bin/report-progress.sh (W. Trevor King 2018-10-21 23:46:09 -0700  1) #!/usr/bin/env bash
775f7991 data/data/bootstrap/files/usr/local/bin/tectonic.sh (W. Trevor King 2018-10-21 23:46:09 -0700  1) #!/usr/bin/env bash

All of those are intentional additions. And the deletions are close to the additions:

$ git show --shortstat --oneline 775f799 | cat
775f799 data/bootstrap: Pull content out of pkg/asset/ignition/bootstrap
 21 files changed, 611 insertions(+), 642 deletions(-)

so it's unlikely I'm removing much by accident. I've also gone through and tried to match up all the additions and removals locally, but it would still be good to give this a careful read in review.

[wking]

e2e-aws hung waiting for the API:

Waiting for API at https://ci-op-fb4bfyg8-1d3f3-api.origin-ci-int-aws.dev.rhcloud.com:6443 to respond ...
Another process exited

/retest

[wking]

Ah:

$ ssh -i libra.pem [email protected] journalctl | grep EXEC | tail -n2
Nov 08 05:19:18 ip-10-0-15-40 systemd[1545]: Failed at step EXEC spawning /usr/local/bin/tectonic.sh: Permission denied
Nov 08 05:19:18 ip-10-0-15-40 systemd[1]: tectonic.service: main process exited, code=exited, status=203/EXEC
$ ssh -i libra.pem [email protected] ls -l /usr/local/bin
total 20
-rw-------. 1 root root 8454 Nov  8 05:13 bootkube.sh
-rw-------. 1 root root  467 Nov  8 05:13 report-progress.sh
-rw-------. 1 root root 1417 Nov  8 05:13 tectonic.sh

Apparently vfsgen doesn't track the executable bit. I've pushed f45763d -> 9ebdb1d rebasing on master and switching the mode to depend on whether the file lives in a bin directory.

[wking]

Green here :)

[abhinavdahiya]

/lgtm

[crawford]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, crawford, 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,crawford,wking]

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