CSI: use HTTP headers for passing CSI secrets

.changelog/12144.txt

@@ -0,0 +1,6 @@
+```release-note:improvement
+api: CSI secrets for list and delete snapshots are now passed in HTTP headers
+```
+
+```release-note:improvement
+cli: CSI secrets argument for `volume snapshot list` has been made consistent with `volume snapshot delete`

api/api.go

@@ -62,6 +62,9 @@ type QueryOptions struct {
 	// Set HTTP parameters on the query.
 	Params map[string]string
 
+	// Set HTTP headers on the query.
+	Headers map[string]string
+
 	// AuthToken is the secret ID of an ACL token
 	AuthToken string
 
@@ -101,6 +104,9 @@ type WriteOptions struct {
 	// AuthToken is the secret ID of an ACL token
 	AuthToken string
 
+	// Set HTTP headers on the query.
+	Headers map[string]string
+
 	// ctx is an optional context pass through to the underlying HTTP
 	// request layer. Use Context() and WithContext() to manage this.
 	ctx context.Context
@@ -606,6 +612,10 @@ func (r *request) setQueryOptions(q *QueryOptions) {
 		r.params.Set(k, v)
 	}
 	r.ctx = q.Context()
+
+	for k, v := range q.Headers {
+		r.header.Set(k, v)
+	}
 }
 
 // durToMsec converts a duration to a millisecond specified string
@@ -632,6 +642,10 @@ func (r *request) setWriteOptions(q *WriteOptions) {
 		r.params.Set("idempotency_token", q.IdempotencyToken)
 	}
 	r.ctx = q.Context()
+
+	for k, v := range q.Headers {
+		r.header.Set(k, v)
+	}
 }
 
 // toHTTP converts the request to an HTTP request

api/csi.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"net/url"
 	"sort"
+	"strings"
 	"time"
 )
 
@@ -129,13 +130,37 @@ func (v *CSIVolumes) DeleteSnapshot(snap *CSISnapshot, w *WriteOptions) error {
 	qp := url.Values{}
 	qp.Set("snapshot_id", snap.ID)
 	qp.Set("plugin_id", snap.PluginID)
-	for k, v := range snap.Secrets {
-		qp.Set("secret", fmt.Sprintf("%v=%v", k, v))
-	}
+	w.SetHeadersFromCSISecrets(snap.Secrets)
 	_, err := v.client.delete("/v1/volumes/snapshot?"+qp.Encode(), nil, w)
 	return err
 }
 
+// ListSnapshotsOpts lists external storage volume snapshots.
+func (v *CSIVolumes) ListSnapshotsOpts(req *CSISnapshotListRequest) (*CSISnapshotListResponse, *QueryMeta, error) {
+	var resp *CSISnapshotListResponse
+
+	qp := url.Values{}
+	if req.PluginID != "" {
+		qp.Set("plugin_id", req.PluginID)
+	}
+	if req.NextToken != "" {
+		qp.Set("next_token", req.NextToken)
+	}
+	if req.PerPage != 0 {
+		qp.Set("per_page", fmt.Sprint(req.PerPage))
+	}
+	req.QueryOptions.SetHeadersFromCSISecrets(req.Secrets)
+
+	qm, err := v.client.query("/v1/volumes/snapshot?"+qp.Encode(), &resp, &req.QueryOptions)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	sort.Sort(CSISnapshotSort(resp.Snapshots))
+	return resp, qm, nil
+}
+
+// DEPRECATED: will be removed in Nomad 1.4.0
 // ListSnapshots lists external storage volume snapshots.
 func (v *CSIVolumes) ListSnapshots(pluginID string, secrets string, q *QueryOptions) (*CSISnapshotListResponse, *QueryMeta, error) {
 	var resp *CSISnapshotListResponse
@@ -150,9 +175,6 @@ func (v *CSIVolumes) ListSnapshots(pluginID string, secrets string, q *QueryOpti
 	if q.PerPage != 0 {
 		qp.Set("per_page", fmt.Sprint(q.PerPage))
 	}
-	if secrets != "" {
-		qp.Set("secrets", secrets)
-	}
 
 	qm, err := v.client.query("/v1/volumes/snapshot?"+qp.Encode(), &resp, q)
 	if err != nil {
@@ -206,6 +228,28 @@ type CSIMountOptions struct {
 // API or in Nomad's logs.
 type CSISecrets map[string]string
 
+func (q *QueryOptions) SetHeadersFromCSISecrets(secrets CSISecrets) {
+	pairs := []string{}
+	for k, v := range secrets {
+		pairs = append(pairs, fmt.Sprintf("%v=%v", k, v))
+	}
+	if q.Headers == nil {
+		q.Headers = map[string]string{}
+	}
+	q.Headers["X-Nomad-CSI-Secrets"] = strings.Join(pairs, ",")
+}
+
+func (q *WriteOptions) SetHeadersFromCSISecrets(secrets CSISecrets) {
+	pairs := []string{}
+	for k, v := range secrets {
+		pairs = append(pairs, fmt.Sprintf("%v=%v", k, v))
+	}
+	if q.Headers == nil {
+		q.Headers = map[string]string{}
+	}
+	q.Headers["X-Nomad-CSI-Secrets"] = strings.Join(pairs, ",")
+}
+
 // CSIVolume is used for serialization, see also nomad/structs/csi.go
 type CSIVolume struct {
 	ID             string

command/agent/csi_endpoint.go

@@ -305,13 +305,9 @@ func (s *HTTPServer) csiSnapshotDelete(resp http.ResponseWriter, req *http.Reque
 	query := req.URL.Query()
 	snap.PluginID = query.Get("plugin_id")
 	snap.ID = query.Get("snapshot_id")
-	secrets := query["secret"]
-	for _, raw := range secrets {
-		secret := strings.Split(raw, "=")
-		if len(secret) == 2 {
-			snap.Secrets[secret[0]] = secret[1]
-		}
-	}
+
+	secrets := parseCSISecrets(req)
+	snap.Secrets = secrets
 
 	args.Snapshots = []*structs.CSISnapshot{snap}
 
@@ -333,19 +329,9 @@ func (s *HTTPServer) csiSnapshotList(resp http.ResponseWriter, req *http.Request
 
 	query := req.URL.Query()
 	args.PluginID = query.Get("plugin_id")
-	querySecrets := query["secrets"]
-
-	// Parse comma separated secrets only when provided
-	if len(querySecrets) >= 1 {
-		secrets := strings.Split(querySecrets[0], ",")
-		args.Secrets = make(structs.CSISecrets)
-		for _, raw := range secrets {
-			secret := strings.Split(raw, "=")
-			if len(secret) == 2 {
-				args.Secrets[secret[0]] = secret[1]
-			}
-		}
-	}
+
+	secrets := parseCSISecrets(req)
+	args.Secrets = secrets
 
 	var out structs.CSISnapshotListResponse
 	if err := s.agent.RPC("CSIVolume.ListSnapshots", &args, &out); err != nil {
@@ -420,6 +406,28 @@ func (s *HTTPServer) CSIPluginSpecificRequest(resp http.ResponseWriter, req *htt
 	return structsCSIPluginToApi(out.Plugin), nil
 }
 
+// parseCSISecrets extracts a map of k/v pairs from the CSI secrets
+// header. Silently ignores invalid secrets
+func parseCSISecrets(req *http.Request) structs.CSISecrets {
+	secretsHeader := req.Header.Get("X-Nomad-CSI-Secrets")
+	if secretsHeader == "" {
+		return nil
+	}
+
+	secrets := map[string]string{}
+	secretkvs := strings.Split(secretsHeader, ",")
+	for _, secretkv := range secretkvs {
+		kv := strings.Split(secretkv, "=")
+		if len(kv) == 2 {
+			secrets[kv[0]] = kv[1]
+		}
+	}
+	if len(secrets) == 0 {
+		return nil
+	}
+	return structs.CSISecrets(secrets)
+}
+
 // structsCSIPluginToApi converts CSIPlugin, setting Expected the count of known plugin
 // instances
 func structsCSIPluginToApi(plug *structs.CSIPlugin) *api.CSIPlugin {

command/agent/csi_endpoint_test.go

@@ -44,6 +44,29 @@ func TestHTTP_CSIEndpointPlugin(t *testing.T) {
 	})
 }
 
+func TestHTTP_CSIParseSecrets(t *testing.T) {
+	t.Parallel()
+	testCases := []struct {
+		val    string
+		expect structs.CSISecrets
+	}{
+		{"", nil},
+		{"one", nil},
+		{"one,two", nil},
+		{"one,two=value_two",
+			structs.CSISecrets(map[string]string{"two": "value_two"})},
+		{"one=value_one,one=overwrite",
+			structs.CSISecrets(map[string]string{"one": "overwrite"})},
+		{"one=value_one,two=value_two",
+			structs.CSISecrets(map[string]string{"one": "value_one", "two": "value_two"})},
+	}
+	for _, tc := range testCases {
+		req, _ := http.NewRequest("GET", "/v1/plugin/csi/foo", nil)
+		req.Header.Add("X-Nomad-CSI-Secrets", tc.val)
+		require.Equal(t, tc.expect, parseCSISecrets(req), tc.val)
+	}
+}
+
 func TestHTTP_CSIEndpointUtils(t *testing.T) {
 	secrets := structsCSISecretsToApi(structs.CSISecrets{
 		"foo": "bar",

command/volume_snapshot_delete.go

@@ -30,7 +30,7 @@ General Options:
 Snapshot Options:
 
   -secret
-    Secrets to pass to the plugin to create the snapshot. Accepts multiple
+    Secrets to pass to the plugin to delete the snapshot. Accepts multiple
     flags in the form -secret key=value
 
 `

command/volume_snapshot_list.go

@@ -9,6 +9,7 @@ import (
 	humanize "github.com/dustin/go-humanize"
 	"github.com/hashicorp/nomad/api"
 	"github.com/hashicorp/nomad/api/contexts"
+	flaghelper "github.com/hashicorp/nomad/helper/flags"
 	"github.com/pkg/errors"
 	"github.com/posener/complete"
 )
@@ -36,7 +37,9 @@ List Options:
   -plugin: Display only snapshots managed by a particular plugin. By default
    this command will query all plugins for their snapshots.
 
-  -secrets: A set of key/value secrets to be used when listing snapshots.
+  -secret
+    Secrets to pass to the plugin to list snapshots. Accepts multiple
+    flags in the form -secret key=value
 `
 	return strings.TrimSpace(helpText)
 }
@@ -70,13 +73,13 @@ func (c *VolumeSnapshotListCommand) Name() string { return "volume snapshot list
 func (c *VolumeSnapshotListCommand) Run(args []string) int {
 	var pluginID string
 	var verbose bool
-	var secrets string
+	var secretsArgs flaghelper.StringFlag
 
 	flags := c.Meta.FlagSet(c.Name(), FlagSetClient)
 	flags.Usage = func() { c.Ui.Output(c.Help()) }
 	flags.StringVar(&pluginID, "plugin", "", "")
 	flags.BoolVar(&verbose, "verbose", false, "")
-	flags.StringVar(&secrets, "secrets", "", "")
+	flags.Var(&secretsArgs, "secret", "secrets for snapshot, ex. -secret key=value")
 
 	if err := flags.Parse(args); err != nil {
 		c.Ui.Error(fmt.Sprintf("Error parsing arguments %s", err))
@@ -122,10 +125,22 @@ func (c *VolumeSnapshotListCommand) Run(args []string) int {
 		pluginID = plugs[0].ID
 	}
 
-	q := &api.QueryOptions{PerPage: 30} // TODO: tune page size
+	secrets := api.CSISecrets{}
+	for _, kv := range secretsArgs {
+		s := strings.Split(kv, "=")
+		if len(s) == 2 {
+			secrets[s[0]] = s[1]
+		}
+	}
+
+	req := &api.CSISnapshotListRequest{
+		PluginID:     pluginID,
+		Secrets:      secrets,
+		QueryOptions: api.QueryOptions{PerPage: 30},
+	}
 
 	for {
-		resp, _, err := client.CSIVolumes().ListSnapshots(pluginID, secrets, q)
+		resp, _, err := client.CSIVolumes().ListSnapshotsOpts(req)
 		if err != nil && !errors.Is(err, io.EOF) {
 			c.Ui.Error(fmt.Sprintf(
 				"Error querying CSI external snapshots for plugin %q: %s", pluginID, err))
@@ -138,8 +153,8 @@ func (c *VolumeSnapshotListCommand) Run(args []string) int {
 		}
 
 		c.Ui.Output(csiFormatSnapshots(resp.Snapshots, verbose))
-		q.NextToken = resp.NextToken
-		if q.NextToken == "" {
+		req.NextToken = resp.NextToken
+		if req.NextToken == "" {
 			break
 		}
 		// we can't know the shape of arbitrarily-sized lists of snapshots,

website/content/docs/commands/volume/snapshot-delete.mdx

@@ -29,6 +29,11 @@ volume` and `plugin:read` capabilities.
 
 @include 'general_options.mdx'
 
+## Snapshot Delete Options
+
+- `-secret`: Secrets to pass to the plugin to delete the
+  snapshot. Accepts multiple flags in the form `-secret key=value`
+
 ## Examples
 
 Delete a volume snapshot:

website/content/docs/commands/volume/snapshot-list.mdx

@@ -27,16 +27,16 @@ Nomad.
 
 @include 'general_options.mdx'
 
-## List Options
+## Snapshot List Options
 
 - `-plugin`: Display only snapshots managed by a particular [CSI
   plugin][csi_plugin]. By default the `snapshot list` command will query all
   plugins for their snapshots. This flag accepts a plugin ID or prefix. If
   there is an exact match based on the provided plugin, then that specific
   plugin will be queried. Otherwise, a list of matching plugins will be
   displayed.
-- `-secrets`: A list of comma separated secret key/value pairs to be passed 
-  to the CSI driver.
+- `-secret`: Secrets to pass to the plugin to delete the
+  snapshot. Accepts multiple flags in the form `-secret key=value`
 
 When ACLs are enabled, this command requires a token with the
 `csi-list-volumes` capability for the plugin's namespace.
@@ -54,12 +54,12 @@ snap-67890   vol-fedcba   50GiB  2021-01-04T15:45:00Z  true
 
 List volume snapshots with two secret key/value pairs:
 ```shell-session
-$ nomad volume snapshot list -secrets key1=value1,key2=val2
+$ nomad volume snapshot list -secret key1=value1 -secret key2=val2
 Snapshot ID  External ID  Size   Creation Time         Ready?
 snap-12345   vol-abcdef   50GiB  2021-01-03T12:15:02Z  true
 ```
 
 [csi]: https://github.com/container-storage-interface/spec
 [csi_plugin]: /docs/job-specification/csi_plugin
 [registered]: /docs/commands/volume/register
-[csi_plugins_internals]: /docs/internals/plugins/csi#csi-plugins
+[csi_plugins_internals]: /docs/internals/plugins/csi#csi-plugins