feat(ticdc): support dynamic secret syncer#6608
feat(ticdc): support dynamic secret syncer#6608ti-chi-bot[bot] merged 4 commits intopingcap:mainfrom
Conversation
Signed-off-by: liubo02 <liubo02@pingcap.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6608 +/- ##
==========================================
- Coverage 41.47% 41.21% -0.27%
==========================================
Files 349 350 +1
Lines 20043 20174 +131
==========================================
+ Hits 8313 8314 +1
- Misses 11730 11860 +130
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/test pull-e2e |
[LGTM Timeline notifier]Timeline:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fgksgf The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
This PR adds support for dynamic secret syncing in TiCDC by introducing a resource syncer sidecar container that can dynamically load Kubernetes secrets into TiCDC pods based on label selectors.
Key changes:
- Introduces a new
TiCDCDynamicSecretSyncerfeature flag (Alpha stage) that enables dynamic secret syncing for TiCDC pods - Adds a resource syncer sidecar container (as init container with
RestartPolicy: Always) that watches and syncs labeled secrets into pods via an EmptyDir volume - Implements RBAC resources (Role and RoleBinding) to grant TiCDC service accounts read access to secrets in their namespace
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/ticdc/ticdc.go | Reordered imports for consistency |
| tests/e2e/framework/desc/patch.go | Added TiCDCPatches helper function for test framework |
| tests/e2e/framework/action/create.go | Added MustCreateTiCDC test helper function |
| pkg/scheme/scheme.go | Added RBAC v1 API group to scheme for Role/RoleBinding support |
| pkg/image/image.go | Defined ResourceSyncer image constant |
| pkg/features/reload.go | Marked TiCDCDynamicSecretSyncer as unreloadable (requires pod restart) |
| pkg/controllers/ticdcgroup/tasks/util.go | Added RBACName helper function |
| pkg/controllers/ticdcgroup/tasks/rbac.go | New file implementing RBAC task to create Role and RoleBinding resources |
| pkg/controllers/ticdcgroup/builder.go | Integrated TaskRBAC into reconciliation pipeline |
| pkg/controllers/ticdc/tasks/pod_test.go | Updated tests to pass feature gates to pod generation |
| pkg/controllers/ticdc/tasks/pod.go | Implemented resource syncer container generation and conditional addition based on feature gate |
| manifests/crd/*.yaml | Added TiCDCDynamicSecretSyncer to feature enum across all CRDs |
| manifests/crd/core.pingcap.com_ticdcs.yaml | Added syncer field with image and labels configuration |
| manifests/crd/core.pingcap.com_ticdcgroups.yaml | Added syncer field to TiCDCGroup template spec |
| cmd/tidb-operator/main.go | Added resource syncer image flag and RBAC cache configuration |
| charts/tidb-operator/values.yaml | Added resourceSyncer image configuration |
| charts/tidb-operator/templates/rbac.yaml | Added permissions for roles and rolebindings resources |
| charts/tidb-operator/templates/deployment.yaml | Added resource syncer image argument to operator deployment |
| api/meta/v1alpha1/feature.go | Defined TiCDCDynamicSecretSyncer feature constant and stage |
| api/core/v1alpha1/zz_generated.deepcopy.go | Generated deepcopy methods for TiCDCSyncer |
| api/core/v1alpha1/ticdc_types.go | Added TiCDCSyncer type definition to API |
| api/core/v1alpha1/names.go | Added constants for resource syncer container and kube-resource volume |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Rules: []rbacv1.PolicyRule{ | ||
| { | ||
| Verbs: []string{ | ||
| "get", | ||
| "list", | ||
| "watch", | ||
| }, | ||
| APIGroups: []string{ | ||
| "", | ||
| }, | ||
| Resources: []string{ | ||
| "secrets", | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The RBAC Role grants unrestricted access to all secrets in the namespace with get, list, and watch permissions. While the resource syncer uses label selectors to filter which secrets to sync (as documented in the TiCDCSyncer API), this creates a security concern because the service account has broader permissions than necessary. Consider whether it's possible to restrict this to specific secret names or add documentation explaining why broad access is required. If label-based filtering is the intended approach, this broad permission might be necessary, but it should be clearly documented as a security consideration.
| } | ||
|
|
||
| func newRoleBinding(cg *v1alpha1.TiCDCGroup) *rbacv1.RoleBinding { | ||
| sa := "default" |
There was a problem hiding this comment.
The default service account "default" is hardcoded here when no custom service account is specified. Consider adding a comment explaining this default behavior and that it assumes the "default" service account exists in the namespace. This helps future maintainers understand the assumption being made.
Uh oh!
There was an error while loading. Please reload this page.