Add support for injecting CA certificates into Secrets #265
Conversation
Closes openshift#264 Signed-off-by: Marco Nenciarini <[email protected]>
openshift-ci
bot
added
the
needs-ok-to-test
|
Hi @mnencia. 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 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mnencia 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 |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
openshift-ci
bot
removed
the
lifecycle/stale
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
WalkthroughIntroduces Secret CA bundle injection support alongside existing ConfigMap injection. Implements a new Secret injector controller with idempotent update logic, integrates it into the startup pipeline, updates documentation to reflect both ConfigMap and Secret support, and adds comprehensive end-to-end tests validating injection and update semantics. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Assessment against linked issues
Out-of-scope changesNo out-of-scope changes detected. All modifications directly support the objective of enabling CA certificate injection into Secrets. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
README.md(1 hunks)pkg/controller/cabundleinjector/secret.go(1 hunks)pkg/controller/cabundleinjector/starter.go(1 hunks)test/e2e/e2e_test.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
README.mdpkg/controller/cabundleinjector/secret.gopkg/controller/cabundleinjector/starter.gotest/e2e/e2e_test.go
🧬 Code graph analysis (2)
pkg/controller/cabundleinjector/secret.go (1)
pkg/controller/api/api.go (3)
InjectCABundleAnnotationName(27-27)InjectionDataKey(29-29)OwningJiraComponent(22-22)
test/e2e/e2e_test.go (1)
pkg/controller/api/api.go (1)
InjectionDataKey(29-29)
🪛 markdownlint-cli2 (0.18.1)
README.md
9-9: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (3)
pkg/controller/cabundleinjector/starter.go (1)
89-97: Wiring the Secret injector into the startup pipeline looks correctAdding
newSecretInjectorConfigtoconfigConstructorsmatches the existing pattern for other injectors and ensures the Secret injector runs with the shared informer factory and annotations filter.README.md (1)
7-9: Updated docs correctly mention Secret-based CA bundle injectionThe README text for the “configmap/secret cabundle injector” accurately reflects that both ConfigMaps and Secrets annotated with
inject-cabundle=trueare now handled, and explains theservice-ca.crtdata item usage clearly.test/e2e/e2e_test.go (1)
213-223: Secret CA-bundle e2e helpers and tests mirror existing ConfigMap coverageThe new helpers for creating annotated Secrets, polling for injection, editing data, and validating contents closely follow the existing ConfigMap patterns and exercise both initial injection and “stomp-on-change” behavior for dedicated injection Secrets. Timeouts and polling are consistent with the rest of the suite, so this should give good end-to-end coverage of the new Secret injector without introducing obvious flakiness.
Also applies to: 251-262, 297-313, 358-372, 1498-1558
| // skip updating when the CA bundle is already there | ||
| if data, ok := secret.Data[api.InjectionDataKey]; ok && | ||
| string(data) == bi.caBundle && len(secret.Data) == 1 { | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| klog.Infof("updating secret %s/%s with the service signing CA bundle", secret.Namespace, secret.Name) | ||
|
|
||
| // make a copy to avoid mutating cache state | ||
| secretCopy := secret.DeepCopy() | ||
| secretCopy.Data = map[string][]byte{api.InjectionDataKey: []byte(bi.caBundle)} | ||
| // set the owning-component unless someone else has claimed it. | ||
| if len(secretCopy.Annotations[apiannotations.OpenShiftComponent]) == 0 { | ||
| secretCopy.Annotations[apiannotations.OpenShiftComponent] = api.OwningJiraComponent | ||
| secretCopy.Annotations[apiannotations.OpenShiftDescription] = fmt.Sprintf("Secret is added/updated with a data item containing the CA signing bundle that can be used to verify service-serving certificates") | ||
| } | ||
|
|
||
| _, err = bi.client.Secrets(secretCopy.Namespace).Update(ctx, secretCopy, metav1.UpdateOptions{}) |
There was a problem hiding this comment.
Fix nil Annotations panic and reconsider overwriting all Secret data
Two things to flag here:
- Potential panic on nil annotations (must fix):
secretCopy.Annotationsmay benilfor a freshly-created Secret. Writing to a nil map will panic:
if len(secretCopy.Annotations[apiannotations.OpenShiftComponent]) == 0 {
secretCopy.Annotations[apiannotations.OpenShiftComponent] = api.OwningJiraComponent
secretCopy.Annotations[apiannotations.OpenShiftDescription] = fmt.Sprintf("Secret is added/updated with a data item containing the CA signing bundle that can be used to verify service-serving certificates")
}Initialize the map before writing:
- secretCopy := secret.DeepCopy()
- secretCopy.Data = map[string][]byte{api.InjectionDataKey: []byte(bi.caBundle)}
- // set the owning-component unless someone else has claimed it.
- if len(secretCopy.Annotations[apiannotations.OpenShiftComponent]) == 0 {
+ secretCopy := secret.DeepCopy()
+ secretCopy.Data = map[string][]byte{api.InjectionDataKey: []byte(bi.caBundle)}
+ // ensure annotations map is initialized before writing to it
+ if secretCopy.Annotations == nil {
+ secretCopy.Annotations = map[string]string{}
+ }
+ // set the owning-component unless someone else has claimed it.
+ if len(secretCopy.Annotations[apiannotations.OpenShiftComponent]) == 0 {
secretCopy.Annotations[apiannotations.OpenShiftComponent] = api.OwningJiraComponent
secretCopy.Annotations[apiannotations.OpenShiftDescription] = fmt.Sprintf("Secret is added/updated with a data item containing the CA signing bundle that can be used to verify service-serving certificates")
}- Secret
Datais fully overwritten (design/semantics):
secretCopy.Data = map[string][]byte{...}drops all existing keys and, together with thelen(secret.Data) == 1skip condition, forces injected Secrets to contain onlyservice-ca.crt. That’s consistent with the new e2e tests (which assertlen(Data)==1and that extra keys get “stomped”), but it is stricter than the README wording (“adds or updates a data item”) and prevents co-locating other material in the same Secret.
If you want parity with a more typical “only manage InjectionDataKey” pattern (and to better match the README text), consider instead:
- secretCopy := secret.DeepCopy()
- secretCopy.Data = map[string][]byte{api.InjectionDataKey: []byte(bi.caBundle)}
+ secretCopy := secret.DeepCopy()
+ if secretCopy.Data == nil {
+ secretCopy.Data = map[string][]byte{}
+ }
+ secretCopy.Data[api.InjectionDataKey] = []byte(bi.caBundle)and relaxing the len(secret.Data) == 1 checks in Sync/tests to only care that InjectionDataKey matches the operator’s bundle.
🤖 Prompt for AI Agents
In pkg/controller/cabundleinjector/secret.go around lines 61 to 79, initialize
secretCopy.Annotations if nil before writing to it to avoid a nil-map panic, and
stop overwriting the entire Secret.Data: instead of assigning a new map, set or
update only the api.InjectionDataKey entry on secretCopy.Data (ensuring
secretCopy.Data is non-nil first) and adjust the earlier skip condition so it
checks only that the InjectionDataKey value matches bi.caBundle rather than
relying on len(secret.Data)==1.
|
/remove-lifecycle stale |
openshift-ci
bot
removed
the
lifecycle/stale
2 participants
This pull request adds support for injecting the service CA bundle into Secrets to the existing functionality for ConfigMaps.
Closes #264.