From afb0c5ac452ba3bcf5c12907351dccb4edbc368b Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Wed, 13 Feb 2019 12:55:48 -0500 Subject: [PATCH] drivers: restore port_map old json support This ensures that `port_map` along with other block like attribute declarations (e.g. ulimit, labels, etc) can handle various hcl and json syntax that was supported in 0.8. In 0.8.7, the following declarations are effectively equivalent: ``` // hcl block port_map { http = 80 https = 443 } // hcl assignment port_map = { http = 80 https = 443 } // json single element array of map (default in API response) {"port_map": [{"http": 80, "https": 443}]} // json array of individual maps (supported accidentally iiuc) {"port_map: [{"http": 80}, {"https": 443}]} ``` We achieve compatbility by using `NewAttr("...", "list(map(string))", false)` to be serialized to a `map[string]string` wrapper, instead of using `BlockAttrs` declaration. The wrapper merges the list of maps automatically, to ease driver development. This approach is closer to how v0.8.7 implemented the fields [1][2], and despite its verbosity, seems to perserve 0.8.7 behavior in hcl2. This is only required for built-in types that have backward compatibility constraints. External drivers should use `BlockAttrs` instead, as they see fit. [1] https://github.com/hashicorp/nomad/blob/v0.8.7/client/driver/docker.go#L216 [2] https://github.com/hashicorp/nomad/blob/v0.8.7/client/driver/docker.go#L698-L700 --- drivers/docker/config.go | 117 +++++++++--------- drivers/docker/config_test.go | 72 +++++++++++ drivers/qemu/driver.go | 13 +- drivers/rkt/driver.go | 23 ++-- helper/pluginutils/hclutils/types.go | 51 ++++++++ helper/pluginutils/hclutils/types_test.go | 139 ++++++++++++++++++++++ 6 files changed, 340 insertions(+), 75 deletions(-) create mode 100644 helper/pluginutils/hclutils/types.go create mode 100644 helper/pluginutils/hclutils/types_test.go diff --git a/drivers/docker/config.go b/drivers/docker/config.go index e28a3081c..1ec3b141d 100644 --- a/drivers/docker/config.go +++ b/drivers/docker/config.go @@ -8,6 +8,7 @@ import ( docker "github.com/fsouza/go-dockerclient" hclog "github.com/hashicorp/go-hclog" + "github.com/hashicorp/nomad/helper/pluginutils/hclutils" "github.com/hashicorp/nomad/helper/pluginutils/loader" "github.com/hashicorp/nomad/plugins/base" "github.com/hashicorp/nomad/plugins/drivers" @@ -251,11 +252,11 @@ var ( "ipc_mode": hclspec.NewAttr("ipc_mode", "string", false), "ipv4_address": hclspec.NewAttr("ipv4_address", "string", false), "ipv6_address": hclspec.NewAttr("ipv6_address", "string", false), - "labels": hclspec.NewBlockAttrs("labels", "string", false), + "labels": hclspec.NewAttr("labels", "list(map(string))", false), "load": hclspec.NewAttr("load", "string", false), "logging": hclspec.NewBlock("logging", false, hclspec.NewObject(map[string]*hclspec.Spec{ "type": hclspec.NewAttr("type", "string", false), - "config": hclspec.NewBlockAttrs("config", "string", false), + "config": hclspec.NewAttr("config", "list(map(string))", false), })), "mac_address": hclspec.NewAttr("mac_address", "string", false), "mounts": hclspec.NewBlockList("mounts", hclspec.NewObject(map[string]*hclspec.Spec{ @@ -275,10 +276,10 @@ var ( })), "volume_options": hclspec.NewBlock("volume_options", false, hclspec.NewObject(map[string]*hclspec.Spec{ "no_copy": hclspec.NewAttr("no_copy", "bool", false), - "labels": hclspec.NewBlockAttrs("labels", "string", false), + "labels": hclspec.NewAttr("labels", "list(map(string))", false), "driver_config": hclspec.NewBlock("driver_config", false, hclspec.NewObject(map[string]*hclspec.Spec{ "name": hclspec.NewAttr("name", "string", false), - "options": hclspec.NewBlockAttrs("options", "string", false), + "options": hclspec.NewAttr("options", "list(map(string))", false), })), })), })), @@ -286,15 +287,15 @@ var ( "network_mode": hclspec.NewAttr("network_mode", "string", false), "pids_limit": hclspec.NewAttr("pids_limit", "number", false), "pid_mode": hclspec.NewAttr("pid_mode", "string", false), - "port_map": hclspec.NewBlockAttrs("port_map", "number", false), + "port_map": hclspec.NewAttr("port_map", "list(map(number))", false), "privileged": hclspec.NewAttr("privileged", "bool", false), "readonly_rootfs": hclspec.NewAttr("readonly_rootfs", "bool", false), "security_opt": hclspec.NewAttr("security_opt", "list(string)", false), "shm_size": hclspec.NewAttr("shm_size", "number", false), - "storage_opt": hclspec.NewBlockAttrs("storage_opt", "string", false), - "sysctl": hclspec.NewBlockAttrs("sysctl", "string", false), + "storage_opt": hclspec.NewAttr("storage_opt", "list(map(string))", false), + "sysctl": hclspec.NewAttr("sysctl", "list(map(string))", false), "tty": hclspec.NewAttr("tty", "bool", false), - "ulimit": hclspec.NewBlockAttrs("ulimit", "string", false), + "ulimit": hclspec.NewAttr("ulimit", "list(map(string))", false), "uts_mode": hclspec.NewAttr("uts_mode", "string", false), "userns_mode": hclspec.NewAttr("userns_mode", "string", false), "volumes": hclspec.NewAttr("volumes", "list(string)", false), @@ -312,51 +313,51 @@ var ( ) type TaskConfig struct { - Image string `codec:"image"` - AdvertiseIPv6Addr bool `codec:"advertise_ipv6_address"` - Args []string `codec:"args"` - Auth DockerAuth `codec:"auth"` - AuthSoftFail bool `codec:"auth_soft_fail"` - CapAdd []string `codec:"cap_add"` - CapDrop []string `codec:"cap_drop"` - Command string `codec:"command"` - CPUCFSPeriod int64 `codec:"cpu_cfs_period"` - CPUHardLimit bool `codec:"cpu_hard_limit"` - Devices []DockerDevice `codec:"devices"` - DNSSearchDomains []string `codec:"dns_search_domains"` - DNSOptions []string `codec:"dns_options"` - DNSServers []string `codec:"dns_servers"` - Entrypoint []string `codec:"entrypoint"` - ExtraHosts []string `codec:"extra_hosts"` - ForcePull bool `codec:"force_pull"` - Hostname string `codec:"hostname"` - Interactive bool `codec:"interactive"` - IPCMode string `codec:"ipc_mode"` - IPv4Address string `codec:"ipv4_address"` - IPv6Address string `codec:"ipv6_address"` - Labels map[string]string `codec:"labels"` - LoadImage string `codec:"load"` - Logging DockerLogging `codec:"logging"` - MacAddress string `codec:"mac_address"` - Mounts []DockerMount `codec:"mounts"` - NetworkAliases []string `codec:"network_aliases"` - NetworkMode string `codec:"network_mode"` - PidsLimit int64 `codec:"pids_limit"` - PidMode string `codec:"pid_mode"` - PortMap map[string]int `codec:"port_map"` - Privileged bool `codec:"privileged"` - ReadonlyRootfs bool `codec:"readonly_rootfs"` - SecurityOpt []string `codec:"security_opt"` - ShmSize int64 `codec:"shm_size"` - StorageOpt map[string]string `codec:"storage_opt"` - Sysctl map[string]string `codec:"sysctl"` - TTY bool `codec:"tty"` - Ulimit map[string]string `codec:"ulimit"` - UTSMode string `codec:"uts_mode"` - UsernsMode string `codec:"userns_mode"` - Volumes []string `codec:"volumes"` - VolumeDriver string `codec:"volume_driver"` - WorkDir string `codec:"work_dir"` + Image string `codec:"image"` + AdvertiseIPv6Addr bool `codec:"advertise_ipv6_address"` + Args []string `codec:"args"` + Auth DockerAuth `codec:"auth"` + AuthSoftFail bool `codec:"auth_soft_fail"` + CapAdd []string `codec:"cap_add"` + CapDrop []string `codec:"cap_drop"` + Command string `codec:"command"` + CPUCFSPeriod int64 `codec:"cpu_cfs_period"` + CPUHardLimit bool `codec:"cpu_hard_limit"` + Devices []DockerDevice `codec:"devices"` + DNSSearchDomains []string `codec:"dns_search_domains"` + DNSOptions []string `codec:"dns_options"` + DNSServers []string `codec:"dns_servers"` + Entrypoint []string `codec:"entrypoint"` + ExtraHosts []string `codec:"extra_hosts"` + ForcePull bool `codec:"force_pull"` + Hostname string `codec:"hostname"` + Interactive bool `codec:"interactive"` + IPCMode string `codec:"ipc_mode"` + IPv4Address string `codec:"ipv4_address"` + IPv6Address string `codec:"ipv6_address"` + Labels hclutils.MapStrStr `codec:"labels"` + LoadImage string `codec:"load"` + Logging DockerLogging `codec:"logging"` + MacAddress string `codec:"mac_address"` + Mounts []DockerMount `codec:"mounts"` + NetworkAliases []string `codec:"network_aliases"` + NetworkMode string `codec:"network_mode"` + PidsLimit int64 `codec:"pids_limit"` + PidMode string `codec:"pid_mode"` + PortMap hclutils.MapStrInt `codec:"port_map"` + Privileged bool `codec:"privileged"` + ReadonlyRootfs bool `codec:"readonly_rootfs"` + SecurityOpt []string `codec:"security_opt"` + ShmSize int64 `codec:"shm_size"` + StorageOpt hclutils.MapStrStr `codec:"storage_opt"` + Sysctl hclutils.MapStrStr `codec:"sysctl"` + TTY bool `codec:"tty"` + Ulimit hclutils.MapStrStr `codec:"ulimit"` + UTSMode string `codec:"uts_mode"` + UsernsMode string `codec:"userns_mode"` + Volumes []string `codec:"volumes"` + VolumeDriver string `codec:"volume_driver"` + WorkDir string `codec:"work_dir"` } type DockerAuth struct { @@ -395,8 +396,8 @@ func (d DockerDevice) toDockerDevice() (docker.Device, error) { } type DockerLogging struct { - Type string `codec:"type"` - Config map[string]string `codec:"config"` + Type string `codec:"type"` + Config hclutils.MapStrStr `codec:"config"` } type DockerMount struct { @@ -454,7 +455,7 @@ func (m DockerMount) toDockerHostMount() (docker.HostMount, error) { type DockerVolumeOptions struct { NoCopy bool `codec:"no_copy"` - Labels map[string]string `codec:"labels"` + Labels hclutils.MapStrStr `codec:"labels"` DriverConfig DockerVolumeDriverConfig `codec:"driver_config"` } @@ -469,8 +470,8 @@ type DockerTmpfsOptions struct { // DockerVolumeDriverConfig holds a map of volume driver specific options type DockerVolumeDriverConfig struct { - Name string `codec:"name"` - Options map[string]string `codec:"options"` + Name string `codec:"name"` + Options hclutils.MapStrStr `codec:"options"` } type DriverConfig struct { diff --git a/drivers/docker/config_test.go b/drivers/docker/config_test.go index 5d759aeca..95ca5a4e2 100644 --- a/drivers/docker/config_test.go +++ b/drivers/docker/config_test.go @@ -69,6 +69,78 @@ func TestConfig_ParseJSON(t *testing.T) { } } +func TestConfig_PortMap_Deserialization(t *testing.T) { + parser := hclutils.NewConfigParser(taskConfigSpec) + + expectedMap := map[string]int{ + "ssh": 25, + "http": 80, + "https": 443, + } + + t.Run("parsing hcl block case", func(t *testing.T) { + validHCL := ` +config { + image = "redis" + port_map { + ssh = 25 + http = 80 + https = 443 + } +}` + + var tc *TaskConfig + parser.ParseHCL(t, validHCL, &tc) + + require.EqualValues(t, expectedMap, tc.PortMap) + }) + + t.Run("parsing hcl assignment case", func(t *testing.T) { + validHCL := ` +config { + image = "redis" + port_map = { + ssh = 25 + http = 80 + https = 443 + } +}` + + var tc *TaskConfig + parser.ParseHCL(t, validHCL, &tc) + + require.EqualValues(t, expectedMap, tc.PortMap) + }) + + validJsons := []struct { + name string + json string + }{ + { + "single map in an array", + `{"Config": {"image": "redis", "port_map": [{"ssh": 25, "http": 80, "https": 443}]}}`, + }, + { + "array of single map entries", + `{"Config": {"image": "redis", "port_map": [{"ssh": 25}, {"http": 80}, {"https": 443}]}}`, + }, + { + "array of maps", + `{"Config": {"image": "redis", "port_map": [{"ssh": 25, "http": 80}, {"https": 443}]}}`, + }, + } + + for _, c := range validJsons { + t.Run("json:"+c.name, func(t *testing.T) { + var tc *TaskConfig + parser.ParseJson(t, c.json, &tc) + + require.EqualValues(t, expectedMap, tc.PortMap) + }) + } + +} + func TestConfig_ParseAllHCL(t *testing.T) { cfgStr := ` config { diff --git a/drivers/qemu/driver.go b/drivers/qemu/driver.go index 65df76668..a4fef4b39 100644 --- a/drivers/qemu/driver.go +++ b/drivers/qemu/driver.go @@ -16,6 +16,7 @@ import ( hclog "github.com/hashicorp/go-hclog" "github.com/hashicorp/nomad/drivers/shared/eventer" "github.com/hashicorp/nomad/drivers/shared/executor" + "github.com/hashicorp/nomad/helper/pluginutils/hclutils" "github.com/hashicorp/nomad/helper/pluginutils/loader" "github.com/hashicorp/nomad/plugins/base" "github.com/hashicorp/nomad/plugins/drivers" @@ -90,7 +91,7 @@ var ( "accelerator": hclspec.NewAttr("accelerator", "string", false), "graceful_shutdown": hclspec.NewAttr("graceful_shutdown", "bool", false), "args": hclspec.NewAttr("args", "list(string)", false), - "port_map": hclspec.NewBlockAttrs("port_map", "number", false), + "port_map": hclspec.NewAttr("port_map", "list(map(number))", false), }) // capabilities is returned by the Capabilities RPC and indicates what @@ -106,11 +107,11 @@ var ( // TaskConfig is the driver configuration of a taskConfig within a job type TaskConfig struct { - ImagePath string `codec:"image_path"` - Accelerator string `codec:"accelerator"` - Args []string `codec:"args"` // extra arguments to qemu executable - PortMap map[string]int `codec:"port_map"` // A map of host port and the port name defined in the image manifest file - GracefulShutdown bool `codec:"graceful_shutdown"` + ImagePath string `codec:"image_path"` + Accelerator string `codec:"accelerator"` + Args []string `codec:"args"` // extra arguments to qemu executable + PortMap hclutils.MapStrInt `codec:"port_map"` // A map of host port and the port name defined in the image manifest file + GracefulShutdown bool `codec:"graceful_shutdown"` } // TaskState is the state which is encoded in the handle returned in StartTask. diff --git a/drivers/rkt/driver.go b/drivers/rkt/driver.go index 1acd672db..9cbeb79ec 100644 --- a/drivers/rkt/driver.go +++ b/drivers/rkt/driver.go @@ -27,6 +27,7 @@ import ( "github.com/hashicorp/nomad/drivers/shared/eventer" "github.com/hashicorp/nomad/drivers/shared/executor" "github.com/hashicorp/nomad/helper" + "github.com/hashicorp/nomad/helper/pluginutils/hclutils" "github.com/hashicorp/nomad/helper/pluginutils/loader" "github.com/hashicorp/nomad/plugins/base" "github.com/hashicorp/nomad/plugins/drivers" @@ -110,7 +111,7 @@ var ( "dns_servers": hclspec.NewAttr("dns_servers", "list(string)", false), "dns_search_domains": hclspec.NewAttr("dns_search_domains", "list(string)", false), "net": hclspec.NewAttr("net", "list(string)", false), - "port_map": hclspec.NewBlockAttrs("port_map", "string", false), + "port_map": hclspec.NewAttr("port_map", "list(map(string))", false), "volumes": hclspec.NewAttr("volumes", "list(string)", false), "insecure_options": hclspec.NewAttr("insecure_options", "list(string)", false), "no_overlay": hclspec.NewAttr("no_overlay", "bool", false), @@ -140,16 +141,16 @@ type Config struct { // TaskConfig is the driver configuration of a taskConfig within a job type TaskConfig struct { - ImageName string `codec:"image"` - Command string `codec:"command"` - Args []string `codec:"args"` - TrustPrefix string `codec:"trust_prefix"` - DNSServers []string `codec:"dns_servers"` // DNS Server for containers - DNSSearchDomains []string `codec:"dns_search_domains"` // DNS Search domains for containers - Net []string `codec:"net"` // Networks for the containers - PortMap map[string]string `codec:"port_map"` // A map of host port and the port name defined in the image manifest file - Volumes []string `codec:"volumes"` // Host-Volumes to mount in, syntax: /path/to/host/directory:/destination/path/in/container[:readOnly] - InsecureOptions []string `codec:"insecure_options"` // list of args for --insecure-options + ImageName string `codec:"image"` + Command string `codec:"command"` + Args []string `codec:"args"` + TrustPrefix string `codec:"trust_prefix"` + DNSServers []string `codec:"dns_servers"` // DNS Server for containers + DNSSearchDomains []string `codec:"dns_search_domains"` // DNS Search domains for containers + Net []string `codec:"net"` // Networks for the containers + PortMap hclutils.MapStrStr `codec:"port_map"` // A map of host port and the port name defined in the image manifest file + Volumes []string `codec:"volumes"` // Host-Volumes to mount in, syntax: /path/to/host/directory:/destination/path/in/container[:readOnly] + InsecureOptions []string `codec:"insecure_options"` // list of args for --insecure-options NoOverlay bool `codec:"no_overlay"` // disable overlayfs for rkt run Debug bool `codec:"debug"` // Enable debug option for rkt command diff --git a/helper/pluginutils/hclutils/types.go b/helper/pluginutils/hclutils/types.go new file mode 100644 index 000000000..24a338484 --- /dev/null +++ b/helper/pluginutils/hclutils/types.go @@ -0,0 +1,51 @@ +package hclutils + +import ( + "github.com/ugorji/go/codec" +) + +// MapStrInt is a wrapper for map[string]int that handles +// deserialization from different hcl2 json representation +// that were supported in Nomad 0.8 +type MapStrInt map[string]int + +func (s *MapStrInt) CodecEncodeSelf(enc *codec.Encoder) { + v := []map[string]int{*s} + enc.MustEncode(v) +} + +func (s *MapStrInt) CodecDecodeSelf(dec *codec.Decoder) { + ms := []map[string]int{} + dec.MustDecode(&ms) + + r := map[string]int{} + for _, m := range ms { + for k, v := range m { + r[k] = v + } + } + *s = r +} + +// MapStrStr is a wrapper for map[string]string that handles +// deserialization from different hcl2 json representation +// that were supported in Nomad 0.8 +type MapStrStr map[string]string + +func (s *MapStrStr) CodecEncodeSelf(enc *codec.Encoder) { + v := []map[string]string{*s} + enc.MustEncode(v) +} + +func (s *MapStrStr) CodecDecodeSelf(dec *codec.Decoder) { + ms := []map[string]string{} + dec.MustDecode(&ms) + + r := map[string]string{} + for _, m := range ms { + for k, v := range m { + r[k] = v + } + } + *s = r +} diff --git a/helper/pluginutils/hclutils/types_test.go b/helper/pluginutils/hclutils/types_test.go new file mode 100644 index 000000000..26a762962 --- /dev/null +++ b/helper/pluginutils/hclutils/types_test.go @@ -0,0 +1,139 @@ +package hclutils_test + +import ( + "testing" + + "github.com/hashicorp/nomad/helper/pluginutils/hclutils" + "github.com/hashicorp/nomad/plugins/shared/hclspec" + "github.com/stretchr/testify/require" +) + +func TestMapStrInt_JsonArrays(t *testing.T) { + spec := hclspec.NewObject(map[string]*hclspec.Spec{ + "port_map": hclspec.NewAttr("port_map", "list(map(number))", false), + }) + + type PidMapTaskConfig struct { + PortMap hclutils.MapStrInt `codec:"port_map"` + } + + parser := hclutils.NewConfigParser(spec) + + expected := PidMapTaskConfig{ + PortMap: map[string]int{ + "http": 80, + "https": 443, + "ssh": 25, + }, + } + + t.Run("hcl case", func(t *testing.T) { + config := ` +config { + port_map { + http = 80 + https = 443 + ssh = 25 + } +}` + // Test decoding + var tc PidMapTaskConfig + parser.ParseHCL(t, config, &tc) + + require.EqualValues(t, expected, tc) + + }) + jsonCases := []struct { + name string + json string + }{ + { + "array of map entries", + `{"Config": {"port_map": [{"http": 80}, {"https": 443}, {"ssh": 25}]}}`, + }, + { + "array with one map", + `{"Config": {"port_map": [{"http": 80, "https": 443, "ssh": 25}]}}`, + }, + { + "array of maps", + `{"Config": {"port_map": [{"http": 80, "https": 443}, {"ssh": 25}]}}`, + }, + } + + for _, c := range jsonCases { + t.Run("json:"+c.name, func(t *testing.T) { + // Test decoding + var tc PidMapTaskConfig + parser.ParseJson(t, c.json, &tc) + + require.EqualValues(t, expected, tc) + + }) + } +} + +func TestMapStrStr_JsonArrays(t *testing.T) { + spec := hclspec.NewObject(map[string]*hclspec.Spec{ + "port_map": hclspec.NewAttr("port_map", "list(map(string))", false), + }) + + type PidMapTaskConfig struct { + PortMap hclutils.MapStrStr `codec:"port_map"` + } + + parser := hclutils.NewConfigParser(spec) + + expected := PidMapTaskConfig{ + PortMap: map[string]string{ + "http": "80", + "https": "443", + "ssh": "25", + }, + } + + t.Run("hcl case", func(t *testing.T) { + config := ` +config { + port_map { + http = "80" + https = "443" + ssh = "25" + } +}` + // Test decoding + var tc PidMapTaskConfig + parser.ParseHCL(t, config, &tc) + + require.EqualValues(t, expected, tc) + + }) + jsonCases := []struct { + name string + json string + }{ + { + "array of map entries", + `{"Config": {"port_map": [{"http": "80"}, {"https": "443"}, {"ssh": "25"}]}}`, + }, + { + "array with one map", + `{"Config": {"port_map": [{"http": "80", "https": "443", "ssh": "25"}]}}`, + }, + { + "array of maps", + `{"Config": {"port_map": [{"http": "80", "https": "443"}, {"ssh": "25"}]}}`, + }, + } + + for _, c := range jsonCases { + t.Run("json:"+c.name, func(t *testing.T) { + // Test decoding + var tc PidMapTaskConfig + parser.ParseJson(t, c.json, &tc) + + require.EqualValues(t, expected, tc) + + }) + } +}