Skip to content

OCPCRT-598: Fix CRD/annotation phase desync for release signing#810

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

Closed
hoxhaeris wants to merge 1 commit into from
+40 −0
Closed

OCPCRT-598: Fix CRD/annotation phase desync for release signing #810

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
1 change: 1 addition & 0 deletions cmd/release-controller/audit.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -41,6 +41,7 @@ func (c *Controller) syncAudit(key queueKey) error {
if err != nil || release == nil {
return err
}
c.populatePayloadPhases(release)

klog.V(4).Infof("Audit %s", release.Config.Name)
c.auditTracker.Sync(release)
Expand Down
39 changes: 39 additions & 0 deletions cmd/release-controller/sync.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -390,6 +390,13 @@ func (c *Controller) syncPending(release *releasecontroller.Release, pendingTags
return err
}
c.precacheChangelog(release, tag)
case releasecontroller.ReleasePhaseAccepted:
if err := c.markReleaseReady(release, nil, tag.Name); err != nil {
return err
}
if err := c.markReleaseAccepted(release, nil, tag.Name); err != nil {
return err
}
Comment on lines +393 to +399

@coderabbitaicoderabbitaiBotAug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win

Preserve changelog pre-caching on every new fast path.

Each new Pending/Accepted → Ready → Accepted path bypasses the c.precacheChangelog call used by the existing Ready path.

  • cmd/release-controller/sync.go#L393-L399: call c.precacheChangelog(release, tag) after markReleaseReady.
  • cmd/release-controller/sync.go#L455-L461: add the same call before markReleaseAccepted.
  • cmd/release-controller/sync.go#L544-L550: add the same call between the two phase transitions.
📍 Affects 1 file
  • cmd/release-controller/sync.go#L393-L399 (this comment)
  • cmd/release-controller/sync.go#L455-L461
  • cmd/release-controller/sync.go#L544-L550
🤖 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 `@cmd/release-controller/sync.go` around lines 393 - 399, Preserve changelog
pre-caching in all new fast paths: in cmd/release-controller/sync.go lines
393-399, call c.precacheChangelog(release, tag) after markReleaseReady; in lines
455-461, add the same call before markReleaseAccepted; and in lines 544-550, add
it between the two phase transitions. Keep the existing error handling and
transition ordering intact.

case releasecontroller.ReleasePhaseFailed:
if err := c.transitionReleasePhaseFailure(release, []string{releasecontroller.ReleasePhasePending}, releasecontroller.ReleasePhaseFailed, reasonAndMessage("CreateReleaseFailed", "Could not create the release image"), tag.Name); err != nil {
return err
Expand DownExpand Up@@ -445,6 +452,13 @@ func (c *Controller) syncPending(release *releasecontroller.Release, pendingTags
return err
}
c.precacheChangelog(release, tag)
case releasecontroller.ReleasePhaseAccepted:
if err := c.markReleaseReady(release, nil, tag.Name); err != nil {
return err
}
if err := c.markReleaseAccepted(release, nil, tag.Name); err != nil {
return err
}
case releasecontroller.ReleasePhaseFailed:
if err := c.transitionReleasePhaseFailure(release, []string{releasecontroller.ReleasePhasePending}, releasecontroller.ReleasePhaseFailed, reasonAndMessage("CreateReleaseFailed", "Could not create the release image"), tag.Name); err != nil {
return err
Expand DownExpand Up@@ -520,6 +534,31 @@ func (c *Controller) syncAccepted(release *releasecontroller.Release) error {
klog.Infof("release=%s accepted=%v", release.Config.Name, releasecontroller.TagNames(acceptedTags))
}

for _, tag := range acceptedTags {
annotationPhase := tag.Annotations[releasecontroller.ReleaseAnnotationPhase]
if annotationPhase == releasecontroller.ReleasePhaseAccepted {
continue
}
klog.V(2).Infof("Reconciling phase for %s: annotation=%q, payload=Accepted", tag.Name, annotationPhase)
switch annotationPhase {
case releasecontroller.ReleasePhasePending, "":
if err := c.markReleaseReady(release, nil, tag.Name); err != nil {
return err
}
if err := c.markReleaseAccepted(release, nil, tag.Name); err != nil {
return err
}
case releasecontroller.ReleasePhaseReady:
if err := c.markReleaseAccepted(release, nil, tag.Name); err != nil {
return err
}
default:
klog.Warningf("Tag %s has payload Accepted but annotation phase %q; skipping reconciliation", tag.Name, annotationPhase)
continue
}
return nil
Comment on lines +555 to +559

@coderabbitaicoderabbitaiBotAug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Block publishing when annotation reconciliation is skipped.

When acceptedTags[0] has annotation phase Rejected, Failed, or another unexpected value, this branch logs and continues. After the loop, Line 562 still enters the publish block and uses acceptedTags[0]. The controller can publish a payload whose annotation is not Accepted. Track a blocked reconciliation result and return before publishing, or gate publishing on the newest tag having annotation phase Accepted.

🤖 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 `@cmd/release-controller/sync.go` around lines 555 - 559, Update the
reconciliation flow around the default branch and subsequent publish block so
skipped annotation phases prevent publishing. Track that reconciliation was
blocked or gate the publish path on the newest acceptedTags entry having
annotation phase Accepted, and return before using acceptedTags[0] when its
phase is Rejected, Failed, or unexpected; preserve the existing warning and
successful Accepted flow.

}

if len(release.Config.Publish) == 0 || len(acceptedTags) == 0 {
return nil
}
Expand Down