Possible nil-deref in image-automation controller

[dholbach]

From Ada Logics:

Consider the following code:

image-automation-controller/controllers/imageupdateautomation_controller.go

Lines 310 to 320 in 674f833

	if rev, err := commitChangedManifests(tracelog, repo, tmp, signingEntity, author, message); err != nil {
	if err == errNoChanges {
	r.event(ctx, auto, events.EventSeverityInfo, "no updates made")
	debuglog.Info("no changes made in working directory; no commit")
	statusMessage = "no updates made"
	if lastCommit, lastTime := auto.Status.LastPushCommit, auto.Status.LastPushTime; lastCommit != "" {
	statusMessage = fmt.Sprintf("%s; last commit %s at %s", statusMessage, lastCommit[:7], lastTime.Format(time.RFC3339))
	}
	} else {
	return failWithError(err)
	}

When the lastTime.Format is called it is not certain that lastTime is not a nil-pointer. As such, similar to a check on lastCommit there should be a check on whether lastTime is nil.

[Nalum]

I'd like to take up this issue if that's good with you

[Nalum]

At the moment there is no error message returned if lastCommit is an empty string, is this okay to stay this way or should there be an else failWithError at this point?

I guess the statusMessage would still be no updates made which is still relevant in that case?

[Nalum]

If lastCommit is not an empty string but lastTime is nil do you still want lastCommit in statusMessage?

I feel like it's unlikely to happen that you would have one without the other but it is possible.

[hiddeco]

I feel the presence of both is required for the observation to be correct.