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

Update aerospike 5.7.0.8 #11449

Closed
wants to merge 1 commit into from
Closed

Conversation

mcoberly2
Copy link
Contributor

https://download.aerospike.com/download/server/notes.html

  1. Based on your excellent feedback from the previous, now Closed, Pull Request (Update aerospike 5.7.0.8 #11342) we built a release of tini (tini-static), signed it (SHA256) and verified we are downloading what we built in the container.

  2. We implemented a feature that allows tini to restart the child process without restarting the container. This allows our database to warm restart / config reload without losing the shared memory index. Implementing this in the database server would be messy at best and not something we want to implement. Although, I could be wrong.

  1. We don't deal with all the edge cases of SIGTERMs in our app. This is some of the idiosyncrasies of the container environment. We expect tini to already do this. This is a reason why tini and dumb-init exist. We used dumb-init before this and there were no concerns previously.

https://download.aerospike.com/download/server/notes.html

1. Based on your excellent feedback from the previous, now Closed, Pull Request (docker-library#11342) we built a release of tini (tini-static), signed it (SHA256) and verified we are downloading what we built in the container.

2. We implemented a feature that allows tini to restart the child process without restarting the container. This allows our database to warm restart / config reload without losing the shared memory index. Implementing this in the database server would be messy at best and not something we want to implement. Although, I could be wrong.

3) We don't deal with all the edge cases of SIGTERMs in our app.  This is some of the idiosyncrasies of the container environment.  We expect tini to already do this. This is a reason why tini and dumb-init exist. We used dumb-init before this and there were no concerns previously.
@github-actions
Copy link

github-actions bot commented Dec 4, 2021

Diff for 929b752:
diff --git a/_bashbrew-cat b/_bashbrew-cat
index 5287b13..7f39e05 100644
--- a/_bashbrew-cat
+++ b/_bashbrew-cat
@@ -6,4 +6,4 @@ GitCommit: 9316b9e2dca468e2bbed08e12550f5d208447a14
 
 Tags: ee-5.7.0.8
 GitRepo: https://github.com/aerospike/aerospike-server-enterprise.docker.git
-GitCommit: 2d11fe93cec1c1adc86968a26b1b8827f61dc81f
+GitCommit: 9c3d341e34497e1e1127c67c5c487eff4b76ed99
diff --git a/aerospike_ee-5.7.0.8/Dockerfile b/aerospike_ee-5.7.0.8/Dockerfile
index 21a7214..fcd635e 100644
--- a/aerospike_ee-5.7.0.8/Dockerfile
+++ b/aerospike_ee-5.7.0.8/Dockerfile
@@ -13,7 +13,12 @@ ENV AEROSPIKE_SHA256 cb3e0c376ae4be9253fa4e44a005599684bf2aec66211fae87edab20b56
 
 RUN \
   apt-get update -y \
-  && apt-get install -y iproute2 procps dumb-init wget python python3 python3-distutils lua5.2 gettext-base libldap-dev libcurl4-openssl-dev \
+  && apt-get install -y iproute2 procps wget python python3 python3-distutils lua5.2 gettext-base libldap-dev libcurl4-openssl-dev \
+  && wget https://github.com/aerospike/tini/releases/download/1.0.0/tini-static -O /usr/bin/tini-static \
+  && wget https://github.com/aerospike/tini/releases/download/1.0.0/tini-static.sha256sum -O /usr/bin/tini-static.sha256sum \
+  && cd /usr/bin && sha256sum -c /usr/bin/tini-static.sha256sum && cd -\
+  && rm /usr/bin/tini-static.sha256sum \
+  && chmod +x /usr/bin/tini-static \
   && wget "https://www.aerospike.com/enterprise/download/server/${AEROSPIKE_VERSION}/artifact/debian10" -O aerospike-server.tgz \
   && echo "$AEROSPIKE_SHA256 *aerospike-server.tgz" | sha256sum -c - \
   && mkdir aerospike \
@@ -55,9 +59,8 @@ COPY entrypoint.sh /entrypoint.sh
 #
 EXPOSE 3000 3001 3002
 
-# Runs as PID 1 /usr/bin/dumb-init -- /my/script --with --args"
-# https://github.com/Yelp/dumb-init
+# Tini init set to restart ASD on SIGUSR1 and terminate ASD on SIGTERM
+ENTRYPOINT ["/usr/bin/tini-static", "-r", "SIGUSR1", "-t", "SIGTERM", "--", "/entrypoint.sh"]
 
-ENTRYPOINT ["/usr/bin/dumb-init", "--", "/entrypoint.sh"]
 # Execute the run script in foreground mode
 CMD ["asd"]

Relevant Maintainers:

@yosifkit
Copy link
Member

From what I can see, signal handling is already in Aerospike's C code, so I am a bit confused what other edge cases or idiosyncrasies of containers you see for PID 1? As far as I understand it, any catchable, blockable, or ignorable signal that would normally kill the process (like "Term" and "Core" dispositions) are instead ignored by default for PID 1, but Aerospike already appears to handle them?

Perhaps the Aerospike SIGUSR1 handler or another signal handler could be modified to do something similar to SIGTERM and unlock the main deadlock thread but instead of the main thread exiting, it would do an execve of itself (/proc/self/exe can be useful here) with the original argv after all the cleanup?

We are mildly concerned about using a custom fork of tini. Because it is still called tini, it may confuse users when they use other images containing tini or via the --init flag in Docker. It looks like both tini and dumb-init upstream developers label this functionality as "out of scope" (krallin/tini#71 (comment), krallin/tini#146, krallin/tini#186, Yelp/dumb-init#156, Yelp/dumb-init#254). So, if there are plans to keep the fork maintained instead of adding more handling into aerospike-server, then it should probably be under a different name than just tini.


The Dockerfile should be written to help mitigate interception attacks during build. Our requirements focus on three main objectives: verifying the source, verifying author, and verifying the content; these are respectively accomplished by the following: using https where possible; importing PGP keys with the full fingerprint in the Dockerfile to check signatures; embedding checksums directly in the Dockerfile. All three should be used when possible. Just https and embedded checksum can be used when no signature is published.
[...]
The purpose in recommending checksum verification is to verify that the artifact is as expected. This ensures that when remote content changes, the Dockerfile also will change and provide a natural docker build cache bust. As a bonus, this also prevents accidentally downloading newer-than-expected artifacts on poorly versioned files.

- https://github.com/docker-library/official-images/tree/2969d4f64a77bc8343486f44f149ce74b70bc5a6#security

Downloading the sha256sum from the same source as the artifact, doesn't provide the same level of content verification that embedding the checksum in the Dockerfile does.

@mcoberly2
Copy link
Contributor Author

Closing...

@mcoberly2 mcoberly2 closed this Dec 17, 2021
@mcoberly2 mcoberly2 deleted the patch-14 branch December 17, 2021 22:48
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.

2 participants