Terraform CLI logs support

[katronquillo]

Description of your changes

Added optional spec.logConfig to the ProviderConfig resource to enable writing of Terraform Plan and Apply output to log file

• spec.logConfig.enableLogging: Specifies whether logging is enabled (true) or disabled (false). When enabled, Terraform CLI command logs will be written to a file. Default is false.
• spec.logConfig.backupLogFilesCount: Specifies the number of archived log files to retain. When a new log file is created due to a change detected by terraform diff, the previous log file is archived and renamed with a timestamp. This parameter controls how many archived log files are kept before older ones are deleted. Default is 0

By default, Terraform CLI stores log files in the workspace directory. The default log file name is terraform.log. When a backup is taken (e.g., due to a new change detected by terraform diff), the current log file is renamed to terraform.log.<timestamp>, where <timestamp> is a placeholder for the actual timestamp of the backup.

Fixes #163

I have:

• Run make reviewable to ensure this PR is ready for review.
• Run make e2e with UPTEST_EXAMPLE_LIST="examples/workspace-random-generator.yaml"

How has this code been tested

• On a local cluster created using make run, applied the following ProviderConfig:

   apiVersion: tf.upbound.io/v1beta1
   kind: ProviderConfig
   metadata:
     name: default
   spec:
     configuration: |
         terraform {
           backend "kubernetes" {
             secret_suffix     = "providerconfig-aws-eu-west-1"
             namespace         = "upbound-system"
             in_cluster_config = true
           }
         }
     logConfig:
       backupLogFilesCount: 1
       enableLogging: True

• To the same cluster, applied the following Workspace:

   apiVersion: tf.upbound.io/v1beta1
   kind: Workspace
   metadata:
     name: example-random-generator
     annotations:
       meta.upbound.io/example-id: tf/v1beta1/workspace
       # The terraform workspace will be named 'random'. If you omit this
       # annotation it would be derived from metadata.name - e.g. 'example-random-generator.
       crossplane.io/external-name: random
   spec:
     forProvider:
       source: Inline
       module: |
         resource "random_id" "example_id" {
           byte_length = 4
         }
         resource "random_password" "password" {
           length = 16
           special = true
         }
         // Non-sensitive Outputs are written to status.atProvider.outputs and to the connection secret.
         output "random_id_hex" {
           value       = random_id.example_id.hex
         }
         // Sensitive Outputs are only written to the connection secret
         output "random_password" {
           value = random_password.password
           sensitive = true
         }
         // Terraform has several other random resources, see the random provider for details
  
     writeConnectionSecretToRef:
       namespace: default
       name: terraform-workspace-example-random-generator

• Observed that terraform.log file was populated with the expected Terraform Plan and Apply logs
• Made changes to the Workspace YAML and re-applied
• Observed that the terraform.log file was updated with the new Terraform Plan and Apply logs
• Observed that no archived files were maintained, since spec.logConfig.backupLogFilesCount was set to 1
  • When this was set to a number greater than 1, observed that previous log files had the Datetime appended and that a new terraform.log file was created with the most recent Terraform Plan and Apply logs
  • Also observed that the oldest archived files were deleted to maintain the specified backupLogFilesCount

[Upbound-CLA]

All committers have signed the CLA.

[ccrockatt]

Hello @ytsarev @bobh66,
I'm wondering if anyone would be able to share a timeline for the review of this PR?
Thanks so much!

[ytsarev]

Sorry for the delay, the kubecon took some energy :) I plan to take care of the review this week

[ytsarev]

/test-examples="examples/workspace-inline-aws.yaml"

[ytsarev]

Thank you so much for the contribution, this is a very useful enhancement 👍

I've tested this change end-to-end, overall it works great with source: Inline Workspace type. There are some suggestions and questions regarding behavior I left inline.

ls -la affe12f0-377a-47a4-a9fd-d73295bda1d9/
total 44
drwx------    3 2000     2000          4096 Apr  2 21:40 .
drwxr-xr-x    1 2000     root          4096 Apr  2 21:44 ..
-rw-------    1 2000     2000            68 Apr  2 21:44 .git-credentials
drwxr-xr-x    3 2000     2000          4096 Apr  2 21:20 .terraform
-rw-r--r--    1 2000     2000          1377 Apr  2 21:20 .terraform.lock.hcl
-rw-------    1 2000     2000           111 Apr  2 21:44 aws-creds.ini
-rw-------    1 2000     2000           276 Apr  2 21:44 crossplane-provider-config.tf
-rw-------    1 2000     2000           424 Apr  2 21:44 main.tf
-rw-------    1 2000     2000          2358 Apr  2 21:40 terraform.log
-rw-------    1 2000     2000          2378 Apr  2 21:39 terraform.log.20240402-214029

The above looks great.

The main blocker is that apparently the change does not work with source: Remote Workspace type, I cannot see log files appearing in this case.

/tf $ ls -al d94ed4b6-61b1-4d28-b7be-b827020b31cf/
total 44
drwxr-xr-x    3 2000     2000          4096 Apr  2 21:37 .
drwxr-xr-x    1 2000     root          4096 Apr  2 21:37 ..
-rw-------    1 2000     2000            68 Apr  2 21:37 .git-credentials
drwxr-xr-x    3 2000     2000          4096 Apr  2 21:37 .terraform
-rw-r--r--    1 2000     2000          1377 Apr  2 21:37 .terraform.lock.hcl
-rw-------    1 2000     2000           111 Apr  2 21:37 aws-creds.ini
-rw-------    1 2000     2000           276 Apr  2 21:37 crossplane-provider-config.tf
-rw-r--r--    1 2000     2000           190 Apr  2 21:37 main.tf
-rw-r--r--    1 2000     2000            98 Apr  2 21:37 outputs.tf
-rw-r--r--    1 2000     2000            73 Apr  2 21:37 variables.tf

Most probably it relates to the fact that we are wiping out the workdir for Remote workspaces due to go-getter behavior here https://github.com/upbound/provider-terraform/blob/main/internal/controller/workspace/workspace.go#L230-L233 on each reconciliation loop.

Maybe we can put the logs to /tf/logs/$uuid to persist independently from the workspace directory.

It's easy to reproduce the issue with https://github.com/upbound/provider-terraform/blob/main/examples/transition/00-mr-tf-workspace/workspace-remote.yaml

[ytsarev]

Suggested change

	To enable it, the `ProviderConfig` spec has an **optional** `LogConfig` field.
	To enable it, the `ProviderConfig` spec has an **optional** `logConfig` field.

nit: LogConfig -> logConfig

[ytsarev]

should we move it to const ?

[ytsarev]

if we move out terraform.log to shared const above, we could reference it from a single place and potentially avoid mismatch in case of the future changes.

[ytsarev]

So we are not running writeTerraformCLILogs in case the terraform succeeded with no diff.
That makes sense in general but in this case, the logs will not appear in case of the ProviderConfig setting change
from enableLogging: false to enableLogging: true for already existing Workspaces.

It might confuse users as the terraform.log file will never appear in this case until some diff event. If it is expected it worth documenting somewhere.

[negz]

@katronquillo Could you provide an example of what this log output looks like? Is it essentially just typical terraform apply output?

I'm coming into this conversation late (sorry!) but I'm pretty wary of writing logs to file paths inside the container, and especially of implementing our own log rotation logic. Ideally we'd outsource log processing to the many existing tools that handle it already (systemd etc).

I'm wondering if there's a more lightweight option here.

For example, I'm wondering if it would be possible to emit Terraform logs to the provider's container stdout. I'm imagining something like this:

{"ts": "...", "level": "info", "workspace": "some-ws", "source": "tf-cli", "msg": "<terraform log line goes here>"}

If we took this approach Terraform CLI logs would be interspersed with the providers other logs, but given we already support structured logging it would be possible for any log parsing backend to reconstruct the Terraform logs for a particular workspace.

[bobh66]

If we took this approach Terraform CLI logs would be interspersed with the providers other logs, but given we already support structured logging it would be possible for any log parsing backend to reconstruct the Terraform logs for a particular workspace.

If we take this approach we may also want to be able to control the log output at the Workspace level to limit the amount of data being sent to the pod logs.

[negz]

If we take this approach we may also want to be able to control the log output at the Workspace level to limit the amount of data being sent to the pod logs.

Using an MR or ProviderConfig to control how a provider process logs to stdout does seem a little unusual to me. My inclination would be for it to be an all or nothing CLI flag.

What would be the consequences of lots of Terraform logs being written to stdout?

[bobh66]

My inclination would be for it to be an all or nothing CLI flag.

Meaning it gets set by a DeploymentRuntimeConfig? The downside of that is it requires a pod restart to enable/disable, and for large numbers of Workspaces a pod restart can take a long time to recover since it has to run terraform init on every Workspace. This can be mitigated by using a PVC for /tf so the workspace directories persist across the reboot, but that's not the default configuration and I suspect most users don't do that.

What would be the consequences of lots of Terraform logs being written to stdout?

Mostly the sheer volume of data to sift through and the tendency of the pod logs to roll over just when you need them. There are ways to work around this but typical development debugging scenarios will just be using kubectl logs and not relying on external log collection systems.

[katronquillo]

Hi @negz @bobh66 @ytsarev! Thank you for reviewing our PR. We agree with your feedback/concerns and we'll go ahead and implement the changes you've proposed. To be specific, we'll create a new PR where we will...

• Add a boolean flag to the Workspace spec to enable/disable logging
• If the flag is True, we will write the Terraform CLI logs to container stdout as structured records, similar to what @negz suggested

[suramasamy]

hi @negz @ytsarev @bobh66 We have created this new PR based on the above discussions. Kindly review when you get a chance.

[bobh66]

One other note on this implementation - for Remote Workspaces we delete the directory on every reconcile due to issues with go-getter so the logs for the previous reconcile will be removed and the current logs will only persist until the next reconciliation happens. I'm thinking that may be another reason to go with #258 instead

[bobh66]

One other note on this implementation - for Remote Workspaces we delete the directory on every reconcile due to issues with go-getter so the logs for the previous reconcile will be removed and the current logs will only persist until the next reconciliation happens. I'm thinking that may be another reason to go with #258 instead

This is no longer true after #276