KAFKA-16373: KIP-1028: Addressing Docker Official Images PR Comments for JVM, Native and Docker Official Images #16664

KrishVora2912 · 2024-07-23T08:04:55Z

This PR aims to add address the comments added via the docker hub folks regarding the JVM based Docker Official Image for Apache Kafka introduced in the following KIP - https://cwiki.apache.org/confluence/display/KAFKA/KIP-1028%3A+Docker+Official+Image+for+Apache+Kafka . The comments by the dockerhub folks can be found here .

This PR:
Addresses comments 1,3,4,5,6,7,8,9 mentioned in the above link.

NOTE: The changes have been made to both the JVM docker images introduced via KIP-975 as well as the GraalVM docer images introduced via KIP-974. This ensures that the docker hub suggested best practices will now be used for both the OSS sponsored images as well as the Docker Official Images.

Gist:

USER root is unnecessary - removed this
apk update is unnecessary with --no-cache - removed this
apk upgrade is not allowed - removed this
Specific, individual GPG keys should be referenced by ID. - new approach uses hardcoded GPG keys
All invocations of gpg should include --batch - this has been adhered to
The kafka tarball should be verified prior to being unpacked - change has been made
The DOI build system sets appropriate annotations, so we recommend not setting labels, which have been deprecated. - deprecated labels have been removed, and now annotations are used
Is apk cache clean necessary since you have previously specified --no-cache? - removed this

Post this PR:

Propogate these modifications to the 3.7.0 docker official images Dockerfile.
Modify existing PR with PR template generation script for 3.7.0 under docker official images repo.
Update release process to update version_keys file for each new release.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

VedarthConfluent · 2024-07-24T10:51:49Z

docker/version_keys.py

@@ -0,0 +1,5 @@
+version_keys = {


Can we use json file instead. I don't think we should be creating a separate python file for this

I used a python file for 2 main reasons

Simple json file has the overhead of opening the JSON file and loading its contents into a Python object, and use it further (boils down to the same as using a python dict)

Secondly, if for unsupported versions, we might want to remove GPG_KEYS, working with python dicts would be much simpler for any automation script we add.

LMK if these sound valid justifications for using a python dict as a new python file.

I think we should use json if we are using a separate file. It's just a more standard way of maintaining data like this imo. Overhead would be minimal imo.

This change has been made

VedarthConfluent · 2024-07-24T10:58:28Z

docker/jvm/Dockerfile

-    wget -nv -O KEYS https://downloads.apache.org/kafka/KEYS; \
-    gpg --import KEYS; \
+    for server in ha.pool.sks-keyservers.net $(shuf -e \
+                          hkp://p80.pool.sks-keyservers.net:80 \


Can we try using https://downloads.apache.org/kafka/KEYS for verification

made this change

This change does not work for all versions, so reverting to the original server list approach.

Can we confirm that the list of key servers is going to cover all the GPG keys created in future? If not can we have a mechanism for the RM to add their GPG key server, if it's not present in the list.

Sure, we will add this to the documentation as part of the release process.

VedarthConfluent · 2024-07-24T10:59:13Z

docker/jvm/Dockerfile

-    wget -nv -O KEYS https://downloads.apache.org/kafka/KEYS; \
-    gpg --import KEYS; \
+    for server in ha.pool.sks-keyservers.net $(shuf -e \
+                          hkp://p80.pool.sks-keyservers.net:80 \


Can we try using https://downloads.apache.org/kafka/KEYS for verification

made this change

This change does not work for all versions, so reverting to the original server list approach.

The guys from docker hub official won't like external download dependencies - we had that discussion for Storm a few months ago. We workaround that for Apache Storm here: https://github.com/apache/storm-docker/blob/master/automation/create-key-section.sh and here https://github.com/apache/storm-docker/blob/master/2.6.3-jre17/Dockerfile#L40 - might also an option for Kafka ;-)

Hi @rzo1 ! Thanks for these suggestions!

A few small queries here:

The guys from docker hub official won't like external download dependencies

By this you mean that they wont approve of something along the lines of

wget -nv -O KEYS https://downloads.apache.org/kafka/KEYS; \ gpg --import KEYS;

correct?
We did receive a comment from the Dockerhub folks regarding this (see Point 5 here).

So we went along and changed the above approach. Now we pass the GPG_KEY as an argument/environment variable, and then use something along the lines of this.

I went through the approach followed by Storm, and that seems like a great way to approach this too. However, for Kafka, there are a lot of existing keys that are returned (see attached image), which might just inflate the length of the Dockerfile (hence we decided with the arg/env approach).

In your experience, do you think the modified approach (here, here and here) would be liked by the Dockerhub folks, or with the discussion you had for Storm, do you anticipate any issue in this approach too?

Thanks a lot again!
Krish.

I think they will just complain, if they don't like it :-) - for Storm, we had one iteration.

omkreddy · 2024-07-25T06:40:27Z

docker/version_keys.py

@@ -0,0 +1,5 @@
+version_keys = {


maybe rename to version_gpg_keys

missing license

Can we add comment to the file about the format of this file.

Thanks for the review @omkreddy .

Renamed this to version_gpg_keys, the name now better reflects what the file stands for.
2, 3: Sorry, missed out on adding these! Added the same to the scripts.
Thanks!

This reverts commit f027b78.

This reverts commit 1f589a5.

VedarthConfluent

lgtm

github-actions · 2025-01-06T03:39:05Z

This PR is being marked as stale since it has not had any activity in 90 days. If you
would like to keep this PR alive, please leave a comment asking for a review. If the PR has
merge conflicts, update it with the latest from the base branch.

If you are having difficulty finding a reviewer, please reach out on the [mailing list](https://kafka.apache.org/contact).

If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed.

KrishVora01 · 2025-01-09T11:51:23Z

This PR is to be kept open as Docker Official Image PR is waiting on review from Dockerhub folks.

KrishVora2912 added 8 commits July 22, 2024 16:55

adding doi pr changes

67cea41

3.7.0 testing with doi pr changes

11521ab

adding 3.8.0 key

4f68262

testing on local

2c2b29a

bringing back commented lines in workflow

dfd1f19

3.7.0 DOI Dockerfile revert to original state

fee1e30

adding newline at end of 3.7.0 doi dockerfile

1de2621

using annotations in place of labels

8da6e72

VedarthConfluent reviewed Jul 24, 2024

View reviewed changes

KrishVora2912 added 2 commits July 24, 2024 17:39

using apache keyserver as keyserver

af07079

making ubuntu as default keyserver

947ce53

omkreddy reviewed Jul 25, 2024

View reviewed changes

KrishVora2912 added 8 commits July 25, 2024 12:23

addressing PR comments -> version_keys to version_gpg_keys

372dacb

reverting faulty import

d21dff3

adding servers as server list insted of keys.apache.org due to failures

ab4a0bf

changing version_gpg_keys to json file as per PR comments

a82c1fc

testing pushing

1f589a5

3.7.0 doi testing, will be reverted

f027b78

Revert "3.7.0 doi testing, will be reverted"

6d8615f

This reverts commit f027b78.

Revert "testing pushing"

321fd4d

This reverts commit 1f589a5.

VedarthConfluent approved these changes Aug 1, 2024

View reviewed changes

KrishVora2912 added 7 commits August 13, 2024 16:07

using PR comments for verification of packages

a63adc4

Fixing few issues, changing order of keyservers

d08cf6a

fixing gpg url error

89127e0

reverting to test

3d3c915

Merge branch 'trunk' of https://github.com/apache/kafka into trunk

5c6bfc4

Checking docker compose

53c75cb

Trying compose v2

5b54175

KrishVora2912 added 2 commits August 13, 2024 19:30

Using new gpg commands for native image too

92ec952

Adding extra \n at the end

2e5a354

github-actions bot added the stale Stale PRs label Jan 6, 2025

github-actions bot removed the stale Stale PRs label Jan 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-16373: KIP-1028: Addressing Docker Official Images PR Comments for JVM, Native and Docker Official Images #16664

KAFKA-16373: KIP-1028: Addressing Docker Official Images PR Comments for JVM, Native and Docker Official Images #16664

KrishVora2912 commented Jul 23, 2024 •

edited

Loading

VedarthConfluent Jul 24, 2024

KrishVora01 Jul 24, 2024

VedarthConfluent Jul 31, 2024

KrishVora01 Jul 31, 2024

VedarthConfluent Jul 24, 2024

KrishVora01 Jul 24, 2024

KrishVora2912 Jul 25, 2024

VedarthConfluent Jul 31, 2024

KrishVora01 Jul 31, 2024

VedarthConfluent Jul 24, 2024

KrishVora01 Jul 24, 2024

KrishVora2912 Jul 25, 2024

rzo1 Jul 26, 2024 •

edited

Loading

KrishVora2912 Jul 28, 2024 •

edited

Loading

rzo1 Aug 9, 2024

omkreddy Jul 25, 2024

KrishVora2912 Jul 25, 2024

VedarthConfluent left a comment

github-actions bot commented Jan 6, 2025

KrishVora01 commented Jan 9, 2025

KAFKA-16373: KIP-1028: Addressing Docker Official Images PR Comments for JVM, Native and Docker Official Images #16664

Are you sure you want to change the base?

KAFKA-16373: KIP-1028: Addressing Docker Official Images PR Comments for JVM, Native and Docker Official Images #16664

Conversation

KrishVora2912 commented Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzo1 Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

KrishVora2912 Jul 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VedarthConfluent left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 6, 2025

KrishVora01 commented Jan 9, 2025

KrishVora2912 commented Jul 23, 2024 •

edited

Loading

rzo1 Jul 26, 2024 •

edited

Loading

KrishVora2912 Jul 28, 2024 •

edited

Loading