Terraform Plan Output in Status Field #226

balu-ce · 2024-01-05T08:01:06Z

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:

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 · 2024-01-05T08:01:10Z

All committers have signed the CLA.

internal/terraform/terraform.go

apis/v1beta1/workspace_types.go

ulucinar · 2024-01-22T13:07:50Z

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?

Co-authored-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

balu-ce · 2024-01-22T14:32:25Z

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 hashicorp/terraform#27577.
https://developer.hashicorp.com/terraform/tutorials/configuration-language/sensitive-variables

ytsarev

As there are valid concerns around the size of CR and performance / functional issues that it may bring, I suggest we make this functionality optional and controlled by the flag in ProviderConfig.

balu-ce · 2024-01-28T06:27:34Z

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 · 2024-01-29T09:15:01Z

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

balu-ce · 2024-01-29T12:52:40Z

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

ytsarev · 2024-01-30T16:12:11Z

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

internal/terraform/terraform.go

balu-ce · 2024-01-31T05:58:08Z

@ytsarev Added in example

apis/v1beta1/workspace_types.go

negz · 2024-01-31T06:58:12Z

For the record, I'm not convinced that we should do this at all. See #223 (comment).

bobh66

I can see the value in this but I'm not sure it should be in status. @ytsarev what do you think? I can really go either way.

bobh66 · 2024-03-26T15:22:54Z

examples/workspace-inline.yaml

@@ -13,6 +13,7 @@ spec:
    # For simple cases you can use an inline source to specify the content of
    # main.tf as opaque, inline HCL.
    source: Inline
+    showPlan: true


Should be includePlan ?

ytsarev · 2024-03-26T20:44:13Z

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

bobh66 · 2024-03-26T21:05:51Z

@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 · 2024-04-03T09:26:34Z

@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 #248 as there is an intersecting data stream in both.

ytsarev

@balu-ce

Thanks a lot for your contribution!

I've performed the manual e2e test of this PR with https://github.com/upbound/provider-terraform/blob/main/examples/workspace-inline-aws.yaml and spec.forProvider.includePlan: true

Unfortunately I was not able to see the plan in status apart from No change in the terraform plan message:

  status:
    atProvider:
      checksum: b1d1f9e12361c1ef23e23c7efd1eccff
      outputs:
        subnet_data:
          arn: arn:aws:ec2:eu-central-1:123456:subnet/subnet-06fa8cd7759bb8832
          id: subnet-06fa8cd7759bb8832
        vpc_id: vpc-0332256723323320a
      tfPlan: No change in the terraform plan

Whenever I try to emulate the configuration drift by manually editing Name tag on VPC resource, the Workspace reconciles but at that moment the tfPlan field disappears from the status:

  status:
    atProvider:
      outputs:
        subnet_data:
          arn: arn:aws:ec2:eu-central-1:123456:subnet/subnet-06fa8cd7759bb8832
          id: subnet-06fa8cd7759bb8832
        vpc_id: vpc-0332256723323320a

After another reconciliation loop it gets back to tfPlan: No change in the terraform plan

So effectively I do not see any actional output in this scenario. Am I missing something with my test flow?

Please also squash the commits in this PR as some of them are conflicting with the rebase with main

Thanks!

ytsarev · 2024-04-03T10:08:24Z

@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 · 2024-04-03T14:26:44Z

@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 · 2024-04-03T14:35:02Z

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 · 2024-05-14T12:54:38Z

@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 · 2024-05-14T13:00:21Z

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

balu-ce · 2024-05-14T13:12:02Z

@ytsarev Yes, Agree.

balu-ce · 2024-05-15T10:47:38Z

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

balu-ce added 2 commits January 3, 2024 23:04

Added tfplan in status

dc2314a

Added test cass

6959ecb

balu-ce requested review from bobh66, ytsarev, negz, ulucinar, sergenyalcin and turkenf as code owners January 5, 2024 08:01

balu-ce added 3 commits January 5, 2024 14:18

reverted files

c9271b1

Revertted permissions for docs

806d519

Making the file executable permission

675d146

samof76 reviewed Jan 6, 2024

View reviewed changes

internal/terraform/terraform.go Outdated Show resolved Hide resolved

balu-ce requested a review from samof76 January 8, 2024 06:25

ulucinar reviewed Jan 22, 2024

View reviewed changes

balu-ce and others added 6 commits January 22, 2024 19:49

Update apis/v1beta1/workspace_types.go

d320a9e

Co-authored-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

Update internal/terraform/terraform.go

6d1b103

Co-authored-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

Update internal/terraform/terraform.go

7ae2cdf

Co-authored-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

Update internal/terraform/terraform.go

a6650bf

Co-authored-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

Update internal/terraform/terraform.go

9470044

Co-authored-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

Update internal/terraform/terraform.go

4a44ed9

Co-authored-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

balu-ce requested a review from ulucinar January 22, 2024 14:34

balu-ce and others added 2 commits January 25, 2024 11:34

Merge branch 'main' into main

80bec59

Fixing syntax issues

0834fc7

ytsarev requested changes Jan 27, 2024

View reviewed changes

balu-ce requested a review from ytsarev January 28, 2024 06:45

Conditional var for show plan

7a83061

negz reviewed Jan 30, 2024

View reviewed changes

internal/terraform/terraform.go Outdated Show resolved Hide resolved

Removed plan format string

e4e29d6

balu-ce requested a review from negz January 31, 2024 06:04

negz reviewed Jan 31, 2024

View reviewed changes

apis/v1beta1/workspace_types.go Outdated Show resolved Hide resolved

apis/v1beta1/workspace_types.go Outdated Show resolved Hide resolved

apis/v1beta1/workspace_types.go Outdated Show resolved Hide resolved

balu-ce added 2 commits January 31, 2024 12:39

Change in variables format

8ae4f30

Added includePlan as optional

bd894e1

balu-ce requested a review from negz February 13, 2024 10:25

bobh66 mentioned this pull request Feb 14, 2024

Support for Policy as Code against the planfile on the Update / Create #223

Open

bobh66 reviewed Mar 26, 2024

View reviewed changes

ytsarev requested changes Apr 3, 2024

View reviewed changes

Merge branch 'main' into main

9d4a36d

Merge branch 'upbound:main' into main

d22ba11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Terraform Plan Output in Status Field #226

Terraform Plan Output in Status Field #226

balu-ce commented Jan 5, 2024

Upbound-CLA commented Jan 5, 2024 •

edited

Loading

ulucinar commented Jan 22, 2024

balu-ce commented Jan 22, 2024 •

edited

Loading

ytsarev left a comment

balu-ce commented Jan 28, 2024 •

edited

Loading

ytsarev commented Jan 29, 2024

balu-ce commented Jan 29, 2024

ytsarev commented Jan 30, 2024

balu-ce commented Jan 31, 2024

negz commented Jan 31, 2024

bobh66 left a comment

bobh66 Mar 26, 2024

ytsarev commented Mar 26, 2024

bobh66 commented Mar 26, 2024

ytsarev commented Apr 3, 2024

ytsarev left a comment

ytsarev commented Apr 3, 2024

bobh66 commented Apr 3, 2024

bobh66 commented Apr 3, 2024 •

edited

Loading

balu-ce commented May 14, 2024

ytsarev commented May 14, 2024

balu-ce commented May 14, 2024

balu-ce commented May 15, 2024

Terraform Plan Output in Status Field #226

Are you sure you want to change the base?

Terraform Plan Output in Status Field #226

Conversation

balu-ce commented Jan 5, 2024

Description of your changes

How has this code been tested

Upbound-CLA commented Jan 5, 2024 • edited Loading

ulucinar commented Jan 22, 2024

balu-ce commented Jan 22, 2024 • edited Loading

ytsarev left a comment

Choose a reason for hiding this comment

balu-ce commented Jan 28, 2024 • edited Loading

ytsarev commented Jan 29, 2024

balu-ce commented Jan 29, 2024

ytsarev commented Jan 30, 2024

balu-ce commented Jan 31, 2024

negz commented Jan 31, 2024

bobh66 left a comment

Choose a reason for hiding this comment

bobh66 Mar 26, 2024

Choose a reason for hiding this comment

ytsarev commented Mar 26, 2024

bobh66 commented Mar 26, 2024

ytsarev commented Apr 3, 2024

ytsarev left a comment

Choose a reason for hiding this comment

ytsarev commented Apr 3, 2024

bobh66 commented Apr 3, 2024

bobh66 commented Apr 3, 2024 • edited Loading

balu-ce commented May 14, 2024

ytsarev commented May 14, 2024

balu-ce commented May 14, 2024

balu-ce commented May 15, 2024

Upbound-CLA commented Jan 5, 2024 •

edited

Loading

balu-ce commented Jan 22, 2024 •

edited

Loading

balu-ce commented Jan 28, 2024 •

edited

Loading

bobh66 commented Apr 3, 2024 •

edited

Loading