Repository navigation
Allow flag or file based configuration - #620
Conversation
|
Could you please rebase this on top of |
codingllama
left a comment
codingllama
left a comment
There was a problem hiding this comment.
Looks good to me, I have mostly nits / suggestions.
Would you mind applying similar changes to server/vmap/trillian_map_server/main.go too? It's fairly small and the only missing entry point.
| etcdServers = flag.String("etcd_servers", "", "A comma-separated list of etcd servers") | ||
| ) | ||
| type config struct { | ||
| Host string |
There was a problem hiding this comment.
nit: As the struct is private, the fields should possibly be private too. (It seems the large majority of the existing code follows this convention, apart from 1 outlier that I found.)
There was a problem hiding this comment.
The Golang JSON package requires that fields be public if you want to unmarshal into them.
There was a problem hiding this comment.
Right, I missed that. Would you mind adding a comment? I.e.:
// config represents all configurable settings of {CT,Log,Map} server.
// Fields may be set either via flag or a configuration file in JSON format.
// Fields must be public due to a requirement of json.Unmarshal.Not everyone is going to use config files, so a "stylistic" change might break you and leave us none the wiser.
Ditto for others.
| if *configFlag != "" { | ||
| // Only allow either --configFlag OR any other flags | ||
| if flag.CommandLine.NFlag() > 2 { | ||
| glog.Exit("Cannot use --configFlag with any other flags") |
There was a problem hiding this comment.
nit: --config instead of --configFlag
| etcdService = flag.String("etcd_service", "trillian-log", "Service name to announce ourselves under") | ||
| ) | ||
| type config struct { | ||
| MySQLURI string |
There was a problem hiding this comment.
Ditto for private struct / public fields.
Same for other files.
| if *configFlag != "" { | ||
| // Only allow either --configFlag OR any other flags | ||
| if flag.CommandLine.NFlag() > 2 { | ||
| glog.Exit("Cannot use --configFlag with any other flags") |
There was a problem hiding this comment.
Ditto for --config.
Same for other files.
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package util |
There was a problem hiding this comment.
WDYT about moving this to a configs package? I kind of dislike util on principle.
| } | ||
|
|
||
| // UnmarshalJSON parses a time.Duration field from a string | ||
| func (cd ConfigDuration) UnmarshalJSON(data []byte) error { |
There was a problem hiding this comment.
Would you mind including a test for this?
|
|
||
| if *maxGetEntriesFlag > 0 { | ||
| ct.MaxGetEntriesAllowed = *maxGetEntriesFlag | ||
| if *configFlag != "" { |
There was a problem hiding this comment.
Have you considered moving this block to a shared function, possibly under the configs package I suggested below?
|
|
||
| // ConfigDuration is an abstraction used to simplifying unmarshalling | ||
| // json into structs that contain time.Duration fields | ||
| type ConfigDuration struct { |
There was a problem hiding this comment.
Maybe rename to JsonDuration?
Please document the expected marshaled format as well (a mention to time.ParseDuration would be good enough, IMO).
04b4c90 to
8b48ccf
Compare
|
I've run into a bit of a problem while testing these changes, Not entirely sure what the best way around this is at the moment, going to drink some coffee and see if I can come up with a good solution. |
|
(this idea doesn't work)
|
|
I know I'm a bit late to this conversation, but while we're discussing flags and configurations, have we thought about configuring through environment variables? Many cluster management products use a |
|
Blerp, current solution doesn't work: I forgot that One super simple option I can think of that would make life significantly easier (and allow all configuration to happen via flags or files) would be to fork
@gdbelvin This would be a viable solution but I personally don't super like it as a method of configuration, you get the benefits of having everything in a file but have to worry about when the last time you |
|
The case of flags defined outside the Trillian codebase is an interesting one, because it seems to force us to a mixed configuration. @rolandshoemaker, what's your deployment plan in regards to those flags? Should they be defined in your configuration object as well? About implementation options, I don't think this is a strong enough reason to fork glog. I would suggest something in those lines instead: allowedFlags := map[string]bool{
"logtostderr": true,
"alsologtostderr": true,
"stderrthreshold": true,
"log_dir": true,
"log_backtrace_at": true,
"v": true,
"vmodule": true,
}
for _, arg := range os.Args[1:] {
var flag string
switch {
case strings.HasPrefix(arg, "--"):
flag = arg[2:]
case strings.HasPrefix(arg, "-"):
flag = arg[1:]
default:
continue // Ignore non-flags
}
if index := strings.Index(flag, "="); index != -1 {
flag = flag[0:index]
}
if _, ok := allowedFlags[flag]; !ok {
return fmt.Errorf("flag not allowed in conjunction with --config: %v", arg)
}
}(Warning: I haven't tested / executed this.) Another option, if we'd like other flags to be configurable via config files, is to add them to the configuration and call flag.Set for each flag. The caveat is that logging won't work as expected up until that point. I'm OK with dropping the --config "exclusiveness" requirement if people think those options are not worth it, though. |
codingllama
left a comment
codingllama
left a comment
There was a problem hiding this comment.
Please have a look at the Travis failure as well.
@RJPercival had an interesting suggestion: we could use flag.CommandLine.Parse directly on the config file, which would preclude all the json-parsing logic and config struct intermediates. That would change the format from json to a "flags file", though (one flag per line, possibly).
At that point, IMO, we might just have a .sh file wrapping the entry points and set the flags there, which would avoid the entire config file business. IDK much about your deployment environment, so I'll leave that to you.
| Dur JSONDuration | ||
| } | ||
|
|
||
| a := []byte(`{"dur":"10s"}`) |
There was a problem hiding this comment.
Would mind refactoring the test into a table-driven style?
| // UnmarshalJSON parses a time.Duration field from a string | ||
| func (jd *JSONDuration) UnmarshalJSON(data []byte) error { | ||
| s := "" | ||
| err := json.Unmarshal(data, &s) |
There was a problem hiding this comment.
nit: Scope err in the if? if err := json.Unmarshal(data, &s); err != nil { ... }
| ct.MaxGetEntriesAllowed = *maxGetEntriesFlag | ||
| if *configFlag != "" { | ||
| // Only allow either --config OR any other flags | ||
| if flags.NFlag() > 1 { |
There was a problem hiding this comment.
Could we move the --config "exclusiveness" check to the shared function as well? (Whatever form it might take, it is.)
| etcdServers = flag.String("etcd_servers", "", "A comma-separated list of etcd servers") | ||
| ) | ||
| type config struct { | ||
| Host string |
There was a problem hiding this comment.
Right, I missed that. Would you mind adding a comment? I.e.:
// config represents all configurable settings of {CT,Log,Map} server.
// Fields may be set either via flag or a configuration file in JSON format.
// Fields must be public due to a requirement of json.Unmarshal.Not everyone is going to use config files, so a "stylistic" change might break you and leave us none the wiser.
Ditto for others.
I wouldn't be explicitly opposed to this, generating a file is generating a file, the only real difference in terms of management is that it's a little simpler to verify that the file has been properly formatted when it's JSON since we can use basically any parser on the config management node to verify the file. I think this approach does have a few downsides though, mainly it would make the config take precedence over flags when both are provided (which I think is basically the opposite of expected behavior) and it would require either a bunch of extra parsing code (to properly split up the flags/flag arguments in the way that |
codingllama
left a comment
codingllama
left a comment
There was a problem hiding this comment.
Many thanks for addressing all comments, Roland. Could you rebase on master again after you address the next round?
| // be set from a file. | ||
| func SetGLogFlags(gf GLogFlags) error { | ||
| if gf.LogToStderr { | ||
| if err := flag.Set("logtostderr", fmt.Sprintf("%t", gf.LogToStderr)); err != nil { |
There was a problem hiding this comment.
Suggestion: Just fmt.Sprint(gf.LogToStderr) should work.
Ditto for alsologtostderr and v.
| wantErr string | ||
| wantDuration time.Duration | ||
| }{ | ||
| {[]byte(`{"dur":"10s"}`), "", time.Second * 10}, |
There was a problem hiding this comment.
nit: Please use named literals on the struct fields. This way you don't have to specify default/unrelated values as well.
| var s struct { | ||
| Dur JSONDuration | ||
| testCases := []struct { | ||
| json []byte |
There was a problem hiding this comment.
Suggestion: You could make this a string and move the cast to the test body.
| "time" | ||
| ) | ||
|
|
||
| func TestJSONDuration(t *testing.T) { |
There was a problem hiding this comment.
Maybe TestJSONDuration_ UnmarshalJSON, so it's clear which method is being tested?
| Dur JSONDuration | ||
| testCases := []struct { | ||
| json []byte | ||
| wantErr string |
There was a problem hiding this comment.
This is tying the test to error messages we don't own. I'd be ok with an wantErr bool and a err != nil check. We have a bunch of tests like that, eg, noop_test.go.
| var s struct { | ||
| Dur JSONDuration | ||
| } | ||
| err := json.Unmarshal(tc.json, &s) |
There was a problem hiding this comment.
Suggestion:
err := json.Unmarshal(tc.json, &s)
switch hasErr, wantErr := err != nil, tc.wantErr != ""; {
case hasErr != wantErr:
t.Errorf(...)
case hasErr && err.Error() != tc.wantErr:
t.Errorf(...)
}Although, if you take the comment above about the error string, the case comparing the messages goes away, so we could simplify it further to an if.
| } else if err == nil && tc.wantErr != "" { | ||
| t.Errorf("json.Unmarshal didn't fail: want %q", tc.wantErr) | ||
| } | ||
| if tc.wantDuration != 0 && tc.wantDuration != s.Dur.Duration { |
There was a problem hiding this comment.
I think you can drop the tc.wantDuration != 0 condition and it'll still work, as zero should be the default on s.Dur.Duration.
| "google.golang.org/grpc/naming" | ||
| ) | ||
|
|
||
| // config represents all configurable settings of {CT,Log,Map} server and Log signer. |
There was a problem hiding this comment.
nit: Replace "{CT,Log,Map} server and Log signer" with just "CT server". This particular config is exclusive to CT server (and so are the others to their respective binaries).
Ditto for others.
| if err := configs.LoadConfig(*configFlag, &c); err != nil { | ||
| glog.Exit(err) | ||
| } | ||
| if err := configs.SetGLogFlags(c.GLogFlags); err != nil { |
There was a problem hiding this comment.
Suggestion: Could we move this to LoadConfig?
If we make changes similar to the ones below I reckon it should work:
// Change SetGLogFlags signature.
func (gf *GLogFlags) SetGLogFlags() error { ... }
type glogFlagsSetter interface {
SetGLogFlags() error
}
func LoadConfig(path string, obj interface{}) error {
// ...
if setter, ok := obj.(glogFlagsSetter); ok {
if err := setter.SetGlogFlags(); err != nil {
return err
}
}
}|
|
||
| // GLogFlags contains the values of the flags that github.com/golang/glog | ||
| // registers. | ||
| type GLogFlags struct { |
There was a problem hiding this comment.
Suggestion: I'd be tempted to remove call this "CommonFlags" (or something in that spirit), so if we add non-glog flags to it later we won't have to rename it. (Ditto for SetGlogFlags).
|
To illustrate my proposal of reading flags from a file, see RJPercival@c6d3fc8. This supports things like glog without any special code being required. Parsing is off-loaded to a shellwords package (admittedly, this means an extra dependency). It supports either letting command-line arguments take precedence or raising an error if any command-line arguments other than "--config" are supplied. Having said all that, it doesn't give you much over just having a wrapper script around the binary with whatever flags you'd like. Can you explain to me what this PR gives us? You mentioned it allows you to "use basically any parser on the config management node to verify the [config]". What sort of verification were you planning to do? |
Yup, this is a MUCH cleaner implementation. Looking at the code I think this is probably the best way forward if we do want to support some kind of file based configuration.
By verification I really just meant a super basic sanity check, for instance in our existing deployment of other software we are able to simply parse the JSON config after it has been generated on the config management node from a template in order to verify that it is at least properly formatted.
After having deployed trillian in its current form a bunch recently I've pretty much come around to this being a basically fine solution. I think the only thing I'd really like file based configuration for at this point is basic secret isolation so that we don't have PEM key passwords or MySQL credentials in the process list but ¯_(ツ)_/¯. Conclusion: I think I'm convinced at this point that this JSON-based solution ends up being very heavyweight for relatively little benefit (we could have native lists or maps! but... nowhere uses that now so eh). I still think having the separation of config from service is nice to have and provides for basic secret isolation but is not necessary. At this point I think there are two real choices: (a.) close this and #578 out and go on our merry way and keep everything as-is or (b.) implement the solution from RJPercival/trillian@c6d3fc8. I think I'd probably go with (b.) but could really go either way to be honest. |
|
I can appreciate not wanting certain flags visible in the process list; that's a good reason to have a config file. Would you like to pick up RJPercival@c6d3fc8, finish it off and see how it works for your use case? |
Sounds good to me. |
7dd8a47 to
ddcc786
Compare
6c2ff65 to
cc9922c
Compare
| "flag" | ||
| "io/ioutil" | ||
|
|
||
| "github.com/mattn/go-shellwords" |
There was a problem hiding this comment.
Having looked at what is available within Google, could we swap this for https://bitbucket.org/creachadair/shell instead please? We'll lose the ability to use environment variables in the flag file, which is a bit of a shame, but it saves having to get https://github.com/mattn/go-shellwords reviewed by our security team. I can go down that road if supporting environment variables is important to you though.
There was a problem hiding this comment.
Works for me.
| if *configFile != "" { | ||
| if err := cmd.ParseFlagFile(*configFile); err != nil { | ||
| fmt.Fprintf(os.Stderr, "Failed to parse %v: %v\n", *configFile, err) | ||
| os.Exit(1) |
There was a problem hiding this comment.
glog.Exitf() rather than fmt.Fprintf(), os.Exit(1) would be more consistent with the rest of the file.
| if *configFile != "" { | ||
| if err := cmd.ParseFlagFile(*configFile); err != nil { | ||
| fmt.Fprintf(os.Stderr, "Failed to parse %v: %v\n", *configFile, err) | ||
| os.Exit(1) |
There was a problem hiding this comment.
glog.Exitf() would be better here.
| if *configFile != "" { | ||
| if err := cmd.ParseFlagFile(*configFile); err != nil { | ||
| fmt.Fprintf(os.Stderr, "Failed to parse %v: %v\n", *configFile, err) | ||
| os.Exit(1) |
There was a problem hiding this comment.
glog.Exitf() would be better here.
| // path. Re-calls flag.Parse() after parsing the flags in the file | ||
| // so that flags provided on the command line take precedence over | ||
| // flags provided in the file. | ||
| func ParseFlagFile(path string) error { |
There was a problem hiding this comment.
This func could do with some tests.
| } | ||
|
|
||
| // Expand any environment variables in the file | ||
| flagsString := os.ExpandEnv(string(file)) |
There was a problem hiding this comment.
Expanding envvars before splitting might lead to unintended splitting if the envvar value contains spaces, quotation marks, etc. Unfortunately, you might have to instead call os.ExpandEnv() on each of args. A test case could confirm this.
| } | ||
|
|
||
| err = flag.CommandLine.Parse(args) | ||
| if err != nil { |
There was a problem hiding this comment.
These two lines could be combined, e.g. if err := flag.CommandLine.Parse(args); err != nil {
RJPercival
left a comment
RJPercival
left a comment
There was a problem hiding this comment.
Once these comments have been addressed, it should be good to merge.
| }, | ||
| } | ||
|
|
||
| initalArgs := os.Args[:] |
There was a problem hiding this comment.
typo: s/initalArgs/initialArgs/
| } | ||
| } | ||
| err := parseFlags(tc.contents) | ||
| if err != nil { |
There was a problem hiding this comment.
Could combine the above two lines.
| for _, tc := range tests { | ||
| a, b = "", "" | ||
| os.Args = initalArgs[:] | ||
| if len(tc.cliArgs) > 0 { |
There was a problem hiding this comment.
No need for this check - os.Args = append(os.Args, tc.cliArgs...) will simply be a noop if len(tc.cliArgs) == 0.
| initalArgs := os.Args[:] | ||
| for _, tc := range tests { | ||
| a, b = "", "" | ||
| os.Args = initalArgs[:] |
There was a problem hiding this comment.
Lines 80-83 could be condensed into os.Args = append(initialArgs, tc.cliArgs...).
|
|
||
| flag.CommandLine.Init(os.Args[0], flag.ContinueOnError) | ||
|
|
||
| tests := []struct { |
There was a problem hiding this comment.
Please add a name field and include it in all of the t.Errorf() calls so that it's easy to identify which test case failed.
RJPercival
left a comment
RJPercival
left a comment
There was a problem hiding this comment.
To save time, and because my comments were minor, I've taken the liberty of addressing them myself and pushing a commit to your branch. Feel free to amend that commit as you like, then you can squash and merge this PR.
Superseded by a more recent review.
|
Changes look fine to me, thanks! Side-note: looks like |
|
(Oh also, I don't have perms on this repo so someone else should merge/kick the travis build) |
|
I've given it a kick. Yes I've noticed client_test.go being pretty flaky, likely because of the sleep call it's relying on. @gdbelvin, any chance you could take a look at that? |
|
The second build hit issue #617 (I'll look into that). Third time is hopefully the charm... |
6 participants
Initial implementation, slightly less hacky than I originally anticipated.
Fixes #578.