Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: SgtCoDFish The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
irbekrm
left a comment
There was a problem hiding this comment.
I've read through the scripts and I think in general this would be a huge improvement as it makes the build steps a lot less complex and I think that's important both because it will be easier to make changes in the future and because it will be less likely that bugs are introduced or scripts drift due to folks not really understanding them.
I agree with the approach in general, would like to play with these a bit more and think about any possible edge cases around ensuring that dependency etc changes are detected and understand the signing/sha generation a bit better. Also interested how this will fit into our e2e test setup
|
This looks great! I tried using nerdctl (containerd and buildkit both running rootless) It works as expected. I think we should go on and have this PR merged and use that when releasing for 1.7.0-alpha.0. Update: we were able to do "reproducible builds" using |
af2f097 to
bce89ca
Compare
| bin/* | ||
|
|
||
| !bin/server/** | ||
| !bin/scratch/cert-manager.license |
There was a problem hiding this comment.
note: this means "ignore everything in bin except these two paths", which are the only ones currently consumed by the Containerfile
|
The PR description is very good!!
Does it mean we are going to run By the way, is there still a need for storing the staged artifacts to a bucket? Could we skip this step and upload the artifacts directly to the github release? And when we click "Publish", we have something that pushes these tarballs to quay.io? |
Thanks 😁
I think that "upload-release" target should probably exist, yeah. I'll write it. All of this will run in Cloud Build for sure; that'll provide us all with centralized build logs, plus a stable but ephemeral build environment which can easily be defined in code.
I think storing them in a bucket is good; it's a central source of truth (and also a backup) of all release artifacts and that's valuable I think. The uploading logic could be moved into the main cert-manager repo longer term - I think all of |
|
/test pull-cert-manager-e2e-v1-22 |
|
The e2e test failure is unrelated and caused by cert-manager/testing#611 |
|
Update: Working on adding the gcs upload + fixing containers atm, should have that finished soon EDIT: Containers are now working as I intended them to. I've not done the google cloud upload, I might leave that for a separate PR just so this one has less in it. (Also gsutil isn't as trivial to install locally, so I was looking at other tools like rclone) |
JoshVanL
left a comment
There was a problem hiding this comment.
Looking good from me. Just have a few things, most of them come down to file/dir structure/placement- slightly bikesheddy, but a good dir structure I think goes a long way. It is obviously a bit trickier atm with bazel still around and squatting all the good names.
Nice work 😄
Thank you! I think I've addressed all comments now; I didn't want to delve too deep into extending |
Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
hash.sh returns just the sha256sum of its input file checkhash.sh uses ha.sh to get the sha256sum of its first argument and then validates that the checksum matches the value provided in its second argument hash.sh isn't currently fully portable since sha256sum isn't present by default on macOS, but it provides a single point around which we can do hashing to validate checksums Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
Includes targets for: - all "server" binaries, for all arches - all containers for all server binaries for all arches - all client binaries (kubectl plugin / cmctl) for all arches - the cert-manager helm chart + signature - the cert-manager static manifests + CRDs - tools which bazel would download, with checksum verification - (commented out) a signed SHA256SUM file for client binaries Upgrades from the bazel flow include that: - we use OS-specific base images rather than just using amd64 everywhere - we easily add support for signing artifacts at build time - we add ".exe" to the end of windows executables - we add a zip file for windows executables, for easier consumption - we concatenate YAML files more robustly - staging a full release should be much faster - hopefully, it's easier to change things! - licenses are trimmed down to reduce bloat in images (the license bundle was 1.4MB in size alone) Changes from the bazel flow include: - containers no longer have a symlink to the binary at an unusual path, but instead just have the binary at a more predictable path (e.g. /app/cmd/webhook/webhook instead of /app/cmd/webhook/webhook.runfiles/com_github_jetstack_cert_manager/cmd/webhook/webhook_/webhook) Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
|
/lgtm 🔥🎄 |
Summary
This PR provides a
make-based flow for building all cert-manager artifacts, with the intention of replacing bazel for deployments, with minimal changes. This isn't intended to fully remove bazel today, but instead to remove it for the development edit-build-run workflow and for cert-manager deployments, which are currently handled bycmrel.Help for Reviewers
If you follow both "Comparison Against Old Releases" and "Building Everything + Signing" you should be able to easily compare an old bazel release and a "new" make release.
Comparison Against Old Releases
The goal here is that
bin/releaseshould contain everything needed forcmrel publishto be able to publish a release based on its contents, with minimal changes.To compare
bin/releasewith a previous release, the following may be helpful to check out an existing cert-manager release locally:Building Everything + Signing
Creating a full staged release includes signing the helm chart, which requires a GCP KMS key. I have a test one in my project and I can allowlist you to use it if you ask. Then, you should be able to run the following:
High Level Structure
The intention of this PR is that everything we need to stage a deployment can be built with exactly one command, including generating signatures. This build step should also be trivially run in parallel using make's
-jNoption.The above command, starting from a clean repo with no
bin/dir but with a populated go build and module cache takes under 2 minutes to completely build and sign every artifact for a release when run on a developer laptop. The 2 minutes includes downloading and installing tools as dependencies (helm, cosign, etc).Artifacts
Everything that's built is placed under
bin/. Crucially, usingmake -f make/Makefile staged-releasewill createbin/releasecontaining the same artifacts which bazel built, in the same format. In other words,bin/releaseshould contain everything required to stage a complete release and side effect of this is that it should obsolete most of the logic incmrel stageand allow a lot of cleanup there.After this PR merges,
cmrel stagewill essentially become "clone repo, run make staged-release, upload to a bucket".Notes / Differences from Bazel Artifacts
Lines prefixed with a⚠️ denote functional changes which could affect others. Lines without a warning sign should be non-functional changes (e.g. a comment change)
Versioning
v1.6.0-67-gf4e2b2c3cwill be generated usinggit describe --tags. This should uniquely identify practically all development builds.v<last-tag>-<N>-<commit-sha>whereNis the number of commits since<last-tag>Windows Client Binaries
Helm Charts
cert-manager.tgz. This more closely matches how helm expects chart filenames.cert-manager-manifests.tar.gzwith their version in their filenamesignKeyannotation directly into the actual helm chart, which should improve the ArtifactHub UX. Previously this annotation was only added in our charts repo'sindex.yamlwhere it seemed to largely be ignored.Signing
cmrelas an installed standalone tool).deploy/charts/cert-manager/signkey_annotation.txt. This can obviously be improved later.Containers
CTRenvironment variable and bothpodmanandnerdctlcan be used as drop-in replacements.cert-manager-controller-<arch>:<version>and doesn't include the registry (quay.io/...). This is to avoid creating a race condition if building containers in parallel. This will require a small change tocmrel publish, but seems worth to avoid this killing the ability to build containers in parallel.Base Images
BASE_IMAGE_TYPE)Binary Locations
Background: Bazel adds binaries to containers at a long unusual path, along with a symlink:
In other words, the actual binary is
/app/cmd/webhook/webhook.runfiles/com_github_jetstack_cert_manager/cmd/webhook/webhook_/webhookand/app/cmd/webhook/webhookexists as a symlink to that binary./app/cmd/webhook/webhook.externalsymlink seen in the tree above, either.Licenses in Containers
LICENSESfile changed into a short string pointing people at our repo if they want to see licenses.LICENSESis 1.4MB on its own, which is absurdly wasteful for a file which nobody ever looks at or cares about.Static manifests: cert-manager.yaml / cert-manager.crds.yaml
Release Notes
/kind feature