Added ECR design document#647
Conversation
Co-authored-by: claude-sonnet-4-6 <claude-sonnet-4-6@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 33 minutes and 17 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: openshift-online/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a design document for migrating ROSA Regional Platform image consumption from Quay.io to AWS ECR. The document describes the image sets, HyperShift image resolution changes, mirroring pipeline extension, ECR authentication, consequences, and open questions. ChangesECR image migration design
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 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: 4
🤖 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 `@docs/design/ecr-image-migration.md`:
- Around line 9-12: The repository-placement decision is inconsistent between
the summary and the open question, so update the affected design sections to
agree on a single choice. Use the language in the migration summary and the
later “shared vs separate repositories” discussion to either confirm that
platform images use the same regional ECR repositories as OCP images or
explicitly mark that as undecided, and make the wording consistent across the
document.
- Around line 145-162: The image pull flow block is still ASCII art with an
unlabeled fence, which should be replaced with a Mermaid diagram in this
markdown doc. Convert the diagram in the design section to Mermaid syntax,
preserving the same nodes and flows shown by the image pull flow and clearly
representing the kubelet, node IAM role, ECR credential provider, ECR regional
images, and the HCP control plane pods with existing HyperShift auth. Use the
diagram’s existing labels and structure so it remains readable in markdown.
- Around line 131-135: The ECR IAM policy description in the MC/RC node pools
and system components section incorrectly suggests both actions can be scoped to
repository-level resources. Update the wording to distinguish
`ecr:GetAuthorizationToken` as requiring Resource `*` and `ecr:BatchGetImage` as
limited to specific repository ARNs, using the same paragraph in the design doc
so the policy scope is described correctly.
- Around line 136-143: The HCP dataplane pod auth description is too vague and
reads like a static pull secret approach, so update the Hosted Clusters section
to explicitly define the dynamic ECR authentication mechanism. In the ECR image
migration design, clarify how HyperShift Operator-managed auth will refresh
tokens for HCP pods (for example via AWS Pod Identity/IRSA or an operator-driven
rotation flow), and make sure the wording around “existing pull secret handling”
does not imply a non-rotating imagePullSecrets setup.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6c799f7d-585f-486f-939b-ad165a9a1ba4
📒 Files selected for processing (1)
docs/design/ecr-image-migration.md
- Reconcile repository-placement: clarify that platform images use the same regional ECR repo structure as OCP images (already decided); narrow open question 4 to naming/namespace conventions within that shared structure - Fix IAM policy scope: ecr:GetAuthorizationToken requires Resource '*' (account-level action, cannot be scoped to repository ARNs); ecr:BatchGetImage/GetDownloadUrlForLayer should use repo-scoped ARNs - Define dynamic auth for HCP pods: replace vague 'pull secret handling' with explicit requirement for Pod Identity / IRSA-based dynamic token rotation; explain why static imagePullSecrets are unsuitable (12h expiry) - Convert ASCII art diagram to Mermaid as required by repo conventions
fix: address CodeRabbit review comments on ECR image migration design doc
|
|
||
| ## Summary | ||
|
|
||
| Migrate all images consumed by the ROSA Regional Platform — both the Hosted Control Plane (HCP) |
There was a problem hiding this comment.
| Migrate all images consumed by the ROSA Regional Platform — both the Hosted Control Plane (HCP) | |
| Migrate all images consumed by ROSA Hyperfleet — both the Hosted Control Plane (HCP) |
| Migrate all images consumed by the ROSA Regional Platform — both the Hosted Control Plane (HCP) | ||
| dataplane images and the platform operational images (kubeapplier, hyperfleet-operator, etc.) — | ||
| from Quay.io to Amazon ECR. OCP images already mirrored to ECR by the OpenShift release process | ||
| are reused directly. Platform images built via Konflux are mirrored into the same regional ECR |
There was a problem hiding this comment.
@psav it just occurred to me that there might be an additional latency (waiting for the image landing in ECR) if we use this process instead of pushing our images directly from Konflux.
Let's say you merge to main, then our AWS CodePipelines for the intetration environment are triggered immediately and ArgoCD tries to pull the new image, but it's not there yet. Have you considered this flow, what would happen here?
I see a separation between the OCP payload, which is fine to be async and latency in having it in ECR doesn't matter, vs our development images, that would need to land in ECR asap.
| Hosted Clusters. ECR image sourcing must therefore be achieved without setting this flag. | ||
| - Platform images (kubeapplier, hyperfleet-operator, and others) must be available in the same | ||
| regional ECR repositories as OCP images so that MC/RC workloads can pull them without public | ||
| internet egress. |
There was a problem hiding this comment.
is it worth to explain WHY this is needed/desired?
| management, storage provisioning, replication, and a new failure domain. Rejected. | ||
|
|
||
| 2. **Use ECR for dataplane images only, retain Quay for infra images**: Introduces two | ||
| paths for obtaining images. Rejected. |
There was a problem hiding this comment.
- Push to ECR from konflux directly?
|
|
||
| ### Mirroring: Extending the OCP Pipeline for Platform Images | ||
|
|
||
| OCP already operates a mirroring pipeline that replicates release images from Quay to regional ECR |
| The preferred approach is a dynamic credential mechanism such as AWS Pod Identity (EKS Pod | ||
| Identity Associations) or IRSA (IAM Roles for Service Accounts), which allows the HyperShift | ||
| Operator to obtain short-lived ECR tokens on demand without operator intervention. An | ||
| investigation should determine which mechanism integrates most cleanly with HyperShift's |
There was a problem hiding this comment.
agreed, this is the sticky point. How do you give access to the data plane to access our private registry? How does it work today with ROSA?. This is the main thing that I think should be hashed out in this design doc.
| requirements; extending it to cover platform images increases its blast radius. | ||
| - ECR repository provisioning (IAM policies, lifecycle rules, replication configuration) must be | ||
| managed per region and per image set, adding Terraform/IaC surface area. | ||
| - Forcing HyperShift to pull from ECR without `--zero-egress` requires either upstream changes or |
There was a problem hiding this comment.
But is this not supported today by ROSA?
| ### Operability | ||
|
|
||
| - The mirroring pipeline extension must be automated. Manual mirroring of platform images is not | ||
| acceptable at the cadence of Konflux builds. |
There was a problem hiding this comment.
I don't understand this sentence
3 participants
Summary by CodeRabbit