feat(docker): Move the ca-trust library here from clp-plugin-presto-connector. - #120
feat(docker): Move the ca-trust library here from clp-plugin-presto-connector.#120jackluo923 wants to merge 2 commits into
Conversation
… containerized builds. Ported from clp-plugin-presto-connector's tools/build-packages/internal/ca-trust with README example paths updated to the consumer-facing submodule path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WalkthroughAdds host CA bundle staging, container TLS environment configuration, and optional Java PKCS#12 trust-store generation. The change also adds usage documentation and links the CA trust guide from the documentation index. ChangesCA trust propagation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Host
participant host.sh
participant container.sh
participant generate.sh
participant ContainerTools
Host->>host.sh: Discover and stage ca-bundle.pem
host.sh->>container.sh: Provide mounted CA_TRUST_DIR
container.sh->>ContainerTools: Export PEM bundle variables
container.sh->>generate.sh: Generate truststore.p12 when CA_TRUST_JVM is set
generate.sh->>container.sh: Return generated trust-store path
container.sh->>ContainerTools: Append Maven trust-store options
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@exports/docker/ca-trust/container.sh`:
- Line 22: Clarify the intended scope of HOST_CA_BUNDLE and
HOST_CA_JAVA_TRUST_STORE in container.sh: if they are internal working
variables, unset both after use alongside the other temporary variables; if they
are public outputs for the calling build script, export both so child processes
can consume them, matching MAVEN_OPTS behavior. Apply the same treatment
consistently to both variables.
- Line 22: Update container.sh to source the adjacent host.sh before
constructing HOST_CA_BUNDLE, then use host.sh’s CA_TRUST_BUNDLE_FILENAME
constant instead of the hardcoded “ca-bundle.pem” filename. Preserve the
existing CA_TRUST_DIR-based path resolution.
- Around line 32-37: Update the CA_TRUST_JVM guard in the container trust-store
setup to emit a warning when CA_TRUST_JVM=1 is set, the staged bundle is
non-empty, and keytool is unavailable, while preserving the silent no-op for
empty bundles and the existing setup path when keytool exists.
In `@exports/docker/ca-trust/host.sh`:
- Line 79: Add an explicit ERROR message to the directory-resolution failure
path in stage_host_ca_bundle, immediately before returning when cd
"${trust_dir}" fails. Preserve the existing return behavior and match the
diagnostics used by the surrounding failure paths.
In `@exports/docker/ca-trust/README.md`:
- Around line 20-27: Update the README quick-start example around
stage_host_ca_bundle to check its exit status and stop execution when staging
fails, before any subsequent Docker invocation; preserve the existing
temporary-directory setup and cleanup behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 20b5f81e-7da1-4c31-8df8-e2b8f2b11dc9
📒 Files selected for processing (6)
docs/index.mdexports/docker/ca-trust/README.mdexports/docker/ca-trust/container.shexports/docker/ca-trust/generators/java-pkcs12/README.mdexports/docker/ca-trust/generators/java-pkcs12/generate.shexports/docker/ca-trust/host.sh
…ocs index. Explain the /repo mount and submodule-path assumptions in the quick start, document expired-cert filtering, and tighten both READMEs to the repo's markdownlint style. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9a79bc7 to
b20ea7a
Compare
Description
Why
Corporate networks often run a TLS-inspecting gateway that re-signs HTTPS traffic with the company's own certificate authority. Your laptop trusts it; a container doesn't. Builds then fail with certificate errors that give no hint of the cause, so each project invents its own fix —
y-scope/clpandy-scope/clp-plugin-presto-connectorboth had one, and they agreed on almost nothing.The connector's was the best of them and had already been factored into a small library. This PR moves it here so there's one home for it. Nothing uses it yet — support for supplying certificates during image builds, which
y-scope/clpneeds, is stacked on top in #119.What changed
A port of
tools/build-packages/internal/ca-trust/fromy-scope/clp-plugin-presto-connector, plus README polish and a link from the docs index.host.sh— discovers the host's CA bundle and stages it into a caller-owned directory, dropping expired certificates on the way.container.sh— sourced inside the container; exportsCURL_CA_BUNDLE,GIT_SSL_CAINFO,PIP_CERT,REQUESTS_CA_BUNDLE, andSSL_CERT_FILEwhen a bundle is present, and optionally builds a Java trust store for Maven.generators/java-pkcs12/— the Java trust-store backend.Four small fixes on top of the port, from review:
CA_TRUST_JVM=1with an empty bundle or nokeytoolnow warns instead of skipping silently — the symptom otherwise appears much later as a PKIX error inside a JVM build.stage_host_ca_bundleprints an error on its one previously silent exit (a trust directory it can't enter).HOST_CA_BUNDLEandHOST_CA_JAVA_TRUST_STOREare unset with the other internals, instead of being left in the caller's namespace. Nothing outside the library reads them.Checklist
Validation performed
This is mostly a move, so the useful check is that it is one. Against the connector at
ec70485:generators/java-pkcs12/generate.shis byte-identical.container.shandhost.shdiffer only by the four fixes above, plus a comment inhost.shthat named a connector-specific script.Each fix was exercised directly: the two warnings by sourcing
container.shwithCA_TRUST_JVM=1against an empty bundle and withkeytooloffPATH, and the new error by pointingstage_host_ca_bundleat a directory it can't enter. The empty-bundle warning also has a test in #122.Nothing in this repo uses the library yet, so there is no behaviour to exercise. Tests arrive in #122, where they can cover both halves of the library at once.
Summary by CodeRabbit
New Features
Documentation