Invalid config could cause runtime error

[raliev12]

Describe the bug
Invalid config could cause runtime error. Since some variables in the code are named as is config, there is a possibility that values will be assigned to nil pointers. It could cause runtime error (e.g. in the case of uninitialized maps).

Steps To Reproduce
Modify clusterType variable name to any other. Run airshipctl version command. The following error will occur:
`airshipctl version
panic: assignment to entry in nil map

goroutine 1 [running]:
opendev.org/airship/airshipctl/pkg/config.(*Config).reconcileClusters(0xc0004ad280, 0xc0004ad280, 0x0)
/root/airshipctl/pkg/config/config.go:184 +0x69c
opendev.org/airship/airshipctl/pkg/config.(*Config).reconcileConfig(0xc0004ad280, 0xc000437140, 0x19)
/root/airshipctl/pkg/config/config.go:118 +0x2f
opendev.org/airship/airshipctl/pkg/config.(*Config).LoadConfig(0xc0004ad280, 0xc0004370c0, 0x15, 0xc000437140, 0x19, 0xc0000c0ff0, 0xc000010020)
/root/airshipctl/pkg/config/config.go:50 +0xa5
opendev.org/airship/airshipctl/pkg/environment.(*AirshipCTLSettings).InitConfig(0xc000719710)
/root/airshipctl/pkg/environment/settings.go:71 +0x8f
opendev.org/airship/airshipctl/cmd.NewRootCmd.func1(0xc0002d1900, 0x3332ec0, 0x0, 0x0)
/root/airshipctl/cmd/root.go:55 +0x90
github.com/spf13/cobra.(*Command).execute(0xc0002d1900, 0x3332ec0, 0x0, 0x0, 0xc0002d1900, 0x3332ec0)
/root/go/pkg/mod/github.com/spf13/cobra@v0.0.5/command.go:810 +0x210
github.com/spf13/cobra.(*Command).ExecuteC(0xc0002d1680, 0xc000010018, 0xc0002d1680, 0xc000719710)
/root/go/pkg/mod/github.com/spf13/cobra@v0.0.5/command.go:914 +0x2fb
github.com/spf13/cobra.(*Command).Execute(...)
/root/go/pkg/mod/github.com/spf13/cobra@v0.0.5/command.go:864
main.main()
/root/airshipctl/main.go:31 +0xc1`

Expected behavior
runtime error should not be observed

Environment

• airshipctl Version: 0.1.0
• Operating System: ubuntu 16.04
• Kernel version: 4.15.0-88-generic
• Kubernetes Version: 1.16
• Go version: 1.12.10
• Hardware specs (e.g. 4 vCPUs, 16GB RAM, bare metal vs VM):
  24 CPUs, 64GB RAM, bare metal

[airshipbot]

Related Change #722621

Subject: Ensure map is initialized before assignment
Link: https://review.opendev.org/722621
Status: NEW
Owner: Ruslan Aliev (raliev@mirantis.com)

This change will close this issue when merged.

Approvals

Code-Review
! None
Verified
+1 Zuul
Workflow
! None

Last Updated: 2020-05-04 18:30:05 CDT

[drewwalters96]

Per the flightplan call: we will fix this quickly but create a new issue to hand subcommand-level validation of config options

[ian-howell]

This may be related: https://github.com/airshipit/airshipctl/blob/master/pkg/config/options.go#L61

[raliev12]

@drewwalters96 @ian-howell

Summarizing this issue history and answering all comments and disagreements.

1. Related issue is real, confirmed and should be fixed ASAP for sure.
2. We can observe this issue when run any airshipctl subcommand, including help and version, because current logic is to load config every time [1], no matter what user wants from airshipctl. I think everybody agree that throwing wrong config error is a bad idea when you try to get a help for example. So, the first proposal - we have to move config load&init to subcommand level where it is really necessary in the future.
3. We get runtime panic due to attempt to write to uninitialized map, that's expected and that's how golang works. So, to fix the issue we have to verify somehow that config contains initialized ClusterTypes map for each cluster before [2], and then there are two options:

• Throw an error that ClusterTypes is missing for cluster_name and exit, or
• Try to mitigate a problem by initializing map automatically, and issue a warning/error that ClusterTypes for cluster_name is missing from config and they were automatically added

The second option is more preferable in my point of view and here is why:

1. LoadConfig internal logic right now does not imply throwing an error and exit if some options are missed from config. If there are config files, if we can read them as valid YAML (no matter what inside) - LoadConfig must success, because its purpose to load config and create Config object, not to verify it. The only cases when LoadConfig fails - missing files or unable to read them as YAML [3].
2. We have to verify whether Config object is valid or not after LoadConfig finishes and creates it (that's how we do it right now). Also, Config object validation can vary depends on subcommand operation. Another point, if config is broken or incomplete - airshipctl must return appropriate exit code (using RunE logic of cobra). Take a look how it's already implemented properly in isogen command: [4] and [5]. So, we can't perform validate functions such as EnsureComplete during LoadConfig workflow, we should do it after LoadConfig finishes and only on subcommand level. But since the issue occurs during LoadConfig workflow - EnsureComplete can't help us to fix runtime panic at all if we add appropriate condition there, because EnsureComplete is post-loading validation. And last but not least - we have to test this scenario using unit tests and add it to test config, but several test functions expects Config object after LoadConfig call, so all of them will fail if we change logic to throwing an error if there is nil ClusterTypes for some cluster.
3. There are suggestions to move nil checking from reconcileClusters function to somewhere, for example to LoadConfig. I don't think it's a good idea, because, first of all reconsileClusters is a part of LoadConfig workflow, so it doesn't change the logic and meaning. Also, the fixing code itself will be the same everywhere and looks like this anyway:
   for clusterName, _ := range c.kubeConfig.Clusters { clusterComplexName := NewClusterComplexNameFromKubeClusterName(clusterName) if c.Clusters[clusterComplexName.Name].ClusterTypes == nil { <fix actions> } } }
   If we do it somewhere else except reconcileClusters it would be overkill, because the same loop code will be executed twice at least. Moreover, reconcileClusters fix actions will be performed only if nested object of ClusterTypes is supposed to be there and is nil, so in my opinion there is nothing to complain about if we put a solution right here [6] as it's already done in patchset.

The last question - what should we throw in this case - error or warning? I think it should be just printable warning because it's not a critical problem if clusterTypes is missing for cluster which defined in kubeconfig, we can try to create new one automatically right in place. However, error option also works. To make this approach work, we have to add to Config struct an additional object where to store load/init issues. Later, on post-validation level using EnsureComplete function, we can check whether some problems occurred during load process by getting information from appropriate field of Config struct.

Nevertheless, we have to take the first steps to fix the problem now, so the first solution seems proper to me before we can proceed in implementation of next steps. I'd ask you to review once again my patchset [7], taking into account my explanations. All comments and opinions are greatly appreciated.

Thank you.

[1] https://github.com/airshipit/airshipctl/blob/master/cmd/root.go#L55
[2] https://github.com/airshipit/airshipctl/blob/master/pkg/config/config.go#L182
[3] https://github.com/airshipit/airshipctl/blob/master/pkg/config/config.go#L58-L109
[4] https://github.com/airshipit/airshipctl/blob/master/cmd/baremetal/isogen.go#L29-L30
[5] https://github.com/airshipit/airshipctl/blob/master/pkg/bootstrap/isogen/command.go#L42-L43
[6] https://github.com/airshipit/airshipctl/blob/master/pkg/config/config.go#L183
[7] https://review.opendev.org/#/c/722621/