Fix downward api for status fields in k3k-kubelet #185
Conversation
galal-hussein
commented
Jan 15, 2025
galal-hussein
commented
- Pods are not registering with k3k-kubelet if they have a fieldpath status.podIP #184
Signed-off-by: galal-hussein <[email protected]>
| // look for status.* fields in the env | ||
| if pod.Annotations == nil { | ||
| pod.Annotations = make(map[string]string) | ||
| } | ||
| for i, container := range pod.Spec.Containers { | ||
| for j, env := range container.Env { | ||
| if env.ValueFrom != nil { | ||
| if env.ValueFrom.FieldRef != nil { | ||
| if strings.Contains(env.ValueFrom.FieldRef.FieldPath, "status.") { | ||
| pod.Annotations[FieldpathField+"_"+strconv.Itoa(i)+"_"+env.Name] = env.ValueFrom.FieldRef.FieldPath | ||
| pod.Spec.Containers[i].Env = removeEnv(pod.Spec.Containers[i].Env, j) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
When possible I try to avoid too many nested ifs, what about a small rewrite like:
| // look for status.* fields in the env | |
| if pod.Annotations == nil { | |
| pod.Annotations = make(map[string]string) | |
| } | |
| for i, container := range pod.Spec.Containers { | |
| for j, env := range container.Env { | |
| if env.ValueFrom != nil { | |
| if env.ValueFrom.FieldRef != nil { | |
| if strings.Contains(env.ValueFrom.FieldRef.FieldPath, "status.") { | |
| pod.Annotations[FieldpathField+"_"+strconv.Itoa(i)+"_"+env.Name] = env.ValueFrom.FieldRef.FieldPath | |
| pod.Spec.Containers[i].Env = removeEnv(pod.Spec.Containers[i].Env, j) | |
| } | |
| } | |
| } | |
| } | |
| } | |
| // look for status.* fields in the env | |
| if pod.Annotations == nil { | |
| pod.Annotations = make(map[string]string) | |
| } | |
| for i, container := range pod.Spec.Containers { | |
| for j, env := range container.Env { | |
| if env.ValueFrom == nil || env.ValueFrom.FieldRef == nil { | |
| continue | |
| } | |
| fieldPath := env.ValueFrom.FieldRef.FieldPath | |
| if strings.Contains(fieldPath, "status.") { | |
| annotationKey := fmt.Sprintf("%s_%d_%s", FieldpathField, i, env.Name) | |
| pod.Annotations[annotationKey] = fieldPath | |
| pod.Spec.Containers[i].Env = removeEnv(pod.Spec.Containers[i].Env, j) | |
| } | |
| } | |
| } |
Also because of the strconv and all the params I find cleaner to use a fmt.Sprintf.
I would also move this logic in a separate func, with a specific name and a small comment on why it's needed. Something like cleanupFieldRef.
k3k-kubelet/provider/provider.go
Outdated
| s := strings.SplitN(name, "_", 3) | ||
| if len(s) != 3 { | ||
| continue | ||
| } | ||
| containerIndex, err = strconv.Atoi(s[1]) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| envName = s[2] |
There was a problem hiding this comment.
I wouldn't try to spread outside this logic, it looks part of the webhook fieldRef stuff. Would it make sense to have inside the webhook package two func to handle this?
Something like:
func FormatFieldPathAnnotationKey() {
// formatting logic here
}
func ParseFieldPathAnnotationKey(annotationKey string) (name string, containerIndex int, envName string, error) {
// parse logic here
}
The cleanest way would be have a FieldPathAnnotationKey type, but this is up to you. If it's used only here it's fine enough.
There was a problem hiding this comment.
ah okay, I can add the logic in the webhook package
| } | ||
| envName = s[2] | ||
|
|
||
| // re-adding these envs to the pod |
There was a problem hiding this comment.
After readding them, should we cleanup the annotations as well? Do we need them anymore?
There was a problem hiding this comment.
I will remove them from the pods on the host cluster, removing them after they are created on the virtual cluster will require a new controller which I think its an overkill
Thanks I will address the comments today |
Signed-off-by: galal-hussein <[email protected]>
