diff --git a/pkg/asset/imagebased/configimage/clusterconfiguration.go b/pkg/asset/imagebased/configimage/clusterconfiguration.go index 711822fd37c..9f8fa9769ea 100644 --- a/pkg/asset/imagebased/configimage/clusterconfiguration.go +++ b/pkg/asset/imagebased/configimage/clusterconfiguration.go @@ -9,6 +9,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "github.com/thoas/go-funk" k8sjson "sigs.k8s.io/json" @@ -108,7 +109,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.Proxy, installConfig.Config.Networking), PullSecret: installConfig.Config.PullSecret, ReleaseRegistry: imageBasedConfig.Config.ReleaseRegistry, SSHKey: installConfig.Config.SSHKey, @@ -216,6 +217,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(proxy *types.Proxy, networking *types.Networking) *types.Proxy { + if proxy == nil { + return nil + } + if proxy.NoProxy == "*" { + return proxy + } + + set := types.BuildNoProxySet(networking, proxy.NoProxy) + + return &types.Proxy{ + HTTPProxy: proxy.HTTPProxy, + HTTPSProxy: proxy.HTTPSProxy, + NoProxy: strings.Join(set.List(), ","), + } +} + 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..42bde4e80f8 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,16 @@ 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 + 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,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 16f47384641..798ccfc78b3 100644 --- a/pkg/asset/manifests/proxy.go +++ b/pkg/asset/manifests/proxy.go @@ -9,7 +9,6 @@ import ( "github.com/pkg/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/sets" "sigs.k8s.io/yaml" configv1 "github.com/openshift/api/config/v1" @@ -43,15 +42,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 +79,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,19 +119,14 @@ 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) { + set := types.BuildNoProxySet(installConfig.Config.Networking, installConfig.Config.Proxy.NoProxy) + 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.Insert(internalAPIServer.Hostname()) platform := installConfig.Config.Platform.Name() @@ -171,24 +163,6 @@ func createNoProxy(installConfig *installconfig.InstallConfig, network *Networki 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 network.Config.Spec.ClusterNetwork { - set.Insert(clusterNetwork.CIDR) - } - - for _, userValue := range strings.Split(installConfig.Config.Proxy.NoProxy, ",") { - if userValue != "" { - set.Insert(userValue) - } - } - return strings.Join(set.List(), ","), nil } diff --git a/pkg/types/proxy.go b/pkg/types/proxy.go new file mode 100644 index 00000000000..5d5bc6c0336 --- /dev/null +++ b/pkg/types/proxy.go @@ -0,0 +1,41 @@ +package types + +import ( + "strings" + + "k8s.io/apimachinery/pkg/util/sets" +) + +// BuildNoProxySet creates a set of NoProxy entries from networking configuration. +// It includes localhost entries (.svc, .cluster.local, 127.0.0.1, localhost), +// all network CIDRs (cluster, service, machine), and user-provided NoProxy values. +// The caller can add platform-specific and API-server entries as needed. +func BuildNoProxySet(networking *Networking, userNoProxy string) sets.String { + set := sets.NewString( + "127.0.0.1", + "localhost", + ".svc", + ".cluster.local", + ) + + for _, network := range networking.ServiceNetwork { + set.Insert(network.String()) + } + + for _, network := range networking.MachineNetwork { + set.Insert(network.CIDR.String()) + } + + for _, network := range networking.ClusterNetwork { + set.Insert(network.CIDR.String()) + } + + for _, userValue := range strings.Split(userNoProxy, ",") { + 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..58f2ce6504e --- /dev/null +++ b/pkg/types/proxy_test.go @@ -0,0 +1,137 @@ +package types + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/openshift/installer/pkg/ipnet" +) + +func TestBuildNoProxySet(t *testing.T) { + cases := []struct { + name string + networking *Networking + userNoProxy string + expected []string + }{ + { + name: "empty networking and no user entries", + networking: &Networking{}, + userNoProxy: "", + expected: []string{"127.0.0.1", "localhost", ".svc", ".cluster.local"}, + }, + { + name: "service network entries are included", + networking: &Networking{ + ServiceNetwork: []ipnet.IPNet{ + *ipnet.MustParseCIDR("172.30.0.0/16"), + }, + }, + userNoProxy: "", + expected: []string{"127.0.0.1", "localhost", ".svc", ".cluster.local", "172.30.0.0/16"}, + }, + { + name: "machine network entries are included", + networking: &Networking{ + MachineNetwork: []MachineNetworkEntry{ + {CIDR: *ipnet.MustParseCIDR("10.0.0.0/16")}, + }, + }, + userNoProxy: "", + expected: []string{"127.0.0.1", "localhost", ".svc", ".cluster.local", "10.0.0.0/16"}, + }, + { + name: "cluster network entries are included", + networking: &Networking{ + ClusterNetwork: []ClusterNetworkEntry{ + {CIDR: *ipnet.MustParseCIDR("10.128.0.0/14")}, + }, + }, + userNoProxy: "", + expected: []string{"127.0.0.1", "localhost", ".svc", ".cluster.local", "10.128.0.0/14"}, + }, + { + name: "single user no-proxy entry is included", + networking: &Networking{}, + userNoProxy: "example.com", + expected: []string{"127.0.0.1", "localhost", ".svc", ".cluster.local", "example.com"}, + }, + { + name: "multiple comma-separated user entries are included", + networking: &Networking{}, + userNoProxy: "example.com,internal.corp,192.168.1.0/24", + expected: []string{"127.0.0.1", "localhost", ".svc", ".cluster.local", "example.com", "internal.corp", "192.168.1.0/24"}, + }, + { + name: "user entries with surrounding whitespace are trimmed", + networking: &Networking{}, + userNoProxy: " example.com , internal.corp ", + expected: []string{"127.0.0.1", "localhost", ".svc", ".cluster.local", "example.com", "internal.corp"}, + }, + { + name: "empty segments in user no-proxy are ignored", + networking: &Networking{}, + userNoProxy: "example.com,,internal.corp,", + expected: []string{"127.0.0.1", "localhost", ".svc", ".cluster.local", "example.com", "internal.corp"}, + }, + { + name: "all network types and user entries combined", + 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")}, + }, + }, + userNoProxy: "example.com", + expected: []string{ + "127.0.0.1", "localhost", ".svc", ".cluster.local", + "172.30.0.0/16", "10.0.0.0/16", "10.128.0.0/14", "example.com", + }, + }, + { + name: "duplicate user entry matching a built-in entry is deduplicated", + networking: &Networking{ + ServiceNetwork: []ipnet.IPNet{ + *ipnet.MustParseCIDR("172.30.0.0/16"), + }, + }, + userNoProxy: "172.30.0.0/16,localhost", + expected: []string{"127.0.0.1", "localhost", ".svc", ".cluster.local", "172.30.0.0/16"}, + }, + { + name: "multiple entries in each network type", + 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")}, + }, + }, + userNoProxy: "", + expected: []string{ + "127.0.0.1", "localhost", ".svc", ".cluster.local", + "172.30.0.0/16", "fd02::/112", "10.0.0.0/16", "fd00::/48", "10.128.0.0/14", "fd01::/48", + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + result := BuildNoProxySet(tc.networking, tc.userNoProxy) + assert.ElementsMatch(t, tc.expected, result.List()) + }) + } +}