Repository: metal3-io/baremetal-operator
Pull request #838 (2021-03-29 14:29:00)

Fix error reporting when creating an host with invalid BMC url
This PR addresses the problem reported in https://github.com/metal3-io/baremetal-operator/issues/843.

Note that it completes the changes introduced by @zaneb in [beea4d0](https://github.com/metal3-io/baremetal-operator/commit/beea4d0ead807a8f19b38d538db3502ee3504b97) which already fixed the problem of deleting a BMH resource being created with a malformed BMC address (such issue originally also addressed by @kirankt in #713)

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/838#" title="Author self-approved">andfasano</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **maelk**
You can assign the PR to them by writing `/assign @maelk` in a comment when ready.

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":["maelk"]} -->
metal3-io-bot: @andfasano: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
unit | c34532d08233c806533ebd0266eb7e13ac7e2794 | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/838/unit/1376542313701969921) | `/test unit`



<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
andfasano: /test unit
metal3-io-bot: @andfasano: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
unit | c34532d08233c806533ebd0266eb7e13ac7e2794 | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/838/unit/1376559924661719040) | `/test unit`



<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
andfasano: > I think this bug is actually fixed already by [beea4d0](https://github.com/metal3-io/baremetal-operator/commit/beea4d0ead807a8f19b38d538db3502ee3504b97) (which merged today).
> If we get a dodgy BMC address we will now just record a registration error for the Host instead of failing to create a provisioner. That should mean that we are able to do deletes as normal.

Thanks @zaneb, I've modified accordingly the PR description and code to just keep the new test
metal3-io-bot: @andfasano: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
generate | f96843bacbc5c8692b10a069e3d931718b558485 | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/838/generate/1377311513282351105) | `/test generate`



<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
andfasano: /test-integration
andfasano: /test-integration
zaneb: Excellent, thanks Andrea!
/lgtm
hardys: /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/838#" title="Author self-approved">andfasano</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/838#issuecomment-814712999" title="Approved">hardys</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/master/OWNERS)~~ [hardys]

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":[]} -->
metal3-io-bot: @andfasano: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
golint | 60b85a4e3f69e46d3497a1099cd4d198af94566e | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/838/golint/1379711749053222917) | `/test golint`



<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
metal3-io-bot: @andfasano: The following tests **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
golint | 60b85a4e3f69e46d3497a1099cd4d198af94566e | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/838/golint/1379711749053222917) | `/test golint`
unit | 60b85a4e3f69e46d3497a1099cd4d198af94566e | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/838/unit/1379711749053222921) | `/test unit`



<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
metal3-io-bot: @andfasano: The following tests **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
golint | 60b85a4e3f69e46d3497a1099cd4d198af94566e | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/838/golint/1379711749053222917) | `/test golint`
unit | 60b85a4e3f69e46d3497a1099cd4d198af94566e | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/838/unit/1379711749053222921) | `/test unit`
govet | 60b85a4e3f69e46d3497a1099cd4d198af94566e | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/838/govet/1379711749053222913) | `/test govet`



<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
metal3-io-bot: New changes are detected. LGTM label has been removed.

Patch:
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index 7611d7d3..01f3ecfd 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -1183,15 +1183,6 @@ func (r *BareMetalHostReconciler) buildAndValidateBMCCredentials(request ctrl.Re
 		return nil, nil, &EmptyBMCAddressError{message: "Missing BMC connection detail 'Address'"}
 	}
 
-	// pass the bmc address to bmc.NewAccessDetails which will do
-	// more in-depth checking on the url to ensure it is
-	// a valid bmc address, returning a bmc.UnknownBMCTypeError
-	// if it is not conformant
-	_, err = bmc.NewAccessDetails(host.Spec.BMC.Address, host.Spec.BMC.DisableCertificateVerification)
-	if err != nil {
-		return nil, nil, err
-	}
-
 	bmcCreds = &bmc.Credentials{
 		Username: string(bmcCredsSecret.Data["username"]),
 		Password: string(bmcCredsSecret.Data["password"]),
diff --git a/pkg/provisioner/fixture/fixture.go b/pkg/provisioner/fixture/fixture.go
index 75f3e8fd..ba3ff816 100644
--- a/pkg/provisioner/fixture/fixture.go
+++ b/pkg/provisioner/fixture/fixture.go
@@ -69,6 +69,8 @@ type Fixture struct {
 	image metal3v1alpha1.Image
 	// state to manage power
 	poweredOn bool
+
+	validateError string
 }
 
 // New returns a new Fixture Provisioner
@@ -83,6 +85,10 @@ func (f *Fixture) New(hostData provisioner.HostData, publisher provisioner.Event
 	return p, nil
 }
 
+func (f *Fixture) SetValidateError(message string) {
+	f.validateError = message
+}
+
 func (p *fixtureProvisioner) HasProvisioningCapacity() (result bool, err error) {
 	return true, nil
 }
@@ -92,6 +98,11 @@ func (p *fixtureProvisioner) HasProvisioningCapacity() (result bool, err error)
 func (p *fixtureProvisioner) ValidateManagementAccess(data provisioner.ManagementAccessData, credentialsChanged, force bool) (result provisioner.Result, provID string, err error) {
 	p.log.Info("testing management access")
 
+	if p.state.validateError != "" {
+		result.ErrorMessage = p.state.validateError
+		return
+	}
+
 	// Fill in the ID of the host in the provisioning system
 	if p.provID == "" {
 		provID = "temporary-fake-id"


Test patch:
diff --git a/controllers/metal3.io/baremetalhost_controller_test.go b/controllers/metal3.io/baremetalhost_controller_test.go
index ebaef87c..a62b4587 100644
--- a/controllers/metal3.io/baremetalhost_controller_test.go
+++ b/controllers/metal3.io/baremetalhost_controller_test.go
@@ -827,18 +827,6 @@ func TestMissingBMCParameters(t *testing.T) {
 				}),
 		},
 
-		{
-			Scenario: "malformed address",
-			Secret:   newBMCCredsSecret("bmc-creds-ok", "User", "Pass"),
-			Host: newHost("invalid-bmc-address",
-				&metal3v1alpha1.BareMetalHostSpec{
-					BMC: metal3v1alpha1.BMCDetails{
-						Address:         "unknown://notAvalidIPMIURL",
-						CredentialsName: "bmc-creds-ok",
-					},
-				}),
-		},
-
 		{
 			Scenario: "missing address",
 			Secret:   newBMCCredsSecret("bmc-creds-ok", "User", "Pass"),
@@ -1964,3 +1952,30 @@ func TestUpdateRAID(t *testing.T) {
 		})
 	}
 }
+
+func doDeleteHost(host *metal3v1alpha1.BareMetalHost, reconciler *BareMetalHostReconciler) {
+	now := metav1.Now()
+	host.DeletionTimestamp = &now
+	reconciler.Client.Update(context.Background(), host)
+}
+
+func TestInvalidBMHCanBeDeleted(t *testing.T) {
+	host := newDefaultHost(t)
+	host.Spec.BMC.Address = fmt.Sprintf("%s%s%s", "<", host.Spec.BMC.Address, ">")
+
+	var fix fixture.Fixture
+	r := newTestReconcilerWithFixture(&fix, host)
+
+	fix.SetValidateError("malformed url")
+	waitForError(t, r, host)
+	assert.Equal(t, metal3v1alpha1.StateRegistering, host.Status.Provisioning.State)
+	assert.Equal(t, metal3v1alpha1.OperationalStatusError, host.Status.OperationalStatus)
+	assert.Equal(t, metal3v1alpha1.RegistrationError, host.Status.ErrorType)
+	assert.Equal(t, "malformed url", host.Status.ErrorMessage)
+
+	doDeleteHost(host, r)
+
+	tryReconcile(t, r, host, func(host *metal3v1alpha1.BareMetalHost, result reconcile.Result) bool {
+		return host.Status.Provisioning.State == metal3v1alpha1.StateDeleting && len(host.Finalizers) == 0
+	})
+}
diff --git a/pkg/provisioner/ironic/validatemanagementaccess_test.go b/pkg/provisioner/ironic/validatemanagementaccess_test.go
index c43a581a..b9242d73 100644
--- a/pkg/provisioner/ironic/validatemanagementaccess_test.go
+++ b/pkg/provisioner/ironic/validatemanagementaccess_test.go
@@ -638,3 +638,28 @@ func TestValidateManagementAccessNoBMCDetails(t *testing.T) {
 	}
 	assert.Equal(t, "failed to parse BMC address information: missing BMC address", result.ErrorMessage)
 }
+
+func TestValidateManagementAccessMalformedBMCAddress(t *testing.T) {
+	ironic := testserver.NewIronic(t).Ready()
+	ironic.Start()
+	defer ironic.Stop()
+
+	host := makeHost()
+	host.Spec.BMC = metal3v1alpha1.BMCDetails{
+		Address: "<ipmi://192.168.122.1:6233>",
+	}
+
+	auth := clients.AuthConfig{Type: clients.NoAuth}
+	prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, nullEventPublisher,
+		ironic.Endpoint(), auth, testserver.NewInspector(t).Endpoint(), auth,
+	)
+	if err != nil {
+		t.Fatalf("could not create provisioner: %s", err)
+	}
+
+	result, _, err := prov.ValidateManagementAccess(provisioner.ManagementAccessData{}, false, false)
+	if err != nil {
+		t.Fatalf("error from ValidateManagementAccess: %s", err)
+	}
+	assert.Equal(t, "failed to parse BMC address information: failed to parse BMC address information: parse \"<ipmi://192.168.122.1:6233>\": first path segment in URL cannot contain colon", result.ErrorMessage)
+}