refactor: extract registry from server.go#249
Conversation
|
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 7 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThis PR introduces configurable registry token lifetime settings to the API, centralizes Kubernetes label/annotation constants into a dedicated package, and extracts registry orchestration logic into a new module that handles internal registry secret creation, token minting, and ImageStream lifecycle management with comprehensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as Build Handler
participant Resolver as Token Lifetime Resolver
participant Config as OperatorConfig
participant TokenMinter as Token Minter
participant K8s as Kubernetes API
participant K8sAuth as Service Account Auth
Handler->>Resolver: resolveTokenLifetime(ctx, k8sClient, namespace)
Resolver->>Config: Get OperatorConfig
alt Config has RegistryTokenLifetimeSeconds
Config-->>Resolver: Use configured value
else Config missing/empty
Config-->>Resolver: Use DefaultRegistryTokenLifetimeSeconds
end
Resolver-->>Handler: tokenLifetimeSeconds
Handler->>K8s: createInternalRegistrySecret(..., tokenLifetimeSeconds)
Handler->>TokenMinter: mintRegistryToken(ctx, c, namespace, tokenLifetimeSeconds)
TokenMinter->>K8sAuth: Create short-lived token for build service account
K8sAuth-->>TokenMinter: token + expiration
TokenMinter-->>Handler: token, metav1.Time
Handler->>K8s: Store dockerconfigjson secret with token
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/buildapi/registry_test.go (1)
116-131: Consider adding a failure-path test fordeleteImageStreamTag.
deleteImageStreamhas both success and "does not exist" cases (Lines 91-114), butdeleteImageStreamTagonly covers the happy path. Adding the symmetric case protects against regressions if the Delete semantics change.Suggested addition
+ + It("returns error when ImageStreamTag does not exist", func() { + scheme := newRegistryTestScheme() + k8sClient := newRegistryTestClient(scheme) + + err := deleteImageStreamTag(context.Background(), k8sClient, "ns", "missing-stream", "v1") + Expect(err).To(HaveOccurred()) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/buildapi/registry_test.go` around lines 116 - 131, Add a new test case beside the existing Describe("deleteImageStreamTag") spec that verifies deleteImageStreamTag returns no error (or appropriate behavior) when the ImageStreamTag does not exist: create a scheme without any existing newUnstructuredImageStreamTag, construct k8sClient via newRegistryTestClient, call deleteImageStreamTag(context.Background(), k8sClient, "ns", "my-stream", "v1"), and assert the result is appropriate (e.g., no error) and that a subsequent Get for the namespaced name "my-stream:v1" still returns a not-found error; this mirrors the failure-path test pattern used for deleteImageStream.internal/buildapi/registry.go (2)
232-261: Assumption worth documenting: single stream name across refs.The loop overwrites
streamNameon each iteration, which is correct today becausesetupInternalRegistryBuildinserver.goalways pushes both the bootc container and the disk image to the same ImageStream (distinguished only by tag). If that invariant ever changes — e.g., hybrid paths begin using distinct streams — this function will silently miss cleaning up one of them. The existing doc comment covers the URL pattern; consider noting the single-stream assumption too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/buildapi/registry.go` around lines 232 - 261, Update the comment on resolveImageStreamRefs to explicitly state it assumes both container push and export OCI refs point to the same ImageStream (single stream name across refs) because setupInternalRegistryBuild currently pushes both artifacts to the same stream; also add a runtime guard in resolveImageStreamRefs that detects if multiple different stream names appear across build.Spec.GetContainerPush() and build.Spec.GetExportOCI() and logs a warning (or returns an error) so future changes that use distinct streams are noticed; reference resolveImageStreamRefs, setupInternalRegistryBuild, build.Spec.GetContainerPush, and build.Spec.GetExportOCI when making the change.
55-60: Preserve underlying error for diagnosability.When
Geton the Route fails for reasons other than NotFound (RBAC denied, apiserver unreachable, CRD missing), the root cause is discarded and callers only see the generic guidance string. Consider wrapping the original error so operators can distinguish "route not configured" from "route lookup is failing".Suggested change
if err := k8sClient.Get(ctx, types.NamespacedName{ Name: "default-route", Namespace: "openshift-image-registry", }, route); err != nil { - return "", fmt.Errorf("cannot determine external registry route: set clusterRegistryRoute in OperatorConfig or expose default-route in openshift-image-registry") + return "", fmt.Errorf("cannot determine external registry route: set clusterRegistryRoute in OperatorConfig or expose default-route in openshift-image-registry: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/buildapi/registry.go` around lines 55 - 60, The current error returned from k8sClient.Get (call to k8sClient.Get with types.NamespacedName for "default-route" into variable route) discards the original error; change the error handling to preserve and wrap the underlying error so callers can distinguish "route not configured" from other failures (RBAC, apiserver unreachable, CRD missing). Specifically, after k8sClient.Get(...) check for apierrors.IsNotFound(err) to keep the existing guidance for a missing route (mentioning OperatorConfig/clusterRegistryRoute), but for any other error return a wrapped error that includes the original err (e.g., using fmt.Errorf("cannot determine external registry route: %w", err) or errors.Wrap) so the root cause is preserved in the returned error. Ensure references: k8sClient.Get, route, OperatorConfig, clusterRegistryRoute.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/buildapi/registry_test.go`:
- Around line 116-131: Add a new test case beside the existing
Describe("deleteImageStreamTag") spec that verifies deleteImageStreamTag returns
no error (or appropriate behavior) when the ImageStreamTag does not exist:
create a scheme without any existing newUnstructuredImageStreamTag, construct
k8sClient via newRegistryTestClient, call
deleteImageStreamTag(context.Background(), k8sClient, "ns", "my-stream", "v1"),
and assert the result is appropriate (e.g., no error) and that a subsequent Get
for the namespaced name "my-stream:v1" still returns a not-found error; this
mirrors the failure-path test pattern used for deleteImageStream.
In `@internal/buildapi/registry.go`:
- Around line 232-261: Update the comment on resolveImageStreamRefs to
explicitly state it assumes both container push and export OCI refs point to the
same ImageStream (single stream name across refs) because
setupInternalRegistryBuild currently pushes both artifacts to the same stream;
also add a runtime guard in resolveImageStreamRefs that detects if multiple
different stream names appear across build.Spec.GetContainerPush() and
build.Spec.GetExportOCI() and logs a warning (or returns an error) so future
changes that use distinct streams are noticed; reference resolveImageStreamRefs,
setupInternalRegistryBuild, build.Spec.GetContainerPush, and
build.Spec.GetExportOCI when making the change.
- Around line 55-60: The current error returned from k8sClient.Get (call to
k8sClient.Get with types.NamespacedName for "default-route" into variable route)
discards the original error; change the error handling to preserve and wrap the
underlying error so callers can distinguish "route not configured" from other
failures (RBAC, apiserver unreachable, CRD missing). Specifically, after
k8sClient.Get(...) check for apierrors.IsNotFound(err) to keep the existing
guidance for a missing route (mentioning OperatorConfig/clusterRegistryRoute),
but for any other error return a wrapped error that includes the original err
(e.g., using fmt.Errorf("cannot determine external registry route: %w", err) or
errors.Wrap) so the root cause is preserved in the returned error. Ensure
references: k8sClient.Get, route, OperatorConfig, clusterRegistryRoute.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d543e9ee-afd3-4bcc-a2c4-270a363272c7
⛔ Files ignored due to path filters (1)
config/crd/bases/automotive.sdv.cloud.redhat.com_operatorconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (13)
api/v1alpha1/operatorconfig_types.gointernal/buildapi/client_tokens.gointernal/buildapi/container_builds.gointernal/buildapi/flash.gointernal/buildapi/flash_helpers.gointernal/buildapi/flash_test.gointernal/buildapi/progress.gointernal/buildapi/registry.gointernal/buildapi/registry_test.gointernal/buildapi/server.gointernal/buildapi/server_test.gointernal/buildapi/workspace.gointernal/common/labels/labels.go
bennyz
force-pushed
the
refactor-server-registry
branch
from
cedd427 to
87a7674
Compare
|
@ambient-code please review |
![ambient-code[bot]](https://avatars.githubusercontent.com/u/231559327?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: refactor: extract registry from server.go
Overall this is a clean refactoring. The separation of registry-related functionality into registry.go is well-motivated, and the labels package is a good step toward eliminating scattered string literals. The configurable RegistryTokenLifetimeSeconds is a useful addition. A few observations below.
What looks good
- The
labelspackage is well-structured with clear grouping (domain-specific keys, standard k8s keys, common values). - All
createInternalRegistrySecret/mintRegistryTokencall sites are updated consistently to pass throughresolveTokenLifetime. - The
getExternalRegistryRoutefunction now properly distinguishesNotFoundfrom other errors when auto-detecting the Route (an improvement over the original behavior). - Test coverage for the new
registry.gofunctions (ImageStream CRUD,setSecretOwnerRef,createFlashClientSecret,getExternalRegistryRoute) is solid. - DeepCopy does not need changes since
RegistryTokenLifetimeSecondsis a value type -- correctly identified and left alone.
Suggestions
See inline comments.
| k8serrors "k8s.io/apimachinery/pkg/api/errors" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | ||
| "k8s.io/apimachinery/pkg/runtime/schema" |
There was a problem hiding this comment.
Nit (optional): registry.go now also contains createFlashClientSecret and setSecretOwnerRef, which are not registry-specific. These are fine here for now since they were co-located in server.go before, but if this file keeps growing, you might consider a further split (e.g., secrets.go for generic secret helpers). Not blocking.
bennyz
force-pushed
the
refactor-server-registry
branch
from
87a7674 to
deec981
Compare
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
bennyz
force-pushed
the
refactor-server-registry
branch
from
deec981 to
601ec35
Compare
2 participants
Summary
Related Issues
Type of Change
Testing
make test)make lint)make manifests generate)Summary by CodeRabbit
New Features
Refactor