implemented cleanup especially for sidecars #16
Conversation
WalkthroughThe recent updates to the codebase focus on enhancing the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files ignored due to filter (6)
- tests/e2e/cleanup/00-assert.yaml
- tests/e2e/cleanup/00-install.yaml
- tests/e2e/cleanup/01-assert.yaml
- tests/e2e/cleanup/01-delete-deployment.yaml
- tests/e2e/cleanup/02-assert.yaml
- tests/e2e/cleanup/02-delete-sidecar.yaml
Files selected for processing (5)
- internal/controller/basic_authenticator/basicauthenticator_controller.go (2 hunks)
- internal/controller/basic_authenticator/cleanup.go (1 hunks)
- internal/controller/basic_authenticator/const.go (2 hunks)
- internal/controller/basic_authenticator/provision.go (3 hunks)
- internal/controller/basic_authenticator/workload.go (6 hunks)
Additional comments: 13
internal/controller/basic_authenticator/basicauthenticator_controller.go (4)
22-22: The addition of the
github.com/opdev/subreconcilerimport aligns with the PR objectives to enhance error handling and conditional logic within theReconcilefunction.27-27: The addition of
"k8s.io/apimachinery/pkg/api/errors"import is necessary for the new error handling logic introduced in theReconcilefunction.61-76: The updated error handling and conditional logic in the
Reconcilefunction are well-structured and follow idiomatic Go practices. The use of a switch statement to handle the different outcomes of theGetcall is appropriate. The use ofsubreconciler.Evaluate(subreconciler.Requeue())for error handling is consistent with the newly addedsubreconcilerpackage.67-75: The logic to call
r.Cleanupin theReconcilefunction when theBasicAuthenticatorobject is not found or is marked for deletion is a good practice for managing cleanup of resources.internal/controller/basic_authenticator/cleanup.go (1)
- 150-168: Verify that the nginx container name and the basic authenticator label are correctly defined and consistently used throughout the codebase.
The verification of the
nginxDefaultContainerNameandbasicAuthenticatorNameLabelconstants shows that they are defined inconst.goand used consistently across various files in the codebase, includingutility.go,cleanup.go, andworkload.go. The usage contexts provided in the output indicate that these constants are being used for their intended purposes, such as filtering out the nginx container during cleanup and labeling resources with the basic authenticator name.Final review comment:
The constants
nginxDefaultContainerNameandbasicAuthenticatorNameLabelare correctly defined inconst.goand consistently used throughout the codebase, as verified by the provided script output.internal/controller/basic_authenticator/const.go (1)
- 4-14: The new constants added for default image address, container name, labels, annotations, and paths are consistent with the naming conventions and the purpose of the PR.
internal/controller/basic_authenticator/provision.go (1)
- 19-26: The addition of
addCleanupFinalizerto thesubProvisionerslice in theProvisionmethod is consistent with the PR objectives to enhance cleanup for sidecars.internal/controller/basic_authenticator/workload.go (6)
30-30: The addition of
basicAuthenticatorNameLabelto the labels map is consistent with the PR's objectives and enhances the identification of resources related toBasicAuthenticator.100-100: The addition of
basicAuthenticatorNameLabelto the labels map in thecreateNginxConfigmapfunction aligns with the PR's objectives and ensures consistency in labeling across resources.144-144: The addition of
basicAuthenticatorNameLabelto the labels map in thecreateCredentialsfunction aligns with the PR's objectives and ensures consistency in labeling across resources.165-165: The addition of
basicAuthenticatorNameLabelto the labels map in thecreateNginxServicefunction aligns with the PR's objectives and ensures consistency in labeling across resources.203-247: The changes in the
injectorfunction, including the addition ofbasicAuthenticatorNameLabeland the logic for injecting the nginx container, are implemented correctly. The function checks for the existence of the container before adding it, which prevents duplicates and aligns with the PR's objectives.280-287: The
getContainerIndexfunction is implemented correctly. It provides a utility to find the index of a container by name within a list of containers, returning -1 if not found, which is a common pattern for such utility functions.
| func removeInjectedResources(deployments []*appsv1.Deployment, secrets []string, configmap []string) []*appsv1.Deployment { | ||
| targetSecret := secrets[0] // there should always be one secret | ||
| targetCm := configmap[0] // there should always be one configmap |
There was a problem hiding this comment.
💡 NOTE
Codebase verification is a beta feature.
The provided script output shows the context around the call to removeInjectedResources and the function definition itself. The log statement before the function call logs the configmaps and secrets, which suggests that there is a check or a retrieval process for these resources. However, the output does not explicitly show a check that ensures there is exactly one secret and one configmap before calling removeInjectedResources.
Given the absence of explicit checks in the provided context, it is not possible to confirm that the assumptions made in removeInjectedResources are always met. Therefore, the concern raised in the original review comment remains valid.
Final review comment:
The removeInjectedResources function assumes there is always one secret and one configmap, which could lead to index out of range errors if the assumptions are not met. The provided context does not show explicit checks to guarantee that there is exactly one secret and one configmap before calling this function. Please ensure that such checks are in place to prevent potential runtime errors.
- Ensure checks are in place to confirm the presence of exactly one secret and one configmap before invoking
removeInjectedResources.
Analysis chain
The removeInjectedResources function assumes there is always one secret and one configmap, which could lead to index out of range errors if the assumptions are not met. Ensure that there are checks in place to guarantee that there is exactly one secret and one configmap before calling this function.
There was a problem hiding this comment.
💡 NOTE
Deep insights is a beta feature.
@sinamna The script output provided does not show any changes to the removeInjectedResources function or the logic preceding its call. The concerns about the assumptions of having exactly one secret and one configmap before calling removeInjectedResources still stand based on the current output. Could you please confirm if the changes have been pushed to the branch or provide the updated logic if it's not yet committed?
Analysis chain
#!/bin/bash
# Search for the updated logic of the removeInjectedResources function and any related checks.
rg 'removeInjectedResources' --context 10[RUN_SCRIPTS]
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the updated logic of the removeInjectedResources function and any related checks.
rg 'removeInjectedResources' --context 10
Length of output: 3861
| func (r *BasicAuthenticatorReconciler) addCleanupFinalizer(ctx context.Context, req ctrl.Request) (*ctrl.Result, error) { | ||
| basicAuthenticator := &v1alpha1.BasicAuthenticator{} | ||
|
|
||
| if r, err := r.getLatestBasicAuthenticator(ctx, req, basicAuthenticator); subreconciler.ShouldHaltOrRequeue(r, err) { | ||
| return r, err | ||
| } | ||
| if !controllerutil.ContainsFinalizer(basicAuthenticator, basicAuthenticatorFinalizer) { | ||
| if objUpdated := controllerutil.AddFinalizer(basicAuthenticator, basicAuthenticatorFinalizer); objUpdated { | ||
| if err := r.Update(ctx, basicAuthenticator); err != nil { | ||
| r.logger.Error(err, "failed to add basicAuthenticator finalizer") | ||
| return subreconciler.Requeue() | ||
| } | ||
| } | ||
| } | ||
| return subreconciler.ContinueReconciling() | ||
| } |
There was a problem hiding this comment.
The addCleanupFinalizer method correctly checks for the presence of a finalizer before attempting to add it and handles errors appropriately. However, the check for objUpdated should be followed by a return statement to avoid unnecessary calls to Update if no changes were made.
+ if objUpdated := controllerutil.AddFinalizer(basicAuthenticator, basicAuthenticatorFinalizer); objUpdated {
+ if err := r.Update(ctx, basicAuthenticator); err != nil {
+ r.logger.Error(err, "failed to add basicAuthenticator finalizer")
+ return subreconciler.Requeue()
+ }
+ return subreconciler.ContinueReconciling() // This line should be added to avoid unnecessary updates
+ }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.
| func (r *BasicAuthenticatorReconciler) addCleanupFinalizer(ctx context.Context, req ctrl.Request) (*ctrl.Result, error) { | |
| basicAuthenticator := &v1alpha1.BasicAuthenticator{} | |
| if r, err := r.getLatestBasicAuthenticator(ctx, req, basicAuthenticator); subreconciler.ShouldHaltOrRequeue(r, err) { | |
| return r, err | |
| } | |
| if !controllerutil.ContainsFinalizer(basicAuthenticator, basicAuthenticatorFinalizer) { | |
| if objUpdated := controllerutil.AddFinalizer(basicAuthenticator, basicAuthenticatorFinalizer); objUpdated { | |
| if err := r.Update(ctx, basicAuthenticator); err != nil { | |
| r.logger.Error(err, "failed to add basicAuthenticator finalizer") | |
| return subreconciler.Requeue() | |
| } | |
| } | |
| } | |
| return subreconciler.ContinueReconciling() | |
| } | |
| func (r *BasicAuthenticatorReconciler) addCleanupFinalizer(ctx context.Context, req ctrl.Request) (*ctrl.Result, error) { | |
| basicAuthenticator := &v1alpha1.BasicAuthenticator{} | |
| if r, err := r.getLatestBasicAuthenticator(ctx, req, basicAuthenticator); subreconciler.ShouldHaltOrRequeue(r, err) { | |
| return r, err | |
| } | |
| if !controllerutil.ContainsFinalizer(basicAuthenticator, basicAuthenticatorFinalizer) { | |
| if objUpdated := controllerutil.AddFinalizer(basicAuthenticator, basicAuthenticatorFinalizer); objUpdated { | |
| if err := r.Update(ctx, basicAuthenticator); err != nil { | |
| r.logger.Error(err, "failed to add basicAuthenticator finalizer") | |
| return subreconciler.Requeue() | |
| } | |
| return subreconciler.ContinueReconciling() | |
| } | |
| } | |
| return subreconciler.ContinueReconciling() | |
| } |
There was a problem hiding this comment.
@@ -34,6 +36,23 @@ func (r *BasicAuthenticatorReconciler) Provision(ctx context.Context, req ctrl.R
return subreconciler.Evaluate(subreconciler.DoNotRequeue())
}
func (r *BasicAuthenticatorReconciler) addCleanupFinalizer(ctx context.Context, req ctrl.Request) (*ctrl.Result, error) {
basicAuthenticator := &v1alpha1.BasicAuthenticator{}
if r, err := r.getLatestBasicAuthenticator(ctx, req, basicAuthenticator); subreconciler.ShouldHaltOrRequeue(r, err) {
return r, err
}
if !controllerutil.ContainsFinalizer(basicAuthenticator, basicAuthenticatorFinalizer) {
if objUpdated := controllerutil.AddFinalizer(basicAuthenticator, basicAuthenticatorFinalizer); objUpdated {
if err := r.Update(ctx, basicAuthenticator); err != nil {
r.logger.Error(err, "failed to add basicAuthenticator finalizer")
return subreconciler.Requeue()
}
+ return subreconciler.ContinueReconciling()
}
}
return subreconciler.ContinueReconciling()
}@sinamna I've prepared the diff reflecting the suggested change. If your refactoring already covers this logic, then we're all set. If there's anything else you'd like me to look at, please let me know!
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- tests/e2e/adaptive-scaling/00-assert.yaml
- tests/e2e/sidecar/00-assert.yaml
Files selected for processing (2)
- internal/controller/basic_authenticator/cleanup.go (1 hunks)
- internal/controller/basic_authenticator/provision.go (3 hunks)
Additional comments: 6
internal/controller/basic_authenticator/cleanup.go (6)
16-30: The
Cleanupfunction uses a slice ofsubreconciler.FnWithRequestto modularly execute cleanup tasks. This approach is maintainable and allows for easy addition or removal of tasks in the future.32-73: The
removeInjectedContainersfunction correctly fetches the necessary resources and performs cleanup operations based on the type ofBasicAuthenticator. It also handles errors and logging appropriately.75-91: The
removeCleanupFinalizerfunction properly checks for and removes the finalizer from theBasicAuthenticatorobject, with error handling and requeuing logic in place.94-108: The
getTargetDeploymentfunction correctly lists deployments using the Kubernetes client with label selectors and namespace scoping, which is a best practice for resource retrieval in Kubernetes.110-123: The
getTargetConfigmapNamesfunction efficiently retrieves the names of configmaps that match the provided labels and are in the same namespace as theBasicAuthenticator.125-138: The
getTargetSecretNamefunction is implemented correctly to list secrets using label selectors and namespace scoping, aligning with Kubernetes API interaction best practices.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- internal/controller/basic_authenticator/cleanup.go (1 hunks)
- internal/controller/basic_authenticator/const.go (3 hunks)
- internal/controller/basic_authenticator/provision.go (4 hunks)
Additional comments: 14
internal/controller/basic_authenticator/cleanup.go (7)
16-31: The implementation of the
Cleanupfunction with subreconciler pattern appears to be correct and modular. It ensures proper error handling and requeuing logic.32-44: The
setDeletionStatusfunction correctly updates theBasicAuthenticatorstatus and handles errors appropriately.45-86: The
removeInjectedContainersfunction includes a check for the "sidecar" type and performs cleanup with appropriate error handling and logging.88-104: The
removeCleanupFinalizerfunction properly checks for and removes the finalizer from theBasicAuthenticatorobject, with appropriate error handling.107-152: Utility functions
getTargetDeployment,getTargetConfigmapNames, andgetTargetSecretNamecorrectly retrieve Kubernetes resources using label selectors and handle errors consistently.154-174: The
removeInjectedResourcesfunction's logic is sound for removing specific containers and volumes from deployments. The performance implications for large numbers of deployments have been acknowledged.177-183: The
existsInListfunction is implemented correctly for its purpose, and the performance impact is minimal with small lists.internal/controller/basic_authenticator/const.go (4)
26-28: The constants
StatusAvailable,StatusReconciling, andStatusDeletinghave been added. Verify that these constants are now being used in the codebase, as previous comments indicated they were not.6-6: The renaming of
SecretAnnotationandConfigmapAnnotationtobasicAuthenticatorNameLabelis mentioned, but the hunk does not show the old names being removed. Verify that the renaming is consistent across the codebase.14-19: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [14-25]
The AI-generated summary mentions the removal of the
resolverline within thetemplateconstant, but the hunk does not show this change. Verify if theresolverline has been removed and assess the impact on functionality.
- 6-6: The addition of
basicAuthenticatorNameLabelconstant is consistent with the PR objectives and AI-generated summary.internal/controller/basic_authenticator/provision.go (3)
13-29: The addition of
setReconcilingStatus,addCleanupFinalizer, andsetAvailableStatusto theProvisionmethod's subProvisioner slice aligns with the PR's objective to enhance cleanup and status management.40-53: The
setReconcilingStatusmethod correctly updates theBasicAuthenticatorstatus toStatusReconcilingand handles errors appropriately.256-269: The
setAvailableStatusmethod correctly updates theBasicAuthenticatorstatus toStatusAvailableand handles errors appropriately.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- internal/controller/basic_authenticator/cleanup.go (1 hunks)
- internal/controller/basic_authenticator/provision.go (8 hunks)
Additional comments: 16
internal/controller/basic_authenticator/cleanup.go (6)
16-31: The
Cleanupmethod is well-structured using sub-reconcilers to separate concerns. This should make the code easier to maintain and extend in the future.32-43: The
setDeletionStatusmethod correctly updates theBasicAuthenticatorstatus toStatusDeleting. Ensure that the status constants are correctly defined and used throughout the codebase.88-104: The
removeCleanupFinalizermethod's logic is sound, and it includes error handling. Verify that the finalizer name is consistent and correctly used throughout the codebase.107-152: The methods
getTargetDeployment,getTargetConfigmapNames, andgetTargetSecretNameuse label selectors to filter Kubernetes resources, which is a standard and efficient way to retrieve resources in a Kubernetes environment.154-174: The
removeInjectedResourcesmethod's logic for cleaning up deployments is correct. The previous discussion acknowledged the potential performance impact for large numbers of deployments, and it was decided to proceed with this approach.177-183: The
existsInListfunction is a simple linear search. The previous discussion concluded that it is acceptable for small lists, and no changes are required at this time.internal/controller/basic_authenticator/provision.go (10)
16-16: The import of
controllerutilis added to support the new finalizer logic. Ensure that this new dependency is properly managed and does not introduce any version conflicts.23-29: The
Provisionmethod now includes calls tosetReconcilingStatus,addCleanupFinalizer, andsetAvailableStatus. Ensure that these methods are called in the correct order and that their side effects are properly managed.40-53: The
setReconcilingStatusmethod updates the status of theBasicAuthenticatorobject. Ensure that the status is updated atomically and that any potential race conditions are handled.55-70: The
addCleanupFinalizermethod adds a finalizer to theBasicAuthenticatorobject. The logic here is consistent with the user's previous clarification that the reconciliation should be retried if the finalizer is not added (!objUpdated).89-89: The
ensureSecretmethod now returns aRequeueWithErrorresult instead of directly returning an error. This change aligns with the PR objectives to improve error handling.156-156: The
ensureConfigmapmethod has been updated to return aRequeueWithErrorresult, which is consistent with the improved error handling strategy.198-198: The
ensureDeploymentmethod has been updated to return aRequeueWithErrorresult, which is consistent with the improved error handling strategy.220-220: The
ensureServicemethod has been updated to return aRequeueWithErrorresult, which is consistent with the improved error handling strategy.256-269: The
setAvailableStatusmethod updates the status of theBasicAuthenticatorobject toStatusAvailable. Ensure that the status is updated atomically and that any potential race conditions are handled.341-344: The logic for adding a finalizer to the deployment object is consistent with the user's previous clarification that the reconciliation should be retried if the finalizer is not added (
!objUpdated).
| func (r *BasicAuthenticatorReconciler) removeInjectedContainers(ctx context.Context, req ctrl.Request) (*ctrl.Result, error) { | ||
| basicAuthenticator := &v1alpha1.BasicAuthenticator{} | ||
|
|
||
| if r, err := r.getLatestBasicAuthenticator(ctx, req, basicAuthenticator); subreconciler.ShouldHaltOrRequeue(r, err) { | ||
| return subreconciler.RequeueWithError(err) | ||
| } | ||
|
|
||
| if basicAuthenticator.Spec.Type != "sidecar" { | ||
| return subreconciler.ContinueReconciling() | ||
| } | ||
| basicAuthLabel := map[string]string{ | ||
| basicAuthenticatorNameLabel: basicAuthenticator.Name, | ||
| } | ||
| deployments, err := getTargetDeployment(ctx, basicAuthenticator, r.Client, basicAuthLabel) | ||
| if err != nil { | ||
| r.logger.Error(err, "failed to get target deployments to clean up") | ||
| return subreconciler.RequeueWithError(err) | ||
| } | ||
| configmaps, err := getTargetConfigmapNames(ctx, basicAuthenticator, r.Client, basicAuthLabel) | ||
| if err != nil { | ||
| r.logger.Error(err, "failed to get target configmap to clean up") | ||
| return subreconciler.RequeueWithError(err) | ||
| } | ||
| secrets, err := getTargetSecretName(ctx, basicAuthenticator, r.Client, basicAuthLabel) | ||
| if err != nil { | ||
| r.logger.Error(err, "failed to get target secret to clean up") | ||
| return subreconciler.RequeueWithError(err) | ||
| } | ||
| r.logger.Info("debug", "configmap", configmaps, "secret", secrets) | ||
|
|
||
| cleanupDeployments := removeInjectedResources(deployments, secrets, configmaps) | ||
| for _, deploy := range cleanupDeployments { | ||
| if controllerutil.ContainsFinalizer(deploy, basicAuthenticatorFinalizer) { | ||
| if objUpdated := controllerutil.RemoveFinalizer(deploy, basicAuthenticatorFinalizer); objUpdated { | ||
| if err := r.Update(ctx, deploy); err != nil { | ||
| r.logger.Error(err, "failed to add basicAuthenticator to injected deployment") | ||
| return subreconciler.RequeueWithError(err) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return subreconciler.ContinueReconciling() |
There was a problem hiding this comment.
The removeInjectedContainers method includes error handling and logging, which is consistent with the rest of the codebase. However, ensure that the error messages are descriptive and consistent with the project's logging standards.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- internal/controller/basic_authenticator/cleanup.go (1 hunks)
- internal/controller/basic_authenticator/provision.go (8 hunks)
Files skipped from review as they are similar to previous changes (1)
- internal/controller/basic_authenticator/cleanup.go
Additional comments: 5
internal/controller/basic_authenticator/provision.go (5)
13-16: The addition of the
controllerutilimport is appropriate for the new finalizer-related functionality.19-30: The
Provisionmethod has been updated to include new subreconcilers for setting reconciling and available statuses, as well as adding a cleanup finalizer. This aligns with the PR objectives to enhance the cleanup process.40-53: The
setReconcilingStatusmethod correctly updates the status of theBasicAuthenticatorobject toStatusReconciling. Ensure that theStatusReconcilingconstant is correctly defined and used throughout the codebase.55-69: The
addCleanupFinalizermethod is implemented according to the user's preference without an additional return statement after updating theBasicAuthenticatorobject, which is consistent with the learning provided.256-269: The
setAvailableStatusmethod correctly updates the status of theBasicAuthenticatorobject toStatusAvailable. Ensure that theStatusAvailableconstant is correctly defined and used throughout the codebase.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- tests/e2e/adaptive-scaling/00-install.yaml
- tests/e2e/sidecar/00-assert.yaml
Files selected for processing (1)
- internal/controller/basic_authenticator/provision.go (10 hunks)
Files skipped from review as they are similar to previous changes (1)
- internal/controller/basic_authenticator/provision.go
| if !controllerutil.ContainsFinalizer(basicAuthenticator, basicAuthenticatorFinalizer) { | ||
| if objUpdated := controllerutil.AddFinalizer(basicAuthenticator, basicAuthenticatorFinalizer); objUpdated { | ||
| if err := r.Update(ctx, basicAuthenticator); err != nil { | ||
| r.logger.Error(err, "failed to add basicAuthenticator finalizer") |
There was a problem hiding this comment.
Is the error message appropriate?
Summary by CodeRabbit
New Features
BasicAuthenticatorresources, including removal of injected resources and finalizers.BasicAuthenticatorobjects during reconciliation and deletion.Enhancements
Reconcilefunction forBasicAuthenticatorobjects.Refactor
Documentation