-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Terraform Plan Output in Status Field #226
base: main
Are you sure you want to change the base?
Conversation
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 My biggest concern here is if the plan output contains sensitive information, then we will be exposing it in |
Co-authored-by: Alper Rifat Ulucinar <[email protected]>
Co-authored-by: Alper Rifat Ulucinar <[email protected]>
Co-authored-by: Alper Rifat Ulucinar <[email protected]>
Co-authored-by: Alper Rifat Ulucinar <[email protected]>
Co-authored-by: Alper Rifat Ulucinar <[email protected]>
Co-authored-by: Alper Rifat Ulucinar <[email protected]>
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 ? |
@balu-ce you are right, let's keep it in Workspace spec, it will provide more granular control. |
@ytsarev Please review the PR, added a conditional value under workspace spec |
/test-examples="examples/workspace-inline-aws.yaml" |
@ytsarev Added in example |
For the record, I'm not convinced that we should do this at all. See #223 (comment). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be includePlan
?
@bobh66 what alternatives to |
@ytsarev two possibilities:
both have advantages and drawbacks, as does the |
@bobh66 I don't have a preference either:
On the other hand, we should probably consolidate this PR with #248 as there is an intersecting data stream in both. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
@bobh66 after the above e2e experiment - the |
@ytsarev The plan text could be gzipped/base64-encoded to keep it small, and the |
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 |
@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 |
@balu-ce thanks for the clarification. In this case, this flow requires documentation. |
@ytsarev Yes, Agree. |
@bobh66 Any progress on this or we are yet to close. |
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:
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