csi: account for nil volume/mount in API-to-structs conversion

[tgross]

Fixes #10834

Fix a nil pointer in the API struct to nomad/structs conversion when a
volume or volume_mount block is empty.

[jrasell]

I just tested this change locally using the repro in the linked issue and the same panic occurred. A little bit of poking makes me think the issue is related to Volume within the mount being nil, rather than mount itself which is populated with some default values. Do we need more validation around the VolumeMount to catch this?

[tgross]

I just tested this change locally using the repro in the linked issue and the same panic occurred. A little bit of poking makes me think the issue is related to Volume within the mount being nil, rather than mount itself which is populated with some default values. Do we need more validation around the VolumeMount to catch this?

That's embarrassing... I should've checked it end-to-end. 🤦

This is the default object we're getting for api.VolumeMount:

(*api.VolumeMount)(0xc0008e5d20)({
 Volume: (*string)(<nil>),
 Destination: (*string)(<nil>),
 ReadOnly: (*bool)(0xc000c46440)(false),
 PropagationMode: (*string)(0xc000b2ac30)((len=7) "private")
})

Unfortunately if I update that to catch nil for Volume, it just crashes at structs validation later because we have a (*structs.VolumeMount)(<nil>).

The problem is the premature optimization we're doing in setting up a slice of pre-allocated volume mounts. When we iterate over those later if any of them weren't "filled" we get out nil volume mounts! I'm pretty sure we're making the same mistake all over this function so I'll take a crack at fixing it everywhere.

[tgross]

@jrasell I've pushed up a first pass at fixing it across that file for your thoughts but need to take another look over to ensure we're catching all the possible nil blocks.

[tgross]

@jrasell I've tested this with empty blocks in volume, template, artifact, device, spread, and scaling.policy and wasn't able to get a crash anywhere but the volume_mount because we're protected elsewhere. So I think this is ready for review.

I'm not wild about having it work "by accident" though, especially because developers usually have a habit of copying such accidentally-working code and reusing it elsewhere in the same file. So I've gone through and fixed it where it was obviously wrong. We can fix it more generally in #10471

The following demonstrates the problem (https://play.golang.org/p/muZcc6R4QLs):

package main

import (
	"fmt"
)

type Volume struct {
	ID string
}

func main() {

	old := []*Volume{{ID: "a"}, {ID: "b"}, {ID: "c"}}
	new := make([]*Volume, len(old))

	for i, o := range old {
		if o.ID == "never" {
			new[i] = &Volume{ID: o.ID}
		}
	}

	for _, n := range new {
		fmt.Println(n)
	}
}

[jrasell]

LGTM!

[github-actions]

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.