-
Notifications
You must be signed in to change notification settings - Fork 2
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
Ubuntu transition #157
Ubuntu transition #157
Conversation
activemq/Dockerfile, line 43 at r1 (raw file):
Dropping a note here that we need to either update to ActiveMQ 5.17.0 to address Trivy scan issues, or get sign-off by the end users. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 17 of 17 files at r1, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @brosenberg42)
docker-compose.core.yml, line 46 at r1 (raw file):
image: ${REGISTRY}openmpf_activemq:${TAG} build: activemq init: true
Why add this?
Jenkinsfile, line 428 at r1 (raw file):
def service = composeYaml.services[serviceName] if (!service.build || serviceName == 'kibana') { echo "Not scanning $service.image since we didn't build it"
I think we should scan all of the images, including those we didn't build (or at least provide an option to enable that). That way we can know of all of the issues before they do scans on the production side. For example, I don't want ActiveMQ issues like the log4j 1 ones to come as a surprise.
Jenkinsfile, line 441 at r1 (raw file):
I think we should alter the status of the job to reflect these findings. How about we mark the build as UNSTABLE
, as shown here?
From the Jenkins API for UNSTABLE
:
The build had some errors but they were not fatal. For example, some tests failed.
If a user is actively deciding to enable Trivy scans, I think they would want the job status to reflect the result of the scans.
I could see how changing the status might be an annoyance if the scan is done as part of a normal build.
activemq/Dockerfile, line 34 at r1 (raw file):
SHELL ["/bin/bash", "-o", "errexit", "-o", "pipefail", "-c"] ENV LANG en_US.UTF-8
When is this needed, and why? I see it in other Dockerfiles too.
components/java_component_build/Dockerfile, line 50 at r1 (raw file):
COPY --from=openmpf_build /usr/local/bin/ffmpeg /usr/local/bin/ffprobe /usr/local/bin/ RUN wget -O- 'https://archive.apache.org/dist/maven/maven-3/3.3.3/binaries/apache-maven-3.3.3-bin.tar.gz' \
Why switch from curl
to wget
?
components/python/Dockerfile, line 82 at r1 (raw file):
RUN python3 -m venv "$COMPONENT_VIRTUALENV"; \ pip3 install --no-cache-dir --upgrade pip;
Will pip3
be updated at the same time as installing python3
?
openmpf_build/Dockerfile, line 43 at r1 (raw file):
ENV PIP_COMPILE=0 ENV DEBIAN_FRONTEND=noninteractive
Maybe we should set this as an ARG
? See this.
workflow_manager/docker-entrypoint.sh, line 80 at r1 (raw file):
[ ! "$cert" ] && continue "$JAVA_HOME/bin/keytool" -import -alias "mpf_imported_$((cert_counter++))" -file "$cert" -cacerts \
We don't need to update the Java keystore anymore? Or does update-ca-certificates
take care of that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 16 of 17 files reviewed, 9 unresolved discussions (waiting on @brosenberg42 and @jrobble)
docker-compose.core.yml, line 46 at r1 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Why add this?
It is so that it will respond to signals. Without it, it doesn't respond to SIGTERM, which then makes Docker kill it after 10 seconds.
Jenkinsfile, line 428 at r1 (raw file):
Previously, jrobble (Jeff Robble) wrote…
I think we should scan all of the images, including those we didn't build (or at least provide an option to enable that). That way we can know of all of the issues before they do scans on the production side. For example, I don't want ActiveMQ issues like the log4j 1 ones to come as a surprise.
This will create a ton of useless logging. If we find a vulnerability in in something like Redis, there isn't anything we can do about it.
Jenkinsfile, line 441 at r1 (raw file):
Previously, jrobble (Jeff Robble) wrote…
I think we should alter the status of the job to reflect these findings. How about we mark the build as
UNSTABLE
, as shown here?From the Jenkins API for
UNSTABLE
:The build had some errors but they were not fatal. For example, some tests failed.
If a user is actively deciding to enable Trivy scans, I think they would want the job status to reflect the result of the scans.
I could see how changing the status might be an annoyance if the scan is done as part of a normal build.
There are almost always going to be vulnerabilities in dependencies we use, so your suggestions will result in every build using Trivy to be unstable.
activemq/Dockerfile, line 34 at r1 (raw file):
Previously, jrobble (Jeff Robble) wrote…
When is this needed, and why? I see it in other Dockerfiles too.
I added it when I got build errors related to locale or encoding. From what I remember, I think boost-regex and the Maven assembly plugin need it.
components/java_component_build/Dockerfile, line 50 at r1 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Why switch from
curl
towget
?
The examples I saw for Ubuntu all used it. Neither is installed by default and wget is a tiny bit smaller.
components/python/Dockerfile, line 82 at r1 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Will
pip3
be updated at the same time as installingpython3
?
We want to use the version of pip that comes from apt-get
. It may lag behind the latest version of pip by a little bit.
openmpf_build/Dockerfile, line 43 at r1 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Maybe we should set this as an
ARG
? See this.
Done.
workflow_manager/docker-entrypoint.sh, line 80 at r1 (raw file):
Previously, jrobble (Jeff Robble) wrote…
We don't need to update the Java keystore anymore? Or does
update-ca-certificates
take care of that?
On Ubuntu, Java will also check the system's certificates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 16 of 17 files reviewed, 3 unresolved discussions (waiting on @brosenberg42 and @jrobble)
Jenkinsfile, line 428 at r1 (raw file):
Previously, brosenberg42 wrote…
This will create a ton of useless logging. If we find a vulnerability in in something like Redis, there isn't anything we can do about it.
We still need to report it or get it waived.
Jenkinsfile, line 441 at r1 (raw file):
Previously, brosenberg42 wrote…
There are almost always going to be vulnerabilities in dependencies we use, so your suggestions will result in every build using Trivy to be unstable.
I hear what you're saying. I think it boils down to the use case.
I certainly wouldn't want to enable Trivy on develop and master builds that automatically get kicked off, or on pull request builds; however, if I'm initiating a build for the sole purpose of looking at the Trivy results, then I probably want the state of the build to reflect the results.
If we're going to enable Trivy scans on the master builds that we tag for GA release then I agree that we don't want to mark those as unstable.
When do you think it makes the most sense to kick off a build with Trivy scans enabled? Do you think they should be part of the GA release build, or a separate build altogether?
Another, additional, option is to consider adding Trivy scans to a nightly master build. If nightly, I don't want to have to look at the logs every time to determine that there is a new vuln. We could consider whitelisting things that are waived.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed (commit messages unreviewed), 3 unresolved discussions (waiting on @brosenberg42)
Jenkinsfile, line 441 at r1 (raw file):
This could be on the clean build, since that pulls in new dependencies. |
Jenkinsfile, line 441 at r1 (raw file): Previously, jrobble (Jeff Robble) wrote…
We decided to do the Trivy scans as part of the release builds. We also considered doing them proactively / periodically, but decided instead to do them on-demand. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 16 of 18 files reviewed, 2 unresolved discussions (waiting on @brosenberg42 and @jrobble)
Jenkinsfile, line 428 at r1 (raw file):
Previously, jrobble (Jeff Robble) wrote…
We still need to report it or get it waived.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @brosenberg42)
activemq/Dockerfile, line 43 at r1 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Dropping a note here that we need to either update to ActiveMQ 5.17.0 to address Trivy scan issues, or get sign-off by the end users.
Resolved by updating to 5.16.4.
Issues:
Related PRs:
This change is