dynamic host volumes: allow plugins to return an error message (#25341)

Errors from `volume create` or `volume delete` only get logged by the client
agent, which may make it harder for volume authors to debug these tasks if they
are not also the cluster administrator with access to host logs.

Allow plugins to include an optional error message in their response. Because we
can't count on receiving this response (the error could come before the plugin
executes), we parse this message optimistically and include it only if
available.

Ref: https://hashicorp.atlassian.net/browse/NET-12087

api/host_volumes.go

@@ -177,6 +177,8 @@ type HostVolumeDeleteRequest struct {
 	ID string
 }
 
+type HostVolumeDeleteResponse struct{}
+
 // Create forwards to client agents so a host volume can be created on those
 // hosts, and registers the volume with Nomad servers.
 func (hv *HostVolumes) Create(req *HostVolumeCreateRequest, opts *WriteOptions) (*HostVolumeCreateResponse, *WriteMeta, error) {
@@ -236,11 +238,12 @@ func (hv *HostVolumes) List(req *HostVolumeListRequest, opts *QueryOptions) ([]*
 }
 
 // Delete deletes a host volume
-func (hv *HostVolumes) Delete(req *HostVolumeDeleteRequest, opts *WriteOptions) (*WriteMeta, error) {
+func (hv *HostVolumes) Delete(req *HostVolumeDeleteRequest, opts *WriteOptions) (*HostVolumeDeleteResponse, *WriteMeta, error) {
+	var resp *HostVolumeDeleteResponse
 	path, err := url.JoinPath("/v1/volume/host/", url.PathEscape(req.ID))
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
-	wm, err := hv.client.delete(path, nil, nil, opts)
-	return wm, err
+	wm, err := hv.client.delete(path, nil, resp, opts)
+	return resp, wm, err
 }

client/hostvolumemanager/host_volume_plugin.go

@@ -52,12 +52,20 @@ type PluginFingerprint struct {
 	Version *version.Version `json:"version"`
 }
 
-// HostVolumePluginCreateResponse gets stored on the volume in server state.
-// Plugins are expected to respond to 'create' calls with json that
-// unmarshals to this struct.
+// HostVolumePluginCreateResponse returns values to the server that may be shown
+// to the user or stored in server state. Plugins are expected to respond to
+// 'create' calls with json that unmarshals to this struct.
 type HostVolumePluginCreateResponse struct {
 	Path      string `json:"path"`
 	SizeBytes int64  `json:"bytes"`
+	Error     string `json:"error"`
+}
+
+// HostVolumePluginDeleteResponse returns values to the server that may be shown
+// to the user or stored in server state. Plugins are expected to respond to
+// 'delete' calls with json that unmarshals to this struct.
+type HostVolumePluginDeleteResponse struct {
+	Error string `json:"error"`
 }
 
 const HostVolumePluginMkdirID = "mkdir"
@@ -255,13 +263,21 @@ func (p *HostVolumePluginExternal) Create(ctx context.Context,
 		fmt.Sprintf("%s=%s", EnvParameters, params),
 	}
 
+	var pluginResp HostVolumePluginCreateResponse
 	log := p.log.With("volume_name", req.Name, "volume_id", req.ID)
 	stdout, _, err := p.runPlugin(ctx, log, "create", envVars)
 	if err != nil {
-		return nil, fmt.Errorf("error creating volume %q with plugin %q: %w", req.ID, p.ID, err)
+		jsonErr := json.Unmarshal(stdout, &pluginResp)
+		if jsonErr != nil {
+			// if we got an error, we can't actually count on getting JSON, so
+			// optimistically look for it and return the original error
+			// otherwise
+			return nil, fmt.Errorf(
+				"error creating volume %q with plugin %q: %w", req.ID, p.ID, err)
+		}
+		return nil, fmt.Errorf("error creating volume %q with plugin %q: %w: %s",
+			req.ID, p.ID, err, pluginResp.Error)
 	}
-
-	var pluginResp HostVolumePluginCreateResponse
 	err = json.Unmarshal(stdout, &pluginResp)
 	if err != nil {
 		// note: if a plugin does not return valid json, a volume may be
@@ -313,10 +329,20 @@ func (p *HostVolumePluginExternal) Delete(ctx context.Context,
 	}
 
 	log := p.log.With("volume_name", req.Name, "volume_id", req.ID)
-	_, _, err = p.runPlugin(ctx, log, "delete", envVars)
+	stdout, _, err := p.runPlugin(ctx, log, "delete", envVars)
 	if err != nil {
-		return fmt.Errorf("error deleting volume %q with plugin %q: %w", req.ID, p.ID, err)
+		var pluginResp HostVolumePluginDeleteResponse
+		jsonErr := json.Unmarshal(stdout, &pluginResp)
+		if jsonErr != nil {
+			// if we got an error, we can't actually count on getting JSON, so
+			// optimistically look for it and return the original error
+			// otherwise
+			return fmt.Errorf("error reading plugin response when deleting volume %q with plugin %q: original error: %w", req.ID, p.ID, err)
+		}
+		return fmt.Errorf("error deleting volume %q with plugin %q: %w: %s",
+			req.ID, p.ID, err, pluginResp.Error)
 	}
+
 	return nil
 }
 

client/hostvolumemanager/host_volume_plugin_test.go

@@ -188,7 +188,7 @@ func TestHostVolumePluginExternal(t *testing.T) {
 			&cstructs.ClientHostVolumeCreateRequest{
 				ID: volID,
 			})
-		must.EqError(t, err, `error creating volume "test-vol-id" with plugin "test_plugin_sad.sh": exit status 1`)
+		must.EqError(t, err, `error creating volume "test-vol-id" with plugin "test_plugin_sad.sh": exit status 1: create: sad plugin is sad`)
 		must.Nil(t, resp)
 		logged = getLogs()
 		must.StrContains(t, logged, "create: sad plugin is sad")
@@ -201,7 +201,7 @@ func TestHostVolumePluginExternal(t *testing.T) {
 			&cstructs.ClientHostVolumeDeleteRequest{
 				ID: volID,
 			})
-		must.EqError(t, err, `error deleting volume "test-vol-id" with plugin "test_plugin_sad.sh": exit status 1`)
+		must.EqError(t, err, `error deleting volume "test-vol-id" with plugin "test_plugin_sad.sh": exit status 1: delete: sad plugin is sad`)
 		logged = getLogs()
 		must.StrContains(t, logged, "delete: sad plugin is sad")
 		must.StrContains(t, logged, "delete: it tells you all about it in stderr")

client/hostvolumemanager/test_fixtures/test_plugin_sad.sh

@@ -2,6 +2,6 @@
 # Copyright (c) HashiCorp, Inc.
 # SPDX-License-Identifier: BUSL-1.1
 
-echo "$1: sad plugin is sad"
+printf '{"error": "%s: sad plugin is sad"}' $1
 echo "$1: it tells you all about it in stderr" 1>&2
 exit 1

command/agent/host_volume_endpoint.go

@@ -139,5 +139,5 @@ func (s *HTTPServer) hostVolumeDelete(id string, resp http.ResponseWriter, req *
 
 	setIndex(resp, out.Index)
 
-	return nil, nil
+	return out, nil
 }

command/agent/host_volume_endpoint_test.go

@@ -87,8 +87,9 @@ func TestHostVolumeEndpoint_CRUD(t *testing.T) {
 
 		req, err = http.NewRequest(http.MethodDelete, fmt.Sprintf("/v1/volume/host/%s", volID), nil)
 		must.NoError(t, err)
-		_, err = s.Server.HostVolumeSpecificRequest(respW, req)
+		obj, err = s.Server.HostVolumeSpecificRequest(respW, req)
 		must.NoError(t, err)
+		must.NotNil(t, obj)
 
 		// Verify volume was deleted
 

command/volume_delete.go

@@ -195,7 +195,7 @@ func (c *VolumeDeleteCommand) deleteHostVolume(client *api.Client, volID string)
 		c.namespace = stub.Namespace
 	}
 
-	_, err := client.HostVolumes().Delete(&api.HostVolumeDeleteRequest{ID: volID}, nil)
+	_, _, err := client.HostVolumes().Delete(&api.HostVolumeDeleteRequest{ID: volID}, nil)
 	if err != nil {
 		c.Ui.Error(fmt.Sprintf("Error deleting volume: %s", err))
 		return 1

website/content/docs/concepts/plugins/storage/host-volumes.mdx

@@ -291,6 +291,15 @@ DHV_PARAMETERS={stringified json of parameters from the volume spec}
 {"path": "/path/to/created/volume", "bytes": 50000000}
 ```
 
+**Expected stdout on error:**
+
+```
+{"error": "error message"}
+```
+
+Returning an error message is optional. Nomad returns the error message in any
+error returned to the user.
+
 **Requirements:**
 
 * Must complete within 60 seconds, or Nomad will kill it.
@@ -333,6 +342,15 @@ DHV_PARAMETERS={stringified json of parameters from the volume spec}
 
 **Expected stdout:** none (stdout is discarded)
 
+**Expected stdout on error:**
+
+```
+{"error": "error message"}
+```
+
+Returning an error message is optional. Nomad returns the error message in any
+error returned to the user.
+
 **Requirements:**
 
 * Must complete within 60 seconds, or Nomad will kill it.