Feature cms api operator#2463
Conversation
Signed-off-by: chrisdeng <chrisdeng@futunn.com>
Signed-off-by: chrisdeng <chrisdeng@futunn.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found 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 introduces a new ConfigMapSet API + controller + webhooks to manage configuration rollouts to Pods, including Pod-side injection of a reload sidecar and update coordination via annotations.
Changes:
- Add
ConfigMapSetCRD/API types, controller reconciliation logic, and RBAC wiring. - Add mutating/validating admission webhooks for
ConfigMapSet, plus Pod mutation logic to inject volumes/sidecar and initialize revision annotations. - Generate/update clientset/informers/listers and add unit tests around conflict detection and validation behavior.
Reviewed changes
Copilot reviewed 21 out of 33 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/webhook/pod/mutating/pod_create_update_handler.go | Inject ConfigMapSet volumes/sidecar and set revision annotations on Pod create |
| pkg/webhook/pod/mutating/configmapset_mutating_test.go | Unit tests for Pod-side conflict/name logic and annotation initialization |
| pkg/webhook/configmapset/validating/webhooks.go | Register ConfigMapSet validating webhook |
| pkg/webhook/configmapset/validating/configmapset_create_update_handler.go | Validate ConfigMapSet spec, conflicts, history limit semantics |
| pkg/webhook/configmapset/validating/configmapset_create_update_handler_test.go | Unit tests for ConfigMapSet validating webhook logic |
| pkg/webhook/configmapset/validating/configmapset_conflicts.go | Cross-ConfigMapSet conflict detection (overlapping selectors) |
| pkg/webhook/configmapset/mutating/webhooks.go | Register ConfigMapSet mutating webhook |
| pkg/webhook/configmapset/mutating/configmapset_create_update_handler.go | Defaulting webhook for ConfigMapSet |
| pkg/webhook/add_configmapset.go | Wire ConfigMapSet webhooks into webhook server |
| pkg/controller/controllers.go | Register ConfigMapSet controller with manager |
| pkg/controller/configmapset/configmapset_utils.go | Shared helpers for annotation keys, pod matching, grouping |
| pkg/controller/configmapset/configmapset_utils_test.go | Tests for utils helpers (pod matching, readiness, CMS matching) |
| pkg/controller/configmapset/configmapset_event_handler.go | Event handlers for ConfigMapSet/Pod enqueueing |
| pkg/controller/configmapset/configmapset_controller.go | Core controller reconcile loop, revision hub management, pod update strategy |
| pkg/controller/configmapset/configmapset_controller_test.go | Tests for hash, distribution logic, pod update prioritization, container name resolution |
| pkg/client/listers/apps/v1alpha1/expansion_generated.go | Add ConfigMapSet lister expansions |
| pkg/client/listers/apps/v1alpha1/configmapset.go | Generated ConfigMapSet lister |
| pkg/client/informers/externalversions/generic.go | Wire generic informer lookup for ConfigMapSets |
| pkg/client/informers/externalversions/apps/v1alpha1/interface.go | Add ConfigMapSets informer accessor |
| pkg/client/informers/externalversions/apps/v1alpha1/configmapset.go | Generated ConfigMapSet informer |
| pkg/client/clientset/versioned/typed/apps/v1alpha1/generated_expansion.go | Add ConfigMapSet client expansion hook |
| pkg/client/clientset/versioned/typed/apps/v1alpha1/fake/fake_configmapset.go | Generated fake ConfigMapSet client |
| pkg/client/clientset/versioned/typed/apps/v1alpha1/fake/fake_apps_client.go | Expose fake ConfigMapSets() accessor |
| pkg/client/clientset/versioned/typed/apps/v1alpha1/configmapset.go | Generated typed ConfigMapSet client |
| pkg/client/clientset/versioned/typed/apps/v1alpha1/apps_client.go | Expose ConfigMapSets() accessor |
| apis/apps/v1alpha1/configmapset_types.go | New ConfigMapSet API types |
| apis/apps/v1alpha1/zz_generated.deepcopy.go | Generated deepcopy for new API types |
| apis/apps/defaults/v1alpha1.go | Add ConfigMapSet defaulting |
| config/crd/bases/apps.kruise.io_configmapsets.yaml | Add ConfigMapSet CRD manifest |
| config/webhook/manifests.yaml | Register ConfigMapSet mutating/validating webhooks |
| config/rbac/role.yaml | Grant RBAC for configmapsets + status |
| go.mod | Add indirect deps needed by tests |
| go.sum | Lock new indirect deps |
Files not reviewed (11)
- apis/apps/v1alpha1/zz_generated.deepcopy.go: Language not supported
- pkg/client/clientset/versioned/typed/apps/v1alpha1/apps_client.go: Language not supported
- pkg/client/clientset/versioned/typed/apps/v1alpha1/configmapset.go: Language not supported
- pkg/client/clientset/versioned/typed/apps/v1alpha1/fake/fake_apps_client.go: Language not supported
- pkg/client/clientset/versioned/typed/apps/v1alpha1/fake/fake_configmapset.go: Language not supported
- pkg/client/clientset/versioned/typed/apps/v1alpha1/generated_expansion.go: Language not supported
- pkg/client/informers/externalversions/apps/v1alpha1/configmapset.go: Language not supported
- pkg/client/informers/externalversions/apps/v1alpha1/interface.go: Language not supported
- pkg/client/informers/externalversions/generic.go: Language not supported
- pkg/client/listers/apps/v1alpha1/configmapset.go: Language not supported
- pkg/client/listers/apps/v1alpha1/expansion_generated.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2463 +/- ##
==========================================
- Coverage 48.77% 47.64% -1.14%
==========================================
Files 324 334 +10
Lines 27928 29701 +1773
==========================================
+ Hits 13623 14151 +528
- Misses 12775 13942 +1167
- Partials 1530 1608 +78
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: chrisdeng <chrisdeng@futunn.com>
Signed-off-by: chrisdeng <chrisdeng@futunn.com>
Signed-off-by: chrisdeng <chrisdeng@futunn.com>
Signed-off-by: chrisdeng <chrisdeng@futunn.com>
Signed-off-by: chrisdeng <chrisdeng@futunn.com>
Signed-off-by: chrisdeng <chrisdeng@futunn.com>
Signed-off-by: chrisdeng <chrisdeng@futunn.com>
Signed-off-by: chrisdeng <chrisdeng@futunn.com>
Signed-off-by: chrisdeng <chrisdeng@futunn.com>
Signed-off-by: chrisdeng <chrisdeng@futunn.com>
ABNER-1
left a comment
ABNER-1
left a comment
There was a problem hiding this comment.
Re-review (2026-05-25)
Thanks for the fixes! I can see the following issues from the previous review have been addressed:
- ✅
md5.New().Sum()→ fixed tomd5.Sum() - ✅
GetMatchedPodsnil check order → fixed - ✅
SetDefaultsConfigMapSetnil pointer → fixed - ✅ Selector/Data now enforced as required in validating webhook
- ✅ Health check script extracted to a helper function with proper path parameterization
However, a few issues remain — one Critical and several Medium/Low items.
| } | ||
| for _, pod := range pods.Items { | ||
| q.Add(reconcile.Request{NamespacedName: types.NamespacedName{Namespace: pod.Namespace, Name: pod.Name}}) | ||
| } |
There was a problem hiding this comment.
[Critical] Event handler enqueues the wrong resource.
configMapSetEventHandler.handle() adds Pod NamespacedNames to the work queue, but Reconcile() expects to receive ConfigMapSet NamespacedNames. This means ConfigMapSet spec changes (create/update/delete) will never trigger reconciliation of the ConfigMapSet itself.
The podEventHandler.handle() correctly enqueues CMS names (line 120), so this handler should simply enqueue the CMS:
func (e *configMapSetEventHandler) handle(cms *appsv1alpha1.ConfigMapSet, q workqueue.TypedRateLimitingInterface[reconcile.Request]) {
q.Add(reconcile.Request{NamespacedName: types.NamespacedName{Namespace: cms.Namespace, Name: cms.Name}})
}Note: since you're already watching ConfigMapSet with handler.TypedEnqueueRequestForObject (in add()), this custom configMapSetEventHandler may actually be redundant — the built-in handler already enqueues the CMS on create/update/delete. You could consider removing configMapSetEventHandler entirely and relying on the built-in watcher.
| } | ||
| return fmt.Errorf("create ConfigMap %s/%s: %v", cmNamespace, cmName, err) | ||
| } | ||
| return fmt.Errorf("failed to get ConfigMap: %v", err) |
There was a problem hiding this comment.
[High] Error returned on successful creation.
After successfully creating the hub ConfigMap, this line returns:
return fmt.Errorf("create ConfigMap %s/%s: %v", cmNamespace, cmName, err)At this point err is nil (the Create call succeeded). This causes RetryOnConflict to unnecessarily retry.
Suggested fix — return a sentinel or restructure:
if err = r.Create(ctx, newCM); err != nil {
return fmt.Errorf("failed to create ConfigMap %s/%s: %v", cmNamespace, cmName, err)
}
// ConfigMap just created, re-fetch to continue with revision append
cm = newCM| "k8s.io/utils/ptr" | ||
|
|
||
| corev1 "k8s.io/api/core/v1" | ||
| v1 "k8s.io/api/core/v1" |
There was a problem hiding this comment.
[Medium] Duplicate import aliases.
Both corev1 "k8s.io/api/core/v1" and v1 "k8s.io/api/core/v1" are imported. Please unify to a single alias — recommend corev1 per project convention.
| "k8s.io/klog/v2" | ||
| "k8s.io/kubernetes/pkg/controller" | ||
| "k8s.io/kubernetes/pkg/fieldpath" | ||
| "k8s.io/utils/pointer" |
There was a problem hiding this comment.
[Medium] Deprecated + redundant package.
"k8s.io/utils/pointer" (line 49) is deprecated and you already import "k8s.io/utils/ptr" (line 34). Please remove the pointer import and use ptr.To() throughout.
| func (p *podEventHandler) Delete(ctx context.Context, evt event.TypedDeleteEvent[*v1.Pod], q workqueue.TypedRateLimitingInterface[reconcile.Request]) { | ||
| //obj := evt.Object.(*corev1.Pod) | ||
| //e.handle(obj, q) | ||
| } |
There was a problem hiding this comment.
[Low] Dead code.
The Delete handler body is commented out. Either implement the delete handling logic (e.g., enqueue the CMS for status update when a matched Pod is deleted) or remove the commented code to keep the codebase clean.
| "k8s.io/client-go/util/retry" | ||
| "k8s.io/klog/v2" | ||
| "k8s.io/kubernetes/pkg/controller" | ||
| "k8s.io/kubernetes/pkg/fieldpath" |
There was a problem hiding this comment.
[Low] Potentially unused import.
"k8s.io/kubernetes/pkg/fieldpath" — is this actually used? If not, please remove to pass go vet.
|
@a932846905: PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Signed-off-by: chrisdeng <chrisdeng@futunn.com>
Signed-off-by: chrisdeng <chrisdeng@futunn.com>
Signed-off-by: chrisdeng <chrisdeng@futunn.com>
Signed-off-by: chrisdeng <chrisdeng@futunn.com>
Signed-off-by: chrisdeng <chrisdeng@futunn.com>
Signed-off-by: chrisdeng <chrisdeng@futunn.com>
Ⅰ. Describe what this PR does
Ⅱ. Does this pull request fix one issue?
Ⅲ. Describe how to verify it
Ⅳ. Special notes for reviews