feat(hyperf): add OTEL collector variants and unify hyperf-otel#2
Open
wilcorrea wants to merge 2 commits into
Open
feat(hyperf): add OTEL collector variants and unify hyperf-otel#2wilcorrea wants to merge 2 commits into
wilcorrea wants to merge 2 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 18 minutes and 29 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThis PR introduces automated Docker image building infrastructure for Hyperf with OpenTelemetry Collector, including a GitHub Actions workflow that detects code changes and builds versioned container images (8.3 and latest) featuring optional pgbouncer database pooling, supervisor-managed processes, and distributed tracing via Google Cloud export. ChangesHyperf-OTEL Docker Images with CI/CD Automation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 13
🤖 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/build-images.yml:
- Around line 1-17: The workflow is missing an explicit permissions block which
causes it to inherit broad repository defaults; add a top-level permissions
entry requesting the minimum required for this job (e.g. set permissions:
contents: read) so the GITHUB_TOKEN used by actions only has read access to the
repo; place this permissions block at the top-level (alongside keys like name
and on) so the workflow explicitly limits the GITHUB_TOKEN scope.
- Around line 40-45: The workflow currently leaves candidates empty when
triggered by workflow_dispatch without inputs.target because the
tj-actions/changed-files step is skipped; update the dispatch path in the
build-images job to either require inputs.target or fall back to a real diff
source: when github.event_name == "workflow_dispatch" and inputs.target is
empty, call or reuse the changed-files logic
(steps.changed.outputs.all_changed_files) or compute a diff against the default
branch (e.g., fetch origin and diff HEAD...origin/main) and populate candidates
from that output; ensure the code paths that reference inputs.target,
steps.changed.outputs.all_changed_files and candidates are adjusted so
candidates is non-empty for manual runs without a target.
- Around line 23-24: Update every GitHub Action `uses:` in the workflow to pin
to an immutable commit SHA instead of a moving tag: replace actions/checkout@v4,
tj-actions/changed-files@v44, docker/setup-buildx-action@v3,
docker/login-action@v3, and docker/build-push-action@v6 with their corresponding
OWNER/REPO@<full_sha> values (you can look up each repo’s latest stable commit
SHA) and optionally retain the human-readable tag as a trailing comment; ensure
the changes are made where those `uses:` entries appear so the workflow
references specific commit SHAs.
- Around line 37-43: The workflow is vulnerable to command injection by
interpolating GitHub expressions directly into shell blocks (candidates+=("${{
inputs.target }}"), image="${{ matrix.target }}", and for dir in ${{
steps.changed.outputs.all_changed_files }}); fix by passing these values through
the environment (use env: or GITHUB_OUTPUT to set a safe variable) and then
reading the environment vars inside the run block, validate the target/matrix
value against a strict allowlist before using it (e.g., check in a switch/if
against allowed names in the run step), and iterate changed files from a
pre-sanitized newline-separated env variable (or read from a file) rather than
unquoted expansion; update the blocks that reference candidates, inputs.target,
matrix.target, steps.changed.outputs.all_changed_files and the image assignment
to use the sanitized env vars and allowlist checks.
In `@hyperf-otel/8.3/Dockerfile`:
- Around line 13-20: Create and use an unprivileged service user in the
Dockerfile: add a non-root user (e.g., "hyperf") and group, chown the runtime
directories created by the RUN mkdir -p line (/etc/pgbouncer,
/var/log/pgbouncer, /var/run/pgbouncer, /etc/supervisor.d, /var/run/supervisor)
plus copied config files (COPY rootfs/... entries) to that user, and set USER to
that account before the ENTRYPOINT so /entrypoint.sh, otelcol-contrib, Hyperf
and optional pgbouncer run unprivileged; ensure the entrypoint still has execute
permission and any startup steps that need root (if any) are performed before
switching to the non-root user.
- Around line 7-12: The Dockerfile currently downloads
otelcol-contrib_${OTEL_COLLECTOR_VERSION}_linux_amd64.tar.gz and extracts it
without verifying integrity; update the RUN step that references
OTEL_COLLECTOR_VERSION and otelcol-contrib to fetch the release checksum (and/or
signature) from the same GitHub release, install any needed tools (sha256sum or
gnupg), verify the tarball against the published SHA256 (or verify the GPG
signature with the collector project's public key), fail the build if
verification fails, and only then extract to /usr/local/bin and remove
artifacts; ensure verification commands are added before the tar -xzf invocation
so the build aborts on mismatch.
In `@hyperf-otel/8.3/rootfs/entrypoint.sh`:
- Around line 35-41: The script writes a pgbouncer stanza even when resolved
backend variables (host, name, user, pass) are empty; change entrypoint.sh to
validate the per-alias variables (host, name, user, pass — allow port default)
before appending the stanza: if any required variable is empty, print a clear
error mentioning the alias to stderr and exit 1 instead of writing "pgb_${alias}
= ..." ; only append the stanza when the check passes. Use the existing variable
names (host, port, name, user, pass, alias) to locate and modify the block.
- Around line 18-23: The code builds a prefix from each PGBOUNCER_DATABASES
alias and passes it into eval (see resolve_prefix, the eval uses around "printf
... \"\${${prefix}_...:-}\"" and "export ${prefix}_...=..."), which is a
command-injection risk; fix by first validating each alias against a strict
whitelist (e.g., /^[A-Za-z0-9_]+$/) and aborting on any mismatch, then eliminate
eval by using safe environment lookups: construct the variable name (e.g.,
var="${prefix}_HOST") and retrieve its value with a non-eval method (use
printenv/printenv "$var" or, in bash, indirect expansion ${!var}) and assign
exports with safe shell expansions instead of eval; update resolve_prefix to
only produce validated uppercase prefixes and ensure all places that previously
used eval now use the validated prefix + safe env lookup and explicit export
commands.
In `@hyperf-otel/latest/Dockerfile`:
- Around line 13-20: Create a non-root service user and switch to it before
ENTRYPOINT: add a user (e.g., "hyperf" or "service") in the Dockerfile, chown
the runtime dirs and config/entrypoint files copied from rootfs (referencing
/etc/pgbouncer, /var/log/pgbouncer, /var/run/pgbouncer, /etc/supervisor.d,
/var/run/supervisor, /etc/otel-collector-config.yaml and /entrypoint.sh) so the
unprivileged user can write them at runtime, then set USER to that account
before the existing ENTRYPOINT ["/entrypoint.sh"]; if any initialization in
/entrypoint.sh requires root, perform that chown/setup at build time or wrap
only that portion with a drop-to-root helper, otherwise ensure the container
runs the otelcol-contrib, Hyperf and optional pgbouncer processes as the new
user.
- Around line 7-12: The Dockerfile currently downloads /tmp/otelcol.tar.gz using
OTEL_COLLECTOR_VERSION and extracts it without verification; update the RUN
sequence to fetch the published checksum (or signature) for the same release
(e.g. the SHA256 or GPG signature from the GitHub Releases for
v${OTEL_COLLECTOR_VERSION}), verify /tmp/otelcol.tar.gz before running tar -xzf
(fail the build if verification fails), and only then chmod and install
otelcol-contrib; reference the OTEL_COLLECTOR_VERSION variable,
/tmp/otelcol.tar.gz, and the otelcol-contrib binary when implementing the
verification step.
In `@hyperf-otel/latest/README.md`:
- Line 50: Replace the image tag used in the README examples: find every
occurrence of the string "devitools/hyperf-otel:8.3" (used in the Dockerfile
example lines like the FROM directive) and change it to
"devitools/hyperf-otel:latest" so the `latest` README shows the unversioned tag
in all examples.
In `@hyperf-otel/latest/rootfs/entrypoint.sh`:
- Around line 35-41: The code currently always writes a pgb_${alias} stanza even
if resolved vars (host, port, name, user, pass) are empty; update the block that
computes host/port/name/user/pass (using prefix and alias) to validate required
fields (at minimum host, name, user, pass) before appending to
/etc/pgbouncer/pgbouncer.ini: if any required variable is empty, print a clear
error naming the alias and missing fields and exit non‑zero instead of writing
the stanza; otherwise write the configured stanza as before. Ensure you
reference the same variables (host, port, name, user, pass, prefix, alias) and
fail fast when validation fails.
🪄 Autofix (Beta)
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
Run ID: 6d22868b-adfd-4d97-b3d2-8c645c7964d6
📒 Files selected for processing (11)
.github/workflows/build-images.ymlhyperf-otel/8.3/Dockerfilehyperf-otel/8.3/README.mdhyperf-otel/8.3/rootfs/entrypoint.shhyperf-otel/8.3/rootfs/etc/otel-collector-config.yamlhyperf-otel/8.3/rootfs/etc/supervisord.confhyperf-otel/latest/Dockerfilehyperf-otel/latest/README.mdhyperf-otel/latest/rootfs/entrypoint.shhyperf-otel/latest/rootfs/etc/otel-collector-config.yamlhyperf-otel/latest/rootfs/etc/supervisord.conf
Comment on lines
+23
to
+24
| - name: Checkout | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
Pin every GitHub Action uses: to an immutable commit SHA (not a moving tag).
In .github/workflows/build-images.yml, these uses: entries reference mutable tags: actions/checkout@v4, tj-actions/changed-files@v44, docker/setup-buildx-action@v3, docker/login-action@v3, and docker/build-push-action@v6. Pin each to OWNER/REPO@<full_sha> (optionally keep the human-readable tag in a comment).
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 23-24: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 24-24: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for 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.
In @.github/workflows/build-images.yml around lines 23 - 24, Update every GitHub
Action `uses:` in the workflow to pin to an immutable commit SHA instead of a
moving tag: replace actions/checkout@v4, tj-actions/changed-files@v44,
docker/setup-buildx-action@v3, docker/login-action@v3, and
docker/build-push-action@v6 with their corresponding OWNER/REPO@<full_sha>
values (you can look up each repo’s latest stable commit SHA) and optionally
retain the human-readable tag as a trailing comment; ensure the changes are made
where those `uses:` entries appear so the workflow references specific commit
SHAs.
Contributor
Author
There was a problem hiding this comment.
Skipping this for now to stay consistent with the rest of the repo — .github/workflows/copilot-sandbox.yml uses tag pins for the same docker/* actions and actions/checkout. Pinning to commit SHAs is reasonable but is a repo-wide policy decision that should be paired with Dependabot for action SHAs; otherwise the workflows drift apart in conventions. Happy to revisit as a separate PR.
There was a problem hiding this comment.
@wilcorrea, that's a fair point — consistency across workflows matters, and introducing SHA pins in isolation without Dependabot or a repo-wide policy would just create two different conventions. Would you like me to open a follow-up issue to track the "pin all action refs to commit SHAs + enable Dependabot for Actions" work so it doesn't get lost?
(ᵔᴥᵔ) 🐰 noted
wilcorrea
force-pushed
the
feat/hyperf-otel-image
branch
from
a215cbc to
458b51b
Compare
wilcorrea
changed the title
Restructures hyperf/ to follow the {image}/{version} pattern used by
aws-cli, node, quasar, vue and merges the standalone hyperf-otel image
into hyperf/8.3 as a multi-stage variant.
Targets:
- hyperf -> devitools/hyperf:8.3 (lean, ~97MB, ENTRYPOINT=php)
- hyperf-otel -> devitools/hyperf:8.3-<col> (~463MB, ENTRYPOINT=supervisord)
Collectors live in hyperf/8.3/otel/collectors/*.yaml. New variants can
be added by dropping a yaml file. Two ship in this PR:
- debug (vendor-neutral, logs spans to stderr)
- google (googlecloud exporter, replaces previous gcloud-specific default)
hyperf/latest is a symlink to 8.3.
Adds .github/workflows/build-images.yml: detects changed {image}/{version}
folders on push to main, follows latest symlinks, and publishes one tag
per (version, target) combo to Docker Hub via DOCKERHUB_USERNAME and
DOCKERHUB_TOKEN secrets.
wilcorrea
force-pushed
the
feat/hyperf-otel-image
branch
from
458b51b to
b7d9de4
Compare
- workflows: declare explicit contents:read permissions and pass
GitHub-context values through env: instead of inline ${{ }}
expansion (mitigates template injection risk)
- workflow_dispatch: require target input (avoids empty matrix on
manual runs without input)
- hyperf/8.3/Dockerfile: verify otelcol-contrib tarball sha256
against the published opentelemetry-collector-releases checksums
- hyperf/8.3/otel/entrypoint.sh: whitelist PGBOUNCER_DATABASES
alias chars before eval, and fail fast when a required
POSTGRES_DB_* var is empty for an alias
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
hyperf/to follow the{image}/{version}pattern used byaws-cli,node,quasar,vue.hyperf-otel/folder intohyperf/8.3/as a multi-stage variant — same image, different--target.variants.yamlmanifest per{image}/{version}that declares which tags to build (target + suffix + optional build-args). Replaces image-specific scanning conventions.hyperf/latestis a symlink tohyperf/8.3.build-images.yml: on push tomain, readsvariants.yamlof changed{image}/{version}folders (followslatestsymlinks) and publishes one Docker Hub tag per entry.validate.yml: onpull_request, validates structure of every changedvariants.yaml.Why
The previous
hyperf-otelimage bundled agooglecloud-specific OTEL config that wouldn't fit AWS/Azure/on-prem consumers. Making the collector and dev-tools toggles configurable via build-args + publishing multiple variants lets consumers pick the right one atFROMtime without rebuilding the base. Merging back intohyperf/removes duplication; the OTEL layer becomes an opt-in second stage.Variant manifest format
Rules:
target(non-empty) andsuffix(key required; empty value = plain:<version>tag).args(optional) is a mapping passed as--build-arg KEY=VALUEto docker build.<image>:<version>if suffix empty, else<image>:<version>-<suffix>.variants.yamlget a single plain build (<image>:<version>from default Dockerfile target). Backwards-compatible withaws-cli,node, etc.Tags produced from this PR
hyperfdevitools/hyperf:8.3hyperfAPP_TARGET=devdevitools/hyperf:8.3-devhyperf-otelCOLLECTOR=debugdevitools/hyperf:8.3-otelhyperf-otelCOLLECTOR=googledevitools/hyperf:8.3-googlePlus matching
:latest,:latest-dev,:latest-otel,:latest-googlevia the symlink (workflow detects the symlink and includes it in the matrix when the canonical version changes).Workflows
build-images.yml(on push to main)*/*/Dockerfile,*/*/variants.yaml,*/*/rootfs/**,*/*/.scripts/**,*/*/otel/**, or*/latest.{image}/{version}dir, followslatestsymlinks pointing to it.(image, version): readsvariants.yamlviayq → jq(both pre-installed on Ubuntu runners) and emits one matrix entry per variant.docker/build-push-action@v6call (linux/amd64) withtargetandbuild-argsderived from the manifest. GHA cache scoped per tag.validate.yml(on PRs)*/*/variants.yaml.yq).targetand thesuffixkey (value may be empty).argsis a mapping when present.DockerfiledeclaresFROM ... AS <target>for every referenced target.Adding new variants
Drop a row in
<image>/<version>/variants.yaml. The build-images workflow picks it up on the next push to main. The validate workflow catches malformed entries during the PR.Examples:
{ target: hyperf-otel, suffix: jaeger, args: { COLLECTOR: jaeger } }(with matchinghyperf/8.3/otel/collectors/jaeger.yaml).{ target: hyperf, suffix: arm, args: { TARGETPLATFORM: linux/arm64 } }.Local validation
docker buildwith appropriate--targetand--build-arg.otelcol-contrib/supervisord— backwards compatible with currentdevitools/hyperf:8.3consumers.validate.ymllogic smoke-tested locally: catches missing target/suffix, invalid args type, missing Dockerfile stage, and duplicate suffix tags.yq → jqparser produces the expected 4 matrix entries from the manifest.Required secrets
DOCKERHUB_USERNAME✅ configuredDOCKERHUB_TOKEN✅ configuredTest plan
validate.yml; passes on this branch (manifest is valid)build-images.ymlhyperf/8.3× 4 variants +hyperf/latest× 4 variants):8.3and:latest,:8.3-devand:latest-dev, etc.docker pull devitools/hyperf:8.3(lean) is ~97MB withphpentrypointdocker pull devitools/hyperf:8.3-otelis ~463MB with/entrypoint.sh(supervisord)