Lokomotive: Constant diff in `lokoctl cluster apply`

[johananl]

When re-running lokoctl cluster install on a cluster which was just deployed and should therefore be aligned with the desired configuration, we see a constant diff:

Terraform will perform the following actions:

  # module.packet-johannes-test.module.bootkube.template_dir.bootstrap-manifests will be created
  + resource "template_dir" "bootstrap-manifests" {
      + destination_dir = "../cluster-assets/bootstrap-manifests"
      + id              = (known after apply)
      + source_dir      = "../lokomotive-kubernetes/bootkube/resources/bootstrap-manifests"
      + vars            = {
          + "cloud_provider"    = ""
          + "etcd_servers"      = "https://johannes-test-etcd0.dev.lokomotive-k8s.net.:2379"
          + "hyperkube_image"   = "k8s.gcr.io/hyperkube:v1.17.3"
          + "pod_cidr"          = "10.2.0.0/16"
          + "service_cidr"      = "10.3.0.0/16"
          + "trusted_certs_dir" = "/usr/share/ca-certificates"
        }
    }

  # module.packet-johannes-test.module.bootkube.template_dir.calico[0] will be created
  + resource "template_dir" "calico" {
      + destination_dir = "../cluster-assets/charts/kube-system/calico"
      + id              = (known after apply)
      + source_dir      = "../lokomotive-kubernetes/bootkube/resources/charts/calico"
    }

  # module.packet-johannes-test.module.bootkube.template_dir.kube-apiserver will be created
  + resource "template_dir" "kube-apiserver" {
      + destination_dir = "../cluster-assets/charts/kube-system/kube-apiserver"
      + id              = (known after apply)
      + source_dir      = "../lokomotive-kubernetes/bootkube/resources/charts/kube-apiserver"
    }
...

Confirming the change results in a successful terraform apply which doesn't seem to have any negative effects, however the diff probably shouldn't be there.

[invidian]

@johananl perhaps this issue should be moved to https://github.com/kinvolk/lokomotive.

TL;DR the issue is how template_dir from template Terraform provider handles diff, which is not great right now.

[johananl]

Can't we use templatefile()? (I didn't look at the details there)

[invidian]

Can't we use templatefile()? (I didn't look at the details there)

No, because what we need is copying directories, which is not-trivial with Terraform right now, and template_dir allows that.

[invidian]

Interesting, it seems running terraform plan inside assets directory does not produce any plan, but cluster apply does. My guess is we don't set the working directory right?

[invidian]

My guess is we don't set the working directory right?

It seems we do.

Actually, now Terraform shows the plan as well. Maybe re-initializing Terraform breaks it somehow then :thinking:

[invidian]

FYI, following patch prevents continuous diff, but if assets directory gets removed, it still occurs:

diff --git a/pkg/util/walkers/copying.go b/pkg/util/walkers/copying.go
index dd82612b..31cfcabb 100644
--- a/pkg/util/walkers/copying.go
+++ b/pkg/util/walkers/copying.go
@@ -16,9 +16,11 @@ package walkers

 import (
        "fmt"
+       "reflect"
        "io"
        "os"
        "path/filepath"
+       "io/ioutil"

        "github.com/pkg/errors"

@@ -44,6 +46,15 @@ func CopyingWalker(path string, newDirPerms os.FileMode) assets.WalkFunc {
 // writeFile writes data from given io.Reader to the file and makes sure, that
 // this is the only content stored in the file.
 func writeFile(p string, r io.Reader) error {
+       newContent, err := fileNeedsUpdate(p, r)
+       if err != nil {
+               return fmt.Errorf("failed checking if file needs to be updated: %w", err)
+       }
+
+       if newContent == nil {
+               return nil
+       }
+
        // TODO: If we start packing binaries, make sure they have executable bit set.
        f, err := os.OpenFile(p, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0644)
        if err != nil {
@@ -51,9 +62,36 @@ func writeFile(p string, r io.Reader) error {
        }
        defer f.Close()

-       if _, err := io.Copy(f, r); err != nil {
+       if _, err := f.Write(newContent); err != nil {
                return fmt.Errorf("failed writing to file %s: %w", p, err)
        }

        return nil
 }
+
+func fileNeedsUpdate(p string, r io.Reader) ([]byte, error) {
+  fc, err := ioutil.ReadAll(r)
+  if err != nil {
+    return nil, fmt.Errorf("failed reading file content from assets: %w", err)
+  }
+
+       _, err = os.Stat(p)
+  if os.IsNotExist(err) {
+               return fc, nil
+  }
+
+       if err != nil {
+               return nil, fmt.Errorf("failed to stat file %s: %w", p, err)
+       }
+
+  ofc, err := ioutil.ReadFile(p)
+  if err != nil {
+    return nil, fmt.Errorf("failed reading file %s for content comparison: %w", p, err)
+  }
+
+       if reflect.DeepEqual(ofc, fc) {
+               return nil, nil
+       }
+
+       return fc, nil
+}

[invidian]

So it seems this constant diff is being created because modification time changes on source files. This can be reproduced on a single example:

provider "template" {
  version = "2.1.2"
}

resource "template_dir" "test" {
  source_dir      = "./src"
  destination_dir = "./dest"
}

Now the question is, what to do with that, as I think this behavior is technically correct in my opinion.

Option 1

Add some parameter to template_dir to ignore modification time changes, if content of the file did not change.

Considering, that upstream Terraform providers are horribly maintained and inability of Terraform to easily fetch custom providers, I'd rather avoid this option.

Option 2

Get rid of template_dir and use something else for copying directories. We currently only use it in few places, to copy assets from source directory to user directory. So Terraform can then copy them from there etc. Using template_dir for copying directories anyway is a workaround, as there is no other declarative way of doing it via Terraform, but as noted in the code, it may have some undesired effects (if source contains Terraform-valid template syntax, it will be evaluated).

Option 3

Avoid copying directories completely somehow.

[surajssd]

Should we at least document it, so users don't find it confusing?

[johananl]

I wouldn't document a bug in the product docs :-/ I suggest we just fix this ASAP.

[ipochi]

From terraform docs (https://www.terraform.io/docs/providers/template/r/dir.html)

Note When working with local files, Terraform will detect the resource as having been deleted each
time a configuration is applied on a new machine where the destination dir is not present and will
generate a diff to create it. This may cause "noise" in diffs in environments where configurations are
routinely applied by many different users or within automation systems.

[johananl]

@ipochi I don't think this is it. We get this diff even when re-running lokoctl immediately after deploying a cluster with no config changes.

[invidian]

Considering, that upstream Terraform providers are horribly maintained and inability of Terraform to easily fetch custom providers, I'd rather avoid this option.

NOTE: this will change with upcoming Terraform 0.13, as it will allow to pull provider from 3rd party repositories, so we can just have our own repository then, with patched version of provider. Definitely not the nicest approach, but one of the easiest in my opinion.

[johananl]

I'm currently evaluating an approach which gets rid of template_dir by using templatefile() when templating is necessary and generating files which don't require templating using Go code directly under $assets_dir/cluster-assets/, thus skipping the "re-generation" by Terraform entirely.

I am also considering looking into fetching charts from a Helm repo instead of copying them over SSH. If it's not too crazy or out of scope for this issue, doing so could move us closer to the idea of self-contained release packages decoupled from lokoctl.

[johananl]

I'm currently evaluating an approach which gets rid of template_dir by using templatefile() when templating is necessary and generating files which don't require templating using Go code directly under $assets_dir/cluster-assets/, thus skipping the "re-generation" by Terraform entirely.

This seems to work nicely and it fixes the constant diff problem. I still need to address two cases which require passing the platform config all the way down to the module which generates files from memory to disk:

• [x] Use self-hosted kubelet only when enabled in the config.
• [x] Use Calico host protection only when the platform is Packet.

[johananl]

https://github.com/kinvolk/lokomotive/issues/716 is a blocker following feedback on the PR.

[invidian]

Hm, as template provider for Terraform is deprecated, the replacement of template_dir is https://registry.terraform.io/modules/hashicorp/dir/template/1.0.2. Perhaps this wouldn't have the same flaws as the template_dir?