-
Notifications
You must be signed in to change notification settings - Fork 41
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
Distribute operator and dependencies' licenses #6
Conversation
Dockerfile
Outdated
# This lets us drop in scratch as a substitute when LICENSES_IMAGE_NAME isn't | ||
# specified (eg: when running `docker build -t tag:latest .`). |
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'm struggling to understand the use-case for anything other than scratch here; can you tell me more?
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 idea here is to make it possible to run docker build
without requiring users to mimic the logic/variables in Makefile
. Where possible, I'd like these sort of resources - in this case the container image - to be buildable without the use of the Makefile
.
There's not a specific alternate-image use case where a different container image is injected - the provided scratch
container fallback (and really any other) will satisfy the "interface" (ie: has a /licenses
directory) with VOLUME
used as it is. Specifically, VOLUME
will create the directory if its missing and avoids the need for a mkdir
executable in the container and subsequent stages can then reliably run COPY --from=licenses /licenses ...
.
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.
Can we remove this for now and add it back in a different PR if you feel strongly about it? I just want to keep focused on the things we must do before launch and move things that could require more conversation elsewhere.
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'll still must provide an injected container image that has the license files. So the ARG
is going to need to stay for us to provide the image, at least with that "compiled" license container image being build separately (as it is currently).
I prefer that we keep building the license image separately to avoid pulling the license collection logic into the build container. The extra VOLUME
line is here primarily to help users from stubbing their toes on the license image being provided correctly (which could happen if they ran docker build -t neio .
).
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.
Chatted a bit offline with @samuelkarp about the LICENSES_IMAGE
sections: what's clear is that I didn't do a good job of explaining of why we're wiring through a container image for licenses inline 🤦♂️
I've updated the comments here (and elsewhere) describing the use of the LICENSES_IMAGE
container image and also explain how its expected to be used.
For context: LICENSES_IMAGE
is wired up to provide a container image that has the scanned license files (of dependencies) in the output from bottlerocket-license-scan
. The Makefile
creates this "licenses" image (using the SDK container) ahead of the container
build target. The container
build is then provided its license files in the "licenses" container image, specified in LICENSES_IMAGE
, for the container
build to COPY
licenses out of and into its final image.
Regarding the WORKDIR
line (was VOLUME
): the top level Dockerfile
only makes the assumption that a tree of license files is at /licenses
in LICENSES_IMAGE
. This assumption lets scratch
be subbed in albeit with the /licenses
directory created. The /licenses
directory is guaranteed to exist by using WORKDIR /licenses
(for any given LICENSES_IMAGE
container image). With this directory in place, we allow for successful fallbacks onto scratch
or others if an image doesn't have this directory already.
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.
Thanks for adding clarification. We can move forward for now, but I'd like to see the separation in Dockerfiles go away later. Coordinating two separate image builds by way of a Makefile used to be a requirement prior to the advent of multi-stage Dockerfiles, but at this point multi-stage Dockerfiles are a more clear representation of the dependencies between build steps of container images. The fact that we're having this discussion and the need for extra documentation is an artifact of this being a non-standard pattern; I see this as a barrier to entry for new contributors who will need to read two Dockerfiles and a Makefile to understand how the image is built. Let's open an issue to track this and go from there.
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.
Tracking this in #26.
I see this as a barrier to entry for new contributors who will need to read two Dockerfiles and a Makefile to understand how the image is built.
I'm not entirely convinced that contributors will be required to read the two Dockerfiles for them to understand the container build. The multi-stage nature of the build, in my opinion, remains clear and evident in this Dockerfile
by using stages to delimit each step and expectation of the build process. Not using the Makefile doesn't preclude a successful build nor does it prevent contributors from making meaningful contributions without having read into the license collection process. Contributors familiar with building docker containers should be able to carry on without having to read two Dockerfile
s and I suspect they'll focus primarily on the top level one.
To me, the idea with injecting the license container image is what keeps the license scan & collection (and its whole ball of wax) as its own concern completely separate from the building of the update operator container.
To reiterate: I see the separation of the Dockerfile
s as a move in the benefit of the contributor (I think I've made this abundantly clear.. please don't take my wall-o-texts as a stalwart opposition!). I am not inherently opposed to combining the two (license image and update operator image) into a single Dockerfile
but would prefer that it be done simply and without compromise.
Let's gather our ideas and thoughts on reorganization & benefits in #26 before we move on this.
fc23890
to
5574070
Compare
5574070
to
17f6741
Compare
b2d39e5
to
d06d8e0
Compare
This adds bottlerocket-license-scanner's license scanning output and the repository's LICENSEs in the built container image. The clarify.toml has been added to clear up the unknown license in the sigs.k8s.io/yaml dependency. By using the scanner, the build process now relies on the bottlerocket SDK container and is downloaded automatically for builds.
This pulls in the Amazon Linux 2 CA certificates bundle to be used by the update operator instead of relying on the bundles that are/were in the building container's filesystem.
d06d8e0
to
d5fb7fd
Compare
@samuelkarp can you take a look over the changes? I think everything's been addressed at this point - if you agree then I'd like to get this merged in and ready to roll out! |
Description of changes:
The licenses of the update operator and its dependencies are now copied into the resulting container image at
/usr/share/licenses/bottlerocket-update-operator
(for the operator itself) and/usr/share/licenses/bottlerocket-update-operator/vendor
(for dependencies). The dependencies' licenses are collected and processed by the project's license scanner for a given build. In addition, this change includes clarification (inclarify.toml
) for the licenses that were not automatically detected - namely:sigs.k8s.io/yaml
.Testing done:
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.
note: the current based branch will be updated to
develop
when #3 is merged