Repository: metal3-io/baremetal-operator
Pull request #900 (2021-05-31 11:09:16)

Fix configdrive handling
I've spotted a few errors with the configdrive handling:

* We forgot to add configdrive to 'deploy failed' handler, so a repeated
  deployment will always happen without a configdrive.
* ConfigDrive creation is conditional on UserData presence. This is
  incorrect, any field being populated should trigger configdrive.
* There was no check on empty networkData, so we always tried to
  deserialize empty string with YAML (which works, but is incorrect).

Create a new helper for configdrive and cover it with unit tests.

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

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please ask for approval from **dtantsur** after the PR has been reviewed.

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/master/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":["dtantsur"]} -->
dtantsur: /test-integration
dtantsur: /test-integration
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/900#pullrequestreview-677338702" title="Approved">elfosardo</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please ask for approval from **dtantsur** after the PR has been reviewed.

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/master/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":["dtantsur"]} -->
hardys: Overall lgtm but I think the revised logic still isn't quite correct, because we'll only pass the default meta-data when one of (meta|network|user)Data is specified?

Previously we only passed it when userData was specified, which I agree was incorrect, but I think the desired behavior is to always pass this metaData regardless of the user-provided data?

dtantsur: > I think the desired behavior is to always pass this metaData regardless of the user-provided data?

I don't really know. It's a serious user-visible change. Could I follow-up with it, so that it can be reviewed separately (and reverted if things go south)?
dtantsur: I'll bring it to the meeting today. If there are no objections, I'll update the PR.

Patch:
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index c5ce111a..3151d7fc 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -1239,6 +1239,59 @@ func (p *ironicProvisioner) Prepare(data provisioner.PrepareData, unprepared boo
 	return
 }
 
+func (p *ironicProvisioner) getConfigDrive(data provisioner.ProvisionData) (configDrive nodes.ConfigDrive, err error) {
+	// Retrieve instance specific user data (cloud-init, ignition, etc).
+	userData, err := data.HostConfig.UserData()
+	if err != nil {
+		return configDrive, errors.Wrap(err, "could not retrieve user data")
+	}
+	if userData != "" {
+		configDrive.UserData = userData
+	}
+
+	// Retrieve OpenStack network_data. Default value is empty.
+	networkDataRaw, err := data.HostConfig.NetworkData()
+	if err != nil {
+		return configDrive, errors.Wrap(err, "could not retrieve network data")
+	}
+	if networkDataRaw != "" {
+		var networkData map[string]interface{}
+		if err = yaml.Unmarshal([]byte(networkDataRaw), &networkData); err != nil {
+			return configDrive, errors.Wrap(err, "failed to unmarshal network_data.json from secret")
+		}
+		configDrive.NetworkData = networkData
+	}
+
+	// Retrieve meta data with fallback to defaults from provisioner.
+	metaData := map[string]interface{}{
+		"uuid":             string(p.objectMeta.UID),
+		"metal3-namespace": p.objectMeta.Namespace,
+		"metal3-name":      p.objectMeta.Name,
+		"local-hostname":   p.objectMeta.Name,
+		"local_hostname":   p.objectMeta.Name,
+		"name":             p.objectMeta.Name,
+	}
+	metaDataRaw, err := data.HostConfig.MetaData()
+	if err != nil {
+		return configDrive, errors.Wrap(err, "could not retrieve metadata")
+	}
+	if metaDataRaw != "" {
+		if err = yaml.Unmarshal([]byte(metaDataRaw), &metaData); err != nil {
+			return configDrive, errors.Wrap(err, "failed to unmarshal metadata from secret")
+		}
+	}
+
+	// Set metaData if any field is populated by a user.
+	if metaDataRaw != "" || networkDataRaw != "" || userData != "" {
+		configDrive.MetaData = metaData
+		p.log.Info("triggering provisioning with config drive")
+	} else {
+		p.log.Info("triggering provisioning without config drive")
+	}
+
+	return
+}
+
 // Provision writes the image from the host spec to the host. It may
 // be called multiple times, and should return true for its dirty flag
 // until the deprovisioning operation is completed.
@@ -1277,8 +1330,18 @@ func (p *ironicProvisioner) Provision(data provisioner.ProvisionData) (result pr
 			return provResult, err
 		}
 
-		return p.changeNodeProvisionState(ironicNode,
-			nodes.ProvisionStateOpts{Target: nodes.TargetActive})
+		configDrive, err := p.getConfigDrive(data)
+		if err != nil {
+			return transientError(err)
+		}
+
+		return p.changeNodeProvisionState(
+			ironicNode,
+			nodes.ProvisionStateOpts{
+				Target:      nodes.TargetActive,
+				ConfigDrive: configDrive,
+			},
+		)
 
 	case nodes.Manageable:
 		return p.changeNodeProvisionState(ironicNode,
@@ -1303,54 +1366,9 @@ func (p *ironicProvisioner) Provision(data provisioner.ProvisionData) (result pr
 		// setting the state to "active".
 		p.log.Info("making host active")
 
-		// Retrieve cloud-init user data
-		userData, err := data.HostConfig.UserData()
-		if err != nil {
-			return transientError(errors.Wrap(err, "could not retrieve user data"))
-		}
-
-		// Retrieve cloud-init network_data.json. Default value is empty
-		networkDataRaw, err := data.HostConfig.NetworkData()
-		if err != nil {
-			return transientError(errors.Wrap(err, "could not retrieve network data"))
-		}
-		var networkData map[string]interface{}
-		if err = yaml.Unmarshal([]byte(networkDataRaw), &networkData); err != nil {
-			return transientError(errors.Wrap(err, "failed to unmarshal network_data.json from secret"))
-		}
-
-		// Retrieve cloud-init meta_data.json with falback to default
-		metaData := map[string]interface{}{
-			"uuid":             string(p.objectMeta.UID),
-			"metal3-namespace": p.objectMeta.Namespace,
-			"metal3-name":      p.objectMeta.Name,
-			"local-hostname":   p.objectMeta.Name,
-			"local_hostname":   p.objectMeta.Name,
-			"name":             p.objectMeta.Name,
-		}
-		metaDataRaw, err := data.HostConfig.MetaData()
+		configDrive, err := p.getConfigDrive(data)
 		if err != nil {
-			return transientError(errors.Wrap(err, "could not retrieve metadata"))
-		}
-		if metaDataRaw != "" {
-			if err = yaml.Unmarshal([]byte(metaDataRaw), &metaData); err != nil {
-				return transientError(errors.Wrap(err, "failed to unmarshal metadata from secret"))
-			}
-		}
-
-		var configDrive nodes.ConfigDrive
-		if userData != "" {
-			configDrive = nodes.ConfigDrive{
-				UserData:    userData,
-				MetaData:    metaData,
-				NetworkData: networkData,
-			}
-			if err != nil {
-				return transientError(errors.Wrap(err, "failed to build config drive"))
-			}
-			p.log.Info("triggering provisioning with config drive")
-		} else {
-			p.log.Info("triggering provisioning without config drive")
+			return transientError(err)
 		}
 
 		return p.changeNodeProvisionState(


Test patch:
diff --git a/pkg/provisioner/ironic/configdrive_test.go b/pkg/provisioner/ironic/configdrive_test.go
new file mode 100644
index 00000000..3a41c0fc
--- /dev/null
+++ b/pkg/provisioner/ironic/configdrive_test.go
@@ -0,0 +1,134 @@
+package ironic
+
+import (
+	"testing"
+
+	"github.com/gophercloud/gophercloud/openstack/baremetal/v1/nodes"
+	"github.com/gophercloud/gophercloud/openstack/baremetalintrospection/v1/introspection"
+	"github.com/stretchr/testify/assert"
+
+	"github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
+	"github.com/metal3-io/baremetal-operator/pkg/bmc"
+	"github.com/metal3-io/baremetal-operator/pkg/provisioner"
+	"github.com/metal3-io/baremetal-operator/pkg/provisioner/fixture"
+	"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/clients"
+	"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/testserver"
+)
+
+func TestEmpty(t *testing.T) {
+	nodeUUID := "33ce8659-7400-4c68-9535-d10766f07a58"
+
+	cases := []struct {
+		name     string
+		hostData provisioner.HostConfigData
+		expected nodes.ConfigDrive
+	}{
+		{
+			name:     "empty",
+			hostData: fixture.NewHostConfigData("", "", ""),
+			expected: nodes.ConfigDrive{},
+		},
+		{
+			name:     "everything",
+			hostData: fixture.NewHostConfigData("testUserData", "test: NetworkData", "test: Meta"),
+			expected: nodes.ConfigDrive{
+				MetaData: map[string]interface{}{
+					"local-hostname":   "myhost",
+					"local_hostname":   "myhost",
+					"metal3-name":      "myhost",
+					"metal3-namespace": "myns",
+					"name":             "myhost",
+					"test":             "Meta",
+				},
+				NetworkData: map[string]interface{}{
+					"test": "NetworkData",
+				},
+				UserData: "testUserData",
+			},
+		},
+		{
+			name:     "only network data",
+			hostData: fixture.NewHostConfigData("", "test: NetworkData", ""),
+			expected: nodes.ConfigDrive{
+				MetaData: map[string]interface{}{
+					"local-hostname":   "myhost",
+					"local_hostname":   "myhost",
+					"metal3-name":      "myhost",
+					"metal3-namespace": "myns",
+					"name":             "myhost",
+				},
+				NetworkData: map[string]interface{}{
+					"test": "NetworkData",
+				},
+			},
+		},
+		{
+			name:     "only user data",
+			hostData: fixture.NewHostConfigData("testUserData", "", ""),
+			expected: nodes.ConfigDrive{
+				MetaData: map[string]interface{}{
+					"local-hostname":   "myhost",
+					"local_hostname":   "myhost",
+					"metal3-name":      "myhost",
+					"metal3-namespace": "myns",
+					"name":             "myhost",
+				},
+				UserData: "testUserData",
+			},
+		},
+		{
+			name:     "only meta data",
+			hostData: fixture.NewHostConfigData("", "", "test: Meta"),
+			expected: nodes.ConfigDrive{
+				MetaData: map[string]interface{}{
+					"local-hostname":   "myhost",
+					"local_hostname":   "myhost",
+					"metal3-name":      "myhost",
+					"metal3-namespace": "myns",
+					"name":             "myhost",
+					"test":             "Meta",
+				},
+			},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			ironic := testserver.NewIronic(t).Ready().Node(nodes.Node{
+				ProvisionState: string(nodes.Active),
+				UUID:           nodeUUID,
+			})
+			ironic.Start()
+			defer ironic.Stop()
+
+			inspector := testserver.NewInspector(t).Ready().WithIntrospection(nodeUUID, introspection.Introspection{
+				Finished: false,
+			})
+			inspector.Start()
+			defer inspector.Stop()
+
+			host := makeHost()
+			host.Status.Provisioning.ID = nodeUUID
+			publisher := func(reason, message string) {}
+			auth := clients.AuthConfig{Type: clients.NoAuth}
+			prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, publisher,
+				ironic.Endpoint(), auth, inspector.Endpoint(), auth,
+			)
+			if err != nil {
+				t.Fatalf("could not create provisioner: %s", err)
+			}
+
+			result, err := prov.getConfigDrive(provisioner.ProvisionData{
+				HostConfig: tc.hostData,
+				BootMode:   v1alpha1.DefaultBootMode,
+			})
+
+			if len(tc.expected.MetaData) > 0 {
+				tc.expected.MetaData["uuid"] = string(prov.objectMeta.UID)
+			}
+
+			assert.Equal(t, tc.expected, result)
+			assert.NoError(t, err)
+		})
+	}
+}