Repository: metal3-io/baremetal-operator
Pull request #826 (2021-03-23 13:28:49)

Handle hardwaredetails annotation value error
Currently we only handle the case where the json fails to parse
e.g is invalid json, but we silently ignore any value which is
valid data but fails to unpack into the HardwareDetails object.

In this case we should avoid removing the annotation since it's
not been copied into the status.hardware, and return an error
so the user can detect that the json data wasn't in the expected
format.

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/826#" title="Author self-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":[]} -->
hardys: /test-integration
hardys: @tsorya FYI this should catch the issue you encountered where the json was valid, but the format was not
hardys: @andfasano @zaneb PTAL - thanks!

Patch:
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index ddfce81e..1f2b7c54 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -274,7 +274,7 @@ func (r *BareMetalHostReconciler) updateHardwareDetails(request ctrl.Request, ho
 	if host.Status.HardwareDetails == nil || inspectionDisabled(host) {
 		objHardwareDetails, err := r.getHardwareDetailsFromAnnotation(host)
 		if err != nil {
-			return updated, errors.Wrap(err, "Error getting HardwareDetails from annotation")
+			return updated, errors.Wrap(err, "Error parsing HardwareDetails from annotation")
 		}
 		if objHardwareDetails != nil {
 			host.Status.HardwareDetails = objHardwareDetails
@@ -1038,9 +1038,10 @@ func (r *BareMetalHostReconciler) getHardwareDetailsFromAnnotation(host *metal3v
 	if annotations[hardwareDetailsAnnotation] == "" {
 		return nil, nil
 	}
-	content := []byte(annotations[hardwareDetailsAnnotation])
 	objHardwareDetails := &metal3v1alpha1.HardwareDetails{}
-	if err := json.Unmarshal(content, objHardwareDetails); err != nil {
+	decoder := json.NewDecoder(strings.NewReader(annotations[hardwareDetailsAnnotation]))
+	decoder.DisallowUnknownFields()
+	if err := decoder.Decode(objHardwareDetails); err != nil {
 		return nil, err
 	}
 	return objHardwareDetails, nil


Test patch:
diff --git a/controllers/metal3.io/baremetalhost_controller_test.go b/controllers/metal3.io/baremetalhost_controller_test.go
index 31746d2e..8ae29f4d 100644
--- a/controllers/metal3.io/baremetalhost_controller_test.go
+++ b/controllers/metal3.io/baremetalhost_controller_test.go
@@ -267,6 +267,28 @@ func TestHardwareDetails_StatusPresentInspectDisabled(t *testing.T) {
 	)
 }
 
+// TestHardwareDetails_Invalid
+// Tests scenario where the hardwaredetails value is invalid
+func TestHardwareDetails_Invalid(t *testing.T) {
+	host := newDefaultHost(t)
+	badAnnotation := fmt.Sprintf("{\"hardware\": %s}", hwdAnnotation)
+	host.Annotations = map[string]string{
+		inspectAnnotationPrefix:   "disabled",
+		hardwareDetailsAnnotation: badAnnotation,
+	}
+	time := metav1.Now()
+	host.Status.LastUpdated = &time
+	hwd := metal3v1alpha1.HardwareDetails{}
+	hwd.Hostname = "existinghost"
+	host.Status.HardwareDetails = &hwd
+
+	r := newTestReconciler(host)
+	request := newRequest(host)
+	_, err := r.Reconcile(context.Background(), request)
+	expectedErr := "json: unknown field"
+	assert.Contains(t, err.Error(), expectedErr)
+}
+
 // TestStatusAnnotation_EmptyStatus ensures that status is manually populated
 // when status annotation is present and status field is empty.
 func TestStatusAnnotation_EmptyStatus(t *testing.T) {