ci: move Harbor registry refs to repository variables, add .gitleaksignore - #360
ci: move Harbor registry refs to repository variables, add .gitleaksignore#360Abdelsalam-Abbas wants to merge 7 commits into
Conversation
…gnore The internal registry hostname now comes from the ASTERA_REGISTRY and SAMPLEWORKS_CHECKPOINTS_SOURCE_IMAGE repository variables instead of workflow literals. The astera job fails fast when ASTERA_REGISTRY is unset, since login-action would otherwise default to Docker Hub and send the Harbor credentials there. Remaining README/Dockerfile.astera references are intentional docs for internal use; their historical scan fingerprints are listed in .gitleaksignore to quiet the warn-only internal-refs check.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe Docker workflow now composes checkpoint image references from configured registry inputs, requires digest-pinned sources, and validates registry values. Astera builds use ChangesDocker registry parameterization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/docker.yml:
- Around line 20-26: Update the README configuration-variable table to mark
SAMPLEWORKS_CHECKPOINTS_SOURCE_IMAGE and ASTERA_REGISTRY as required. Replace
the former source-image default description with the requirement that
SAMPLEWORKS_CHECKPOINTS_SOURCE_IMAGE reference the digest-pinned Harbor source
image, and add ASTERA_REGISTRY with its required-variable description.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: abf3ab14-2098-496a-88be-10d87b527ccd
📒 Files selected for processing (2)
.github/workflows/docker.yml.gitleaksignore
sampleworks is a public repo, so suppressing the internal-reference warning is the wrong response: the alert is reporting real disclosure, not noise.
The actl image catalog already resolves the `sampleworks` alias to a digest-pinned Harbor ref, so the README no longer needs to spell out the internal registry. That also pins the image the docs point at, which the mutable tag did not. Maintainer-only local builds take the registry host from ASTERA_REGISTRY, the same variable CI uses.
Repository variables are not masked in Actions logs, and this repo is public, so the registry host would have been printed by login-action and metadata-action on every run. Making the bare host a secret masks it everywhere it appears, since masking is substring-based, while leaving image paths and digests readable for debugging a failed build. The source-image guard now checks the parts rather than the composed ref: an unset host still yields a non-empty, digest-pinned-looking string.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/docker.yml:
- Around line 55-61: Update the validation in the workflow before constructing
or using the source image reference to normalize ASTERA_REGISTRY and reject
Docker Hub aliases, including docker.io, registry-1.docker.io, index.docker.io,
and any value matching PUBLIC_REGISTRY. Keep the existing empty checks, and
ensure the rejected value cannot reach the skopeo copy operation or receive
Harbor credentials.
In `@Dockerfile.astera`:
- Around line 8-9: Update the local-build documentation wording from “CI
repository variable” to “CI repository secret” at Dockerfile.astera lines 8-9
and README.md lines 341-343, matching the workflow’s use of
secrets.ASTERA_REGISTRY.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a9005f8-6f9d-4ee5-bfa3-3cb703975b46
📒 Files selected for processing (3)
.github/workflows/docker.ymlDockerfile.asteraREADME.md
Records a deliberate risk acceptance rather than hiding a finding: the host resolves only on the tailnet, so the exposure is reconnaissance -only, and removal would rewrite 81 commits and 22 tags while our forks would keep the objects regardless. Fingerprints are commit-scoped, so new occurrences still alert. Taken from the CI run's own report so they match the org rule config exactly.
The Harbor credentials are passed to whatever host ASTERA_REGISTRY names, as skopeo --src-creds and as the login-action registry, so pointing it at Docker Hub would hand them to a third party. The empty case was already guarded; this covers the explicitly-public case. Also corrects the local build docs, which still called the value a repository variable after it became a secret.
The 18 accepted fingerprints are recorded in ENG-597. Whether they live in-repo or in a central private repo is still open, so this PR ships only the registry cleanup. The internal-ref check stays red meanwhile, which is the status quo, not a regression.
|
The workflow changes are solid. Two things about the PR itself rather than the code.
The merge requirement names the wrong things. The body says to set Body also says README and What's good: checking the parts instead of the composed ref is a genuinely subtle catch, since an unset host still yields a non-empty |
2 participants
Quiets the recurring internal-reference secret-scan alerts for this repo (18 findings re-posting on every PR; now 25 in grown history).
docker.yml: the internal registry hostname now comes from theASTERA_REGISTRYandSAMPLEWORKS_CHECKPOINTS_SOURCE_IMAGErepository variables, with no in-repo fallback. New fail-fast guard in the astera job:docker/login-actionwith an empty registry defaults to Docker Hub and would send the Harbor credentials there..gitleaksignore: fingerprints for all historical findings (allastera-internal-hostname). README andDockerfile.asterakeep their references as intentional internal docs while the repo is private; going public requires republishing as a fresh single-commit repo per the runbook regardless.Merge requirement: the two repository variables must be set first or the next docker workflow run fails fast:
ASTERA_REGISTRY= the Harbor hostnameSAMPLEWORKS_CHECKPOINTS_SOURCE_IMAGE= the digest-pinned checkpoints refVerified: full-history scan with the internal-refs rules is clean with this ignore file; workflow YAML parses; pre-commit hooks pass.
Summary by CodeRabbit
Documentation
sampleworksACTL image alias.Bug Fixes