Terraform Plan Output in Status Field

[balu-ce]

Description of your changes

This PR is to add the terraform plan output in the status field of crd. This can be integrated with managment polices to apply based on plan

Fixes #223

I have:

• [x] Run make reviewable to ensure this PR is ready for review.

How has this code been tested

With Local setup as well as been deployed in one of our eks cluster and tested it

[Upbound-CLA]

All committers have signed the CLA.

[ulucinar]

Thank you @balu-ce for the PR. I believe the plan output in the status can be beneficial in understanding the modifications to be applied to the external resource, and as described in this PR, it can be used in tandem with the management policies to implement a workflow where the updates to the external resources are first reviewed and then approved (via adding the Update management policy).

My biggest concern here is if the plan output contains sensitive information, then we will be exposing it in status.atProvider. Do we know whether Terraform can potentially leak sensitive data in the plan output?

[balu-ce]

I agree @ulucinar , can the team have their variable as sensitive in terraform defined so that the values will not be shown in plan . For ref https://github.com/hashicorp/terraform/issues/27577. https://developer.hashicorp.com/terraform/tutorials/configuration-language/sensitive-variables

[balu-ce]

I agree that we can have a flag but cant we just keep it in workspace object itself . Any Specific reason to bring at config level ?

[ytsarev]

@balu-ce you are right, let's keep it in Workspace spec, it will provide more granular control.

[balu-ce]

@ytsarev Please review the PR, added a conditional value under workspace spec

[ytsarev]

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

[balu-ce]

@ytsarev Added in example

[negz]

For the record, I'm not convinced that we should do this at all. See https://github.com/upbound/provider-terraform/issues/223#issuecomment-1918499388.

[ytsarev]

@bobh66 what alternatives to status do you have in mind?

[bobh66]

@ytsarev two possibilities:

• an Event with the plan stored in the message field
• a Plan child resource of the Workspace

both have advantages and drawbacks, as does the status, and I don't have a good feeling for which is "better"

[ytsarev]

@bobh66 I don't have a preference either:

• the tf plan seems too much for the single Event
• New Plan resource seems to be overkilling solution

status to figure out what is happening without getting into the pod seems to be 'good enough'.

On the other hand, we should probably consolidate this PR with https://github.com/upbound/provider-terraform/pull/248 as there is an intersecting data stream in both.

[ytsarev]

@bobh66 after the above e2e experiment - the Event actually looks like a better candidate - we could look into the tf plan data that relates to the previous reconciliation loops

[bobh66]

@ytsarev The plan text could be gzipped/base64-encoded to keep it small, and the Event should only be created when there is a diff detected. The number of available plan Events will depend on how long the API server retains the data. It seems like a reasonable "kubernetes-ish" solution.

[bobh66]

Another option would be to converge with #248 and send the data to the pod logs, though I do like the idea of creating an Event when a diff is detected and including the plan output in the Event.

[balu-ce]

@ytsarev @bobh66 This tfPlan disappeared due to it sets the status object again. Basically the update is not calling the observe. Also the use of show plan is to integrate with conditional before apply. If you execute the code with setting management policy to only observe, it will work as expected. In this way we can integrate with tight approval or we need to call observe in update

[ytsarev]

@balu-ce thanks for the clarification. In this case, this flow requires documentation.

[balu-ce]

@ytsarev Yes, Agree.

[balu-ce]

@ytsarev @bobh66 Is there is any plan to bring this feature in next release

[balu-ce]

@bobh66 Any progress on this or we are yet to close.