Golang zero values are not set in kops cluster config

[peter-svensson]

The creation of cluster (and perhaps other ) configs are not handling golang zero values.
The zero value for a bool is false and the parameters set in the terraform files with false are not propagated to the kops configuration.

For example:

  metrics_server {
    enabled  = true
    insecure = true
  }

Renders correctly to:

  metricsServer:                                                                                                  
    enabled: true
    insecure: true

However:

  metrics_server {
    enabled  = true
    insecure = false
  }

Renders to:

  metricsServer:                                                                                                  
    enabled: true

This happens for all settings that I've tested, which more or less breaks every cluster.

[peter-svensson]

A possible fix might be to remove the checks for zero values in the DataSource_XXX.generated.go files:

if reflect.DeepEqual(in, reflect.Zero(reflect.TypeOf(in)).Interface()) {
  return nil
}

Removing those will set the value to *false instead of nil which seems to work fine at least for my small test

[eddycharly]

Hello, thanks for reporting the issue.
This is a general problem in terraform that does not distinguish zero values and values not set in config.

One way to disable the behaviour is to mark the field mandatory.

I will look at it today or tomorrow.

[eddycharly]

@peter-svensson there was this fix #256 I applied to anonymous auth, I wonder if something similar would make sense here. WDYT ?

[peter-svensson]

Probably, but there are a lot of places where this will have to be applied?
The metrics_server was just an example, the issue I discovered was for the cluster autoscaler addon.

[eddycharly]

Indeed it could be needed in a lot of places.
I'm not sure how kOps handles nil vs false (maybe when the field is nil kOps has some internal logic to determine the value to apply).

I will try to dig a bit deeper in the kOps code.

[peter-svensson]

Any progress on this? Anyway I can help?

[eddycharly]

The metrics_server was just an example, the issue I discovered was for the cluster autoscaler addon.

What attribute was causing the issue in the autoscaler addon please ?

From what i see those two could be a problem as they have default true:

	// SkipNodesWithSystemPods makes cluster autoscaler skip scale-down of nodes with non-DaemonSet pods in the kube-system namespace.
	// Default: true
	SkipNodesWithSystemPods *bool `json:"skipNodesWithSystemPods,omitempty"`
	// SkipNodesWithLocalStorage makes cluster autoscaler skip scale-down of nodes with local storage.
	// Default: true
	SkipNodesWithLocalStorage *bool `json:"skipNodesWithLocalStorage,omitempty"`

[argoyle]

If I remember correctly it was SkipNodesWithLocalStorage that we tried to set to false. Right @peter-svensson ?

[argoyle]

But in general it's any value where the default-value is different than the zero-value.

[eddycharly]

I see two options here:

• make the fields required so that we don't have to try to make sense out of zero values
• add support for default values in tf schemas and somewhat duplicate the existing logic in kOps

I feel more comfortable with the first approach, it shifts some work on the end user side but it has the advantage of being explicit.

[eddycharly]

@argoyle @peter-svensson sorry for taking so long, i opened #346 to fix the case with bool fields when default is true.
i'll cut a new alpha tonight, let me know if it fixes your issue and/or if there are other cases to cover 🤞

[eddycharly]

v1.21.0-alpha.5 should be available in a couple of minutes.

[peter-svensson]

Tested the autoscaler and it seems to work just fine at least 👍

[eddycharly]

@peter-svensson @argoyle can we close the issue ?

[argoyle]

I would say so. We'll open another one if we find anything else. Thanks for your work! ❤️

[eddycharly]

Thanks. Yes, don't hesitate to open issues, it helps improving the provider 🙏