Skip to content

ci: image collection and scanning - #461

Closed
davrad wants to merge 10 commits into
mainfrom
feat/add-image-check
Closed

ci: image collection and scanning#461
davrad wants to merge 10 commits into
mainfrom
feat/add-image-check

Conversation

@davrad

@davrad davrad commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

📝 Summary

This PR adds image scanning and Trivy analysis to the CI.
In addition it updates the goreleaser schema to include an image section into the header, such that in each release, all the image versions of the standard kubara catalog are also listed.

🧩 Type of change

  • 🔧 CLI / Go code
  • 📦 Helm chart
  • 🧱 Terraform module
  • 📝 Documentation
  • 🧪 Test or CI change
  • ♻️ Refactor / cleanup

⚠️ Is this a breaking change?

  • Yes, this change breaks existing functionality (explain in summary)

🧪 Testing

  • CI passed
  • Manually tested (local/dev cluster)
  • Unit tested
  • Not tested (explain why below)

🔗 Related Issues / Tickets

✅ Checklist

  • Code compiles and passes all tests
  • Linting and style checks pass
  • Comments added for complex logic
  • Documentation updated (if applicable)

📎 Additional Context (optional)

@davrad
davrad force-pushed the feat/add-image-check branch 2 times, most recently from 23d42ed to cd48076 Compare June 29, 2026 18:52
@tuunit tuunit changed the title Feat/add image check ci: image collection and scanning Jul 1, 2026
@github-advanced-security

Copy link
Copy Markdown

You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool.

What Enabling Code Scanning Means:

  • The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
  • Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
  • You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

Comment thread .scripts/image-version.sh
@davrad
davrad marked this pull request as ready for review July 3, 2026 07:19
@davrad
davrad requested a review from a team July 3, 2026 07:19
@davrad
davrad force-pushed the feat/add-image-check branch from 4ef46cb to 375297f Compare July 3, 2026 07:20
@davrad

davrad commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

The Pipepiline currently runs into Caching issues.

Caching issues were addressed.

@davrad
davrad force-pushed the feat/add-image-check branch 3 times, most recently from 576ead3 to 7b31aea Compare July 10, 2026 14:53
Comment thread .scripts/kubara-config-update.sh
@tuunit
tuunit force-pushed the feat/add-image-check branch from e494143 to 279eb27 Compare July 14, 2026 08:11

@Matthiator Matthiator left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice addition — rendering every chart and scanning all catalog images is a great step 🙌

I went through it in detail and left inline comments. The main thing before merge is that helm-checks currently goes red (see the config comment), plus a few path/error-handling edge cases in the release job and the extraction script. Nothing here is hard to fix. Happy to help with any of them!

A couple of tiny nits I didn't inline: typo helm template for for '$chart' (double "for"), the comment # pipefail that pipes break reads a bit confusingly, the config comment Enable every catalog service to the image/vuln report… is missing a word, and SCRIPT_DIR looks unused. All optional. 🙂

apply_yaml_if_set KUBARA_DNS_NAME ".clusters[0].terraform.dns.name"

# Enable every catalog service to the image/vuln report and the release header cover all charts
yq eval '(.clusters[0].services[] | .status) = "enabled"' -i "$CFG"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the intent is to enable everything so all charts get rendered for the scan — makes sense 👍

The catch: this same generated config is also consumed by the existing helm-checks job, which now renders reloader (disabled by default) and fails on its template-library dependency → the whole job goes red (that's the currently failing check).

Could we scope the "enable all" to a separate config used only for the extract/scan step, so helm-checks keeps its current service set? (Alternatively: make reloader renderable, or only enable services that render cleanly.)


- name: Extract images and set goreleaser header
run: |
MANAGED=/tmp/gen/managed-service-catalog/helm \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small path mismatch here: the generated files live under /tmp/gen, but only MANAGED is overridden — and it points to managed-service-catalog/helm, while generate now produces platform-components/helm. CONFIG_FILE/CONFIGS also stay relative to the repo root, where there's no config.yaml.

As written the script would exit before GoReleaser, so the header wouldn't be filled. Something like:

MANAGED=/tmp/gen/platform-components/helm \
CONFIG_FILE=/tmp/gen/config.yaml \
CONFIGS=/tmp/gen/platform-configs/<cluster>/helm \
  "$GITHUB_WORKSPACE"/.scripts/image-version.sh

should fix it. (Not caught by PR CI since release only runs on tag push.)

Comment thread .scripts/image-version.sh


render_dir="$(mktemp -d)"; trap 'rm -rf "$render_dir"' EXIT
FAILED=()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice that failures get collected in FAILED — but the array is never checked afterwards, so a chart that fails dependency update/template is silently skipped and the job still passes. That means its images quietly disappear from the scan (this is exactly what happens to reloader).

Could we fail (or at least emit a visible ::warning::) when FAILED is non-empty?

Minor: on the helm dependency update line below, dep_out=$(… >/dev/null 2>&1) discards all output, so the later echo "$dep_out" never prints anything.

image-ref: ${{ matrix.image }}
version: ${{ env.TRIVY_VERSION }}
scanners: vuln
severity: HIGH

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

severity: HIGH only reports HIGH — CRITICAL findings are excluded from the SARIF. Did you mean severity: HIGH,CRITICAL? (Combined with exit-code: '0' the scan is report-only, which is totally fine if that's intended.)

Comment thread .scripts/image-version.sh

echo "Done Rendering!"

[[ -n "$IMAGES" ]] || { echo "::warning::No image references found"; exit 0; }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With set -euo pipefail, if grep matches nothing the pipeline exits 1 and the script dies in the IMAGES="$( … )" assignment above — before ever reaching this friendly ::warning::No image references found / exit 0. OUTPUT_FILE then never gets written, and the workflow reads reports/images.txt unconditionally, so the has_images=false branch can't actually be reached.

A || true on the grep (or writing the file first) would restore the intended empty-case handling.

Comment thread .scripts/image-version.sh

MANAGED="${MANAGED:-${PWD}/platform-components/helm}"
CONFIG_FILE="${CONFIG_FILE:-config.yaml}"
CLUSTER_NAME="$(yq -r '.clusters[0].name' "$CONFIG_FILE")"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny thing: yq reads $CONFIG_FILE here before the [[ -f … ]] / command -v checks below, so a missing file/tool produces a cryptic command-substitution error instead of your nice diagnostic. Moving the checks above this line would surface the friendly message.

# metalb, loki and velero need custom configs
yq eval '.clusters[0].services.metallb.config.publicLoadBalancerIPs = "203.0.113.10"' -i "$CFG"
yq eval '.clusters[0].services.metallb.config.loadBalancerAddressPool = ["203.0.113.0/24"]' -i "$CFG"
yq eval '.clusters[0].storage.bucketNames.chunks = "loki"' -i "$CFG"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This writes .clusters[0].storage.bucketNames.chunks, but I couldn't find a matching field in the config schema (the Loki chunks bucket comes from the static platform-components/helm/loki/values.yaml). It may be silently ignored — worth double-checking whether this actually has the intended effect.

Comment thread .scripts/image-version.sh

IMAGES="$(
cat "$render_dir"/*.yaml |
grep -E '^[[:space:]]*image:' |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick note on coverage: since extraction is a grep image: over the helm template output, it catches all standard container / init-container / sidecar images (good!), but by design it misses:

  • runtime-injected images (mutating webhooks, Kyverno, operator-set images via flags),
  • image refs passed via args/env rather than an image: field,
  • - image: list-item lines — the regex only matches image: after whitespace, not after - .

Not a blocker, but might be worth a comment noting this is best-effort, so "scan passed" isn't read as "every running image was scanned."

@tuunit

tuunit commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

closed in favour of #501

@tuunit tuunit closed this Jul 22, 2026
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.

4 participants