Refactor enableTerraformCLILogging flag, Fix time formattting for log…

…ging

Signed-off-by: Suresh Ramasamy <[email protected]>

apis/v1beta1/workspace_types.go

@@ -159,7 +159,7 @@ type WorkspaceObservation struct {
 type WorkspaceSpec struct {
 	xpv1.ResourceSpec `json:",inline"`
 	ForProvider       WorkspaceParameters `json:"forProvider"`
-	EnableLogging     bool                `json:"enableLogging,omitempty"`
+	EnableTerraformCLILogging     bool                `json:"enableTerraformCLILogging,omitempty"`
 }
 
 // A WorkspaceStatus represents the observed state of a Workspace.

cmd/provider/main.go

@@ -18,6 +18,7 @@ package main
 
 import (
 	"context"
+	zapuber "go.uber.org/zap"
 	"os"
 	"path/filepath"
 	"time"
@@ -149,19 +150,10 @@ func main() {
 }
 
 func UseJSONencoder() zap.Opts {
-	// Create a zap logger with JSON encoding
-	encoderCfg := zapcore.EncoderConfig{
-		TimeKey:        "time",
-		LevelKey:       "level",
-		NameKey:        "logger",
-		MessageKey:     "message",
-		StacktraceKey:  "stacktrace",
-		LineEnding:     zapcore.DefaultLineEnding,
-		EncodeLevel:    zapcore.LowercaseLevelEncoder,
-		EncodeTime:     zapcore.ISO8601TimeEncoder,
-	}
 	return func(o *zap.Options) {
-		o.Encoder = zapcore.NewJSONEncoder(encoderCfg)
+		encoderConfig := zapuber.NewProductionEncoderConfig()
+		encoderConfig.EncodeTime = zapcore.ISO8601TimeEncoder
+		o.Encoder = zapcore.NewJSONEncoder(encoderConfig)
 	}
 }
 

examples/workspace-enable-logging.yaml

@@ -8,7 +8,7 @@ metadata:
     # annotation it would be derived from metadata.name - e.g. 'example-random-generator.
     crossplane.io/external-name: random
 spec:
-  enableLogging: true
+  enableTerraformCLILogging: true
   forProvider:
     source: Inline
     module: |

internal/controller/workspace/workspace.go

@@ -132,8 +132,8 @@ func Setup(mgr ctrl.Manager, o controller.Options, timeout, pollJitter time.Dura
 		usage:  resource.NewProviderConfigUsageTracker(mgr.GetClient(), &v1beta1.ProviderConfigUsage{}),
 		logger: o.Logger,
 		fs:     fs,
-		terraform: func(dir string, usePluginCache bool, enableLogging bool, logger logging.Logger, envs ...string) tfclient {
-			return terraform.Harness{Path: tfPath, Dir: dir, UsePluginCache: usePluginCache, EnableLogging: enableLogging, Logger: logger, Envs: envs}
+		terraform: func(dir string, usePluginCache bool, enableTerraformCLILogging bool, logger logging.Logger, envs ...string) tfclient {
+			return terraform.Harness{Path: tfPath, Dir: dir, UsePluginCache: usePluginCache, EnableTerraformCLILogging: enableTerraformCLILogging, Logger: logger, Envs: envs}
 		},
 	}
 
@@ -168,7 +168,7 @@ type connector struct {
 	usage     resource.Tracker
 	logger    logging.Logger
 	fs        afero.Afero
-	terraform func(dir string, usePluginCache bool, enableLogging bool, logger logging.Logger, envs ...string) tfclient
+	terraform func(dir string, usePluginCache bool, enableTerraformCLILogging bool, logger logging.Logger, envs ...string) tfclient
 }
 
 func (c *connector) Connect(ctx context.Context, mg resource.Managed) (managed.ExternalClient, error) { //nolint:gocyclo
@@ -320,7 +320,7 @@ func (c *connector) Connect(ctx context.Context, mg resource.Managed) (managed.E
 		envs[idx] = strings.Join([]string{env.Name, runtimeVal}, "=")
 	}
 
-	tf := c.terraform(dir, *pc.Spec.PluginCache, cr.Spec.EnableLogging, l, envs...)
+	tf := c.terraform(dir, *pc.Spec.PluginCache, cr.Spec.EnableTerraformCLILogging, l, envs...)
 	if cr.Status.AtProvider.Checksum != "" {
 		checksum, err := tf.GenerateChecksum(ctx)
 		if err != nil {

internal/controller/workspace/workspace_test.go

@@ -124,7 +124,7 @@ func TestConnect(t *testing.T) {
 		kube      client.Client
 		usage     resource.Tracker
 		fs        afero.Afero
-		terraform func(dir string, usePluginCache bool, envs ...string) tfclient
+		terraform func(dir string, usePluginCache bool, enableTerraformCLILogging bool, logger logging.Logger, envs ...string) tfclient
 	}
 
 	type args struct {
@@ -216,7 +216,7 @@ func TestConnect(t *testing.T) {
 				},
 				usage: resource.TrackerFn(func(_ context.Context, _ resource.Managed) error { return nil }),
 				fs:    afero.Afero{Fs: afero.NewMemMapFs()},
-				terraform: func(_ string, _ bool, _ ...string) tfclient {
+				terraform: func(_ string, _ bool, _ bool, _ logging.Logger, _ ...string) tfclient {
 					return &MockTf{
 						MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil },
 					}
@@ -255,7 +255,7 @@ func TestConnect(t *testing.T) {
 						errs: map[string]error{filepath.Join(tfDir, string(uid), tfCreds): errBoom},
 					},
 				},
-				terraform: func(_ string, _ bool, _ ...string) tfclient {
+				terraform: func(_ string, _ bool, _ bool, _ logging.Logger, _ ...string) tfclient {
 					return &MockTf{
 						MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil },
 					}
@@ -294,7 +294,7 @@ func TestConnect(t *testing.T) {
 						errs: map[string]error{filepath.Join(tfDir, string(uid), "subdir", tfCreds): errBoom},
 					},
 				},
-				terraform: func(_ string, _ bool, _ ...string) tfclient {
+				terraform: func(_ string, _ bool, _ bool, _ logging.Logger, _ ...string) tfclient {
 					return &MockTf{
 						MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil },
 					}
@@ -338,7 +338,7 @@ func TestConnect(t *testing.T) {
 						errs: map[string]error{filepath.Join("/tmp", tfDir, string(uid), ".git-credentials"): errBoom},
 					},
 				},
-				terraform: func(_ string, _ bool, _ ...string) tfclient {
+				terraform: func(_ string, _ bool, _ bool, _ logging.Logger, _ ...string) tfclient {
 					return &MockTf{
 						MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil },
 					}
@@ -381,7 +381,7 @@ func TestConnect(t *testing.T) {
 						errs: map[string]error{filepath.Join("/tmp", tfDir, string(uid)): errBoom},
 					},
 				},
-				terraform: func(_ string, _ bool, _ ...string) tfclient {
+				terraform: func(_ string, _ bool, _ bool, _ logging.Logger, _ ...string) tfclient {
 					return &MockTf{
 						MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil },
 					}
@@ -422,7 +422,7 @@ func TestConnect(t *testing.T) {
 						errs: map[string]error{filepath.Join(tfDir, string(uid), tfConfig): errBoom},
 					},
 				},
-				terraform: func(_ string, _ bool, _ ...string) tfclient {
+				terraform: func(_ string, _ bool, _ bool, _ logging.Logger, _ ...string) tfclient {
 					return &MockTf{
 						MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil },
 					}
@@ -463,7 +463,7 @@ func TestConnect(t *testing.T) {
 						errs: map[string]error{filepath.Join(tfDir, string(uid), "subdir", tfConfig): errBoom},
 					},
 				},
-				terraform: func(_ string, _ bool, _ ...string) tfclient {
+				terraform: func(_ string, _ bool, _ bool, _ logging.Logger, _ ...string) tfclient {
 					return &MockTf{
 						MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil },
 					}
@@ -499,7 +499,7 @@ func TestConnect(t *testing.T) {
 						errs: map[string]error{filepath.Join(tfDir, string(uid), tfMain): errBoom},
 					},
 				},
-				terraform: func(_ string, _ bool, _ ...string) tfclient {
+				terraform: func(_ string, _ bool, _ bool, _ logging.Logger, _ ...string) tfclient {
 					return &MockTf{
 						MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil },
 					}
@@ -529,7 +529,7 @@ func TestConnect(t *testing.T) {
 				},
 				usage: resource.TrackerFn(func(_ context.Context, _ resource.Managed) error { return nil }),
 				fs:    afero.Afero{Fs: afero.NewMemMapFs()},
-				terraform: func(_ string, _ bool, _ ...string) tfclient {
+				terraform: func(_ string, _ bool, _ bool, _ logging.Logger, _ ...string) tfclient {
 					return &MockTf{MockInit: func(_ context.Context, _ ...terraform.InitOption) error { return errBoom }}
 				},
 			},
@@ -553,7 +553,7 @@ func TestConnect(t *testing.T) {
 				},
 				usage: resource.TrackerFn(func(_ context.Context, _ resource.Managed) error { return nil }),
 				fs:    afero.Afero{Fs: afero.NewMemMapFs()},
-				terraform: func(_ string, _ bool, _ ...string) tfclient {
+				terraform: func(_ string, _ bool, _ bool, _ logging.Logger, _ ...string) tfclient {
 					return &MockTf{
 						MockInit:      func(ctx context.Context, o ...terraform.InitOption) error { return nil },
 						MockWorkspace: func(_ context.Context, _ string) error { return errBoom },
@@ -579,7 +579,7 @@ func TestConnect(t *testing.T) {
 			},
 				usage: resource.TrackerFn(func(_ context.Context, _ resource.Managed) error { return nil }),
 				fs:    afero.Afero{Fs: afero.NewMemMapFs()},
-				terraform: func(_ string, _ bool, _ ...string) tfclient {
+				terraform: func(_ string, _ bool, _ bool, _ logging.Logger, _ ...string) tfclient {
 					return &MockTf{
 						MockGenerateChecksum: func(ctx context.Context) (string, error) { return "", errBoom },
 					}
@@ -613,7 +613,7 @@ func TestConnect(t *testing.T) {
 			},
 				usage: resource.TrackerFn(func(_ context.Context, _ resource.Managed) error { return nil }),
 				fs:    afero.Afero{Fs: afero.NewMemMapFs()},
-				terraform: func(_ string, _ bool, _ ...string) tfclient {
+				terraform: func(_ string, _ bool, _ bool, _ logging.Logger, _ ...string) tfclient {
 					return &MockTf{
 						MockGenerateChecksum: func(ctx context.Context) (string, error) { return tfChecksum, nil },
 						MockWorkspace:        func(_ context.Context, _ string) error { return nil },
@@ -649,7 +649,7 @@ func TestConnect(t *testing.T) {
 				},
 				usage: resource.TrackerFn(func(_ context.Context, _ resource.Managed) error { return nil }),
 				fs:    afero.Afero{Fs: afero.NewMemMapFs()},
-				terraform: func(_ string, _ bool, _ ...string) tfclient {
+				terraform: func(_ string, _ bool, _ bool, _ logging.Logger, _ ...string) tfclient {
 					return &MockTf{
 						MockInit:             func(ctx context.Context, o ...terraform.InitOption) error { return nil },
 						MockGenerateChecksum: func(ctx context.Context) (string, error) { return tfChecksum, nil },
@@ -688,7 +688,7 @@ func TestConnect(t *testing.T) {
 				},
 				usage: resource.TrackerFn(func(_ context.Context, _ resource.Managed) error { return nil }),
 				fs:    afero.Afero{Fs: afero.NewMemMapFs()},
-				terraform: func(_ string, _ bool, _ ...string) tfclient {
+				terraform: func(_ string, _ bool, _ bool, _ logging.Logger, _ ...string) tfclient {
 					return &MockTf{
 						MockInit: func(ctx context.Context, o ...terraform.InitOption) error {
 							args := terraform.InitArgsToString(o)

internal/terraform/terraform.go

@@ -125,8 +125,8 @@ type Harness struct {
 	// Whether to use the terraform plugin cache
 	UsePluginCache bool
 
-	// Whether to enable logging to container stdout
-	EnableLogging bool
+	// Whether to enable writing Terraform CLI logs to container stdout
+	EnableTerraformCLILogging bool
 
 	// Logger
 	Logger logging.Logger
@@ -570,26 +570,18 @@ func (h Harness) Diff(ctx context.Context, o ...Option) (bool, error) {
 	case 1: 
 		ee := &exec.ExitError{}
 		errors.As(err, &ee)
-		if err = h.writeLogs(ee.Stderr, h.EnableLogging ); err != nil {
-			return false, errors.Wrap(err, errWriteLogs)
+		if h.EnableTerraformCLILogging{
+			h.Logger.Info(string(ee.Stderr))
 		}
 	case 2:
-		if err = h.writeLogs(log, h.EnableLogging); err != nil {
-			return false, errors.Wrap(err, errWriteLogs)
+		if h.EnableTerraformCLILogging{
+			h.Logger.Info(string(log))
 		}
 		return true, nil
 	}
 	return false, Classify(err)
 }
 
-func (h Harness) writeLogs(out []byte, enableLogging bool) error {
-	if !enableLogging {
-		return nil
-	}
-	h.Logger.Debug(string(out))
-	return nil
-}
-
 // Apply a Terraform configuration.
 func (h Harness) Apply(ctx context.Context, o ...Option) error {
 	ao := &options{}
@@ -615,7 +607,23 @@ func (h Harness) Apply(ctx context.Context, o ...Option) error {
 		defer rwmutex.RUnlock()
 	}
 
-	_, err := runCommand(ctx, cmd)
+	// In case of terraform apply
+	// 0 - Succeeded
+	// Non Zero output - Errored
+
+	log, err := runCommand(ctx, cmd)
+	switch cmd.ProcessState.ExitCode() {
+	case 0:
+		if h.EnableTerraformCLILogging{
+			h.Logger.Info(string(log))
+		}
+	default:
+		ee := &exec.ExitError{}
+		errors.As(err, &ee)
+		if h.EnableTerraformCLILogging{
+			h.Logger.Info(string(ee.Stderr))
+		}
+	}
 	return Classify(err)
 }
 
@@ -644,7 +652,23 @@ func (h Harness) Destroy(ctx context.Context, o ...Option) error {
 		defer rwmutex.RUnlock()
 	}
 
-	_, err := runCommand(ctx, cmd)
+	log, err := runCommand(ctx, cmd)
+
+	// In case of terraform destroy
+	// 0 - Succeeded
+	// Non Zero output - Errored
+	switch cmd.ProcessState.ExitCode() {
+	case 0:
+		if h.EnableTerraformCLILogging{
+			h.Logger.Info(string(log))
+		}
+	default:
+		ee := &exec.ExitError{}
+		errors.As(err, &ee)
+		if h.EnableTerraformCLILogging{
+			h.Logger.Info(string(ee.Stderr))
+		}
+	}
 	return Classify(err)
 }
 

package/crds/tf.upbound.io_workspaces.yaml

@@ -61,7 +61,7 @@ spec:
                 - Orphan
                 - Delete
                 type: string
-              enableLogging:
+              enableTerraformCLILogging:
                 type: boolean
               forProvider:
                 description: WorkspaceParameters are the configurable fields of a