feat: support running skillctl in OpenShift containers#8
Conversation
Multi-stage Dockerfile now compiles the binary inside the container (no host cross-compilation needed) and creates an OpenShift-compatible home directory writable by GID 0. Add --tls-verify flag to inspect, push, pull, and promote commands for registries with self-signed certificates (e.g. OpenShift internal registry). Fix digest reference parsing in splitRefTag and parseNameFromTag so @sha256:... references resolve correctly. Update README with namespace field docs, remote naming convention, and OpenShift usage instructions. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Pavel Anni <panni@redhat.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 50 minutes and 40 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis pull request introduces TLS certificate verification control across OCI client commands and improves reference parsing to handle digest-style references. The Dockerfile is updated to a multi-stage build with proper binary compilation and home directory setup. Documentation adds guidance on namespace conventions and OpenShift integration workflows. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Address code review findings from Kimi-2.6: - Fix skillNameFromRef to strip @sha256:... before extracting the skill name (was producing "skill@sha256" directory names) - Use splitRefTag in imageFromManifest for consistent tag extraction - Add unit tests for splitRefTag, parseNameFromTag, and skillNameFromRef covering digest refs, port-based registries, and edge cases Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Pavel Anni <panni@redhat.com>
![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
🧹 Nitpick comments (4)
README.md (1)
183-186: Consider line-breaking the--overridesJSON for readability.The single-line JSON is hard to read/copy-edit. Either pretty-print it inline (shell accepts multi-line single-quoted strings) or reference a small
overrides.jsonfile via--overrides="$(cat overrides.json)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 183 - 186, The current oc run skillctl command uses a single-line --overrides JSON which is hard to read; update the invocation to either pretty-print the JSON inside the single-quoted --overrides string (split across lines) or move the JSON into a small overrides.json file and change the invocation to use --overrides="$(cat overrides.json)"; target the oc run skillctl command and the --overrides argument so the JSON becomes readable and easy to edit.pkg/oci/inspect.go (1)
26-32: Breaking signature change on exported method.
InspectRemotegained a requiredskipTLSVerifyparameter. Any external importer ofpkg/ociwill fail to build. If external consumers are a concern, consider adding anInspectOptionsstruct (mirroringPullOptions/PushOptions/PromoteOptions) for a non-breaking, forward-compatible API; otherwise call this out in release notes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/oci/inspect.go` around lines 26 - 32, The exported InspectRemote gained a breaking boolean parameter; add a non-breaking InspectOptions struct (similar to PullOptions/PushOptions/PromoteOptions) and change the implementation to accept an *InspectOptions (or nil) instead of a raw bool, e.g. InspectRemote(ctx context.Context, ref string, opts *InspectOptions) and use opts.SkipTLSVerify when calling newRemoteRepository/inspect; then provide a thin shim InspectRemote(ctx, ref string) that calls the new signature with nil for backward compatibility (or vice‑versa: keep current signature and add InspectRemoteWithOptions) so external callers don’t break. Ensure you reference InspectOptions.SkipTLSVerify in the newRemoteRepository call and update any internal calls to use the new API.pkg/oci/ref_test.go (1)
5-66: Good coverage of the digest/port edge cases.Table-driven tests exercise the port-in-host + digest combination that motivated the parsing fix. Consider adding a subtest name via
t.Run(tt.ref, ...)so failures pinpoint the offending case, and one case for an empty-digest boundary (name@) if that's considered invalid input worth locking down — otherwise this is sufficient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/oci/ref_test.go` around lines 5 - 66, Add subtest names to the table-driven tests so failures show the offending ref: in TestSplitRefTag, TestParseNameFromTag, and TestSkillNameFromRef wrap the loop body with t.Run(tt.ref, func(t *testing.T) { ... }) (remember to capture tt := tt inside the loop). Also consider adding an explicit empty-digest boundary case to TestSplitRefTag such as the ref "name@" with expected repo "name" and empty tag to lock down behavior for that input; update assertions inside the subtest bodies to use the local t.pkg/oci/push.go (1)
61-71: Optional: pinMinVersionon the TLS config.Static analysis flags the missing
MinVersion. Even withInsecureSkipVerify: true(certificate identity not validated), pinning a minimum protocol version still avoids negotiating weak/legacy TLS when talking to a misconfigured internal registry. Low risk given this is opt-in via--tls-verify=false, so treat as a nice-to-have hardening.🔒 Proposed hardening
- TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec // user-requested via --tls-verify=false + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, //nolint:gosec // user-requested via --tls-verify=false + MinVersion: tls.VersionTLS12, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/oci/push.go` around lines 61 - 71, The TLS config created when skipTLSVerify is true should also pin a minimum protocol version to avoid negotiating legacy TLS; update the tls.Config instantiation used for authClient.Client (the TLSClientConfig in the http.Transport assigned when skipTLSVerify is true) to set MinVersion to tls.VersionTLS12 (or higher), keeping the existing InsecureSkipVerify and the nolint comment as appropriate so repo.Client still uses the authClient with a minimum TLS version enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 182-190: Update the oc run invocation and notes: when using oc run
skillctl --rm add the stdin/attach flag (e.g., -i or --attach=true) so the --rm
cleanup runs, or explicitly instruct users to run oc delete pod skillctl after
the pod reaches Completed; also clarify the token expiry by mentioning oc whoami
-t is short-lived and users must recreate the secret when it expires.
---
Nitpick comments:
In `@pkg/oci/inspect.go`:
- Around line 26-32: The exported InspectRemote gained a breaking boolean
parameter; add a non-breaking InspectOptions struct (similar to
PullOptions/PushOptions/PromoteOptions) and change the implementation to accept
an *InspectOptions (or nil) instead of a raw bool, e.g. InspectRemote(ctx
context.Context, ref string, opts *InspectOptions) and use opts.SkipTLSVerify
when calling newRemoteRepository/inspect; then provide a thin shim
InspectRemote(ctx, ref string) that calls the new signature with nil for
backward compatibility (or vice‑versa: keep current signature and add
InspectRemoteWithOptions) so external callers don’t break. Ensure you reference
InspectOptions.SkipTLSVerify in the newRemoteRepository call and update any
internal calls to use the new API.
In `@pkg/oci/push.go`:
- Around line 61-71: The TLS config created when skipTLSVerify is true should
also pin a minimum protocol version to avoid negotiating legacy TLS; update the
tls.Config instantiation used for authClient.Client (the TLSClientConfig in the
http.Transport assigned when skipTLSVerify is true) to set MinVersion to
tls.VersionTLS12 (or higher), keeping the existing InsecureSkipVerify and the
nolint comment as appropriate so repo.Client still uses the authClient with a
minimum TLS version enforced.
In `@pkg/oci/ref_test.go`:
- Around line 5-66: Add subtest names to the table-driven tests so failures show
the offending ref: in TestSplitRefTag, TestParseNameFromTag, and
TestSkillNameFromRef wrap the loop body with t.Run(tt.ref, func(t *testing.T) {
... }) (remember to capture tt := tt inside the loop). Also consider adding an
explicit empty-digest boundary case to TestSplitRefTag such as the ref "name@"
with expected repo "name" and empty tag to lock down behavior for that input;
update assertions inside the subtest bodies to use the local t.
In `@README.md`:
- Around line 183-186: The current oc run skillctl command uses a single-line
--overrides JSON which is hard to read; update the invocation to either
pretty-print the JSON inside the single-quoted --overrides string (split across
lines) or move the JSON into a small overrides.json file and change the
invocation to use --overrides="$(cat overrides.json)"; target the oc run
skillctl command and the --overrides argument so the JSON becomes readable and
easy to edit.
🪄 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: CHILL
Plan: Pro Plus
Run ID: f5fbd631-e983-4402-b9f2-bea3acff99f2
📒 Files selected for processing (13)
DockerfileREADME.mdinternal/cli/inspect.gointernal/cli/promote.gointernal/cli/pull.gointernal/cli/push.gopkg/oci/client.gopkg/oci/inspect.gopkg/oci/pack.gopkg/oci/promote.gopkg/oci/pull.gopkg/oci/push.gopkg/oci/ref_test.go
- README: add -i flag to oc run for proper --rm cleanup, clarify token expiry duration - Add InspectOptions struct for API consistency with Push/Pull/Promote - Set TLS MinVersion to 1.2 in skip-verify transport - Use t.Run subtests in ref parsing tests for readable failures Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Pavel Anni <panni@redhat.com>
1 participant
Summary
--tls-verifyflag: added toinspect,push,pull, andpromotefor registries with self-signed certificates (e.g. OpenShift internal registry)splitRefTagandparseNameFromTagnow handle@sha256:...references correctlyoc run+ mounted auth secretsTest plan
go vet ./...passesgo test ./...passessplitRefTagparses digest refs, tag refs, and port-based registry refs correctlypodman buildon RHEL x86_64skillctl inspect --tls-verify=falseagainst OpenShift internal registry from a pod with mounted auth secret🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--tls-verifyflag toinspect,promote,pull, andpushcommands to control TLS certificate verification when accessing remote registries.Documentation
namespacefield semantics and registry path conventions.Chores