diff --git a/controllers/workspace/devworkspace_controller.go b/controllers/workspace/devworkspace_controller.go index 6a25994da..69e496be1 100644 --- a/controllers/workspace/devworkspace_controller.go +++ b/controllers/workspace/devworkspace_controller.go @@ -144,9 +144,6 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request reqLogger = reqLogger.WithValues(constants.DevWorkspaceIDLoggerKey, workspace.Status.DevWorkspaceId) reqLogger.Info("Reconciling Workspace", "resolvedConfig", configString) - // Inject ca certificates to the http client, if the certificates configmap is created and defined in the config. - InjectCertificates(r.Client, r.Log) - // Check if the DevWorkspaceRouting instance is marked to be deleted, which is // indicated by the deletion timestamp being set. if workspace.GetDeletionTimestamp() != nil { @@ -260,6 +257,8 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request return reconcile.Result{Requeue: true}, err } + httpClient := httpClientsFactory.GetHttpClient(ctx, config.Routing) + flattenHelpers := flatten.ResolverTools{ WorkspaceNamespace: workspace.Namespace, Context: ctx, @@ -788,7 +787,10 @@ func (r *DevWorkspaceReconciler) getWorkspaceId(ctx context.Context, workspace * } func (r *DevWorkspaceReconciler) SetupWithManager(mgr ctrl.Manager) error { - setupHttpClients(mgr.GetClient(), mgr.GetLogger()) + err := SetupHttpClientsFactory(mgr.GetClient(), mgr.GetLogger()) + if err != nil { + return err + } maxConcurrentReconciles, err := wkspConfig.GetMaxConcurrentReconciles() if err != nil { diff --git a/controllers/workspace/http.go b/controllers/workspace/http.go index d4df30e3e..82d77df35 100644 --- a/controllers/workspace/http.go +++ b/controllers/workspace/http.go @@ -1,4 +1,4 @@ -// Copyright (c) 2019-2025 Red Hat, Inc. +// Copyright (c) 2019-2026 Red Hat, Inc. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at @@ -17,12 +17,14 @@ import ( "context" "crypto/tls" "crypto/x509" + "fmt" "net/http" "net/url" + "reflect" + "sync" "time" - "github.com/devfile/devworkspace-operator/pkg/config" - + controller "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" "k8s.io/apimachinery/pkg/types" "github.com/go-logr/logr" @@ -32,87 +34,242 @@ import ( "golang.org/x/net/http/httpproxy" ) -var ( +var httpClientsFactory HttpClientsFactory + +type HttpClientsFactory interface { + // GetHttpClient returns an HTTP client configured with proxy, TLS, and custom CA certificates + // from routingConfig. + GetHttpClient(context.Context, *controller.RoutingConfig) *http.Client + + // GetHealthCheckHttpClient returns an HTTP client that skips TLS verification. + // This client MUST only be used for workspace health/readiness checks, not for + // fetching external content or making security-sensitive requests. + GetHealthCheckHttpClient(*controller.RoutingConfig) *http.Client +} + +// DefaultHttpClientsFactory is a thread-safe, caching implementation of HttpClientsFactory. +// It caches one HTTP client and one health-check client, rebuilding either only when the +// relevant routing configuration (proxy settings, TLS certificates) changes. +type DefaultHttpClientsFactory struct { + k8s client.Client + logger logr.Logger + httpClient *http.Client healthCheckHttpClient *http.Client -) -func setupHttpClients(k8s client.Client, logger logr.Logger) { - transport := http.DefaultTransport.(*http.Transport).Clone() - healthCheckTransport := http.DefaultTransport.(*http.Transport).Clone() - healthCheckTransport.TLSClientConfig = &tls.Config{ - InsecureSkipVerify: true, + mu sync.RWMutex + + httpClientProxyConfig *controller.Proxy + httpClientConfigmapRef *controller.ConfigmapReference + httpClientCertsVersion string + + healthCheckHttpClientProxyConfig *controller.Proxy + + systemCertPool *x509.CertPool +} + +func SetupHttpClientsFactory(k8s client.Client, logger logr.Logger) error { + systemCertPool, err := x509.SystemCertPool() + if err != nil { + return fmt.Errorf("failed to load system cert pool: %w", err) } - globalConfig := config.GetGlobalConfig() + httpClientsFactory = &DefaultHttpClientsFactory{ + k8s: k8s, + logger: logger, + systemCertPool: systemCertPool, + } - if globalConfig.Routing != nil && globalConfig.Routing.ProxyConfig != nil { - proxyConf := httpproxy.Config{} - if globalConfig.Routing.ProxyConfig.HttpProxy != nil { - proxyConf.HTTPProxy = *globalConfig.Routing.ProxyConfig.HttpProxy - } - if globalConfig.Routing.ProxyConfig.HttpsProxy != nil { - proxyConf.HTTPSProxy = *globalConfig.Routing.ProxyConfig.HttpsProxy - } - if globalConfig.Routing.ProxyConfig.NoProxy != nil { - proxyConf.NoProxy = *globalConfig.Routing.ProxyConfig.NoProxy + return nil +} + +func (h *DefaultHttpClientsFactory) GetHttpClient(ctx context.Context, routingConfig *controller.RoutingConfig) *http.Client { + certsCM := h.readCertificates(ctx, routingConfig) + + h.mu.RLock() + if !h.shouldCreateHttpClient(routingConfig, certsCM) { + defer h.mu.RUnlock() + return h.httpClient + } + h.mu.RUnlock() + + h.mu.Lock() + defer h.mu.Unlock() + + if h.shouldCreateHttpClient(routingConfig, certsCM) { + h.httpClient = h.createHttpClient(routingConfig, certsCM) + + if routingConfig == nil { + h.httpClientProxyConfig = nil + } else { + h.httpClientProxyConfig = routingConfig.ProxyConfig.DeepCopy() } - proxyFunc := func(req *http.Request) (*url.URL, error) { - return proxyConf.ProxyFunc()(req.URL) + if certsCM == nil { + h.httpClientCertsVersion = "" + h.httpClientConfigmapRef = nil + } else { + h.httpClientCertsVersion = certsCM.ResourceVersion + h.httpClientConfigmapRef = &controller.ConfigmapReference{ + Name: certsCM.Name, + Namespace: certsCM.Namespace, + } } - transport.Proxy = proxyFunc - healthCheckTransport.Proxy = proxyFunc } - httpClient = &http.Client{ + return h.httpClient +} + +func (h *DefaultHttpClientsFactory) createHttpClient(routingConfig *controller.RoutingConfig, certsCM *corev1.ConfigMap) *http.Client { + transport := http.DefaultTransport.(*http.Transport).Clone() + transport.Proxy = h.getProxyFunc(routingConfig) + transport.TLSClientConfig = &tls.Config{ + RootCAs: h.getCaCertPool(certsCM), + } + + return &http.Client{ Transport: transport, + Timeout: 10 * time.Second, } - healthCheckHttpClient = &http.Client{ - Transport: healthCheckTransport, - Timeout: 500 * time.Millisecond, +} + +func (h *DefaultHttpClientsFactory) shouldCreateHttpClient(routingConfig *controller.RoutingConfig, certsCM *corev1.ConfigMap) bool { + if h.httpClient == nil { + return true + } + + var certsVersion string + var configmapRef *controller.ConfigmapReference + var proxyConfig *controller.Proxy + + if certsCM != nil { + certsVersion = certsCM.ResourceVersion + configmapRef = &controller.ConfigmapReference{ + Name: certsCM.Name, + Namespace: certsCM.Namespace, + } } - InjectCertificates(k8s, logger) + + if routingConfig != nil { + proxyConfig = routingConfig.ProxyConfig + } + + return certsVersion != h.httpClientCertsVersion || + !reflect.DeepEqual(configmapRef, h.httpClientConfigmapRef) || + !reflect.DeepEqual(proxyConfig, h.httpClientProxyConfig) } -func InjectCertificates(k8s client.Client, logger logr.Logger) { - if certs, ok := readCertificates(k8s, logger); ok { - for _, certsPem := range certs { - injectCertificates([]byte(certsPem), httpClient.Transport.(*http.Transport), logger) +func (h *DefaultHttpClientsFactory) GetHealthCheckHttpClient(routingConfig *controller.RoutingConfig) *http.Client { + h.mu.RLock() + if !h.shouldCreateHealthCheckHttpClient(routingConfig) { + defer h.mu.RUnlock() + return h.healthCheckHttpClient + } + h.mu.RUnlock() + + h.mu.Lock() + defer h.mu.Unlock() + + if h.shouldCreateHealthCheckHttpClient(routingConfig) { + h.healthCheckHttpClient = h.createHealthCheckHttpClient(routingConfig) + + if routingConfig == nil { + h.healthCheckHttpClientProxyConfig = nil + } else { + h.healthCheckHttpClientProxyConfig = routingConfig.ProxyConfig.DeepCopy() } } + + return h.healthCheckHttpClient } -func readCertificates(k8s client.Client, logger logr.Logger) (map[string]string, bool) { - configmapRef := config.GetGlobalConfig().Routing.TLSCertificateConfigmapRef - if configmapRef == nil { - return nil, false +func (h *DefaultHttpClientsFactory) shouldCreateHealthCheckHttpClient(routingConfig *controller.RoutingConfig) bool { + if h.healthCheckHttpClient == nil { + return true } - configMap := &corev1.ConfigMap{} - namespacedName := &types.NamespacedName{ - Name: configmapRef.Name, - Namespace: configmapRef.Namespace, + + var proxyConfig *controller.Proxy + + if routingConfig != nil { + proxyConfig = routingConfig.ProxyConfig } - err := k8s.Get(context.Background(), *namespacedName, configMap) - if err != nil { - logger.Error(err, "Failed to read configmap with certificates") - return nil, false + + return !reflect.DeepEqual(proxyConfig, h.healthCheckHttpClientProxyConfig) +} + +func (h *DefaultHttpClientsFactory) createHealthCheckHttpClient(routingConfig *controller.RoutingConfig) *http.Client { + transport := http.DefaultTransport.(*http.Transport).Clone() + transport.Proxy = h.getProxyFunc(routingConfig) + transport.TLSClientConfig = &tls.Config{ + InsecureSkipVerify: true, + } + + return &http.Client{ + Transport: transport, + Timeout: 500 * time.Millisecond, } - return configMap.Data, true } -func injectCertificates(certsPem []byte, transport *http.Transport, logger logr.Logger) { - caCertPool := transport.TLSClientConfig.RootCAs - if caCertPool == nil { - systemCertPool, err := x509.SystemCertPool() - if err != nil { - logger.Error(err, "Failed to load system cert pool") - caCertPool = x509.NewCertPool() - } else { - caCertPool = systemCertPool +// getProxyFunc returns a proxy function based on the proxy settings in routingConfig. +// Returns nil if no proxy is configured; a nil proxy func causes the HTTP transport to +// use the default proxy settings from environment variables. +func (h *DefaultHttpClientsFactory) getProxyFunc(routingConfig *controller.RoutingConfig) func(*http.Request) (*url.URL, error) { + if routingConfig == nil || routingConfig.ProxyConfig == nil { + return nil + } + + proxyConfig := httpproxy.Config{} + if routingConfig.ProxyConfig.HttpProxy != nil { + proxyConfig.HTTPProxy = *routingConfig.ProxyConfig.HttpProxy + } + if routingConfig.ProxyConfig.HttpsProxy != nil { + proxyConfig.HTTPSProxy = *routingConfig.ProxyConfig.HttpsProxy + } + if routingConfig.ProxyConfig.NoProxy != nil { + proxyConfig.NoProxy = *routingConfig.ProxyConfig.NoProxy + } + + return func(req *http.Request) (*url.URL, error) { + return proxyConfig.ProxyFunc()(req.URL) + } +} + +// getCaCertPool returns a CA cert pool that includes system certs and any additional certs from the ConfigMap. +// A nil pool causes the HTTP client to use the system default root CAs. +func (h *DefaultHttpClientsFactory) getCaCertPool(certsCM *corev1.ConfigMap) *x509.CertPool { + if certsCM == nil || len(certsCM.Data) == 0 { + return nil + } + + caCertPool := h.systemCertPool.Clone() + + for _, certsPem := range certsCM.Data { + if !caCertPool.AppendCertsFromPEM([]byte(certsPem)) { + h.logger.Error(fmt.Errorf("failed to parse one or more certificates from ConfigMap"), "Could not append CA certificates to pool") } } - if ok := caCertPool.AppendCertsFromPEM(certsPem); ok { - transport.TLSClientConfig = &tls.Config{RootCAs: caCertPool} + + return caCertPool +} + +func (h *DefaultHttpClientsFactory) readCertificates(ctx context.Context, routingConfig *controller.RoutingConfig) *corev1.ConfigMap { + if routingConfig == nil || routingConfig.TLSCertificateConfigmapRef == nil { + return nil + } + + configmapRef := routingConfig.TLSCertificateConfigmapRef + + namespacedName := types.NamespacedName{ + Name: configmapRef.Name, + Namespace: configmapRef.Namespace, } + + configMap := &corev1.ConfigMap{} + if err := h.k8s.Get(ctx, namespacedName, configMap); err != nil { + // print and ignore the error, http clients will be created with host's root CA set. + h.logger.Error(err, "Failed to read ConfigMap containing certificates", "namespace", configmapRef.Namespace, "name", configmapRef.Name) + return nil + } + + return configMap } diff --git a/controllers/workspace/http_test.go b/controllers/workspace/http_test.go index a32d3c196..c1fd05786 100644 --- a/controllers/workspace/http_test.go +++ b/controllers/workspace/http_test.go @@ -1,4 +1,4 @@ -// Copyright (c) 2019-2025 Red Hat, Inc. +// Copyright (c) 2019-2026 Red Hat, Inc. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at @@ -13,9 +13,224 @@ package controllers -import "net/http" +import ( + "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "math/big" + "net/http" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + controller "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/log/zap" +) + +type TestHttpClientsFactory struct { + client *http.Client + healthCheckHttpClient *http.Client +} + +func (t *TestHttpClientsFactory) GetHttpClient(_ context.Context, _ *controller.RoutingConfig) *http.Client { + return t.client +} + +func (t *TestHttpClientsFactory) GetHealthCheckHttpClient(_ *controller.RoutingConfig) *http.Client { + return t.healthCheckHttpClient +} func SetupHttpClientsForTesting(client *http.Client) { - httpClient = client - healthCheckHttpClient = client + httpClientsFactory = &TestHttpClientsFactory{ + client: client, + healthCheckHttpClient: client, + } +} + +type getClientFunc func(factory HttpClientsFactory, routingConfig *controller.RoutingConfig) *http.Client + +func TestGetHttpClient(t *testing.T) { + getClient := func( + f HttpClientsFactory, + routingConfig *controller.RoutingConfig, + ) *http.Client { + return f.GetHttpClient(context.Background(), routingConfig) + } + + runCommonClientTests(t, getClient) + + t.Run("rebuilds client when certs changes", func(t *testing.T) { + factory := newTestFactory(t, + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-certs-1", + Namespace: "default", + }, + Data: map[string]string{"ca.crt": generateTestCACert(t)}, + }, + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-certs-2", + Namespace: "default", + }, + Data: map[string]string{"ca.crt": generateTestCACert(t)}, + }) + routingConfig1 := routingConfigWithCerts("test-certs-1", "default") + routingConfig2 := routingConfigWithCerts("test-certs-2", "default") + + client1 := factory.GetHttpClient(context.Background(), routingConfig1) + + assert.NotNil(t, client1.Transport.(*http.Transport).TLSClientConfig.RootCAs) + + client2 := factory.GetHttpClient(context.Background(), routingConfig2) + + assert.NotNil(t, client2.Transport.(*http.Transport).TLSClientConfig.RootCAs) + assert.NotSame(t, client1, client2) + + client3 := factory.GetHttpClient(context.Background(), nil) + + assert.NotSame(t, client2, client3) + assert.Nil(t, client3.Transport.(*http.Transport).TLSClientConfig.RootCAs) + }) +} + +func TestGetHealthCheckHttpClient(t *testing.T) { + getClient := func( + f HttpClientsFactory, + rc *controller.RoutingConfig, + ) *http.Client { + return f.GetHealthCheckHttpClient(rc) + } + + runCommonClientTests(t, getClient) +} + +func runCommonClientTests(t *testing.T, getClient getClientFunc) { + t.Run("returns non-nil client", func(t *testing.T) { + factory := newTestFactory(t) + + client := getClient(factory, nil) + + require.NotNil(t, client) + }) + + t.Run("caches client on repeated calls", func(t *testing.T) { + factory := newTestFactory(t) + + client1 := getClient(factory, nil) + client2 := getClient(factory, nil) + + assert.Same(t, client1, client2) + }) + + t.Run("rebuilds client when proxy changes", func(t *testing.T) { + factory := newTestFactory(t) + routingConfig1 := routingConfigWithProxy("http://proxy:80", "", "") + routingConfig2 := routingConfigWithProxy("http://proxy:90", "", "") + + client1 := getClient(factory, routingConfig1) + + assert.NotNil(t, client1.Transport.(*http.Transport).Proxy) + + client2 := getClient(factory, routingConfig2) + + assert.NotNil(t, client2.Transport.(*http.Transport).Proxy) + assert.NotSame(t, client1, client2) + + client3 := getClient(factory, nil) + + assert.NotSame(t, client2, client3) + assert.Nil(t, client3.Transport.(*http.Transport).Proxy) + }) + + t.Run("safe for concurrent access", func(t *testing.T) { + factory := newTestFactory(t) + routingConfigs := []*controller.RoutingConfig{nil, routingConfigWithProxy("http://proxy:80", "", "")} + + var wg sync.WaitGroup + for i := 0; i < 50; i++ { + wg.Add(1) + go func(idx int) { + defer wg.Done() + + routingConfig := routingConfigs[idx%len(routingConfigs)] + client := getClient(factory, routingConfig) + + assert.NotNil(t, client) + if routingConfig != nil { + assert.NotNil(t, client.Transport.(*http.Transport).Proxy) + } + }(i) + } + wg.Wait() + }) +} + +func generateTestCACert(t *testing.T) string { + t.Helper() + + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + + template := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "test-ca"}, + NotBefore: time.Now(), + NotAfter: time.Now().Add(time.Hour), + IsCA: true, + BasicConstraintsValid: true, + } + der, err := x509.CreateCertificate(rand.Reader, template, template, &key.PublicKey, key) + require.NoError(t, err) + + return string(pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der})) +} + +func newTestFactory(t *testing.T, objs ...runtime.Object) *DefaultHttpClientsFactory { + t.Helper() + + systemCertPool, err := x509.SystemCertPool() + require.NoError(t, err) + + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + + k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(objs...).Build() + + return &DefaultHttpClientsFactory{ + k8s: k8sClient, + logger: zap.New(zap.UseDevMode(true)), + systemCertPool: systemCertPool, + } +} + +func routingConfigWithProxy(httpProxy, httpsProxy, noProxy string) *controller.RoutingConfig { + return &controller.RoutingConfig{ + ProxyConfig: &controller.Proxy{ + HttpProxy: ptr.To(httpProxy), + HttpsProxy: ptr.To(httpsProxy), + NoProxy: ptr.To(noProxy), + }, + } +} + +func routingConfigWithCerts(name, namespace string) *controller.RoutingConfig { + return &controller.RoutingConfig{ + TLSCertificateConfigmapRef: &controller.ConfigmapReference{ + Name: name, + Namespace: namespace, + }, + } } diff --git a/controllers/workspace/status.go b/controllers/workspace/status.go index 6f359302e..8cac692aa 100644 --- a/controllers/workspace/status.go +++ b/controllers/workspace/status.go @@ -1,5 +1,5 @@ // -// Copyright (c) 2019-2025 Red Hat, Inc. +// Copyright (c) 2019-2026 Red Hat, Inc. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at @@ -210,10 +210,16 @@ func checkServerStatus(workspace *common.DevWorkspaceWithConfig) (ok bool, respo } healthz.Path = path.Join(healthz.Path, "healthz") + healthCheckHttpClient := httpClientsFactory.GetHealthCheckHttpClient(workspace.Config.Routing) resp, err := healthCheckHttpClient.Get(healthz.String()) if err != nil { return false, nil, &dwerrors.RetryError{Err: err, Message: "Failed to check server status", RequeueAfter: 1 * time.Second} } + + defer func() { + _ = resp.Body.Close() + }() + if (resp.StatusCode / 100) == 4 { // Assume endpoint is unimplemented and/or * is covered with authentication. return true, &resp.StatusCode, nil