From f9d957c504c71a945793a48dee528ac590dc5feb Mon Sep 17 00:00:00 2001 From: Pablo Rodriguez Nava Date: Thu, 20 Aug 2026 11:56:34 +0200 Subject: [PATCH] OCPBUGS-112043: Preserve proxy environment vars The MCO's registry utilities was using the CC CR to fetch the proxy configuration and directly pass it to the container-libs image library (the old one, not the new mono-repo). Due to known limitations of the old containers/images libs and that the OS Image Stream logic used to live in the operator and not in the MCC, that recently gained the proxy env-vars, we handled the proxy in best-effort. There's already a fix in container-libs that allows us passing all the needed information to the library without the need of the env-vars (useful for scenarios that don't have them, like the operator), but the fix cannot be consumed till we migrate to container-libs from container/images. This change basically paws the way for the future migration while assuming that in the new current scenario with everything running in pods/containers/scripts that have the env-vars properly set, by ignoring the user given proxy settings if the configuration has NO_PROXY or different proxies for HTTP and HTTPS, delegating the proxy configuration to the underlaying container/images that will pick them from the env-vars. Signed-off-by: Pablo Rodriguez Nava --- cmd/machine-config-osimagestream/helpers.go | 8 +-- pkg/controller/bootstrap/bootstrap.go | 9 +-- pkg/imageutils/sys_context.go | 45 +++++++++----- pkg/imageutils/sys_context_fs.go | 4 ++ pkg/imageutils/sys_context_fs_test.go | 7 +-- pkg/imageutils/sys_context_test.go | 69 +++++++++++---------- 6 files changed, 76 insertions(+), 66 deletions(-) diff --git a/cmd/machine-config-osimagestream/helpers.go b/cmd/machine-config-osimagestream/helpers.go index bcaf7985de..5b3638d70b 100644 --- a/cmd/machine-config-osimagestream/helpers.go +++ b/cmd/machine-config-osimagestream/helpers.go @@ -113,12 +113,12 @@ func getProxyConfig() *configv1.ProxyStatus { proxyStatus.HTTPSProxy = httpsProxy } - // Although a newer version of container-libs uses the NO_PROXY env var, the - // version we are using now does not. We should add that functionality here - // if https://redhat.atlassian.net/browse/MCO-2016 is addressed. + if noProxy := os.Getenv("NO_PROXY"); noProxy != "" { + proxyStatus.NoProxy = noProxy + } // If none of the environment variables were set, return a nil config. - if proxyStatus.HTTPProxy == "" && proxyStatus.HTTPSProxy == "" { + if proxyStatus.HTTPProxy == "" && proxyStatus.HTTPSProxy == "" && proxyStatus.NoProxy == "" { return nil } diff --git a/pkg/controller/bootstrap/bootstrap.go b/pkg/controller/bootstrap/bootstrap.go index 5da3beadb5..d86a8eca6c 100644 --- a/pkg/controller/bootstrap/bootstrap.go +++ b/pkg/controller/bootstrap/bootstrap.go @@ -242,7 +242,7 @@ func (b *Bootstrap) Run(destDir string) error { return fmt.Errorf("error filtering pools: %w", err) } - sysCtxFactory := buildSysContextFactory(pullSecret, cconfig, cconfig.Spec.Infra, imgCfg, icspRules, idmsRules, itmsRules) + sysCtxFactory := buildSysContextFactory(pullSecret, cconfig, imgCfg, icspRules, idmsRules, itmsRules) // Enable OSImageStreams if the FeatureGate is active. // Previously this also excluded ExternalTopologyMode (HyperShift) because @@ -523,7 +523,6 @@ func (b *Bootstrap) fetchOSImageStream( func buildSysContextFactory( pullSecret *corev1.Secret, cconfig *mcfgv1.ControllerConfig, - infra *apicfgv1.Infrastructure, imgCfg *apicfgv1.Image, icspRules []*apioperatorsv1alpha1.ImageContentSourcePolicy, idmsRules []*apicfgv1.ImageDigestMirrorSet, @@ -534,12 +533,6 @@ func buildSysContextFactory( WithControllerConfig(cconfig). WithSecret(pullSecret) - // In HCP the proxy config belongs to the data plane cluster and is - // unreachable from the management cluster where this code runs. - if infra != nil && infra.Status.ControlPlaneTopology == apicfgv1.ExternalTopologyMode { - builder.WithoutProxy() - } - registriesConfig, err := imageutils.GenerateRegistriesConfig(imgCfg, icspRules, idmsRules, itmsRules) if err != nil { return nil, fmt.Errorf("failed to generate registries config: %w", err) diff --git a/pkg/imageutils/sys_context.go b/pkg/imageutils/sys_context.go index 919840e137..e14708f175 100644 --- a/pkg/imageutils/sys_context.go +++ b/pkg/imageutils/sys_context.go @@ -3,6 +3,7 @@ package imageutils import ( "bytes" "fmt" + configv1 "github.com/openshift/api/config/v1" "net/url" "os" "path/filepath" @@ -30,7 +31,7 @@ type SysContextBuilder struct { secret *corev1.Secret controllerConfig *mcfgv1.ControllerConfig registriesConfig *sysregistriesv2.V2RegistriesConf - skipProxy bool + explicitProxy *configv1.ProxyStatus } // NewSysContextBuilder creates a new SysContextBuilder for building SysContext instances. @@ -44,15 +45,15 @@ func (b *SysContextBuilder) WithSecret(secret *corev1.Secret) *SysContextBuilder return b } -// WithControllerConfig adds certificates and proxy settings from ControllerConfig to the SysContext. +// WithControllerConfig adds certificates from ControllerConfig to the SysContext. func (b *SysContextBuilder) WithControllerConfig(cc *mcfgv1.ControllerConfig) *SysContextBuilder { b.controllerConfig = cc return b } -// WithoutProxy disables proxy configuration even if the ControllerConfig has one. -func (b *SysContextBuilder) WithoutProxy() *SysContextBuilder { - b.skipProxy = true +// WithProxy overrides the system proxy defined by HTTP_PROXY, HTTPS_PROXY and NO_PROXY environment variables. +func (b *SysContextBuilder) WithProxy(proxy *configv1.ProxyStatus) *SysContextBuilder { + b.explicitProxy = proxy return b } @@ -160,22 +161,36 @@ func (b *SysContextBuilder) buildRegistries(sysContext *SysContext) error { return nil } -// buildProxy configures the Docker proxy URL from ControllerConfig.Spec.Proxy. -// Prioritizes HTTPS proxy over HTTP proxy when both are configured. -// Returns early if no controller config was provided or no proxy is configured. +// buildProxy configures the Docker proxy URL from the one specified by WithProxy. +// If no explicit proxy was set the context won't use an explicit proxy and the +// system-wide HTTP_PROXY, HTTPS_PROXY and NO_PROXY environment variables will +// be used while performing operations. func (b *SysContextBuilder) buildProxy(sysContext *SysContext) error { - if b.controllerConfig == nil || b.skipProxy { + if b.explicitProxy == nil { + return nil + } + + // TODO: Remove when containers-libs is used with https://github.com/containers/container-libs/pull/583 + if b.explicitProxy.NoProxy != "" || + (b.explicitProxy.HTTPProxy != "" && + b.explicitProxy.HTTPSProxy != "" && + b.explicitProxy.HTTPProxy != b.explicitProxy.HTTPSProxy) { + // Not supported right now: + // 1. NO_PROXY + // 2. Different proxies for HTTPS and HTTP + // Till we have proper proxy support by the new container-libs just trust that the system-proxy vars are set. + // Note to the reader: If this code runs in the MCC, the OS Builder o an installer script + // the environment variables should be present and early returning would be just fine as + // the values of the env-vars will be used. return nil } - // TODO: Improve when containers-libs is used with https://github.com/containers/container-libs/pull/583 - // proxy settings var proxyRawURL string //nolint:gocritic // if-else chain is clearer than switch for this proxy priority logic - if b.controllerConfig.Spec.Proxy != nil && b.controllerConfig.Spec.Proxy.HTTPSProxy != "" { - proxyRawURL = b.controllerConfig.Spec.Proxy.HTTPSProxy - } else if b.controllerConfig.Spec.Proxy != nil && b.controllerConfig.Spec.Proxy.HTTPProxy != "" { - proxyRawURL = b.controllerConfig.Spec.Proxy.HTTPProxy + if b.explicitProxy.HTTPSProxy != "" { + proxyRawURL = b.explicitProxy.HTTPSProxy + } else if b.explicitProxy.HTTPProxy != "" { + proxyRawURL = b.explicitProxy.HTTPProxy } else { // No proxy configured return nil diff --git a/pkg/imageutils/sys_context_fs.go b/pkg/imageutils/sys_context_fs.go index f44183dee8..99ca04c734 100644 --- a/pkg/imageutils/sys_context_fs.go +++ b/pkg/imageutils/sys_context_fs.go @@ -139,6 +139,10 @@ func NewSysContextFromFilesystem(opts SysContextPaths) (*SysContext, error) { sysCtxBuilder = sysCtxBuilder.WithControllerConfig(ctrlCfg) + if opts.Proxy != nil { + sysCtxBuilder = sysCtxBuilder.WithProxy(opts.Proxy) + } + sysCtx, err := sysCtxBuilder.Build() if err != nil { return nil, err diff --git a/pkg/imageutils/sys_context_fs_test.go b/pkg/imageutils/sys_context_fs_test.go index f78642b3b8..8272196170 100644 --- a/pkg/imageutils/sys_context_fs_test.go +++ b/pkg/imageutils/sys_context_fs_test.go @@ -640,7 +640,7 @@ func TestNewSysContextFromFilesystem_EdgeCases(t *testing.T) { require.NoError(t, err, "Cleanup should not fail") }) - t.Run("Both HTTP and HTTPS proxy set - HTTPS should take precedence", func(t *testing.T) { + t.Run("Different HTTP and HTTPS proxies falls back to env vars", func(t *testing.T) { paths := SysContextPaths{ Proxy: &configv1.ProxyStatus{ HTTPProxy: "http://http-proxy.example.com:8080", @@ -651,10 +651,7 @@ func TestNewSysContextFromFilesystem_EdgeCases(t *testing.T) { sysCtx, err := NewSysContextFromFilesystem(paths) require.NoError(t, err, "Should not fail") require.NotNil(t, sysCtx, "SysContext should not be nil") - require.NotNil(t, sysCtx.SysContext.DockerProxyURL, "DockerProxyURL should not be nil") - - assert.Equal(t, "https", sysCtx.SysContext.DockerProxyURL.Scheme, "HTTPS proxy should take precedence") - assert.Equal(t, "https-proxy.example.com:3128", sysCtx.SysContext.DockerProxyURL.Host, "Proxy host should match HTTPS proxy") + assert.Nil(t, sysCtx.SysContext.DockerProxyURL, "DockerProxyURL should be nil when HTTP and HTTPS proxies differ") err = sysCtx.Cleanup() require.NoError(t, err, "Cleanup should not fail") diff --git a/pkg/imageutils/sys_context_test.go b/pkg/imageutils/sys_context_test.go index 36c39c8d1d..969cee2d47 100644 --- a/pkg/imageutils/sys_context_test.go +++ b/pkg/imageutils/sys_context_test.go @@ -111,6 +111,7 @@ func TestSysContextBuilder(t *testing.T) { name string secret *corev1.Secret controllerConfig *mcfgv1.ControllerConfig + proxy *configv1.ProxyStatus registriesConfig *sysregistriesv2.V2RegistriesConf expectTempDir bool expectAuthFile bool @@ -171,7 +172,7 @@ MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAuOSW8w== expectCerts: true, }, { - name: "WithControllerConfig only - proxy", + name: "WithControllerConfig only - proxy in controllerconfig does not set DockerProxyURL", controllerConfig: &mcfgv1.ControllerConfig{ Spec: mcfgv1.ControllerConfigSpec{ Proxy: &configv1.ProxyStatus{ @@ -179,8 +180,8 @@ MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAuOSW8w== }, }, }, - expectTempDir: false, // Proxy doesn't need temp dir - expectProxy: true, + expectTempDir: false, + expectProxy: false, }, { name: "WithControllerConfig only - just rootCA", @@ -220,15 +221,18 @@ MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAuOSW8w== -----END CERTIFICATE-----`), }, }, - Proxy: &configv1.ProxyStatus{ - HTTPSProxy: "https://proxy.example.com:3128", - }, }, }, expectTempDir: true, expectAuthFile: true, expectCerts: true, - expectProxy: true, + }, + { + name: "WithProxy sets DockerProxyURL", + proxy: &configv1.ProxyStatus{ + HTTPSProxy: "https://proxy.example.com:3128", + }, + expectProxy: true, }, } @@ -244,6 +248,10 @@ MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAuOSW8w== builder.WithControllerConfig(tc.controllerConfig) } + if tc.proxy != nil { + builder.WithProxy(tc.proxy) + } + if tc.registriesConfig != nil { builder.WithRegistriesConfig(tc.registriesConfig) } @@ -320,7 +328,7 @@ func TestSysContextBuilderWithProxy(t *testing.T) { name string httpProxy string httpsProxy string - skipProxy bool + useProxy bool expectedScheme string expectedHost string expectedUsername string @@ -329,18 +337,21 @@ func TestSysContextBuilderWithProxy(t *testing.T) { { name: "HTTPS proxy with complete URL", httpsProxy: "https://proxy.example.com:3128", + useProxy: true, expectedScheme: "https", expectedHost: "proxy.example.com:3128", }, { name: "HTTP proxy with complete URL", httpProxy: "http://proxy.example.com:8080", + useProxy: true, expectedScheme: "http", expectedHost: "proxy.example.com:8080", }, { name: "HTTPS proxy with authentication", httpsProxy: "https://user:password@proxy.example.com:3128", + useProxy: true, expectedScheme: "https", expectedHost: "proxy.example.com:3128", expectedUsername: "user", @@ -349,58 +360,48 @@ func TestSysContextBuilderWithProxy(t *testing.T) { { name: "HTTP proxy with authentication", httpProxy: "http://proxyuser:proxypass@proxy.example.com:8080", + useProxy: true, expectedScheme: "http", expectedHost: "proxy.example.com:8080", expectedUsername: "proxyuser", expectedPassword: "proxypass", }, { - name: "Both proxies - HTTPS preferred with auth", - httpProxy: "http://httpuser:httppass@http-proxy.example.com:8080", - httpsProxy: "https://httpsuser:httpspass@https-proxy.example.com:3128", - expectedScheme: "https", - expectedHost: "https-proxy.example.com:3128", - expectedUsername: "httpsuser", - expectedPassword: "httpspass", + name: "Different HTTP and HTTPS proxies falls back to env vars", + httpProxy: "http://httpuser:httppass@http-proxy.example.com:8080", + httpsProxy: "https://httpsuser:httpspass@https-proxy.example.com:3128", + useProxy: true, }, { name: "HTTPS proxy without port", httpsProxy: "https://proxy.example.com", + useProxy: true, expectedScheme: "https", expectedHost: "proxy.example.com", }, { name: "HTTPS proxy with special characters in password", httpsProxy: "https://user:p@ssw0rd!@proxy.example.com:3128", + useProxy: true, expectedScheme: "https", expectedHost: "proxy.example.com:3128", expectedUsername: "user", expectedPassword: "p@ssw0rd!", }, { - name: "WithoutProxy skips proxy even when configured", - httpsProxy: "https://proxy.example.com:3128", - httpProxy: "http://proxy.example.com:8080", - skipProxy: true, + name: "No explicit proxy leaves DockerProxyURL nil", }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - cc := &mcfgv1.ControllerConfig{ - Spec: mcfgv1.ControllerConfigSpec{ - Proxy: &configv1.ProxyStatus{ - HTTPProxy: tc.httpProxy, - HTTPSProxy: tc.httpsProxy, - }, - }, - } - builder := NewSysContextBuilder(). - WithSecret(secret). - WithControllerConfig(cc) - if tc.skipProxy { - builder.WithoutProxy() + WithSecret(secret) + if tc.useProxy { + builder.WithProxy(&configv1.ProxyStatus{ + HTTPProxy: tc.httpProxy, + HTTPSProxy: tc.httpsProxy, + }) } sysCtx, err := builder.Build() @@ -408,8 +409,8 @@ func TestSysContextBuilderWithProxy(t *testing.T) { require.NotNil(t, sysCtx, "SysContext wrapper should not be nil") require.NotNil(t, sysCtx.SysContext, "Underlying SystemContext should not be nil") - if tc.skipProxy { - assert.Nil(t, sysCtx.SysContext.DockerProxyURL, "DockerProxyURL should be nil when proxy is skipped") + if tc.expectedScheme == "" { + assert.Nil(t, sysCtx.SysContext.DockerProxyURL, "DockerProxyURL should be nil") } else { require.NotNil(t, sysCtx.SysContext.DockerProxyURL, "DockerProxyURL should not be nil") assert.Equal(t, tc.expectedScheme, sysCtx.SysContext.DockerProxyURL.Scheme, "Proxy scheme should match")