ESO-155: Fixes bitwarden deployment to use custom certificates #64
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@bharath-b-rh: This pull request references ESO-155 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.21.0" version, but no target version was set. In 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 APPROVED This pull-request has been approved by: bharath-b-rh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
| case bitwardenDeploymentAssetName: | ||
| deployment.Labels["app.kubernetes.io/version"] = bitwardenImageVersionEnvVarName | ||
| updateBitwardenServerContainerSpec(deployment, bitwardenImage) | ||
| updateBitwardenVolumeConfig(deployment, externalsecrets) |
There was a problem hiding this comment.
[claude generated] Good architectural improvement: Adding the volume config update here ensures Bitwarden deployments get the proper certificate configuration. However, consider adding error handling for cases where the deployment structure is unexpected.
| } | ||
| for i, volume := range deployment.Spec.Template.Spec.Volumes { | ||
| if volume.Name == "bitwarden-tls-certs" { | ||
| deployment.Spec.Template.Spec.Volumes[i].Secret.SecretName = secretName |
There was a problem hiding this comment.
[claude generated] Security consideration: The code assumes volume.Secret is not nil, but this could cause a nil pointer dereference panic if the volume is configured as a different type (ConfigMap, EmptyDir, etc.). Consider adding a nil check: if volume.Secret != nil before accessing SecretName.
| } | ||
| return true, nil | ||
| }) | ||
| m.UpdateWithRetryCalls(func(ctx context.Context, obj client.Object, _ ...client.UpdateOption) error { |
There was a problem hiding this comment.
[claude generated] Test improvement: Good addition of deployment capture mechanism. This allows proper validation of the reconciler's behavior. Consider adding validation for the case where capturedDeployment is nil.
| } | ||
| }, | ||
| }, | ||
| { |
There was a problem hiding this comment.
[claude generated] Excellent test coverage: The new Bitwarden test case thoroughly validates both volume secret name updates and container image updates. This provides good regression protection for the certificate configuration feature.
| }, | ||
| } | ||
| }, | ||
| validateDeployment: func(t *testing.T, deployment *appsv1.Deployment) { |
There was a problem hiding this comment.
The validation logic is a bit too verbose.
Please consider a declarative approach now or in future.
Like a list of paths/hierarchies in the deployment struct, the expected value, and the assertion text.
| updateWebhookContainerSpec(deployment, image, logLevel) | ||
| case certControllerDeploymentAssetName: | ||
| updateCertControllerContainerSpec(deployment, image, logLevel) | ||
| case bitwardenDeploymentAssetName: |
There was a problem hiding this comment.
Initially we are creating a list of various deployments, and feeding it to createOrApplyDeploymentFromAsset.
createOrApplyDeploymentFromAsset is calling the current function, and then we are again having conditional logic based on the specific kind of deployment we are working with.
i feel we can refactor it by supplementing the "deployments" struct:
deployments := []struct {
assetName string
condition bool
}
with more fields so that the code that works on it further need not differentiate on the actual kind.
Attaching an example implementation (it excludes the current PR changes):
restructure.patch
There was a problem hiding this comment.
Agree, will create a follow up PR to refactor the code.
Signed-off-by: Bharath B <[email protected]>
bharath-b-rh
force-pushed
the
eso-155-2
branch
from
6f7ce58 to
faa64e8
Compare
|
New changes are detected. LGTM label has been removed. |
WalkthroughAdds a helper that sets the Bitwarden TLS certs volume SecretName from BitwardenSecretManagerProvider.SecretRef and calls it when building the Bitwarden Deployment. Tests were expanded to capture the reconciled Deployment, assert the volume secret name, verify container image, and validate pod spec fields in a new Bitwarden-with-secretRef test. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (2)
🔇 Additional comments (4)
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. 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: 3
📜 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 (2)
pkg/controller/external_secrets/deployments.go(2 hunks)pkg/controller/external_secrets/deployments_test.go(16 hunks)
🔇 Additional comments (4)
pkg/controller/external_secrets/deployments.go (1)
134-134: LGTM!The integration of the Bitwarden volume configuration update is properly placed within the bitwarden deployment case.
pkg/controller/external_secrets/deployments_test.go (3)
24-47: LGTM!The deployment capture mechanism is well implemented and enables proper validation of the reconciler's behavior. The use of a pointer-to-pointer pattern allows the preReq function to populate the captured deployment for later validation.
Also applies to: 630-633
52-312: Comprehensive validation with room for future refactoring.The validation functions provide thorough coverage of deployment configurations including images, labels, affinity rules, tolerations, node selectors, and resource requirements. While the validation logic is verbose, it ensures proper behavior.
As per a past review comment: Consider a declarative approach in future refactoring—a list of paths/hierarchies in the deployment struct with expected values and assertion text could reduce verbosity.
651-658: LGTM!The validation ensures that the status image is correctly updated after successful deployment reconciliation and that the validateDeployment callback is invoked when provided. This completes the end-to-end validation flow.
| func updateBitwardenVolumeConfig(deployment *appsv1.Deployment, esc *operatorv1alpha1.ExternalSecretsConfig) { | ||
| if esc.Spec.Plugins.BitwardenSecretManagerProvider.SecretRef != nil && | ||
| esc.Spec.Plugins.BitwardenSecretManagerProvider.SecretRef.Name != "" { | ||
| for i := range deployment.Spec.Template.Spec.Volumes { | ||
| if deployment.Spec.Template.Spec.Volumes[i].Name == "bitwarden-tls-certs" { | ||
| deployment.Spec.Template.Spec.Volumes[i].Secret.SecretName = esc.Spec.Plugins.BitwardenSecretManagerProvider.SecretRef.Name | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add nil checks to prevent potential panics.
The function has two nil pointer dereference risks:
- Line 398:
esc.Spec.Plugins.BitwardenSecretManagerProvider.SecretRef- No nil check forBitwardenSecretManagerProviderbefore accessingSecretRef - Line 402:
deployment.Spec.Template.Spec.Volumes[i].Secret.SecretName- No nil check forSecretbefore accessingSecretName. If the volume is configured as a different type (ConfigMap, EmptyDir, etc.), this will panic.
Apply this diff to add defensive checks and optimize the loop:
func updateBitwardenVolumeConfig(deployment *appsv1.Deployment, esc *operatorv1alpha1.ExternalSecretsConfig) {
- if esc.Spec.Plugins.BitwardenSecretManagerProvider.SecretRef != nil &&
- esc.Spec.Plugins.BitwardenSecretManagerProvider.SecretRef.Name != "" {
+ if esc.Spec.Plugins.BitwardenSecretManagerProvider != nil &&
+ esc.Spec.Plugins.BitwardenSecretManagerProvider.SecretRef != nil &&
+ esc.Spec.Plugins.BitwardenSecretManagerProvider.SecretRef.Name != "" {
for i := range deployment.Spec.Template.Spec.Volumes {
if deployment.Spec.Template.Spec.Volumes[i].Name == "bitwarden-tls-certs" {
+ if deployment.Spec.Template.Spec.Volumes[i].Secret == nil {
+ continue
+ }
deployment.Spec.Template.Spec.Volumes[i].Secret.SecretName = esc.Spec.Plugins.BitwardenSecretManagerProvider.SecretRef.Name
+ break
}
}
}
}📝 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.
| func updateBitwardenVolumeConfig(deployment *appsv1.Deployment, esc *operatorv1alpha1.ExternalSecretsConfig) { | |
| if esc.Spec.Plugins.BitwardenSecretManagerProvider.SecretRef != nil && | |
| esc.Spec.Plugins.BitwardenSecretManagerProvider.SecretRef.Name != "" { | |
| for i := range deployment.Spec.Template.Spec.Volumes { | |
| if deployment.Spec.Template.Spec.Volumes[i].Name == "bitwarden-tls-certs" { | |
| deployment.Spec.Template.Spec.Volumes[i].Secret.SecretName = esc.Spec.Plugins.BitwardenSecretManagerProvider.SecretRef.Name | |
| } | |
| } | |
| } | |
| } | |
| func updateBitwardenVolumeConfig(deployment *appsv1.Deployment, esc *operatorv1alpha1.ExternalSecretsConfig) { | |
| if esc.Spec.Plugins.BitwardenSecretManagerProvider != nil && | |
| esc.Spec.Plugins.BitwardenSecretManagerProvider.SecretRef != nil && | |
| esc.Spec.Plugins.BitwardenSecretManagerProvider.SecretRef.Name != "" { | |
| for i := range deployment.Spec.Template.Spec.Volumes { | |
| if deployment.Spec.Template.Spec.Volumes[i].Name == "bitwarden-tls-certs" { | |
| if deployment.Spec.Template.Spec.Volumes[i].Secret == nil { | |
| continue | |
| } | |
| deployment.Spec.Template.Spec.Volumes[i].Secret.SecretName = esc.Spec.Plugins.BitwardenSecretManagerProvider.SecretRef.Name | |
| break | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In pkg/controller/external_secrets/deployments.go around lines 397 to 406, guard
against nil dereferences by first checking
esc.Spec.Plugins.BitwardenSecretManagerProvider is not nil before accessing its
SecretRef, then check SecretRef is non-nil and has a non-empty Name; iterate the
deployment.Spec.Template.Spec.Volumes and for each volume only update
Secret.SecretName if volume.Secret is non-nil (skip other volume types), assign
the SecretName from
esc.Spec.Plugins.BitwardenSecretManagerProvider.SecretRef.Name and break the
loop once the "bitwarden-tls-certs" volume is found and updated.
Signed-off-by: Bharath B <[email protected]>
bharath-b-rh
force-pushed
the
eso-155-2
branch
from
abab87b to
5e3364d
Compare
openshift-ci
bot
added
px-approved
|
@bharath-b-rh: This pull request references ESO-155 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.21.0" version, but no target version was set. In 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. |
|
/label px-approved |
openshift-ci
bot
added
the
docs-approved
|
/remove qe-approved |
|
/remove-qe-approved |
|
/hold for verification |
openshift-ci
bot
added
the
do-not-merge/hold
|
@bharath-b-rh: The following test failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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 kubernetes-sigs/prow repository. |
PR has following changes