diff --git a/controllers/workspace/http.go b/controllers/workspace/http.go index 82d77df35..65a4db090 100644 --- a/controllers/workspace/http.go +++ b/controllers/workspace/http.go @@ -25,6 +25,7 @@ import ( "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" @@ -44,7 +45,7 @@ type HttpClientsFactory interface { // 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 + GetHealthCheckHttpClient() *http.Client } // DefaultHttpClientsFactory is a thread-safe, caching implementation of HttpClientsFactory. @@ -59,13 +60,11 @@ type DefaultHttpClientsFactory struct { mu sync.RWMutex - httpClientProxyConfig *controller.Proxy httpClientConfigmapRef *controller.ConfigmapReference httpClientCertsVersion string - healthCheckHttpClientProxyConfig *controller.Proxy - systemCertPool *x509.CertPool + proxyFunc func(*http.Request) (*url.URL, error) } func SetupHttpClientsFactory(k8s client.Client, logger logr.Logger) error { @@ -74,10 +73,26 @@ func SetupHttpClientsFactory(k8s client.Client, logger logr.Logger) error { return fmt.Errorf("failed to load system cert pool: %w", err) } + proxyFunc := getProxyFunc() + + healthCheckHttpClientTransport := http.DefaultTransport.(*http.Transport).Clone() + if proxyFunc != nil { + // Preserve the default proxy (from env vars) when no explicit proxy is configured. + healthCheckHttpClientTransport.Proxy = proxyFunc + } + healthCheckHttpClientTransport.TLSClientConfig = &tls.Config{ + InsecureSkipVerify: true, + } + httpClientsFactory = &DefaultHttpClientsFactory{ k8s: k8s, logger: logger, systemCertPool: systemCertPool, + proxyFunc: proxyFunc, + healthCheckHttpClient: &http.Client{ + Transport: healthCheckHttpClientTransport, + Timeout: 500 * time.Millisecond, + }, } return nil @@ -97,13 +112,7 @@ func (h *DefaultHttpClientsFactory) GetHttpClient(ctx context.Context, routingCo 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() - } + h.httpClient = h.createHttpClient(certsCM) if certsCM == nil { h.httpClientCertsVersion = "" @@ -120,9 +129,12 @@ func (h *DefaultHttpClientsFactory) GetHttpClient(ctx context.Context, routingCo return h.httpClient } -func (h *DefaultHttpClientsFactory) createHttpClient(routingConfig *controller.RoutingConfig, certsCM *corev1.ConfigMap) *http.Client { +func (h *DefaultHttpClientsFactory) createHttpClient(certsCM *corev1.ConfigMap) *http.Client { transport := http.DefaultTransport.(*http.Transport).Clone() - transport.Proxy = h.getProxyFunc(routingConfig) + if h.proxyFunc != nil { + // Preserve the default proxy (from env vars) when no explicit proxy is configured. + transport.Proxy = h.proxyFunc + } transport.TLSClientConfig = &tls.Config{ RootCAs: h.getCaCertPool(certsCM), } @@ -140,7 +152,6 @@ func (h *DefaultHttpClientsFactory) shouldCreateHttpClient(routingConfig *contro var certsVersion string var configmapRef *controller.ConfigmapReference - var proxyConfig *controller.Proxy if certsCM != nil { certsVersion = certsCM.ResourceVersion @@ -150,88 +161,46 @@ func (h *DefaultHttpClientsFactory) shouldCreateHttpClient(routingConfig *contro } } - if routingConfig != nil { - proxyConfig = routingConfig.ProxyConfig - } - return certsVersion != h.httpClientCertsVersion || - !reflect.DeepEqual(configmapRef, h.httpClientConfigmapRef) || - !reflect.DeepEqual(proxyConfig, h.httpClientProxyConfig) + !reflect.DeepEqual(configmapRef, h.httpClientConfigmapRef) } -func (h *DefaultHttpClientsFactory) GetHealthCheckHttpClient(routingConfig *controller.RoutingConfig) *http.Client { +func (h *DefaultHttpClientsFactory) GetHealthCheckHttpClient() *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() - } - } + defer h.mu.RUnlock() return h.healthCheckHttpClient } -func (h *DefaultHttpClientsFactory) shouldCreateHealthCheckHttpClient(routingConfig *controller.RoutingConfig) bool { - if h.healthCheckHttpClient == nil { - return true - } - - var proxyConfig *controller.Proxy - - if routingConfig != nil { - proxyConfig = routingConfig.ProxyConfig - } - - 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, - } -} - -// getProxyFunc returns a proxy function based on the proxy settings in routingConfig. +// getProxyFunc returns a proxy function based on the global operator configuration. // 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 - } +func getProxyFunc() func(*http.Request) (*url.URL, error) { + globalConfig := config.GetGlobalConfig() - 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 - } + 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 + } + + if proxyConf.HTTPProxy == "" && proxyConf.HTTPSProxy == "" { + return nil + } - return func(req *http.Request) (*url.URL, error) { - return proxyConfig.ProxyFunc()(req.URL) + proxyFn := proxyConf.ProxyFunc() + return func(req *http.Request) (*url.URL, error) { + return proxyFn(req.URL) + } } + + return nil } // getCaCertPool returns a CA cert pool that includes system certs and any additional certs from the ConfigMap. diff --git a/controllers/workspace/http_test.go b/controllers/workspace/http_test.go index c1fd05786..59eda013e 100644 --- a/controllers/workspace/http_test.go +++ b/controllers/workspace/http_test.go @@ -18,6 +18,7 @@ import ( "crypto/ecdsa" "crypto/elliptic" "crypto/rand" + "crypto/tls" "crypto/x509" "crypto/x509/pkix" "encoding/pem" @@ -48,7 +49,7 @@ func (t *TestHttpClientsFactory) GetHttpClient(_ context.Context, _ *controller. return t.client } -func (t *TestHttpClientsFactory) GetHealthCheckHttpClient(_ *controller.RoutingConfig) *http.Client { +func (t *TestHttpClientsFactory) GetHealthCheckHttpClient() *http.Client { return t.healthCheckHttpClient } @@ -59,17 +60,42 @@ func SetupHttpClientsForTesting(client *http.Client) { } } -type getClientFunc func(factory HttpClientsFactory, routingConfig *controller.RoutingConfig) *http.Client +func TestHealthCheckHttpClient(t *testing.T) { + t.Run("returns non-nil client", func(t *testing.T) { + factory := newTestFactory(t) + + client := factory.GetHealthCheckHttpClient() + + require.NotNil(t, client) + }) + + t.Run("caches client on repeated calls", func(t *testing.T) { + factory := newTestFactory(t) + + client1 := factory.GetHealthCheckHttpClient() + client2 := factory.GetHealthCheckHttpClient() + + assert.Same(t, client1, client2) + }) +} func TestGetHttpClient(t *testing.T) { - getClient := func( - f HttpClientsFactory, - routingConfig *controller.RoutingConfig, - ) *http.Client { - return f.GetHttpClient(context.Background(), routingConfig) - } + t.Run("returns non-nil client", func(t *testing.T) { + factory := newTestFactory(t) + + client := factory.GetHttpClient(context.Background(), nil) - runCommonClientTests(t, getClient) + require.NotNil(t, client) + }) + + t.Run("caches client on repeated calls", func(t *testing.T) { + factory := newTestFactory(t) + + client1 := factory.GetHttpClient(context.Background(), nil) + client2 := factory.GetHttpClient(context.Background(), nil) + + assert.Same(t, client1, client2) + }) t.Run("rebuilds client when certs changes", func(t *testing.T) { factory := newTestFactory(t, @@ -104,60 +130,28 @@ func TestGetHttpClient(t *testing.T) { 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", "", "")} + 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)}, + }) + routingConfigs := []*controller.RoutingConfig{ + nil, + routingConfigWithCerts("test-certs-1", "default"), + routingConfigWithCerts("test-certs-2", "default"), + } var wg sync.WaitGroup for i := 0; i < 50; i++ { @@ -166,11 +160,14 @@ func runCommonClientTests(t *testing.T, getClient getClientFunc) { defer wg.Done() routingConfig := routingConfigs[idx%len(routingConfigs)] - client := getClient(factory, routingConfig) + client := factory.GetHttpClient(context.Background(), routingConfig) - assert.NotNil(t, client) - if routingConfig != nil { - assert.NotNil(t, client.Transport.(*http.Transport).Proxy) + require.NotNil(t, client) + + tlsConfig := client.Transport.(*http.Transport).TLSClientConfig + require.NotNil(t, tlsConfig) + if routingConfig != nil && routingConfig.TLSCertificateConfigmapRef != nil { + assert.NotNil(t, tlsConfig.RootCAs) } }(i) } @@ -209,10 +206,19 @@ func newTestFactory(t *testing.T, objs ...runtime.Object) *DefaultHttpClientsFac k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(objs...).Build() + healthCheckTransport := http.DefaultTransport.(*http.Transport).Clone() + healthCheckTransport.TLSClientConfig = &tls.Config{ + InsecureSkipVerify: true, + } + return &DefaultHttpClientsFactory{ k8s: k8sClient, logger: zap.New(zap.UseDevMode(true)), systemCertPool: systemCertPool, + healthCheckHttpClient: &http.Client{ + Transport: healthCheckTransport, + Timeout: 500 * time.Millisecond, + }, } } diff --git a/controllers/workspace/status.go b/controllers/workspace/status.go index 8cac692aa..c7b07a3fe 100644 --- a/controllers/workspace/status.go +++ b/controllers/workspace/status.go @@ -210,7 +210,7 @@ func checkServerStatus(workspace *common.DevWorkspaceWithConfig) (ok bool, respo } healthz.Path = path.Join(healthz.Path, "healthz") - healthCheckHttpClient := httpClientsFactory.GetHealthCheckHttpClient(workspace.Config.Routing) + healthCheckHttpClient := httpClientsFactory.GetHealthCheckHttpClient() 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}