- Notifications
You must be signed in to change notification settings - Fork6
Add support for multiple namespaces#107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
- Allow CODER_NAMESPACE to accept comma-separated list of namespaces- Support watching all namespaces when CODER_NAMESPACE is empty- Automatically use ClusterRole/ClusterRoleBinding for multi-namespace or all-namespace scenarios- Update Helm chart to support both namespace-scoped and cluster-wide RBAC- Add comprehensive documentation for multi-namespace usage- Maintain backward compatibility with single namespace deploymentsFixes#5
uvishere commentedJun 10, 2025
Hello, |
- Replace deprecated AgentLogWriter with agentsdk.Client and PatchLogs- Fix struct field names in tests (namespace -> namespaces)- Add missing isEmpty method to tokenCache- Update setPodToken and setReplicaSetToken to return values- Fix agentLog struct literals in tests- Add PatchLogs endpoint to fake API for tests- Update test expectations to match current log message formatCo-authored-by: kylecarbs <7122116+kylecarbs@users.noreply.github.com>
- Add proper context management to logger goroutines- Fix log level conversion in fake API PatchLogs method- Improve goroutine lifecycle with cancellation support- Use ticker-based approach for more reliable log sending- Continue on PatchLogs errors instead of exiting goroutineCo-authored-by: kylecarbs <7122116+kylecarbs@users.noreply.github.com>
- Fix Test_logQueuer to use real clock and proper timing- Add clock advances and sleeps to handle async log processing- Improve test reliability for TestPodEvents and TestReplicaSetEvents- Use quartz.NewTicker for proper mock clock integration- Simplify test expectations for better CI compatibilityCo-authored-by: kylecarbs <7122116+kylecarbs@users.noreply.github.com>
cc:@ericpaulsen |
uvishere commentedJun 19, 2025
Hey amazing people, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
This PR adds support for watching pods across multiple namespaces (including all namespaces) by treating theCODER_NAMESPACE environment variable or--namespaces flag as a comma-separated list. It also updates RBAC handling to switch between namespace-scoped Roles and cluster-wide ClusterRoles, and revises the Helm chart and documentation accordingly.
- Renamed the CLI flag from
--namespaceto--namespacesand updatednewPodEventLoggerto accept a list - Enhanced Helm chart to auto-detect cluster-wide RBAC based on
namespaces(empty or multiple) or explicit override - Added
parseNamespacesunit tests and updated documentation/README examples
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| main.go | Renamednamespace tonamespaces and updated Cobra flag and logger invocation |
| logger_test.go | Updated tests to usenamespaces, added clock advances, and newparseNamespaces tests |
| helm/values.yaml | Replacednamespace withnamespaces, introducedrbac.clusterWide |
| helm/templates/service.yaml | Added logic for selecting ClusterRole vs Role/RoleBinding and updated ServiceAccount deployment |
| README.md | Documented multi-namespace/all-namespace usage and updated badges |
Comments suppressed due to low confidence (4)
helm/templates/service.yaml:39
- Fix the YAML list formatting under 'subjects': ensure both 'kind' and 'name' entries are prefixed with '-' at the same indent level to produce valid YAML.
- kind: ServiceAccounthelm/templates/service.yaml:46
- [nitpick] Use a consistent resource naming convention by prefixing the namespace-scoped Role name with '{{ .Release.Name }}-' to match the ClusterRole naming pattern.
name: coder-logstream-kube-rolehelm/values.yaml:106
- [nitpick] Remove or clarify the commented-out legacy securityContext lines to keep the values.yaml file clean and maintainable.
# readOnlyRootFilesystem: truehelm/templates/service.yaml:1
- Consider adding unit tests for the Helm template logic that determines whether to create cluster-wide or namespace-scoped RBAC resources based on 'namespaces' and 'rbac.clusterWide'.
{{/*| @@ -41,13 +42,14 @@ func TestReplicaSetEvents(t *testing.T) { | |||
| agentURL,err:=url.Parse(api.server.URL) | |||
| require.NoError(t,err) | |||
| namespace:="test-namespace" | |||
| sourceUUID:=agentsdk.ExternalLogSourceID | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Remove the unused variable 'sourceUUID' or integrate it into the test; as-is it will cause a compilation error due to being unused.
| sourceUUID:=agentsdk.ExternalLogSourceID | |
| // Removed unused variable `sourceUUID` to prevent compilation error. |
Copilot uses AI. Check for mistakes.
- Add configurable tickInterval to logQueuer struct- Update Test_logQueuer to use real clock with fast tick interval (10ms)- Update TestPodEvents and TestReplicaSetEvents to use real clock- Replace mock clock advances with real time waits- Fix Test_logQueuer test which now passes consistentlyThe new ticker-based logging implementation requires different timingthan the original synchronous approach. Tests now use fast tickintervals (10ms) with real clocks instead of mock clocks to avoidtiming conflicts.Co-authored-by: matifali <10648092+matifali@users.noreply.github.com>
README.md Outdated
| [](https://discord.gg/coder) | ||
| [](https://github.com/coder/envbuilder/pkgs/container/coder-logstream-kube) | ||
| [](https://pkg.go.dev/github.com/coder/coder-logstream-kube) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Keep godoc and discord, and update the release link to the correct release page.
- Restore discord and godoc badges in README.md- Fix release link to point to correct repository releases page- Use consistent naming for namespace-scoped Role with release prefix- Clean up commented-out legacy securityContext lines in values.yamlThe sourceUUID variable is actually used in tests so no changes needed there.YAML formatting in service.yaml is already correct.Co-authored-by: matifali <10648092+matifali@users.noreply.github.com>
This PR addresses issue#5 by adding support for watching pods across multiple namespaces.
Changes
Core Functionality
CODER_NAMESPACEnow accepts comma-separated list of namespacesCODER_NAMESPACEwatches all namespaces cluster-wideRBAC Improvements
rbac.clusterWideoption for manual overrideHelm Chart Updates
namespacesparameter replacesnamespace(backward compatible)Documentation
Usage Examples
Single namespace (existing behavior):
helm install coder-logstream-kube coder-logstream-kube/coder-logstream-kube \ --set url=https://coder.example.com \ --set namespaces="coder-workspaces"Multiple namespaces:
helm install coder-logstream-kube coder-logstream-kube/coder-logstream-kube \ --set url=https://coder.example.com \ --set namespaces="user1,user2,user3"All namespaces:
helm install coder-logstream-kube coder-logstream-kube/coder-logstream-kube \ --set url=https://coder.example.com \ --set namespaces=""Testing
Fixes#5