Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Extract Terraform error messages from JSON logs #143

Merged
merged 3 commits into from
Nov 18, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions pkg/controller/api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,15 @@ import (
"context"
"testing"

xpresource "github.com/crossplane/crossplane-runtime/pkg/resource"
xpfake "github.com/crossplane/crossplane-runtime/pkg/resource/fake"
"github.com/crossplane/crossplane-runtime/pkg/test"
"github.com/google/go-cmp/cmp"
"github.com/pkg/errors"
"sigs.k8s.io/controller-runtime/pkg/client"
ctrl "sigs.k8s.io/controller-runtime/pkg/manager"

xpresource "github.com/crossplane/crossplane-runtime/pkg/resource"
xpfake "github.com/crossplane/crossplane-runtime/pkg/resource/fake"
"github.com/crossplane/crossplane-runtime/pkg/test"

"github.com/crossplane-contrib/terrajet/pkg/resource"
"github.com/crossplane-contrib/terrajet/pkg/resource/fake"
tjerrors "github.com/crossplane-contrib/terrajet/pkg/terraform/errors"
Expand Down Expand Up @@ -56,15 +57,15 @@ func TestAPICallbacks_Apply(t *testing.T) {
MockGet: test.NewMockGetFn(nil),
MockStatusUpdate: func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error {
got := obj.(resource.Terraformed).GetCondition(resource.TypeAsyncOperation)
if diff := cmp.Diff(resource.AsyncOperationCondition(tjerrors.NewApplyFailed(errBoom.Error())), got); diff != "" {
if diff := cmp.Diff(resource.AsyncOperationCondition(tjerrors.NewApplyFailed(nil)), got); diff != "" {
t.Errorf("\nApply(...): -want error, +got error:\n%s", diff)
}
return nil
},
},
Scheme: xpfake.SchemeWith(&fake.Terraformed{}),
},
err: tjerrors.NewApplyFailed(errBoom.Error()),
err: tjerrors.NewApplyFailed(nil),
},
},
"ApplyOperationSucceeded": {
Expand Down Expand Up @@ -138,15 +139,15 @@ func TestAPICallbacks_Destroy(t *testing.T) {
MockGet: test.NewMockGetFn(nil),
MockStatusUpdate: func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error {
got := obj.(resource.Terraformed).GetCondition(resource.TypeAsyncOperation)
if diff := cmp.Diff(resource.AsyncOperationCondition(tjerrors.NewDestroyFailed(errBoom.Error())), got); diff != "" {
if diff := cmp.Diff(resource.AsyncOperationCondition(tjerrors.NewDestroyFailed(nil)), got); diff != "" {
t.Errorf("\nApply(...): -want error, +got error:\n%s", diff)
}
return nil
},
},
Scheme: xpfake.SchemeWith(&fake.Terraformed{}),
},
err: tjerrors.NewDestroyFailed(errBoom.Error()),
err: tjerrors.NewDestroyFailed(nil),
},
},
"DestroyOperationSucceeded": {
Expand Down
161 changes: 145 additions & 16 deletions pkg/terraform/errors/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,40 +16,169 @@ limitations under the License.

package errors

import (
"fmt"
"strings"

jsoniter "github.com/json-iterator/go"
"github.com/pkg/errors"
)

const (
levelError = "error"
)

type tfError struct {
message string
}

type applyFailed struct {
log string
*tfError
}

// TerraformLog represents relevant fields of a Terraform CLI JSON-formatted log line
type TerraformLog struct {
Level string `json:"@level"`
Message string `json:"@message"`
Diagnostic LogDiagnostic `json:"diagnostic"`
}

// LogDiagnostic represents relevant fields of a Terraform CLI JSON-formatted
// log line diagnostic info
type LogDiagnostic struct {
Severity string `json:"severity"`
Summary string `json:"summary"`
Detail string `json:"detail"`
Range Range `json:"range"`
}

// Range represents a line range in a Terraform workspace file
type Range struct {
FileName string `json:"filename"`
}

func (t *tfError) Error() string {
return t.message
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpitck

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

Copy link
Collaborator Author

@ulucinar ulucinar Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

tfError := &tfError{
message: message,
}

tfLogs, err := parseTerraformLogs(logs)
if err != nil {
return err.Error(), tfError
}

messages := make([]string, 0, len(tfLogs))
for _, l := range tfLogs {
// only use error logs
if l == nil || l.Level != levelError {
continue
}
m := l.Message
if l.Diagnostic.Severity == levelError && l.Diagnostic.Summary != "" {
ulucinar marked this conversation as resolved.
Show resolved Hide resolved
m = fmt.Sprintf("%s: %s", l.Diagnostic.Summary, l.Diagnostic.Detail)
if len(l.Diagnostic.Range.FileName) != 0 {
m = m + ": File name: " + l.Diagnostic.Range.FileName
}
}
messages = append(messages, m)
}
tfError.message = fmt.Sprintf("%s: %s", message, strings.Join(messages, "\n"))
return "", tfError
}

func parseTerraformLogs(logs []byte) ([]*TerraformLog, error) {
logLines := strings.Split(string(logs), "\n")
tfLogs := make([]*TerraformLog, 0, len(logLines))
for _, l := range logLines {
log := &TerraformLog{}
l := strings.TrimSpace(l)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @ulucinar !

if l == "" {
continue
}
if err := jsoniter.ConfigCompatibleWithStandardLibrary.UnmarshalFromString(l, log); err != nil {
return nil, err
}
tfLogs = append(tfLogs, log)
}
return tfLogs, nil
}

// NewApplyFailed returns a new apply failure error with given logs.
func NewApplyFailed(log string) error {
return &applyFailed{log: log}
func NewApplyFailed(logs []byte) error {
parseError, tfError := newTFError("apply failed", logs)
result := &applyFailed{tfError: tfError}
if parseError == "" {
return result
}
return errors.WithMessage(result, parseError)
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return errors.As(err, &r)
}

type destroyFailed struct {
log string
}

func (a *destroyFailed) Error() string {
return a.log
*tfError
}

// NewDestroyFailed returns a new destroy failure error with given logs.
func NewDestroyFailed(log string) error {
return &destroyFailed{log: log}
func NewDestroyFailed(logs []byte) error {
parseError, tfError := newTFError("destroy failed", logs)
result := &destroyFailed{tfError: tfError}
if parseError == "" {
return result
}
return errors.WithMessage(result, parseError)
}

// IsDestroyFailed returns whether error is due to failure of a destroy operation.
func IsDestroyFailed(err error) bool {
_, ok := err.(*destroyFailed)
return ok
r := &destroyFailed{}
return errors.As(err, &r)
}

type refreshFailed struct {
*tfError
}

// NewRefreshFailed returns a new destroy failure error with given logs.
func NewRefreshFailed(logs []byte) error {
parseError, tfError := newTFError("refresh failed", logs)
result := &refreshFailed{tfError: tfError}
if parseError == "" {
return result
}
return errors.WithMessage(result, parseError)
}

// IsRefreshFailed returns whether error is due to failure of a destroy operation.
func IsRefreshFailed(err error) bool {
r := &refreshFailed{}
return errors.As(err, &r)
}

type planFailed struct {
*tfError
}

// NewPlanFailed returns a new destroy failure error with given logs.
func NewPlanFailed(logs []byte) error {
parseError, tfError := newTFError("plan failed", logs)
result := &planFailed{tfError: tfError}
if parseError == "" {
return result
}
return errors.WithMessage(result, parseError)
}

// IsPlanFailed returns whether error is due to failure of a destroy operation.
func IsPlanFailed(err error) bool {
r := &planFailed{}
return errors.As(err, &r)
}
Loading