refactor: rework acl fetching for kubernetes and docker#1028
Conversation
📝 WalkthroughWalkthroughThe PR replaces domain-specific label retrieval with callback-based lookup. Access-control matching is centralized in ChangesAccess-control lookup refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AccessControlsService
participant LabelProvider
participant DockerService
participant KubernetesService
AccessControlsService->>AccessControlsService: Build domain and app-name locator
AccessControlsService->>LabelProvider: Lookup(locator)
LabelProvider->>DockerService: Scan decoded app labels
LabelProvider->>KubernetesService: Scan cached ingress entries
DockerService-->>AccessControlsService: Invoke locator for matching app
KubernetesService-->>AccessControlsService: Invoke locator for matching app
AccessControlsService-->>AccessControlsService: Return matched ACL app
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
dosubot
Bot
added
the
size:L
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![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
🧹 Nitpick comments (1)
internal/service/kubernetes_service.go (1)
114-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer an explicit local copy before taking the address of the loop value.
locatorreceives&entry.app, and the shared locator (getACLs) retains that pointer (nameMatch = app) while returningfalse, so the scan keeps iterating with a pointer to a loop-copy still held. This is safe under per-iteration loop variables, but the project prefers the explicit-copy pattern here.♻️ Proposed change
for _, entries := range k.ingressEntries { for _, entry := range entries { - if ok := locator(entry.name, &entry.app); ok { + app := entry.app + if ok := locator(entry.name, &app); ok { return } } }Based on learnings: when storing/returning a pointer to a
for rangeloop value, prefer the explicit local copy pattern rather than&<range-variable>directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/kubernetes_service.go` around lines 114 - 118, In the nested iteration over k.ingressEntries, create an explicit local copy of each entry before passing its app address to locator. Update the locator call to use the copied entry while preserving the existing early return behavior.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/service/docker_service.go`:
- Around line 70-100: Update DockerService.Lookup so inspectContainer and
DecodeLabels failures are handled per container: log or otherwise record each
error, then continue scanning remaining containers instead of returning
immediately. Keep getContainers errors and successful locator matching behavior
unchanged, and ensure malformed labels or unavailable containers do not prevent
ACL resolution for unrelated apps.
---
Nitpick comments:
In `@internal/service/kubernetes_service.go`:
- Around line 114-118: In the nested iteration over k.ingressEntries, create an
explicit local copy of each entry before passing its app address to locator.
Update the locator call to use the copied entry while preserving the existing
early return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 07e29bcf-4390-41ff-9794-cba7b8ac5636
📒 Files selected for processing (5)
internal/service/access_controls_service.gointernal/service/access_controls_service_test.gointernal/service/docker_service.gointernal/service/kubernetes_service.gointernal/service/kubernetes_service_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/service/docker_service.go (1)
94-95: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAvoid passing pointers to range loop variables.
Passing
&configdirectly to thelocatorcallback takes the address of the loop variable. Because the downstream callback stores this pointer, older Go versions (<1.22) will incorrectly reference the final iteration's value for all matched apps.Based on learnings, when you need to pass or store a pointer to a loop value, prefer creating an explicit local copy (e.g.,
cfg := config) rather than returning&<range-variable>directly, even if newer Go versions mitigate the address-taking risk.💡 Proposed fix
for app, config := range labels.Apps { - if ok := locator(app, &config); ok { + cfg := config + if ok := locator(app, &cfg); ok { return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/docker_service.go` around lines 94 - 95, Update the loop in the app-label processing flow to copy each range value into a distinct local variable before passing its address to locator. Replace the direct &config argument in the locator call while preserving the existing matching and callback behavior.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/service/docker_service.go`:
- Around line 94-95: Update the loop in the app-label processing flow to copy
each range value into a distinct local variable before passing its address to
locator. Replace the direct &config argument in the locator call while
preserving the existing matching and callback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 79a171c1-e519-486c-a661-4ecaa59a393e
📒 Files selected for processing (1)
internal/service/docker_service.go
2 participants
Summary by CodeRabbit
Improvements
Lookupbehavior for discovering matching apps.Bug Fixes
Tests