-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix kubernetes waiting containers cache index #60284
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This PR resolves an issue with the Kubernetes ephemeral containers cache used by Kubernetes agents, where the keys used in the index and get methods did not match. This mismatch caused the get method to fail with not found error, preventing the debug container from being created. Signed-off-by: Tiago Silva <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thought in the comments - I'll leave it up to you if you feel it's worthwhile.
lib/cache/kube.go
Outdated
map[kubeWaitingContainerIndex]func(*kubewaitingcontainerv1.KubernetesWaitingContainer) string{ | ||
kubeWaitingContainerNameIndex: func(u *kubewaitingcontainerv1.KubernetesWaitingContainer) string { | ||
return u.GetMetadata().GetName() | ||
return u.GetSpec().GetUsername() + "/" + u.GetSpec().GetCluster() + "/" + u.GetSpec().GetNamespace() + "/" + u.GetSpec().GetPodName() + "/" + u.GetMetadata().GetName() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This same construction is done in two other places - ListKubernetesWaitingContainers()
which is identical and GetKubernetesWaitingContainer()
which constructs the cache key from a request and not a resource.
At the very least, can this be factored out into a separate function that is used here and in ListKubernetesWaitingContainers()
? It would be nice if it could also be used by GetKubernetesWaitingContainer()
, which something like this poorly named code snippet does:
type kubernetesWaitingContainerCacheKeyFieldGetter interface {
GetUsername() string
GetCluster() string
GetNamespace() string
GetPodName() string
}
func kubernetesWaitingContainerCacheKey(c kubernetesWaitingContainerCacheKeyFieldGetter, containerName string) string {
return c.GetUsername() + "/" + c.GetCluster() + "/" + c.GetNamespace() + "/" + c.GetPodName() + "/" + containerName
}
This would help prevent them diverging in future causing this issue again.
Then again, perhaps with the tests there now, this isn't necessary - if they do diverge again, the tests should pick it up.
This PR resolves an issue with the Kubernetes ephemeral containers cache used by Kubernetes agents, where the keys used in the index and get methods did not match.
This mismatch caused the get method to fail with not found error, preventing the debug container from being created.
Fixes #60283
Changelog: Fixed an issue that caused Kubernetes debug containers to fail with a “container not valid” error when launched by a user requiring moderated sessions.