Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor Dockerfile and Makefile #1241

Merged
merged 11 commits into from
Mar 2, 2021
Merged

Refactor Dockerfile and Makefile #1241

merged 11 commits into from
Mar 2, 2021

Conversation

lucacome
Copy link
Member

Proposed changes

  • Use only one Dockerfile (for almost all images) so we can reuse layers.
  • Different make targets for the images
  • Added help to Makefile

@lucacome lucacome self-assigned this Nov 19, 2020
@lucacome
Copy link
Member Author

If this looks good the docs will be updated.

@mikestephen
Copy link

Overall, looks very clean indeed!

@lucacome lucacome closed this Nov 19, 2020
@lucacome lucacome reopened this Nov 19, 2020
Base automatically changed from update-crd-gen to master November 20, 2020 16:19
Dean-Coakley
Dean-Coakley reviewed Nov 20, 2020
View reviewed changes
Copy link
Contributor

@Dean-Coakley Dean-Coakley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great 😍

Just some typos and one suggestion/question.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
mikestephen
mikestephen approved these changes Dec 2, 2020
View reviewed changes
Copy link

@mikestephen mikestephen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some really good changes here, looks much cleaner!

Dean-Coakley
Dean-Coakley reviewed Dec 2, 2020
View reviewed changes
Copy link
Contributor

@Dean-Coakley Dean-Coakley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I think you can proceed with the docs.

.dockerignore Show resolved Hide resolved
pleshakov
pleshakov reviewed Dec 4, 2020
View reviewed changes
build/Dockerfile Outdated Show resolved Hide resolved
@pleshakov pleshakov mentioned this pull request Dec 4, 2020
Closed
6 tasks
@Dean-Coakley
Copy link
Contributor

Can you resolve the mixed indentation introduced in 095a751 in this PR?

@lucacome lucacome force-pushed the cleanup-makefile branch 2 times, most recently from 69c3823 to a85fad3 Compare February 4, 2021 04:07
@lucacome
Copy link
Member Author

lucacome commented Feb 4, 2021

I think the nginx-repo.* are encoded right @ciarams87 ? Can we change it now that the action supports multiline secrets?

@ciarams87
Copy link
Member

I think the nginx-repo.* are encoded right @ciarams87 ? Can we change it now that the action supports multiline secrets?

Sure, @lucacome I can change the secrets. To allow for a transition though, it would probably be better to create new, unencoded secrets first, then I can change the names of the secrets back in a later PR. I'll let you know when I have that done.

Also, the same changes are going to have to be added to the nightly.yml file too.

Dean-Coakley
Dean-Coakley approved these changes Feb 5, 2021
View reviewed changes
Copy link
Contributor

@Dean-Coakley Dean-Coakley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good.

Seeing as NGINX Plus supports Alpine since R17 is there anything stopping us from making our Plus image Alpine based instead of Debian? - Just wondering, could be done in a separate PR.

@lucacome lucacome force-pushed the cleanup-makefile branch 4 times, most recently from 04bf974 to ed0be6c Compare February 10, 2021 18:56
@github-actions github-actions bot added dependencies Pull requests that update a dependency file documentation Pull requests/issues for documentation labels Mar 1, 2021
jputrino
jputrino approved these changes Mar 1, 2021
View reviewed changes
Copy link
Contributor

@jputrino jputrino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a few suggestions

@pleshakov pleshakov added the change Pull requests that introduce a change label Mar 1, 2021
pleshakov
pleshakov reviewed Mar 1, 2021
View reviewed changes
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lucacome

Could we make the default TARGET=container. Otherwise, if you just follow instructions here https://deploy-preview-1241--nginx-kubernetes-ingress.netlify.app/nginx-ingress-controller/installation/building-ingress-controller-image/#building-the-image-and-pushing-it-to-the-private-registry , you will get :

$ make debian-image-plus PREFIX=myregistry.example.com/nginx-plus-ingress
Docker version 20.10.4, build d3cb89e
make: go: Command not found
make: *** [Makefile:51: build] Error 127

Note that previously we had build in container enabled as default. This is for users/customers, as we don't expect them to have the go installed/configured on their systems.

Also I added the "change" label to this PR, as it introduces two changes:

  1. changes in Makefile - customers will need to use different command and arguments to build the images
  2. changes in Dockerfiles - customers who customized their Dockerfiles will have to migrate their customizations to the new Dockerfile.

Those changes will have to be reflected in our CHANELOG. Could you update the description of this PR with upgrade notes related to the two changes? This way we will insert them into our CHANGELOG into the UPGRADE section.

Also, I left a number of additional comments

build/Dockerfile Outdated Show resolved Hide resolved
build/Dockerfile
&& apt-get update \
&& apt-get install --no-install-recommends --no-install-suggests -y ca-certificates gnupg wget \
&& wget https://nginx.org/keys/nginx_signing.key \
&& gpg --no-default-keyring --keyring nginx_keyring.gpg --import nginx_signing.key \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to double check, are these instructions https://github.com/nginxinc/kubernetes-ingress/blob/master/build/DockerfileForPlus#L22-L31 no longer necessary?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

build/Dockerfile


RUN mkdir licenses
COPY --chown=nginx:0 LICENSE /licenses
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this mean we will expose our license in all OpenShift images, including the ones for OSS that we publish on DockerHub?
also, should we mount it as NGINX Plus key ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no this is just the apache license

build/Dockerfile Outdated Show resolved Hide resolved
build/Dockerfile Outdated Show resolved Hide resolved
build/Dockerfile
--mount=type=secret,id=nginx-repo.key,dst=/etc/ssl/nginx/nginx-repo.key,mode=0644 \
apt-get update && apt-get install -y apt-transport-https libcap2-bin nginx-plus=${NGINX_PLUS_VERSION} nginx-plus-module-njs=${NGINX_NJS_VERSION} \
&& apt-get purge --auto-remove -y apt-transport-https \
&& rm -rf /var/lib/apt/lists/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about removing files /etc/apt/apt.conf.d/90nginx /etc/apt/sources.list.d/nginx-plus.list like we do here https://github.com/nginxinc/kubernetes-ingress/blob/master/build/DockerfileForPlus#L46 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they're needed in the ap stage

build/Dockerfile Show resolved Hide resolved
build/Dockerfile Outdated Show resolved Hide resolved
docs-web/installation/building-ingress-controller-image.md Outdated Show resolved Hide resolved
tests/suite/resources_utils.py
@@ -22,6 +22,7 @@ class RBACAuthorization:
role (str): cluster role name
binding (str): cluster role binding name
"""

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like irrelevant changes for this PR

lucacome and others added 4 commits March 2, 2021 11:43
@lucacome
feedback
7c1657d
@lucacome @pleshakov
Apply suggestions from code review
0feb0f1 
Co-authored-by: Michael Pleshakov <pleshakov@users.noreply.github.com>
@lucacome
update docs
d17a480
@lucacome
Add error messages to makefile
e7764fe
@lucacome
Copy link
Member Author

lucacome commented Mar 2, 2021

Could we make the default TARGET=container.

Note that previously we had build in container enabled as default. This is for users/customers, as we don't expect them to have the go installed/configured on their systems.

I've added a note in the docs and errors in the Makefile, to use TARGET=container if go is not installed. I don't think the default should be to build in a container (much slower) and for the devs to have to add an additional parameter every time. Also, the builds and tests all have the binary built locally, so it's more consistent imo.

@lucacome lucacome changed the title Consolidate Dockerfile and Makefile Refactor Dockerfile and Makefile Mar 2, 2021
@lucacome lucacome merged commit a24f837 into master Mar 2, 2021
@lucacome lucacome deleted the cleanup-makefile branch March 2, 2021 23:12
@TechIsCool
Copy link

https://docs.nginx.com/nginx-ingress-controller/third-party-modules/opentracing/
https://www.nginx.com/blog/opentracing-with-nginx-ingress-controller-for-kubernetes/#Building-OpenTracing-into-the-NGINX-Ingress-Controller-Image

A couple places for Open Tracing need docs updated.

@lucacome
Copy link
Member Author

Hi @TechIsCool

thanks for pointing this out, the documentation is for https://github.com/nginxinc/kubernetes-ingress/releases/tag/v1.10.0. This change will go in v1.11.0 so it will be updated once we release that. Unfortunately, we don't have documentation for the edge version (master) at the moment.

I'll see if we can update that blog post once we release v1.11.0.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pleshakov pleshakov pleshakov left review comments

@mikestephen mikestephen mikestephen approved these changes

@jputrino jputrino jputrino approved these changes

@Dean-Coakley Dean-Coakley Dean-Coakley approved these changes

@ciarams87 ciarams87 ciarams87 approved these changes

@vepatel vepatel vepatel approved these changes

Assignees

@lucacome lucacome

Labels
change Pull requests that introduce a change chore Pull requests for routine tasks dependencies Pull requests that update a dependency file documentation Pull requests/issues for documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@lucacome @mikestephen @Dean-Coakley @ciarams87 @TechIsCool @pleshakov @jputrino @vepatel