introduce internal registry#81
Conversation
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughAdds internal registry support across CLI, API server, controller, and CRDs: new CLI flags and BuildRequest fields enable pushing to OpenShift internal registry; server pre-creates ImageStreams, mints short-lived service-account tokens, creates push secrets, and translates internal URLs to external routes; RBAC and CRD schemas updated to expose service-account auth. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Client
participant Validator as Flag Validator
participant CredHandler as Credential Handler
participant API as API Server
participant K8s as Kubernetes API
participant Registry as Internal Registry
CLI->>Validator: run build with --internal-registry
Validator->>CredHandler: enforce mutual exclusivity, set internal fields
CredHandler->>API: POST BuildRequest(useInternalRegistry=true,imageName,tag)
API->>K8s: ensure ImageStream exists
API->>K8s: create secret (dockerconfigjson from SA token)
API->>K8s: trigger Tekton Pipeline with secret
K8s->>API: pipeline/build completes
API->>K8s: mint short-lived SA registry token
API->>Registry: (push completed) return image refs
API->>CLI: BuildResponse(ContainerImage, DiskImage, RegistryToken, Warning)
sequenceDiagram
participant CLI as CLI Client
participant API as API Server
participant External as External Registry
CLI->>API: POST BuildRequest(useInternalRegistry=false, credentials)
API->>External: push using provided credentials
External->>API: push result
API->>CLI: BuildResponse(ContainerImage, DiskImage)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/buildapi/openapi.yaml (1)
201-212:⚠️ Potential issue | 🟡 Minor
BuildResponseschema is missing the newregistryTokenfield.
types.goaddsRegistryToken stringtoBuildResponse(line 218), but the OpenAPI schema here doesn't include it. Clients consuming the spec won't know about this field.Proposed fix
BuildResponse: type: object properties: name: type: string phase: type: string message: type: string requestedBy: type: string nullable: true + registryToken: + type: string + description: Short-lived token for pulling from the internal registry (only set when useInternalRegistry was used)
🤖 Fix all issues with AI agents
In `@cmd/caib/main.go`:
- Around line 606-633: In validateBootcBuildFlags(), remove the
duplicate/incorrect validation call or change its flag text: replace the first
call validateOutputRequiresPush(outputDir, exportOCI, "--push") with
validateOutputRequiresPush(outputDir, exportOCI, "--push-disk") (or simply
delete that first call) so that the exportOCI check uses the correct flag name
and you don't short-circuit the later, intended validation; keep the remaining
validateOutputRequiresPush(outputDir, exportOCI, "--push-disk") (or the single
corrected call) and ensure outputDir/exportOCI logic and useInternalRegistry
interactions remain unchanged.
- Around line 660-687: displayBuildResults currently ignores the error returned
by api.GetBuild, so failures are silent; update the GetBuild call in
displayBuildResults to handle err != nil by logging or printing the error
(including context like buildName) to stderr or the logger and then return early
(or continue appropriately) so the user sees why build results are missing;
refer to the GetBuild call and variables st, err, and buildName when adding the
error handling and message.
In `@internal/buildapi/server.go`:
- Around line 1484-1497: The getBuild flow silently keeps internal registry URLs
when getExternalRegistryRoute fails; update the logic around the
getExternalRegistryRoute call so that if err != nil you log the error (include
the error value) and attach a clear warning to the getBuild response indicating
the external registry lookup failed and internal URLs are returned; keep using
translateToExternalURL when externalRoute is present. Reference the
getExternalRegistryRoute call, the containerImage/diskImage variables,
translateToExternalURL, and the getBuild response object so the error is both
logged and surfaced to callers.
- Around line 1532-1536: The code currently calls mintRegistryToken for every
GET /builds/{name} when build.Spec.GetUseServiceAccountAuth() is true, which
mints a new 4-hour SA token on every request; change the logic to only call
mintRegistryToken when the build is in a terminal/completed phase (e.g., check
build.Status.GetPhase() or build.Status.Phase for Completed/Succeeded/Failed
states) so in-progress builds do not trigger token creation; keep the
registryToken variable and the call site (where registryToken =
mintRegistryToken(ctx, c, namespace)) but guard it with a phase check and return
an empty token or nil if the build is not completed.
- Around line 80-107: The getExternalRegistryRoute function currently swallows
all errors from the k8sClient.Get call for OperatorConfig; change the error
handling so that after calling k8sClient.Get(ctx, NamespacedName{Name: "config",
Namespace: namespace}, operatorConfig) you only fall through to Route
auto-detection when the error is a NotFound (errors.IsNotFound(err)), and for
any other error (RBAC, network, etc.) immediately return that error (or wrap it
with context) instead of continuing; update references in this function
(operatorConfig, k8sClient.Get) accordingly.
- Around line 200-223: mintRegistryToken currently swallows all errors and
returns an empty string, making failures indistinguishable from "no internal
registry"; fix by surfacing/logging errors: change mintRegistryToken to accept a
logger (or convert it to an APIServer method to use the existing logger) and log
non-nil errors returned from getRESTConfigFromRequest, kubernetes.NewForConfig,
and the CreateToken call (including context like namespace and serviceAccount
name), and return an explicit error (or at least return empty string plus log)
so callers can distinguish token-mint failures from "not used"; update callers
accordingly to handle the new signature or method.
🧹 Nitpick comments (3)
internal/buildapi/server.go (3)
102-102: Unchecked error fromNestedString.
unstructured.NestedStringreturns(string, bool, error). Bothfoundanderrare discarded. If the field exists but has the wrong type, this silently returns an empty host, leading to the confusing error "default-route exists but has no host".Proposed fix
- host, _, _ := unstructured.NestedString(route.Object, "spec", "host") + host, found, err := unstructured.NestedString(route.Object, "spec", "host") + if err != nil { + return "", fmt.Errorf("error reading host from default-route: %w", err) + } - if host == "" { + if !found || host == "" { return "", fmt.Errorf("default-route exists but has no host") }
128-130: JSON built viafmt.Sprintf— preferjson.Marshalfor correctness.The SA token is a JWT (base64url characters) so it won't break the
Sprintftemplate in practice. However,createRegistrySecret(line 849) already usesjson.Marshalfor the same purpose. Usingfmt.Sprintfhere is inconsistent and fragile if a token ever contains a"character.Proposed fix: use json.Marshal like the existing createRegistrySecret
- auth := base64.StdEncoding.EncodeToString([]byte("serviceaccount:" + tokenResp.Status.Token)) - dockerConfig := fmt.Sprintf(`{"auths":{"%s":{"auth":"%s"}}}`, defaultInternalRegistryURL, auth) + auth := base64.StdEncoding.EncodeToString([]byte("serviceaccount:" + tokenResp.Status.Token)) + dockerConfigMap := map[string]interface{}{ + "auths": map[string]interface{}{ + defaultInternalRegistryURL: map[string]string{ + "auth": auth, + }, + }, + } + dockerConfigBytes, err := json.Marshal(dockerConfigMap) + if err != nil { + return "", fmt.Errorf("error marshaling docker config: %w", err) + } + dockerConfig := string(dockerConfigBytes)
1151-1167:resolveRegistryForBuildhas mixed error semantics.When
setupBuildSecretsfails (line 1163), this function writes the HTTP response and returns an error. But the caller at line 1332 only checksapiErr != nilto bail. This works, but it's fragile — any future caller must know that the HTTP response was already written when err is non-nil. The internal-registry path (setupInternalRegistryBuild) follows the same pattern.Consider documenting this contract clearly in the function comment, since the current doc says "non-nil means the response was already written" but this pattern is easy to misuse.
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
force-pushed
the
internal-registry
branch
from
3e4cc2a to
4c3dd6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/caib/main.go`:
- Around line 735-739: Add upfront CLI validation to reject the unsupported
combination of flash with internal registry: inside validateBootcBuildFlags (and
duplicate the same check early in runDisk and runBuildDev) detect when
flashAfterBuild is true and useInternalRegistry is true (or equivalently when
--flash and --internal-registry flags are both set) and call handleError or
return an error with a clear message like "cannot enable --flash with
--internal-registry (not supported)"; this mirrors the server-side rejection and
prevents sending the request to the server. Ensure you reference the existing
variables flashAfterBuild, useInternalRegistry, and exportOCI where appropriate
and keep the same error-handling pathway used elsewhere (e.g., handleError).
In `@internal/buildapi/types.go`:
- Around line 218-219: The RegistryToken field in responses exposes a
short-lived service account token; ensure the build API only serves over TLS in
production by enforcing HTTPS at server start and in handlers: locate the build
API server startup (e.g., the code that registers HTTP handlers / starts the
listener or any NewBuildAPIServer/StartServer entrypoint) and require a valid
TLS config in production builds (fail-fast or panic if TLS is disabled), add
middleware to reject non-TLS requests or redirect HTTP -> HTTPS, and set strict
transport headers (HSTS) and secure cookie flags so responses containing
RegistryToken are never sent over plaintext.
🧹 Nitpick comments (3)
cmd/caib/main.go (1)
672-676: Registry token printed to stdout — consider security implications.
displayBuildResultsprints the SA token directly to stdout. This is standard CLI behavior for credential display, but the "valid ~4 hours" message is helpful. Consider adding a note to users that this token grants registry access.internal/buildapi/server.go (2)
112-158: Duplicate token-minting logic withmintRegistryToken.Both
createInternalRegistrySecret(Lines 118-129) andmintRegistryToken(Lines 214-224) independently create a 4-hour SA token for"pipeline"using nearly identical code. Consider extracting the common token-creation into a shared helper to keep the expiry and SA name in one place.Sketch of shared helper
+func mintPipelineSAToken(ctx context.Context, clientset kubernetes.Interface, namespace string) (string, error) { + expSeconds := int64(4 * 3600) + tokenReq := &authnv1.TokenRequest{ + Spec: authnv1.TokenRequestSpec{ + ExpirationSeconds: &expSeconds, + }, + } + tokenResp, err := clientset.CoreV1().ServiceAccounts(namespace). + CreateToken(ctx, "pipeline", tokenReq, metav1.CreateOptions{}) + if err != nil { + return "", fmt.Errorf("error creating SA token for pipeline in %s: %w", namespace, err) + } + return tokenResp.Status.Token, nil +}
1335-1338: Variable naming:apiErrshadows the intent — it's not just an API error.
resolveRegistryForBuildreturns a Goerrorthat signals "response already written," but the nameapiErrdoesn't convey that the HTTP response was already sent. A name likeregistryError a comment would clarify the contract.
| // Add flash configuration if enabled | ||
| if flashAfterBuild { | ||
| // Flash requires a disk image pushed to a registry | ||
| if exportOCI == "" { | ||
| if exportOCI == "" && !useInternalRegistry { | ||
| handleError(fmt.Errorf("cannot enable --flash without exporting a disk image (--push-disk)")) | ||
| } |
There was a problem hiding this comment.
CLI allows --flash --internal-registry but server rejects this combination.
The server explicitly rejects flash with useInternalRegistry (see internal/buildapi/server.go Line 1187-1189: "flash with useInternalRegistry is not yet supported"). The CLI should validate this up front for better UX rather than letting the user wait for a server-side error.
The same pattern applies in runDisk (Line 827) and runBuildDev (Line 1219).
Proposed fix (in validateBootcBuildFlags and similarly in runDisk/runBuildDev)
if useInternalRegistry {
if containerPush != "" || exportOCI != "" {
handleError(fmt.Errorf("--internal-registry cannot be used with --push or --push-disk"))
}
+ if flashAfterBuild {
+ handleError(fmt.Errorf("--flash with --internal-registry is not yet supported"))
+ }
}🤖 Prompt for AI Agents
In `@cmd/caib/main.go` around lines 735 - 739, Add upfront CLI validation to
reject the unsupported combination of flash with internal registry: inside
validateBootcBuildFlags (and duplicate the same check early in runDisk and
runBuildDev) detect when flashAfterBuild is true and useInternalRegistry is true
(or equivalently when --flash and --internal-registry flags are both set) and
call handleError or return an error with a clear message like "cannot enable
--flash with --internal-registry (not supported)"; this mirrors the server-side
rejection and prevents sending the request to the server. Ensure you reference
the existing variables flashAfterBuild, useInternalRegistry, and exportOCI where
appropriate and keep the same error-handling pathway used elsewhere (e.g.,
handleError).
| RegistryToken string `json:"registryToken,omitempty"` | ||
| Warning string `json:"warning,omitempty"` |
There was a problem hiding this comment.
RegistryToken is returned in API responses — ensure transport is TLS-only.
The RegistryToken field exposes a short-lived SA token in the HTTP response body. This is acceptable for the workflow, but make sure the build API is only served over TLS in production to avoid leaking the token in transit.
🤖 Prompt for AI Agents
In `@internal/buildapi/types.go` around lines 218 - 219, The RegistryToken field
in responses exposes a short-lived service account token; ensure the build API
only serves over TLS in production by enforcing HTTPS at server start and in
handlers: locate the build API server startup (e.g., the code that registers
HTTP handlers / starts the listener or any NewBuildAPIServer/StartServer
entrypoint) and require a valid TLS config in production builds (fail-fast or
panic if TLS is disabled), add middleware to reject non-TLS requests or redirect
HTTP -> HTTPS, and set strict transport headers (HSTS) and secure cookie flags
so responses containing RegistryToken are never sent over plaintext.
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@internal/buildapi/server.go`:
- Around line 1538-1547: The token minting currently logs errors from
mintRegistryToken but leaves registryToken empty, so update the build response
to surface minting failures via the response Warning field: when
mintRegistryToken returns an error (in the block that sets registryToken),
append a descriptive warning message (including the error message) to the
response's Warning slice/string (or the existing warnings aggregator used by the
handler) so the client/CLI can distinguish "mint failed" from "no internal
registry"; keep the existing log.Error call and ensure registryToken remains
unset when failed but the warning is populated so callers see the failure.
In `@internal/common/tasks/scripts/flash_image.sh`:
- Around line 58-68: The inline credential assignment using FULL_CMD and the
unquoted ${FULL_CMD} won't deliver OCI_USERNAME/OCI_PASSWORD to the remote
exporter because jmp shell runs on the remote agent and local exports/inline
VAR=... syntax are not forwarded; also unquoted ${FULL_CMD} risks
word-splitting. Fix by removing the inline credential wrapping of FLASH_CMD and
instead call jmp shell with explicit --env flags when credentials exist (use
--env OCI_USERNAME="..." --env OCI_PASSWORD="...") and pass FLASH_CMD directly
(properly quoted), invoking jmp shell with the existing JMP_CLIENT_CONFIG and
LEASE_NAME; alternatively, branch so the credentials are only added via --env
and not by modifying FULL_CMD.
In `@internal/common/tasks/tasks.go`:
- Around line 637-648: The Task currently exposes sensitive values via params
"flash-oci-username" and "flash-oci-password" in tasks.go which causes
credentials to be persisted in PipelineRun specs; change the task to stop using
plain-text params for these values and instead read them from a Kubernetes
Secret (either mount the Secret as a workspace or inject as env vars with
valueFrom.secretKeyRef in the step(s) that need them). Update the task param
definitions to remove "flash-oci-username" and "flash-oci-password" (or make
them non-sensitive references), add a Secret-based workspace or document the
Secret name/key expected, and modify the step(s) that previously referenced
those params to use env.valueFrom.secretKeyRef (or a file path from the mounted
Secret workspace) so credentials are not stored in PipelineRun/TaskRun specs.
In `@internal/controller/imagebuild/controller.go`:
- Around line 44-46: r.RestConfig is used without a nil-check before calling
kuberneteslib.NewForConfig (which will panic on nil). Add a nil-guard where
r.RestConfig is consumed (the code path that calls kuberneteslib.NewForConfig) —
check if r.RestConfig == nil and return an explicit error or log and exit early
(or create a clear panic with descriptive message) so consumers don’t hit an
unexpected nil; alternatively document the invariant on the controller type, but
prefer adding the runtime nil-check and an explicit error return near the call
to kuberneteslib.NewForConfig to make the failure visible and safe.
- Around line 476-506: Replace the hardcoded internal registry literal used in
the strings.Replace call that updates flashImageRef with the shared constant
instead of the string literal; locate the replacement in the function handling
flash image resolution where flashImageRef is set (the strings.Replace call
around "image-registry.openshift-image-registry.svc:5000") and switch it to use
the exported constant (e.g., defaultInternalRegistryURL) from the common package
where server.go defines it, adding the necessary import and updating any
references to the constant name so the code no longer duplicates the literal.
- Around line 493-505: The current logic silently continues when
kuberneteslib.NewForConfig or
clientset.CoreV1().ServiceAccounts(...).CreateToken fails, leaving
flashOCIUsername/flashOCIPassword empty; change this to abort the ImageBuild
creation with a clear error: when NewForConfig returns an error, return that
error (with context "failed to create clientset for flash OCI credentials")
instead of just logging; when CreateToken returns an error, return that error
(with context "failed to create SA token for flash OCI credentials") rather than
proceeding; ensure references to flashOCIUsername and flashOCIPassword are only
set after successful tokenResp.Status.Token retrieval and propagate the error
back to the caller so the build fails fast.
🧹 Nitpick comments (3)
internal/buildapi/server.go (3)
112-158: Duplicate token creation logic betweencreateInternalRegistrySecretandmintRegistryToken.Both
createInternalRegistrySecret(line 112) andmintRegistryToken(line 205) mint SA tokens with the same 4-hour expiry for thepipelineSA. Consider extracting the token-creation logic into a shared helper to reduce duplication.
1231-1244: Same secret returned as bothenvSecretRefandpushSecretName.
setupInternalRegistryBuildreturnssecretNamefor both the env secret ref and push secret name. Downstream increateBuild(lines 1389-1407),setSecretOwnerRefwill be called twice on the same secret — the second call overwrites the first with an identical owner reference. This is functionally harmless but could be confusing.More importantly, the existing
setupBuildSecretspath at line 1141-1149 enforces thatRegistryCredentialsmust be provided whenPushRepositoryorExportOCIis set. ButsetupInternalRegistryBuildsetsreq.ContainerPushandreq.ExportOCIon lines 1206-1217 after validating mutual exclusivity. SinceresolveRegistryForBuildis called before the normal path, this works correctly — just noting the implicit coupling.
1174-1201:setupInternalRegistryBuildmutates the request object in place.Lines 1206-1217 modify
req.ContainerPush,req.ExportOCI, andreq.BuildDiskImageas side effects. This makes the function harder to reason about and test. Consider returning the computed values and having the caller set them, or at minimum documenting this mutation contract clearly.
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
1 participant
Summary by CodeRabbit
New Features
Documentation