From 9117ee31bf55e13ae3627633ad334e9b6427e3fc Mon Sep 17 00:00:00 2001 From: Tiago Silva Date: Tue, 24 Sep 2024 16:45:04 +0100 Subject: [PATCH] [kube] fix data race caused by returning pointer to struct value (#46711) 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 use deepcopy of static kube objects --- lib/kube/proxy/cluster_details.go | 4 ++-- lib/kube/proxy/scheme.go | 8 ++++---- lib/kube/proxy/testing/kube_server/kube_mock.go | 2 +- lib/kube/proxy/testing/kube_server/pods.go | 2 +- lib/kube/proxy/url_test.go | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/kube/proxy/cluster_details.go b/lib/kube/proxy/cluster_details.go index 0ac7a2c0149f9..1bd7ed579eef5 100644 --- a/lib/kube/proxy/cluster_details.go +++ b/lib/kube/proxy/cluster_details.go @@ -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. @@ -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. diff --git a/lib/kube/proxy/scheme.go b/lib/kube/proxy/scheme.go index de76d2e449d4f..85d80739ada50 100644 --- a/lib/kube/proxy/scheme.go +++ b/lib/kube/proxy/scheme.go @@ -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 @@ -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 { @@ -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 diff --git a/lib/kube/proxy/testing/kube_server/kube_mock.go b/lib/kube/proxy/testing/kube_server/kube_mock.go index eb1cc97edacce..501573b4b3768 100644 --- a/lib/kube/proxy/testing/kube_server/kube_mock.go +++ b/lib/kube/proxy/testing/kube_server/kube_mock.go @@ -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{ diff --git a/lib/kube/proxy/testing/kube_server/pods.go b/lib/kube/proxy/testing/kube_server/pods.go index f1b27891f1b51..4efc623aadf79 100644 --- a/lib/kube/proxy/testing/kube_server/pods.go +++ b/lib/kube/proxy/testing/kube_server/pods.go @@ -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") diff --git a/lib/kube/proxy/url_test.go b/lib/kube/proxy/url_test.go index b296bf0c0bce0..64bd96ed72375 100644 --- a/lib/kube/proxy/url_test.go +++ b/lib/kube/proxy/url_test.go @@ -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{ {