Repository: openshift/assisted-service (language: python)

Issue:
MGMT-17106: Disable ironic vlan creation when static networking config is present
Previously the ironic agent would define vlan interfaces when it
detected that vlans were present using LLDP

This change only allows this type of reconfiguration if the user didn't
provide static networking config. If a user does provide static
networking config and requires vlans it's expected that they will
configure them manually rather than relying on LLDP-based configuration
from the ironic agent.

This also requires new ICC code to vendor in the change to accept this
parameter when we create the ironic agent ignition.

## List all the issues related to this PR

Resolves https://issues.redhat.com/browse/MGMT-17106

- [ ] New Feature <!-- new functionality -->
- [ ] Enhancement <!-- refactor, code changes, improvement, that won't add new features -->
- [x] Bug fix
- [ ] Tests
- [ ] Documentation
- [ ] CI/CD <!-- Notice that changes for Dockerfiles/Jenkinsfiles aren't tested in CI due to a known bug. -->

## What environments does this code impact?

- [ ] Automation (CI, tools, etc)
- [ ] Cloud
- [x] Operator Managed Deployments
- [ ] None

## How was this code tested?

<!-- Please, select one or more if needed: -->

- [ ] assisted-test-infra environment
- [ ] dev-scripts environment
- [ ] Reviewer's test appreciated
- [x] Waiting for CI to do a full test run
- [ ] Manual (Elaborate on how it was tested)
- [ ] No tests needed

I don't have the environment to test this, unit tests are all I can do for now.
CI will catch any potential regressions and hopefully QE has the kind of environment required to actually test it.

## Checklist

- [x] Title and description added to both, commit and PR.
- [x] Relevant issues have been associated (see [CONTRIBUTING] guide)
- [x] This change does not require a documentation update (docstring, `docs`, README, etc)
- [x] Does this change include unit-tests (note that code changes require unit-tests)

## Reviewers Checklist

- Are the title and description (in both PR and commit) meaningful and clear?
- Is there a bug required (and linked) for this change?
- Should this PR be backported?



Discussion / hints:

Fix patch:
diff --git a/go.mod b/go.mod
index fa448722f3e..d0e1d68524f 100644
--- a/go.mod
+++ b/go.mod
@@ -57,7 +57,7 @@ require (
 	github.com/openshift/custom-resource-status v1.1.2
 	github.com/openshift/generic-admission-server v1.14.1-0.20231020105858-8dcc3c9b298f
 	github.com/openshift/hive/apis v0.0.0-20220222213051-def9088fdb5a
-	github.com/openshift/image-customization-controller v0.0.0-20231018032321-4677f060c4bf
+	github.com/openshift/image-customization-controller v0.0.0-20240129110832-60a3867d7f9e
 	github.com/ory/dockertest/v3 v3.9.1
 	github.com/patrickmn/go-cache v2.1.0+incompatible
 	github.com/pelletier/go-toml v1.9.5
diff --git a/go.sum b/go.sum
index 338d51e82b5..91b37dd49be 100644
--- a/go.sum
+++ b/go.sum
@@ -1366,8 +1366,8 @@ github.com/openshift/generic-admission-server v1.14.1-0.20231020105858-8dcc3c9b2
 github.com/openshift/generic-admission-server v1.14.1-0.20231020105858-8dcc3c9b298f/go.mod h1:/CLsleDcQ6AFTGKJe9VL3Y4rB9DqX3fQwQv47q2/ZJc=
 github.com/openshift/hive/apis v0.0.0-20220222213051-def9088fdb5a h1:E+XPJs/aVvYsrlJzo2ED38ZTR2RTNUlFMmOaFAAdMZg=
 github.com/openshift/hive/apis v0.0.0-20220222213051-def9088fdb5a/go.mod h1:E1bgquRiwfugdArdecPbpYIrAdve5kTzMaJb0+8jMXI=
-github.com/openshift/image-customization-controller v0.0.0-20231018032321-4677f060c4bf h1:Z+LlKXsPGtzMsIQxSWAtebZLlUV/o678wYReGItPSkc=
-github.com/openshift/image-customization-controller v0.0.0-20231018032321-4677f060c4bf/go.mod h1:rTvO75VGcFhEuE2HgQe10OijcR9HBDNC+CnKei8p1HE=
+github.com/openshift/image-customization-controller v0.0.0-20240129110832-60a3867d7f9e h1:x1j5gWm4PdTTXTmuX3S7VAmsnazbbMiefQyCLdyjH74=
+github.com/openshift/image-customization-controller v0.0.0-20240129110832-60a3867d7f9e/go.mod h1:rTvO75VGcFhEuE2HgQe10OijcR9HBDNC+CnKei8p1HE=
 github.com/openshift/library-go v0.0.0-20191003152030-97c62d8a2901/go.mod h1:NBttNjZpWwup/nthuLbPAPSYC8Qyo+BBK5bCtFoyYjo=
 github.com/openshift/library-go v0.0.0-20200831114015-2ab0c61c15de/go.mod h1:6vwp+YhYOIlj8MpkQKkebTTSn2TuYyvgiAFQ206jIEQ=
 github.com/openshift/library-go v0.0.0-20231110170715-08d73a9c798b h1:BmunS/b02dKSwQ0H21+3EbXDfDr9t8UDlO54WxKkOts=
diff --git a/internal/ignition/ironic.go b/internal/ignition/ironic.go
index 6c48fb9cf19..eb3f97cac59 100644
--- a/internal/ignition/ironic.go
+++ b/internal/ignition/ironic.go
@@ -13,12 +13,17 @@ func GenerateIronicConfig(ironicBaseURL string, ironicInspectorURL string, infra
 	config.Ignition.Version = "3.2.0"
 
 	httpProxy, httpsProxy, noProxy := common.GetProxyConfigs(infraEnv.Proxy)
+	ironicInspectorVlanInterfaces := ""
+	// allow ironic to configure vlans if no static network config is provided
+	if infraEnv.StaticNetworkConfig == "" {
+		ironicInspectorVlanInterfaces = "all"
+	}
 	// TODO: this should probably get the pullSecret as well
-	ib, err := iccignition.New([]byte{}, []byte{}, ironicBaseURL, ironicInspectorURL, ironicAgentImage, "", "", "", httpProxy, httpsProxy, noProxy, "")
+	ib, err := iccignition.New([]byte{}, []byte{}, ironicBaseURL, ironicInspectorURL, ironicAgentImage, "", "", "", httpProxy, httpsProxy, noProxy, "", ironicInspectorVlanInterfaces)
 	if err != nil {
 		return []byte{}, err
 	}
-	config.Storage.Files = []ignition_config_types_32.File{ib.IronicAgentConf()}
+	config.Storage.Files = []ignition_config_types_32.File{ib.IronicAgentConf(ironicInspectorVlanInterfaces)}
 	// TODO: sort out the flags (authfile...) and copy network
 	config.Systemd.Units = []ignition_config_types_32.Unit{ib.IronicAgentService(false)}
 	return json.Marshal(config)
diff --git a/vendor/github.com/openshift/image-customization-controller/pkg/ignition/builder.go b/vendor/github.com/openshift/image-customization-controller/pkg/ignition/builder.go
index f4bcf100c3c..8d69471b857 100644
--- a/vendor/github.com/openshift/image-customization-controller/pkg/ignition/builder.go
+++ b/vendor/github.com/openshift/image-customization-controller/pkg/ignition/builder.go
@@ -14,28 +14,26 @@ import (
 const (
 	// https://github.com/openshift/ironic-image/blob/master/scripts/configure-coreos-ipa#L14
 	ironicAgentPodmanFlags = "--tls-verify=false"
-
-	// https://github.com/openshift/ironic-image/blob/master/scripts/configure-coreos-ipa#L11
-	ironicInspectorVlanInterfaces = "all"
 )
 
 type ignitionBuilder struct {
-	nmStateData            []byte
-	registriesConf         []byte
-	ironicBaseURL          string
-	ironicInspectorBaseURL string
-	ironicAgentImage       string
-	ironicAgentPullSecret  string
-	ironicRAMDiskSSHKey    string
-	networkKeyFiles        []byte
-	ipOptions              string
-	httpProxy              string
-	httpsProxy             string
-	noProxy                string
-	hostname               string
+	nmStateData               []byte
+	registriesConf            []byte
+	ironicBaseURL             string
+	ironicInspectorBaseURL    string
+	ironicAgentImage          string
+	ironicAgentPullSecret     string
+	ironicRAMDiskSSHKey       string
+	networkKeyFiles           []byte
+	ipOptions                 string
+	httpProxy                 string
+	httpsProxy                string
+	noProxy                   string
+	hostname                  string
+	ironicAgentVlanInterfaces string
 }
 
-func New(nmStateData, registriesConf []byte, ironicBaseURL, ironicInspectorBaseURL, ironicAgentImage, ironicAgentPullSecret, ironicRAMDiskSSHKey, ipOptions string, httpProxy, httpsProxy, noProxy string, hostname string) (*ignitionBuilder, error) {
+func New(nmStateData, registriesConf []byte, ironicBaseURL, ironicInspectorBaseURL, ironicAgentImage, ironicAgentPullSecret, ironicRAMDiskSSHKey, ipOptions string, httpProxy, httpsProxy, noProxy string, hostname string, ironicAgentVlanInterfaces string) (*ignitionBuilder, error) {
 	if ironicBaseURL == "" {
 		return nil, errors.New("ironicBaseURL is required")
 	}
@@ -47,18 +45,19 @@ func New(nmStateData, registriesConf []byte, ironicBaseURL, ironicInspectorBaseU
 	}
 
 	return &ignitionBuilder{
-		nmStateData:            nmStateData,
-		registriesConf:         registriesConf,
-		ironicBaseURL:          ironicBaseURL,
-		ironicInspectorBaseURL: ironicInspectorBaseURL,
-		ironicAgentImage:       ironicAgentImage,
-		ironicAgentPullSecret:  ironicAgentPullSecret,
-		ironicRAMDiskSSHKey:    ironicRAMDiskSSHKey,
-		ipOptions:              ipOptions,
-		httpProxy:              httpProxy,
-		httpsProxy:             httpsProxy,
-		noProxy:                noProxy,
-		hostname:               hostname,
+		nmStateData:               nmStateData,
+		registriesConf:            registriesConf,
+		ironicBaseURL:             ironicBaseURL,
+		ironicInspectorBaseURL:    ironicInspectorBaseURL,
+		ironicAgentImage:          ironicAgentImage,
+		ironicAgentPullSecret:     ironicAgentPullSecret,
+		ironicRAMDiskSSHKey:       ironicRAMDiskSSHKey,
+		ipOptions:                 ipOptions,
+		httpProxy:                 httpProxy,
+		httpsProxy:                httpsProxy,
+		noProxy:                   noProxy,
+		hostname:                  hostname,
+		ironicAgentVlanInterfaces: ironicAgentVlanInterfaces,
 	}, nil
 }
 
@@ -97,8 +96,21 @@ func (b *ignitionBuilder) GenerateConfig() (config ignition_config_types_32.Conf
 		}
 	}
 
+	var ironicInspectorVlanInterfaces string
+	if strings.ToLower(b.ironicAgentVlanInterfaces) == "always" {
+		ironicInspectorVlanInterfaces = "all"
+	} else if strings.ToLower(b.ironicAgentVlanInterfaces) == "never" {
+		ironicInspectorVlanInterfaces = ""
+	} else {
+		if len(b.nmStateData) > 0 {
+			ironicInspectorVlanInterfaces = ""
+		} else {
+			ironicInspectorVlanInterfaces = "all"
+		}
+	}
+
 	config.Ignition.Version = "3.2.0"
-	config.Storage.Files = []ignition_config_types_32.File{b.IronicAgentConf()}
+	config.Storage.Files = []ignition_config_types_32.File{b.IronicAgentConf(ironicInspectorVlanInterfaces)}
 	config.Storage.Files = append(config.Storage.Files, netFiles...)
 	config.Systemd.Units = []ignition_config_types_32.Unit{b.IronicAgentService(len(netFiles) > 0)}
 
diff --git a/vendor/github.com/openshift/image-customization-controller/pkg/ignition/service_config.go b/vendor/github.com/openshift/image-customization-controller/pkg/ignition/service_config.go
index 4cdb738dc52..5e21070571c 100644
--- a/vendor/github.com/openshift/image-customization-controller/pkg/ignition/service_config.go
+++ b/vendor/github.com/openshift/image-customization-controller/pkg/ignition/service_config.go
@@ -2,21 +2,57 @@ package ignition
 
 import (
 	"fmt"
+	"net"
+	"net/url"
 	"strings"
 
 	ignition_config_types_32 "github.com/coreos/ignition/v2/config/v3_2/types"
 	"k8s.io/utils/pointer"
 )
 
-func (b *ignitionBuilder) IronicAgentConf() ignition_config_types_32.File {
+const (
+	defaultIronicPort    = "6385"
+	defaultInspectorPort = "5050"
+)
+
+func processURLs(baseURL, defaultPath, defaultPort string) string {
+	urls := strings.Split(baseURL, ",")
+	var result []string
+	for _, urlString := range urls {
+		if urlString == "" {
+			continue // tolerate empty strings or trailing commas
+		}
+
+		parsed, err := url.Parse(urlString)
+		if err != nil {
+			continue // I wish we had a logger here...
+		}
+
+		if defaultPort != "" && parsed.Port() == "" {
+			parsed.Host = net.JoinHostPort(parsed.Hostname(), defaultPort)
+		}
+
+		if defaultPath != "" && !strings.HasSuffix(parsed.Path, defaultPath) {
+			parsed.Path = fmt.Sprintf("%s%s", parsed.Path, defaultPath)
+		}
+
+		result = append(result, parsed.String())
+	}
+
+	return strings.Join(result, ",")
+}
+
+func (b *ignitionBuilder) IronicAgentConf(ironicInspectorVlanInterfaces string) ignition_config_types_32.File {
 	template := `
 [DEFAULT]
-api_url = %s:6385
-inspection_callback_url = %s:5050/v1/continue
+api_url = %s
+inspection_callback_url = %s
 insecure = True
 enable_vlan_interfaces = %s
 `
-	contents := fmt.Sprintf(template, b.ironicBaseURL, b.ironicInspectorBaseURL, ironicInspectorVlanInterfaces)
+	ironicURLs := processURLs(b.ironicBaseURL, "", defaultIronicPort)
+	inspectorURLs := processURLs(b.ironicInspectorBaseURL, "/v1/continue", defaultInspectorPort)
+	contents := fmt.Sprintf(template, ironicURLs, inspectorURLs, ironicInspectorVlanInterfaces)
 	return ignitionFileEmbed("/etc/ironic-python-agent.conf", 0644, false, []byte(contents))
 }
 
diff --git a/vendor/modules.txt b/vendor/modules.txt
index 6e25822affb..f485f9aeab8 100644
--- a/vendor/modules.txt
+++ b/vendor/modules.txt
@@ -750,7 +750,7 @@ github.com/openshift/hive/apis/hive/v1/openstack
 github.com/openshift/hive/apis/hive/v1/ovirt
 github.com/openshift/hive/apis/hive/v1/vsphere
 github.com/openshift/hive/apis/scheme
-# github.com/openshift/image-customization-controller v0.0.0-20231018032321-4677f060c4bf
+# github.com/openshift/image-customization-controller v0.0.0-20240129110832-60a3867d7f9e
 ## explicit; go 1.19
 github.com/openshift/image-customization-controller/pkg/ignition
 # github.com/openshift/library-go v0.0.0-20231110170715-08d73a9c798b


Test patch:
diff --git a/internal/ignition/ironic_test.go b/internal/ignition/ironic_test.go
index b0c40d049ae..a4149156ded 100644
--- a/internal/ignition/ironic_test.go
+++ b/internal/ignition/ironic_test.go
@@ -9,6 +9,7 @@ import (
 	. "github.com/onsi/gomega"
 	"github.com/openshift/assisted-service/internal/common"
 	"github.com/openshift/assisted-service/models"
+	"github.com/vincent-petithory/dataurl"
 )
 
 var _ = Describe("GenerateIronicConfig", func() {
@@ -45,6 +46,17 @@ var _ = Describe("GenerateIronicConfig", func() {
 		_, err := GenerateIronicConfig(ironicBaseURL, inspectorURL, infraEnv, "")
 		Expect(err).To(HaveOccurred())
 	})
+	It("set the ironic inspector config to not tag interfaces when static networking is configured", func() {
+		infraEnv.StaticNetworkConfig = "some network config here"
+		conf, err := GenerateIronicConfig(ironicBaseURL, inspectorURL, infraEnv, "ironicAgentImage:custom")
+		Expect(err).NotTo(HaveOccurred())
+		Expect(conf).Should(ContainSubstring(dataurl.Escape([]byte("enable_vlan_interfaces = \n"))))
+	})
+	It("sets the ironic inspector config to tag all interfaces when static networking is not configured", func() {
+		conf, err := GenerateIronicConfig(ironicBaseURL, inspectorURL, infraEnv, "ironicAgentImage:custom")
+		Expect(err).NotTo(HaveOccurred())
+		Expect(conf).Should(ContainSubstring(dataurl.Escape([]byte("enable_vlan_interfaces = all\n"))))
+	})
 })
 
 func validateIgnition(conf []byte) {