golxc: added configuration reading

golxc: added configuration reading Added the reading of configuration files, especially the default configuration at /etc/default/lxc. https://code.launchpad.net/~themue/golxc/002-added-configuration-reading/+merge/135166 (do not edit description out of merge proposal)

[TheMue, 2012-11-20 14:31:40]

Please take a look.

[rog, 2012-11-20 16:20:54]

LGTM, but again, i don't know LXC. https://codereview.appspot.com/6853075/diff/1/config.go File config.go (right): https://codereview.appspot.com/6853075/diff/1/config.go#newcode21 config.go:21: // Allways check line, could also s/Allways/Always/ and it's not just io.EOF - it could happen on any error. https://codereview.appspot.com/6853075/diff/1/config.go#newcode23 config.go:23: line = strings.TrimSpace(line) if line == "" || strings.HasPrefix(line, "#") { continue } ? https://codereview.appspot.com/6853075/diff/1/config.go#newcode27 config.go:27: return nil, fmt.Errorf("illegal line: %q", line) s/illegal/illegal config/ ? https://codereview.appspot.com/6853075/diff/1/config.go#newcode30 config.go:30: value := strings.Trim(strings.TrimSpace(kv[1]), `"`) is this sufficient? can values ever contain double-quotes, for example?

[TheMue, 2012-11-21 13:02:34]

Please take a look. https://codereview.appspot.com/6853075/diff/1/config.go File config.go (right): https://codereview.appspot.com/6853075/diff/1/config.go#newcode21 config.go:21: // Allways check line, could also On 2012/11/20 16:20:54, rog wrote: > s/Allways/Always/ > > and it's not just io.EOF - it could happen on any error. Done the Always change and redesigned error handling. https://codereview.appspot.com/6853075/diff/1/config.go#newcode23 config.go:23: line = strings.TrimSpace(line) On 2012/11/20 16:20:54, rog wrote: > if line == "" || strings.HasPrefix(line, "#") { > continue > } > ? In that case I also have to add a test for io.EOF. Otherwise an empty line in front of the EOF leads to an endless loop. https://codereview.appspot.com/6853075/diff/1/config.go#newcode27 config.go:27: return nil, fmt.Errorf("illegal line: %q", line) On 2012/11/20 16:20:54, rog wrote: > s/illegal/illegal config/ ? Done. https://codereview.appspot.com/6853075/diff/1/config.go#newcode30 config.go:30: value := strings.Trim(strings.TrimSpace(kv[1]), `"`) On 2012/11/20 16:20:54, rog wrote: > is this sufficient? can values ever contain double-quotes, for example? They may use leading and trailing double-quotes to surround them. Sadly it's bad documented. So far as I've seen the value w/o the d'quotes is interesting.

[fwereade, 2012-11-21 17:37:11]

Again, a bit uncertain about the assumptions the tests make about the environment. https://codereview.appspot.com/6853075/diff/3002/golxc_test.go File golxc_test.go (right): https://codereview.appspot.com/6853075/diff/3002/golxc_test.go#newcode47 golxc_test.go:47: c.Assert(env["LXC_BRIDGE"], Equals, "lxcbr0") Not sure about this -- is it possible to swap out the old config file so we don't assume this?

[TheMue, 2012-11-26 10:17:56]

Please take a look.

[TheMue, 2012-11-26 11:04:06]

Please take a look.

[dave_cheney.net, 2012-11-27 09:18:05]

Looking good. I think it is more correct to say parsing, rather than reading. https://codereview.appspot.com/6853075/diff/8001/config.go File config.go (right): https://codereview.appspot.com/6853075/diff/8001/config.go#newcode17 config.go:17: env := make(Configuration) var err error https://codereview.appspot.com/6853075/diff/8001/config.go#newcode18 config.go:18: for { for err != io.EOF https://codereview.appspot.com/6853075/diff/8001/config.go#newcode30 config.go:30: env[key] = parseValue(kv[1]) do you need to trim spaces about kv[1] ? key=value key =value key= value key = value and so forth https://codereview.appspot.com/6853075/diff/8001/config.go#newcode34 config.go:34: } remove https://codereview.appspot.com/6853075/diff/8001/golxc_test.go File golxc_test.go (right): https://codereview.appspot.com/6853075/diff/8001/golxc_test.go#newcode30 golxc_test.go:30: VALUE8 = ""` VALUE9 =foo

[TheMue, 2012-11-27 09:45:24]

Please take a look. https://codereview.appspot.com/6853075/diff/8001/config.go File config.go (right): https://codereview.appspot.com/6853075/diff/8001/config.go#newcode17 config.go:17: env := make(Configuration) On 2012/11/27 09:18:05, dfc wrote: > var err error Done. https://codereview.appspot.com/6853075/diff/8001/config.go#newcode18 config.go:18: for { On 2012/11/27 09:18:05, dfc wrote: > for err != io.EOF Done. https://codereview.appspot.com/6853075/diff/8001/config.go#newcode30 config.go:30: env[key] = parseValue(kv[1]) On 2012/11/27 09:18:05, dfc wrote: > do you need to trim spaces about kv[1] ? > > key=value > key =value > key= value > key = value > > and so forth It's done in parseValue() due to possible leading comments. https://codereview.appspot.com/6853075/diff/8001/config.go#newcode34 config.go:34: } On 2012/11/27 09:18:05, dfc wrote: > remove Done. https://codereview.appspot.com/6853075/diff/8001/golxc_test.go File golxc_test.go (right): https://codereview.appspot.com/6853075/diff/8001/golxc_test.go#newcode30 golxc_test.go:30: VALUE8 = ""` On 2012/11/27 09:18:05, dfc wrote: > VALUE9 =foo Done.

[niemeyer, 2012-11-27 17:55:34]

https://codereview.appspot.com/6853075/diff/12001/config.go File config.go (right): https://codereview.appspot.com/6853075/diff/12001/config.go#newcode12 config.go:12: type Configuration map[string]string Config for the type, as we have elsewhere? https://codereview.appspot.com/6853075/diff/12001/config.go#newcode15 config.go:15: func ParseConfiguration(r io.Reader) (Configuration, error) { ParseConfig https://codereview.appspot.com/6853075/diff/12001/config.go#newcode20 config.go:20: for err != io.EOF { for { ... if err == io.EOF { break } if err != nil { return nil, err } https://codereview.appspot.com/6853075/diff/12001/config.go#newcode31 config.go:31: key := strings.TrimSpace(kv[0]) FOO = BAR is not a valid shell definition. https://codereview.appspot.com/6853075/diff/12001/config.go#newcode51 config.go:51: func parseValue(raw string) string { Shell quoting is significantly more involved than this. We can say we won't support them, or we can actually implement it right in multiple ways. Either way this function is suspect, because it never fails, which means it's necessarily eating errors and treating them as valid values.

[TheMue, 2012-11-28 10:14:50]

Please take a look. https://codereview.appspot.com/6853075/diff/12001/config.go File config.go (right): https://codereview.appspot.com/6853075/diff/12001/config.go#newcode12 config.go:12: type Configuration map[string]string On 2012/11/27 17:55:34, niemeyer wrote: > Config for the type, as we have elsewhere? Changed to Environ. https://codereview.appspot.com/6853075/diff/12001/config.go#newcode15 config.go:15: func ParseConfiguration(r io.Reader) (Configuration, error) { On 2012/11/27 17:55:34, niemeyer wrote: > ParseConfig Found a better way reading the environment, thanks for your hint regarding the shell quoting below. https://codereview.appspot.com/6853075/diff/12001/config.go#newcode20 config.go:20: for err != io.EOF { On 2012/11/27 17:55:34, niemeyer wrote: > for { > ... > if err == io.EOF { > break > } > if err != nil { > return nil, err > } Gone. https://codereview.appspot.com/6853075/diff/12001/config.go#newcode31 config.go:31: key := strings.TrimSpace(kv[0]) On 2012/11/27 17:55:34, niemeyer wrote: > FOO = BAR is not a valid shell definition. Gone, see above. https://codereview.appspot.com/6853075/diff/12001/config.go#newcode51 config.go:51: func parseValue(raw string) string { On 2012/11/27 17:55:34, niemeyer wrote: > Shell quoting is significantly more involved than this. We can say we won't > support them, or we can actually implement it right in multiple ways. Either way > this function is suspect, because it never fails, which means it's necessarily > eating errors and treating them as valid values. Yes, you're absolutely right. Changed it to sourcing the file and parsing the environment.

[TheMue, 2012-11-28 12:38:32]

Please take a look.

[TheMue, 2012-11-28 12:48:07]

Please take a look.

[rog, 2012-11-28 12:57:44]

https://codereview.appspot.com/6853075/diff/13001/environ.go File environ.go (right): https://codereview.appspot.com/6853075/diff/13001/environ.go#newcode12 environ.go:12: script := `""set -a; . ` + lxcDefaultFile + `; env""` what are the double quotes doing here? https://codereview.appspot.com/6853075/diff/4004/environ.go File environ.go (right): https://codereview.appspot.com/6853075/diff/4004/environ.go#newcode9 environ.go:9: type Environ map[string]string is there a good reason to have this as a type? os seems to do fine with just map[string]string. https://codereview.appspot.com/6853075/diff/4004/environ.go#newcode13 environ.go:13: // ReadDefaultEnviron returns the environment including // ReadDefaultEnviron runs /etc/default/lxc and // returns the environment variables that it sets. ? https://codereview.appspot.com/6853075/diff/4004/environ.go#newcode22 environ.go:22: for _, line := range strings.Split(out, string([]byte{0})) { s/string([]byte{0})/"\x00"/ or string(0) (they both produce code with no allocations, unlike the current version) https://codereview.appspot.com/6853075/diff/4004/golxc_test.go File golxc_test.go (right): https://codereview.appspot.com/6853075/diff/4004/golxc_test.go#newcode66 golxc_test.go:66: c.Assert(env["MIRROR"], Equals, "") i think this test should assert the entire contents of the environment.

[TheMue, 2012-11-28 15:38:45]

Please take a look. https://codereview.appspot.com/6853075/diff/13001/environ.go File environ.go (right): https://codereview.appspot.com/6853075/diff/13001/environ.go#newcode12 environ.go:12: script := `""set -a; . ` + lxcDefaultFile + `; env""` On 2012/11/28 12:57:45, rog wrote: > what are the double quotes doing here? Done. https://codereview.appspot.com/6853075/diff/4004/environ.go File environ.go (right): https://codereview.appspot.com/6853075/diff/4004/environ.go#newcode9 environ.go:9: type Environ map[string]string On 2012/11/28 12:57:45, rog wrote: > is there a good reason to have this as a type? > os seems to do fine with just map[string]string. Done. https://codereview.appspot.com/6853075/diff/4004/environ.go#newcode13 environ.go:13: // ReadDefaultEnviron returns the environment including On 2012/11/28 12:57:45, rog wrote: > // ReadDefaultEnviron runs /etc/default/lxc and > // returns the environment variables that it sets. > > ? Done. https://codereview.appspot.com/6853075/diff/4004/environ.go#newcode22 environ.go:22: for _, line := range strings.Split(out, string([]byte{0})) { On 2012/11/28 12:57:45, rog wrote: > s/string([]byte{0})/"\x00"/ > > or string(0) > > (they both produce code with no allocations, unlike the current version) Done. https://codereview.appspot.com/6853075/diff/4004/golxc_test.go File golxc_test.go (right): https://codereview.appspot.com/6853075/diff/4004/golxc_test.go#newcode66 golxc_test.go:66: c.Assert(env["MIRROR"], Equals, "") On 2012/11/28 12:57:45, rog wrote: > i think this test should assert the entire contents of the environment. Done.

[fwereade, 2013-01-30 16:37:39]

A few comments: https://codereview.appspot.com/6853075/diff/8003/environ.go File environ.go (right): https://codereview.appspot.com/6853075/diff/8003/environ.go#newcode8 environ.go:8: var lxcDefaultFile = "/etc/default/lxc" I'm a little opposed to the term "default" when it's not actually changeable except by the tests. "confPath"? https://codereview.appspot.com/6853075/diff/8003/environ.go#newcode11 environ.go:11: // returns the environment variables that it sets. Is this how LXC uses it? If so, I guess I can put up with it, it is a vulnerability in LXC and in our LXC client. If LXC reads that file in a less execute-arbitrary-script-as-root-ey way, though, I don't think it's sensible to do it like this. Crack? https://codereview.appspot.com/6853075/diff/8003/environ.go#newcode12 environ.go:12: func ReadDefaultEnviron() (map[string]string, error) { ReadConf? https://codereview.appspot.com/6853075/diff/8003/golxc_test.go File golxc_test.go (right): https://codereview.appspot.com/6853075/diff/8003/golxc_test.go#newcode46 golxc_test.go:46: f, err := ioutil.TempFile("/tmp", "lxc-test") c.MkDir() https://codereview.appspot.com/6853075/diff/8003/golxc_test.go#newcode60 golxc_test.go:60: c.Assert(len(env), Equals, 9) A `DeepEquals, map[string]string{...}` here might make this a bit clearer. https://codereview.appspot.com/6853075/diff/8003/golxc_test.go#newcode75 golxc_test.go:75: orig := golxc.SetLXCDefaultFile("/tmp/not-existing-exc-test") filepath.Join(c.MkDir(), "does-not-exist")

[TheMue, 2013-01-31 15:54:11]

Please take a look. https://codereview.appspot.com/6853075/diff/8003/environ.go File environ.go (right): https://codereview.appspot.com/6853075/diff/8003/environ.go#newcode8 environ.go:8: var lxcDefaultFile = "/etc/default/lxc" On 2013/01/30 16:37:39, fwereade wrote: > I'm a little opposed to the term "default" when it's not actually changeable > except by the tests. "confPath"? Done. Named it confFile as it is a file, no path. https://codereview.appspot.com/6853075/diff/8003/environ.go#newcode11 environ.go:11: // returns the environment variables that it sets. On 2013/01/30 16:37:39, fwereade wrote: > Is this how LXC uses it? If so, I guess I can put up with it, it is a > vulnerability in LXC and in our LXC client. If LXC reads that file in a less > execute-arbitrary-script-as-root-ey way, though, I don't think it's sensible to > do it like this. Crack? Done. Original boot scripts are doing it that way. As we are only interested in two values I've changed it into a simple parsing. https://codereview.appspot.com/6853075/diff/8003/environ.go#newcode12 environ.go:12: func ReadDefaultEnviron() (map[string]string, error) { On 2013/01/30 16:37:39, fwereade wrote: > ReadConf? Done. https://codereview.appspot.com/6853075/diff/8003/golxc_test.go File golxc_test.go (right): https://codereview.appspot.com/6853075/diff/8003/golxc_test.go#newcode46 golxc_test.go:46: f, err := ioutil.TempFile("/tmp", "lxc-test") On 2013/01/30 16:37:39, fwereade wrote: > c.MkDir() Done. https://codereview.appspot.com/6853075/diff/8003/golxc_test.go#newcode60 golxc_test.go:60: c.Assert(len(env), Equals, 9) On 2013/01/30 16:37:39, fwereade wrote: > A `DeepEquals, map[string]string{...}` here might make this a bit clearer. Done. https://codereview.appspot.com/6853075/diff/8003/golxc_test.go#newcode75 golxc_test.go:75: orig := golxc.SetLXCDefaultFile("/tmp/not-existing-exc-test") On 2013/01/30 16:37:39, fwereade wrote: > filepath.Join(c.MkDir(), "does-not-exist") File isn't created, can be any non-existing name.

[fwereade, 2013-02-04 14:58:14]

Couple more remarks: https://codereview.appspot.com/6853075/diff/8003/environ.go File environ.go (right): https://codereview.appspot.com/6853075/diff/8003/environ.go#newcode8 environ.go:8: var lxcDefaultFile = "/etc/default/lxc" On 2013/01/31 15:54:11, TheMue wrote: > On 2013/01/30 16:37:39, fwereade wrote: > > I'm a little opposed to the term "default" when it's not actually changeable > > except by the tests. "confPath"? > > Done. Named it confFile as it is a file, no path. It's not a file... it is a path. https://codereview.appspot.com/6853075/diff/18001/golxc_test.go File golxc_test.go (right): https://codereview.appspot.com/6853075/diff/18001/golxc_test.go#newcode36 golxc_test.go:36: LXC_BRIDGE="lxcbr9" Add a commented-out LXC_BRIDGE line somewhere before and somewhere after to check that only the valid one is matched and taken. https://codereview.appspot.com/6853075/diff/18001/golxc_test.go#newcode37 golxc_test.go:37: LXC_ADDR="10.0.9.1" Ditto here, check for values that are close but don't actually match.

[TheMue, 2013-02-05 12:47:52]

Please take a look. https://codereview.appspot.com/6853075/diff/18001/golxc_test.go File golxc_test.go (right): https://codereview.appspot.com/6853075/diff/18001/golxc_test.go#newcode36 golxc_test.go:36: LXC_BRIDGE="lxcbr9" On 2013/02/04 14:58:14, fwereade wrote: > Add a commented-out LXC_BRIDGE line somewhere before and somewhere after to > check that only the valid one is matched and taken. Done. https://codereview.appspot.com/6853075/diff/18001/golxc_test.go#newcode37 golxc_test.go:37: LXC_ADDR="10.0.9.1" On 2013/02/04 14:58:14, fwereade wrote: > Ditto here, check for values that are close but don't actually match. Done.

[fwereade, 2013-02-07 09:43:11]

LGTM assuming complaints below are addressed. Var names and funky defers are very much suggestions, feel free to ignore those. https://codereview.appspot.com/6853075/diff/8003/golxc_test.go File golxc_test.go (right): https://codereview.appspot.com/6853075/diff/8003/golxc_test.go#newcode75 golxc_test.go:75: orig := golxc.SetLXCDefaultFile("/tmp/not-existing-exc-test") On 2013/01/31 15:54:11, TheMue wrote: > On 2013/01/30 16:37:39, fwereade wrote: > > filepath.Join(c.MkDir(), "does-not-exist") > > File isn't created, can be any non-existing name. Please make sure we're consistent with the other branch in use of c.MkDir(). https://codereview.appspot.com/6853075/diff/25001/config.go File config.go (right): https://codereview.appspot.com/6853075/diff/25001/config.go#newcode11 config.go:11: bridgeRE string = `\n\s*LXC_BRIDGE="(\w+)"` Maybe MustCompile them up here, rather than waiting until ReadConf time to panic? https://codereview.appspot.com/6853075/diff/25001/config.go#newcode22 config.go:22: addrs := regexp.MustCompile(addrRE).FindStringSubmatch(string(confData)) nitpick var name -- I'd call this groups and stick the whole lot in its own scope, but YMMV. MustCompile should be extracted, though. https://codereview.appspot.com/6853075/diff/25001/config.go#newcode26 config.go:26: bridges := regexp.MustCompile(bridgeRE).FindStringSubmatch(string(confData)) ditto. https://codereview.appspot.com/6853075/diff/25001/golxc_test.go File golxc_test.go (right): https://codereview.appspot.com/6853075/diff/25001/golxc_test.go#newcode61 golxc_test.go:61: defer golxc.SetConfPath(orig) Hey, you could do: defer golxc.SetConfPath(golxc.SetConfPath(cf)) ...but I accept that may not be to everybody's taste.

[TheMue, 2013-02-07 11:39:18]

*** Submitted: golxc: added configuration reading Added the reading of configuration files, especially the default configuration at /etc/default/lxc. R=rog, fwereade, dfc, niemeyer CC= https://codereview.appspot.com/6853075 https://codereview.appspot.com/6853075/diff/25001/config.go File config.go (right): https://codereview.appspot.com/6853075/diff/25001/config.go#newcode11 config.go:11: bridgeRE string = `\n\s*LXC_BRIDGE="(\w+)"` On 2013/02/07 09:43:11, fwereade wrote: > Maybe MustCompile them up here, rather than waiting until ReadConf time to > panic? Done. https://codereview.appspot.com/6853075/diff/25001/config.go#newcode22 config.go:22: addrs := regexp.MustCompile(addrRE).FindStringSubmatch(string(confData)) On 2013/02/07 09:43:11, fwereade wrote: > nitpick var name -- I'd call this groups and stick the whole lot in its own > scope, but YMMV. MustCompile should be extracted, though. Done. https://codereview.appspot.com/6853075/diff/25001/config.go#newcode26 config.go:26: bridges := regexp.MustCompile(bridgeRE).FindStringSubmatch(string(confData)) On 2013/02/07 09:43:11, fwereade wrote: > ditto. Done. https://codereview.appspot.com/6853075/diff/25001/golxc_test.go File golxc_test.go (right): https://codereview.appspot.com/6853075/diff/25001/golxc_test.go#newcode61 golxc_test.go:61: defer golxc.SetConfPath(orig) On 2013/02/07 09:43:11, fwereade wrote: > Hey, you could do: > > defer golxc.SetConfPath(golxc.SetConfPath(cf)) > > ...but I accept that may not be to everybody's taste. Done.