Repository: metal3-io/baremetal-operator
Pull request #1162 (2022-08-24 09:04:51)

Set external_http_url on the node when BMC is IPv6
When the BMC address is IPv6, we need to set the `external_http_url`
value on the node so that the node can access the provisioning image.

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 **honza** 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/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":["honza"]} -->
iurygregory: /test-centos-integration-main
/test-ubuntu-integration-main
dtantsur: Left a couple of comments, but I'm fine with a follow-up.

/approve
metal3-io-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/1162#issuecomment-1245645216" title="Approved">dtantsur</a>*

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

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

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

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

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":[]} -->

Patch:
diff --git a/OWNERS b/OWNERS
index 9260cf5f..d1d22bc0 100644
--- a/OWNERS
+++ b/OWNERS
@@ -3,6 +3,7 @@
 approvers:
  - andfasano
  - dtantsur
+ - fmuyassarov
  - honza
  - zaneb
 
@@ -19,4 +20,3 @@ reviewers:
 emeritus_approvers:
  - maelk
  - hardys
- - fmuyassarov
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index 65a9f1da..f742bbaf 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -129,6 +129,28 @@ func (r *BareMetalHostReconciler) Reconcile(ctx context.Context, request ctrl.Re
 		// Error reading the object - requeue the request.
 		return ctrl.Result{}, errors.Wrap(err, "could not load host data")
 	}
+	// Fetch the HardwareData
+	hardwareData := &metal3v1alpha1.HardwareData{}
+	hardwareDataKey := client.ObjectKey{
+		Name:      host.Name,
+		Namespace: host.Namespace,
+	}
+	err = r.Get(ctx, hardwareDataKey, hardwareData)
+	if err != nil && !k8serrors.IsNotFound(err) {
+		reqLogger.Error(err, "failed to find hardwareData")
+	}
+
+	// Host is being deleted, so we delete the finalizer from the hardwareData to allow its deletion.
+	if !host.DeletionTimestamp.IsZero() {
+		if controllerutil.ContainsFinalizer(hardwareData, hardwareDataFinalizer) {
+			controllerutil.RemoveFinalizer(hardwareData, hardwareDataFinalizer)
+			reqLogger.Info("removing finalizer from hardwareData")
+			if err := r.Update(context.Background(), hardwareData); err != nil {
+				return ctrl.Result{}, errors.Wrap(err, "failed to remove hardwareData finalizer")
+			}
+		}
+		reqLogger.Info("hardwareData is ready to be deleted")
+	}
 
 	// If the reconciliation is paused, requeue
 	annotations := host.GetAnnotations()
@@ -139,10 +161,44 @@ func (r *BareMetalHostReconciler) Reconcile(ctx context.Context, request ctrl.Re
 		}
 	}
 
-	hostData, err := r.reconciletHostData(ctx, host, request)
-	if err != nil {
-		return ctrl.Result{}, errors.Wrap(err, "Could not reconcile host data")
-	} else if hostData.Requeue {
+	// Check if Status is empty and status annotation is present
+	// Manually restore data.
+	if !r.hostHasStatus(host) {
+		reqLogger.Info("Reconstructing Status from hardwareData and annotation")
+		objStatus, err := r.getHostStatusFromAnnotation(host)
+
+		if err == nil && objStatus != nil {
+			// hardwareData takes predence over statusAnnotation data
+			if hardwareData.Spec.HardwareDetails != nil && objStatus.HardwareDetails != hardwareData.Spec.HardwareDetails {
+				objStatus.HardwareDetails = hardwareData.Spec.HardwareDetails
+			}
+
+			host.Status = *objStatus
+			if host.Status.LastUpdated.IsZero() {
+				// Ensure the LastUpdated timestamp is set to avoid
+				// infinite loops if the annotation only contained
+				// part of the status information.
+				t := metav1.Now()
+				host.Status.LastUpdated = &t
+			}
+			errStatus := r.Status().Update(ctx, host)
+			if errStatus != nil {
+				return ctrl.Result{}, errors.Wrap(errStatus, "Could not restore status from annotation")
+			}
+			return ctrl.Result{Requeue: true}, nil
+		}
+		reqLogger.Info("No status cache found")
+
+	}
+	// The status annotation is unneeded, as the status subresource is
+	// already present. The annotation data will get outdated, so remove it.
+	if _, present := annotations[metal3v1alpha1.StatusAnnotation]; present {
+		delete(annotations, metal3v1alpha1.StatusAnnotation)
+		errStatus := r.Update(ctx, host)
+		if errStatus != nil {
+			return ctrl.Result{}, errors.Wrap(errStatus, "Could not delete status annotation")
+		}
+		reqLogger.Info("deleted status annotation")
 		return ctrl.Result{Requeue: true}, nil
 	}
 
@@ -1693,75 +1749,3 @@ func (r *BareMetalHostReconciler) SetupWithManager(mgr ctrl.Manager, preprovImgE
 
 	return controller.Complete(r)
 }
-
-func (r *BareMetalHostReconciler) reconciletHostData(ctx context.Context, host *metal3v1alpha1.BareMetalHost, request ctrl.Request) (result ctrl.Result, err error) {
-
-	reqLogger := r.Log.WithValues("baremetalhost", request.NamespacedName)
-
-	// Fetch the HardwareData
-	hardwareData := &metal3v1alpha1.HardwareData{}
-	hardwareDataKey := client.ObjectKey{
-		Name:      host.Name,
-		Namespace: host.Namespace,
-	}
-	err = r.Get(ctx, hardwareDataKey, hardwareData)
-	if err != nil && !k8serrors.IsNotFound(err) {
-		reqLogger.Error(err, "failed to find hardwareData")
-	}
-
-	// Host is being deleted, so we delete the finalizer from the hardwareData to allow its deletion.
-	if !host.DeletionTimestamp.IsZero() {
-		if controllerutil.ContainsFinalizer(hardwareData, hardwareDataFinalizer) {
-			controllerutil.RemoveFinalizer(hardwareData, hardwareDataFinalizer)
-			reqLogger.Info("removing finalizer from hardwareData")
-			if err := r.Update(context.Background(), hardwareData); err != nil {
-				return ctrl.Result{}, errors.Wrap(err, "failed to remove hardwareData finalizer")
-			}
-		}
-		reqLogger.Info("hardwareData is ready to be deleted")
-	}
-
-	// Check if Status is empty and status annotation is present
-	// Manually restore data.
-	if !r.hostHasStatus(host) {
-		reqLogger.Info("Reconstructing Status from hardwareData and annotation")
-		objStatus, err := r.getHostStatusFromAnnotation(host)
-
-		if err == nil && objStatus != nil {
-			// hardwareData takes predence over statusAnnotation data
-			if hardwareData.Spec.HardwareDetails != nil && objStatus.HardwareDetails != hardwareData.Spec.HardwareDetails {
-				objStatus.HardwareDetails = hardwareData.Spec.HardwareDetails
-			}
-
-			host.Status = *objStatus
-			if host.Status.LastUpdated.IsZero() {
-				// Ensure the LastUpdated timestamp is set to avoid
-				// infinite loops if the annotation only contained
-				// part of the status information.
-				t := metav1.Now()
-				host.Status.LastUpdated = &t
-			}
-			errStatus := r.Status().Update(ctx, host)
-			if errStatus != nil {
-				return ctrl.Result{}, errors.Wrap(errStatus, "Could not restore status from annotation")
-			}
-			return ctrl.Result{Requeue: true}, nil
-		}
-		reqLogger.Info("No status cache found")
-
-	}
-	// The status annotation is unneeded, as the status subresource is
-	// already present. The annotation data will get outdated, so remove it.
-	annotations := host.GetAnnotations()
-	if _, present := annotations[metal3v1alpha1.StatusAnnotation]; present {
-		delete(annotations, metal3v1alpha1.StatusAnnotation)
-		errStatus := r.Update(ctx, host)
-		if errStatus != nil {
-			return ctrl.Result{}, errors.Wrap(errStatus, "Could not delete status annotation")
-		}
-		reqLogger.Info("deleted status annotation")
-		return ctrl.Result{Requeue: true}, nil
-	}
-	return ctrl.Result{}, nil
-
-}
diff --git a/docs/configuration.md b/docs/configuration.md
index f0a57b99..0651e6b6 100644
--- a/docs/configuration.md
+++ b/docs/configuration.md
@@ -44,6 +44,10 @@ but overflows could happen in case of slow provisioners and / or higher number o
 concurrent reconciles. For such reasons, it is highly recommended to keep
 BMO_CONCURRENCY value lower than the requested PROVISIONING_LIMIT. Default is 20.
 
+`IRONIC_EXTERNAL_URL_V6` -- This is the URL where Ironic will find the image for
+nodes that use IPv6. In dual stack environments, this can be used to tell Ironic which IP
+version it should set on the BMC.
+
 Kustomization Configuration
 ---------------------------
 
diff --git a/pkg/provisioner/ironic/factory.go b/pkg/provisioner/ironic/factory.go
index 5d3e10bf..5e9fdac3 100644
--- a/pkg/provisioner/ironic/factory.go
+++ b/pkg/provisioner/ironic/factory.go
@@ -3,6 +3,7 @@ package ironic
 import (
 	"errors"
 	"fmt"
+	"net/url"
 	"os"
 	"strconv"
 	"strings"
@@ -153,6 +154,17 @@ func loadConfigFromEnv(havePreprovImgBuilder bool) (ironicConfig, error) {
 		c.liveISOForcePersistentBootDevice = forcePersistentBootDevice
 	}
 
+	c.externalURL = os.Getenv("IRONIC_EXTERNAL_URL_V6")
+
+	// Let's see if externalURL looks like a URL
+	if c.externalURL != "" {
+		_, externalURLParseErr := url.Parse(c.externalURL)
+
+		if externalURLParseErr != nil {
+			return c, externalURLParseErr
+		}
+	}
+
 	return c, nil
 }
 
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 9c575752..bdc231c0 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -2,6 +2,7 @@ package ironic
 
 import (
 	"fmt"
+	"net"
 	"strings"
 	"time"
 
@@ -75,6 +76,7 @@ type ironicConfig struct {
 	deployISOURL                     string
 	liveISOForcePersistentBootDevice string
 	maxBusyHosts                     int
+	externalURL                      string
 }
 
 // Provisioner implements the provisioning.Provisioner interface
@@ -352,6 +354,7 @@ func (p *ironicProvisioner) ValidateManagementAccess(data provisioner.Management
 	}
 
 	driverInfo := bmcAccess.DriverInfo(p.bmcCreds)
+	driverInfo = setExternalURL(p, driverInfo)
 	deployImageInfo := setDeployImage(driverInfo, p.config, bmcAccess, data.PreprovisioningImage)
 
 	// If we have not found a node yet, we need to create one
@@ -580,6 +583,48 @@ func (p *ironicProvisioner) PreprovisioningImageFormats() ([]metal3v1alpha1.Imag
 	return formats, nil
 }
 
+func setExternalURL(p *ironicProvisioner, driverInfo map[string]interface{}) map[string]interface{} {
+	if _, ok := driverInfo["external_http_url"]; ok {
+		driverInfo["external_http_url"] = nil
+	}
+
+	if p.config.externalURL == "" {
+		return driverInfo
+	}
+
+	parsedURL, err := bmc.GetParsedURL(p.bmcAddress)
+	if err != nil {
+		p.log.Info("Failed to parse BMC address", "bmcAddress", p.bmcAddress, "err", err)
+		return driverInfo
+	}
+
+	ip := net.ParseIP(parsedURL.Hostname())
+	if ip == nil {
+		// Maybe it's a hostname?
+		ips, err := net.LookupIP(p.bmcAddress)
+		if err != nil {
+			p.log.Info("Failed to look up the IP address for BMC hostname", "hostname", p.bmcAddress)
+			return driverInfo
+		}
+
+		if len(ips) == 0 {
+			p.log.Info("Zero IP addresses for BMC hostname", "hostname", p.bmcAddress)
+			return driverInfo
+		}
+
+		ip = ips[0]
+	}
+
+	// In the case of IPv4, we don't have to do anything.
+	if ip.To4() != nil {
+		return driverInfo
+	}
+
+	driverInfo["external_http_url"] = p.config.externalURL
+
+	return driverInfo
+}
+
 func setDeployImage(driverInfo map[string]interface{}, config ironicConfig, accessDetails bmc.AccessDetails, hostImage *provisioner.PreprovisioningImage) optionsData {
 	deployImageInfo := optionsData{
 		deployKernelKey:  nil,


Test patch:
diff --git a/controllers/metal3.io/baremetalhost_controller_test.go b/controllers/metal3.io/baremetalhost_controller_test.go
index 74f11666..b04b2f75 100644
--- a/controllers/metal3.io/baremetalhost_controller_test.go
+++ b/controllers/metal3.io/baremetalhost_controller_test.go
@@ -171,13 +171,14 @@ func tryReconcile(t *testing.T, r *BareMetalHostReconciler, host *metal3v1alpha1
 	for i := 0; ; i++ {
 		t.Logf("tryReconcile: top of loop %d", i)
 		if i >= 25 {
-			t.Fatal("Exceeded 25 iterations")
+			t.Fatal(fmt.Errorf("Exceeded 25 iterations"))
 		}
 
 		result, err := r.Reconcile(context.Background(), request)
 
 		if err != nil {
 			t.Fatal(err)
+			break
 		}
 
 		// The FakeClient keeps a copy of the object we update, so we
@@ -197,7 +198,8 @@ func tryReconcile(t *testing.T, r *BareMetalHostReconciler, host *metal3v1alpha1
 
 		t.Logf("tryReconcile: loop bottom %d result=%v", i, result)
 		if !result.Requeue && result.RequeueAfter == 0 {
-			t.Fatalf("Ended reconcile at iteration %d without test condition being true", i)
+			t.Fatal(fmt.Errorf("Ended reconcile at iteration %d without test condition being true", i))
+			break
 		}
 	}
 }
diff --git a/pkg/provisioner/ironic/validatemanagementaccess_test.go b/pkg/provisioner/ironic/validatemanagementaccess_test.go
index 205718cc..5eb202a7 100644
--- a/pkg/provisioner/ironic/validatemanagementaccess_test.go
+++ b/pkg/provisioner/ironic/validatemanagementaccess_test.go
@@ -1148,3 +1148,98 @@ func TestSetDeployImage(t *testing.T) {
 		})
 	}
 }
+
+func TestSetExternalURL(t *testing.T) {
+	host := makeHost()
+	host.Spec.BMC.Address = "redfish-virtualmedia://[fe80::fc33:62ff:fe83:8a76]:6233"
+
+	ironic := testserver.NewIronic(t).Ready().
+		Node(nodes.Node{
+			Name: host.Namespace + nameSeparator + host.Name,
+			UUID: host.Status.Provisioning.ID,
+		}).NodeUpdate(nodes.Node{
+		UUID: host.Status.Provisioning.ID,
+	})
+
+	ironic.Start()
+	defer ironic.Stop()
+
+	auth := clients.AuthConfig{Type: clients.NoAuth}
+	prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, nil,
+		ironic.Endpoint(), auth, testserver.NewInspector(t).Endpoint(), auth,
+	)
+
+	if err != nil {
+		t.Fatalf("could not create provisioner: %s", err)
+	}
+
+	prov.config.externalURL = "XXX"
+
+	driverInfo := make(map[string]interface{}, 0)
+	updatedDriverInfo := setExternalURL(prov, driverInfo)
+
+	assert.Equal(t, "XXX", updatedDriverInfo["external_http_url"])
+}
+
+func TestSetExternalURLIPv4(t *testing.T) {
+	host := makeHost()
+	host.Spec.BMC.Address = "redfish-virtualmedia://1.1.1.1:1111"
+
+	ironic := testserver.NewIronic(t).Ready().
+		Node(nodes.Node{
+			Name: host.Namespace + nameSeparator + host.Name,
+			UUID: host.Status.Provisioning.ID,
+		}).NodeUpdate(nodes.Node{
+		UUID: host.Status.Provisioning.ID,
+	})
+
+	ironic.Start()
+	defer ironic.Stop()
+
+	auth := clients.AuthConfig{Type: clients.NoAuth}
+	prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, nil,
+		ironic.Endpoint(), auth, testserver.NewInspector(t).Endpoint(), auth,
+	)
+
+	if err != nil {
+		t.Fatalf("could not create provisioner: %s", err)
+	}
+
+	prov.config.externalURL = "XXX"
+
+	driverInfo := make(map[string]interface{}, 0)
+	updatedDriverInfo := setExternalURL(prov, driverInfo)
+
+	assert.Equal(t, nil, updatedDriverInfo["external_http_url"])
+}
+
+func TestSetExternalURLRemoving(t *testing.T) {
+	host := makeHost()
+	host.Spec.BMC.Address = "redfish-virtualmedia://1.1.1.1:1111"
+
+	ironic := testserver.NewIronic(t).Ready().
+		Node(nodes.Node{
+			Name: host.Namespace + nameSeparator + host.Name,
+			UUID: host.Status.Provisioning.ID,
+		}).NodeUpdate(nodes.Node{
+		UUID: host.Status.Provisioning.ID,
+	})
+
+	ironic.Start()
+	defer ironic.Stop()
+
+	auth := clients.AuthConfig{Type: clients.NoAuth}
+	prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, nil,
+		ironic.Endpoint(), auth, testserver.NewInspector(t).Endpoint(), auth,
+	)
+
+	if err != nil {
+		t.Fatalf("could not create provisioner: %s", err)
+	}
+
+	driverInfo := make(map[string]interface{}, 0)
+	driverInfo["external_http_url"] = "non-empty"
+	updatedDriverInfo := setExternalURL(prov, driverInfo)
+
+	assert.Equal(t, nil, updatedDriverInfo["external_http_url"])
+}