ci: Add Binary Signing Task #3649
Conversation
|
/acn run ACN Test Unofficial |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a CI task to sign build binaries across multiple platforms.
- Adds a new pipeline job block in run-pipeline.yaml for binary signing with OS-specific details.
- Extends binaries.jobs.yaml with a conditional branch to execute signing when the "official" parameter is true.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| .pipelines/run-pipeline.yaml | Adds new signing jobs for Linux/AMD64, Windows/AMD64, and Linux/ARM64 using a shared template. |
| .pipelines/build/binaries.jobs.yaml | Introduces a conditional branch that executes signing tasks when the official flag is set. |
| @@ -43,3 +43,30 @@ jobs: | |||
| target: $(name) | |||
| os: $(OS) | |||
| arch: $(ARCH) | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
[nitpick] Please add a comment explaining the conditional branch logic for triggering the signing tasks when the 'official' parameter is true.
| # Trigger signing tasks only when the action is 'sign' and the 'official' parameter is true. |
Copilot uses AI. Check for mistakes.
.pipelines/run-pipeline.yaml
Outdated
| azure_ipam: | ||
| artifact: azure-ipam | ||
| cni: | ||
| artifact: cni | ||
| cns: | ||
| artifact: cns | ||
| ipv6_hp_bpf: | ||
| artifact: ipv6-hp-bpf | ||
| npm: |
There was a problem hiding this comment.
[nitpick] Consider adding inline comments in the matrix section to clarify the purpose of each artifact mapping, which would improve the readability and long-term maintenance of this configuration.
| azure_ipam: | |
| artifact: azure-ipam | |
| cni: | |
| artifact: cni | |
| cns: | |
| artifact: cns | |
| ipv6_hp_bpf: | |
| artifact: ipv6-hp-bpf | |
| npm: | |
| azure_ipam: # Azure IPAM (IP Address Management) component | |
| artifact: azure-ipam | |
| cni: # Container Network Interface plugin | |
| artifact: cni | |
| cns: # Container Networking Service | |
| artifact: cns | |
| ipv6_hp_bpf: # IPv6 High-Performance BPF program | |
| artifact: ipv6-hp-bpf | |
| npm: # Network Policy Manager |
Copilot uses AI. Check for mistakes.
|
/azp run ACN Test Unofficial |
|
No pipelines are associated with this pull request. |
|
/azp run Azure Container Networking PR |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -43,3 +43,37 @@ jobs: | |||
| target: $(name) | |||
| os: $(OS) | |||
| arch: $(ARCH) | |||
|
|
|||
|
|
|||
| - ${{ elseif and(eq(job_data.templateContext.action, 'sign'), job_data.templateContext.isOfficial) }}: | |||
There was a problem hiding this comment.
Does this template get called multiple times? Any reason why wouldn't we want to run this job on every run that generates binaries?
There was a problem hiding this comment.
Trying to understand the conditional differences between lines 7,8 and 48
There was a problem hiding this comment.
Signing is really only necessary for official builds. Have 2 different actions allows me to separate the two jobs, but also ensure that the dependencies are populated as needed.
|
|
||
|
|
||
| jobs: | ||
| - ${{ each job_data in parameters.images }}: |
There was a problem hiding this comment.
What unique information do we need to have to call this template correctly? Technically every single line here is a parameter
azure-container-networking/.pipelines/run-pipeline.yaml
Lines 53 to 174 in 0efbbc7
As a nit, I feel like this could have been pulled up one level
azure-container-networking/.pipelines/run-pipeline.yaml
Lines 49 to 52 in 0efbbc7
and iterated.
Instead we are passing a normal job.strategy.matrix into a jobList and parsing it.
There was a problem hiding this comment.
The idea is to focus on the configuration for someone looking to add/change basic stuff for each image. The items in the templateContext are variables that are like additional information that isn't expected to change in format. The only unique information needed is what is added to the matrix.
| @@ -51,7 +51,7 @@ steps: | |||
| echo "##vso[task.setvariable variable=Tag;isOutput=true]$TAG" | |||
| echo "Tag: $TAG" | |||
|
|
|||
| IMAGEREPOPATH="artifact/dd590928-4e04-48cb-9d3d-ee06c5f0e17f/buddy" | |||
| IMAGEREPOPATH="artifact/dd590928-4e04-48cb-9d3d-ee06c5f0e17f/$BUILD_TYPE" | |||
There was a problem hiding this comment.
TODO: ACR cleanup task for both repo paths
| CNI_NET_DIR="$REPO_ROOT"/cni/network/plugin | ||
| pushd "$CNI_NET_DIR" | ||
| go build -v -a -trimpath \ | ||
| -o "$OUT_DIR"/bin/azure-vnet"$FILE_EXT" \ | ||
| -ldflags "-X main.version="$CNI_VERSION"" \ | ||
| -gcflags="-dwarflocationlists=true" \ | ||
| ./main.go | ||
| popd |
There was a problem hiding this comment.
Why are we continuously using pushd and popd? Can we just use it once for ACN root and then call the respective make commands or just azure-cni-plugin?
If not, why not just use relative pathing as we do in the Dockerfile?
azure-container-networking/cni/Dockerfile
Lines 19 to 24 in 0efbbc7
There was a problem hiding this comment.
This logic goes to all build/scripts/*
There was a problem hiding this comment.
Relative pathing can be flaky and some utilities ask for non-relative paths. Absolute paths are much easier to debug in logs. They are also less subject to issues when repo paths change on a higher level.
Reason for Change:
This PR enables us to build signed binaries.
Scripts part out the builder portions of the dockerfiles to allow us to sign the binaries that are internal to the images.
The packages are then added to the image during build time. This results in signed binaries as an integrated part of our built images.
Also updated the build time image name to switch the docker image path.
Issue Fixed:
Requirements:
Notes: