Extract Terraform error messages from JSON logs #143

ulucinar · 2021-11-15T14:31:52Z

Description of your changes

Fixes #44

This PR proposes changes that parse the actual error messages from respective JSON-formatted Terraform CLI logs in case the Terraform CLI pipeline fails.

I have:

Read and followed Crossplane's contribution process.
Run make reviewable to ensure this PR is ready for review.
Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

On top of crossplane-contrib/provider-jet-azure#83, an errored condition without these changes results in:

status:
  atProvider: {}
  conditions:
  - lastTransitionTime: "2021-11-15T14:05:25Z"
    reason: Creating
    status: "False"
    type: Ready
  - lastTransitionTime: "2021-11-15T14:05:25Z"
    message: |-
      create failed: cannot apply: cannot apply: {"@level":"info","@message":"Terraform 1.0.3","@module":"terraform.ui","@timestamp":"2021-11-15T17:05:05.983000+03:00","terraform":"1.0.3","type":"version","ui":"0.1.0"}
      {"@level":"info","@message":"azurerm_resource_group.example-alper-2: Plan to create","@module":"terraform.ui","@timestamp":"2021-11-15T17:05:15.213746+03:00","change":{"resource":{"addr":"azurerm_resource_group.example-alper-2","module":"","resource":"azurerm_resource_group.example-alper-2","implied_provider":"azurerm","resource_type":"azurerm_resource_group","resource_name":"example-alper-2","resource_key":null},"action":"create"},"type":"planned_change"}
      {"@level":"info","@message":"Plan: 1 to add, 0 to change, 0 to destroy.","@module":"terraform.ui","@timestamp":"2021-11-15T17:05:15.213823+03:00","changes":{"add":1,"change":0,"remove":0,"operation":"plan"},"type":"change_summary"}
      {"@level":"info","@message":"azurerm_resource_group.example-alper-2: Creating...","@module":"terraform.ui","@timestamp":"2021-11-15T17:05:24.376958+03:00","hook":{"resource":{"addr":"azurerm_resource_group.example-alper-2","module":"","resource":"azurerm_resource_group.example-alper-2","implied_provider":"azurerm","resource_type":"azurerm_resource_group","resource_name":"example-alper-2","resource_key":null},"action":"create"},"type":"apply_start"}
      {"@level":"info","@message":"azurerm_resource_group.example-alper-2: Creation errored after 1s","@module":"terraform.ui","@timestamp":"2021-11-15T17:05:24.985199+03:00","hook":{"resource":{"addr":"azurerm_resource_group.example-alper-2","module":"","resource":"azurerm_resource_group.example-alper-2","implied_provider":"azurerm","resource_type":"azurerm_resource_group","resource_name":"example-alper-2","resource_key":null},"action":"create","elapsed_seconds":1},"type":"apply_errored"}
      {"@level":"error","@message":"Error: A resource with the ID \"/subscriptions/.../resourceGroups/example-resourcegroup-alper\" already exists - to be managed via Terraform this resource needs to be imported into the State. Please see the resource documentation for \"azurerm_resource_group\" for more information.","@module":"terraform.ui","@timestamp":"2021-11-15T17:05:25.033488+03:00","diagnostic":{"severity":"error","summary":"A resource with the ID \"/subscriptions/.../resourceGroups/example-resourcegroup-alper\" already exists - to be managed via Terraform this resource needs to be imported into the State. Please see the resource documentation for \"azurerm_resource_group\" for more information.","detail":"","address":"azurerm_resource_group.example-alper-2","range":{"filename":"main.tf.json","start":{"line":1,"column":228,"byte":227},"end":{"line":1,"column":229,"byte":228}},"snippet":{"context":"resource.azurerm_resource_group.example-alper-2.tags","code":"{\"provider\":{\"azurerm\":{\"features\":{}}},\"resource\":{\"azurerm_resource_group\":{\"example-alper-2\":{\"lifecycle\":{\"prevent_destroy\":true},\"location\":\"East US\",\"name\":\"example-resourcegroup-alper\",\"tags\":{\"provisioner\":\"crossplane\"}}}},\"terraform\":{\"required_providers\":{\"azurerm\":{\"source\":\"hashicorp/azurerm\",\"version\":\"2.78.0\"}}}}","start_line":1,"highlight_start_offset":227,"highlight_end_offset":228,"values":[]}},"type":"diagnostic"}
      : exit status 1
    reason: ReconcileError
    status: "False"
    type: Synced

With the proposed changes:

status:
  atProvider: {}
  conditions:
  - lastTransitionTime: "2021-11-15T14:05:25Z"
    reason: Creating
    status: "False"
    type: Ready
  - lastTransitionTime: "2021-11-15T14:22:13Z"
    message: 'create failed: cannot apply: apply failed: A resource with the ID "/subscriptions/.../resourceGroups/example-resourcegroup-alper"
      already exists - to be managed via Terraform this resource needs to be imported
      into the State. Please see the resource documentation for "azurerm_resource_group"
      for more information.: '
    reason: ReconcileError
    status: "False"
    type: Synced

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

muvaf

Thanks @ulucinar ! My general comment is that there are only a limited set of errors, shapes and even error content that we expect this library to be used for and I believe we should take advantage of this as much as possible by reducing the logic to handle only those instead of making it generic for future possibilities. Both approaches are valid but if we start with the smallest possible set, we won't have to pay the cost of generic-ness until we actually need it.

pkg/terraform/errors/errors.go

muvaf · 2021-11-15T15:33:59Z

pkg/terraform/errors/errors.go

+	tfLogs := make([]*TerraformLog, 0, len(logLines))
+	for _, l := range logLines {
+		log := &TerraformLog{}
+		l := strings.TrimSpace(l)


Is there any metadata information we can use to filter out the information from HCL? In the past, we were seeing the creds printed in errors and removed them from HCL but if there is a way to not include that at this level, I think we can remove Env []string functionality we added back then to simplify things.

I would propose to see if we hit that situation again. Previously, we were supplying the provider credentials via the Terraform configuration file (main.hcl) which had caused that issue. Do we know any other cases which could cause a similar issue. If so, I would suggest investigating it in a separate issue maybe.

Previously, we were supplying the provider credentials via the Terraform configuration file (main.hcl) which had caused that issue.

I vaguely recall that the error was printed as one type of JSON and then HCL was printed in another JSON with different classification. What I'm proposing is that if we're able to differentiate programmatically by checking the properties of those JSONs, it'd be great to do it so that we revert to writing credentials to HCL to simplify that part of the codebase. If all was in a single error message that we can't differentiate, then there isn't much we can do.

I believe env solution fixing more than just error messages and we should not revert it: #99 (comment)

Opened #151 to track credential consumption discussion. @muvaf @turkenh, extended the logs with the file name (summary and detail were already there) as we have discussed offline.

Thank you @ulucinar !

…ors, respectively - Do not defer error messages extraction to Error() Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

muvaf

Thanks @ulucinar !

muvaf · 2021-11-18T13:12:32Z

pkg/terraform/errors/errors.go

 }

-func (a *applyFailed) Error() string {
-	return a.log
+func newTFError(message string, logs []byte) (string, *tfError) {


nitpitck

Suggested change

func newTFError(message string, logs []byte) (string, *tfError) {

func newTFError(message string, logs []byte) (*tfError, error) {

Because tfError is shared (embedded) inside actual error types and because we need to consume the returned error instance as a tfError, doing this will necessitate type assertions at those sites. I'd prefer to keep it as it's as it's not part of the public interface.

muvaf · 2021-11-18T13:13:14Z

pkg/terraform/errors/errors.go

 }

 // IsApplyFailed returns whether error is due to failure of an apply operation.
 func IsApplyFailed(err error) bool {
-	_, ok := err.(*applyFailed)
-	return ok
+	r := &applyFailed{}


nit: now it's possible to get away with simple type casting.

This I also considered. However, in case these errors are themselves wrapped, then the current implementations are more robust. In my opinion, as a principle, we had better always implement similar methods with wrapping in mind.

Extract Terraform error messages from JSON logs

c6e11fd

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

ulucinar requested review from turkenh and muvaf November 15, 2021 14:32

muvaf reviewed Nov 15, 2021

View reviewed changes

Add errors.New{Refresh,Plan}Failed for dedicated refresh and plan err…

0e3cd3d

…ors, respectively - Do not defer error messages extraction to Error() Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

ulucinar force-pushed the improve-errors branch from d7cdee9 to 0e3cd3d Compare November 17, 2021 23:10

ulucinar mentioned this pull request Nov 18, 2021

Reconsider How Provider Credentials are Consumed in Terraform-based Providers #151

Open

Include an error message's associated file name in the logs

3cb8d45

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

muvaf approved these changes Nov 18, 2021

View reviewed changes

ulucinar merged commit 9e514cd into crossplane:main Nov 18, 2021

ulucinar deleted the improve-errors branch November 18, 2021 14:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract Terraform error messages from JSON logs #143

Extract Terraform error messages from JSON logs #143

ulucinar commented Nov 15, 2021

muvaf left a comment

muvaf Nov 15, 2021

ulucinar Nov 15, 2021

muvaf Nov 16, 2021

turkenh Nov 18, 2021

ulucinar Nov 18, 2021

muvaf Nov 18, 2021

muvaf left a comment

muvaf Nov 18, 2021

ulucinar Nov 18, 2021 •

edited

Loading

muvaf Nov 18, 2021

ulucinar Nov 18, 2021

	func newTFError(message string, logs []byte) (string, *tfError) {
	func newTFError(message string, logs []byte) (*tfError, error) {

Extract Terraform error messages from JSON logs #143

Extract Terraform error messages from JSON logs #143

Conversation

ulucinar commented Nov 15, 2021

Description of your changes

How has this code been tested

muvaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muvaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulucinar Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulucinar Nov 18, 2021 •

edited

Loading