feat(operator): manage namespace if not exist #1354
Conversation
WalkthroughThe update introduces a new configuration setting to control namespace creation in a component's operational logic. It modifies how namespaces are handled by adding a condition that checks a newly added configuration before creating or updating namespaces. Additionally, a function was renamed for clarity, and the logic for cleaning up resources now respects the new namespace creation setting. Changes
Warning Following errors were encountered
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 your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![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: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- components/operator/.earthly/configuration/templates/settings.yaml (1 hunks)
- components/operator/internal/resources/stacks/controller.go (4 hunks)
Additional comments: 6
components/operator/.earthly/configuration/templates/settings.yaml (1)
- 70-80: The addition of the
stacks.namespace.createsetting with a default value of "false" is correctly implemented and follows proper YAML syntax. This change aligns with the PR objectives to allow users to control the automatic creation of namespaces in stack operations.components/operator/internal/resources/stacks/controller.go (5)
- 12-12: The addition of the
settingsimport is correctly implemented and necessary for accessing the new configuration setting within the controller logic.- 112-112: Renaming
RetrieveReferenceModulestoretrieveReferenceModulescorrectly follows Go's naming conventions for unexported functions, enhancing code readability and maintainability.- 159-159: The introduction of the
autoCreateEnabledvariable to track the state of thestacks.namespace.createsetting is correctly implemented. Its usage in conditional logic for namespace creation aligns with the PR objectives.- 158-173: Updates to the
Reconcilemethod, including the conditional namespace creation logic based on theautoCreateEnabledvariable, are correctly implemented. This change aligns with the PR objectives to provide configurable namespace management.- 200-209: Updates to the
Cleanmethod, including the conditional logic for namespace deletion based on theautoCreateEnabledvariable, are correctly implemented. This ensures that namespaces are not deleted if the setting is disabled, aligning with the PR objectives.
Dav-14
force-pushed
the
feat/eng-862-operator-stack-deletion-is-removing-namespace
branch
from
9db8fe8
to
0a534ee
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- components/operator/api/formance.com/v1beta1/shared.go (1 hunks)
- components/operator/internal/resources/stacks/controller.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- components/operator/internal/resources/stacks/controller.go
Additional comments: 1
components/operator/api/formance.com/v1beta1/shared.go (1)
- 284-286: The addition of the
CreatedByAgentLabelconstant is consistent with the existing pattern of defining labels. This change is straightforward and aligns with the naming convention used for other labels in the file. However, it's important to ensure that this new label is utilized appropriately in the rest of the codebase, particularly in areas where resources are created by the agent.Ensure that the new label is applied to all resources created by the agent to maintain consistency and enable easy filtering and identification of such resources.
|
I know use Controller Reference which delete the namespace if added If already exist -> Do nothing |
Dav-14
force-pushed
the
feat/eng-862-operator-stack-deletion-is-removing-namespace
branch
from
0a534ee
to
b50e4c0
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- components/operator/api/formance.com/v1beta1/shared.go (1 hunks)
- components/operator/internal/resources/stacks/controller.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- components/operator/api/formance.com/v1beta1/shared.go
Additional comments: 6
components/operator/internal/resources/stacks/controller.go (6)
- 10-10: The addition of the
apierrorsimport is appropriate given the new logic in theReconcilefunction that checks for namespace existence and handles errors accordingly.- 114-114: Renaming
RetrieveReferenceModulestoretrieveReferenceModulesimproves consistency with Go naming conventions for unexported functions. Ensure that this change is reflected wherever the function is called.- 160-178: <!--
Monologue:
[task item_1/1] Investigate how configurations are loaded and managed in the project, focusing on the usage of Viper for configuration management.
[observations] The output shows that the project extensively uses Viper for configuration management across various components and modules. This confirms that Viper is a central part of the configuration system within the project. However, the specific setting 'stacks.namespace.create' was not found in the initial search, which could mean it's not directly referenced or it's managed under a different key.
[analysis] Given the extensive use of Viper, it's plausible that the configuration setting in question is indeed managed through Viper but under a different key or through a more complex configuration structure that wasn't captured by a direct string search.
[actions] To further investigate the specific configuration setting 'stacks.namespace.create', a more targeted approach is needed. Since direct searches did not yield results, consider searching for patterns that might indicate where and how namespace creation settings are managed. This could involve looking for generic terms related to namespace management or creation, or examining areas of the code that deal with namespace operations to see if they reference any configuration settings.Plan the next steps:
- Search for terms related to namespace management or creation to find potential references to the configuration setting.
- Examine code areas related to namespace operations for configuration references.
-->#!/bin/bash # Search for terms related to namespace management or creation to find potential references to the configuration setting. rg --type go 'namespace' | rg 'create|config'
- 181-185: Ensuring that dependent resources are ready before proceeding is a good practice. This logic appears to be correctly implemented and should help in maintaining the stability and consistency of stack operations.
- 160-178: While the namespace handling logic in
Reconcileis a significant addition, it's important to ensure that it aligns with the user's intention as specified by thestacks.namespace.createsetting. Consider adding a check for this setting before proceeding with namespace creation or update.- 160-178: The logic for handling namespace existence and creation by the agent is a key part of the new feature. However, it's crucial to ensure that this logic does not inadvertently lead to namespace deletion or modification in scenarios where it's not intended. Consider adding safeguards or additional checks to prevent such scenarios.
Verification successful
Given the information from the script output, which indicates that the RBAC permissions for the controller include 'delete' for namespaces, it's important to further investigate how these permissions are utilized within the application. To accurately assess the presence of safeguards against unintended namespace deletion or modification, a more targeted search for explicit deletion logic within the codebase is necessary.
Based on the absence of findings from the search for explicit namespace deletion logic within the Go files, there is no direct evidence of unintended namespace deletion or modification logic in the codebase. This suggests that the concerns raised in the review comment about the need for safeguards against unintended namespace deletion or modification are addressed by the current state of the code, as far as can be determined from the information available. However, it's important for developers to remain vigilant and consider the broader context and potential for indirect risks.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that safeguards are in place to prevent unintended namespace deletion or modification. rg --type go 'namespace' | grep -i 'delete'Length of output: 203
Script:
#!/bin/bash # Search for explicit namespace deletion logic within the Go files. rg --type go 'Delete.*namespace'Length of output: 34
| ns := &corev1.Namespace{} | ||
| ns.SetName(stack.Name) | ||
|
|
||
| exist := true | ||
| if err := ctx.GetClient().Get(ctx, types.NamespacedName{Name: stack.Name}, ns); err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| exist = false | ||
| } else { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| _, createdByAgent := stack.GetLabels()[v1beta1.CreatedByAgentLabel] | ||
| if !exist || createdByAgent { | ||
| if _, _, err := CreateOrUpdate(ctx, types.NamespacedName{ | ||
| Name: stack.Name, | ||
| }, core.WithController[*corev1.Namespace](ctx.GetScheme(), stack)); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
The introduction of the ns variable and the subsequent logic for namespace existence check and creation/update is a significant change. It's important to ensure that this logic is thoroughly tested, especially in edge cases such as API failures, concurrent operations, and permission issues.
Would you like me to help draft some unit tests for this new logic?
Dav-14
force-pushed
the
feat/eng-862-operator-stack-deletion-is-removing-namespace
branch
from
b50e4c0
to
7c0fb1d
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- components/operator/api/formance.com/v1beta1/shared.go (1 hunks)
- components/operator/internal/resources/stacks/controller.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- components/operator/api/formance.com/v1beta1/shared.go
Additional comments: 7
components/operator/internal/resources/stacks/controller.go (7)
- 5-5: The addition of the
errorspackage indicates new error handling logic is introduced. Ensure that this import is used effectively throughout the file and that custom error handling aligns with the overall error strategy of the application.- 113-113: Renaming
RetrieveReferenceModulestoretrieveReferenceModulesimproves consistency with Go naming conventions for unexported functions. This change should be cross-checked across the codebase to ensure all references to this function are updated accordingly.#!/bin/bash # Search for old function name usage across the codebase. Expecting no results. rg --type go 'RetrieveReferenceModules'
- 159-159: Introduction of
errAlreadyExistfor error handling in namespace creation logic is a good practice. It allows for specific error handling related to namespace existence. Ensure that this pattern is consistently used in similar contexts throughout the application.- 176-180: Calling
retrieveReferenceModulesandareDependentReadysequentially in theReconcilemethod ensures that the stack's modules are retrieved and that all dependencies are ready before proceeding. This structured approach aids in maintaining the integrity of stack operations. Consider adding error handling documentation or inline comments for clarity on how errors from these calls are expected to be handled.- 159-180: The changes in the
Reconcilemethod introduce significant logic for namespace management based on the new configuration setting. Ensure that the logic aligns with the overall architecture and error handling strategy of the application. Additionally, verify that the changes do not introduce any regressions or unintended side effects in stack operations.#!/bin/bash # Verify that the new logic does not conflict with existing stack operations. # This script is a placeholder for specific verification steps that may include running unit tests, integration tests, or manual testing procedures. echo "Placeholder for verification steps."
- 180-180: The final return statement in the
Reconcilemethod signifies the successful completion of the reconciliation process. It's important to ensure that all preceding operations, including error handling and dependency checks, are correctly implemented to avoid false positives of successful reconciliation.- 180-180: In the
Cleanmethod, the logic for removing dependencies and the namespace is updated to handle conditions based on the new configuration setting. This update is crucial for ensuring that namespace deletion is handled correctly and aligns with user expectations. Review the logic to ensure it properly accounts for all scenarios, including error handling and rollback mechanisms in case of failures.#!/bin/bash # Verify that the updated Clean method correctly handles all scenarios for namespace deletion. # This script is a placeholder for specific verification steps that may include running unit tests, integration tests, or manual testing procedures. echo "Placeholder for verification steps."
| errAlreadyExist := errors.New("namespace already exists") | ||
| if _, _, err := CreateOrUpdate(ctx, types.NamespacedName{ | ||
| Name: stack.Name, | ||
| }) | ||
| if err != nil { | ||
| return err | ||
| }, | ||
| func(ns *corev1.Namespace) error { | ||
| _, stackCreatedByAgent := stack.GetLabels()[v1beta1.CreatedByAgentLabel] | ||
| if ns.ResourceVersion == "" || stackCreatedByAgent { | ||
| return nil | ||
| } | ||
|
|
||
| return errAlreadyExist | ||
| }, core.WithController[*corev1.Namespace](ctx.GetScheme(), stack)); err != nil { | ||
| if !errors.Is(err, errAlreadyExist) { | ||
| return err | ||
| } |
There was a problem hiding this comment.
The logic within Reconcile for handling namespace creation and update conditions is crucial. It's important to ensure comprehensive testing, especially for edge cases such as API failures, concurrent operations, and permission issues. The use of errAlreadyExist to differentiate between an actual error and a namespace already existing scenario is a good approach.
Would you like me to help draft some unit tests for this new logic?
| return err | ||
| } | ||
|
|
||
| err = areDependentReady(ctx, stack) | ||
| if err != nil { | ||
| if err := areDependentReady(ctx, stack); err != nil { |
There was a problem hiding this comment.
The areDependentReady function call within Reconcile is critical for ensuring that all dependencies of a stack are ready before proceeding. This check is essential for maintaining the stability and reliability of stack operations. Ensure that this function is robustly tested, particularly in scenarios with complex dependency graphs.
Would you like assistance in creating additional tests for complex dependency scenarios?
Dav-14
changed the title
Dav-14
deleted the
feat/eng-862-operator-stack-deletion-is-removing-namespace
branch
Summary by CodeRabbit