-
Notifications
You must be signed in to change notification settings - Fork 54
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
chore: secure hermetic-build docker image #3196
base: main
Are you sure you want to change the base?
Conversation
…ge' into secure-hermetic-build-docker-image
…ge' into secure-hermetic-build-docker-image
# We use the esbuild tool. See https://esbuild.github.io/ | ||
COPY ./.cloudbuild/library_generation/image-configuration/owlbot-cli-build-config.js . | ||
RUN npm i esbuild | ||
RUN node owlbot-cli-build-config.js |
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.
In order to create a SEA, do we have to bundle source code into a single js file?
Can we combine the two steps into one?
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.
Yes, bundling is necessary:
sdk-platform-java/.cloudbuild/library_generation/library_generation.Dockerfile
Lines 39 to 40 in fb98222
# This is because SEA (see below) cannot | |
# resolve external modules in a multi-file project. |
From the docs: The single executable application feature currently only supports running a single embedded script using the CommonJS module system.
Can be combine the two steps?
Do you mean using a single RUN
+ &&
?
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.
I was referring whether we can build a SEA from multiple js source code, looks like it has to be bundled together first.
Co-authored-by: Joe Wang <106995533+JoeWang1127@users.noreply.github.com>
…ge' into secure-hermetic-build-docker-image
Quality Gate passed for 'gapic-generator-java-root'Issues Measures |
Quality Gate passed for 'java_showcase_integration_tests'Issues Measures |
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.
We can add a secret in the repo or migrate this action to Cloud Build.
Agree that we should migrate them to Cloud Build, same as the integration tests. This gets us closer to not dependent on Github specific features.
# with the transferred source code and jars | ||
FROM gcr.io/cloud-devrel-public-resources/java21 AS ggj-build | ||
# node:22.1-alpine | ||
FROM us-docker.pkg.dev/artifact-foundry-prod/docker-3p-trusted/node@sha256:487dc5d5122d578e13f2231aa4ac0f63068becd921099c4c677c850df93bede8 as owlbot-cli-build |
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.
How do we plan to update these base images? It might be fine to not update this one, but for the Java and Python one, we may want to update the Maven/JDK/Python version regularly.
RUN mvn install -B -ntp -DskipTests -Dclirr.skip -Dcheckstyle.skip | ||
RUN cp "/root/.m2/repository/com/google/api/gapic-generator-java/${DOCKER_GAPIC_GENERATOR_VERSION}/gapic-generator-java-${DOCKER_GAPIC_GENERATOR_VERSION}.jar" \ | ||
"./gapic-generator-java.jar" | ||
# use Docker Buildkit caching for faster local builds |
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.
What does Docker Buildkit caching
mean? Since the build does not take too much time(~1.5 minute), I think it's OK to download the maven dependencies every time. What I'm trying to avoid is that we build an image with stale dependencies.
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.
The Buildkit caching allows to reuse the specified folders in the specific step. This would not impact any CI pipeline as they don't preserve any kind of cache. The main purpose is to speed up local builds for development, and can be disabled via docker build --no-cache
.
My perspective is that we probably won't work on modifying the java source code when working on the Docker image, so several image builds may benefit from caching the mvn install
output.
This reverts commit a3490e2.
) This PR switches the base images of all the Hermetic Build Docker image stages to publicly hosted docker images. The chosen images are mirrored by Airlock or are at least in process of incorporation. The choice of OS family is `alpine` for its security-oriented setup. 0 vulnerabilities were found in the public image scan reports ([example](https://hub.docker.com/layers/library/python/3.12.7-alpine3.20/images/sha256-f498302457ec11162f872199b92239c34e1fbcdbc391ff37a4959e820224aa98?context=explore)). Although several optimizations are possible (see #3196 and its intermediate commits), this PR is restricted to the minimal changes to have a secure base image. _Notes:_ * Since the UNIX tools are from FreeBSD, the `-d` flag in the `rm` command is not supported. This is why we removed its usage in the scripts * A few downstream checks are failing in other PRs as well. I raised #3325 to track this.
This PR improves the Hermetic Build Docker image in a few different ways:
-d
flag in therm
command is not supported. This is why we removed its usage in the scriptsPending items
APPLICATION_DEFAULT_CREDENTIAL
secrets such as the one declared in the Dockerfile. In the meantime, I left a build workflow in.cloudbuild
to replace the integration tests and removed the integration tests portion of the existing GitHub Action. Another possible approach is to have a repository secret and keep using the GitHub Action.