fix(release): quay promotion in Go#5255
Open
deepsm007 wants to merge 1 commit into
Open
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/hold
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (10)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
💤 Files with no reviewable changes (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughImplements Go-native Quay promotion orchestration via dedicated ChangesQuay Promotion Refactor and skippedImages Threading
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deepsm007 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![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 (2)
pkg/steps/release/quay_promotion.go (2)
574-586: 💤 Low valueString-based JSON parsing is fragile.
parseImageInfoDigestuses substring matching rather thanjson.Unmarshal. This works for simple cases but could break if the JSON contains escaped quotes within the digest value (unlikely but possible) or if the output format changes.Consider using proper JSON unmarshaling for robustness:
Alternative implementation
func parseImageInfoDigest(output string) (string, bool) { var info struct { Digest string `json:"digest"` } if err := json.Unmarshal([]byte(output), &info); err != nil || info.Digest == "" { return "", false } return info.Digest, true }🤖 Prompt for 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. In `@pkg/steps/release/quay_promotion.go` around lines 574 - 586, The parseImageInfoDigest function uses fragile string-based JSON parsing with substring matching (looking for the prefix "digest":" and finding the closing quote) which can break if the JSON contains escaped quotes within the digest value or if the output format changes. Replace this with proper JSON unmarshaling by defining a struct with a Digest field tagged with json:"digest", then use json.Unmarshal to parse the output string into this struct, returning the Digest field value and a bool indicating success (checking both for unmarshal errors and empty digest values).
485-494: ⚖️ Poor tradeoffMissing resource limits and security context on container.
Per coding guidelines, Kubernetes containers should have resource limits (
cpu,memory) and security hardening (runAsNonRoot,readOnlyRootFilesystem: falsesince KUBECACHEDIR writes to /tmp,allowPrivilegeEscalation: false). This pod runs in CI namespaces but should still follow best practices.Suggested addition
Containers: []coreapi.Container{{ Name: quayPromotionContainer, Image: cliImage, Command: []string{"sleep", fmt.Sprintf("%d", quayPromotionPodSleepSec)}, Env: []coreapi.EnvVar{ {Name: "KUBECONFIG", Value: promotionPodKubeconfigPath}, {Name: "KUBECACHEDIR", Value: "/tmp/.kube/cache"}, }, Resources: coreapi.ResourceRequirements{ Requests: coreapi.ResourceList{ coreapi.ResourceCPU: resource.MustParse("100m"), coreapi.ResourceMemory: resource.MustParse("256Mi"), }, Limits: coreapi.ResourceList{ coreapi.ResourceCPU: resource.MustParse("500m"), coreapi.ResourceMemory: resource.MustParse("512Mi"), }, }, SecurityContext: &coreapi.SecurityContext{ AllowPrivilegeEscalation: ptr.To(false), RunAsNonRoot: ptr.To(true), Capabilities: &coreapi.Capabilities{Drop: []coreapi.Capability{"ALL"}}, }, VolumeMounts: mounts, }},🤖 Prompt for 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. In `@pkg/steps/release/quay_promotion.go` around lines 485 - 494, Add resource limits and security hardening to the quayPromotionContainer in the Containers array. Add a Resources field with cpu and memory requests (100m cpu, 256Mi memory) and limits (500m cpu, 512Mi memory), and add a SecurityContext field configured with AllowPrivilegeEscalation set to false, RunAsNonRoot set to true, and Capabilities with all capabilities dropped. These additions follow Kubernetes best practices for pod security and resource management.Source: Coding guidelines
🤖 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 `@pkg/steps/release/quay_promotion.go`:
- Around line 273-276: The resolveAndTag method in quayPromotionRunner has a
potential panic because strings.LastIndex returns -1 when the colon substring is
not found in quayProxyTag, and using -1 as a slice bound causes a bounds panic.
Add a defensive check after the strings.LastIndex call to verify the returned
index is not -1; if it is, return an error indicating that quayProxyTag is
malformed (missing the expected colon separator). This ensures invalid input
cannot cause the function to panic.
---
Nitpick comments:
In `@pkg/steps/release/quay_promotion.go`:
- Around line 574-586: The parseImageInfoDigest function uses fragile
string-based JSON parsing with substring matching (looking for the prefix
"digest":" and finding the closing quote) which can break if the JSON contains
escaped quotes within the digest value or if the output format changes. Replace
this with proper JSON unmarshaling by defining a struct with a Digest field
tagged with json:"digest", then use json.Unmarshal to parse the output string
into this struct, returning the Digest field value and a bool indicating success
(checking both for unmarshal errors and empty digest values).
- Around line 485-494: Add resource limits and security hardening to the
quayPromotionContainer in the Containers array. Add a Resources field with cpu
and memory requests (100m cpu, 256Mi memory) and limits (500m cpu, 512Mi
memory), and add a SecurityContext field configured with
AllowPrivilegeEscalation set to false, RunAsNonRoot set to true, and
Capabilities with all capabilities dropped. These additions follow Kubernetes
best practices for pod security and resource management.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e6aaa0e5-336f-4f00-bab0-1449d7acd2b8
📒 Files selected for processing (10)
pkg/defaults/defaults.gopkg/steps/release/promote.gopkg/steps/release/promote_test.gopkg/steps/release/quay_promote_test.gopkg/steps/release/quay_promotion.gopkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_4.12.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_non_release_namespace.yamlpkg/steps/release/testdata/zz_fixture_TestQuayPromotionPod.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
💤 Files with no reviewable changes (4)
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_4.12.yaml
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_non_release_namespace.yaml
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay.yaml
- pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yaml
deepsm007
force-pushed
the
fix/promotion-quay-go
branch
2 times, most recently
from
71c6b73 to
193b133
Compare
deepsm007
force-pushed
the
fix/promotion-quay-go
branch
from
193b133 to
72c356f
Compare
Contributor
Author
|
/test e2e |
Contributor
Author
|
/test all |
Contributor
Author
|
/override ci/prow/e2e |
Contributor
|
@deepsm007: Overrode contexts on behalf of deepsm007: ci/prow/e2e DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Contributor
Author
|
/test integration |
Contributor
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
Contributor
|
@deepsm007: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
/hold
/cc @danilo-gemoli
Convert
[promotion-quay]from bash promotion pod to Go while keeping the same promotion logic and routing ([promotion] vs [promotion-quay]Go Implementation of the
promotion-quayPromotion StepThis pull request replaces the bash-based
promotion-quaypromotion pod with a Go-native implementation, while preserving the intentional separation between the generalpromotionflow and the Quay-specificpromotion-quayrouting.What changes for CI operators
promotion-quaystep is now executed by Go code (runQuayPromotion) instead of being driven by the generic promotion pod’s embedded shell logic.build_if_affectedis enabled, promotion planning now skips optional tool-image tag promotion unless those images are explicitly required, reducing unnecessary mirroring/tagging work.Key behavior changes
pkg/steps/release/promote.go)PromotionStepnow detects thepromotion-quaystep name and routes execution torunQuayPromotion(...)rather than applying Quay-specific behavior inside the generic promotion pod.build_if_affected)skippedImagesset.skippedImages(unless the tag’s image is also inrequiredImages).pkg/steps/release/quay_promotion.go)occommands inside apromotion-quaypod.ocoperations.Tests and fixtures
pkg/steps/release/quay_promote_test.go) covering promotion JSON generation, float phases, digest-pinned tagging, retry behavior,resolveAndTagerror cases, and pod spec generation (amd64 and arm64).pkg/steps/release/promote_test.go) to validateskippedImagesbehavior underbuild_if_affected, and removes older quay-shell-specific promotion pod fixture coverage.promotion-quaypod and replaces them with a Go-basedpromotion-quaypod fixture for spec validation.