Skip to content
This repository was archived by the owner on Sep 9, 2026. It is now read-only.

Revert "NO-ISSUE: discover fulfillment certificates and deployments by label in refresh script" - #237

Merged
omer-vishlitzky merged 1 commit into
mainfrom
revert-236-installer
Jun 7, 2026
Merged

omer-vishlitzky merged 1 commit into
mainfrom
revert-236-installer

Conversation

@omer-vishlitzky

@omer-vishlitzky omer-vishlitzky commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Reverts #236

Summary by CodeRabbit

  • Improvements
    • Enhanced error recovery during service initialization, allowing the system to continue and retry on temporary failures
    • More deterministic pod restart and TLS certificate handling during snapshot refresh operations for improved reliability

@openshift-ci-robot

Copy link
Copy Markdown

@omer-vishlitzky: This pull request explicitly references no jira issue.

Details

In response to this:

Reverts #236

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci
openshift-ci Bot requested review from adriengentil and larsks June 7, 2026 20:32
@openshift-ci

openshift-ci Bot commented Jun 7, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: omer-vishlitzky

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved label Jun 7, 2026
@coderabbitai

coderabbitai Bot commented Jun 7, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 37763cdc-2e25-4dd6-9f1f-cf86e8f9947e

📥 Commits

Reviewing files that changed from the base of the PR and between 5c5ef33 and 08289d6.

📒 Files selected for processing (2)
  • scripts/prepare-fulfillment-service.sh
  • scripts/refresh-after-snapshot.sh

Walkthrough

This PR modifies fulfillment service initialization in two deployment scripts: changing AAP job-launch error handling from automatic curl failure to retry-loop dependency, and replacing dynamic label-based discovery with explicit hardcoded fulfillment deployment lists across TLS waiting and restart phases.

Changes

Fulfillment Service Reliability and Determinism

Layer / File(s) Summary
AAP Job Launch Error Handling
scripts/prepare-fulfillment-service.sh
The curl command for launching the osac-publish-templates AAP job changes from -kfsS to -ksS, removing the -f flag so HTTP errors no longer automatically terminate the request; subsequent jq parsing and existing retry logic now govern failure handling.
Hardcoded Fulfillment Component Discovery
scripts/refresh-after-snapshot.sh
TLS certificate waiting, deployment restart, and rollout-status monitoring phases replace label-based discovery (app=fulfillment-service) with explicit hardcoded deployment lists, applied uniformly in the TLS phase, initial restart, the wait_fulfillment_rollouts() helper, and the final rollout phase, with parallel background PID tracking for improved error detection.

Sequence Diagram

sequenceDiagram
    participant PrepScript as prepare-fulfillment-service.sh
    participant AAP as AAP Job Launcher
    participant curl as curl
    participant jq as jq parser
    participant RefreshScript as refresh-after-snapshot.sh
    participant OC as oc kubectl

    PrepScript->>curl: curl -ksS (no -f flag)
    curl->>AAP: POST /launch job
    AAP-->>curl: HTTP response (2xx or error)
    curl-->>jq: response body
    jq->>jq: parse job ID or error
    jq-->>PrepScript: result / proceed to retry
    
    RefreshScript->>OC: wait for hardcoded certs in parallel
    OC-->>RefreshScript: cert readiness
    RefreshScript->>OC: restart hardcoded deployments
    OC-->>RefreshScript: restart initiated
    RefreshScript->>OC: rollout status (PID tracked)
    OC-->>RefreshScript: rollout success/failure
Loading

Risk Assessment & Security Implications

🔒 Error Handling Semantic Shift (Medium Risk)
Removing curl's -f flag in prepare-fulfillment-service.sh changes the failure mode: HTTP error responses (4xx, 5xx) will no longer automatically terminate the curl invocation. Instead, the script proceeds to parse the response and relies on downstream retry logic. Risk: If the AAP endpoint returns an error with an invalid response body (e.g., HTML error pages instead of JSON), jq parsing may fail silently or with unclear error messages, potentially masking job-launch failures. Mitigation: The existing retry loop should catch persistent failures, but response validation is critical to prevent transient misconfigurations from being silently skipped.

✓ Determinism Hardening (Low to Medium Risk Reduction)
Replacing label-based discovery with hardcoded fulfillment deployment lists in refresh-after-snapshot.sh improves predictability and reduces attack surface from unexpected label matches. The explicit list cannot be silently affected by rogue deployments labeled app=fulfillment-service. Benefit: Eliminates discovery-based ambiguity during critical post-snapshot TLS and rollout operations. Tradeoff: New deployments or renamed components must be manually added to the hardcoded list; missed updates could leave services in an incomplete state.


Estimated Code Review Effort

🎯 2 (Simple) | ⏱️ ~10 minutes


Possibly Related PRs


Suggested Labels

lgtm, approved


Suggested Reviewers

  • tzvatot
  • danmanor

Poem

🔧 Where labels once guided our way,
Now hardcoded paths keep deployment at bay.
Curl drops its -f, trusts retry's embrace—
Fulfillment rolls forward with determinism's grace. 🚀

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch revert-236-installer

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@omer-vishlitzky
omer-vishlitzky merged commit 86a4b0d into main Jun 7, 2026
6 of 9 checks passed
@eliorerz
eliorerz deleted the revert-236-installer branch July 8, 2026 16:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants