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

Distribute operator and dependencies' licenses #6

Merged
merged 2 commits into from
Mar 5, 2020

Conversation

jahkeup
Copy link
Member

@jahkeup jahkeup commented Feb 26, 2020

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 (in clarify.toml) for the licenses that were not automatically detected - namely: sigs.k8s.io/yaml.

Testing done:

make container
make dist
make check

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

@jahkeup jahkeup self-assigned this Feb 26, 2020
Dockerfile Outdated
Comment on lines 7 to 8
# This lets us drop in scratch as a substitute when LICENSES_IMAGE_NAME isn't
# specified (eg: when running `docker build -t tag:latest .`).
Copy link
Contributor

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?

Copy link
Member Author

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 ....

Copy link
Contributor

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.

Copy link
Member Author

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 .).

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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 Dockerfiles 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 Dockerfiles 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.

Dockerfile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
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.
@jahkeup
Copy link
Member Author

jahkeup commented Mar 4, 2020

@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!

@jahkeup jahkeup merged commit cd17004 into develop Mar 5, 2020
@cbgbt cbgbt deleted the licenses-licenses branch January 21, 2022 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants