OCPBUGS-73938: cabundleinjector: Add CA bundles watcher#329
OCPBUGS-73938: cabundleinjector: Add CA bundles watcher#329tchap wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@tchap: This pull request references Jira Issue OCPBUGS-73938, 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. |
openshift-ci-robot
added
the
jira/invalid-bug
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR refactors CA bundle management across multiple injector types (webhook, API service, config map, CRD) from direct byte/string storage to atomic pointers for thread-safe concurrent access. It introduces a file observer mechanism in the starter that polls CA bundle files and updates the atomic pointers dynamically. Changes
Sequence DiagramsequenceDiagram
participant FileSystem as File System<br/>(ca-bundle.crt, sa.crt)
participant Observer as CA Observer<br/>(startCAObserver)
participant AtomicPtr as Atomic Pointer<br/>Store
participant Injectors as CA Bundle<br/>Injectors
participant Sync as Sync Flow
Observer->>FileSystem: Initial read via readBundles
FileSystem-->>Observer: ca-bundle, sa-token-ca-bundle
Observer->>AtomicPtr: Store bundles in atomic.Pointer
Observer->>Observer: Start file polling loop
Injectors->>AtomicPtr: Initialize with pointer refs
FileSystem->>Observer: File change detected (poll)
Observer->>FileSystem: Re-read bundles
FileSystem-->>Observer: Updated content
Observer->>AtomicPtr: Update atomic pointers
Sync->>AtomicPtr: Load() pointer value
AtomicPtr-->>Sync: Dereference CA bundle
Sync->>Injectors: Apply CA bundle to resources
FileSystem->>Observer: File deletion
Observer->>AtomicPtr: Retain last-known value
Sync->>AtomicPtr: Load() still returns previous value
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@tchap: This pull request references Jira Issue OCPBUGS-73938, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tchap 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 |
|
@tchap: This pull request references Jira Issue OCPBUGS-73938, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controller/cabundleinjector/starter.go`:
- Around line 132-143: The observer loses a retained saToken CA bundle when
saTokenCAFile is deleted but caBundleFile is later updated because
reloadCABundles() calls readBundles(caBundleFile, saTokenCAFile) which returns
nil for saTokenCABundleContent on missing file and that nil is written back to
saTokenCABundleData; fix by changing reloadCABundles()/readBundles call to pass
the current cached saTokenCABundleData as the fallback (e.g., pass
saTokenCABundleData into readBundles or add a parameter/current value) and
update readBundles to return the existing saToken bundle when the file is absent
so saTokenCABundleData is not overwritten; apply same pattern for the legacy
vulnerable CA path (vulnerableLegacyCABundleData) wherever readBundles is
called.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fb29a04f-5862-4090-b895-beee77fc1cb5
📒 Files selected for processing (7)
pkg/controller/cabundleinjector/admissionwebhook.gopkg/controller/cabundleinjector/admissionwebhook_test.gopkg/controller/cabundleinjector/apiservice.gopkg/controller/cabundleinjector/configmap.gopkg/controller/cabundleinjector/crd.gopkg/controller/cabundleinjector/starter.gopkg/controller/cabundleinjector/starter_test.go
|
/retitle WIP: OCPBUGS-73938: cabundleinjector: Add CA bundles watcher |
openshift-ci
bot
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
tchap
force-pushed
the
watch-ca-bundles
branch
9 times, most recently
from
e4b41d8 to
83343d9
Compare
tchap
force-pushed
the
watch-ca-bundles
branch
2 times, most recently
from
778254d to
291cf41
Compare
Add a watcher to watch all relevant CA bundle files for changes so that they are automatically reloaded when modified. There is also a periodic resync implemented and set to 10 minutes. The current implementation does not update all affected ConfigMaps. This means that only the ConfigMaps created after the bundles are reloaded are affected and up-to-date with respect to the files present on the disk.
|
@tchap: This pull request references Jira Issue OCPBUGS-73938, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (ksiddiqu@redhat.com), skipping review request. 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. |
|
/retitle OCPBUGS-73938: cabundleinjector: Add CA bundles watcher |
openshift-ci
bot
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@tchap: 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. |
2 participants
Add a watcher to watch all relevant CA bundle files for changes
so that they are automatically reloaded when modified.
There is also a periodic resync implemented and set to 10 minutes.
The current implementation does not update all affected ConfigMaps. This
means that only the ConfigMaps created after the bundles are reloaded
are affected and up-to-date with respect to the files present on the
disk.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests