Skip to content

OCPBUGS-60993: Enrich IBI config image proxy NoProxy with cluster networks #10543

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
carbonin wants to merge 2 commits into
base: main
Choose a base branch
from
Open

OCPBUGS-60993: Enrich IBI config image proxy NoProxy with cluster networks #10543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
68a05fb
Use installConfig consistently for all network types in proxy
carbonin May 11, 2026
24f4372
Enrich IBI NoProxy with cluster network CIDRs in config image
carbonin May 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion pkg/asset/imagebased/configimage/clusterconfiguration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"fmt"
"os"
"path/filepath"
"strings"

"github.com/thoas/go-funk"
k8sjson "sigs.k8s.io/json"
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down
16 changes: 13 additions & 3 deletions pkg/asset/imagebased/configimage/clusterconfiguration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -159,7 +159,7 @@ func TestClusterConfiguration_Generate(t *testing.T) {
},

expectedConfig: clusterConfiguration().
proxy(proxy()).
proxy(enrichedProxy()).
additionalTrustBundle(imagebased.AdditionalTrustBundle{
UserCaBundle: testCert,
}).
Expand All @@ -183,7 +183,7 @@ func TestClusterConfiguration_Generate(t *testing.T) {
},

expectedConfig: clusterConfiguration().
proxy(proxy()).
proxy(enrichedProxy()).
additionalTrustBundle(imagebased.AdditionalTrustBundle{
UserCaBundle: testCert,
ProxyConfigmapBundle: testCert,
Expand Down Expand Up @@ -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"),
Expand Down
38 changes: 6 additions & 32 deletions pkg/asset/manifests/proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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()

Expand Down Expand Up @@ -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
}

Expand Down
41 changes: 41 additions & 0 deletions pkg/types/proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package types

import (
"strings"

"k8s.io/apimachinery/pkg/util/sets"
)

// BuildNoProxySet creates a set of NoProxy entries from networking configuration.
Copy link
Copy Markdown
Contributor

@omertuc omertuc May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// BuildNoProxySet creates a set of NoProxy entries from networking configuration.
// BuildNoProxySet creates a set of NoProxy entries based on install-config networking configuration.

I think we should call out which specific "Networking configuration" we're expecting here, gives better context on what's happening

// 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 {
Copy link
Copy Markdown
Contributor

@omertuc omertuc May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is causing a diagnostic:

Diagnostics:
1. sets.String is deprecated: use generic Set instead.
   new ways:
   s1 := Set[string]{}
   s2 := New[string]() [default]

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)
Copy link
Copy Markdown
Contributor

@omertuc omertuc May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is new. Intentional? If yes please call it out in the commit message and justify

Copy link
Copy Markdown
Contributor

@omertuc omertuc May 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also must match CNO behavior

if trimmed != "" {
set.Insert(trimmed)
}
}
Comment on lines +33 to +38
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Preserve "*" as a terminal NO_PROXY value.

"*" is not just another token here. The manifest path already has to patch this up later, and any caller that serializes BuildNoProxySet(...).List() directly will turn a user-configured wildcard into "*,..." instead of the exact "*". That changes the serialized config and can change downstream proxy parsing.

Suggested fix 
 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 == "*" {
+			return sets.NewString("*")
+		}
 		if trimmed != "" {
 			set.Insert(trimmed)
 		}
 	}

 	return set
 }

Please add a regression case for userNoProxy == "*" as well.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for _, userValue := range strings.Split(userNoProxy, ",") {
trimmed := strings.TrimSpace(userValue)
if trimmed != "" {
set.Insert(trimmed)
}
}
for _, userValue := range strings.Split(userNoProxy, ",") {
trimmed := strings.TrimSpace(userValue)
if trimmed == "*" {
return sets.NewString("*")
}
if trimmed != "" {
set.Insert(trimmed)
}
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/types/proxy.go` around lines 33 - 38, The loop that parses userNoProxy
must treat "*" as a terminal token: if any trimmed token equals "*" then clear
any existing entries, insert only "*" into the set, and break out so
serialization yields exactly "*" (not "*,..."); update the parsing loop that
iterates over strings.Split(userNoProxy, ",") and uses set.Insert(trimmed) to
implement this special-case, and add a regression test for userNoProxy == "*" to
assert the serialized/List() output is exactly "*".


return set
}
137 changes: 137 additions & 0 deletions pkg/types/proxy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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())
})
}
}