User description
What type of PR is this?
- [ ] API-change
- [ ] BUG
- [x] Improvement
- [ ] Documentation
- [ ] Feature
- [ ] Test and CI
- [ ] Code Refactoring
Which issue(s) this PR fixes:
Fixes # https://github.com/matrixorigin/MO-Cloud/issues/3875
What this PR does / why we need it:
Not Available
Special notes for your reviewer:
Not Available
Additional documentation (e.g. design docs, usage docs, etc.):
Not Available
PR Type
Enhancement, Configuration changes
Description
- Added support to watch resources only in the released namespace if
OnlyWatchReleasedNS is enabled.
- Introduced
cache.Options in the operator main to configure namespace-specific resource watching.
- Extended
OperatorConfig to include OnlyWatchReleasedNS and implemented GetRuntimeNS function.
- Updated Helm chart templates to conditionally create roles and bindings based on
onlyWatchReleasedNS.
- Added
POD_NAMESPACE environment variable to the deployment template.
- Removed the
watched_namespace template.
Changes walkthrough 📝
| Relevant files |
|---|
| Enhancement | 2 files
main.goAdd namespace-specific resource watching in operator main
cmd/operator/main.go
Added logic to watch resources only in the released namespace if
OnlyWatchReleasedNS is enabled.
Introduced cache.Options to configure namespace watching.
|
+9/-0 |
operatorcfg.goExtend OperatorConfig to support namespace-specific watching
pkg/controllers/common/operatorcfg.go
Added OnlyWatchReleasedNS field to OperatorConfig.
Implemented GetRuntimeNS function to fetch runtime namespace.
|
+18/-4 |
|
| Configuration changes | 8 files
cluster_role.yamlAdd ClusterRole template with conditional rules
charts/matrixone-operator/templates/cluster_role.yaml
Added ClusterRole template with conditional rules based on
onlyWatchReleasedNS.
|
+89/-0 |
cluster_role_binding.yamlAdd ClusterRoleBinding template
charts/matrixone-operator/templates/cluster_role_binding.yaml
- Added ClusterRoleBinding template.
|
+16/-0 |
configmap.yamlAdd onlyWatchReleasedNS to configmap template
charts/matrixone-operator/templates/configmap.yaml
- Added
onlyWatchReleasedNS to configmap template.
|
+2/-0 |
deployment.yamlAdd POD_NAMESPACE to deployment template
charts/matrixone-operator/templates/deployment.yaml
- Added
POD_NAMESPACE environment variable to deployment template.
|
+4/-0 |
role.yamlUpdate Role template for conditional namespace watching
charts/matrixone-operator/templates/role.yaml
Modified Role template to be conditional based on onlyWatchReleasedNS.
|
+2/-19 |
role_binding.yamlUpdate RoleBinding template for conditional namespace watching
charts/matrixone-operator/templates/role_binding.yaml
Modified RoleBinding template to be conditional based on
onlyWatchReleasedNS.
|
+3/-6 |
watched_namespace.yamlRemove watched_namespace template
charts/matrixone-operator/templates/watched_namespace.yaml
- Removed watched_namespace template.
|
+0/-21 |
values.yamlAdd onlyWatchReleasedNS configuration option
charts/matrixone-operator/values.yaml
- Added
onlyWatchReleasedNS configuration option.
|
+2/-2 |
|
💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions
PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.
PR Reviewer Guide 🔍
| ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪ |
| 🧪 No relevant tests |
| 🔒 No security concerns identified |
⚡ Key issues to review
Error Handling
The error from common.GetRuntimeNS() is not logged before exiting. Consider logging the error for better debugging and traceability.
Error Message Clarity
The error message "%s env must be set" could be more descriptive. Specify which environment variable is missing, e.g., "POD_NAMESPACE environment variable must be set."
|
PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.
PR Code Suggestions ✨
| Category | Suggestion | Score |
| Security |
Specify explicit verbs for resources in the cluster role to avoid granting excessive permissions
To ensure that the cluster role is correctly scoped and does not inadvertently grant
excessive permissions, explicitly define the verbs for each resource when
.Values.onlyWatchReleasedNS is false.
charts/matrixone-operator/templates/cluster_role.yaml [21-32]
- apiGroups:
- ""
resources:
- pods
- services
- configmaps
- secrets
- persistentvolumeclaims
- pods/status
- pods/exec
verbs:
- - '*'
+ - get
+ - list
+ - watch
+ - create
+ - update
+ - patch
+ - delete
- [ ] Apply this suggestion <!-- /improve --apply_suggestion=0 -->
Suggestion importance[1-10]: 10
Why: The suggestion addresses a significant security concern by explicitly defining the verbs for each resource, thereby preventing the cluster role from inadvertently granting excessive permissions.
| 10
|
| Enhancement |
Enhance error message clarity by providing more context when the namespace environment variable is not set
To improve error handling, consider wrapping the error message with additional
context when the namespace environment variable is not set, to aid in debugging.
pkg/controllers/common/operatorcfg.go [86-87]
if ns == "" {
- return "", fmt.Errorf("%s env must be set", PodNamespaceEnvKey)
+ return "", fmt.Errorf("environment variable %s must be set for runtime namespace", PodNamespaceEnvKey)
}
- [ ] Apply this suggestion <!-- /improve --apply_suggestion=1 -->
Suggestion importance[1-10]: 8
Why: The suggestion improves the clarity of the error message, making it easier to debug issues related to the namespace environment variable. This is a minor but useful enhancement.
| 8
|
| Maintainability |
Simplify the conditional check in the role template by removing the redundant check for .Values.rbac.create
To maintain consistency and clarity in the template, remove the redundant check for
.Values.rbac.create in the conditional that also checks .Values.onlyWatchReleasedNS.
charts/matrixone-operator/templates/role.yaml [1]
-{{- if and .Values.rbac.create .Values.onlyWatchReleasedNS }}
+{{- if .Values.onlyWatchReleasedNS }}
- [ ] Apply this suggestion <!-- /improve --apply_suggestion=2 -->
Suggestion importance[1-10]: 7
Why: The suggestion improves code maintainability by removing a redundant check, making the conditional statement simpler and clearer. This is a minor but beneficial improvement.
| 7
|
matrixorigin
![qodo-code-review[bot] avatar](https://avatars.githubusercontent.com/in/484649?v=4 "Published by a pub.dev verified publisher"){kind=small}