Fixes #979: Adds code to check the CLI and config file port definitions

[tjmcs]

Fixes #979

Summary of changes:

• Adds a new (unexported) flag to the RESTful server configuration so that we can determine if there was a port defined for the restapi::port parameter in the global configuration file and a 'getter' function to retrieve the value of that flag.
• refactors the SetAddress method from the Server type in the mgmt/server package so that it just takes an address (which can include a port as part of the address string); also refactored the associated tests so that they use the new form of this method.
• Adds functionality to the snapd.go file to carefully check all of the possible sources (the SNAP_ADDR and SNAP_PORT environment variables, the --api-addr and --api-port command-line parameters, and the restapi::addr and restapi::port parameters in the global configuration file) to make sure that the port is only defined once (either using just an address parameter or just a port parameter), but not both (you can define the port more than once, provided it the definitions are all done using the same 'type' of parameter; eg. using the SNAP_ADDR environment variable and the restapi::addr parameter in the global configuration file). Defining the port using more than one type of parameter will result in an error (eg. using the SNAP_ADDR environment variable and the restapi::port parameter in the global configuration file).
• Adds tests that check the address passed in through the SNAP_ADDR environment variable, the --api-addr command-line parameter, or the restapi::addr global configuration parameter to make sure it is a parseable IP address or a resolvable hostname.
• Adds tests to ensure that the port number passed in (through whatever source it is passed in) is an integer value that complies with the constraints defined for this parameter (regardless of the source it was obtained from)

Testing done:

• tested using go run ... commands on the CLI and passing in all of the various combinations and permutations of inputs to ensure that all of the error conditions that should be caught were caught and that the port was set correctly, regardless of the source that was used to pass this port into the Snap daemon.

@intelsdi-x/snap-maintainers

[IRCody]

I'm not sure I agree with making everything a stringflag.

The problem it's trying to solve (I think) is that ctx.isSet will not return true when environment variables are set? Would it be easier/cleaner to just check if the env variable is set ourselves where we check ctx.IsSet?

[tjmcs]

The problem is two-fold:

• if we include a default value for the integer values that is sensible (i.e. set to the same default value defined for the corresponding property in the configuration file) then that default value overrides any value set in the configuration file if the corresponding command-line option or environment variable is not used to set a value for that property
• if we don't include a default value for these properties (specifically the integer and duration properties) then the codegansta library outputs "(default: 0)" for the usage of all of those properties when a "snapd --help" command is run

To make things worse the codegansta library doesn't give us a way to get the name of the environment variable associated with a field nor does it give us a way to tell if the value was set using the default value or the environment variable associated with a particular field. By defining these fields as string fields (and parsing the resulting field values into variables of the right type) we can skip defining a default value for these fields and not see improper values as the "default value" in the output of a "snapd --help" command. Does that clarify things sufficiently, @IRCody?

[IRCody]

@tjmcs: I think I understand the motivation behind your change. There are some limitations to the cli library we are currently using (probably held back more by the fact that we are pinned to an older version iirc). It feels like we are working around the cli library instead of with it. I wonder if there are other libraries that would suit our needs better?

[tjmcs]

Yes, you are absolutely correct, @IRCody; the change to string flags is a change chosen to work around an obvious issue with the cli library we are using.

The reason I made the changes I made in this PR (swapping these fields over so that they are parsed as string flags and adding error handling to ensure they can still be used as integers or durations) is that I didn't feel that this change was nearly as drastic as a change to a different cli library would be (yes, there are others out there) and it resolves this issue without introducing the secondary issue I encountered and reported as issue #1016 (which is now closed, but which was triggered by the fact that there were default values set for these properties and that we couldn't tell the difference between a value that was set via an environment variable and a value that was set using the defined default value in this cli library; default values that were added in an earlier PR simply to avoid having bogus default values added to usage text that is output by that cli library in response to a snapd --help or go run snapd.go --help command).

This change to string flags seemed to be the smallest change that I could make and still resolve this issue without causing any side-effects that were unwanted. If we are going to switch over to another cli library, I think that should be handled in a separate PR that addresses the question of which cli library we should be using in Snap as a completely separate issue. Yes, the change to string flags for these fields is a work-around for an the fact that we can't get to the detail we need to resolve the associated issue (Issue #1016) through the codegangsta library itself without removing the default values that were added in an earlier PR (which would return us to the situation where the codegangsta library is adding bogus default values to the --help output for these integer and duration values), but I don't see the harm in swapping over to parsing these as string flags, especially since I've added code in this PR to catch situations where the values passed using these flags cannot be parsed as the appropriate integer/duration type and log a fatal error that describes the problem in those situations.

So, in short, I don't think we should switch to a different library at this time and I think the changes made in this PR do resolve the issue associated with this PR, all while making as few changes to the existing codebase as possible.

[pittma]

I certainly agree that we don't switch CLI libraries. I think the other option here is to use lookups for envs, and retrieve them via os.Getenv rather than from the CLI library, because, as you said, Tom, you can't get the CLI envs from the library itself.

[tjmcs]

I looked into that option, @danielscottt; the issue there is that we don't even have an easy way to get the name of the environment variable that goes with a given command-line flag from the library. If we had that functionality, I would have done exactly what you're suggesting here (using an os.GetEnv call to retrieve the environment variable and see if it has been set), but without that functionality I'm stuck. The only other alternative would be to put together a lookup table of some sort where I could match the key for the field with the environment variable that was used to set the value for that field, but now I'm duplicating the mapping between this key and the corresponding environment variable (tracking it in the flags managed by the codegangsta/cli library and in the lookup table that I'm constructing). Overall, as I said earlier, I think the changes in this PR are much simpler than the alternatives...

[pittma]

ping @IRCody.

After a discussion with @tjmcs, I think the two options we have, const envs and parsing our own typed values, result in the same end state. Whether one or the other is cleaner is more a matter of opinion, and for the case of what is already implemented here, well, it's already implemented here 😄. It involves less changes and resolved the issue. To take this further (a refactor) is something we may choose to do later on.

I'm going to go ahead and land this patch for now.

[pittma]

LGTM