Run COSA as an OpenShift custom build strategy #1752

darkmuggle · 2020-10-02T01:48:10Z

This provides the support for making COSA an OCP/OKD custom build strategy.

Please review by commit.

This large change extends entrypoint with the aim of running as a full-featured OpenShift BuildConfig and Kubernetes Client:

Automated secret discovery using a service account. Secrets that are labeled and follow the COSA defined format will be automatically placed into /srv/secret/<NAME> and have envVars appropriately set.
Any buildConfig secret will be copied to /srv/secret/<NAME>. These secrets are provided using the Kubernetes Secret Mapping format.
Supports both binary (oc ... --from-{file,repo,dir}) and Git clones.
Works on both 4.x and 3.x OpenShift clusters.
Allows unprivileged mode to use /etc/resolv.conf and CA certificates from the container.

/cc @bh7cw @ravanelli @mike-nguyen

cgwalters · 2020-10-05T21:09:15Z

ocp/cosa-image.yaml

+parameters:
+  - description: Git source URI
+    name: REPO_URL
+    value: "https://github.com/darkmuggle/coreos-assembler"


Will need to change after being merged.

Exactly. I'll be putting a clean PR shortly.

ocp/cosa-image.yaml

cgwalters · 2020-10-05T21:09:39Z

ocp/cosa-image.yaml

+objects:
+
+  ### Fedora Caching ###
+  # keep a local copy of Fedora, since its use to build our Docker Images


Do we really need this?

ocp/cosa-image.yaml

src/cmdlib.sh

cgwalters · 2020-10-07T20:14:25Z

A long time ago when OpenShift 4 was being formed Clayton pushed really hard for the idea of OS builds as containers, and I think it has been very successful for RHCOS (and OKD's FCOS builds) but...we basically ended up going back to unifying our bootimage and oscontainer builds (for RHCOS).

If we go this route it would be quite interesting I think to actually use the "output to" part of OpenShift builds and push the oscontainer via this route. We still need to build bootimages - but we don't need to build them as often, so that could be a separate buildconfig that would take the oscontainer as input.

(This also gets into the question of oscontainers for FCOS)

darkmuggle · 2020-10-08T19:16:42Z

If we go this route it would be quite interesting I think to actually use the "output to" part of OpenShift builds and push the oscontainer via this route.

I seriously wondered about this exact thing. While the current POC does all the build steps, pushing the OSContainer seemed like the "natural" build thing to do.

We still need to build bootimages - but we don't need to build them as often, so that could be a separate buildconfig that would take the oscontainer as input.

Ha. I've tried to figure out what that would look like in practice. But I wondered about this very idea myself. The main reason I have been thinking about this idea is that we could literally do a just-in-time build of boot images and even back fill if needed.

At the risk of putting potential bad ideas into the universal consciousness, this opens the door to potentially providing the ability for customers to do boot image images in their own clusters. What this PR doesn't show (yet), is that I have gone down the rabbit hole of teaching entry about using the in-cluster OpenShift API's to handle secrets in a sane way (use annotations on secrets to find the secrets, instead of having to manually map the secrets in).

darkmuggle · 2020-10-10T15:31:42Z

The latest push adds some nice improvements:

uses the OpenShift build client API to parse the builds (which is a lot nicer)
uses a Kubernetes in-cluster GoLang client for automated discovery of secrets
starting work for standardizing the secrets with envVar and file-based secrets

The secret handling requires a service account and the templates have been updated. The reason I spent a bit of time working out the secrets was to make it dead simple for secret handling. The RHCOS jobspec grew out of different secrets needing to be mapped. By having COSA deal with the secrets that Groovy used to do, the pipelines for setting up secrets can be a bit simpler; this does not, however, change anything for the FCOS pipelines.

With support for secrets the setup for a new build environment is simplified:

create an OCP namespace
populate the secrets
apply the COSA image template
apply the COSA buildconfig template (which creates the Service Account and role-bindings)

The remaining work for this is the glue-work:

CI Libs to use this
Adding OCP secrets for remote OCP environments
Figure out the best way to get the logs and build artifacts off the pods.

I also think that it would be nice to have a COSA to flesh out the dependency handling. This provides a foundation for creating those abstractions.

Given how massive this PR is now, I think it best to deal with the other things in follow on PR's. This PR has the basic "must-have features" to be useful.

Required GoLang dependencies to run entrypoint as a proper Kubernetes and OpenShift client.

darkmuggle · 2020-10-14T23:21:34Z

Okay, I've lifted the WIP and confirmed that we run this on 3.x and 4.x clusters.

To review this work, I highly recommend that you go commit by commit. Also, I would happily provide a live-walk through and a live PR review if any would find this helpful. I fully understand that I'm asking a lot in from my reviewers; this is the culmination of two solid weeks' worth of work and that's why it's big. Rather than deliver something that doesn't work I wanted to deliver something that usable today (with rough edges).

I'll push a fix to the CI the entrypoint test that's failing (it's detecting that's it's on OpenShift when the test expects it to fail -- it's basically saying that the Kubernetes Client code works).

cgwalters · 2020-10-14T23:35:17Z

At a high level, we're still supporting running cosa directly via podman, right? I think we really need to do this in order to avoid dependency cycles w/OpenShift plus the local dev case.

Maybe rework the docs to denote the two entrypoints and how one would get started with those?

cgwalters · 2020-10-14T23:37:38Z

src/cmdlib.sh

-        fatal "$(pwd) must be a volume"
-    fi
+    if [ -n "${BUILD:-}" ]; then
+        info "COSA is running as a buildconfig, skipping checks"


Hmm. I think I'd say we should make these into explicit options via environment variable instead; something like COSA_SKIP_SANITY=kvm,volume ?

Running without /dev/kvm probably works (qemu full emulation) but it's going to be ~10x slower and particularly painful for tests. I think that's only something we'd want people to explicitly opt in to.

I used the BUILD envVar since its specified in buildconfig.openshift.io/v1 and gets set to via the OCP build operator. In fact, is the JSON buildConfig object.

However, I see a way to do this in a saner way. Once entrypoint establishes that its on an OpenShift Build, then have it set the envVars. Fix up to follow.

I split out #1795

cgwalters · 2020-10-14T23:39:38Z

src/vmdeps.txt

@@ -27,3 +27,5 @@ cryptsetup
 # filesystems/storage
 gdisk xfsprogs e2fsprogs dosfstools btrfs-progs

+# needed for basic CA support
+ca-certificates


Longer term it'd be nicer to entirely avoid doing networking in supermin - basically we pull content from the container and then operate on it via the VM. But obviously not the case today.

cgwalters · 2020-10-14T23:42:12Z

entrypoint/ocp/build_secrets.go

+}
+
+// buildSecretsSetup copies the pod mapped secrets so that the build-user is
+// able to read them. Secrets on 3.x clusters are mapped to 0400 and owned by root.


Hmm wouldn't that be a function of the user the build runs as? Are these builds running as uid 0 or part of runAsRange?

On 3.x, Custom Build Strategies run as 0. So entry runs as root since it runs before the cosa commands which sudo to builder.

Is that still true in 4.x? Wouldn't we want to match what the current FCOS pipeline does and runAsAny i.e. use a dynamic UID - I don't think we use sudo at all there.

You are correct that the behavior for 4.x is different. Truthfully, I would hope that this goes away once we have all the builds on 4.x clusters. I'm filing a PR to get the permissions to do builds on api.ci (and figure out how to get the context from behind the firewall).

entrypoint/ocp/builder.go

entrypoint/ocp/cosa_init.go

darkmuggle · 2020-10-15T00:33:10Z

ocp/README.md

+# COSA as a Custom Build
+
+With the `entry` command, COSA has the wire-framing to become a full-fledged "custom build strategy" for OpenShift.
+


Note: I'll fill out a complete README.md later. This is provided as the stub for followup.

darkmuggle · 2020-10-15T02:23:56Z

At a high level, we're still supporting running cosa directly via podman, right? I think we really need to do this in order to avoid dependency cycles w/OpenShift plus the local dev case.

💯 we are still supporting Podman and I intend entrypoint to be used to orchestrate COSA itself. One of the unstated goals is to remove the "it worked in Podman but does work in $PIPELINE problem". I see using Pods for some builds and BuildConfigs for others. The pipeline that will be the most impacted is RHCOS's.

jlebon · 2020-10-15T15:52:11Z

entrypoint/cmd/entry.go

-		Short: "Run Steps from [file]",
-		Args:  cobra.MinimumNArgs(1),
-		Run:   runSteps,
+		Use:          "run-steps",


This is bikeshedding 🖌️, though WDYT about e.g. run-script instead to better reflect what this does? run-steps sounds like the input file is in a DSL. The rest of the codebase also seems to use script.

If we're bike-shedding, it would be run-scripts, since it takes N arguments. However, I'll concede your bike shed on the condition of fixing that in a follow-up. The naming of "run-steps" betrays some thinking I've been doing on an interface on dependencies. And I'm not entirely sure what I want to propose there.

Bikeshed implemented.

jlebon · 2020-10-15T18:51:15Z

entrypoint/ocp/builder.go

+	DeveloperMode string `envVar:"COSA_DEVELOPER_MODE"`
+	JobSpecURL    string `envVar:"COSA_JOBSPEC_URL"`
+	JobSpecRef    string `envVar:"COSA_JOBSPEC_REF"`
+	JobSpecFile   string `envVar:"COSA_JOBSPEC_FILE"`
+	CosaCmds      string `envVar:"COSA_CMDS"`


Ahhh, does the golang OCP API we vendor here somehow understand these envVar:"..." field annotations?

Nope, these are our own custom envVars and are processed via https://github.com/coreos/coreos-assembler/pull/1752/files#diff-1778431a450c147795d65948008d1e55f9d4d43badf4585872975c59485295f2R53

I wanted to make it simple to add envVars so I used reflection.

jlebon · 2020-10-15T18:56:03Z

entrypoint/ocp/builder.go

+		if err := cosaInit(cosaSrvDir); err != ErrNoSourceInput {
+			return err
+		}
+		log.Info("No source input, relying solely on envVars...this won't end well.")


Is this mode supported just for local testing?

Exactly. If we hit this condition it means we have a workable $BUILD JSON object, but can't talk to the OpenShift API. The snark was to signal that this won't end as intended.

entrypoint/ocp/cosa_init.go

entrypoint/ocp/ocp.go

jlebon · 2020-10-15T19:14:42Z

src/cmdlib.sh

                    -append "root=/dev/vda console=${DEFAULT_TERMINAL} selinux=1 enforcing=0 autorelabel=1" \
                   )

    # support local dev cases where src/config is a symlink
    if [ -L "${workdir}/src/config" ]; then
        # qemu follows symlinks
-        base_qemu_args+=("-virtfs" 'local,id=source,path='"${workdir}"'/src/config,security_model=none,mount_tag=source')
+        base_qemu_args+=("-virtfs" 'local,id=source,path='"${workdir}"'/src/config,security_model=none,mount_tag=source,multidevs=remap')


Why do we need multidevs=remap? Maybe a comment here or in the commit message?

Sure, I'll add that. The reason, though, is that Qemu emits a warning that its needed. This was added because of /etc/pki being presented along with $WORKDIR.

cgwalters · 2020-10-15T21:01:35Z

entrypoint/ocp/ocp.go

+	// BuildConfigs do not use overlays. And when running in Buildconfig
+	// mode, force running as unprivileged.
+	os.Setenv("COSA_SKIP_OVERLAY", "skip")
+	os.Setenv("FORCE_UNPRIVILEGED", "1")


I think we should should probably set these on the pod spec. Setenv isn't threadsafe, Java actually did the right thing in disallowing it from the start. See:

https://sourceware.org/bugzilla/show_bug.cgi?id=15607

libstd: Add thread unsafety warnings around setenv() and unsetenv() rust-lang/rust#24741

(I still have the scar from my days on the GNOME team at RH where several of us spent hours and hours at one point chasing an intermittent memory corruption bug that was caused by gnome-settings-daemon calling setenv() after a thread had been started, the code changed in an innocent-looking refactoring to start a thread before that)

Yes, but....this runs before the COSA commands (i.e. we're not spawning threads yet). I went back and forth on. My thinking was to try and do the "better thing." But to concede a point, there is a pod-template for 3.x and 4.x already.

Right, the gnome-settings-daemon code was also fine for years until an apparently innocent refactoring later...

(Also this is golang, libraries will absoutely spawn background goroutines that can be threads etc.)

Look, I understand your old wounds are informing your opinion here.

I have my scars that that are pushing me the other way. I think that putting this in the pod spec is a bad idea. I'm aiming to remove what's in the pod spec because of scalability. Pushing into the pod Spec works if you only care about one or two pipelines. The reason I am arguing for this world is because of the cost of having to change and update the templates is expensive -- I am trying to reduce the times that we have to go to the ART and Multiarch teams and tell them that a change made in COSA requires action on their part when we can address the problem in a cheaper way.

I even did an audit of the envVar's being set AND read by the vendored code as a due-diligence. There is no collision or potential collision in the existing code.

I'm holding my ground on the use of envVars. As a middle ground compromise, which I think is unneeded at this point, I am now explicitly setting thecmd.Env for the command.

I even did an audit of the envVar's being set AND read by the vendored code as a due-diligence. There is no collision or potential collision in the existing code.

Right, I should have been more clear - the real problem is when you link C (or C++/Rust) code into the same process, which we aren't doing right now (AFAIK). But https://github.com/ostreedev/ostree-go exists and it could make sense to use it - and the second we do so, every setenv() call introduces the risk of all the getenv() calls on the C side returning arbitrary data and hence in the limit arbitrary behavior. (Think of things like the http_proxy variable, plus things like PATH telling us where to find binaries etc)

As a middle ground compromise, which I think is unneeded at this point, I am now explicitly setting thecmd.Env for the command.

Oh yep that's totally correct as well and a good solution.

The reason I am arguing for this world is because of the cost of having to change and update the templates is expensive -- I am trying to reduce the times that we have to go to the ART and Multiarch teams and tell them that a change made in COSA requires action on their part when we can address the problem in a cheaper way.

Don't we control the pod spec generated from the buildconfig? I wouldn't expect we fork the copy of that per pipeline.

Thank you @cgwalters for the clarification on the concerns; context matters, and the added context is really helpful.

Don't we control the pod spec generated from the buildconfig?

The first spec, yes. But when running as a buildConfig that is the only pod.

One of the un-obvious aspects of this PR is the setup of secrets based on Kubernetes Labels and well-formed secrets for automated secret discoveries. Using AWS as an example a secret thusly formed:

apiVersion: v1 data: config: <REDACTED> default_region: <BASED64 encoded> kind: Secret metadata: labels: coreos-assembler.coreos.com/secret: aws name: mysecret-thats-not-obviously-an-aws-one type: Opaque

could be automatically discovered (so long as the service account had permissions to read the secret) and would then write /srv/secret/mysecret-thats-not-obviously-an-aws-one/config and set AWS_CONFIG to that file and AWS_DEFAULT_REGION to the value of the unencrypted secret.

For the pod spec, the question is how the pod is created. In the buildConfig situation, entry is already pre-created as the pod.

BUT, your comment makes me realize that I haven't been imaginative enough in my thinking. And I honestly, I can't believe that I didn't think of this before: if the Service Account has edit permissions, then we create a pod with some of this all set up for us and we can work around the restrictions on /srv needing to be an overlay and not needing FORCE_UNPRIVILEGED. Okay, Mr @cgwalters, well done.

Right, I'd be pretty surprised if the custom build support didn't allow control over the pod as far as volume mounts, environment variables etc.

But another option is to have a wrapper shell script that sets up environment variables before calling the Go entrypoint.

cgwalters

Adding an overall approve to this, I think we can do more things as a followup indeed.

Files: - production and development Dockerfiles - cosa-image.yaml is a template for importing COSA from quay.io and building the BuildConfig image using entry. - cosa-bc*.yaml defines COSA as a CustomBuild Strategy for both 3.x and 4.x clusters. Note: to use these templates, you *must* have been granted access to the 'system:build-strategy-custom' role. These templates have been tested on CentOS CI and the internal Red Hat privilaged cluster.

To support the environmental differences between running as a pod versus a buildconfig: - src/cmdlib.sh: * when COSA_BC_MODE is set, skip check on /srv being an overlay * enable networking mode for unprivileged pass by exposing /etc/pki and /etc/resolv.conf into the Supermin container. - src/supermin-init-prelude.sh: mount /etc/pki - src/vmdeps.txt: add ca-certificates

High-level changes: - cmd/entry.go: changed `runSteps` to `runScripts` - cmd/build.go: added `builder` command for running as either a Pod or a CustomBuildStrategy. - entrypoint/ocp: added OCP Build Client for both binary and source builds entrypoint has been extended to include both API clients for OpenShift Builds and Kubernetes. By using the API clients we avoid parsing envVars and and directly discover Kubernetes object suchas secrets, config maps, and start other builds. entrypoint also fully implements the buildconfig.openshift.io/v1 client supporting both binary and source builds. When the buildConfig has been created, users will be able to run `oc start-build bc/....`. Binary mode will unpack the tarball into `/srv`. If the binary payload or source repo contains: - jobspec.yaml: the YAML values will be presented as GoLang templates. For example the command, `cosa init {{ .Recipe.URL }}`. - *.cosa.sh: Any file matching this pattern will be run based on file globbing rules. - If no script is provided, then the envVars will be used. When run in source mode, the API will inform where to find the build recipe. If running in pod mode, SOURCE_{URI,REF} should be set. The envVar "COSA_CMDS" should contain the steps to be run except "cosa init". This change also includes secret discovery when a service account is present to the buildconfig or pod. Consider the following secret: apiVersion: v1 data: aws_default_region: dXMtZWFzdC0xCg== config:... kind: Secret metadata: annotations: labels: coreos-assembler.coreos.com/secret: aws name: my-super-secret-AWS-keys type: Opaque When ocp.secretMapper(contextDir) is called, it will look for secrets with the label 'coreos-assembler.coreos.com/secret' and then look for a matching configuration. If the secret is defined, then entrypoint will map in the envVars to the common CLI envars, otherwise iti will write secret to file and then set an envar to the name. In the above example, it would: - set the envVar "AWS_DEFAULT_REGION" to "us-east-1" - write config to /srv/secrets/my-super-secret-AWS-keys/config and set AWS_CONFIG_FILE to that location. Signed-off-by: Ben Howard <[email protected]>

To address concerns about potential debug trama, all envVars for command execution are now put to a []string. This ensures that some random Go routine won't change a variable. Signed-off-by: Ben Howard <[email protected]>

mike-nguyen · 2020-10-19T21:53:35Z

/lgtm

openshift-ci-robot · 2020-10-19T21:53:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, darkmuggle, mike-nguyen

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:

~~OWNERS~~ [cgwalters,darkmuggle,mike-nguyen]

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

darkmuggle added the WIP PR still being worked on label Oct 2, 2020

openshift-ci-robot requested review from bh7cw, mike-nguyen and ravanelli October 2, 2020 01:48

openshift-ci-robot added do-not-merge/work-in-progress approved labels Oct 2, 2020

cgwalters reviewed Oct 5, 2020

View reviewed changes

entrypoint: add dependencies OpenShift/Kubernetes

8e81ce7

Required GoLang dependencies to run entrypoint as a proper Kubernetes and OpenShift client.

darkmuggle removed WIP PR still being worked on do-not-merge/work-in-progress labels Oct 14, 2020

darkmuggle changed the title ~~WIP: Basic OpenShift custom-build strategy~~ Run COSA as an OpenShift custom build strategy Oct 14, 2020

cgwalters reviewed Oct 14, 2020

View reviewed changes

darkmuggle commented Oct 15, 2020

View reviewed changes

jlebon reviewed Oct 15, 2020

View reviewed changes

cgwalters reviewed Oct 15, 2020

View reviewed changes

cgwalters approved these changes Oct 15, 2020

View reviewed changes

Ben Howard added 4 commits October 15, 2020 19:12

entrypoint: be overly pendantic about envVars

d367ba4

To address concerns about potential debug trama, all envVars for command execution are now put to a []string. This ensures that some random Go routine won't change a variable. Signed-off-by: Ben Howard <[email protected]>

openshift-ci-robot assigned mike-nguyen Oct 19, 2020

openshift-ci-robot added the lgtm label Oct 19, 2020

openshift-merge-robot merged commit fe12a53 into coreos:master Oct 19, 2020

jlebon mentioned this pull request Oct 23, 2020

cmdlib: stop copying resolv.conf #1806

Merged

jlebon mentioned this pull request Nov 9, 2020

mantle: openstack: fix passing of openstack config file path #1840

Merged

darkmuggle deleted the feature/ocp_bc branch April 15, 2021 14:23

		# COSA as a Custom Build

		With the `entry` command, COSA has the wire-framing to become a full-fledged "custom build strategy" for OpenShift.

Run COSA as an OpenShift custom build strategy #1752

Run COSA as an OpenShift custom build strategy #1752

Uh oh!

Conversation

darkmuggle commented Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cgwalters commented Oct 7, 2020

Uh oh!

darkmuggle commented Oct 8, 2020

Uh oh!

darkmuggle commented Oct 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

darkmuggle commented Oct 14, 2020

Uh oh!

cgwalters commented Oct 14, 2020

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!

darkmuggle Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darkmuggle commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Uh oh!

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

darkmuggle commented Oct 2, 2020 •

edited

Loading

darkmuggle commented Oct 10, 2020 •

edited

Loading

darkmuggle Oct 15, 2020 •

edited

Loading

darkmuggle commented Oct 15, 2020 •

edited

Loading

darkmuggle Oct 16, 2020 •

edited

Loading

darkmuggle Oct 19, 2020 •

edited

Loading