OCPBUGS-35210: prevent KCM from deleting SA-token secrets created before their SA - #1347
Draft
rsacherer wants to merge 1 commit into
Draft
OCPBUGS-35210: prevent KCM from deleting SA-token secrets created before their SA#1347rsacherer wants to merge 1 commit into
rsacherer wants to merge 1 commit into
Conversation
…ore their SA
SA-token Secrets included in an operator bundle are placed earlier in the
InstallPlan step list than the synthesized ServiceAccount step. OLM creates
the Secret before the SA exists; the Kubernetes token controller (KCM)
immediately deletes any token secret whose referenced ServiceAccount is
absent, and OLM then marks the step as Created permanently — preventing
any future retry and leaving the operator without its token secret.
The issue is not always reproduceable (see test document) because a
rescheduled loop run is able to get the stale RV, which still does not
see the Secret as created, but the prior loop created the SA so this time
the Secret will not be deleted.
Fix: add a StepperFunc for BundleSecretKind (mirroring the existing CRD
StepperFunc pattern). The new NewBundleSecretStep checks whether the SA
referenced by the secret already exists before attempting creation:
- SA absent → return WaitingForAPI; NeedsRequeue() returns true, keeping
phase=Installing and triggering a 5-second requeue.
- SA present → create the secret with correct owner refs (live API UID
lookup, matching getUpdatedOwnerReferences behaviour) and
return Created/Present.
Because the StepperFunc handles WaitingForAPI internally it never reaches
the main ExecutePlan switch case that would otherwise skip the step, so no
changes to the switch statement or to NeedsRequeue() are required.
Also adds structured debug logging to syncInstallPlans (plan resourceVersion,
per-step BS/SA status at reconcile start, UpdateStatus call/result) to make
the race observable in OLM pod logs during investigation.
Most likely the additional logging will be removed via further commits.
Tested: 30-iteration statistical reproducer against OCP 4.17 — 0/30 bug
fires with the fix (without the fix we are looking at roughly 60%
failure/40% Success).
This PR is still a draft and work in progress.
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
jira/severity-moderate
|
@rsacherer: This pull request references Jira Issue OCPBUGS-35210, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThe catalog controller now resolves ChangesBundleSecret execution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant InstallPlanReconciliation
participant ExecutePlan
participant NewBundleSecretStep
participant OLMClient
InstallPlanReconciliation->>ExecutePlan: execute BundleSecret step
ExecutePlan->>NewBundleSecretStep: resolve manifest
NewBundleSecretStep->>OLMClient: check service account and CSV
OLMClient-->>NewBundleSecretStep: return referenced resources
NewBundleSecretStep->>OLMClient: create or update Secret
NewBundleSecretStep-->>ExecutePlan: return step status
``
<!-- walkthrough_end -->
<!-- pre_merge_checks_walkthrough_start -->
<details>
<summary>🚥 Pre-merge checks | ✅ 15</summary>
<details>
<summary>✅ Passed checks (15 passed)</summary>
| Check name | Status | Explanation |
| :----------------------------------------------: | :------- | :----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly identifies the bug and the main change: preventing deletion of ServiceAccount-token Secrets created before their ServiceAccount. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Stable And Deterministic Test Names | ✅ Passed | The PR diff changes eight implementation files and zero *_test.go files; added-line scanning found no Ginkgo test declarations or dynamic test titles. |
| Test Structure And Quality | ✅ Passed | The PR changes only production and vendor Go files. The diff adds no Ginkgo tests, setup, waits, or assertions, so this test-specific check is inapplicable. |
| Microshift Test Compatibility | ✅ Passed | The commit changes only OLM source and vendored source files; it adds no Ginkgo e2e tests or It/Describe/Context/When constructs to assess for MicroShift compatibility. |
| Single Node Openshift (Sno) Test Compatibility | ✅ Passed | The diff changes only OLM implementation and vendored Go files; it adds no Ginkgo e2e tests or It/Describe/Context/When constructs, so SNO test compatibility is not applicable. |
| Topology-Aware Scheduling Compatibility | ✅ Passed | The diff adds BundleSecret reconciliation, ServiceAccount checks, owner references, and logging only; it adds no replicas, affinity, topology spread, node selectors, tolerations, or PDB constraints. |
| Ote Binary Stdout Contract | ✅ Passed | The pull request changes OLM controller and catalog logging, not OTE process-level code; no changed stdout write in main or suite setup is evident. |
| Ipv6 And Disconnected Network Test Compatibility | ✅ Passed | The patch changes only controller and command Go files plus vendor copies; no new Ginkgo test or IPv4/external connectivity assumption was added. |
| No-Weak-Crypto | ✅ Passed | The diff adds Logrus formatting, diagnostics, and BundleSecret handling; searches of all changed and vendor-mirrored files found no weak-crypto APIs, custom crypto, ECB, or secret/token comparisons. |
| Container-Privileges | ✅ Passed | The parent diff changes only Go source and vendor copies. It adds no manifests or privilege settings such as privileged, hostPID, hostNetwork, hostIPC, SYS_ADMIN, or allowPrivilegeEscalation. |
| No-Sensitive-Data-In-Logs | ✅ Passed | Added logs contain only resource names, ServiceAccount name, statuses, phases, resource versions, and errors; code never logs Secret.Data["token"] or other credential values. |
</details>
</details>
<!-- pre_merge_checks_walkthrough_end -->
<!-- finishing_touch_checkbox_start -->
<details>
<summary>✨ Finishing Touches</summary>
<details>
<summary>🧪 Generate unit tests (beta)</summary>
- [ ] <!-- {"checkboxId": "f47ac10b-58cc-4372-a567-0e02b2c3d479", "radioGroupId": "utg-output-choice-group-unknown_comment_id"} --> Create PR with unit tests
</details>
</details>
<!-- finishing_touch_checkbox_end -->
<!-- This is an auto-generated comment: all tool run failures by coderabbit.ai -->
> [!WARNING]
> There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.
>
> <details>
> <summary>🔧 golangci-lint (2.12.2)</summary>
>
> Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions
> The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions
>
>
>
>
> </details>
<!-- end of auto-generated comment: all tool run failures by coderabbit.ai -->
<!-- tips_start -->
---
Thanks for using [CodeRabbit](https://coderabbit.ai?utm_source=oss&utm_medium=github&utm_campaign=openshift/operator-framework-olm&utm_content=1347)! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
<details>
<summary>❤️ Share</summary>
- [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai)
- [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai)
- [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai)
- [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)
</details>
<sub>Comment `@coderabbitai help` to get the list of available commands.</sub>
<!-- tips_end -->
> [!WARNING]
> ⚠️ This pull request shows signs of AI-generated slop (redundant_comments). It has been flagged by CodeRabbit slop detection and should be reviewed carefully.
<!-- internal state start -->
<!-- N4IgzgxgFgpgtgQwGowE5gJYHsB2IBcAjADTgAuqArhGZajACYDKZCZMBoYF1t9K6bHiKkADqgDyAIwBWMGhgBuMMARABidQAIACgCUtYSnESoAngB0cVgCpQMYLQ1QIAZmV0GEDZyrAqtBC0XCBgtAHdYei0JABkAWS0IejYApjRFDFCAQQgILEocMgBaMiwAaxgcLXTkmDJHKRhXLGiyWAxUYOa0KtCGGoysmFz8woaAOi07MIBpSibUHHqAssrq/KLULAAbHbQnGH32R3asf0G6huII+32YhK1RNERlop2zLVNy09hDdlEjgQjjqqQYEysVgAkh5vAwgVoAAYAIUKDH2tXoZFmGBwDERNQBz1QADFChAtO02BEEBgGoE8d0AI6UGCsxyRKqUv70Vy9HD9QaoTI5PIFIpaByBKT+Io3dpc0EnblhTH1CJ0qCBUTiLDiDCpLRYcLLLq8/mhRy4UIqoUikZi8ZaGAADwcky0MMCOzAWECPkc3CoNDojEOUkoAHMtDssJHI7jo2UkWAzAKoThuAg9jodghMwSWl1ccpuBhI2xE7aQjAIdYcDNPN1RK16QAGLSuWk7UOOXGBLQAZjbxTpaEruGb2wY1AOk4kAGEdFoACwTQgAdg17VtrgwLpu+TgogQ9AG4U12t1bsQ7A+WgAbG2AKSd7u97dQAoeOl1qyaLQFywEwqnpIwTFPSx60bCRnhwJh7HcQCoWCLApCwDx6BbVB2AGKkPAAKU6IIHCMMJEUXHRkQAVQAcSYYpBwAVgAJkINsCSlEtswwAYmggBBKAuOkYwQCAfltV1nhoMNESYiY2wUglWFQSN1WUQRJ3zAYuOqRF4gkAARKESShABRQyCRcBUuipapcDCLBXFtMTQlEXD/lSMB8CRAA5MyAHVERuRFsiYJgoVo/zLJuVokR0CQmBsREpi9XEg14MMFUCSgzlsv0qD0gB6GRiO6Vx6DAKACTcdgukoUQGErHAkz+IiXElMAyKNLpoHzNTbX0Sk6XuZNzXoAUwnzDCok6siJhAUgXHCJhjFMMx4gQUROHIBAExaoq9XHMpUGKHYMD5CAzAgfZilePa0CKiA4AYJ62GzOMiqzHCJkjLA1AA7IoWKNTTTBQw1sgo1nL6lqVEhHAgJwPdI1DAY4AwPYHHkXAGGKcR5AcIRhrgFRWGPTtWlvMgqyLLR0J3bKj1eBhAEwCRxYwEnYtFiOMqA5uM1K6bTbUjWMpGzHm+aEmNBbQG4Lx3VxKD2EmyYQY9HCqBApH2cEEcBnY6onTNKT9bKGHkPNrKEK1nKk1swzi0QFnOilQLpDB4frXyHIR2JcQCWG1IYHyAGp2KK4o2wRsyy1vMN8kt7pMhgcJnVcIsyB83nwkW3b9sjQ7iTYVozou+Rrtu+6haK0RyiLzYKF2fZUGL47WjAN7WFjIujus1pfv+/ANG0IGQaqY6w3A9boaSKB+u9qxsh8MNKJo+jGNY9inANSMcHOGmKV7wvuW2KMv1yz1Mx7nZc3zbpNggTGDRprTGVdeRcqEKZGyb7Y9gHAPunXujh6D7EUPmDw3AYCiE8rQfwQJGSVQKKgG0GlMC4EaM0VoYR6BPxfibG4TQixTUZLVA4iJqKNVSCwNgQlgq3C5GAcoGAdRVjikqKsqI8QYnkFiIq6RhTDFGOKaBAIwA3BFpyaouIGBKF4pQSWMDARvkxj1JIwFRDHFrEiMyLov7sHvjgGqPo/Qni6qsP4/dS5dGyDoFCN0vYSlxMmREyxwiokxpbVAKUDZjyNsdW2ZtbSWxuqeE2dtnQumwh5Z2rssjOiKJ7JeOBfbLH9oHEEC84ahy0GHAAHAUqOK5Y7x3BknPBMBU7p2aFnHy8RGAYGMPnLMhd24D1OudS6VcYB3XzA9Nu9dG64GboAtuNiTpdwEj3T6Kih4AzHsDUGU8Bgzyhk5eei8wAIxXpbAYKCdgaQhjqVslMugojRHwq4BIVFIJ0lsLAM5BSIgjN4tAExdhwAXOdUCKU/Jpx4eiGAaoyAsFgQSYBTxTyIIZAMJUAQ4oNSasqUFkiaR0kcHTDGXUqz+GETaVyYiwE9AmoKKQnx8X2mKES8YpQKhcjRVInUvzHBxESHmJoPopGMhrgEBcTAkBGhNAccafQAhCSrOdZQWhqJQkMrLCoDUphmVQNsdA3Q+B6WojgcowCTEAG4IZ5D8MrbmXDJxYjoKbREC4Ui4UYSLV07oqxosCNEZF4MRZWqWPFSqfy/w4ENsbN+ptkwWythE0NUSHY4Sdl0F2utEkexpqk4oWhGlnAGHCRgPlES+UBVckF/D6jgtEAAChUVoAAVIoQg2ZRAL0IBMMtNxXgV24P8VAiYACUhJYHEjJAKAkk5XmUHeb4yUelhlPVGQA1uHTbHTPer3L6AIh6IkycsbJi88lhwAJzFOKIQMpNME7wqeVUmpGd6laAABLligK01g7TJll26ZXG6fS+VDIbrOx54yir0ATEGMwQGVC7A0mu2BYAFkj0Bssye1lp6Q3MHPYOqS9mJ2AqTIojgGBYAgMYD2LVJJujLKRqtrQfFVn7PmtOZa9AQboKEEk2w4BAv2IiHyEZeGOVkPIekp4wjwDpB5EhuDDBpgVJgAAXtPIYoQaWOiKEVPQyJsgLm6L6FjKgeV4XsFaVANHSNcUtskvcYZgS2kKHuAxfEYAL0yHFYEvpn7g0VlqdedEGLMTYopfxWhsiBJtlgkJEbwmhdNps2NcSE0JPdsk1NOyfZ+3rAHbdWzcnhxYlHGO9Y45noqZelOXtamZ1bDnY0IAAC+pBLasExowICltkSc3KKtCC5g9DbIIAAbVAC4OGUIGBqCG2pAA+vWpiUgCnyCYoQFci384nnaGoNpiZF0nXLj0r9/ScCDKei9buH0i7fTIAs0g52MscHwKxUgVQGA3YIA+Eg4BUNmDUFh7NhgF5nilpGfmlJXQeCLNTA4krKP7mKPIhMoOQihuzMUfwmw8IYFJlmCmYO2A01IyLLa2ivY6Q8MmBmWpsqc0liLMWaFJYnzQLB/OlS2uEfKEBY8+w3RkE+yPWMec6uDcXiNsbi8JtMQ3A+CAG42wbg3K4QcBSWIrbYE+keG2Dpvq6RXK6e2f11z/f/Fuj1NfLtmX3EuJ1Lu7Rws9/A/nBwPbxLbtiLF91XY+2oDMWYcx5mqPg60hDQ1aChaArQnHi1XDhXaERKnxGwPgWECT0RdSWkwHj5BONjOnBeLiJHjbgQidPPeZ1kwmeXpZ+Jdn2iS885AHz2rxBBfDdGyPcbMApsriYgUtsm4VwIA3BAJiyu1tq5fZtzXO3P3VwGbXGdhvAMm5O6uzXVvruBwIGxfdLFHdPfX3btib31nmDUHQhBTwGfunFcH40stIy/CqTptBYQMFCBuPn/w+nDBHEE9PAE8ChIBBJ7kTUIoogqsAIKIhGrZQ+rLADCepxICY0CdRJBbR8Bhh0xGAygwAsigTGqp636FwLSkDM7tZV6c50i16kzyItIC4gBt7C6t6i6uAFJSArhSAbgsRSBSCEBMT7rD6q4Fzj4W7vra69L7aHZz5zpG4TLCHoBL6fQr5/TPqnhkDO4sQsRtg75qEsQPju5da16rQmpdRmrXze53y+7/6ODwEBAh5xi2geLaaoLoIX7vwDDv61hl6takFaLkHc5qD160H0Et50Gi4sQbiK4wCED7pthtjqEID8HrZj4a6yGT467T4Haz4G5SEL6yGm6nZbaDxKFXYqSqF74sSsTb4gCPbO4VF6HrRqCIj6KGIwDGKQo35GAspez35GiyFBYOJJC/IShuIeJeI7A+IpSeEwAV5s4+E15qBUHNJwAN5N5qQMEhFwwTb7oS5NR94QCuAMAPhK5iAq6JF7RCEdxa67bpESFZEAYLqL4zL5GKH/TFEqE1EPhu5VFO5lFd4bh1GQRqBNFEZB6h4sJsLPADA6D+qqZ2owDgzh6gpR5CL2iiJOh3JR64g3SUCWzdFVpyKuhMLVAnxVgoK6bRKGJCBGoBS0i46RgkitD2IoQ5LogknwC0imyA4FAQmfiGCsI6i/70KIJEEgAkGs5kFzG87VaBFC7BFt4TYFIMDS7lEIArgwBsTxHHEj6CHJEXGpFiF66SF3HG65HyHm4XGr4lHO4PiK5aFlEfGVFH614IkloeAWrVC2F362go5Yg3BUrDCBCx4MgHzgGv5PAWGklP5aAv64C+l/74kHhR5EZqq4FVpZgILCmimV6zFc614BGN7rGrGymi4LZtgIBPiWxMSuCEBHEgCrYCHq5mmdJ6m64z6PSGljL3EmmPHL6yEWlvFlGHpMS2nLAEDDhtgO7vb6ENFAm5QtG+5tEgJxjdFdgvyUZ/5AGarWpVgPyfzAnExoDqpR6h5wnQD/CwLszaYqy0mWGBDuAHBGBGFgAmEp5+CJgZnl7eEc4Sl15Sn5lBEi4bGWxwnDhNDREwB8Gan1lJGNm2LNnXGZEjJGkyEXF5GrrzJFHW6lEjl3bDm3afGOlfarwDAERMASC+TQqCDp4HIQZHLG4ir1Q0znTc6HBwTmbPwBD0AsidDoFxTOmR7MnnQtTvleFinZkUH+G/krEwBrFylcF95MTWkIAwAFLhEJGj5nE6lNkfppHfqtm/qIUdnGkoWmnQaiB9k25lEri4Ub74Ue4jzfa2jRmwEPCJBOK4EizGimjFBiqTRaDfxMWfDo6xKM7EEfmiVfk5kSX85/kykAWTYKkFIIBtjgWuAbgrhtgFJqXakwXbbaX6l6X64GXzpGWdKoVzLroYVr7YVMSaFfG77VW1UEX2VEVIhfI/LOIeBOWMATAZh1RdihDKR+hjoToTGhUiVZkRXiUjwLE0ExXN5xXt6JX7quBtgMAsQ6z7qbhZUNkFGXFT66UZFtm3GGXIWlUmXoUvGYW268HWX4APhWWTn1EjwgHgzuKFojUhKuSwKwiMjcCSbZTsoDEdXCVTGfnV6RXTVNKzVSUyWi5tj7pb4wArjrWbWEBSDbXQW7VwUHU3FFXSG7VlVnYVWXVVW3bsS1XVF77sSH52UgAOWXJ8ago4h4gEjyJgCrannJgIoDh8VYhnn8moAg3THikQ0gAzVLHSnzWMGAUZVtjI08GDjhEPgY0aU5UiFXE40IX/onUE3nXE3KEWXYWDiu63VLZ/GPUAnNX7IAqeJFqgplqMKRBZBagnjoBpAulfD5gdrXAYr0h0w+WCh+mEqx7ooE6srB4awqAniEqMjspnQ6xHBfD1DeDvS+n1D3KOG0WOACpCqeWiqkrirooIqOBMp+U0LKjOoUbRil3er1DWpAg6jbD6iGipngEAEZ0HmdxC1g2+G172CRhPqS2FkLWbGVnKWEDDjlFS4q2vopF5UtmHX6Xa3FVtzAbujmDga+i0Vtx3LmVYVk0La3U8EOm02GSEbEZFBZQLweC8bApGiIH0gEz9BhCphFCwByYKYEp9K0oSgizqaaaOG6ah28qniVAGZSjUZoDbndG2b7hrxLgby+bbwdhNBObYCC2THC1iV+GSn84AC6S01SZWzWOGdII2qg+AfWIACA/ec2D4mcrufI0uBSm4bYUg/mbYvBipbYD4G4g4D4/ergneD4TEEAIABDtZ9AjSqkMAC4sA4kFDoADg+g0xJDIERQOgh8jABAXYPoMApADgEgGk3aq8wgujH+IABGEAQYiYQEGkD0vVaAB2Owp+t2wAdWIpl6PWXBdIAU2wRiqANgI0t25j+j4Al8Yx0x+gOj2YFj8iDAeghQZ9EALA3aLUYAcj8g5QBAPAYT8TiTDYwTmT4kOTVAeTvEBThkKgyQbCoaxT2T+AoTpAglYDUIXU7IqTag+ceY3A9TTGRgRsFDA2Ip8j5QvkkdagVTkA3a7kxM9TyhCCpTrID2MSvuJsag9TvJ4JYY6aLWMA3jUgdIF5/dUAZ01SidjpyB2sus3V+ccAl6ag4Qp4OAiY+crQ5YueOw9T4zpMagOJNTszuAyxIzWTPzt2IAQTZA9w0AWTCzQkSzYTUkazoaagjYNMULYQX6RegV5mNMlm3REY0YIs2UiA/YGGPkBMygySpGlsxwxMmyyJMeYwRQ9K6wlwWIII9qYYG5ConQ0eoozLF2+chy38QL+AOAKsOwpA9zlsjzzzrzhDmwKMoYMTejpA7z+02Y3zEzI86L+wwLML4kYLkzhGNjpGdj44akcLFDuTKz2iAyKLI8vsnY5I0alMaIU6toGGAwe4+wpwfo1S2YSi7ATgprFAVY+Q9jakUwTAfJogVYVjZr0YkblrmLozwpIrjrIAfjY4ob1j4b6T5yu4rrwSOKaerU8AISYAlYT5nwTMWAUbYQ7QlUX4YxwpMr4LTzSwCrIAGrnz2rvzI8ibBbd+BrozxrI8GWYDno7TQcoz1rCLdryLQgGzozWz/JDmAkAB1+MYuI07pE7IEQvQ7rjIdM7QUoLsqsnF7IQr0rDzI8XbLzLUbz3amrXz47Ordee7jAbTZEYA2QliXUuGZAY7oLn7EgV8DL+QzwgEOSak2SsLrxiz+AtrVRqzDrK7I8mzYJG79M8ggkFwB8u7uqYYB7AQ4Qx7LQHrZ7hmTwkrzIN77b97IAj7PbfbzjA74L34EgrgTA0HsjcHKgAHiCYAwHoHRrn7dCNzQWjIVTdUGMLzZYFINgZMWgYLqgSH8LKHZTiruAyr9AqrFjSLGHYrELfwQ08imcWW8Hzoj6P4X5wHJsnYTWGd8m2wNaE2JwF2f0znfrRqOaeMglL9AkOAT70YVHjIRHtEe7PnXnhwkWkSGiDAaYGsiScXerKgGbNForwgErewd7srI8ndqAL7HzHHH7g7IpQkZQEt+ZhrYzn7KnnaqTvAoYMnAwAAikogFQu9p8s3QTjMjOWCq407E4i+hwdlm42ENBhlaDgPeLqM8kHiLFSwRl0LRH6L65l9MH8JZ85HCI4FFzF+bGTOiv4LQKIArDSeii5pYjhLbL6ebLR158jtJBdIkvV8gbnp0QJDc1l1vTlwQHl1K2Lcx6x8++q6+/2xV+C0Rn9bV6APVxOyAPEFkNsFVBdB4E1x4FXpWIcz15pzazpwN0q8NwZ6N2q2h/a5N5h2Z5i6Q66YJ3N/eIDY/jHQMKt7gmsk4WEFt2AEaiJAdzu9F7qj52qU26dxojCEVFMzU00EVEjOwC6GQEVAFLABsFgjwDQP64EIB1inFKj8kFgAhJj5oseHjy/Nzv97sID+K5KwV52/KxD721D+V2B5V3DzV+Jw15V0wImPcL7MnLBFUBj0hOWkwAfH2tj4BFohbwT7tMh6h/7kN6jOT001T8u6Z42Ht9Z4ivN58IDUFfsI58t4yJz/9ht75yoAL7CAGML8d86CxBL9wEZp6Cr7L92vL4ryDqr+r5ojfMGD7b6DUL5BIMDp2keKthgPjxQcgcGVeG7DrPsNbz2I68Dw73K9287+x1qzD2oJ78BN78jzYHqLsHGGYMUNkF26qDCzOIJdGLjzTDP34YT4uyT3p2TyE2N0uyZ8IHT7vFZyF6811QyfZ+OdCIT8sHQgreeFkxu70VyoFoPTEeUFgcJ8+Nff0PCB3ZYRF+odTOLiAoLyhT+vcSlATG8A3AD4ycfwPsBoCdxCBrcSJLFC6A6BDIyIfvkGHZKl5CGAPNfvb1B6FcWOTvSMKVzfacd9+1XQ/rQSR7gcQ2yIXPGhhYAEYr4ivBHL1yT6Dd9OX/SnsZxp5Z8/gl7bmNe1U6zcXK/fE6omRXR2FiSLUcgRhBiA2AzITwbYKnjObKBzUl6I1ER29b/BFBHgcIN2hDb9hSW9kLoEYFzbncGoVzTILixX6291+fA8FsV2EHQ93esPcQQjxBYSdKuUIUQIoAfDtctAhkBwJsGWAyQBgBaMgOEFaDlBpgqnR/tP0t6fZX+fXMJsnw0GGdxu1PdZiPEbAc0tQxg3APeHnytxEyOGEWJXz55hkZY5fTRPG2r47sHCIvBuCd07RxQoQOgRQCuCKgg4nGksYoYJgUTMVXMxgQFtUCeZAgiKMQngfl3iGb8wuSQt3pkNSHw8j+n7X2MUAChwlygxQO1GYHcgk026RPfrq0M/7tCf+Ogv/tnwuj7c6+vMQHDLGxxkBaSNweRHtAPhKdgGAwYAR4AEqJgjU/gU8DCyiTZhzUgnbNGX0exlwMYaqLnlX314esiOFHBAF8OSC/DkwjJIumkKSDmA/hNwMyAuGRCMCv8VwIqGsEVCx9u0voTMJcMw5xCO2agRIZDzK678UhYg54ZIL35YdRk7JNAMUChIKJ9g8HVQcT2BGp9NBRnCbl0P/6u1cCOffofny0CV82eZCDnhSN6h6guiqUWvpgKI7to+QLfDRPqEyAGiX69QWkoGGoBahrMgYprCHBuBfhuADiQyHGMPjlDKhqAcoMmO4BrCFwNwJgAAE0mAE2bIIZHiBQhfIgo4kcaD1FBiYAakOOFzBNjSixWso5jgqJd5Kj32KokeAf3SFSDKubw9IJmE9jKBighkd6MUAzDFBYRGnBPlpzUGk9TRoIjPr/0IrW1Q8TcdkkaHtERkbQB2DHLmMUxQCxEEdUmLGUFJID3C6KHcc/hcKZhP8xXfnpomTjLBjkoeUFBMDHGsA+sFgEAKKOsDiMNEM0A4HUFxYGhuYkCHsJl2FbZcrhIPOUUV2pEldFRIgjUVVzVESMemZAfQB31qa08mA2QVloyhdJ9gBQPYa2v2Afia56YRad1GEHtaCg4SqAX5MWGqDZQvct8YxPzV3ado7IXpaTO/QwDyYBgjLAVsePmQmDi6toREjyz+D4Tok7oR8dlHmCLBlgyoP8aYJXpaBy0swBcPED7To4FiqQe8LSxWBII62DKaoN6XVCRBzgVSPkGSjDAiSjxToKUDrFlBkBP8gNBUNUG+C4k/81mWEuDGJCvBQI94QACgEjg85tSyJY4BPgysNApqjQwix9gCATIKRmyhUTPM34SUPSHUlWSLsFgVAJCCKkNg/gZHOfrYOzBPMzAJKRbrOCX5hBK0MAZvh4CsYX0yAfafiARymjaZb+KsMMLGD1DBBCgyBBqSEjUgk4/g3uMIHoCQAKx7Ap5MsKrAIw2FbB/gJtrJI9rWYEUDAYhFfGyj6g4og0uBDtKknZBDAT3KUGeg2mqgPaF4VWPPyaCHAdE+sEqYVKsAkh9wPkOEAODLSDpyQRbLEczQGDloqR6qKsNlErrXkFwegBVH9LQBDoKQq2OqDgB7S/w/gDhAtLbUZouky0MAhRkwhshnSEBDkviHWymlbSdgKQZLvJP9Ebkcc8AWZqRjdL4B3p1gEqemjkluTcCWgQAEmEm5X1NSU9gtR6SqARkkagLSMAwATGHAqyHLR9oYCpwHTloEqCwIqw7hAALzsTiR25RkOG3jBQM8cWgJiMjkG7UVZZtYNmZzPOkEx3JfMrkXCRDbZR8pPJfIMmSQJ50zQzQRwOWmlRhBGSsqeVIqnKANQ20bAaAFWAmkvVcIEgeikxnsmF08OaDVBH2lrpapAIXLV6FCRUCgQIQb0kqciHw7btso8M0kADIEoBAhZtJUWQHNcQ7C9ggVDwC+PzpiR36toIIVoBnJGILCYARWKeQEgXB8IEQAoGMSNA2QLwFwHDl6QBCPcd2xg8NFNL7lag0y8AXAnFGTCSz4QMs1kHLNTnRBr23FV6cVOXhmIMBgYLXmgQGCWxCWBBCGX6FfoQBtZPufMD7Op6AMn8AgTBDgDfw6iq0yIJgIInOlpkZY1IUAU1k8g4Qbg0csAoKRQJ7BN6V5PtMmEQCVBqwYkfjFSjGn9hAaLYAYKHhnBpNowJYMmOWEbFsy2Z8QQ+Lu0qD3hsocIT2LgDpwoDSM907mE9PoD3NlAHPA0C6xwizRmYmKPOcfIbBkxc0Q4EcGOCizwIXUXMKcE8lnDCwKw6UDwJRFXDrgtwEUtsEVGHDUTwu3FDkJeGyh2YtJWU/aX8GMUUdaJwc7cphAKAD17wT4V8CuR7D0Aio6VV8IYVTxozyFJUuwFKCGhSglp3MIIM4FqhR40x1Q/sLqEBx+BhSmE/QJC31YjxvMm8PzOxApb0AqWHgHSYkAqjARnp9QV1ARLynESHZ4MGSTAD5b4SG81DXKFgB0DdTXGO0eJd1IYDZAyATGGpGowxhkAgI4wAgG2A8aZKiYYrUAH4LHBrEe8y1VwA+AfCDho4hxFiMUGlzeBigUgCAMxGKAsRhwTEFUk+EIACNXAtWGrDViAA= -->
<!-- internal state end -->
|
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rsacherer The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
needs-ok-to-test
Contributor
|
Hi @rsacherer. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@staging/operator-lifecycle-manager/cmd/catalog/start.go`:
- Around line 52-58: Update the TimestampFormat in the msFormatter configuration
to use the millisecond layout `.000` instead of `.000000`, while preserving the
existing timestamp format and logger setup.
- Around line 56-58: Update the logrus.TextFormatter configuration in the start
logging setup to use three fractional-second digits and UTC-normalized
timestamps. Change TimestampFormat from six-digit microsecond precision to
millisecond precision and ensure the formatted entry time is converted to UTC
before formatting, preserving FullTimestamp behavior.
In
`@staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go`:
- Around line 2682-2691: Remove the unreachable BundleSecret creation diagnostic
from the fallback resolver.BundleSecretKind case in doStep. Add the equivalent
log to the active NewBundleSecretStep creation path, preserving the existing
fields and message so it records the plan resourceVersion when the secret is
actually created.
In `@staging/operator-lifecycle-manager/pkg/controller/operators/catalog/step.go`:
- Around line 403-404: The BundleSecret flow must use the
OperatorGroup-attenuated namespaced client rather than the privileged client. In
staging/operator-lifecycle-manager/pkg/controller/operators/catalog/step.go:403-404,
update NewBundleSecretStep’s Secret creation to use the scoped client for
ServiceAccount and Secret operations; in
staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go:2522,
pass both the privileged CRD client and attenuated namespaced client through
ExecutePlan and select the scoped client for BundleSecret steps.
- Around line 395-400: Update the CSV-not-found branch in the step status logic
to remove only the empty-UID CSV owner reference created earlier, rather than
clearing all references via SetOwnerReferences(nil). Preserve every unrelated
existing owner reference while retaining the intended cleanup of the absent CSV
reference.
🪄 Autofix
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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c8f498a-03f8-4ff0-9e74-b9f1f9afbcd4
⛔ Files ignored due to path filters (4)
vendor/github.com/operator-framework/operator-lifecycle-manager/cmd/catalog/start.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/catalog/step.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver/steps.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (4)
staging/operator-lifecycle-manager/cmd/catalog/start.gostaging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.gostaging/operator-lifecycle-manager/pkg/controller/operators/catalog/step.gostaging/operator-lifecycle-manager/pkg/controller/registry/resolver/steps.go
Comment on lines
+52
to
+58
| // OCPBUGS-35210: use millisecond timestamps so OLM and audit log | ||
| // entries can be correlated at sub-second precision. | ||
| // Set on both the local logger AND the global package logger so that | ||
| // code using logrus.WithFields() directly also emits milliseconds. | ||
| msFormatter := &logrus.TextFormatter{ | ||
| TimestampFormat: "2006-01-02T15:04:05.000000Z07:00", | ||
| FullTimestamp: true, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
file="staging/operator-lifecycle-manager/cmd/catalog/start.go"
sed -n '52,61p' "$file"Repository: openshift/operator-framework-olm
Length of output: 642
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
file="staging/operator-lifecycle-manager/cmd/catalog/start.go"
printf '%s\n' '--- start.go context ---'
sed -n '1,100p' "$file"
printf '%s\n' '--- related timestamp references ---'
rg -n -S 'OCPBUGS-35210|TimestampFormat|millisecond|microsecond|FullTimestamp|SetFormatter' \
staging/operator-lifecycle-manager staging 2>/dev/null | head -200
printf '%s\n' '--- repository status and focused diff ---'
git status --short
git diff --stat -- "$file"
git diff -- "$file"Repository: openshift/operator-framework-olm
Length of output: 10505
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
layout = "2006-01-02T15:04:05.000000Z07:00"
fraction = layout.split("T", 1)[1].split("Z", 1)[0].split(".", 1)[1]
print(f"fractional layout: .{fraction}")
print(f"fractional digits: {len(fraction)}")
print("precision: " + ("milliseconds" if fraction == "000" else
"microseconds" if fraction == "000000" else "other"))
sample = "2026-08-01T12:34:56.123456Z"
rendered_fraction = sample.split(".", 1)[1].split("Z", 1)[0]
print(f"sample fractional digits: {len(rendered_fraction)}")
PY
printf '%s\n' '--- logrus formatter references ---'
rg -n -S 'type TextFormatter struct|TimestampFormat|FullTimestamp' \
. -g '*.go' -g 'go.mod' -g 'vendor/**' 2>/dev/null | head -120
printf '%s\n' '--- module version ---'
rg -n '^github.com/sirupsen/logrus' go.mod staging/operator-lifecycle-manager/go.mod 2>/dev/null || trueRepository: openshift/operator-framework-olm
Length of output: 8605
Use millisecond timestamp formatting.
The OCPBUGS-35210 comments require milliseconds, but .000000 emits microseconds. Change the layout to .000.
🤖 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 `@staging/operator-lifecycle-manager/cmd/catalog/start.go` around lines 52 -
58, Update the TimestampFormat in the msFormatter configuration to use the
millisecond layout `.000` instead of `.000000`, while preserving the existing
timestamp format and logger setup.
Comment on lines
+56
to
+58
| msFormatter := &logrus.TextFormatter{ | ||
| TimestampFormat: "2006-01-02T15:04:05.000000Z07:00", | ||
| FullTimestamp: true, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n --glob '*.go' --glob '*.yaml' --glob '*.yml' \
'TimestampFormat|Z07:00|UTC\(\)|time\.Local|TZ' .Repository: openshift/operator-framework-olm
Length of output: 50389
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
file="staging/operator-lifecycle-manager/cmd/catalog/start.go"
printf '%s\n' '--- target file ---'
sed -n '1,110p' "$file"
printf '%s\n' '--- relevant repository references ---'
rg -n --glob '*.go' --glob '*.yaml' --glob '*.yml' \
'msFormatter|TimestampFormat|SetFormatter|time\.Local|TZ|UTC\(\)|TZ=' \
staging/operator-lifecycle-manager deploy config manifests 2>/dev/null | head -n 300 || true
printf '%s\n' '--- module/logrus version ---'
rg -n 'sirupsen/logrus|logrus v' go.mod go.sum staging/operator-lifecycle-manager 2>/dev/null | head -n 40 || trueRepository: openshift/operator-framework-olm
Length of output: 32321
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("staging/operator-lifecycle-manager/cmd/catalog/start.go")
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if 45 <= i <= 70:
print(f"{i:4}: {line}")
PYRepository: openshift/operator-framework-olm
Length of output: 1107
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
file="staging/operator-lifecycle-manager/cmd/catalog/start.go"
sed -n '1,110p' "$file"
rg -n --glob '*.go' --glob '*.yaml' --glob '*.yml' \
'msFormatter|TimestampFormat|SetFormatter|time\.Local|TZ|UTC\(\)|TZ=' \
staging/operator-lifecycle-manager deploy config manifests 2>/dev/null | head -n 300 || true
rg -n 'sirupsen/logrus|logrus v' go.mod go.sum staging/operator-lifecycle-manager 2>/dev/null | head -n 40 || trueRepository: openshift/operator-framework-olm
Length of output: 32455
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("staging/operator-lifecycle-manager/cmd/catalog/start.go")
for i, line in enumerate(p.read_text().splitlines(), 1):
if 45 <= i <= 70:
print(f"{i:4}: {line}")
PYRepository: openshift/operator-framework-olm
Length of output: 1107
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- Logrus v1.9.4 formatter behavior ---'
curl -fsSL \
https://raw.githubusercontent.com/sirupsen/logrus/v1.9.4/text_formatter.go |
rg -n -C 8 'TimestampFormat|entry\.Time|time\.Now\(\)|Format\('
printf '%s\n' '--- Go time layout references ---'
rg -n --glob '*.go' --glob '*.md' --glob '*.yaml' --glob '*.yml' \
'2006-01-02T15:04:05\.(000|000000)|millisecond timestamps|microsecond|RFC3339' \
staging/operator-lifecycle-manager | head -n 100Repository: openshift/operator-framework-olm
Length of output: 5208
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- candidate catalog deployment and image files ---'
git ls-files | rg -i 'catalog|operator.*deployment|deployment.*operator|olm.*operator' | head -n 250
printf '%s\n' '--- timezone settings in candidate deployment/config files ---'
files=$(git ls-files | rg -i 'catalog|operator.*deployment|deployment.*operator|olm.*operator' | head -n 250)
if [ -n "$files" ]; then
rg -n -i '(^|[[:space:]-])(env|name|value|TZ|timezone|utc)(:|[[:space:]])|TZ=' $files 2>/dev/null | head -n 200 || true
fiRepository: openshift/operator-framework-olm
Length of output: 43950
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
curl -fsSL \
https://raw.githubusercontent.com/sirupsen/logrus/v1.9.4/text_formatter.go |
rg -n -C 8 'TimestampFormat|entry\.Time|time\.Now\(\)|Format\('
rg -n --glob '*.go' --glob '*.md' --glob '*.yaml' --glob '*.yml' \
'2006-01-02T15:04:05\.(000|000000)|millisecond timestamps|microsecond|RFC3339' \
staging/operator-lifecycle-manager | head -n 100Repository: openshift/operator-framework-olm
Length of output: 5133
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
git ls-files | rg -i 'catalog|operator.*deployment|deployment.*operator|olm.*operator' | head -n 250
files=$(git ls-files | rg -i 'catalog|operator.*deployment|deployment.*operator|olm.*operator' | head -n 250)
if [ -n "$files" ]; then
rg -n -i '(^|[[:space:]-])(env|name|value|TZ|timezone|utc)(:|[[:space:]])|TZ=' \
$files 2>/dev/null | head -n 200 || true
fiRepository: openshift/operator-framework-olm
Length of output: 42988
Use millisecond precision and normalize timestamps to UTC.
000000 emits six fractional digits, not milliseconds; use 000. Logrus formats entry.Time directly, and Z07:00 preserves non-UTC offsets. Normalize timestamps to UTC before formatting when UTC output is required.
🤖 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 `@staging/operator-lifecycle-manager/cmd/catalog/start.go` around lines 56 -
58, Update the logrus.TextFormatter configuration in the start logging setup to
use three fractional-second digits and UTC-normalized timestamps. Change
TimestampFormat from six-digit microsecond precision to millisecond precision
and ensure the formatted entry time is converted to UTC before formatting,
preserving FullTimestamp behavior.
Comment on lines
+2682
to
+2691
| // OCPBUGS-35210: log the plan resourceVersion at the moment of creation. | ||
| // This ties the secret creation to a specific plan version, letting us | ||
| // confirm whether a concurrent reconcile held a stale or current view. | ||
| o.logger.WithFields(logrus.Fields{ | ||
| "secret": s.Name, | ||
| "sa": s.Annotations[corev1.ServiceAccountNameKey], | ||
| "planRV": plan.ResourceVersion, | ||
| "stepIndex": i, | ||
| "stepStatus": step.Status, | ||
| }).Debug("ExecutePlan: creating BundleSecret (OCPBUGS-35210)") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Move the BundleSecret creation log to the active execution path.
b.create dispatches BundleSecret steps to NewBundleSecretStep, and doStep returns at Line 2565. This fallback resolver.BundleSecretKind case does not execute. Move this diagnostic into NewBundleSecretStep, or remove it.
🤖 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
`@staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go`
around lines 2682 - 2691, Remove the unreachable BundleSecret creation
diagnostic from the fallback resolver.BundleSecretKind case in doStep. Add the
equivalent log to the active NewBundleSecretStep creation path, preserving the
existing fields and message so it records the plan resourceVersion when the
secret is actually created.
Comment on lines
+395
to
+400
| } else if !apierrors.IsNotFound(err) { | ||
| return v1alpha1.StepStatusUnknown, err | ||
| } else { | ||
| // CSV not found — clear the empty-UID owner ref to avoid API rejection. | ||
| s.SetOwnerReferences(nil) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve existing owner references when the CSV is absent.
Line 399 removes every owner reference. It must remove only the empty-UID CSV reference added at Line 384. Otherwise, a Secret manifest with other owner references loses them and can outlive its intended owner.
Proposed fix
- s.SetOwnerReferences(nil)
+ refs := s.GetOwnerReferences()
+ filtered := refs[:0]
+ for _, ref := range refs {
+ if ref.Kind == v1alpha1.ClusterServiceVersionKind &&
+ ref.Name == step.Resolving && ref.UID == "" {
+ continue
+ }
+ filtered = append(filtered, ref)
+ }
+ s.SetOwnerReferences(filtered)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else if !apierrors.IsNotFound(err) { | |
| return v1alpha1.StepStatusUnknown, err | |
| } else { | |
| // CSV not found — clear the empty-UID owner ref to avoid API rejection. | |
| s.SetOwnerReferences(nil) | |
| } | |
| } else if !apierrors.IsNotFound(err) { | |
| return v1alpha1.StepStatusUnknown, err | |
| } else { | |
| // CSV not found — clear the empty-UID owner ref to avoid API rejection. | |
| refs := s.GetOwnerReferences() | |
| filtered := refs[:0] | |
| for _, ref := range refs { | |
| if ref.Kind == v1alpha1.ClusterServiceVersionKind && | |
| ref.Name == step.Resolving && ref.UID == "" { | |
| continue | |
| } | |
| filtered = append(filtered, ref) | |
| } | |
| s.SetOwnerReferences(filtered) | |
| } |
🤖 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 `@staging/operator-lifecycle-manager/pkg/controller/operators/catalog/step.go`
around lines 395 - 400, Update the CSV-not-found branch in the step status logic
to remove only the empty-UID CSV owner reference created earlier, rather than
clearing all references via SetOwnerReferences(nil). Preserve every unrelated
existing owner reference while retaining the intended cleanup of the absent CSV
reference.
Comment on lines
+403
to
+404
| _, createErr := b.opclient.KubernetesInterface().CoreV1(). | ||
| Secrets(namespace).Create(context.TODO(), &s, metav1.CreateOptions{}) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Create BundleSecret resources with the attenuated client.
NewBundleSecretStep creates a namespaced Secret through b.opclient. ExecutePlan supplies builderKubeClient, which comes from the unattenuated factory for CRD installation. BundleSecret creation now bypasses the OperatorGroup attenuated ServiceAccount and uses default OLM credentials.
staging/operator-lifecycle-manager/pkg/controller/operators/catalog/step.go#L403-L404: Use a scoped client for ServiceAccount and Secret operations.staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go#L2522-L2522: Pass both the privileged CRD client and the attenuated namespaced client, then select the scoped client for BundleSecret steps.
📍 Affects 2 files
staging/operator-lifecycle-manager/pkg/controller/operators/catalog/step.go#L403-L404(this comment)staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go#L2522-L2522
🤖 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 `@staging/operator-lifecycle-manager/pkg/controller/operators/catalog/step.go`
around lines 403 - 404, The BundleSecret flow must use the
OperatorGroup-attenuated namespaced client rather than the privileged client. In
staging/operator-lifecycle-manager/pkg/controller/operators/catalog/step.go:403-404,
update NewBundleSecretStep’s Secret creation to use the scoped client for
ServiceAccount and Secret operations; in
staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go:2522,
pass both the privileged CRD client and attenuated namespaced client through
ExecutePlan and select the scoped client for BundleSecret steps.
Contributor
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
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
3 participants
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.
SA-token Secrets included in an operator bundle are placed earlier in the InstallPlan step list than the synthesized ServiceAccount step. OLM creates the Secret before the SA exists; the Kubernetes token controller (KCM) immediately deletes any token secret whose referenced ServiceAccount is absent, and OLM then marks the step as Created permanently — preventing any future retry and leaving the operator without its token secret.
The issue is not always reproduceable (see test document) because a rescheduled loop run is able to get the stale RV, which still does not see the Secret as created, but the prior loop created the SA so this time the Secret will not be deleted.
Fix: add a StepperFunc for BundleSecretKind (mirroring the existing CRD StepperFunc pattern). The new NewBundleSecretStep checks whether the SA referenced by the secret already exists before attempting creation:
Because the StepperFunc handles WaitingForAPI internally it never reaches the main ExecutePlan switch case that would otherwise skip the step, so no changes to the switch statement or to NeedsRequeue() are required.
Also adds structured debug logging to syncInstallPlans (plan resourceVersion, per-step BS/SA status at reconcile start, UpdateStatus call/result) to make the race observable in OLM pod logs during investigation.
Most likely the additional logging will be removed via further commits.
Tested: 30-iteration statistical reproducer against OCP 4.17 — 0/30 bug fires with the fix (without the fix we are looking at roughly 60% failure/40% Success).
This PR is still a draft and work in progress.
Summary by CodeRabbit
Bug Fixes
Diagnostics