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.

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

Add API-Firewall official image #11100

Merged
merged 11 commits into from
Feb 2, 2022
Merged

Add API-Firewall official image #11100

merged 11 commits into from
Feb 2, 2022

Conversation

afr1ka
Copy link
Contributor

@afr1ka afr1ka commented Oct 13, 2021

Hello,

We would like to add the API-Firewall to the official image repository.

Thanks.

Checklist for Review

NOTE: This checklist is intended for the use of the Official Images maintainers both to track the status of your PR and to help inform you and others of where we're at. As such, please leave the "checking" of items to the repository maintainers. If there is a point below for which you would like to provide additional information or note completion, please do so by commenting on the PR. Thanks! (and thanks for staying patient with us ❤️)

Copy link

@d0znpp d0znpp left a comment

Choose a reason for hiding this comment

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

reviewed

@d0znpp
Copy link

d0znpp commented Nov 4, 2021

  • does it fit into one of the common categories? ("service", "language stack", "base distribution") - base distribution
  • is it reasonably popular, or does it solve a particular use case well? - API firewall have 500M+ docker pulls already, according to the docker hub stats https://hub.docker.com/r/wallarm/api-firewall
  • passes current tests? any simple new tests that might be appropriate to add? (https://github.com/docker-library/official-images/tree/master/test) - all the tests passed
  • existing official images have been considered as a base? (ie, if foobar needs Node.js, has FROM node:... instead of grabbing node via other means been considered?) - yes, it's based on an alpine:3.14

@afr1ka
Copy link
Contributor Author

afr1ka commented Nov 4, 2021

Documentation PR at docker-library/docs#2074

@d0znpp
Copy link

d0znpp commented Nov 11, 2021

Hi Docker team!

Could you please advise us on how to meet this requirement?

  • official-images maintainer dockerization review for best practices and cache gotchas/improvements (ala the official review guidelines)?

@d0znpp
Copy link

d0znpp commented Nov 15, 2021

@tianon, could you please have a look?

@d0znpp
Copy link

d0znpp commented Dec 8, 2021

API firewall got 1B (!!!) docker pulls. But it's not an official image yet.

@tianon
Copy link
Member

tianon commented Dec 8, 2021

Sorry for the delay 😩 🙇

I guess my first question is whether API Firewall is free/open source software (https://github.com/docker-library/official-images#what-are-official-images)?

From what I can tell/find, it appears to be freely available, but not open source?

@d0znpp
Copy link

d0znpp commented Dec 8, 2021 via email

@d0znpp
Copy link

d0znpp commented Jan 6, 2022

Happy New Year! Our pull request survived through 3 seasons on a Planet Earth in 3 months already.

@tianon
Copy link
Member

tianon commented Jan 6, 2022

Indeed, apologies 🙇

Unfortunately, we've had to enforce a harder line on the Open Source qualification than we've done in the past, so in its current form, I don't think API Firewall is something we can accept in this program, but I think we should revisit as soon as you've got it open sourced!

@d0znpp
Copy link

d0znpp commented Jan 6, 2022

Does it mean that you will drop all the previously approved images from the registry?

@tianon
Copy link
Member

tianon commented Jan 7, 2022

To be clear, I'm only talking about images that are part of the "Official Images" program, ala images accepted into https://github.com/docker-library/official-images -- this does not apply to images on Docker Hub at large. For existing images which have been accepted here already which don't meet that requirement (of which there are only a handful), that's going to be a conversation between the image maintainers and Docker Inc (and unfortunately I can't comment much further than that on those).

@d0znpp
Copy link

d0znpp commented Jan 7, 2022

But it was not there when we applied this request. It technically means that we are in the same position as other non-OSS projects approved before. We spent a lot of time on this project to drop the ball now after 3 months of the review.

@d0znpp
Copy link

d0znpp commented Jan 24, 2022

@tianon we released the API firewall to OSS completely https://github.com/wallarm/api-firewall. Please proceed with the official repository.

@d0znpp
Copy link

d0znpp commented Jan 27, 2022

@tianon please check our project. The PR became huge after releasing the source code and we fixed it by moving from Docker file to docker-entrypoint.sh

The pipeline should be good now, please recheck manually.

@tianon
Copy link
Member

tianon commented Jan 27, 2022

Unfortunately having the Dockerfile in the root of the upstream repository (while useful for local development) doesn't work well as the target for official-images. Since the Dockerfile uses COPY . ..., it essentially means the entire upstream source code is now part of the Docker build context, and thus becomes part of the reviewed artifacts (see diff-pr.sh for how we generate our diff; it is basically taking all docker build contexts from the starting library/foo file and diffing them to the new contexts from the PR).

What we suggest instead is creating a separate repository to hold the "packaging", especially since you might have, for example, changes to make to the Dockerfile in between upstream releases of the software.

@d0znpp
Copy link

d0znpp commented Jan 27, 2022

Do you have any examples of such projects with separate repositories for packaging?

@tianon
Copy link
Member

tianon commented Jan 28, 2022

I would suggest that golang is probably an example whose Dockerfile is going to be reasonably similar to what you'll want here -- it appears that the API Firewall project publishes official binaries for a variety of architectures, so I would suggest that the Dockerfile for consuming it should be a reasonably straightforward one.

Conceptually, I would expect to see something like, FROM some-base, RUN detect-arch ("dpkg --print-architecture" or "apk --print-arch" are useful utilities for detecting the userspace architecture) + download appropriate release artifact + verify with some Dockerfile-embedded checksum like sha256 + install in appropriate place within the image + CMD ["some-reasonable-default"].

If you want an example that's similar but much simpler than the golang example, you might look at julia:
https://github.com/docker-library/julia/blob/fe407643a5e54f54063d8d67d487cf7997ded40f/1.7/bullseye/Dockerfile

@d0znpp
Copy link

d0znpp commented Jan 28, 2022

I would suggest that golang is probably an example whose Dockerfile is going to be reasonably similar to what you'll want here -- it appears that the API Firewall project publishes official binaries for a variety of architectures, so I would suggest that the Dockerfile for consuming it should be a reasonably straightforward one.

Conceptually, I would expect to see something like, FROM some-base, RUN detect-arch ("dpkg --print-architecture" or "apk --print-arch" are useful utilities for detecting the userspace architecture) + download appropriate release artifact + verify with some Dockerfile-embedded checksum like sha256 + install in appropriate place within the image + CMD ["some-reasonable-default"].

If you want an example that's similar but much simpler than the golang example, you might look at julia: https://github.com/docker-library/julia/blob/fe407643a5e54f54063d8d67d487cf7997ded40f/1.7/bullseye/Dockerfile

Thank you! It seems like we got it.

@d0znpp
Copy link

d0znpp commented Jan 31, 2022

@tianon we passed all the checks. Please approve pull request

@tianon
Copy link
Member

tianon commented Feb 1, 2022

Thanks for the update! Overall, this looks much better, and is really close. I've got two very minor comments -- the first is just a suggestion for improvement, but the second is something that'll need to be fixed before we merge:


+ENV APIFIREWALL_VERSION v0.6.7
+
+ENV APIFW_PATH /opt/api-firewall
+ENV PATH $APIFW_PATH:$PATH
+
+RUN set -eux; \
+    adduser -u 1000 -H -h /opt -D -s /bin/sh api-firewall

I would suggest swapping these for better utilization of the Docker build cache, ala:

ENV APIFW_PATH /opt/api-firewall
ENV PATH $APIFW_PATH:$PATH

RUN set -eux; \
    adduser -u 1000 -H -h /opt -D -s /bin/sh api-firewall

ENV APIFIREWALL_VERSION v0.6.7

+# smoke test
+    api-firewall -v
+
+COPY docker-entrypoint.sh $APIFW_PATH/
+
+RUN chmod 755 $APIFW_PATH/api-firewall; \
+    chmod 755 $APIFW_PATH/docker-entrypoint.sh

The api-firewall -v smoke test already verifies that the api-firewall binary itself is executable, so I don't think that chmod is actually necessary, but if it is, it should be up in the layer which added said api-firewall binary (otherwise it'll create a copy of that binary in the lower layer, just to change the permissions).

The other half of this (docker-entrypoint.sh) I would suggest fixing by setting the executable bit on docker-entrypoint.sh in Git instead so that COPY includes it natively. If you need a PR for this (due to being on Windows or something where doing chmod +x in Git is difficult to accomplish appropriately), I'm happy to open one. 👍

@tianon
Copy link
Member

tianon commented Feb 1, 2022

Oh! Just noticed one more (only suggestion) item that's worth considering:

+if [ "$1" = 'api-firewall' ]; then
+	shift # "api-firewall"
+	set -- api-firewall "$@"
+fi

This entire if block amounts to "if the arguments start with api-firewall, remove it, then add it back", so I think the whole block could just be removed entirely. 😄

(Again, happy to send a PR with all of these suggestions if that's helpful.)

@afr1ka
Copy link
Contributor Author

afr1ka commented Feb 1, 2022

@tianon thank you for the suggestions. We have updated docs and docker repos.

@github-actions
Copy link

github-actions bot commented Feb 1, 2022

Diff for 4cdfb34:
diff --git a/_bashbrew-cat b/_bashbrew-cat
index bdfae4a..300e853 100644
--- a/_bashbrew-cat
+++ b/_bashbrew-cat
@@ -1 +1,8 @@
-Maintainers: New Image! :D (@docker-library-bot)
+Maintainers: Ivan Novikov <in@wallarm.com> (@d0znpp), Nikolay Tkachenko (@afr1ka)
+GitRepo: https://github.com/wallarm/api-firewall-docker.git
+
+Tags: 0.6.7, latest
+Architectures: amd64, arm64v8, i386
+GitFetch: refs/heads/main
+GitCommit: 5cd07f115f16d10095a385a73568a4e60caf278b
+Directory: 0.6.7
diff --git a/_bashbrew-list b/_bashbrew-list
index e69de29..93f9600 100644
--- a/_bashbrew-list
+++ b/_bashbrew-list
@@ -0,0 +1,2 @@
+api-firewall:0.6.7
+api-firewall:latest
diff --git a/api-firewall_latest/Dockerfile b/api-firewall_latest/Dockerfile
new file mode 100644
index 0000000..db8e014
--- /dev/null
+++ b/api-firewall_latest/Dockerfile
@@ -0,0 +1,51 @@
+FROM alpine:3.15
+
+ENV APIFW_PATH /opt/api-firewall
+ENV PATH $APIFW_PATH:$PATH
+
+RUN set -eux; \
+    adduser -u 1000 -H -h /opt -D -s /bin/sh api-firewall
+
+ENV APIFIREWALL_VERSION v0.6.7
+
+RUN set -eux; \
+    \
+    apk add --no-cache wget; \
+    \
+    arch="$(apk --print-arch)"; \
+    case "$arch" in \
+        'x86_64') \
+            url="https://github.com/wallarm/api-firewall/releases/download/${APIFIREWALL_VERSION}/api-firewall-amd64-musl.tar.gz"; \
+            sha256='fb61a3ca620340a76f97856ec30190de8422535c31d17ddc89019e87825d60b9'; \
+            ;; \
+        'aarch64') \
+            url="https://github.com/wallarm/api-firewall/releases/download/${APIFIREWALL_VERSION}/api-firewall-arm64-musl.tar.gz"; \
+            sha256='cdcd6679536e3e0fa0d5cdd8f422ef58f31f65c2d279228150c269c32641dd1d'; \
+            ;; \
+        'x86') \
+            url="https://github.com/wallarm/api-firewall/releases/download/${APIFIREWALL_VERSION}/api-firewall-386-musl.tar.gz"; \
+            sha256='0850d87b52624085265554605f46df5cda2a006d7b9dc9a7d369cdb9c9c88305'; \
+            ;; \
+        *) \
+            echo >&2 "error: current architecture ($arch) does not have a corresponding API-Firewall binary release"; \
+            exit 1; \
+            ;; \
+    esac; \
+    \
+    wget -O api-firewall.tar.gz "$url"; \
+    echo "$sha256 *api-firewall.tar.gz" | sha256sum -c; \
+    \
+    mkdir -p "$APIFW_PATH"; \
+    tar -xzf api-firewall.tar.gz -C "$APIFW_PATH" --strip-components 1; \
+    rm api-firewall.tar.gz; \
+    \
+    chmod 755 $APIFW_PATH/api-firewall; \
+    \
+# smoke test
+    api-firewall -v
+
+COPY docker-entrypoint.sh $APIFW_PATH/
+
+USER api-firewall
+ENTRYPOINT ["docker-entrypoint.sh"]
+CMD ["api-firewall"]
diff --git a/api-firewall_latest/docker-entrypoint.sh b/api-firewall_latest/docker-entrypoint.sh
new file mode 100755
index 0000000..9e57b4b
--- /dev/null
+++ b/api-firewall_latest/docker-entrypoint.sh
@@ -0,0 +1,11 @@
+#!/bin/sh
+set -e
+
+# this if will check if the first argument is a flag
+# but only works if all arguments require a hyphenated flag
+# -v; -SL; -f arg; etc will work, but not arg1 arg2
+if [ "$#" -eq 0 ] || [ "${1#-}" != "$1" ]; then
+    set -- api-firewall "$@"
+fi
+
+exec "$@"

@d0znpp

This comment has been minimized.

@yosifkit yosifkit merged commit c8e11dc into docker-library:master Feb 2, 2022
@d0znpp
Copy link

d0znpp commented Feb 3, 2022

Hello @tianon , can you please advise us on what to do with the initial repository https://hub.docker.com/r/wallarm/api-firewall? Should we close it? How to move stats from there to an official one?

@tianon
Copy link
Member

tianon commented Feb 4, 2022

Regarding whether to close it -- that's up to you. Generally, we suggest that you add some kind of deprecation notice to it and maybe even update it with a copy of the built official image copy for a short time so that users can have some kind of transition period, but even that's up to you. Some maintainers like to use a repository like that for pre-releases or for letting users test changes before they push them into the official image.

Regarding the stats, unfortunately that's not something we have access to / visibility of / control over. If you already have a contact at Docker Inc, I'd suggest asking them whether copying the stats is possible (I think other maintainers have tried in the past and there were technical limitations to doing so, but I might be misremembering or that might no longer be the case).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants