From 182241c8d3564c50d5f3c9f759386ae07fe5fdda Mon Sep 17 00:00:00 2001 From: Alex Crawford Date: Sat, 20 Dec 2014 17:47:17 -0800 Subject: [PATCH 1/9] config: clean up and remove some tests Small modification to make these align with our test-table-style tests. Also removed TestCloudConfigInvalidKeys since it hasn't been a useful test since d3294bcb86b85e59bc79687b589d1bb64c24d996. --- config/config_test.go | 46 +++++++++++-------------------------------- 1 file changed, 12 insertions(+), 34 deletions(-) diff --git a/config/config_test.go b/config/config_test.go index 29171ad..68a42cc 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -23,8 +23,9 @@ import ( ) func TestIsZero(t *testing.T) { - for _, tt := range []struct { - c interface{} + tests := []struct { + c interface{} + empty bool }{ {struct{}{}, true}, @@ -34,7 +35,9 @@ func TestIsZero(t *testing.T) { {struct{ A string }{A: "hello"}, false}, {struct{ A int }{}, true}, {struct{ A int }{A: 1}, false}, - } { + } + + for _, tt := range tests { if empty := IsZero(tt.c); tt.empty != empty { t.Errorf("bad result (%q): want %t, got %t", tt.c, tt.empty, empty) } @@ -42,8 +45,9 @@ func TestIsZero(t *testing.T) { } func TestAssertStructValid(t *testing.T) { - for _, tt := range []struct { - c interface{} + tests := []struct { + c interface{} + err error }{ {struct{}{}, nil}, @@ -71,41 +75,15 @@ func TestAssertStructValid(t *testing.T) { {struct { A, b int `valid:"1,2"` }{A: 9, b: 2}, &ErrorValid{Value: "9", Field: "A", Valid: []string{"1", "2"}}}, - } { + } + + for _, tt := range tests { if err := AssertStructValid(tt.c); !reflect.DeepEqual(tt.err, err) { t.Errorf("bad result (%q): want %q, got %q", tt.c, tt.err, err) } } } -func TestCloudConfigInvalidKeys(t *testing.T) { - defer func() { - if r := recover(); r != nil { - t.Fatalf("panic while instantiating CloudConfig with nil keys: %v", r) - } - }() - - for _, tt := range []struct { - contents string - }{ - {"coreos:"}, - {"ssh_authorized_keys:"}, - {"ssh_authorized_keys:\n -"}, - {"ssh_authorized_keys:\n - 0:"}, - {"write_files:"}, - {"write_files:\n -"}, - {"write_files:\n - 0:"}, - {"users:"}, - {"users:\n -"}, - {"users:\n - 0:"}, - } { - _, err := NewCloudConfig(tt.contents) - if err != nil { - t.Fatalf("error instantiating CloudConfig with invalid keys: %v", err) - } - } -} - func TestCloudConfigUnknownKeys(t *testing.T) { contents := ` coreos: From 057ab3736452bd0bad244bb4130f9ceac9f15e5c Mon Sep 17 00:00:00 2001 From: Alex Crawford Date: Sun, 21 Dec 2014 12:14:23 -0800 Subject: [PATCH 2/9] config: seperate the CoreOS type from CloudConfig Renamed Coreos to CoreOS while I was at it. --- config/config.go | 20 +++++++++++--------- config/config_test.go | 12 ++++++------ initialize/config.go | 16 ++++++++-------- 3 files changed, 25 insertions(+), 23 deletions(-) diff --git a/config/config.go b/config/config.go index 9278ecc..1cdad17 100644 --- a/config/config.go +++ b/config/config.go @@ -29,15 +29,7 @@ import ( // used for internal use) have the YAML tag '-' so that they aren't marshalled. type CloudConfig struct { SSHAuthorizedKeys []string `yaml:"ssh_authorized_keys"` - Coreos struct { - Etcd Etcd `yaml:"etcd"` - Flannel Flannel `yaml:"flannel"` - Fleet Fleet `yaml:"fleet"` - Locksmith Locksmith `yaml:"locksmith"` - OEM OEM `yaml:"oem"` - Update Update `yaml:"update"` - Units []Unit `yaml:"units"` - } `yaml:"coreos"` + CoreOS CoreOS `yaml:"coreos"` WriteFiles []File `yaml:"write_files"` Hostname string `yaml:"hostname"` Users []User `yaml:"users"` @@ -46,6 +38,16 @@ type CloudConfig struct { NetworkConfig string `yaml:"-"` } +type CoreOS struct { + Etcd Etcd `yaml:"etcd"` + Flannel Flannel `yaml:"flannel"` + Fleet Fleet `yaml:"fleet"` + Locksmith Locksmith `yaml:"locksmith"` + OEM OEM `yaml:"oem"` + Update Update `yaml:"update"` + Units []Unit `yaml:"units"` +} + func IsCloudConfig(userdata string) bool { header := strings.SplitN(userdata, "\n", 2)[0] diff --git a/config/config_test.go b/config/config_test.go index 68a42cc..7cc6756 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -114,7 +114,7 @@ hostname: if cfg.Hostname != "foo" { t.Fatalf("hostname not correctly set when invalid keys are present") } - if cfg.Coreos.Etcd.Discovery != "https://discovery.etcd.io/827c73219eeb2fa5530027c37bf18877" { + if cfg.CoreOS.Etcd.Discovery != "https://discovery.etcd.io/827c73219eeb2fa5530027c37bf18877" { t.Fatalf("etcd section not correctly set when invalid keys are present") } if len(cfg.WriteFiles) < 1 || cfg.WriteFiles[0].Content != "fun" || cfg.WriteFiles[0].Path != "/var/party" { @@ -220,10 +220,10 @@ hostname: trontastic } } - if len(cfg.Coreos.Units) != 1 { + if len(cfg.CoreOS.Units) != 1 { t.Error("Failed to parse correct number of units") } else { - u := cfg.Coreos.Units[0] + u := cfg.CoreOS.Units[0] expect := `[Match] Name=eth47 @@ -241,14 +241,14 @@ Address=10.209.171.177/19 } } - if cfg.Coreos.OEM.ID != "rackspace" { - t.Errorf("Failed parsing coreos.oem. Expected ID 'rackspace', got %q.", cfg.Coreos.OEM.ID) + if cfg.CoreOS.OEM.ID != "rackspace" { + t.Errorf("Failed parsing coreos.oem. Expected ID 'rackspace', got %q.", cfg.CoreOS.OEM.ID) } if cfg.Hostname != "trontastic" { t.Errorf("Failed to parse hostname") } - if cfg.Coreos.Update.RebootStrategy != "reboot" { + if cfg.CoreOS.Update.RebootStrategy != "reboot" { t.Errorf("Failed to parse locksmith strategy") } diff --git a/initialize/config.go b/initialize/config.go index 99e18c1..034ba97 100644 --- a/initialize/config.go +++ b/initialize/config.go @@ -110,10 +110,10 @@ func Apply(cfg config.CloudConfig, env *Environment) error { } for _, ccf := range []CloudConfigFile{ - system.OEM{OEM: cfg.Coreos.OEM}, - system.Update{Update: cfg.Coreos.Update, ReadConfig: system.DefaultReadConfig}, + system.OEM{OEM: cfg.CoreOS.OEM}, + system.Update{Update: cfg.CoreOS.Update, ReadConfig: system.DefaultReadConfig}, system.EtcHosts{EtcHosts: cfg.ManageEtcHosts}, - system.Flannel{Flannel: cfg.Coreos.Flannel}, + system.Flannel{Flannel: cfg.CoreOS.Flannel}, } { f, err := ccf.File() if err != nil { @@ -125,15 +125,15 @@ func Apply(cfg config.CloudConfig, env *Environment) error { } var units []system.Unit - for _, u := range cfg.Coreos.Units { + for _, u := range cfg.CoreOS.Units { units = append(units, system.Unit{Unit: u}) } for _, ccu := range []CloudConfigUnit{ - system.Etcd{Etcd: cfg.Coreos.Etcd}, - system.Fleet{Fleet: cfg.Coreos.Fleet}, - system.Locksmith{Locksmith: cfg.Coreos.Locksmith}, - system.Update{Update: cfg.Coreos.Update, ReadConfig: system.DefaultReadConfig}, + system.Etcd{Etcd: cfg.CoreOS.Etcd}, + system.Fleet{Fleet: cfg.CoreOS.Fleet}, + system.Locksmith{Locksmith: cfg.CoreOS.Locksmith}, + system.Update{Update: cfg.CoreOS.Update, ReadConfig: system.DefaultReadConfig}, } { units = append(units, ccu.Units()...) } From 4ed1d03c976b3fad35a87d1c10c503df6b079f7e Mon Sep 17 00:00:00 2001 From: Alex Crawford Date: Wed, 24 Dec 2014 13:16:47 -0800 Subject: [PATCH 3/9] godeps: bump github.com/coreos/yaml --- Godeps/Godeps.json | 2 +- .../src/github.com/coreos/yaml/README.md | 3 +++ .../src/github.com/coreos/yaml/decode.go | 17 +++++++++++------ .../src/github.com/coreos/yaml/decode_test.go | 19 ++++++++++++++++++- .../src/github.com/coreos/yaml/encode_test.go | 2 +- .../src/github.com/coreos/yaml/yaml.go | 13 ++++++++++++- 6 files changed, 46 insertions(+), 10 deletions(-) diff --git a/Godeps/Godeps.json b/Godeps/Godeps.json index ee26a08..77a58e9 100644 --- a/Godeps/Godeps.json +++ b/Godeps/Godeps.json @@ -15,7 +15,7 @@ }, { "ImportPath": "github.com/coreos/yaml", - "Rev": "9f9df34309c04878acc86042b16630b0f696e1de" + "Rev": "6b16a5714269b2f70720a45406b1babd947a17ef" }, { "ImportPath": "github.com/dotcloud/docker/pkg/netlink", diff --git a/Godeps/_workspace/src/github.com/coreos/yaml/README.md b/Godeps/_workspace/src/github.com/coreos/yaml/README.md index af07056..4427005 100644 --- a/Godeps/_workspace/src/github.com/coreos/yaml/README.md +++ b/Godeps/_workspace/src/github.com/coreos/yaml/README.md @@ -1,3 +1,6 @@ +Note: This is a fork of https://github.com/go-yaml/yaml. The following README +doesn't necessarily apply to this fork. + # YAML support for the Go language Introduction diff --git a/Godeps/_workspace/src/github.com/coreos/yaml/decode.go b/Godeps/_workspace/src/github.com/coreos/yaml/decode.go index a098626..e219d4b 100644 --- a/Godeps/_workspace/src/github.com/coreos/yaml/decode.go +++ b/Godeps/_workspace/src/github.com/coreos/yaml/decode.go @@ -30,13 +30,15 @@ type node struct { // Parser, produces a node tree out of a libyaml event stream. type parser struct { - parser yaml_parser_t - event yaml_event_t - doc *node + parser yaml_parser_t + event yaml_event_t + doc *node + transform transformString } -func newParser(b []byte) *parser { - p := parser{} +func newParser(b []byte, t transformString) *parser { + p := parser{transform: t} + if !yaml_parser_initialize(&p.parser) { panic("Failed to initialize YAML emitter") } @@ -175,7 +177,10 @@ func (p *parser) mapping() *node { p.anchor(n, p.event.anchor) p.skip() for p.event.typ != yaml_MAPPING_END_EVENT { - n.children = append(n.children, p.parse(), p.parse()) + key := p.parse() + key.value = p.transform(key.value) + value := p.parse() + n.children = append(n.children, key, value) } p.skip() return n diff --git a/Godeps/_workspace/src/github.com/coreos/yaml/decode_test.go b/Godeps/_workspace/src/github.com/coreos/yaml/decode_test.go index ef3d37f..349bee7 100644 --- a/Godeps/_workspace/src/github.com/coreos/yaml/decode_test.go +++ b/Godeps/_workspace/src/github.com/coreos/yaml/decode_test.go @@ -1,8 +1,8 @@ package yaml_test import ( + "github.com/coreos/yaml" . "gopkg.in/check.v1" - "gopkg.in/yaml.v1" "math" "reflect" "strings" @@ -557,6 +557,23 @@ func (s *S) TestUnmarshalWithFalseSetterIgnoresValue(c *C) { c.Assert(m["ghi"].value, Equals, 3) } +func (s *S) TestUnmarshalWithTransform(c *C) { + data := `{a_b: 1, c-d: 2, e-f_g: 3, h_i-j: 4}` + expect := map[string]int{ + "a_b": 1, + "c_d": 2, + "e_f_g": 3, + "h_i_j": 4, + } + m := map[string]int{} + yaml.UnmarshalMappingKeyTransform = func(i string) string { + return strings.Replace(i, "-", "_", -1) + } + err := yaml.Unmarshal([]byte(data), m) + c.Assert(err, IsNil) + c.Assert(m, DeepEquals, expect) +} + // From http://yaml.org/type/merge.html var mergeTests = ` anchors: diff --git a/Godeps/_workspace/src/github.com/coreos/yaml/encode_test.go b/Godeps/_workspace/src/github.com/coreos/yaml/encode_test.go index c9febc2..2cd0ea7 100644 --- a/Godeps/_workspace/src/github.com/coreos/yaml/encode_test.go +++ b/Godeps/_workspace/src/github.com/coreos/yaml/encode_test.go @@ -7,8 +7,8 @@ import ( "strings" "time" + "github.com/coreos/yaml" . "gopkg.in/check.v1" - "gopkg.in/yaml.v1" ) var marshalIntTest = 123 diff --git a/Godeps/_workspace/src/github.com/coreos/yaml/yaml.go b/Godeps/_workspace/src/github.com/coreos/yaml/yaml.go index f1c390e..16e1365 100644 --- a/Godeps/_workspace/src/github.com/coreos/yaml/yaml.go +++ b/Godeps/_workspace/src/github.com/coreos/yaml/yaml.go @@ -84,7 +84,7 @@ type Getter interface { func Unmarshal(in []byte, out interface{}) (err error) { defer handleErr(&err) d := newDecoder() - p := newParser(in) + p := newParser(in, UnmarshalMappingKeyTransform) defer p.destroy() node := p.parse() if node != nil { @@ -146,6 +146,17 @@ func Marshal(in interface{}) (out []byte, err error) { return } +// UnmarshalMappingKeyTransform is a string transformation that is applied to +// each mapping key in a YAML document before it is unmarshalled. By default, +// UnmarshalMappingKeyTransform is an identity transform (no modification). +var UnmarshalMappingKeyTransform transformString = identityTransform + +type transformString func(in string) (out string) + +func identityTransform(in string) (out string) { + return in +} + // -------------------------------------------------------------------------- // Maintain a mapping of keys to structure field indexes From 248536a5cd9e2c21b8ce42fb16c5cb0993bdac10 Mon Sep 17 00:00:00 2001 From: Alex Crawford Date: Sat, 20 Dec 2014 17:57:15 -0800 Subject: [PATCH 4/9] config: use a YAML transform to normalize keys This removes the problematic two-pass unmarshalling. --- config/config.go | 41 +++--------------- config/config_test.go | 98 ++++++++++++++++--------------------------- 2 files changed, 41 insertions(+), 98 deletions(-) diff --git a/config/config.go b/config/config.go index 1cdad17..9a80ff3 100644 --- a/config/config.go +++ b/config/config.go @@ -63,15 +63,12 @@ func IsCloudConfig(userdata string) bool { // string of YAML), returning any error encountered. It will ignore unknown // fields but log encountering them. func NewCloudConfig(contents string) (*CloudConfig, error) { + yaml.UnmarshalMappingKeyTransform = func(nameIn string) (nameOut string) { + return strings.Replace(nameIn, "-", "_", -1) + } var cfg CloudConfig - ncontents, err := normalizeConfig(contents) - if err != nil { - return &cfg, err - } - if err = yaml.Unmarshal(ncontents, &cfg); err != nil { - return &cfg, err - } - return &cfg, nil + err := yaml.Unmarshal([]byte(contents), &cfg) + return &cfg, err } func (cc CloudConfig) String() string { @@ -159,31 +156,3 @@ func isZero(v reflect.Value) bool { func isFieldExported(f reflect.StructField) bool { return f.PkgPath == "" } - -func normalizeConfig(config string) ([]byte, error) { - var cfg map[interface{}]interface{} - if err := yaml.Unmarshal([]byte(config), &cfg); err != nil { - return nil, err - } - return yaml.Marshal(normalizeKeys(cfg)) -} - -func normalizeKeys(m map[interface{}]interface{}) map[interface{}]interface{} { - for k, v := range m { - if m, ok := m[k].(map[interface{}]interface{}); ok { - normalizeKeys(m) - } - - if s, ok := m[k].([]interface{}); ok { - for _, e := range s { - if m, ok := e.(map[interface{}]interface{}); ok { - normalizeKeys(m) - } - } - } - - delete(m, k) - m[strings.Replace(fmt.Sprint(k), "-", "_", -1)] = v - } - return m -} diff --git a/config/config_test.go b/config/config_test.go index 7cc6756..b1516be 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -22,6 +22,42 @@ import ( "testing" ) +func TestNewCloudConfig(t *testing.T) { + tests := []struct { + contents string + + config CloudConfig + }{ + {}, + { + contents: "#cloud-config\nwrite_files:\n - path: underscore", + config: CloudConfig{WriteFiles: []File{File{Path: "underscore"}}}, + }, + { + contents: "#cloud-config\nwrite-files:\n - path: hyphen", + config: CloudConfig{WriteFiles: []File{File{Path: "hyphen"}}}, + }, + { + contents: "#cloud-config\ncoreos:\n update:\n reboot-strategy: off", + config: CloudConfig{CoreOS: CoreOS{Update: Update{RebootStrategy: "off"}}}, + }, + { + contents: "#cloud-config\ncoreos:\n update:\n reboot-strategy: false", + config: CloudConfig{CoreOS: CoreOS{Update: Update{RebootStrategy: "false"}}}, + }, + } + + for i, tt := range tests { + config, err := NewCloudConfig(tt.contents) + if err != nil { + t.Errorf("bad error (test case #%d): want %v, got %s", i, nil, err) + } + if !reflect.DeepEqual(&tt.config, config) { + t.Errorf("bad config (test case #%d): want %#v, got %#v", i, tt.config, config) + } + } +} + func TestIsZero(t *testing.T) { tests := []struct { c interface{} @@ -251,40 +287,6 @@ Address=10.209.171.177/19 if cfg.CoreOS.Update.RebootStrategy != "reboot" { t.Errorf("Failed to parse locksmith strategy") } - - contents = ` -coreos: -write_files: - - path: /home/me/notes - permissions: 0744 -` - cfg, err = NewCloudConfig(contents) - if err != nil { - t.Fatalf("Encountered unexpected error :%v", err) - } - - if len(cfg.WriteFiles) != 1 { - t.Error("Failed to parse correct number of write_files") - } else { - wf := cfg.WriteFiles[0] - if wf.Content != "" { - t.Errorf("WriteFile has incorrect contents '%s'", wf.Content) - } - if wf.Encoding != "" { - t.Errorf("WriteFile has incorrect encoding %s", wf.Encoding) - } - // Verify that the normalization of the config converted 0744 to its decimal - // representation, 484. - if wf.RawFilePermissions != "484" { - t.Errorf("WriteFile has incorrect permissions %s", wf.RawFilePermissions) - } - if wf.Path != "/home/me/notes" { - t.Errorf("WriteFile has incorrect path %s", wf.Path) - } - if wf.Owner != "" { - t.Errorf("WriteFile has incorrect owner %s", wf.Owner) - } - } } // Assert that our interface conversion doesn't panic @@ -451,31 +453,3 @@ users: t.Errorf("ssh import url is %q, expected 'https://token:x-auth-token@github.enterprise.com/api/v3/polvi/keys'", user.SSHImportURL) } } - -func TestNormalizeKeys(t *testing.T) { - for _, tt := range []struct { - in string - out string - }{ - {"my_key_name: the-value\n", "my_key_name: the-value\n"}, - {"my-key_name: the-value\n", "my_key_name: the-value\n"}, - {"my-key-name: the-value\n", "my_key_name: the-value\n"}, - - {"a:\n- key_name: the-value\n", "a:\n- key_name: the-value\n"}, - {"a:\n- key-name: the-value\n", "a:\n- key_name: the-value\n"}, - - {"a:\n b:\n - key_name: the-value\n", "a:\n b:\n - key_name: the-value\n"}, - {"a:\n b:\n - key-name: the-value\n", "a:\n b:\n - key_name: the-value\n"}, - - {"coreos:\n update:\n reboot-strategy: off\n", "coreos:\n update:\n reboot_strategy: false\n"}, - {"coreos:\n update:\n reboot-strategy: 'off'\n", "coreos:\n update:\n reboot_strategy: \"off\"\n"}, - } { - out, err := normalizeConfig(tt.in) - if err != nil { - t.Fatalf("bad error (%q): want nil, got %s", tt.in, err) - } - if string(out) != tt.out { - t.Fatalf("bad normalization (%q): want %q, got %q", tt.in, tt.out, out) - } - } -} From 40d943fb7a345559ac3d493fffd2caed9216bd2c Mon Sep 17 00:00:00 2001 From: Alex Crawford Date: Sat, 20 Dec 2014 18:04:45 -0800 Subject: [PATCH 5/9] reboot-strategy: remove the 'false' value Since we no longer do a two-stage unmarshal, the 'false' value will no longer be necessary. --- config/update.go | 2 +- system/update.go | 2 +- system/update_test.go | 19 +------------------ 3 files changed, 3 insertions(+), 20 deletions(-) diff --git a/config/update.go b/config/update.go index 723f244..a42a3e1 100644 --- a/config/update.go +++ b/config/update.go @@ -17,7 +17,7 @@ package config type Update struct { - RebootStrategy string `yaml:"reboot_strategy" env:"REBOOT_STRATEGY" valid:"best-effort,etcd-lock,reboot,off,false"` + RebootStrategy string `yaml:"reboot_strategy" env:"REBOOT_STRATEGY" valid:"best-effort,etcd-lock,reboot,off"` Group string `yaml:"group" env:"GROUP"` Server string `yaml:"server" env:"SERVER"` } diff --git a/system/update.go b/system/update.go index bd745da..dd21515 100644 --- a/system/update.go +++ b/system/update.go @@ -126,7 +126,7 @@ func (uc Update) Units() []Unit { Runtime: true, }} - if uc.Update.RebootStrategy == "false" || uc.Update.RebootStrategy == "off" { + if uc.Update.RebootStrategy == "off" { ls.Command = "stop" ls.Mask = true } diff --git a/system/update_test.go b/system/update_test.go index 4ebf70b..822add0 100644 --- a/system/update_test.go +++ b/system/update_test.go @@ -71,15 +71,6 @@ func TestUpdateUnits(t *testing.T) { Runtime: true, }}}, }, - { - config: config.Update{RebootStrategy: "false"}, - units: []Unit{{config.Unit{ - Name: "locksmithd.service", - Command: "stop", - Runtime: true, - Mask: true, - }}}, - }, { config: config.Update{RebootStrategy: "off"}, units: []Unit{{config.Unit{ @@ -109,7 +100,7 @@ func TestUpdateFile(t *testing.T) { }, { config: config.Update{RebootStrategy: "wizzlewazzle"}, - err: &config.ErrorValid{Value: "wizzlewazzle", Field: "RebootStrategy", Valid: []string{"best-effort", "etcd-lock", "reboot", "off", "false"}}, + err: &config.ErrorValid{Value: "wizzlewazzle", Field: "RebootStrategy", Valid: []string{"best-effort", "etcd-lock", "reboot", "off"}}, }, { config: config.Update{Group: "master", Server: "http://foo.com"}, @@ -143,14 +134,6 @@ func TestUpdateFile(t *testing.T) { RawFilePermissions: "0644", }}, }, - { - config: config.Update{RebootStrategy: "false"}, - file: &File{config.File{ - Content: "REBOOT_STRATEGY=false\n", - Path: "etc/coreos/update.conf", - RawFilePermissions: "0644", - }}, - }, { config: config.Update{RebootStrategy: "off"}, file: &File{config.File{ From af8e5905758066b683ebdbbe600ab2d8e08b8bb3 Mon Sep 17 00:00:00 2001 From: Alex Crawford Date: Sat, 20 Dec 2014 23:26:05 -0800 Subject: [PATCH 6/9] config: change valid tag to use regexp A regular expression is much more useful than a list of strings. --- config/config.go | 14 ++++++------- config/config_test.go | 48 ++++++++++++++++++++++++++++++++++--------- config/file.go | 2 +- config/file_test.go | 48 +++++++++++++++++++++++++++++++++++++++++++ config/unit.go | 2 +- config/unit_test.go | 46 +++++++++++++++++++++++++++++++++++++++++ config/update.go | 2 +- config/update_test.go | 43 ++++++++++++++++++++++++++++++++++++++ system/update_test.go | 2 +- 9 files changed, 186 insertions(+), 21 deletions(-) create mode 100644 config/file_test.go create mode 100644 config/unit_test.go create mode 100644 config/update_test.go diff --git a/config/config.go b/config/config.go index 9a80ff3..33eaef9 100644 --- a/config/config.go +++ b/config/config.go @@ -19,6 +19,7 @@ package config import ( "fmt" "reflect" + "regexp" "strings" "github.com/coreos/coreos-cloudinit/Godeps/_workspace/src/github.com/coreos/yaml" @@ -91,7 +92,7 @@ func IsZero(c interface{}) bool { type ErrorValid struct { Value string - Valid []string + Valid string Field string } @@ -125,16 +126,15 @@ func AssertValid(value reflect.Value, valid string) *ErrorValid { if valid == "" || isZero(value) { return nil } + vs := fmt.Sprintf("%v", value.Interface()) - valids := strings.Split(valid, ",") - for _, valid := range valids { - if vs == valid { - return nil - } + if m, _ := regexp.MatchString(valid, vs); m { + return nil } + return &ErrorValid{ Value: vs, - Valid: valids, + Valid: valid, } } diff --git a/config/config_test.go b/config/config_test.go index b1516be..f4b7bc4 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -18,6 +18,7 @@ package config import ( "reflect" + "regexp" "strings" "testing" ) @@ -88,29 +89,29 @@ func TestAssertStructValid(t *testing.T) { }{ {struct{}{}, nil}, {struct { - A, b string `valid:"1,2"` + A, b string `valid:"^1|2$"` }{}, nil}, {struct { - A, b string `valid:"1,2"` + A, b string `valid:"^1|2$"` }{A: "1", b: "2"}, nil}, {struct { - A, b string `valid:"1,2"` + A, b string `valid:"^1|2$"` }{A: "1", b: "hello"}, nil}, {struct { - A, b string `valid:"1,2"` - }{A: "hello", b: "2"}, &ErrorValid{Value: "hello", Field: "A", Valid: []string{"1", "2"}}}, + A, b string `valid:"^1|2$"` + }{A: "hello", b: "2"}, &ErrorValid{Value: "hello", Field: "A", Valid: "^1|2$"}}, {struct { - A, b int `valid:"1,2"` + A, b int `valid:"^1|2$"` }{}, nil}, {struct { - A, b int `valid:"1,2"` + A, b int `valid:"^1|2$"` }{A: 1, b: 2}, nil}, {struct { - A, b int `valid:"1,2"` + A, b int `valid:"^1|2$"` }{A: 1, b: 9}, nil}, {struct { - A, b int `valid:"1,2"` - }{A: 9, b: 2}, &ErrorValid{Value: "9", Field: "A", Valid: []string{"1", "2"}}}, + A, b int `valid:"^1|2$"` + }{A: 9, b: 2}, &ErrorValid{Value: "9", Field: "A", Valid: "^1|2$"}}, } for _, tt := range tests { @@ -120,6 +121,33 @@ func TestAssertStructValid(t *testing.T) { } } +func TestConfigCompile(t *testing.T) { + tests := []interface{}{ + Etcd{}, + File{}, + Flannel{}, + Fleet{}, + Locksmith{}, + OEM{}, + Unit{}, + Update{}, + } + + for _, tt := range tests { + ttt := reflect.TypeOf(tt) + for i := 0; i < ttt.NumField(); i++ { + ft := ttt.Field(i) + if !isFieldExported(ft) { + continue + } + + if _, err := regexp.Compile(ft.Tag.Get("valid")); err != nil { + t.Errorf("bad regexp(%s.%s): want %v, got %s", ttt.Name(), ft.Name, nil, err) + } + } + } +} + func TestCloudConfigUnknownKeys(t *testing.T) { contents := ` coreos: diff --git a/config/file.go b/config/file.go index 26dbc99..04f12c2 100644 --- a/config/file.go +++ b/config/file.go @@ -17,7 +17,7 @@ package config type File struct { - Encoding string `yaml:"encoding" valid:"base64,b64,gz,gzip,gz+base64,gzip+base64,gz+b64,gzip+b64"` + Encoding string `yaml:"encoding" valid:"^(base64|b64|gz|gzip|gz\\+base64|gzip\\+base64|gz\\+b64|gzip\\+b64)$"` Content string `yaml:"content"` Owner string `yaml:"owner"` Path string `yaml:"path"` diff --git a/config/file_test.go b/config/file_test.go new file mode 100644 index 0000000..a1e03e2 --- /dev/null +++ b/config/file_test.go @@ -0,0 +1,48 @@ +/* + Copyright 2014 CoreOS, Inc. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package config + +import ( + "testing" +) + +func TestEncodingValid(t *testing.T) { + tests := []struct { + value string + + isValid bool + }{ + {value: "base64", isValid: true}, + {value: "b64", isValid: true}, + {value: "gz", isValid: true}, + {value: "gzip", isValid: true}, + {value: "gz+base64", isValid: true}, + {value: "gzip+base64", isValid: true}, + {value: "gz+b64", isValid: true}, + {value: "gzip+b64", isValid: true}, + {value: "gzzzzbase64", isValid: false}, + {value: "gzipppbase64", isValid: false}, + {value: "unknown", isValid: false}, + } + + for _, tt := range tests { + isValid := (nil == AssertStructValid(File{Encoding: tt.value})) + if tt.isValid != isValid { + t.Errorf("bad assert (%s): want %t, got %t", tt.value, tt.isValid, isValid) + } + } +} diff --git a/config/unit.go b/config/unit.go index 3a09bbb..a86bc8c 100644 --- a/config/unit.go +++ b/config/unit.go @@ -22,7 +22,7 @@ type Unit struct { Enable bool `yaml:"enable"` Runtime bool `yaml:"runtime"` Content string `yaml:"content"` - Command string `yaml:"command" valid:"start,stop,restart,reload,try-restart,reload-or-restart,reload-or-try-restart"` + Command string `yaml:"command" valid:"^(start|stop|restart|reload|try-restart|reload-or-restart|reload-or-try-restart)$"` DropIns []UnitDropIn `yaml:"drop_ins"` } diff --git a/config/unit_test.go b/config/unit_test.go new file mode 100644 index 0000000..d94dd8b --- /dev/null +++ b/config/unit_test.go @@ -0,0 +1,46 @@ +/* + Copyright 2014 CoreOS, Inc. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package config + +import ( + "testing" +) + +func TestCommandValid(t *testing.T) { + tests := []struct { + value string + + isValid bool + }{ + {value: "start", isValid: true}, + {value: "stop", isValid: true}, + {value: "restart", isValid: true}, + {value: "reload", isValid: true}, + {value: "try-restart", isValid: true}, + {value: "reload-or-restart", isValid: true}, + {value: "reload-or-try-restart", isValid: true}, + {value: "tryrestart", isValid: false}, + {value: "unknown", isValid: false}, + } + + for _, tt := range tests { + isValid := (nil == AssertStructValid(Unit{Command: tt.value})) + if tt.isValid != isValid { + t.Errorf("bad assert (%s): want %t, got %t", tt.value, tt.isValid, isValid) + } + } +} diff --git a/config/update.go b/config/update.go index a42a3e1..147085d 100644 --- a/config/update.go +++ b/config/update.go @@ -17,7 +17,7 @@ package config type Update struct { - RebootStrategy string `yaml:"reboot_strategy" env:"REBOOT_STRATEGY" valid:"best-effort,etcd-lock,reboot,off"` + RebootStrategy string `yaml:"reboot_strategy" env:"REBOOT_STRATEGY" valid:"^(best-effort|etcd-lock|reboot|off)$"` Group string `yaml:"group" env:"GROUP"` Server string `yaml:"server" env:"SERVER"` } diff --git a/config/update_test.go b/config/update_test.go new file mode 100644 index 0000000..2b0851e --- /dev/null +++ b/config/update_test.go @@ -0,0 +1,43 @@ +/* + Copyright 2014 CoreOS, Inc. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package config + +import ( + "testing" +) + +func TestRebootStrategyValid(t *testing.T) { + tests := []struct { + value string + + isValid bool + }{ + {value: "best-effort", isValid: true}, + {value: "etcd-lock", isValid: true}, + {value: "reboot", isValid: true}, + {value: "off", isValid: true}, + {value: "besteffort", isValid: false}, + {value: "unknown", isValid: false}, + } + + for _, tt := range tests { + isValid := (nil == AssertStructValid(Update{RebootStrategy: tt.value})) + if tt.isValid != isValid { + t.Errorf("bad assert (%s): want %t, got %t", tt.value, tt.isValid, isValid) + } + } +} diff --git a/system/update_test.go b/system/update_test.go index 822add0..deeb66f 100644 --- a/system/update_test.go +++ b/system/update_test.go @@ -100,7 +100,7 @@ func TestUpdateFile(t *testing.T) { }, { config: config.Update{RebootStrategy: "wizzlewazzle"}, - err: &config.ErrorValid{Value: "wizzlewazzle", Field: "RebootStrategy", Valid: []string{"best-effort", "etcd-lock", "reboot", "off"}}, + err: &config.ErrorValid{Value: "wizzlewazzle", Field: "RebootStrategy", Valid: "^(best-effort|etcd-lock|reboot|off)$"}, }, { config: config.Update{Group: "master", Server: "http://foo.com"}, From 0e70d4f01f7e75841984dbacd509737de242c766 Mon Sep 17 00:00:00 2001 From: Alex Crawford Date: Sun, 21 Dec 2014 11:14:03 -0800 Subject: [PATCH 7/9] config: add validity check for file permissions --- config/file.go | 2 +- config/file_test.go | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/config/file.go b/config/file.go index 04f12c2..76b5827 100644 --- a/config/file.go +++ b/config/file.go @@ -21,5 +21,5 @@ type File struct { Content string `yaml:"content"` Owner string `yaml:"owner"` Path string `yaml:"path"` - RawFilePermissions string `yaml:"permissions"` + RawFilePermissions string `yaml:"permissions" valid:"^0?[0-7]{3,4}$"` } diff --git a/config/file_test.go b/config/file_test.go index a1e03e2..e3f0871 100644 --- a/config/file_test.go +++ b/config/file_test.go @@ -46,3 +46,26 @@ func TestEncodingValid(t *testing.T) { } } } + +func TestRawFilePermissionsValid(t *testing.T) { + tests := []struct { + value string + + isValid bool + }{ + {value: "744", isValid: true}, + {value: "0744", isValid: true}, + {value: "1744", isValid: true}, + {value: "01744", isValid: true}, + {value: "11744", isValid: false}, + {value: "rwxr--r--", isValid: false}, + {value: "800", isValid: false}, + } + + for _, tt := range tests { + isValid := (nil == AssertStructValid(File{RawFilePermissions: tt.value})) + if tt.isValid != isValid { + t.Errorf("bad assert (%s): want %t, got %t", tt.value, tt.isValid, isValid) + } + } +} From 54a64454b979c128b641beb8843a726baa4117ef Mon Sep 17 00:00:00 2001 From: Alex Crawford Date: Sun, 21 Dec 2014 11:24:10 -0800 Subject: [PATCH 8/9] validate: fix printing for non-string values --- config/validate/rules.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/validate/rules.go b/config/validate/rules.go index 7651819..6da2772 100644 --- a/config/validate/rules.go +++ b/config/validate/rules.go @@ -76,7 +76,7 @@ func checkValidity(cfg node, report *Report) { func checkNodeValidity(n, g node, r *Report) { if err := config.AssertValid(n.Value, g.field.Tag.Get("valid")); err != nil { - r.Error(n.line, fmt.Sprintf("invalid value %v", n.Value)) + r.Error(n.line, fmt.Sprintf("invalid value %v", n.Value.Interface())) } switch g.Kind() { case reflect.Struct: From 5527f097787f159a7715fb8767d027b1008e36d2 Mon Sep 17 00:00:00 2001 From: Alex Crawford Date: Sun, 21 Dec 2014 11:16:42 -0800 Subject: [PATCH 9/9] config: fix parsing of file permissions These reintroduces the braindead '744' syntax for file permissions. Even though this number isn't octal, it is assumed by convention to be. In order to pull this off, coerceNodes() was introduced to try to counteract the type inferrencing that occurs during the yaml unmarshalling. The config is unmarshalled twice: once into an empty interface and once into the CloudConfig structure. The two resulting node structures are combined together. The nodes from the CloudConfig process replace those from the interface{} when the types of the two nodes are compatible. For example, with the input `0744`, yaml interprets that as the integer 484 giving us the nodes '0744'(string) and 484(int). Because the types string and int are compatible, we opt to take the string node instead of the integer. --- config/config_test.go | 16 ++++++++ config/validate/validate.go | 66 +++++++++++++++++++++++++++----- config/validate/validate_test.go | 25 ++++++++++++ system/file.go | 2 +- system/file_test.go | 2 +- 5 files changed, 100 insertions(+), 11 deletions(-) diff --git a/config/config_test.go b/config/config_test.go index f4b7bc4..0eaa4de 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -46,6 +46,22 @@ func TestNewCloudConfig(t *testing.T) { contents: "#cloud-config\ncoreos:\n update:\n reboot-strategy: false", config: CloudConfig{CoreOS: CoreOS{Update: Update{RebootStrategy: "false"}}}, }, + { + contents: "#cloud-config\nwrite_files:\n - permissions: 0744", + config: CloudConfig{WriteFiles: []File{File{RawFilePermissions: "0744"}}}, + }, + { + contents: "#cloud-config\nwrite_files:\n - permissions: 744", + config: CloudConfig{WriteFiles: []File{File{RawFilePermissions: "744"}}}, + }, + { + contents: "#cloud-config\nwrite_files:\n - permissions: '0744'", + config: CloudConfig{WriteFiles: []File{File{RawFilePermissions: "0744"}}}, + }, + { + contents: "#cloud-config\nwrite_files:\n - permissions: '744'", + config: CloudConfig{WriteFiles: []File{File{RawFilePermissions: "744"}}}, + }, } for i, tt := range tests { diff --git a/config/validate/validate.go b/config/validate/validate.go index 26ad0b5..02a2baa 100644 --- a/config/validate/validate.go +++ b/config/validate/validate.go @@ -65,7 +65,6 @@ func validateCloudConfig(config []byte, rules []rule) (report Report, err error) return report, err } - c = normalizeNodeNames(c, &report) for _, r := range rules { r(c, &report) } @@ -75,30 +74,79 @@ func validateCloudConfig(config []byte, rules []rule) (report Report, err error) // parseCloudConfig parses the provided config into a node structure and logs // any parsing issues into the provided report. Unrecoverable errors are // returned as an error. -func parseCloudConfig(config []byte, report *Report) (n node, err error) { - var raw map[interface{}]interface{} - if err := yaml.Unmarshal(config, &raw); err != nil { +func parseCloudConfig(cfg []byte, report *Report) (node, error) { + yaml.UnmarshalMappingKeyTransform = func(nameIn string) (nameOut string) { + return nameIn + } + // unmarshal the config into an implicitly-typed form. The yaml library + // will implicitly convert types into their normalized form + // (e.g. 0744 -> 484, off -> false). + var weak map[interface{}]interface{} + if err := yaml.Unmarshal(cfg, &weak); err != nil { matches := yamlLineError.FindStringSubmatch(err.Error()) if len(matches) == 3 { line, err := strconv.Atoi(matches[1]) if err != nil { - return n, err + return node{}, err } msg := matches[2] report.Error(line, msg) - return n, nil + return node{}, nil } matches = yamlError.FindStringSubmatch(err.Error()) if len(matches) == 2 { report.Error(1, matches[1]) - return n, nil + return node{}, nil } - return n, errors.New("couldn't parse yaml error") + return node{}, errors.New("couldn't parse yaml error") + } + w := NewNode(weak, NewContext(cfg)) + w = normalizeNodeNames(w, report) + + // unmarshal the config into the explicitly-typed form. + yaml.UnmarshalMappingKeyTransform = func(nameIn string) (nameOut string) { + return strings.Replace(nameIn, "-", "_", -1) + } + var strong config.CloudConfig + if err := yaml.Unmarshal([]byte(cfg), &strong); err != nil { + return node{}, err + } + s := NewNode(strong, NewContext(cfg)) + + // coerceNodes weak nodes and strong nodes. strong nodes replace weak nodes + // if they are compatible types (this happens when the yaml library + // converts the input). + // (e.g. weak 484 is replaced by strong 0744, weak 4 is not replaced by + // strong false) + return coerceNodes(w, s), nil +} + +// coerceNodes recursively evaluates two nodes, returning a new node containing +// either the weak or strong node's value and its recursively processed +// children. The strong node's value is used if the two nodes are leafs, are +// both valid, and are compatible types (defined by isCompatible()). The weak +// node is returned in all other cases. coerceNodes is used to counteract the +// effects of yaml's automatic type conversion. The weak node is the one +// resulting from unmarshalling into an empty interface{} (the type is +// inferred). The strong node is the one resulting from unmarshalling into a +// struct. If the two nodes are of compatible types, the yaml library correctly +// parsed the value into the strongly typed unmarshalling. In this case, we +// prefer the strong node because its actually the type we are expecting. +func coerceNodes(w, s node) node { + n := w + n.children = nil + if len(w.children) == 0 && len(s.children) == 0 && + w.IsValid() && s.IsValid() && + isCompatible(w.Kind(), s.Kind()) { + n.Value = s.Value } - return NewNode(raw, NewContext(config)), nil + for _, cw := range w.children { + n.children = append(n.children, coerceNodes(cw, s.Child(cw.name))) + } + return n } // normalizeNodeNames replaces all occurences of '-' with '_' within key names diff --git a/config/validate/validate_test.go b/config/validate/validate_test.go index b390eda..328878b 100644 --- a/config/validate/validate_test.go +++ b/config/validate/validate_test.go @@ -65,6 +65,31 @@ func TestValidateCloudConfig(t *testing.T) { rules: []rule{func(_ node, _ *Report) { panic("something happened") }}, err: errors.New("something happened"), }, + { + config: "write_files:\n - permissions: 0744", + rules: Rules, + }, + { + config: "write_files:\n - permissions: '0744'", + rules: Rules, + }, + { + config: "write_files:\n - permissions: 744", + rules: Rules, + }, + { + config: "write_files:\n - permissions: '744'", + rules: Rules, + }, + { + config: "coreos:\n update:\n reboot-strategy: off", + rules: Rules, + }, + { + config: "coreos:\n update:\n reboot-strategy: false", + rules: Rules, + report: Report{entries: []Entry{{entryError, "invalid value false", 3}}}, + }, } for _, tt := range tests { diff --git a/system/file.go b/system/file.go index 312caf4..22118c1 100644 --- a/system/file.go +++ b/system/file.go @@ -43,7 +43,7 @@ func (f *File) Permissions() (os.FileMode, error) { } // Parse string representation of file mode as integer - perm, err := strconv.ParseInt(f.RawFilePermissions, 0, 32) + perm, err := strconv.ParseInt(f.RawFilePermissions, 8, 32) if err != nil { return 0, fmt.Errorf("Unable to parse file permissions %q as integer", f.RawFilePermissions) } diff --git a/system/file_test.go b/system/file_test.go index 8bd441b..eb6f947 100644 --- a/system/file_test.go +++ b/system/file_test.go @@ -97,7 +97,7 @@ func TestDecimalFilePermissions(t *testing.T) { wf := File{config.File{ Path: fn, - RawFilePermissions: "484", // Decimal representation of 0744 + RawFilePermissions: "744", }} path, err := WriteFile(&wf, dir)