modules/tectonic/assets: ignore cluster_id regeneration

[Quentin-M]

This PR reverts #225 as it breaks the Tectonic Channel Operator, which expects an uuid. We instead use the ignore_changes lifecycle attribute, as documented to achieve the same goal of avoiding the regeneration of the assets at every plan/apply executions.

/cc @alexsomesan

[coreosbot]

Can one of the admins verify this patch?

[Quentin-M]

https://i.giphy.com/qxtxlL4sFFle.gif

[alexsomesan]

The documented suggestion to use the lifecycle/ignore_changes is correct. It's the preferred in such situations. It was also my first choice when originally trying to address this issue.

Documentation exemplifies usage of lifecycle like this:

lifecycle {
    [ignore_changes = [ATTRIBUTE NAME, ...]]
}

Unfortunately, in this particular setting, it's not applicable. The reason behind is that "cluster_id" is not a resource attribute, it's a member of the "vars" map value ("vars" is a resource attribute here).

The problem is easily reproducible by doing an apply and then a plan. The plan will show a change with the following cause:

vars.cluster_id:                         "a11cdadb-b13b-0b93-9e71-b6fa8a4af9b1" => "ec33990c-645f-9d94-a8b6-d4ceef88fd28" (forces new resource)

We need another solution here.
Potentially, you could try moving the generation of the cluster_id into a null_data_source and apply ignore_changes there, on the inputs. This suggestion off the top of my head and not actually tested.

[sym3tri]

see @alexsomesan's comment

[sym3tri]

worst case: we should revert the change entirely back to just use uuid(). It's annoyance vs broken.

[Quentin-M]

@alexsomesan data sources do not have the lifecycle block, as they do not have any lifecycle at all (only the READ operation). And null resources do not let you specify any extra attribute anymore but triggers.

[alexsomesan]

Would loosening the expectation of a uuid on the TCO not be an option?

[alexsomesan]

Yeah, I tried the datasource approach in the mean time. No worky :/

[alexsomesan]

There actually seems to be a troubled history with uuid():
See hashicorp/terraform#5644 and various referenced issues in the comments.

[Quentin-M]

I figured out a fix, but just found out that the interp function I am using is only avail in v0.9.2+..
Edit: And a gross way to do it with v0.8.8, cool.

[alexsomesan]

Terraform 0.9.4 was just released with the ignition fix included. We are good to upgrade.
I have a WIP PR with the template changes required for 0.9.x wich I can merge tomorrow and do the upgrade (or you can merge it).

Go ahead and PR the fix for 0.9.x

[Quentin-M]

Just submitted the idea for v0.8.8. Can be shortened a lot when using v0.9.2+ thanks to substr. Could also simply submit a patch upstream to add a first-class random_uuid resource - it'd make lots of sense given the drawback of the interp function.

[Quentin-M]

(v0.9.4 includes both local_file and template_dir, so they should be removed from the installer too)

[alexsomesan]

Nice!
Before merging can you please build a cluster and confirm TCO is working properly with this change?
Otherwise LGTM

[yifan-gu]

@alexsomesan Some background: IIRC the omaha server requires a uuid when reporting back to the server. And the uuid should map to the cluster so we can know which cluster is reporting back.

[Quentin-M]

Tests are borked due to account quotas. Deployed cluster fine, TCO was running fine, and the UUID was stable across runs, therefore no additional assets.

$ PLATFORM=aws CLUSTER=qmachu make plan
No changes. Infrastructure is up-to-date.

This means that Terraform did not detect any differences between your
configuration and real physical resources that exist. As a result, Terraform
doesn't need to do anything.

$ kubectl logs -n=tectonic-system tectonic-channel-operator-3494414446-k2vwl
I0427 06:47:31.607898       1 leader_election.go:47] Trying to be the leader...
I0427 06:47:31.644025       1 leaderelection.go:188] sucessfully acquired lease tectonic-system/tectonic-channel-operator
I0427 06:47:35.661896       1 main.go:511] Tectonic Channel Operator starts watching updates from core update
I0427 06:47:35.665117       1 main.go:242] No updates available
I0427 06:47:45.669098       1 main.go:242] No updates available
I0427 06:47:55.671619       1 main.go:242] No updates available
I0427 06:48:05.674246       1 main.go:242] No updates available
I0427 06:48:15.676954       1 main.go:242] No updates available
I0427 06:48:25.679585       1 main.go:242] No updates available

$ kubectl get configmap tectonic-config -n=tectonic-system -o yaml
apiVersion: v1
data:
  certificatesStrategy: installerGeneratedCA
  clusterID: 4f2ca7e4-f243-9a6b-4aaf-78f32fb7b447
  consoleBaseAddress: <redacted>
  dexAPIHost: tectonic-identity-api.tectonic-system.svc.cluster.local:5557
  installerPlatform: aws
  kubeAPIServerURL: <redacted>
  tectonicUpdaterEnabled: "true"
  tectonicVersion: 1.6.2-tectonic.0
kind: ConfigMap
metadata:
  creationTimestamp: 2017-04-27T06:46:38Z
  name: tectonic-config
  namespace: tectonic-system
  resourceVersion: "1268"
  selfLink: /api/v1/namespaces/tectonic-system/configmaps/tectonic-config
  uid: 437790df-2b15-11e7-b7e3-060cb9e672ab

[alexsomesan]

All looks good!
Let's get this in.