From da74bf545bec531d9f4e743704c61bfce5da11eb Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Fri, 15 Apr 2022 10:48:06 -0400 Subject: [PATCH] CSI: replace structs->api with serialization extension The CSI HTTP API has to transform the CSI volume to redact secrets, remove the claims fields, and to consolidate the allocation stubs into a single slice of alloc stubs. This was done manually in #8590 but this is a large amount of code and has proven both very bug prone (see #8659, #8666, #8699, #8735, and #12150) and requires updating lots of code every time we add a field to volumes or plugins. In #10202 we introduce encoding improvements for the `Node` struct that allow a more minimal transformation. Apply this same approach to serializing `structs.CSIVolume` to API responses. Also, the original reasoning behind #8590 for plugins no longer holds because the counts are now denormalized within the state store, so we can simply remove this transformation entirely. --- command/agent/csi_endpoint.go | 404 +---------------------------- command/agent/csi_endpoint_test.go | 40 +-- nomad/structs/csi.go | 6 +- nomad/structs/extensions.go | 51 +++- 4 files changed, 56 insertions(+), 445 deletions(-) diff --git a/command/agent/csi_endpoint.go b/command/agent/csi_endpoint.go index 75cc54dc566..fd0f6926658 100644 --- a/command/agent/csi_endpoint.go +++ b/command/agent/csi_endpoint.go @@ -2,11 +2,9 @@ package agent import ( "net/http" - "sort" "strconv" "strings" - "github.com/hashicorp/nomad/api" "github.com/hashicorp/nomad/nomad/structs" ) @@ -132,13 +130,7 @@ func (s *HTTPServer) csiVolumeGet(id string, resp http.ResponseWriter, req *http return nil, CodedError(404, "volume not found") } - vol := structsCSIVolumeToApi(out.Volume) - - // remove sensitive fields, as our redaction mechanism doesn't - // help serializing here - vol.Secrets = nil - - return vol, nil + return out.Volume, nil } func (s *HTTPServer) csiVolumeRegister(resp http.ResponseWriter, req *http.Request) (interface{}, error) { @@ -403,7 +395,7 @@ func (s *HTTPServer) CSIPluginSpecificRequest(resp http.ResponseWriter, req *htt return nil, CodedError(404, "plugin not found") } - return structsCSIPluginToApi(out.Plugin), nil + return out.Plugin, nil } // parseCSISecrets extracts a map of k/v pairs from the CSI secrets @@ -427,395 +419,3 @@ func parseCSISecrets(req *http.Request) structs.CSISecrets { } return structs.CSISecrets(secrets) } - -// structsCSIPluginToApi converts CSIPlugin, setting Expected the count of known plugin -// instances -func structsCSIPluginToApi(plug *structs.CSIPlugin) *api.CSIPlugin { - if plug == nil { - return nil - } - out := &api.CSIPlugin{ - ID: plug.ID, - Provider: plug.Provider, - Version: plug.Version, - Allocations: make([]*api.AllocationListStub, 0, len(plug.Allocations)), - ControllerRequired: plug.ControllerRequired, - ControllersHealthy: plug.ControllersHealthy, - ControllersExpected: plug.ControllersExpected, - Controllers: make(map[string]*api.CSIInfo, len(plug.Controllers)), - NodesHealthy: plug.NodesHealthy, - NodesExpected: plug.NodesExpected, - Nodes: make(map[string]*api.CSIInfo, len(plug.Nodes)), - CreateIndex: plug.CreateIndex, - ModifyIndex: plug.ModifyIndex, - } - - for k, v := range plug.Controllers { - out.Controllers[k] = structsCSIInfoToApi(v) - } - - for k, v := range plug.Nodes { - out.Nodes[k] = structsCSIInfoToApi(v) - } - - for _, a := range plug.Allocations { - if a != nil { - out.Allocations = append(out.Allocations, structsAllocListStubToApi(a)) - } - } - - return out -} - -// structsCSIVolumeToApi converts CSIVolume, creating the allocation array -func structsCSIVolumeToApi(vol *structs.CSIVolume) *api.CSIVolume { - if vol == nil { - return nil - } - - allocCount := len(vol.ReadAllocs) + len(vol.WriteAllocs) - - out := &api.CSIVolume{ - ID: vol.ID, - Name: vol.Name, - ExternalID: vol.ExternalID, - Namespace: vol.Namespace, - - Topologies: structsCSITopolgiesToApi(vol.Topologies), - AccessMode: structsCSIAccessModeToApi(vol.AccessMode), - AttachmentMode: structsCSIAttachmentModeToApi(vol.AttachmentMode), - MountOptions: structsCSIMountOptionsToApi(vol.MountOptions), - Secrets: structsCSISecretsToApi(vol.Secrets), - Parameters: vol.Parameters, - Context: vol.Context, - Capacity: vol.Capacity, - - RequestedCapacityMin: vol.RequestedCapacityMin, - RequestedCapacityMax: vol.RequestedCapacityMax, - RequestedCapabilities: structsCSICapabilityToApi(vol.RequestedCapabilities), - CloneID: vol.CloneID, - SnapshotID: vol.SnapshotID, - - // Allocations is the collapsed list of both read and write allocs - Allocations: make([]*api.AllocationListStub, 0, allocCount), - - // tracking of volume claims - ReadAllocs: map[string]*api.Allocation{}, - WriteAllocs: map[string]*api.Allocation{}, - - Schedulable: vol.Schedulable, - PluginID: vol.PluginID, - Provider: vol.Provider, - ProviderVersion: vol.ProviderVersion, - ControllerRequired: vol.ControllerRequired, - ControllersHealthy: vol.ControllersHealthy, - ControllersExpected: vol.ControllersExpected, - NodesHealthy: vol.NodesHealthy, - NodesExpected: vol.NodesExpected, - ResourceExhausted: vol.ResourceExhausted, - CreateIndex: vol.CreateIndex, - ModifyIndex: vol.ModifyIndex, - } - - if vol.RequestedTopologies != nil { - out.RequestedTopologies = &api.CSITopologyRequest{ - Preferred: structsCSITopolgiesToApi(vol.RequestedTopologies.Preferred), - Required: structsCSITopolgiesToApi(vol.RequestedTopologies.Required), - } - } - - // WriteAllocs and ReadAllocs will only ever contain the Allocation ID, - // with a null value for the Allocation; these IDs are mapped to - // allocation stubs in the Allocations field. This indirection is so the - // API can support both the UI and CLI consumer in a safely backwards - // compatible way - for _, a := range vol.WriteAllocs { - if a != nil { - alloc := structsAllocListStubToApi(a.Stub(nil)) - if alloc != nil { - out.WriteAllocs[alloc.ID] = nil - out.Allocations = append(out.Allocations, alloc) - } - } - } - - for _, a := range vol.ReadAllocs { - if a != nil { - alloc := structsAllocListStubToApi(a.Stub(nil)) - if alloc != nil { - out.ReadAllocs[alloc.ID] = nil - out.Allocations = append(out.Allocations, alloc) - } - } - } - - sort.Slice(out.Allocations, func(i, j int) bool { - return out.Allocations[i].ModifyIndex > out.Allocations[j].ModifyIndex - }) - - return out -} - -// structsCSIInfoToApi converts CSIInfo, part of CSIPlugin -func structsCSIInfoToApi(info *structs.CSIInfo) *api.CSIInfo { - if info == nil { - return nil - } - out := &api.CSIInfo{ - PluginID: info.PluginID, - AllocID: info.AllocID, - Healthy: info.Healthy, - HealthDescription: info.HealthDescription, - UpdateTime: info.UpdateTime, - RequiresControllerPlugin: info.RequiresControllerPlugin, - RequiresTopologies: info.RequiresTopologies, - } - - if info.ControllerInfo != nil { - ci := info.ControllerInfo - out.ControllerInfo = &api.CSIControllerInfo{ - SupportsCreateDelete: ci.SupportsCreateDelete, - SupportsAttachDetach: ci.SupportsAttachDetach, - SupportsListVolumes: ci.SupportsListVolumes, - SupportsGetCapacity: ci.SupportsGetCapacity, - SupportsCreateDeleteSnapshot: ci.SupportsCreateDeleteSnapshot, - SupportsListSnapshots: ci.SupportsListSnapshots, - SupportsClone: ci.SupportsClone, - SupportsReadOnlyAttach: ci.SupportsReadOnlyAttach, - SupportsExpand: ci.SupportsExpand, - SupportsListVolumesAttachedNodes: ci.SupportsListVolumesAttachedNodes, - SupportsCondition: ci.SupportsCondition, - SupportsGet: ci.SupportsGet, - } - } - - if info.NodeInfo != nil { - out.NodeInfo = &api.CSINodeInfo{ - ID: info.NodeInfo.ID, - MaxVolumes: info.NodeInfo.MaxVolumes, - RequiresNodeStageVolume: info.NodeInfo.RequiresNodeStageVolume, - SupportsStats: info.NodeInfo.SupportsStats, - SupportsExpand: info.NodeInfo.SupportsExpand, - SupportsCondition: info.NodeInfo.SupportsCondition, - } - - if info.NodeInfo.AccessibleTopology != nil { - out.NodeInfo.AccessibleTopology = &api.CSITopology{} - out.NodeInfo.AccessibleTopology.Segments = info.NodeInfo.AccessibleTopology.Segments - } - } - - return out -} - -// structsAllocListStubToApi converts AllocListStub, for CSIPlugin -func structsAllocListStubToApi(alloc *structs.AllocListStub) *api.AllocationListStub { - if alloc == nil { - return nil - } - out := &api.AllocationListStub{ - ID: alloc.ID, - EvalID: alloc.EvalID, - Name: alloc.Name, - Namespace: alloc.Namespace, - NodeID: alloc.NodeID, - NodeName: alloc.NodeName, - JobID: alloc.JobID, - JobType: alloc.JobType, - JobVersion: alloc.JobVersion, - TaskGroup: alloc.TaskGroup, - DesiredStatus: alloc.DesiredStatus, - DesiredDescription: alloc.DesiredDescription, - ClientStatus: alloc.ClientStatus, - ClientDescription: alloc.ClientDescription, - TaskStates: make(map[string]*api.TaskState, len(alloc.TaskStates)), - FollowupEvalID: alloc.FollowupEvalID, - PreemptedAllocations: alloc.PreemptedAllocations, - PreemptedByAllocation: alloc.PreemptedByAllocation, - CreateIndex: alloc.CreateIndex, - ModifyIndex: alloc.ModifyIndex, - CreateTime: alloc.CreateTime, - ModifyTime: alloc.ModifyTime, - } - - out.DeploymentStatus = structsAllocDeploymentStatusToApi(alloc.DeploymentStatus) - out.RescheduleTracker = structsRescheduleTrackerToApi(alloc.RescheduleTracker) - - for k, v := range alloc.TaskStates { - out.TaskStates[k] = structsTaskStateToApi(v) - } - - return out -} - -// structsAllocDeploymentStatusToApi converts RescheduleTracker, part of AllocListStub -func structsAllocDeploymentStatusToApi(ads *structs.AllocDeploymentStatus) *api.AllocDeploymentStatus { - if ads == nil { - return nil - } - out := &api.AllocDeploymentStatus{ - Healthy: ads.Healthy, - Timestamp: ads.Timestamp, - Canary: ads.Canary, - ModifyIndex: ads.ModifyIndex, - } - return out -} - -// structsRescheduleTrackerToApi converts RescheduleTracker, part of AllocListStub -func structsRescheduleTrackerToApi(rt *structs.RescheduleTracker) *api.RescheduleTracker { - if rt == nil { - return nil - } - out := &api.RescheduleTracker{ - Events: make([]*api.RescheduleEvent, 0, len(rt.Events)), - } - - for _, e := range rt.Events { - out.Events = append(out.Events, &api.RescheduleEvent{ - RescheduleTime: e.RescheduleTime, - PrevAllocID: e.PrevAllocID, - PrevNodeID: e.PrevNodeID, - }) - } - - return out -} - -// structsTaskStateToApi converts TaskState, part of AllocListStub -func structsTaskStateToApi(ts *structs.TaskState) *api.TaskState { - if ts == nil { - return nil - } - out := &api.TaskState{ - State: ts.State, - Failed: ts.Failed, - Restarts: ts.Restarts, - LastRestart: ts.LastRestart, - StartedAt: ts.StartedAt, - FinishedAt: ts.FinishedAt, - Events: make([]*api.TaskEvent, 0, len(ts.Events)), - } - - for _, te := range ts.Events { - out.Events = append(out.Events, structsTaskEventToApi(te)) - } - - return out -} - -// structsTaskEventToApi converts TaskEvents, part of AllocListStub -func structsTaskEventToApi(te *structs.TaskEvent) *api.TaskEvent { - if te == nil { - return nil - } - out := &api.TaskEvent{ - Type: te.Type, - Time: te.Time, - DisplayMessage: te.DisplayMessage, - Details: te.Details, - - // DEPRECATION NOTICE: The following fields are all deprecated. see TaskEvent struct in structs.go for details. - FailsTask: te.FailsTask, - RestartReason: te.RestartReason, - SetupError: te.SetupError, - DriverError: te.DriverError, - DriverMessage: te.DriverMessage, - ExitCode: te.ExitCode, - Signal: te.Signal, - Message: te.Message, - KillReason: te.KillReason, - KillTimeout: te.KillTimeout, - KillError: te.KillError, - StartDelay: te.StartDelay, - DownloadError: te.DownloadError, - ValidationError: te.ValidationError, - DiskLimit: te.DiskLimit, - FailedSibling: te.FailedSibling, - VaultError: te.VaultError, - TaskSignalReason: te.TaskSignalReason, - TaskSignal: te.TaskSignal, - GenericSource: te.GenericSource, - } - - return out -} - -// structsCSITopolgiesToApi converts topologies, part of structsCSIVolumeToApi -func structsCSITopolgiesToApi(tops []*structs.CSITopology) []*api.CSITopology { - out := make([]*api.CSITopology, 0, len(tops)) - for _, t := range tops { - if t != nil { - out = append(out, &api.CSITopology{ - Segments: t.Segments, - }) - } - } - - return out -} - -// structsCSIAccessModeToApi converts access mode, part of structsCSIVolumeToApi -func structsCSIAccessModeToApi(mode structs.CSIVolumeAccessMode) api.CSIVolumeAccessMode { - switch mode { - case structs.CSIVolumeAccessModeSingleNodeReader: - return api.CSIVolumeAccessModeSingleNodeReader - case structs.CSIVolumeAccessModeSingleNodeWriter: - return api.CSIVolumeAccessModeSingleNodeWriter - case structs.CSIVolumeAccessModeMultiNodeReader: - return api.CSIVolumeAccessModeMultiNodeReader - case structs.CSIVolumeAccessModeMultiNodeSingleWriter: - return api.CSIVolumeAccessModeMultiNodeSingleWriter - case structs.CSIVolumeAccessModeMultiNodeMultiWriter: - return api.CSIVolumeAccessModeMultiNodeMultiWriter - default: - } - return api.CSIVolumeAccessModeUnknown -} - -// structsCSIAttachmentModeToApiModeToApi converts attachment mode, part of structsCSIVolumeToApi -func structsCSIAttachmentModeToApi(mode structs.CSIVolumeAttachmentMode) api.CSIVolumeAttachmentMode { - switch mode { - case structs.CSIVolumeAttachmentModeBlockDevice: - return api.CSIVolumeAttachmentModeBlockDevice - case structs.CSIVolumeAttachmentModeFilesystem: - return api.CSIVolumeAttachmentModeFilesystem - default: - } - return api.CSIVolumeAttachmentModeUnknown -} - -// structsCSICapabilityToApi converts capabilities, part of structsCSIVolumeToApi -func structsCSICapabilityToApi(caps []*structs.CSIVolumeCapability) []*api.CSIVolumeCapability { - out := make([]*api.CSIVolumeCapability, len(caps)) - for i, cap := range caps { - out[i] = &api.CSIVolumeCapability{ - AccessMode: api.CSIVolumeAccessMode(cap.AccessMode), - AttachmentMode: api.CSIVolumeAttachmentMode(cap.AttachmentMode), - } - } - return out -} - -// structsCSIMountOptionsToApi converts mount options, part of structsCSIVolumeToApi -func structsCSIMountOptionsToApi(opts *structs.CSIMountOptions) *api.CSIMountOptions { - if opts == nil { - return nil - } - apiOpts := &api.CSIMountOptions{ - FSType: opts.FSType, - } - if len(opts.MountFlags) > 0 { - apiOpts.MountFlags = []string{"[REDACTED]"} - } - - return apiOpts -} - -func structsCSISecretsToApi(secrets structs.CSISecrets) api.CSISecrets { - out := make(api.CSISecrets, len(secrets)) - for k, v := range secrets { - out[k] = v - } - return out -} diff --git a/command/agent/csi_endpoint_test.go b/command/agent/csi_endpoint_test.go index 7115027e2d0..9805a822881 100644 --- a/command/agent/csi_endpoint_test.go +++ b/command/agent/csi_endpoint_test.go @@ -8,7 +8,6 @@ import ( "github.com/hashicorp/nomad/api" "github.com/hashicorp/nomad/ci" - "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/state" "github.com/hashicorp/nomad/nomad/structs" "github.com/stretchr/testify/require" @@ -30,7 +29,7 @@ func TestHTTP_CSIEndpointPlugin(t *testing.T) { require.NoError(t, err) require.Equal(t, 200, resp.Code) - out, ok := obj.(*api.CSIPlugin) + out, ok := obj.(*structs.CSIPlugin) require.True(t, ok) // ControllersExpected is 0 because this plugin was created without a job, @@ -68,20 +67,6 @@ func TestHTTP_CSIParseSecrets(t *testing.T) { } } -func TestHTTP_CSIEndpointUtils(t *testing.T) { - secrets := structsCSISecretsToApi(structs.CSISecrets{ - "foo": "bar", - }) - - require.Equal(t, "bar", secrets["foo"]) - - tops := structsCSITopolgiesToApi([]*structs.CSITopology{{ - Segments: map[string]string{"foo": "bar"}, - }}) - - require.Equal(t, "bar", tops[0].Segments["foo"]) -} - func TestHTTP_CSIEndpointRegisterVolume(t *testing.T) { ci.Parallel(t) httpTest(t, nil, func(s *TestAgent) { @@ -111,7 +96,7 @@ func TestHTTP_CSIEndpointRegisterVolume(t *testing.T) { resp = httptest.NewRecorder() raw, err := s.Server.CSIVolumeSpecificRequest(resp, req) require.NoError(t, err, "get error") - out, ok := raw.(*api.CSIVolume) + out, ok := raw.(*structs.CSIVolume) require.True(t, ok) require.Equal(t, 1, out.ControllersHealthy) require.Equal(t, 2, out.NodesHealthy) @@ -178,24 +163,3 @@ func TestHTTP_CSIEndpointSnapshot(t *testing.T) { require.Error(t, err, "no such volume: bar") }) } - -// TestHTTP_CSIEndpoint_Cast is a smoke test for converting from structs to -// API structs -func TestHTTP_CSIEndpoint_Cast(t *testing.T) { - ci.Parallel(t) - - plugin := mock.CSIPlugin() - plugin.Nodes["node1"] = &structs.CSIInfo{ - PluginID: plugin.ID, - AllocID: "alloc1", - NodeInfo: &structs.CSINodeInfo{ID: "instance-1", MaxVolumes: 3}, - } - apiPlugin := structsCSIPluginToApi(plugin) - require.Equal(t, - plugin.Nodes["node1"].NodeInfo.MaxVolumes, - apiPlugin.Nodes["node1"].NodeInfo.MaxVolumes) - - vol := mock.CSIVolume(plugin) - apiVol := structsCSIVolumeToApi(vol) - require.Equal(t, vol.MountOptions.MountFlags, apiVol.MountOptions.MountFlags) -} diff --git a/nomad/structs/csi.go b/nomad/structs/csi.go index 9dfefaa844c..2147cc08a2b 100644 --- a/nomad/structs/csi.go +++ b/nomad/structs/csi.go @@ -297,9 +297,9 @@ type CSIVolume struct { ReadAllocs map[string]*Allocation // AllocID -> Allocation WriteAllocs map[string]*Allocation // AllocID -> Allocation - ReadClaims map[string]*CSIVolumeClaim // AllocID -> claim - WriteClaims map[string]*CSIVolumeClaim // AllocID -> claim - PastClaims map[string]*CSIVolumeClaim // AllocID -> claim + ReadClaims map[string]*CSIVolumeClaim `json:"-"` // AllocID -> claim + WriteClaims map[string]*CSIVolumeClaim `json:"-"` // AllocID -> claim + PastClaims map[string]*CSIVolumeClaim `json:"-"` // AllocID -> claim // Schedulable is true if all the denormalized plugin health fields are true, and the // volume has not been marked for garbage collection diff --git a/nomad/structs/extensions.go b/nomad/structs/extensions.go index e16c3e4d890..19d287aa2c6 100644 --- a/nomad/structs/extensions.go +++ b/nomad/structs/extensions.go @@ -8,8 +8,10 @@ var ( // extendedTypes is a mapping of extended types to their extension function // TODO: the duplicates could be simplified by looking up the base type in the case of a pointer type in ConvertExt extendedTypes = map[reflect.Type]extendFunc{ - reflect.TypeOf(Node{}): nodeExt, - reflect.TypeOf(&Node{}): nodeExt, + reflect.TypeOf(Node{}): nodeExt, + reflect.TypeOf(&Node{}): nodeExt, + reflect.TypeOf(CSIVolume{}): csiVolumeExt, + reflect.TypeOf(&CSIVolume{}): csiVolumeExt, } ) @@ -29,3 +31,48 @@ func nodeExt(v interface{}) interface{} { Drain: node != nil && node.DrainStrategy != nil, } } + +func csiVolumeExt(v interface{}) interface{} { + vol := v.(*CSIVolume) + type EmbeddedCSIVolume CSIVolume + + allocCount := len(vol.ReadAllocs) + len(vol.WriteAllocs) + + apiVol := &struct { + *EmbeddedCSIVolume + Allocations []*AllocListStub + }{ + EmbeddedCSIVolume: (*EmbeddedCSIVolume)(vol), + Allocations: make([]*AllocListStub, 0, allocCount), + } + + // WriteAllocs and ReadAllocs will only ever contain the Allocation ID, + // with a null value for the Allocation; these IDs are mapped to + // allocation stubs in the Allocations field. This indirection is so the + // API can support both the UI and CLI consumer in a safely backwards + // compatible way + for _, a := range vol.ReadAllocs { + if a != nil { + apiVol.ReadAllocs[a.ID] = nil + apiVol.Allocations = append(apiVol.Allocations, a.Stub(nil)) + } + } + for _, a := range vol.WriteAllocs { + if a != nil { + apiVol.WriteAllocs[a.ID] = nil + apiVol.Allocations = append(apiVol.Allocations, a.Stub(nil)) + } + } + + // MountFlags can contain secrets, so we always redact it but want + // to show the user that we have the value + if vol.MountOptions != nil && len(vol.MountOptions.MountFlags) > 0 { + apiVol.MountOptions.MountFlags = []string{"[REDACTED]"} + } + + // would be better not to have at all but left in and redacted for + // backwards compatibility with the existing API + apiVol.Secrets = nil + + return apiVol +}