Skip to content

Expand consolidated Quay promotion to OCP 4.11–4.22 #5198

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
deepsm007 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Expand consolidated Quay promotion to OCP 4.11–4.22 #5198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions pkg/api/promotion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,11 @@ func ConsolidatedQuayPromotion(c *ReleaseBuildConfiguration) bool {
if c == nil {
return false
}
if c.ReleaseTagConfiguration != nil && releaseVersionEquals(c.ReleaseTagConfiguration.Name, 4, 12) {
if c.ReleaseTagConfiguration != nil && consolidatedQuayPromotionVersion(c.ReleaseTagConfiguration.Name) {
return true
}
for _, target := range PromotionTargets(c.PromotionConfiguration) {
if releaseVersionEquals(target.Name, 4, 12) {
if consolidatedQuayPromotionVersion(target.Name) {
return true
}
}
Expand All @@ -130,8 +130,23 @@ func releaseVersionEquals(name string, major, minor int) bool {
return gotMajor == major && gotMinor == minor
}

func ConsolidatedQuayPromotionStream(name string) bool {
return consolidatedQuayPromotionVersion(name)
}

func consolidatedQuayPromotionVersion(name string) bool {
if releaseVersionEquals(name, 4, 23) || releaseVersionEquals(name, 5, 0) {
return false
}
var major, minor int
if _, err := fmt.Sscanf(name, "%d.%d", &major, &minor); err != nil {
return false
}
return major == 4 && minor >= 11 && minor <= 22
}
Comment on lines +137 to 146
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

In Go, with fmt.Sscanf and format "%d.%d", does scanning "4.12.1" or "4.12-foo" still return two successful assignments while leaving trailing input?

💡 Result:

Answer: No—both inputs will still successfully assign two values, and scanning does not require consuming the entire input. However, whether the second %d succeeds depends on what follows the first “.%d” component. For fmt.Sscanf with format "%d.%d": 1) Input "4.12.1" - First %d (%d) matches "4" and the literal '.' in the format matches the '.' after it. - Second %d matches "12" and stops when it encounters the next '.' (a non-digit for an integer scan). - Sscanf can stop before consuming all input (“need not consume its entire input”), so the remaining ".1" is left unconsumed. - Result: n == 2 (two successful assignments) and err == nil. 2) Input "4.12-foo" - First %d matches "4"; the '.' literal matches the '.' after it. - Second %d matches "12" and stops when it encounters '-' (not a digit for an integer scan). - Again, Sscanf need not consume its entire input, so the remaining "-foo" is left unconsumed. - Result: n == 2 (two successful assignments) and err == nil. Key rules from Go docs: - Sscanf returns the number of items successfully parsed, and it “need not consume its entire input.” [1] - Also, for Scanf/Sscanf, “text in the format string must match the input text; scanning stops if it does not,” and the return value indicates how many arguments were scanned. [2] Implication for your specific cases: after successfully matching “4” + "." + “12”, the parser is allowed to stop, so extra trailing characters like ".1" or "-foo" do not prevent the two assignments from succeeding. [1][2]

Citations:

Use strict parsing for consolidated-quay version gating (avoid fmt.Sscanf partial-match).

fmt.Sscanf(name, "%d.%d", ...) does not require consuming the whole string; values like 4.12.1 and 4.12-foo can still parse as major=4/minor=12 (n==2, err==nil), so they may incorrectly pass the consolidated-quay range checks. Switch to exact <major>.<minor> parsing.

Proposed fix 
 import (
 	"fmt"
+	"strconv"
 	"strings"
@@
 func consolidatedQuayPromotionVersion(name string) bool {
-	var major, minor int
-	if _, err := fmt.Sscanf(name, "%d.%d", &major, &minor); err != nil {
+	parts := strings.Split(name, ".")
+	if len(parts) != 2 {
+		return false
+	}
+	major, err := strconv.Atoi(parts[0])
+	if err != nil {
+		return false
+	}
+	minor, err := strconv.Atoi(parts[1])
+	if err != nil {
 		return false
 	}
 	if major != consolidatedQuayPromotionMajor {
 		return false
 	}
 	return minor >= consolidatedQuayPromotionMinMinor && minor <= consolidatedQuayPromotionMaxMinor
 }
🤖 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/api/promotion.go` around lines 136 - 145, The
consolidatedQuayPromotionVersion function currently uses fmt.Sscanf(name,
"%d.%d", &major, &minor) which allows partial matches like "4.12.1" to parse;
update the parsing to require an exact "<major>.<minor>" match (e.g. use a regex
anchored ^(\d+)\.(\d+)$ or split on '.' and validate there are exactly two
numeric parts with strconv.Atoi) so only fully-formed versions are accepted;
keep the rest of the function logic (compare major to
consolidatedQuayPromotionMajor and minor to
consolidatedQuayPromotionMinMinor/consolidatedQuayPromotionMaxMinor) unchanged
and return false on any parse failure or extra trailing characters.


func quayProxyStreamSuffix(tag ImageStreamTagReference) string {
if releaseVersionEquals(tag.Name, 4, 12) {
if consolidatedQuayPromotionVersion(tag.Name) {
return ""
}
return "-quay"
Expand Down
24 changes: 20 additions & 4 deletions pkg/api/promotion_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,23 @@ func TestQuayCombinedMirrorFunc(t *testing.T) {
expected: map[string]string{
"quay.io/openshift/ci:ocp_4.22_ovn-kubernetes": "registry.build02.ci.openshift.org/ci-op-abc/pipeline@sha256:abc123",
"quay.io/openshift/ci:20241024102030_prune_ocp_4.22_ovn-kubernetes": "quay.io/openshift/ci:ocp_4.22_ovn-kubernetes",
"ocp/4.22-quay:ovn-kubernetes": "quay-proxy.ci.openshift.org/openshift/ci@sha256:abc123",
"ocp/4.22:ovn-kubernetes": "quay-proxy.ci.openshift.org/openshift/ci@sha256:abc123",
},
},
{
name: "4.23 uses quay suffix",
source: "registry.build02.ci.openshift.org/ci-op-abc/pipeline@sha256:abc123",
target: "quay.io/openshift/ci:ocp_4.23_ovn-kubernetes",
tag: ImageStreamTagReference{
Namespace: "ocp",
Name: "4.23",
Tag: "ovn-kubernetes",
},
time: "20241024102030",
expected: map[string]string{
"quay.io/openshift/ci:ocp_4.23_ovn-kubernetes": "registry.build02.ci.openshift.org/ci-op-abc/pipeline@sha256:abc123",
"quay.io/openshift/ci:20241024102030_prune_ocp_4.23_ovn-kubernetes": "quay.io/openshift/ci:ocp_4.23_ovn-kubernetes",
"ocp/4.23-quay:ovn-kubernetes": "quay-proxy.ci.openshift.org/openshift/ci@sha256:abc123",
},
},
{
Expand Down Expand Up @@ -245,7 +261,7 @@ func TestQuayCombinedMirrorFunc(t *testing.T) {
},
time: "",
expected: map[string]string{
"ocp/4.22-quay:installer": "quay-proxy.ci.openshift.org/openshift/ci@sha256:xyz789",
"ocp/4.22:installer": "quay-proxy.ci.openshift.org/openshift/ci@sha256:xyz789",
},
},
{
Expand All @@ -261,7 +277,7 @@ func TestQuayCombinedMirrorFunc(t *testing.T) {
expected: map[string]string{
"quay.io/openshift/ci:ocp_4.20_cluster-api": "registry.build02.ci.openshift.org/ci-op-abc/pipeline@sha256:111222",
"quay.io/openshift/ci:20241024104500_prune_ocp_4.20_cluster-api": "quay.io/openshift/ci:ocp_4.20_cluster-api",
"ocp/4.20-quay:cluster-api": "quay-proxy.ci.openshift.org/openshift/ci@sha256:111222",
"ocp/4.20:cluster-api": "quay-proxy.ci.openshift.org/openshift/ci@sha256:111222",
},
},
{
Expand Down Expand Up @@ -309,7 +325,7 @@ func TestQuayCombinedMirrorFunc(t *testing.T) {
expected: map[string]string{
"quay.io/openshift/ci:ocp_4.22_ovn-kubernetes": "registry.build02.ci.openshift.org/ci-op-abc/pipeline:some-tag",
"quay.io/openshift/ci:20241024102030_prune_ocp_4.22_ovn-kubernetes": "quay.io/openshift/ci:ocp_4.22_ovn-kubernetes",
"ocp/4.22-quay:ovn-kubernetes": "registry.build02.ci.openshift.org/ci-op-abc/pipeline:some-tag",
"ocp/4.22:ovn-kubernetes": "registry.build02.ci.openshift.org/ci-op-abc/pipeline:some-tag",
},
},
}
Expand Down
28 changes: 28 additions & 0 deletions pkg/defaults/defaults_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1716,6 +1716,34 @@ func TestFromConfig(t *testing.T) {
promote: true,
expectedSteps: []string{"[output-images]", "[images]"},
expectedPost: []string{"[promotion-quay]"},
}, {
name: "promote 4.23 legacy quay",
config: api.ReleaseBuildConfiguration{
PromotionConfiguration: &api.PromotionConfiguration{
Targets: []api.PromotionTarget{{
Namespace: ns,
Name: "4.23",
Tag: "tag",
}},
},
},
promote: true,
expectedSteps: []string{"[output-images]", "[images]"},
expectedPost: []string{"[promotion]", "[promotion-quay]"},
}, {
name: "promote 5.0 legacy quay",
config: api.ReleaseBuildConfiguration{
PromotionConfiguration: &api.PromotionConfiguration{
Targets: []api.PromotionTarget{{
Namespace: ns,
Name: "5.0",
Tag: "tag",
}},
},
},
promote: true,
expectedSteps: []string{"[output-images]", "[images]"},
expectedPost: []string{"[promotion]", "[promotion-quay]"},
}, {
name: "duplicate input images",
config: api.ReleaseBuildConfiguration{
Expand Down
12 changes: 1 addition & 11 deletions pkg/steps/release/create_release.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,18 +334,8 @@ func joinOcAdmReleaseNewCommand(config *api.ReleaseTagConfiguration, namespace,
return strings.Join(cmd, " ")
}

func hasExactVersion(config *api.ReleaseTagConfiguration, majorVersion, minorVersion int) bool {
var major, minor int
n, err := fmt.Sscanf(config.Name, "%d.%d", &major, &minor)
if err != nil || n != 2 {
logrus.Warnf("Could not parse release version from release tag configuration name=%q: %v", config.Name, err)
return false
}
return major == majorVersion && minor == minorVersion
}

func buildOcAdmReleaseNewCommand(config *api.ReleaseTagConfiguration, namespace, streamName, cvo, destination, version string) string {
if !hasExactVersion(config, 4, 12) {
if !api.ConsolidatedQuayPromotionStream(config.Name) {
return joinOcAdmReleaseNewCommand(config, namespace, cvo, destination, version, "--from-image-stream", streamName)
}
filePathVar := "${_CI_RELEASE_IS_FILE}"
Expand Down
2 changes: 1 addition & 1 deletion pkg/steps/release/create_release_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ fi`
if diff := cmp.Diff(want, got); diff != "" {
t.Fatalf("buildOcAdmReleaseNewCommand() mismatch (-want +got):\n%s", diff)
}
got = buildOcAdmReleaseNewCommand(&api.ReleaseTagConfiguration{Name: "4.15", ReferencePolicy: &srcPol}, "test-ns", "stable", "cvo-pullspec", "dest:tag", "0.0.1-ver")
got = buildOcAdmReleaseNewCommand(&api.ReleaseTagConfiguration{Name: "4.23", ReferencePolicy: &srcPol}, "test-ns", "stable", "cvo-pullspec", "dest:tag", "0.0.1-ver")
want = "oc adm release new --max-per-registry=32 -n test-ns --from-image-stream stable --to-image-base cvo-pullspec --to-image dest:tag --name 0.0.1-ver --reference-mode=source --keep-manifest-list"
if diff := cmp.Diff(want, got); diff != "" {
t.Fatalf("buildOcAdmReleaseNewCommand() mismatch (-want +got):\n%s", diff)
Expand Down
8 changes: 4 additions & 4 deletions pkg/steps/release/promote_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -760,9 +760,9 @@ func TestGetPromotionPod(t *testing.T) {
"quay.io/openshift/ci:ocp_4.21_ovn-kubernetes": "registry.build02.ci.openshift.org/ci-op-y2n8rsh3/pipeline@sha256:aaa",
"quay.io/openshift/ci:ocp_4.21_ovn-kubernetes-base": "registry.build02.ci.openshift.org/ci-op-y2n8rsh3/pipeline@sha256:bbb",
"quay.io/openshift/ci:ocp_4.21_ovn-kubernetes-microshift": "registry.build02.ci.openshift.org/ci-op-y2n8rsh3/pipeline@sha256:ccc",
"ocp/4.21-quay:ovn-kubernetes": "quay-proxy.ci.openshift.org/openshift/ci@sha256:aaa",
"ocp/4.21-quay:ovn-kubernetes-base": "quay-proxy.ci.openshift.org/openshift/ci@sha256:bbb",
"ocp/4.21-quay:ovn-kubernetes-microshift": "quay-proxy.ci.openshift.org/openshift/ci@sha256:ccc",
"ocp/4.21:ovn-kubernetes": "quay-proxy.ci.openshift.org/openshift/ci@sha256:aaa",
"ocp/4.21:ovn-kubernetes-base": "quay-proxy.ci.openshift.org/openshift/ci@sha256:bbb",
"ocp/4.21:ovn-kubernetes-microshift": "quay-proxy.ci.openshift.org/openshift/ci@sha256:ccc",
},
namespace: "ci-op-9bdij1f6",
},
Expand All @@ -786,7 +786,7 @@ func TestGetPromotionPod(t *testing.T) {
nodeArchitectures: []string{"amd64"},
imageMirror: map[string]string{
"quay.io/openshift/ci:ocp_4.21_ovn-kubernetes": "registry.build02.ci.openshift.org/ci-op-y2n8rsh3/pipeline@sha256:aaa",
"ocp/4.21-quay:ovn-kubernetes": "quay-proxy.ci.openshift.org/openshift/ci@sha256:aaa",
"ocp/4.21:ovn-kubernetes": "quay-proxy.ci.openshift.org/openshift/ci@sha256:aaa",
"quay.io/openshift/ci:ci_ci_sanitize-prow-jobs": "registry.build02.ci.openshift.org/ci-op-y2n8rsh3/pipeline@sha256:bbb",
"ci/ci-quay:sanitize-prow-jobs": "quay-proxy.ci.openshift.org/openshift/ci@sha256:bbb",
},
Expand Down
28 changes: 26 additions & 2 deletions pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,32 @@ spec:
for r in {1..5}; do echo Mirror attempt $r; oc image mirror --loglevel=2 --keep-manifest-list --registry-config=/etc/push-secret/.dockerconfigjson --max-per-registry=10 registry.build02.ci.openshift.org/ci-op-y2n8rsh3/pipeline@sha256:bbb=quay.io/openshift/ci:ci_a_latest registry.build02.ci.openshift.org/ci-op-y2n8rsh3/pipeline@sha256:ddd=quay.io/openshift/ci:ci_c_latest && break; backoff=$(($RANDOM % 120))s; echo Sleeping randomized $backoff before retry; sleep $backoff; done
set +e
for r in {1..2}; do echo 'Tag attempt $r (all together)'; oc tag --source=docker --loglevel=2 --reference-policy='source' --import-mode='PreserveOriginal' --reference quay-proxy.ci.openshift.org/openshift/ci@sha256:ddd ci/${component}-quay:c quay-proxy.ci.openshift.org/openshift/ci@sha256:bbb ci/ci-quay:${component} && break; :; done
for r in {1..3}; do echo 'Tag attempt $r (individual)'; oc tag --source=docker --loglevel=2 --reference-policy='source' --import-mode='PreserveOriginal' --reference quay-proxy.ci.openshift.org/openshift/ci@sha256:ddd ci/${component}-quay:c && break; backoff=$(($RANDOM % 120))s; echo Sleeping randomized $backoff before retry; sleep $backoff; done
for r in {1..3}; do echo 'Tag attempt $r (individual)'; oc tag --source=docker --loglevel=2 --reference-policy='source' --import-mode='PreserveOriginal' --reference quay-proxy.ci.openshift.org/openshift/ci@sha256:bbb ci/ci-quay:${component} && break; backoff=$(($RANDOM % 120))s; echo Sleeping randomized $backoff before retry; sleep $backoff; done
tagged=false
for r in {1..3}; do
echo 'Tag attempt $r (individual)'
if oc tag --source=docker --loglevel=2 --reference-policy='source' --import-mode='PreserveOriginal' --reference quay-proxy.ci.openshift.org/openshift/ci@sha256:ddd ci/${component}-quay:c; then
tagged=true
break
fi
backoff=$(($RANDOM % 120))s; echo Sleeping randomized $backoff before retry; sleep $backoff
done
if [ "$tagged" != "true" ]; then
echo "failed to tag ci/${component}-quay:c after retries" >&2
exit 1
fi
tagged=false
for r in {1..3}; do
echo 'Tag attempt $r (individual)'
if oc tag --source=docker --loglevel=2 --reference-policy='source' --import-mode='PreserveOriginal' --reference quay-proxy.ci.openshift.org/openshift/ci@sha256:bbb ci/ci-quay:${component}; then
tagged=true
break
fi
backoff=$(($RANDOM % 120))s; echo Sleeping randomized $backoff before retry; sleep $backoff
done
if [ "$tagged" != "true" ]; then
echo "failed to tag ci/ci-quay:${component} after retries" >&2
exit 1
fi
set -e
command:
- /bin/sh
Expand Down
28 changes: 26 additions & 2 deletions pkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_4.12.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,32 @@ spec:
for r in {1..5}; do echo Mirror attempt $r; oc image mirror --loglevel=2 --keep-manifest-list --registry-config=/etc/push-secret/.dockerconfigjson --max-per-registry=10 registry.build02.ci.openshift.org/ci-op-y2n8rsh3/pipeline@sha256:aaa=quay.io/openshift/ci:ocp_4.12_ovn-kubernetes registry.build02.ci.openshift.org/ci-op-y2n8rsh3/pipeline@sha256:bbb=quay.io/openshift/ci:ocp_4.12_ovn-kubernetes-base && break; backoff=$(($RANDOM % 120))s; echo Sleeping randomized $backoff before retry; sleep $backoff; done
set +e
for r in {1..2}; do echo 'Tag attempt $r (all together)'; oc tag --source=docker --loglevel=2 --reference-policy='source' --import-mode='PreserveOriginal' --reference quay-proxy.ci.openshift.org/openshift/ci@sha256:aaa ocp/4.12:ovn-kubernetes quay-proxy.ci.openshift.org/openshift/ci@sha256:bbb ocp/4.12:ovn-kubernetes-base && break; :; done
for r in {1..3}; do echo 'Tag attempt $r (individual)'; oc tag --source=docker --loglevel=2 --reference-policy='source' --import-mode='PreserveOriginal' --reference quay-proxy.ci.openshift.org/openshift/ci@sha256:aaa ocp/4.12:ovn-kubernetes && break; backoff=$(($RANDOM % 120))s; echo Sleeping randomized $backoff before retry; sleep $backoff; done
for r in {1..3}; do echo 'Tag attempt $r (individual)'; oc tag --source=docker --loglevel=2 --reference-policy='source' --import-mode='PreserveOriginal' --reference quay-proxy.ci.openshift.org/openshift/ci@sha256:bbb ocp/4.12:ovn-kubernetes-base && break; backoff=$(($RANDOM % 120))s; echo Sleeping randomized $backoff before retry; sleep $backoff; done
tagged=false
for r in {1..3}; do
echo 'Tag attempt $r (individual)'
if oc tag --source=docker --loglevel=2 --reference-policy='source' --import-mode='PreserveOriginal' --reference quay-proxy.ci.openshift.org/openshift/ci@sha256:aaa ocp/4.12:ovn-kubernetes; then
tagged=true
break
fi
backoff=$(($RANDOM % 120))s; echo Sleeping randomized $backoff before retry; sleep $backoff
done
if [ "$tagged" != "true" ]; then
echo "failed to tag ocp/4.12:ovn-kubernetes after retries" >&2
exit 1
fi
tagged=false
for r in {1..3}; do
echo 'Tag attempt $r (individual)'
if oc tag --source=docker --loglevel=2 --reference-policy='source' --import-mode='PreserveOriginal' --reference quay-proxy.ci.openshift.org/openshift/ci@sha256:bbb ocp/4.12:ovn-kubernetes-base; then
tagged=true
break
fi
backoff=$(($RANDOM % 120))s; echo Sleeping randomized $backoff before retry; sleep $backoff
done
if [ "$tagged" != "true" ]; then
echo "failed to tag ocp/4.12:ovn-kubernetes-base after retries" >&2
exit 1
fi
set -e
command:
- /bin/sh
Expand Down
Loading