diff --git a/pkg/asset/imagebased/configimage/clusterconfiguration.go b/pkg/asset/imagebased/configimage/clusterconfiguration.go index 711822fd37c..7c1ec61003f 100644 --- a/pkg/asset/imagebased/configimage/clusterconfiguration.go +++ b/pkg/asset/imagebased/configimage/clusterconfiguration.go @@ -9,11 +9,14 @@ 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" + "github.com/openshift/installer/pkg/asset/manifests" "github.com/openshift/installer/pkg/asset/password" "github.com/openshift/installer/pkg/asset/tls" "github.com/openshift/installer/pkg/types" @@ -108,7 +111,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 +219,25 @@ func (cc *ClusterConfiguration) finish() error { return nil } +// enrichNoProxy adds cluster, service, and machine networks to the NoProxy field. +// In this way we align the proxy configuration with what would be generated in normal installations. +func enrichNoProxy(installConfig *types.InstallConfig) *types.Proxy { + if installConfig.Proxy == nil { + return nil + } + if installConfig.Proxy.NoProxy == "*" { + return installConfig.Proxy + } + + set := manifests.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 16f47384641..f1af48f1d2e 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" @@ -23,6 +22,40 @@ import ( "github.com/openshift/installer/pkg/types/powervc" ) +// BuildNoProxySet expands the user's NoProxy configuration with basic entries required for the internal cluster communication to avoid the user's proxy. This function only adds universal etnries that are relevant in all platforms. +// The caller can add platform-specific entries as needed. +func BuildNoProxySet(installConfig *types.InstallConfig) sets.Set[string] { + set := sets.New[string]( + "127.0.0.1", + "localhost", + ".svc", + ".cluster.local", + ) + + for _, network := range installConfig.Networking.ServiceNetwork { + set.Insert(network.String()) + } + + for _, network := range installConfig.Networking.MachineNetwork { + set.Insert(network.CIDR.String()) + } + + for _, network := range installConfig.Networking.ClusterNetwork { + set.Insert(network.CIDR.String()) + } + + // Add internal API server hostname + set.Insert("api-int." + installConfig.ClusterDomain()) + + for _, userValue := range strings.Split(installConfig.Proxy.NoProxy, ",") { + if userValue != "" { + set.Insert(userValue) + } + } + + return set +} + var proxyCfgFilename = path.Join(manifestDir, "cluster-proxy-01-config.yaml") // Proxy generates the cluster-proxy-*.yml files. @@ -43,15 +76,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 +113,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 +153,8 @@ 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) { - 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(), - ) +func createNoProxy(installConfig *installconfig.InstallConfig) (string, error) { + set := BuildNoProxySet(installConfig.Config) platform := installConfig.Config.Platform.Name() @@ -171,25 +191,7 @@ 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 + return strings.Join(sets.List(set), ","), nil } // Files returns the files generated by the asset. diff --git a/pkg/asset/manifests/proxy_test.go b/pkg/asset/manifests/proxy_test.go new file mode 100644 index 00000000000..e619f5adaf5 --- /dev/null +++ b/pkg/asset/manifests/proxy_test.go @@ -0,0 +1,184 @@ +package manifests + +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" + "github.com/openshift/installer/pkg/types" +) + +func TestBuildNoProxySet(t *testing.T) { + cases := []struct { + name string + config *types.InstallConfig + expected []string + }{ + { + name: "empty networking and no user entries", + config: &types.InstallConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + BaseDomain: "example.com", + Networking: &types.Networking{}, + Proxy: &types.Proxy{}, + }, + expected: []string{".cluster.local", ".svc", "127.0.0.1", "api-int.test.example.com", "localhost"}, + }, + { + name: "service network entries are included", + config: &types.InstallConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + BaseDomain: "example.com", + Networking: &types.Networking{ + ServiceNetwork: []ipnet.IPNet{ + *ipnet.MustParseCIDR("172.30.0.0/16"), + }, + }, + Proxy: &types.Proxy{}, + }, + 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: &types.InstallConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + BaseDomain: "example.com", + Networking: &types.Networking{ + MachineNetwork: []types.MachineNetworkEntry{ + {CIDR: *ipnet.MustParseCIDR("10.0.0.0/16")}, + }, + }, + Proxy: &types.Proxy{}, + }, + 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: &types.InstallConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + BaseDomain: "example.com", + Networking: &types.Networking{ + ClusterNetwork: []types.ClusterNetworkEntry{ + {CIDR: *ipnet.MustParseCIDR("10.128.0.0/14")}, + }, + }, + Proxy: &types.Proxy{}, + }, + 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: &types.InstallConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + BaseDomain: "example.com", + Networking: &types.Networking{}, + Proxy: &types.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: &types.InstallConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + BaseDomain: "example.com", + Networking: &types.Networking{}, + Proxy: &types.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: "empty segments in user no-proxy are ignored", + config: &types.InstallConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + BaseDomain: "example.com", + Networking: &types.Networking{}, + Proxy: &types.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: &types.InstallConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + BaseDomain: "example.com", + Networking: &types.Networking{ + ServiceNetwork: []ipnet.IPNet{ + *ipnet.MustParseCIDR("172.30.0.0/16"), + }, + MachineNetwork: []types.MachineNetworkEntry{ + {CIDR: *ipnet.MustParseCIDR("10.0.0.0/16")}, + }, + ClusterNetwork: []types.ClusterNetworkEntry{ + {CIDR: *ipnet.MustParseCIDR("10.128.0.0/14")}, + }, + }, + Proxy: &types.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: &types.InstallConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + BaseDomain: "example.com", + Networking: &types.Networking{ + ServiceNetwork: []ipnet.IPNet{ + *ipnet.MustParseCIDR("172.30.0.0/16"), + }, + }, + Proxy: &types.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: &types.InstallConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + BaseDomain: "example.com", + Networking: &types.Networking{ + ServiceNetwork: []ipnet.IPNet{ + *ipnet.MustParseCIDR("172.30.0.0/16"), + *ipnet.MustParseCIDR("fd02::/112"), + }, + MachineNetwork: []types.MachineNetworkEntry{ + {CIDR: *ipnet.MustParseCIDR("10.0.0.0/16")}, + {CIDR: *ipnet.MustParseCIDR("fd00::/48")}, + }, + ClusterNetwork: []types.ClusterNetworkEntry{ + {CIDR: *ipnet.MustParseCIDR("10.128.0.0/14")}, + {CIDR: *ipnet.MustParseCIDR("fd01::/48")}, + }, + }, + Proxy: &types.Proxy{}, + }, + 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)) + }) + } +}