auto-detect build runtime arch

[clnperez]

If the user hasn't explictyly set GOARCH, don't default to amd64. This
way, a user has to expliciltly cross-compile, instead of needing to know
to set GOARCH on build for a native arch that isn't amd64 (the current behaviour).

Signed-off-by: Christy Norman christy@linux.vnet.ibm.com

Description of the change:
use the go runtime's goarch instead of a potentially (probably) un-set local environment variable

Motivation for the change:
an internal build team was getting a ppc64le docker image with an x86 binary inside and was super confused :D

[camilamacedo86]

It shows perfect fine for me 👍

The CI will check this command for all types. See here
So, since it passed all still working.

Just a nit.: It is missing an entry in the CHANGELOG.
Could you please add it?

[joelanford]

The code to explicitly set GOARCH to amd64 has been around for awhile, and I'm not sure of the origins of that. If I had to guess, the originial reasoning is that operator-sdk build builds a docker container with the embedded binary, and since you might use a non-linux, non-amd64 machine as a developer, you are typically targetting linux-amd64 when you're deploying your operator.

Obviously that isn't always true (like in your case), but if we make this change, we could break a lot of other users that build from one architecture and target another for their operator deployments.

/hold

@AlexNPavel @hasbro17 @estroz any thoughts on this?

[estroz]

@joelanford @clnperez yes that is why GOARCH was explicitly set. This change will break build for a lot of users unnecessarily as you can simply set GOARCH yourself. Instead of targeting the build host arch, we should document this option in the user guide.

[hasbro17]

Ditto to not breaking existing behavior if we can mitigate this by setting GOARCH. We should document that setting though.

[camilamacedo86]

Could please you check the review comments?

[clnperez]

Hm. So -- when you do the build on a non-AMD arch, you still get the outer container for the arch you're on b/c the UBI images are using manifest lists. How would, for example, you keep from getting an arm image with a AMD binary? IOW -- what is the build procedure/use-case we'd be breaking exactly?

[clnperez]

Oops. Typo there ^ (edited). ARM image with an AMD binary.

[estroz]

@clnperez users may not be building the operator binary on an amd64 system (operator-sdk builds directly on the host), so following current workflows after changing to the suggested behavior could cause runtime errors when running the binary in a container created from the default base image. We currently provide the ability to set GOARCH if you want to cross-compile for a particular target arch, ex. ppc, so this change isn't necessary.

[clnperez]

@estroz I guess I'm trying to say that the default base image isn't an x86 one per se, so I don't think that this is actually going to break anything.

> ./operator-sdk-ppc64le-linux-gnu build quay.io/example/app-operator | tee op-sdk-build.out
INFO[0001] Building OCI image quay.io/example/app-operator 
Sending build context to Docker daemon  103.4MB
Step 1/7 : FROM registry.access.redhat.com/ubi8/ubi-minimal:latest
 ---> e63154cdf50b
Step 2/7 : ENV OPERATOR=/usr/local/bin/app-operator     USER_UID=1001     USER_NAME=app-operator
 ---> Using cache
 ---> 835ce8c58ab5
Step 3/7 : COPY build/_output/bin/app-operator ${OPERATOR}
 ---> Using cache
 ---> 2abd1ff5e521
Step 4/7 : COPY build/bin /usr/local/bin
 ---> Using cache
 ---> 618a4cfcab4d
Step 5/7 : RUN  /usr/local/bin/user_setup
 ---> Using cache
 ---> 01114f58eb82
Step 6/7 : ENTRYPOINT ["/usr/local/bin/entrypoint"]
 ---> Using cache
 ---> f95410b9fdc5
Step 7/7 : USER ${USER_UID}
 ---> Using cache
 ---> 3eb75c37e76f
Successfully built 3eb75c37e76f
Successfully tagged quay.io/example/app-operator:latest
INFO[0002] Operator build complete.                   

That FROM line is a multi-arch image. I'm assuming that the operator-sdk cli creates that Dockerfile that ends up in your build/ directory, and will always use that registry.access.redhat.com/ubi8/ubi-minimal image. If that's not the case, then maybe there is a use-case I'm missing that this would break. As I understand this, though, someone would have to modify the dockerfile to use a platform specification flag in order to get a different arch's layers and do a cross-compile. Hopefully that was a little more clear on why I don't believe this will break anyone. If you have a concrete counter-example you could provide, though, that would definitely be great.

[joelanford]

@clnperez That's a good point. There are possibilities for how things may already be broken.

1. I'm not sure when the docker CLI started supporting multi-arch images, but it may be that our original (and still current) implementation made the assumption that the scaffolded base image would always be amd64. That assumption is clearly no longer valid.
2. We accidentally already broke users expecting the default targeting of amd64 when we switched the base image from alpine to UBI. Again, not sure on the timeline of when these base images started supporting multi-arch.
3. Users building on non-amd64 systems who are targeting amd64 are getting lucky and benefiting from the fact that the only binary program running in their image is compiled for the architecture they're targeting (amd64) despite the fact that all of the other binaries (that are unused) are compiled for some non-amd64 architecture.

Either way, I think I'm convinced that users are already broken or are just getting lucky. Does anyone see something I don't?

This is yet another reason why I'm a fan of ditching the operator-sdk build subcommand and getting on board with giving users more direct insight and flexibility with their builds via a Makefile (a la Kubebuilder). See #1655. We will likely be picking up #1655 as a result of our planned kubebuilder integration.

In the meantime, perhaps the best option would be to use Go's default GOARCH handling. I'm pretty sure the logic that's in this PR would be identical to the logic the go tooling would use, so we can probably just delete the lines that handle setting GOARCH.

Thoughts?

[clnperez]

1. The manifest subcommand is still in experimental state, and was added in 18.02 (see Add manifest command docker/cli#138).
2. What exactly was that breakage (curious more than anything)? I know the UBI images were multiarch a while back (the 7.x ones were).
3. Woah. What other binaries are there? :D

If we just delte those lines, that would probably be fine. I think you could still override by setting GOARCH on a build if someone wanted to cross-compile.

[estroz]

runtime.GOARCH is set to the arch that operator-sdk was built with since it is set at compile time, which is not the compilation behavior we want.

Considering that users have been "broken" since we switched base images, and multi-arch support in UBI and eventually the docker cli, the only way to get expected behavior is to delete GOARCH setting logic (as @clnperez suggested). Users should rely on existing go binary configuration.

[joelanford]

2. What exactly was that breakage (curious more than anything)? I know the UBI images were multiarch a while back (the 7.x ones were).

I don't know if there actually was a breakage, but if alpine was not a multi-arch image, and UBI was, then we introduced the possibility of building an amd64 binary into a non-amd64 image.

3. Woah. What other binaries are there? :D

Just everything else that's in the base image, e.g. /bin/*

[clnperez]

@joelanford you mean this bin from the ubi base?

[root@777c08a0d06b /]# ./bin/rpm
RPM version 4.14.2
Copyright (C) 1998-2002 - Red Hat, Inc.
This program may be freely redistributed under the terms of the GNU GPL

(this is on my power vm, so it pulled down the power ubi image)

[clnperez]

Was this what you were thinking @joelanford #2268 ?

[camilamacedo86]

Hi @clnperez,

I understand that in order to address the comments here you did the #2268 which is merged now. So, I am closing this one. However, please feel free to re-open if I misunderstood and you think that this PR still required.

[openshift-ci-robot]

@clnperez: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.