Add multistage docker builds

[Dean-Coakley]

Proposed changes

• Converted spaces to tabs in all Dockerfiles for consistency
• Building with BUILD_IN_CONTAINER = 1 now uses a multi stage docker build instead of using volumes.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

• I have read the CONTRIBUTING doc
• I have checked that all unit tests pass after adding my changes
• I have updated necessary documentation
• I have rebased my branch onto master
• I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

[pleshakov]

@Dean-Coakley thanks for the PR!

I suggest making the following changes:

• In the Dockerfiles, remove all the stages expect for the base
• Add two files in the build directory. One, let's call it StagesForBuildInContainer will contain the stages for building the container, the other - StagesForLocalBuild will contain the stages for building locally.
• In the makefile you will have something like that:

container: test verify-codegen nginx-ingress certificate-and-key
	cp $(DOCKERFILEPATH)/$(DOCKERFILE) ./Dockerfile
ifeq ($(BUILD_IN_CONTAINER),1)
	cat $(DOCKERFILEPATH)/StagesForBuildInContainer >> Dockerfile
	docker build $(DOCKER_BUILD_OPTIONS) --build-arg IC_VERSION=$(VERSION)-$(GIT_COMMIT) -f Dockerfile -t $(PREFIX):$(TAG) .
else
	cat $(DOCKERFILEPATH)/StagesForLocalBuild >> Dockerfile
	docker build $(DOCKER_BUILD_OPTIONS) --build-arg IC_VERSION=$(VERSION)-$(GIT_COMMIT) -f Dockerfile -t $(PREFIX):$(TAG) .
endif

That is we're generating the Dockerfile based on the variable BUILD_IN_CONTAINER.

With this change, we will reduce the duplication in the Dockerfiles and also we will not require the latest docker version (I noticed you used DOCKER_BUILDKIT = 1 in the Makefile). My understanding is that you used DOCKER_BUILDKIT = 1 so that the stage FROM base AS local COPY nginx-ingress / is not executed when BUILD_IN_CONTAINER = 1. Is it correct?

Let me know what you think. Also @dboenig @Rulox

[pleshakov]

similar thing should be done to the test stage - run it inside the builder container, not here

[Rulox]

Looks nice @Dean-Coakley ! I have mixed feelings about your proposal @pleshakov.

On the one hand the removal of the buildkit is something that I like.
On the other hand, it seems we make things more complicated just to avoid a little bit of repetition (for instance, there are other parts of the Dockerfiles that we repeat normally, what happens with those? we keep repeating them?)

So I am not sure, let me have a deeper review and will be back to you guys (also @dboenig probably has something to say here)

[lucacome]

Looks good @Dean-Coakley . I'd argue we could have even fewer Dockerfiles because of the multistage approach :)

@pleshakov not sure how I feel about your proposed solution of creating the Dockerfile on the fly instead of having a static file, I think I prefer the latter because it's more straightforward.

@pleshakov @Rulox I personally really like buildkit, because it speeds up the building process (it can build concurrently, better caching, etc) and makes the multistage approach even better. Also, it's not really new so it doesn't require the latest version of Docker but just 18.09 or higher (18.09 was released in November 2018).

[lucacome]

I've never seen /proc/1/fd/1 being used instead of /dev/stdout in a Dockerfile and looks weird to me 😄. Any chance you could change it since you're modifying the files anyway? Also, the nginx base image already has the forward for error and access log, so we really need just the one for stream-access.log.

[Rulox]

I am ok with buildkit, I've never used it, so that's why I can't really have a strong opinion. If I have to choose between that and the multiple files and the cat >> in files I prefer buildkit, that's for sure. @lucacome @Dean-Coakley

[pleshakov]

@Dean-Coakley

Good job with the stages!

However, unfortunately, the PR drastically changes how the images and binaries are built:

For example, if you run:

$ make DOCKERFILE=DockerfileForPlus PREFIX=myregistry.example.com/nginx-plus-ingress BUILD_IN_CONTAINER=0

Previously:

1. Unit tests are run using local go env
2. The autogenerated code for CRDs is checked (verify-codegen)
3. The binary is built using local go
4. The IC image is built
5. The IC image is pushed to the registry

Now:

1. Unit tests are run in Docker container using shared volumes
2. The binary is built in a container in a stage
3. The IC image is built
4. The IC image is pushed to the registry

If you run (the default option of building in a container):

$ make DOCKERFILE=DockerfileForPlus PREFIX=myregistry.example.com/nginx-plus-ingress 

Previously:

1. Unit tests are run in Docker container using shared volumes
2. The binary is built in a container using shared volumes
3. The IC image is built
4. The IC image is pushed to the registry

Now:

1. Unit tests are run in Docker container using shared volumes
2. Docker binaries are build in a container in a stage
3. The IC image is built
4. The IC image is pushed to the registry

As you can see, both the developer workflow (building using local go) and the user workflow (building in Docker) has been changed. Mostly the developer workflow.

Given the task, I don't see why we need to make those backward incompatible changes. Could you revert targets and the flow while not changing the process? As part of that, let's keep the BUILD_IN_CONTAINER variable.

Note, the reason we're moving to using stages is because of several reports of problems when building an image using a container with shared volumes. However, the PR keeps using volumes - for running unit tests. I suggest removing unit testing for the case of building in acontainer - this is not necessary for IC users and will speed up the build process.

@lucacome thanks for the explanation about the build kit.

@Dean-Coakley thanks for making documentation changes. Make sure to also specified the minimal Docker version required for building the image because we started using the build kit.

Please also see some additional comments.

[pleshakov]

@Dean-Coakley thanks!

Please see my comments and suggestions.

It looks there is a bug. After I built the image with BUILD_IN_CONTAINER=1 and run it, I got:

│I0124 00:56:21.486111       1 main.go:169] Starting NGINX Ingress controller Version= GitCommit=

Version and GitCommit must be populated

[pleshakov]

thanks @Dean-Coakley !
please see my comments

[pleshakov]

👍