Repository: metal3-io/baremetal-operator
Pull request #1380 (2023-10-05 12:43:14)

:sparkles: Pass preprovisioningNetworkData to Ironic
**What this PR does / why we need it**:

Ironic supports [DHCP-less provisioning](https://docs.openstack.org/ironic/latest/admin/dhcp-less.html), which I expected to work via Metal<sup>3</sup>, particularly since we already provide the [IPAM controller](https://github.com/metal3-io/ip-address-manager) which can be used to statically assign control plane IPs.

[Recent discussions](https://kubernetes.slack.com/archives/CHD49TLE7/p1695719774588889) however highlighted that this flow is not currently possible via Metal<sup>3</sup>, unless you implement a custom `PreprovisioningImage` controller, or have other downstream customisations.

In the case where `--build-preprov-image` is passed to BMO, we enable creation of the `PreprovisioningImage` CR and an internal default controller (which does not embed the `preprovisioningNetworkData`, but downstream custom controllers are doing so, so we don't want to break existing users opting into this flow)

In the case where `--build-preprov-image` is *not* passed, Ironic can be configured to inject `network_data` into the Ramdisk, and IMO this seems like reasonable default behaviour when no image controller is enabled. 

In this mode if the user provides `preprovsioningNetworkDataName` we'll set the Node `network_data` on initial registration of the Ironic node.  

This means when IPA boots for inspection, the virtual media device can be mounted and the `openstack/latest/network_data.json` file retrieved to configure the network (e.g by `glean` in the upstream Ironic flow, but users can build a Ramdisk image with any tool/script that consumes this file)

Note that I don't think the current upstream IPA image currently includes simple-init/glean so this will still require a custom Ramdisk image, but it does mean we can support DHCP-less without mandating a custom `PreprovisioningImage` controller.




metal3-io-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
**Once this PR has been reviewed and has the lgtm label**, please assign kashifest for approval. For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["kashifest"]} -->
hardys: @dtantsur @Rozzii - as discussed on slack - in my environment this allows me to specify a secret with network_data.json contents via `preprovisioningNetworkDataName`, then retrieve the `network_data.json` inside the ramdisk from the attached virtual media device.

Also @zaneb if you could take a look that would be much appreciated, thanks!
hardys: /test-centos-e2e-integration-main
/test-ubuntu-integration-main
hardys: > However, there is an issue with this implementation - during provisioning at least, we will bounce ironic back and forth between the preprovisioningNetworkData and the actual networkData (assuming they are different) on every reconcile. It'll set the former during ValidateManagementAccess() and switch back to the latter immediately afterwards during Provision(). I'd assume this carries a risk of races.

I'm not sure this is correct - the code I'm adding here sets the [Node network_data ](https://docs.openstack.org/api-ref/baremetal/#create-node)(which not currently set at all by BMO) - @dtantsur can confirm but IIUC this API is only used to provide network configuration to the IPA Ramdisk pre-provisioning (so it makes sense to only consume `preprovisioningNetworkData` here)

Then on Provision we'll use the existing code, which creates a config drive with the `networkData` which is passed to the [Ironic API](https://docs.openstack.org/api-ref/baremetal/#change-node-provision-state) when we change the provision state - this data can potentially be different to the preprovisioningNetworkData AFAICS (although it's not a scenario I've tested)

Am I missing something? By setting currently-unset the Node network_data, I don't see how we can introduce a race with the config-drive data?
zaneb: > I'm not sure this is correct - the code I'm adding here sets the [Node network_data ](https://docs.openstack.org/api-ref/baremetal/#create-node)(which is not currently set at all by BMO) - @dtantsur can confirm but IIUC this API is only used to provide network configuration to the IPA Ramdisk pre-provisioning (so it makes sense to only consume preprovisioningNetworkData here)

Oh, sorry, you're right. I assumed that this was how we were passing the networkData today, but evidently that's set in the ConfigDrive instead.
metal3-io-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/1380#pullrequestreview-1670304048" title="Approved">Rozzii</a>*
**Once this PR has been reviewed and has the lgtm label**, please assign kashifest for approval. For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["kashifest"]} -->
dtantsur: /lgtm

Patch:
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index 40c964d9..3264b7b0 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -802,14 +802,25 @@ func (r *BareMetalHostReconciler) registerHost(prov provisioner.Provisioner, inf
 		return actionError{err}
 	}
 
+	hostConf := &hostConfigData{
+		host:          info.host,
+		log:           info.log.WithName("host_config_data"),
+		secretManager: r.secretManager(info.log),
+	}
+	preprovisioningNetworkData, err := hostConf.PreprovisioningNetworkData()
+	if err != nil {
+		return recordActionFailure(info, metal3api.RegistrationError, "failed to read preprovisioningNetworkData")
+	}
+
 	provResult, provID, err := prov.ValidateManagementAccess(
 		provisioner.ManagementAccessData{
-			BootMode:              info.host.Status.Provisioning.BootMode,
-			AutomatedCleaningMode: info.host.Spec.AutomatedCleaningMode,
-			State:                 info.host.Status.Provisioning.State,
-			CurrentImage:          getCurrentImage(info.host),
-			PreprovisioningImage:  preprovImg,
-			HasCustomDeploy:       hasCustomDeploy(info.host),
+			BootMode:                   info.host.Status.Provisioning.BootMode,
+			AutomatedCleaningMode:      info.host.Spec.AutomatedCleaningMode,
+			State:                      info.host.Status.Provisioning.State,
+			CurrentImage:               getCurrentImage(info.host),
+			PreprovisioningImage:       preprovImg,
+			PreprovisioningNetworkData: preprovisioningNetworkData,
+			HasCustomDeploy:            hasCustomDeploy(info.host),
 		},
 		credsChanged,
 		info.host.Status.ErrorType == metal3api.RegistrationError)
diff --git a/controllers/metal3.io/host_config_data.go b/controllers/metal3.io/host_config_data.go
index 4eb198c0..266ffd86 100644
--- a/controllers/metal3.io/host_config_data.go
+++ b/controllers/metal3.io/host_config_data.go
@@ -94,6 +94,26 @@ func (hcd *hostConfigData) NetworkData() (string, error) {
 	return networkDataRaw, err
 }
 
+// PreprovisioningNetworkData get preprovisioning network configuration
+func (hcd *hostConfigData) PreprovisioningNetworkData() (string, error) {
+	if hcd.host.Spec.PreprovisioningNetworkDataName == "" {
+		return "", nil
+	}
+	networkDataRaw, err := hcd.getSecretData(
+		hcd.host.Spec.PreprovisioningNetworkDataName,
+		hcd.host.Namespace,
+		"networkData",
+	)
+	if err != nil {
+		_, isNoDataErr := err.(NoDataInSecretError)
+		if isNoDataErr {
+			hcd.log.Info("PreprovisioningNetworkData networkData key is not set, returning empty data")
+			return "", nil
+		}
+	}
+	return networkDataRaw, err
+}
+
 // MetaData get host metatdata
 func (hcd *hostConfigData) MetaData() (string, error) {
 	if hcd.host.Spec.MetaData == nil {
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 162b204c..b2452be4 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -456,6 +456,25 @@ func (p *ironicProvisioner) ValidateManagementAccess(data provisioner.Management
 		// below.
 	}
 
+	// If no PreprovisioningImage builder is enabled we set the Node network_data
+	// this enables Ironic to inject the network_data into the ramdisk image
+	if !p.config.havePreprovImgBuilder {
+		networkDataRaw := data.PreprovisioningNetworkData
+		if networkDataRaw != "" {
+			var networkData map[string]interface{}
+			if yamlErr := yaml.Unmarshal([]byte(networkDataRaw), &networkData); yamlErr != nil {
+				p.log.Info("failed to unmarshal networkData from PreprovisioningNetworkData")
+				result, err = transientError(fmt.Errorf("invalid preprovisioningNetworkData: %w", yamlErr))
+				return
+			}
+			numUpdates := len(updater.Updates)
+			updater.SetTopLevelOpt("network_data", networkData, ironicNode.NetworkData)
+			if len(updater.Updates) != numUpdates {
+				p.log.Info("adding preprovisioning network_data for node", "node", ironicNode.UUID)
+			}
+		}
+	}
+
 	ironicNode, success, result, err := p.tryUpdateNode(ironicNode, updater)
 	if !success {
 		return
diff --git a/pkg/provisioner/provisioner.go b/pkg/provisioner/provisioner.go
index b4a52d8e..fa2774d0 100644
--- a/pkg/provisioner/provisioner.go
+++ b/pkg/provisioner/provisioner.go
@@ -74,12 +74,13 @@ type PreprovisioningImage struct {
 }
 
 type ManagementAccessData struct {
-	BootMode              metal3api.BootMode
-	AutomatedCleaningMode metal3api.AutomatedCleaningMode
-	State                 metal3api.ProvisioningState
-	CurrentImage          *metal3api.Image
-	PreprovisioningImage  *PreprovisioningImage
-	HasCustomDeploy       bool
+	BootMode                   metal3api.BootMode
+	AutomatedCleaningMode      metal3api.AutomatedCleaningMode
+	State                      metal3api.ProvisioningState
+	CurrentImage               *metal3api.Image
+	PreprovisioningImage       *PreprovisioningImage
+	PreprovisioningNetworkData string
+	HasCustomDeploy            bool
 }
 
 type AdoptData struct {


Test patch:
diff --git a/controllers/metal3.io/host_config_data_test.go b/controllers/metal3.io/host_config_data_test.go
index 6014ed81..2d8fa468 100644
--- a/controllers/metal3.io/host_config_data_test.go
+++ b/controllers/metal3.io/host_config_data_test.go
@@ -92,17 +92,19 @@ func TestProvisionWithHostConfig(t *testing.T) {
 	testBMCSecret := newBMCCredsSecret(defaultSecretName, "User", "Pass")
 
 	testCases := []struct {
-		Scenario                 string
-		Host                     *metal3api.BareMetalHost
-		UserDataSecret           *corev1.Secret
-		PreprovNetworkDataSecret *corev1.Secret
-		NetworkDataSecret        *corev1.Secret
-		ExpectedUserData         string
-		ErrUserData              bool
-		ExpectedNetworkData      string
-		ErrNetworkData           bool
-		ExpectedMetaData         string
-		ErrMetaData              bool
+		Scenario                           string
+		Host                               *metal3api.BareMetalHost
+		UserDataSecret                     *corev1.Secret
+		PreprovNetworkDataSecret           *corev1.Secret
+		NetworkDataSecret                  *corev1.Secret
+		ExpectedUserData                   string
+		ErrUserData                        bool
+		ExpectedNetworkData                string
+		ErrNetworkData                     bool
+		ExpectedPreprovisioningNetworkData string
+		ErrPreprovisioningNetworkData      bool
+		ExpectedMetaData                   string
+		ErrMetaData                        bool
 	}{
 		{
 			Scenario: "host with user data only",
@@ -151,11 +153,12 @@ func TestProvisionWithHostConfig(t *testing.T) {
 					},
 					PreprovisioningNetworkDataName: "net-data",
 				}),
-			NetworkDataSecret:   newSecret("net-data", map[string]string{"networkData": "key: value"}),
-			ExpectedUserData:    "",
-			ErrUserData:         false,
-			ExpectedNetworkData: base64.StdEncoding.EncodeToString([]byte("key: value")),
-			ErrNetworkData:      false,
+			NetworkDataSecret:                  newSecret("net-data", map[string]string{"networkData": "key: value"}),
+			ExpectedUserData:                   "",
+			ErrUserData:                        false,
+			ExpectedNetworkData:                base64.StdEncoding.EncodeToString([]byte("key: value")),
+			ExpectedPreprovisioningNetworkData: base64.StdEncoding.EncodeToString([]byte("key: value")),
+			ErrNetworkData:                     false,
 		},
 		{
 			Scenario: "host with preprov and regular network data",
@@ -165,17 +168,19 @@ func TestProvisionWithHostConfig(t *testing.T) {
 						Address:         "ipmi://192.168.122.1:6233",
 						CredentialsName: defaultSecretName,
 					},
-					PreprovisioningNetworkDataName: "preprov-net-data",
+					PreprovisioningNetworkDataName: "net-data2",
 					NetworkData: &corev1.SecretReference{
 						Name:      "net-data",
 						Namespace: namespace,
 					},
 				}),
-			NetworkDataSecret:   newSecret("net-data", map[string]string{"networkData": "key: value"}),
-			ExpectedUserData:    "",
-			ErrUserData:         false,
-			ExpectedNetworkData: base64.StdEncoding.EncodeToString([]byte("key: value")),
-			ErrNetworkData:      false,
+			NetworkDataSecret:                  newSecret("net-data", map[string]string{"networkData": "key: value"}),
+			PreprovNetworkDataSecret:           newSecret("net-data2", map[string]string{"networkData": "key: value2"}),
+			ExpectedUserData:                   "",
+			ErrUserData:                        false,
+			ExpectedNetworkData:                base64.StdEncoding.EncodeToString([]byte("key: value")),
+			ExpectedPreprovisioningNetworkData: base64.StdEncoding.EncodeToString([]byte("key: value2")),
+			ErrNetworkData:                     false,
 		},
 		{
 			Scenario: "host with network data only",
@@ -336,6 +341,7 @@ func TestProvisionWithHostConfig(t *testing.T) {
 			c.Create(goctx.TODO(), testBMCSecret)
 			c.Create(goctx.TODO(), tc.UserDataSecret)
 			c.Create(goctx.TODO(), tc.NetworkDataSecret)
+			c.Create(goctx.TODO(), tc.PreprovNetworkDataSecret)
 			baselog := ctrl.Log.WithName("controllers").WithName("BareMetalHost")
 			hcd := &hostConfigData{
 				host:          tc.Host,
@@ -358,7 +364,16 @@ func TestProvisionWithHostConfig(t *testing.T) {
 			}
 
 			if actualNetworkData != tc.ExpectedNetworkData {
-				t.Fatal(fmt.Errorf("Failed to assert NetworkData. Expected '%s' got '%s'", actualNetworkData, tc.ExpectedNetworkData))
+				t.Fatal(fmt.Errorf("Failed to assert NetworkData. Expected '%s' got '%s'", tc.ExpectedNetworkData, actualNetworkData))
+			}
+
+			actualPreprovisioningNetworkData, err := hcd.PreprovisioningNetworkData()
+			if err != nil && !tc.ErrPreprovisioningNetworkData {
+				t.Fatal(err)
+			}
+
+			if actualPreprovisioningNetworkData != tc.ExpectedPreprovisioningNetworkData {
+				t.Fatal(fmt.Errorf("Failed to assert PreprovisioningNetworkData. Expected '%s' got '%s'", tc.ExpectedPreprovisioningNetworkData, actualPreprovisioningNetworkData))
 			}
 
 			actualMetaData, err := hcd.MetaData()