add support for egress control injection#397
Conversation
|
[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 |
|
Welcome @Kuromesi! It looks like this is your first PR to openkruise/agents 🎉 |
| } | ||
|
|
||
| // NewRuntimeController creates an Injector that watches the sandbox-injection-config | ||
| func NewRuntimeController(cfg *rest.Config) (*Injector, error) { |
There was a problem hiding this comment.
plz use informer cache based controller in controller-runtime, not directly with client-go and shared informer. Actually, is it really necessary to use a controller loop ? consider fetch config directly from informer cache, and cache the parsed config values
| AddFunc: func(obj interface{}) { | ||
| cm := obj.(*corev1.ConfigMap) | ||
| if cm.Name == SandboxInjectionConfigName && namespace == cm.Namespace { | ||
| i.onConfigChange(cm) |
There was a problem hiding this comment.
it is bad practice to put heavy operation(lock in onConfigChange) in event handler, which will block watch operations
| Sidecars []corev1.Container `json:"csiSidecar" yaml:"csiSidecar"` | ||
| // Support injection for volume mount configurations | ||
| Volumes []corev1.Volume `json:"volume"` | ||
| Volume []corev1.Volume `json:"volume" yaml:"volume"` |
There was a problem hiding this comment.
plz keep the field and json name as Volumes
There was a problem hiding this comment.
Json tag is not changed, only the go struct filed is changed, the go struct field and json tag is not consistent now.
| // Template is a raw Pod spec overlay used by the controller-runtime Injector | ||
| // for unknown runtime types (e.g. egress-control). It is applied via strategic | ||
| // merge patch after container conflict checks. | ||
| Template json.RawMessage `json:"template" yaml:"template"` |
There was a problem hiding this comment.
template patch is too free form, consider using the injection logic similar to csi/agent-runtime
| } | ||
|
|
||
| // patchRewriteProbe generates the patch for webhook. | ||
| func patchRewriteProbe(annotations map[string]string, pod *corev1.Pod, defaultPort int32) { |
There was a problem hiding this comment.
patchRewriteProbe accepts annotations parameter but never uses it
| } | ||
|
|
||
| // kubeLifecycleHandlerToInternalProber converts a Kubernetes LifecycleHandler to an Istio internal Prober | ||
| func kubeLifecycleHandlerToInternalProber(lifecycelHandler *corev1.LifecycleHandler) *Prober { |
There was a problem hiding this comment.
lifecycelHandler -> lifecycleHandler
| // templatePod is the parsed form of Template, set during parsing. | ||
| templatePod *corev1.Pod | ||
| // raw caches the original config string for change detection. | ||
| raw string |
There was a problem hiding this comment.
SidecarInjectConfig mixes public API and internal cache, consider using configmap generation for revision detection
|
|
||
| err := json.Unmarshal([]byte(configRaw[configKey]), &sidecarConfig) | ||
| var err error | ||
| if strings.HasPrefix(configValue, "{") || strings.HasPrefix(configValue, "[") { |
There was a problem hiding this comment.
The heuristic of checking the first character to decide JSON vs YAML is fragile. A YAML file could start with { (flow style). The parseConfigValue function in injector.go always uses yaml.Unmarshal (which also handles JSON), making the JSON-detection logic in parseInjectConfig redundant and inconsistent. Consider unifying both paths to always use json
Signed-off-by: Kuromesi <blackfacepan@163.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #397 +/- ##
==========================================
+ Coverage 76.51% 76.74% +0.22%
==========================================
Files 146 151 +5
Lines 10684 11012 +328
==========================================
+ Hits 8175 8451 +276
- Misses 2168 2204 +36
- Partials 341 357 +16
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:
|
3 participants
Ⅰ. Describe what this PR does
Add egress-control sidecar injection with automatic health probe rewriting via a new controller-runtime managed Injector, replacing the old inline injection path.
Support strategic merge patch runtime injection, allow us to inject with more flexibility and extensibility.
Ⅱ. Does this pull request fix one issue?
Ⅲ. Describe how to verify it
Ⅳ. Special notes for reviews