Skip to content

Commit

Permalink
[kube] fix data race caused by returning pointer to struct value (#46711
Browse files Browse the repository at this point in the history
)

This PR fixes a data race caused `getClusterSupportedResources` returning a pointer to a struct
value.

Teleport didn't had the race because it was under RWMutex but kubernetes code ended up reading from it while handling the unmarshal during parallel requests.

Signed-off-by: Tiago Silva <tiago.silva@goteleport.com>

use deepcopy of static kube objects
  • Loading branch information
tigrato authored Sep 24, 2024
1 parent 3b80232 commit 9117ee3
Show file tree
Hide file tree
Showing 5 changed files with 9 additions and 9 deletions.
4 changes: 2 additions & 2 deletions lib/kube/proxy/cluster_details.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ type kubeDetails struct {
// that are supported by the cluster.
// The codec factory is updated periodically to include the latest custom resources
// that are added to the cluster.
kubeCodecs serializer.CodecFactory
kubeCodecs *serializer.CodecFactory
// rbacSupportedTypes is the list of supported types for RBAC for the cluster.
// The list is updated periodically to include the latest custom resources
// that are added to the cluster.
Expand Down Expand Up @@ -245,7 +245,7 @@ func (k *kubeDetails) getClusterSupportedResources() (*serializer.CodecFactory,
if k.isClusterOffline {
return nil, nil, trace.ConnectionProblem(nil, "kubernetes cluster %q is offline", k.kubeCluster.GetName())
}
return &(k.kubeCodecs), k.rbacSupportedTypes, nil
return k.kubeCodecs, k.rbacSupportedTypes, nil
}

// getObjectGVK returns the default GVK (if any) registered for the specified request path.
Expand Down
8 changes: 4 additions & 4 deletions lib/kube/proxy/scheme.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,13 +113,13 @@ type gvkSupportedResources map[gvkSupportedResourcesKey]*schema.GroupVersionKind
// This schema includes all well-known Kubernetes types and all namespaced
// custom resources.
// It also returns a map of resources that we support RBAC restrictions for.
func newClusterSchemaBuilder(log logrus.FieldLogger, client kubernetes.Interface) (serializer.CodecFactory, rbacSupportedResources, gvkSupportedResources, error) {
func newClusterSchemaBuilder(log logrus.FieldLogger, client kubernetes.Interface) (*serializer.CodecFactory, rbacSupportedResources, gvkSupportedResources, error) {
kubeScheme := runtime.NewScheme()
kubeCodecs := serializer.NewCodecFactory(kubeScheme)
supportedResources := maps.Clone(defaultRBACResources)
gvkSupportedRes := make(gvkSupportedResources)
if err := registerDefaultKubeTypes(kubeScheme); err != nil {
return serializer.CodecFactory{}, nil, nil, trace.Wrap(err)
return nil, nil, nil, trace.Wrap(err)
}
// discoveryErr is returned when the discovery of one or more API groups fails.
var discoveryErr *discovery.ErrGroupDiscoveryFailed
Expand All @@ -137,7 +137,7 @@ func newClusterSchemaBuilder(log logrus.FieldLogger, client kubernetes.Interface
// available in the cluster.
log.WithError(err).Debugf("Failed to discover some API groups: %v", maps.Keys(discoveryErr.Groups))
case err != nil:
return serializer.CodecFactory{}, nil, nil, trace.Wrap(err)
return nil, nil, nil, trace.Wrap(err)
}

for _, apiGroup := range apiGroups {
Expand Down Expand Up @@ -201,7 +201,7 @@ func newClusterSchemaBuilder(log logrus.FieldLogger, client kubernetes.Interface
}
}

return kubeCodecs, supportedResources, gvkSupportedRes, nil
return &kubeCodecs, supportedResources, gvkSupportedRes, nil
}

// getKubeAPIGroupAndVersion returns the API group and version from the given
Expand Down
2 changes: 1 addition & 1 deletion lib/kube/proxy/testing/kube_server/kube_mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ func (s *KubeMockServer) exec(w http.ResponseWriter, req *http.Request, p httpro

q := req.URL.Query()
if s.execPodError != nil {
s.writeResponseError(w, nil, s.execPodError)
s.writeResponseError(w, nil, s.execPodError.DeepCopy())
return nil, nil
}
request := remoteCommandRequest{
Expand Down
2 changes: 1 addition & 1 deletion lib/kube/proxy/testing/kube_server/pods.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ func (s *KubeMockServer) listPods(w http.ResponseWriter, req *http.Request, p ht

func (s *KubeMockServer) getPod(w http.ResponseWriter, req *http.Request, p httprouter.Params) (any, error) {
if s.getPodError != nil {
s.writeResponseError(w, nil, s.getPodError)
s.writeResponseError(w, nil, s.getPodError.DeepCopy())
return nil, nil
}
namespace := p.ByName("namespace")
Expand Down
2 changes: 1 addition & 1 deletion lib/kube/proxy/url_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ func Test_getResourceFromRequest(t *testing.T) {
verb = http.MethodPost
}
got, _, err := getResourceFromRequest(&http.Request{Method: verb, URL: &url.URL{Path: tt.path}, Body: tt.body}, &kubeDetails{
kubeCodecs: globalKubeCodecs,
kubeCodecs: &globalKubeCodecs,
rbacSupportedTypes: defaultRBACResources,
gvkSupportedResources: map[gvkSupportedResourcesKey]*schema.GroupVersionKind{
{
Expand Down

0 comments on commit 9117ee3

Please sign in to comment.