diff --git a/controllers/workspace/devworkspace_controller.go b/controllers/workspace/devworkspace_controller.go index 6a25994da..16921efc7 100644 --- a/controllers/workspace/devworkspace_controller.go +++ b/controllers/workspace/devworkspace_controller.go @@ -141,12 +141,11 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request workspace.DevWorkspace = rawWorkspace workspace.Config = config + httpClientsHolder.ConfigureHttpClients(ctx, config.Routing) + 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 { @@ -264,7 +263,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request WorkspaceNamespace: workspace.Namespace, Context: ctx, K8sClient: r.Client, - HttpClient: httpClient, + HttpClient: httpClientsHolder.GetHttpClient(), DefaultResourceRequirements: workspace.Config.Workspace.DefaultContainerResources, } @@ -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 := SetupHttpClients(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..123470ad9 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,15 @@ import ( "context" "crypto/tls" "crypto/x509" + "fmt" "net/http" "net/url" + "reflect" + "sync" "time" + controller "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" "github.com/devfile/devworkspace-operator/pkg/config" - "k8s.io/apimachinery/pkg/types" "github.com/go-logr/logr" @@ -32,87 +35,253 @@ import ( "golang.org/x/net/http/httpproxy" ) -var ( - httpClient *http.Client +var httpClientsHolder HttpClientsHolder + +type HttpClientsHolder interface { + GetHttpClient() *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() *http.Client + + ConfigureHttpClients(context.Context, *controller.RoutingConfig) +} + +type DefaultHttpClientsHolder struct { + k8s client.Client + logger logr.Logger + + client *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 + + lastProxyConfig *controller.Proxy + lastCertsCMVersion string + + systemCertPool *x509.CertPool +} + +func SetupHttpClients(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() + httpClientsHolder = &DefaultHttpClientsHolder{ + k8s: k8s, + logger: logger, + systemCertPool: systemCertPool, + } + httpClientsHolder.ConfigureHttpClients(context.Background(), config.GetGlobalConfig().Routing) - if globalConfig.Routing != nil && globalConfig.Routing.ProxyConfig != nil { - proxyConf := httpproxy.Config{} - if globalConfig.Routing.ProxyConfig.HttpProxy != nil { - proxyConf.HTTPProxy = *globalConfig.Routing.ProxyConfig.HttpProxy + return nil +} + +func (h *DefaultHttpClientsHolder) GetHttpClient() *http.Client { + h.mu.RLock() + defer h.mu.RUnlock() + + return h.client +} + +func (h *DefaultHttpClientsHolder) GetHealthCheckHttpClient() *http.Client { + h.mu.RLock() + defer h.mu.RUnlock() + + return h.healthCheckHttpClient +} + +func (h *DefaultHttpClientsHolder) ConfigureHttpClients(ctx context.Context, routingConfig *controller.RoutingConfig) { + var newProxyConfig *controller.Proxy + var newCertsCM *corev1.ConfigMap + + if routingConfig != nil { + if routingConfig.ProxyConfig != nil { + newProxyConfig = routingConfig.ProxyConfig } - if globalConfig.Routing.ProxyConfig.HttpsProxy != nil { - proxyConf.HTTPSProxy = *globalConfig.Routing.ProxyConfig.HttpsProxy + + if routingConfig.TLSCertificateConfigmapRef != nil { + certsCM, err := h.readCertCM(ctx, routingConfig.TLSCertificateConfigmapRef) + if err != nil { + h.logger.Error(err, "Failed to read TLS certificate ConfigMap") + + // certsCM == nil, + // http clients will be rebuilt with a system cert pool, not an issue at all + } + + newCertsCM = certsCM } - if globalConfig.Routing.ProxyConfig.NoProxy != nil { - proxyConf.NoProxy = *globalConfig.Routing.ProxyConfig.NoProxy + } + + buildNewHttpClient, buildNewHealthCheckHttpClient := h.shouldRebuildClients(newProxyConfig, newCertsCM) + + if buildNewHttpClient || buildNewHealthCheckHttpClient { + newClient, newHealthCheckClient := h.buildNewClients( + buildNewHttpClient, + buildNewHealthCheckHttpClient, + newProxyConfig, + newCertsCM, + ) + + h.setNewClients( + newClient, + newHealthCheckClient, + newProxyConfig, + newCertsCM, + ) + } +} + +func (h *DefaultHttpClientsHolder) shouldRebuildClients(newProxyConfig *controller.Proxy, newCertsCM *corev1.ConfigMap) (bool, bool) { + h.mu.RLock() + defer h.mu.RUnlock() + + // Always rebuild if clients haven't been initialized yet + if h.client == nil || h.healthCheckHttpClient == nil { + return true, true + } + + if !reflect.DeepEqual(newProxyConfig, h.lastProxyConfig) { + return true, true + } + + certsCMVersion := "" + if newCertsCM != nil { + certsCMVersion = newCertsCM.ResourceVersion + } + + if certsCMVersion != h.lastCertsCMVersion { + return true, false + } + + return false, false +} + +func (h *DefaultHttpClientsHolder) buildNewClients( + buildNewHttpClient bool, + buildNewHealthCheckHttpClient bool, + newProxyConfig *controller.Proxy, + newCertsCM *corev1.ConfigMap, +) (*http.Client, *http.Client) { + + var newClient *http.Client + var newHealthCheckClient *http.Client + + proxyFunc := h.getProxyFunc(newProxyConfig) + caCertPool := h.getCaCertPool(newCertsCM) + + if buildNewHttpClient { + transport := http.DefaultTransport.(*http.Transport).Clone() + transport.Proxy = proxyFunc + transport.TLSClientConfig = &tls.Config{ + RootCAs: caCertPool, } - proxyFunc := func(req *http.Request) (*url.URL, error) { - return proxyConf.ProxyFunc()(req.URL) + newClient = &http.Client{ + Transport: transport, + Timeout: 5 * time.Second, } - transport.Proxy = proxyFunc + } + + if buildNewHealthCheckHttpClient { + healthCheckTransport := http.DefaultTransport.(*http.Transport).Clone() healthCheckTransport.Proxy = proxyFunc + healthCheckTransport.TLSClientConfig = &tls.Config{ + InsecureSkipVerify: true, + } + + newHealthCheckClient = &http.Client{ + Transport: healthCheckTransport, + Timeout: 500 * time.Millisecond, + } + } + + return newClient, newHealthCheckClient +} + +func (h *DefaultHttpClientsHolder) setNewClients( + newClient *http.Client, + newHealthCheckClient *http.Client, + newProxyConfig *controller.Proxy, + newCertsCM *corev1.ConfigMap, +) { + h.mu.Lock() + defer h.mu.Unlock() + + if newClient != nil { + h.client = newClient + } + + if newHealthCheckClient != nil { + h.healthCheckHttpClient = newHealthCheckClient } - httpClient = &http.Client{ - Transport: transport, + if newProxyConfig != nil { + h.lastProxyConfig = newProxyConfig.DeepCopy() + } else { + h.lastProxyConfig = nil } - healthCheckHttpClient = &http.Client{ - Transport: healthCheckTransport, - Timeout: 500 * time.Millisecond, + + if newCertsCM != nil { + h.lastCertsCMVersion = newCertsCM.ResourceVersion + } else { + h.lastCertsCMVersion = "" } - InjectCertificates(k8s, logger) } -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 *DefaultHttpClientsHolder) getProxyFunc(proxyConfig *controller.Proxy) func(*http.Request) (*url.URL, error) { + if proxyConfig != nil { + proxyConf := httpproxy.Config{} + if proxyConfig.HttpProxy != nil { + proxyConf.HTTPProxy = *proxyConfig.HttpProxy + } + if proxyConfig.HttpsProxy != nil { + proxyConf.HTTPSProxy = *proxyConfig.HttpsProxy + } + if proxyConfig.NoProxy != nil { + proxyConf.NoProxy = *proxyConfig.NoProxy + } + + return func(req *http.Request) (*url.URL, error) { + return proxyConf.ProxyFunc()(req.URL) } } + + return nil } -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 *DefaultHttpClientsHolder) getCaCertPool(cm *corev1.ConfigMap) *x509.CertPool { + if cm == nil { + return nil } - configMap := &corev1.ConfigMap{} - namespacedName := &types.NamespacedName{ - Name: configmapRef.Name, - Namespace: configmapRef.Namespace, - } - err := k8s.Get(context.Background(), *namespacedName, configMap) - if err != nil { - logger.Error(err, "Failed to read configmap with certificates") - return nil, false + + caCertPool := h.systemCertPool.Clone() + + for _, certsPem := range cm.Data { + if !caCertPool.AppendCertsFromPEM([]byte(certsPem)) { + h.logger.V(1).Info("Warning: failed to parse one or more certificates from ConfigMap") + } } - return configMap.Data, true + + return caCertPool } -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 - } +func (h *DefaultHttpClientsHolder) readCertCM(ctx context.Context, cmReference *controller.ConfigmapReference) (*corev1.ConfigMap, error) { + if cmReference == nil { + return nil, nil } - if ok := caCertPool.AppendCertsFromPEM(certsPem); ok { - transport.TLSClientConfig = &tls.Config{RootCAs: caCertPool} + + namespacedName := types.NamespacedName{ + Name: cmReference.Name, + Namespace: cmReference.Namespace, } + + configMap := &corev1.ConfigMap{} + if err := h.k8s.Get(ctx, namespacedName, configMap); err != nil { + return nil, fmt.Errorf("failed to read ConfigMap %s/%s containing certificates: %w", cmReference.Namespace, cmReference.Name, err) + } + + return configMap, nil } diff --git a/controllers/workspace/http_clients_holder_test.go b/controllers/workspace/http_clients_holder_test.go new file mode 100644 index 000000000..4b20726af --- /dev/null +++ b/controllers/workspace/http_clients_holder_test.go @@ -0,0 +1,44 @@ +// Copyright (c) 2019-2025 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 +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package controllers + +import ( + "context" + "net/http" + + controller "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" +) + +type TestHttpClientsHolder struct { + client *http.Client + healthCheckHttpClient *http.Client +} + +func (t *TestHttpClientsHolder) GetHttpClient() *http.Client { + return t.client +} + +func (t *TestHttpClientsHolder) GetHealthCheckHttpClient() *http.Client { + return t.healthCheckHttpClient +} + +func (t *TestHttpClientsHolder) ConfigureHttpClients(_ context.Context, _ *controller.RoutingConfig) { +} + +func SetupHttpClientsForTesting(client *http.Client) { + httpClientsHolder = &TestHttpClientsHolder{ + client: client, + healthCheckHttpClient: client, + } +} diff --git a/controllers/workspace/http_test.go b/controllers/workspace/http_test.go index a32d3c196..bd3315cfe 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,264 @@ package controllers -import "net/http" +import ( + "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "math/big" + "net/http" + "testing" + "time" -func SetupHttpClientsForTesting(client *http.Client) { - httpClient = client - healthCheckHttpClient = client + controller "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/utils/pointer" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/log/zap" +) + +func TestConfigureHttpClients_InitializesClientsWithNilRoutingConfig(t *testing.T) { + h := newTestHolder(nil) + + h.ConfigureHttpClients(context.Background(), nil) + + assert.NotNil(t, h.GetHttpClient()) + assert.NotNil(t, h.GetHealthCheckHttpClient()) +} + +func TestConfigureHttpClients_InitializesClientsWithEmptyRoutingConfig(t *testing.T) { + h := newTestHolder(nil) + + h.ConfigureHttpClients(context.Background(), &controller.RoutingConfig{}) + + assert.NotNil(t, h.GetHttpClient()) + assert.NotNil(t, h.GetHealthCheckHttpClient()) +} + +func TestConfigureHttpClients_SetsProxyOnBothClients(t *testing.T) { + h := newTestHolder(nil) + + routingConfig := &controller.RoutingConfig{ + ProxyConfig: &controller.Proxy{ + HttpProxy: pointer.String("http://proxy:8080"), + }, + } + + h.ConfigureHttpClients(context.Background(), routingConfig) + + httpTransport := getHttpClientTransport(t, h.GetHttpClient()) + assert.NotNil(t, httpTransport.Proxy) + + healthTransport := getHttpClientTransport(t, h.GetHealthCheckHttpClient()) + assert.NotNil(t, healthTransport.Proxy) +} + +func TestConfigureHttpClients_LoadsCertificatesFromConfigMap(t *testing.T) { + certPEM := generateSelfSignedCertPEM(t) + certCM := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "tls-certs", + Namespace: "cert-ns", + ResourceVersion: "1", + }, + Data: map[string]string{ + "ca.crt": string(certPEM), + }, + } + + h := newTestHolder(certCM) + + routingConfig := &controller.RoutingConfig{ + TLSCertificateConfigmapRef: &controller.ConfigmapReference{ + Name: "tls-certs", + Namespace: "cert-ns", + }, + } + + h.ConfigureHttpClients(context.Background(), routingConfig) + + tlsCfg := getHttpClientTransport(t, h.GetHttpClient()).TLSClientConfig + assert.NotNil(t, tlsCfg.RootCAs) +} + +func TestConfigureHttpClients_SkipsRebuildWhenNothingChanged(t *testing.T) { + h := newTestHolder(nil) + + routingConfig := &controller.RoutingConfig{ + ProxyConfig: &controller.Proxy{ + HttpProxy: pointer.String("http://proxy:8080"), + }, + } + + h.ConfigureHttpClients(context.Background(), routingConfig) + firstClient := h.GetHttpClient() + firstHealthCheck := h.GetHealthCheckHttpClient() + + h.ConfigureHttpClients(context.Background(), routingConfig) + + assert.Same(t, firstClient, h.GetHttpClient()) + assert.Same(t, firstHealthCheck, h.GetHealthCheckHttpClient()) +} + +func TestConfigureHttpClients_RebuildsBothWhenProxyChanges(t *testing.T) { + h := newTestHolder(nil) + + h.ConfigureHttpClients(context.Background(), &controller.RoutingConfig{ + ProxyConfig: &controller.Proxy{ + HttpProxy: pointer.String("http://old-proxy:8080"), + }, + }) + firstClient := h.GetHttpClient() + firstHealthCheck := h.GetHealthCheckHttpClient() + + h.ConfigureHttpClients(context.Background(), &controller.RoutingConfig{ + ProxyConfig: &controller.Proxy{ + HttpProxy: pointer.String("http://new-proxy:8080"), + }, + }) + + assert.NotSame(t, firstClient, h.GetHttpClient()) + assert.NotSame(t, firstHealthCheck, h.GetHealthCheckHttpClient()) +} + +func TestConfigureHttpClients_RebuildOnlyHttpClientWhenCertCMChanges(t *testing.T) { + certCM := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "tls-certs", + Namespace: "cert-ns", + ResourceVersion: "1", + }, + Data: map[string]string{ + "ca.crt": string(generateSelfSignedCertPEM(t)), + }, + } + + h := newTestHolder(certCM) + routingConfig := &controller.RoutingConfig{ + TLSCertificateConfigmapRef: &controller.ConfigmapReference{ + Name: "tls-certs", + Namespace: "cert-ns", + }, + } + + h.ConfigureHttpClients(context.Background(), routingConfig) + firstClient := h.GetHttpClient() + firstHealthCheck := h.GetHealthCheckHttpClient() + + // Simulate configmap update: read current, update data, write back + currentCM := &corev1.ConfigMap{} + require.NoError(t, h.k8s.Get(context.Background(), client.ObjectKeyFromObject(certCM), currentCM)) + + currentCM.Data["ca.crt"] = string(generateSelfSignedCertPEM(t)) + require.NoError(t, h.k8s.Update(context.Background(), currentCM)) + + h.ConfigureHttpClients(context.Background(), routingConfig) + + assert.NotSame(t, firstClient, h.GetHttpClient()) + assert.Same(t, firstHealthCheck, h.GetHealthCheckHttpClient()) +} + +func TestConfigureHttpClients_HandlesInvalidCertDataGracefully(t *testing.T) { + invalidCertCM := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bad-certs", + Namespace: "cert-ns", + ResourceVersion: "1", + }, + Data: map[string]string{ + "ca.crt": "not-a-valid-certificate", + }, + } + + h := newTestHolder(invalidCertCM) + + routingConfig := &controller.RoutingConfig{ + TLSCertificateConfigmapRef: &controller.ConfigmapReference{ + Name: "bad-certs", + Namespace: "cert-ns", + }, + } + + h.ConfigureHttpClients(context.Background(), routingConfig) + + assert.NotNil(t, h.GetHttpClient()) + assert.NotNil(t, h.GetHealthCheckHttpClient()) +} + +func TestConfigureHttpClients_HandlesMissingCertConfigMapGracefully(t *testing.T) { + h := newTestHolder(nil) + + routingConfig := &controller.RoutingConfig{ + TLSCertificateConfigmapRef: &controller.ConfigmapReference{ + Name: "nonexistent", + Namespace: "cert-ns", + }, + } + + h.ConfigureHttpClients(context.Background(), routingConfig) + + assert.NotNil(t, h.GetHttpClient()) + assert.NotNil(t, h.GetHealthCheckHttpClient()) +} + +func newTestHolder(certCM *corev1.ConfigMap) *DefaultHttpClientsHolder { + scheme := runtime.NewScheme() + _ = corev1.AddToScheme(scheme) + + h := &DefaultHttpClientsHolder{ + logger: zap.New(zap.UseDevMode(true)), + systemCertPool: x509.NewCertPool(), + } + + if certCM != nil { + h.k8s = fake.NewClientBuilder(). + WithScheme(scheme). + WithRuntimeObjects(certCM). + Build() + } else { + h.k8s = fake.NewClientBuilder(). + WithScheme(scheme). + Build() + } + + return h +} + +func generateSelfSignedCertPEM(t *testing.T) []byte { + 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, + KeyUsage: x509.KeyUsageCertSign, + } + + certDER, err := x509.CreateCertificate(rand.Reader, &template, &template, &key.PublicKey, key) + require.NoError(t, err) + + return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: certDER}) +} + +func getHttpClientTransport(t *testing.T, c *http.Client) *http.Transport { + t.Helper() + + transport, ok := c.Transport.(*http.Transport) + require.True(t, ok) + + return transport } diff --git a/controllers/workspace/status.go b/controllers/workspace/status.go index 6f359302e..494ca9487 100644 --- a/controllers/workspace/status.go +++ b/controllers/workspace/status.go @@ -210,7 +210,14 @@ func checkServerStatus(workspace *common.DevWorkspaceWithConfig) (ok bool, respo } healthz.Path = path.Join(healthz.Path, "healthz") - resp, err := healthCheckHttpClient.Get(healthz.String()) + resp, err := httpClientsHolder.GetHealthCheckHttpClient().Get(healthz.String()) + + defer func() { + if resp != nil && resp.Body != nil { + _ = resp.Body.Close() + } + }() + if err != nil { return false, nil, &dwerrors.RetryError{Err: err, Message: "Failed to check server status", RequeueAfter: 1 * time.Second} }