Fix namespaceByLabel when multiple namespaces have same label #523
Conversation
WalkthroughThe changes involve significant modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ReconcilerBase
participant NamespaceManager
participant PolicyManager
User->>ReconcilerBase: Call setPortsForRules(rules, skipObjNamespace)
ReconcilerBase->>NamespaceManager: Retrieve namespaces based on rules
NamespaceManager-->>ReconcilerBase: Return namespaceList
ReconcilerBase->>PolicyManager: Apply access policies for each namespace
PolicyManager-->>ReconcilerBase: Confirm policy application
ReconcilerBase-->>User: Return success or error
Poem
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
martinhny
force-pushed
the
fix_namespacebylabel_accesspolicy
branch
2 times, most recently
from
5e4e740 to
6f50d78
Compare
martinhny
force-pushed
the
fix_namespacebylabel_accesspolicy
branch
from
6f50d78 to
e721015
Compare
![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
Outside diff range, codebase verification and nitpick comments (1)
internal/controllers/common/reconciler.go (1)
215-215: Update function documentation to reflect parameter change.The parameter
namespacehas been replaced withskipObjNamespace. Ensure that the function documentation is updated to reflect this change.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- internal/controllers/common/reconciler.go (1 hunks)
- tests/application/access-policy/chainsaw-test.yaml (1 hunks)
- tests/application/access-policy/multiple-ns-same-label-assert.yaml (1 hunks)
- tests/application/access-policy/multiple-ns-same-label.yaml (1 hunks)
Additional comments not posted (14)
tests/application/access-policy/multiple-ns-same-label-assert.yaml (3)
1-4: LGTM!The metadata section is correctly defined.
5-35: LGTM!The spec section is correctly defined.
12-35: LGTM!The ingress and egress rules are correctly defined.
tests/application/access-policy/chainsaw-test.yaml (2)
Line range hint
1-8: LGTM!The metadata section is correctly defined.
Line range hint
9-44: LGTM!The spec section is correctly defined.
tests/application/access-policy/multiple-ns-same-label.yaml (5)
1-13: LGTM!The namespace definitions are correctly defined.
15-31: LGTM!The application definitions are correctly defined.
33-38: LGTM!The namespace definition is correctly defined.
40-46: LGTM!The application definition is correctly defined.
48-66: LGTM!The access policy definition is correctly defined.
internal/controllers/common/reconciler.go (4)
222-224: LGTM!The switch case for
rule.Namespaceis correctly implemented.
232-233: LGTM!The loop for appending namespaces based on labels is correctly implemented.
235-236: LGTM!The default case for appending
skipObjNamespaceis correctly implemented.
243-248: LGTM!The loop for retrieving application ports for each namespace is correctly implemented.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- internal/controllers/common/reconciler.go (1 hunks)
- tests/application/access-policy/multiple-ns-same-label-assert.yaml (1 hunks)
- tests/application/access-policy/multiple-ns-same-label.yaml (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- tests/application/access-policy/multiple-ns-same-label-assert.yaml
- tests/application/access-policy/multiple-ns-same-label.yaml
Additional comments not posted (6)
internal/controllers/common/reconciler.go (6)
215-215: Review the updated function signature.The function signature has been changed from using a single
namespaceparameter toskipObjNamespace. This change aligns with the PR's goal to handle multiple namespaces more effectively.The change is approved as it supports the intended functionality enhancement.
222-239: Enhanced namespace determination logic.The switch statement effectively handles different scenarios for namespace determination. It checks for direct namespace specification, namespaces by label, and defaults to
skipObjNamespaceif no other conditions are met.The logic is robust and aligns with the PR's objectives to handle multiple namespaces.
227-230: Proper error handling forLabelSelectorAsSelector.The error from
metav1.LabelSelectorAsSelectoris now correctly handled, which enhances the robustness of the function.The error handling improvement is a positive change.
242-243: Improve error message.The error message can be more informative by including the rule details.
- return fmt.Errorf("expected namespace, but found none for rule %s", rule.Application) + return fmt.Errorf("expected namespace, but found none for rule %s in application %s", rule, rule.Application)This suggestion has been made in a previous review and is still valid.
246-251: Review the logic for appending ports.The function iterates over each namespace in
namespaceListto retrieve and append application ports. This approach ensures that ports are set correctly for each namespace, supporting the functionality when multiple namespaces share the same label.The implementation is correct and supports the intended enhancements for handling multiple namespaces.
221-221: InitializenamespaceListwith a capacity.As previously suggested, initializing
namespaceListwith a capacity could improve performance if the number of namespaces can be estimated.Consider initializing
namespaceListwith a capacity to optimize memory allocation:- var namespaceList []string + namespaceList := make([]string, 0, estimatedCapacity)Likely invalid or redundant comment.
https://kartverketgroup.slack.com/archives/C028ZEED280/p1724920375968729
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests