From 68a05fb93c595de1346a5cddb0dea4eac866bb15 Mon Sep 17 00:00:00 2001 From: Nick Carboni Date: Mon, 11 May 2026 09:57:25 -0400 Subject: [PATCH 1/2] Use installConfig consistently for all network types in proxy Remove the Networking asset dependency from createNoProxy and use installConfig directly for ClusterNetwork, matching the existing approach for ServiceNetwork and MachineNetwork. The Networking asset performs no business logic on ClusterNetwork data, only type conversion from ipnet.IPNet to string. Using installConfig directly for all three network types makes the code more consistent and removes an unnecessary dependency. --- pkg/asset/manifests/proxy.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/pkg/asset/manifests/proxy.go b/pkg/asset/manifests/proxy.go index 16f47384641..3531dafa723 100644 --- a/pkg/asset/manifests/proxy.go +++ b/pkg/asset/manifests/proxy.go @@ -43,15 +43,13 @@ func (*Proxy) Name() string { func (*Proxy) Dependencies() []asset.Asset { return []asset.Asset{ &installconfig.InstallConfig{}, - &Networking{}, } } // Generate generates the Proxy config and its CRD. func (p *Proxy) Generate(_ context.Context, dependencies asset.Parents) error { installConfig := &installconfig.InstallConfig{} - network := &Networking{} - dependencies.Get(installConfig, network) + dependencies.Get(installConfig) p.Config = &configv1.Proxy{ TypeMeta: metav1.TypeMeta{ @@ -82,7 +80,7 @@ func (p *Proxy) Generate(_ context.Context, dependencies asset.Parents) error { } if p.Config.Spec.HTTPProxy != "" || p.Config.Spec.HTTPSProxy != "" { - noProxy, err := createNoProxy(installConfig, network) + noProxy, err := createNoProxy(installConfig) if err != nil { return err } @@ -122,7 +120,7 @@ func (p *Proxy) Generate(_ context.Context, dependencies asset.Parents) error { // https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-instance-metadata.html // https://docs.microsoft.com/en-us/azure/virtual-machines/windows/instance-metadata-service // https://cloud.google.com/compute/docs/storing-retrieving-metadata -func createNoProxy(installConfig *installconfig.InstallConfig, network *Networking) (string, error) { +func createNoProxy(installConfig *installconfig.InstallConfig) (string, error) { internalAPIServer, err := url.Parse(getInternalAPIServerURL(installConfig.Config)) if err != nil { return "", errors.New("failed parsing internal API server when creating Proxy manifest") @@ -179,8 +177,8 @@ func createNoProxy(installConfig *installconfig.InstallConfig, network *Networki set.Insert(network.CIDR.String()) } - for _, clusterNetwork := range network.Config.Spec.ClusterNetwork { - set.Insert(clusterNetwork.CIDR) + for _, clusterNetwork := range installConfig.Config.Networking.ClusterNetwork { + set.Insert(clusterNetwork.CIDR.String()) } for _, userValue := range strings.Split(installConfig.Config.Proxy.NoProxy, ",") { From 554180dd302e6e2ff1b6cf6dd2d2cdea378cb087 Mon Sep 17 00:00:00 2001 From: Nick Carboni Date: Mon, 11 May 2026 14:45:07 -0400 Subject: [PATCH 2/2] Enrich IBI NoProxy with cluster network CIDRs in config image IBI cluster configuration was forwarding the user-provided Proxy config directly without adding cluster, service, and machine network CIDRs to NoProxy, which would cause in-cluster traffic to be routed through the proxy. Extract BuildNoProxySet into pkg/types so the manifests proxy asset and the IBI config image asset share the same NoProxy building logic. Update tests accordingly. Co-Authored-By: Claude Sonnet 4.6 --- .../configimage/clusterconfiguration.go | 23 ++- .../configimage/clusterconfiguration_test.go | 17 +- pkg/asset/manifests/proxy.go | 34 +--- pkg/types/proxy.go | 47 +++++ pkg/types/proxy_test.go | 190 ++++++++++++++++++ 5 files changed, 275 insertions(+), 36 deletions(-) create mode 100644 pkg/types/proxy.go create mode 100644 pkg/types/proxy_test.go diff --git a/pkg/asset/imagebased/configimage/clusterconfiguration.go b/pkg/asset/imagebased/configimage/clusterconfiguration.go index 711822fd37c..dc82fd53a63 100644 --- a/pkg/asset/imagebased/configimage/clusterconfiguration.go +++ b/pkg/asset/imagebased/configimage/clusterconfiguration.go @@ -9,8 +9,10 @@ import ( "fmt" "os" "path/filepath" + "strings" "github.com/thoas/go-funk" + "k8s.io/apimachinery/pkg/util/sets" k8sjson "sigs.k8s.io/json" "github.com/openshift/installer/pkg/asset" @@ -108,7 +110,7 @@ func (cc *ClusterConfiguration) Generate(_ context.Context, dependencies asset.P Hostname: imageBasedConfig.Config.Hostname, InfraID: clusterID.InfraID, KubeadminPasswordHash: pwdHash, - Proxy: installConfig.Config.Proxy, + Proxy: enrichNoProxy(installConfig.Config), PullSecret: installConfig.Config.PullSecret, ReleaseRegistry: imageBasedConfig.Config.ReleaseRegistry, SSHKey: installConfig.Config.SSHKey, @@ -216,6 +218,25 @@ func (cc *ClusterConfiguration) finish() error { return nil } +// enrichNoProxy adds cluster, service, and machine networks to the NoProxy field +// to ensure internal cluster communication bypasses the proxy. +func enrichNoProxy(installConfig *types.InstallConfig) *types.Proxy { + if installConfig.Proxy == nil { + return nil + } + if installConfig.Proxy.NoProxy == "*" { + return installConfig.Proxy + } + + set := types.BuildNoProxySet(installConfig) + + return &types.Proxy{ + HTTPProxy: installConfig.Proxy.HTTPProxy, + HTTPSProxy: installConfig.Proxy.HTTPSProxy, + NoProxy: strings.Join(sets.List(set), ","), + } +} + func chronyConfWithAdditionalNTPSources(sources []string) string { content := defaultChronyConf[:] for _, source := range sources { diff --git a/pkg/asset/imagebased/configimage/clusterconfiguration_test.go b/pkg/asset/imagebased/configimage/clusterconfiguration_test.go index 76c95d3ac0d..9abc7a58548 100644 --- a/pkg/asset/imagebased/configimage/clusterconfiguration_test.go +++ b/pkg/asset/imagebased/configimage/clusterconfiguration_test.go @@ -120,7 +120,7 @@ func TestClusterConfiguration_Generate(t *testing.T) { imageBasedConfig(), }, - expectedConfig: clusterConfiguration().proxy(proxy()).build().Config, + expectedConfig: clusterConfiguration().proxy(enrichedProxy()).build().Config, }, { name: "valid configuration with additionalTrustBundle, policyAlways without proxy", @@ -159,7 +159,7 @@ func TestClusterConfiguration_Generate(t *testing.T) { }, expectedConfig: clusterConfiguration(). - proxy(proxy()). + proxy(enrichedProxy()). additionalTrustBundle(imagebased.AdditionalTrustBundle{ UserCaBundle: testCert, }). @@ -183,7 +183,7 @@ func TestClusterConfiguration_Generate(t *testing.T) { }, expectedConfig: clusterConfiguration(). - proxy(proxy()). + proxy(enrichedProxy()). additionalTrustBundle(imagebased.AdditionalTrustBundle{ UserCaBundle: testCert, ProxyConfigmapBundle: testCert, @@ -416,6 +416,17 @@ func proxy() *types.Proxy { } } +func enrichedProxy() *types.Proxy { + // This is the proxy after enrichNoProxy() is applied with default test networks: + // MachineNetwork: 10.10.11.0/24, ClusterNetwork: 192.168.111.0/24, ServiceNetwork: 172.30.0.0/16 + // ClusterDomain: ocp-ibi.testing.com (from defaultInstallConfig) + return &types.Proxy{ + HTTPProxy: "http://10.10.10.11:80", + HTTPSProxy: "http://my-lab-proxy.org:443", + NoProxy: ".cluster.local,.svc,10.10.11.0/24,127.0.0.1,172.30.0.0/16,192.168.111.0/24,api-int.ocp-ibi.testing.com,internal.com,localhost", + } +} + func kubeadminPassword() *password.KubeadminPassword { return &password.KubeadminPassword{ PasswordHash: []byte("a-password-hash"), diff --git a/pkg/asset/manifests/proxy.go b/pkg/asset/manifests/proxy.go index 3531dafa723..dd89c98e82e 100644 --- a/pkg/asset/manifests/proxy.go +++ b/pkg/asset/manifests/proxy.go @@ -3,7 +3,6 @@ package manifests import ( "context" "fmt" - "net/url" "path" "strings" @@ -121,18 +120,7 @@ func (p *Proxy) Generate(_ context.Context, dependencies asset.Parents) error { // https://docs.microsoft.com/en-us/azure/virtual-machines/windows/instance-metadata-service // https://cloud.google.com/compute/docs/storing-retrieving-metadata func createNoProxy(installConfig *installconfig.InstallConfig) (string, error) { - internalAPIServer, err := url.Parse(getInternalAPIServerURL(installConfig.Config)) - if err != nil { - return "", errors.New("failed parsing internal API server when creating Proxy manifest") - } - - set := sets.NewString( - "127.0.0.1", - "localhost", - ".svc", - ".cluster.local", - internalAPIServer.Hostname(), - ) + set := types.BuildNoProxySet(installConfig.Config) platform := installConfig.Config.Platform.Name() @@ -169,25 +157,7 @@ func createNoProxy(installConfig *installconfig.InstallConfig) (string, error) { set.Insert("metadata", "metadata.google.internal", "metadata.google.internal.") } - for _, network := range installConfig.Config.Networking.ServiceNetwork { - set.Insert(network.String()) - } - - for _, network := range installConfig.Config.Networking.MachineNetwork { - set.Insert(network.CIDR.String()) - } - - for _, clusterNetwork := range installConfig.Config.Networking.ClusterNetwork { - set.Insert(clusterNetwork.CIDR.String()) - } - - for _, userValue := range strings.Split(installConfig.Config.Proxy.NoProxy, ",") { - if userValue != "" { - set.Insert(userValue) - } - } - - return strings.Join(set.List(), ","), nil + return strings.Join(sets.List(set), ","), nil } // Files returns the files generated by the asset. diff --git a/pkg/types/proxy.go b/pkg/types/proxy.go new file mode 100644 index 00000000000..50d6547dc09 --- /dev/null +++ b/pkg/types/proxy.go @@ -0,0 +1,47 @@ +package types + +import ( + "strings" + + "k8s.io/apimachinery/pkg/util/sets" +) + +// BuildNoProxySet creates a set of NoProxy entries from install configuration. +// It includes localhost entries (.svc, .cluster.local, 127.0.0.1, localhost), +// all network CIDRs (cluster, service, machine), the internal API server hostname, +// and user-provided NoProxy values. +// The caller can add platform-specific entries as needed. +func BuildNoProxySet(config *InstallConfig) sets.Set[string] { + set := sets.New[string]( + "127.0.0.1", + "localhost", + ".svc", + ".cluster.local", + ) + + for _, network := range config.Networking.ServiceNetwork { + set.Insert(network.String()) + } + + for _, network := range config.Networking.MachineNetwork { + set.Insert(network.CIDR.String()) + } + + for _, network := range config.Networking.ClusterNetwork { + set.Insert(network.CIDR.String()) + } + + // Add internal API server hostname + set.Insert("api-int." + config.ClusterDomain()) + + if config.Proxy != nil { + for _, userValue := range strings.Split(config.Proxy.NoProxy, ",") { + trimmed := strings.TrimSpace(userValue) + if trimmed != "" { + set.Insert(trimmed) + } + } + } + + return set +} diff --git a/pkg/types/proxy_test.go b/pkg/types/proxy_test.go new file mode 100644 index 00000000000..a059989cae0 --- /dev/null +++ b/pkg/types/proxy_test.go @@ -0,0 +1,190 @@ +package types + +import ( + "testing" + + "github.com/stretchr/testify/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/sets" + + "github.com/openshift/installer/pkg/ipnet" +) + +func TestBuildNoProxySet(t *testing.T) { + cases := []struct { + name string + config *InstallConfig + expected []string + }{ + { + name: "empty networking and no user entries", + config: &InstallConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + BaseDomain: "example.com", + Networking: &Networking{}, + }, + expected: []string{".cluster.local", ".svc", "127.0.0.1", "api-int.test.example.com", "localhost"}, + }, + { + name: "service network entries are included", + config: &InstallConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + BaseDomain: "example.com", + Networking: &Networking{ + ServiceNetwork: []ipnet.IPNet{ + *ipnet.MustParseCIDR("172.30.0.0/16"), + }, + }, + }, + expected: []string{".cluster.local", ".svc", "127.0.0.1", "172.30.0.0/16", "api-int.test.example.com", "localhost"}, + }, + { + name: "machine network entries are included", + config: &InstallConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + BaseDomain: "example.com", + Networking: &Networking{ + MachineNetwork: []MachineNetworkEntry{ + {CIDR: *ipnet.MustParseCIDR("10.0.0.0/16")}, + }, + }, + }, + expected: []string{".cluster.local", ".svc", "10.0.0.0/16", "127.0.0.1", "api-int.test.example.com", "localhost"}, + }, + { + name: "cluster network entries are included", + config: &InstallConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + BaseDomain: "example.com", + Networking: &Networking{ + ClusterNetwork: []ClusterNetworkEntry{ + {CIDR: *ipnet.MustParseCIDR("10.128.0.0/14")}, + }, + }, + }, + expected: []string{".cluster.local", ".svc", "10.128.0.0/14", "127.0.0.1", "api-int.test.example.com", "localhost"}, + }, + { + name: "single user no-proxy entry is included", + config: &InstallConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + BaseDomain: "example.com", + Networking: &Networking{}, + Proxy: &Proxy{ + NoProxy: "example.com", + }, + }, + expected: []string{".cluster.local", ".svc", "127.0.0.1", "api-int.test.example.com", "example.com", "localhost"}, + }, + { + name: "multiple comma-separated user entries are included", + config: &InstallConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + BaseDomain: "example.com", + Networking: &Networking{}, + Proxy: &Proxy{ + NoProxy: "example.com,internal.corp,192.168.1.0/24", + }, + }, + expected: []string{".cluster.local", ".svc", "127.0.0.1", "192.168.1.0/24", "api-int.test.example.com", "example.com", "internal.corp", "localhost"}, + }, + { + name: "user entries with surrounding whitespace are trimmed", + config: &InstallConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + BaseDomain: "example.com", + Networking: &Networking{}, + Proxy: &Proxy{ + NoProxy: " example.com , internal.corp ", + }, + }, + expected: []string{".cluster.local", ".svc", "127.0.0.1", "api-int.test.example.com", "example.com", "internal.corp", "localhost"}, + }, + { + name: "empty segments in user no-proxy are ignored", + config: &InstallConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + BaseDomain: "example.com", + Networking: &Networking{}, + Proxy: &Proxy{ + NoProxy: "example.com,,internal.corp,", + }, + }, + expected: []string{".cluster.local", ".svc", "127.0.0.1", "api-int.test.example.com", "example.com", "internal.corp", "localhost"}, + }, + { + name: "all network types and user entries combined", + config: &InstallConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + BaseDomain: "example.com", + Networking: &Networking{ + ServiceNetwork: []ipnet.IPNet{ + *ipnet.MustParseCIDR("172.30.0.0/16"), + }, + MachineNetwork: []MachineNetworkEntry{ + {CIDR: *ipnet.MustParseCIDR("10.0.0.0/16")}, + }, + ClusterNetwork: []ClusterNetworkEntry{ + {CIDR: *ipnet.MustParseCIDR("10.128.0.0/14")}, + }, + }, + Proxy: &Proxy{ + NoProxy: "example.com", + }, + }, + expected: []string{ + ".cluster.local", ".svc", "10.0.0.0/16", "10.128.0.0/14", + "127.0.0.1", "172.30.0.0/16", "api-int.test.example.com", "example.com", "localhost", + }, + }, + { + name: "duplicate user entry matching a built-in entry is deduplicated", + config: &InstallConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + BaseDomain: "example.com", + Networking: &Networking{ + ServiceNetwork: []ipnet.IPNet{ + *ipnet.MustParseCIDR("172.30.0.0/16"), + }, + }, + Proxy: &Proxy{ + NoProxy: "172.30.0.0/16,localhost", + }, + }, + expected: []string{".cluster.local", ".svc", "127.0.0.1", "172.30.0.0/16", "api-int.test.example.com", "localhost"}, + }, + { + name: "multiple entries in each network type", + config: &InstallConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + BaseDomain: "example.com", + Networking: &Networking{ + ServiceNetwork: []ipnet.IPNet{ + *ipnet.MustParseCIDR("172.30.0.0/16"), + *ipnet.MustParseCIDR("fd02::/112"), + }, + MachineNetwork: []MachineNetworkEntry{ + {CIDR: *ipnet.MustParseCIDR("10.0.0.0/16")}, + {CIDR: *ipnet.MustParseCIDR("fd00::/48")}, + }, + ClusterNetwork: []ClusterNetworkEntry{ + {CIDR: *ipnet.MustParseCIDR("10.128.0.0/14")}, + {CIDR: *ipnet.MustParseCIDR("fd01::/48")}, + }, + }, + }, + expected: []string{ + ".cluster.local", ".svc", "10.0.0.0/16", "10.128.0.0/14", + "127.0.0.1", "172.30.0.0/16", "api-int.test.example.com", + "fd00::/48", "fd01::/48", "fd02::/112", "localhost", + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + result := BuildNoProxySet(tc.config) + assert.ElementsMatch(t, tc.expected, sets.List(result)) + }) + } +}