Skip to content

Conversation

@Rory-Z
Copy link
Contributor

@Rory-Z Rory-Z commented Sep 10, 2021

documentation PR: docker-library/docs#2032

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 ❤️)

  • associated with or contacted upstream?
  • available under an OSI-approved license?
    • Apache-2.0
  • does it fit into one of the common categories? ("service", "language stack", "base distribution")
    • service
  • is it reasonably popular, or does it solve a particular use case well?
  • does a documentation PR exist? (should be reviewed and merged at roughly the same time so that we don't have an empty image page on the Hub for very long)
  • official-images maintainer dockerization review for best practices and cache gotchas/improvements (ala the official review guidelines)?
  • 2+ official-images maintainer dockerization review?
  • 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?)
  • if FROM scratch, tarballs only exist in a single commit within the associated history?
  • passes current tests? any simple new tests that might be appropriate to add? (https://github.com/docker-library/official-images/tree/master/test)

@tianon
Copy link
Member

tianon commented Mar 25, 2022

Sorry for the delay! New image reviews usually take a lot more effort than image update review, so they often take a back seat (we're trying to be better about that). 🙇

Here's a few Dockerization review comments -- let me know if there's anything I can do to help with these or explain them better!


+        apt-transport-https \

This shouldn't be necessary with newer versions of APT -- I think even Buster was new enough, but if not, maybe considering Bullseye makes sense now? (Hopefully adding Bullseye to https://repos.emqx.io/emqx-ce/deb/debian/ is planned? 😅)


+        software-properties-common \
...
+    && add-apt-repository "deb https://repos.emqx.io/emqx-ce/deb/debian/ ./$(lsb_release -cs) stable" \

With this usage, this add-apt-repository is essentially just going to take that string verbatim and drop it into a new file in /etc/apt/sources.list.d. I would suggest dropping software-properties-common/add-apt-repository and just putting the appropriate contents directly into a file. Additionally, you already know which release you're in (FROM debian:buster-slim), so you can avoid lsb_release too if you wanted to.


+RUN curl -fsSL https://repos.emqx.io/gpg.pub | apt-key add - \

Using apt-key add is deprecated by the APT maintainers, and they prefer to see either a file in /etc/apt/trusted.gpg.d or better, a file elsewhere and the use of signed-by in the sources.list line (deb [ signed-by=/etc/apt/keyrings/something.gpg ] ...).

Also, downloading this key without any verification (from the same place the packages come from, no less 😬) is not going to be acceptable. What we'd suggest instead is either pulling the key by full fingerprint from a keyserver or downloading this flie, importing it into gpg, then exporting the key you're looking for by the full fingerprint into a file that's then used by APT.

See https://github.com/docker-library/mysql/blob/e4b225b3eed5c774fa11799f04832e0ad351da62/8.0/Dockerfile.debian#L54-L67 for a pretty straightforward example of that.


+LOCAL_IP=$(hostname -i | grep -oE '((25[0-5]|(2[0-4]|1[0-9]|[1-9]|)[0-9])\.){3}(25[0-5]|(2[0-4]|1[0-9]|[1-9]|)[0-9])' | head -n 1)

That's quite a complicated regular expression -- my best read of this is that it's trying to filter hostname -i output to just IPv4 addresses?


+fill_tuples() {

This function looks interesting, but it doesn't appear to be used anywhere -- just a leftover?

@Rory-Z
Copy link
Contributor Author

Rory-Z commented Mar 31, 2022

Hi, @tianon Thank you for your reply
I've updated the Dockerfile and entryponit scripts as you suggested, could you please review them again?

@tianon
Copy link
Member

tianon commented Apr 1, 2022

Looking at emqx/emqx-docker@7a24e0e...75d44b6 (but in my terminal so it can give me a better diff from 4.3.3/Dockerfile to 4.3/Dockerfile that GitHub isn't willing to for some reason 😅):


+RUN groupadd -r emqx && useradd -r -g emqx emqx

Given there's on-disk storage here, I'd suggest picking an explicit UID/GID here -- a common choice is to use the "default" or most common port number (since that's usually unused), but anything that isn't already taken by your current base image (and hopefully untaken by any base image you might use in the future, including just newer versions of Debian) should be fine (for example, 1000:1000 is a pretty safe choice).


+ENV EMQX_MAJOR 4.3
+ENV EMQX_VERSION 4.3.5

I would suggest moving these down to right before the RUN line where they're actually used so we can maximize the build cache.


Other than that, I don't see much else! This is looking good. 👍
(Once again, I really appreciate your patience with us! ❤️)

Signed-off-by: zhanghongtong <rory-z@outlook.com>
@Rory-Z
Copy link
Contributor Author

Rory-Z commented Apr 2, 2022

Hi, @tianon
I've assigned GID/UID for docker user, plz review again.
I'm looking forward to being part of official images, it will help us a lot
Thanks again

@github-actions
Copy link

github-actions bot commented Apr 6, 2022

Diff for 2776cb4:
diff --git a/_bashbrew-cat b/_bashbrew-cat
index bdfae4a..9995514 100644
--- a/_bashbrew-cat
+++ b/_bashbrew-cat
@@ -1 +1,7 @@
-Maintainers: New Image! :D (@docker-library-bot)
+Maintainers: Rory Z <rory-z@outlook.com> (@rory-z)
+GitRepo: https://github.com/emqx/emqx-docker.git
+GitFetch: refs/heads/main
+
+Tags: 4.3.5, 4.3, 4, latest
+GitCommit: 915d12f4a289def311fdc350f85539c74068bdb9
+Directory: 4.3
diff --git a/_bashbrew-list b/_bashbrew-list
index e69de29..e69f61b 100644
--- a/_bashbrew-list
+++ b/_bashbrew-list
@@ -0,0 +1,4 @@
+emqx:4
+emqx:4.3
+emqx:4.3.5
+emqx:latest
diff --git a/emqx_latest/Dockerfile b/emqx_latest/Dockerfile
new file mode 100644
index 0000000..54d918d
--- /dev/null
+++ b/emqx_latest/Dockerfile
@@ -0,0 +1,49 @@
+FROM debian:buster-slim
+
+RUN groupadd -r -g 1000 emqx && useradd -r -u 1000 -g emqx emqx
+
+RUN set -eu; \
+    apt-get update; \
+    apt-get install -y --no-install-recommends ca-certificates curl gnupg; \
+    rm -rf /var/lib/apt/lists/*
+
+RUN set -eu; \
+# gpg: key C0B409463E640D53: public key "emqx team <support@emqx.io>" imported
+    key='FC841BA637755CA8487B1E3CC0B409463E640D53'; \
+    export GNUPGHOME="$(mktemp -d)"; \
+    gpg --batch --keyserver keyserver.ubuntu.com --recv-keys "$key"; \
+    mkdir -p /etc/apt/keyrings; \
+    gpg --batch --export "$key" > /etc/apt/keyrings/emqx.gpg; \
+    gpgconf --kill all; \
+    rm -rf "$GNUPGHOME"
+
+ENV EMQX_VERSION 4.3.5
+RUN set -eu; \
+    echo "deb [signed-by=/etc/apt/keyrings/emqx.gpg] https://repos.emqx.io/emqx-ce/deb/debian/ ./buster stable" >> /etc/apt/sources.list.d/emqx.list; \
+    apt-get update; \
+    apt-get install -y --no-install-recommends emqx="$EMQX_VERSION"; \
+    rm -rf /var/lib/apt/lists/*
+
+USER emqx
+
+VOLUME ["/opt/emqx/log", "/opt/emqx/data"]
+
+# emqx will occupy these port:
+# - 1883 port for MQTT
+# - 8081 for mgmt API
+# - 8083 for WebSocket/HTTP
+# - 8084 for WSS/HTTPS
+# - 8883 port for MQTT(SSL)
+# - 11883 port for internal MQTT/TCP
+# - 18083 for dashboard
+# - 4369 epmd (Erlang-distrbution port mapper daemon) listener (deprecated)
+# - 4370 default Erlang distrbution port
+# - 5369 for gen_rpc port mapping
+# - 6369 6370 for distributed node
+EXPOSE 1883 8081 8083 8084 8883 11883 18083 4369 4370 5369 6369 6370
+
+COPY docker-entrypoint.sh /usr/bin/
+
+ENTRYPOINT ["/usr/bin/docker-entrypoint.sh"]
+
+CMD ["emqx", "foreground"]
diff --git a/emqx_latest/docker-entrypoint.sh b/emqx_latest/docker-entrypoint.sh
new file mode 100755
index 0000000..6502310
--- /dev/null
+++ b/emqx_latest/docker-entrypoint.sh
@@ -0,0 +1,44 @@
+#!/bin/bash
+## EMQ docker image start script
+# Huang Rui <vowstar@gmail.com>
+# EMQ X Team <support@emqx.io>
+
+## Shell setting
+if [[ -n "$DEBUG" ]]; then
+    set -ex
+else
+    set -e
+fi
+
+shopt -s nullglob
+
+if [[ -z "$EMQX_NODE_NAME" ]]; then
+
+    if [[ -z "$EMQX_NAME" ]]; then
+        EMQX_NAME="$(hostname)"
+    fi
+
+    if [[ -z "$EMQX_HOST" ]]; then
+        LOCAL_IP=$(hostname -i)
+        if [[ "$EMQX_CLUSTER__K8S__ADDRESS_TYPE" == "dns" ]] && [[ -n "$EMQX_CLUSTER__K8S__NAMESPACE" ]]; then
+            EMQX_CLUSTER__K8S__SUFFIX=${EMQX_CLUSTER__K8S__SUFFIX:-"pod.cluster.local"}
+            EMQX_HOST="${LOCAL_IP//./-}.$EMQX_CLUSTER__K8S__NAMESPACE.$EMQX_CLUSTER__K8S__SUFFIX"
+        elif [[ "$EMQX_CLUSTER__K8S__ADDRESS_TYPE" == 'hostname' ]] && [[ -n "$EMQX_CLUSTER__K8S__NAMESPACE" ]]; then
+            EMQX_CLUSTER__K8S__SUFFIX=${EMQX_CLUSTER__K8S__SUFFIX:-'svc.cluster.local'}
+            EMQX_HOST=$(grep -h "^$LOCAL_IP" /etc/hosts | grep -o "$(hostname).*.$EMQX_CLUSTER__K8S__NAMESPACE.$EMQX_CLUSTER__K8S__SUFFIX")
+        else
+            EMQX_HOST="$LOCAL_IP"
+        fi
+    fi
+    export EMQX_NODE_NAME="$EMQX_NAME@$EMQX_HOST"
+    unset EMQX_NAME
+    unset EMQX_HOST
+fi
+
+# The default rpc port discovery 'stateless' is mostly for clusters
+# having static node names. So it's troulbe-free for multiple emqx nodes
+# running on the same host.
+# When start emqx in docker, it's mostly one emqx node in one container
+export EMQX_RPC__PORT_DISCOVERY="${EMQX_RPC__PORT_DISCOVERY:-manual}"
+
+exec "$@"

@Rory-Z Rory-Z requested a review from tianon April 6, 2022 01:45
@Rory-Z
Copy link
Contributor Author

Rory-Z commented Apr 6, 2022

Hi, @tianon
Thanks for your comment

@yosifkit yosifkit merged commit acb2541 into docker-library:master Apr 7, 2022
@Rory-Z
Copy link
Contributor Author

Rory-Z commented Apr 7, 2022

Hi,@tianon, @yosifkit Thanks for your work.
How long will it take to pull this image from the docker hub?

@tianon
Copy link
Member

tianon commented Apr 7, 2022

Sorry for the delay - forgot about one additional bit we had to do manually to get this up. It's now available to pull, but will take a bit of time before https://hub.docker.com/_/emqx exists - in the meantime, you can check https://hub.docker.com/r/amd64/emqx to get an idea of how the documentation will render once it does. 👍

@Rory-Z
Copy link
Contributor Author

Rory-Z commented Apr 8, 2022

So cooool,now I get image for https://hub.docker.com/_/emqx,this's great !

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.

3 participants