Refactor backend handling #828

johananl · 2020-08-21T20:49:44Z

This PR simplifies the Backend interface and cleans up the implementations.

johananl · 2020-08-21T22:29:13Z

Make backend.s3.region required in the docs.

invidian

Hmm, I don't fully agree with the changes done in this PR. Please see my comments with explanation.

pkg/backend/s3/template.go

pkg/backend/s3/s3.go

invidian · 2020-08-23T15:43:53Z

pkg/backend/local/template.go

@@ -14,10 +14,10 @@

 package local

-var backendConfigTmpl = `


We could move the template into main file, as it's not very big, that would make reading easier.

Yes, I was thinking about this, too. Didn't do it in the end to try and keep things simple. Frankly I think the whole backend abstraction might be an overkill and seems to have been copy-pasted from the platform abstraction. I'll take another look at the whole thing and see what I can improve.

Actually in the current state including the backend {} block in cluster.tf would mean duplicating the block in every platform 😕

invidian · 2020-08-23T15:45:10Z

pkg/backend/local/local.go

-	backend.Register("local", NewLocalBackend())
-}
+// NewConfig creates a new Config and returns a pointer to it as well as any HCL diagnostics.
+func NewConfig(b *hcl.Body, ctx *hcl.EvalContext) (*Config, hcl.Diagnostics) {


Maybe FromHCL would be a better name for this function? So it reads pkg/backend/s3.FromHCL(...) ?

Actually, I'd get rid of this code completely and do the reading only once somewhere, as reading is always exactly the same regardless of the backend type.

That's what we had with LoadConfig(), however I don't think this is a good approach - see #828 (comment).

invidian · 2020-08-23T15:49:21Z

pkg/backend/local/local.go

-func (l *local) Validate() error {
-	return nil
+// NewBackend constructs a Backend based on the provided config and returns a pointer to it.
+func NewBackend(c *Config) (*Backend, error) {


It seems moving error handling from Render method to NewBackend, which is not part of the interface makes us do extra work when constructing the backend for each supported backend type. This seems like a step backwards to me.

I don't think this is extra work, you just do the error handling earlier. I don't see why we would want to allow creating a backend with a wrong config where you'll learn about the problem only when trying to render it.

pkg/backend/local/local.go

invidian · 2020-08-23T15:54:05Z

pkg/backend/local/local.go

 }

-func NewLocalBackend() *local {
-	return &local{}
+// Backend implements the Backend interface for a local backend.


I'm not sure if it make sense to have another intermediate struct here. Is the idea to use NewBackend as a gateway for the interface, to make sure the data is valid? If so, the type should be unexported.

invidian · 2020-08-23T15:55:49Z

cli/cmd/cluster.go

+		bc, diags := local.NewConfig(&config.RootConfig.Backend.Config, config.EvalContext)
+		if diags.HasErrors() {
+			for _, diagnostic := range diags {
+				logger.Error(diagnostic.Error())
+			}
+
+			logger.Fatal("Errors found while loading backend configuration")
+		}
+
+		b, err := local.NewBackend(bc)
+		if err != nil {
+			logger.Fatalf("Error constructing backend: %v", err)
+		}
+
+		return b


The only unique code per backend is the underlying configuration struct. I suggest we focus to only select this unique information rather than re-doing the same thing for each backend.

I suggest we focus to only select this unique information rather than re-doing the same thing for each backend.

What does this mean in practice? I don't understand your suggestion.

I suggest we only select the right structure to valuate the config into, rather than re-doing error checking and creating the backend. So we have one line per backend type, not many.

I understand. The problem is that I wanted to do all the "work" at creation time, i.e. unmarshaling, validation etc. It doesn't make sense to me to return an uninitialized, unvalidated struct and hope that the user calls all the relevant methods on the Backend interface.

AFAICT, to do what you're suggesting without exposing things like LoadConfig() and Validate() on the Backend interface we would have to do everything inside NewBackend(), which means we would have to return "regular" Go errors inside hcl.Diagnostics 😖

That's why I've chosen to separate the HCL unmarshaling from things like custom validations and rendering the template, and that's why we have a Config separately from the Backend, and that's why we have to explicitly create a Config and a Backend for each backend type.

I'm wondering if we should stop using hcl.Diagnostics altogether. Personally I don't like this mechanism at all and would rather deal with "regular" errors myself.

What concrete type does c.NewBackend() return?

Also, what you're proposing kind of breaks encapsulation: now the caller needs to deal with gohcl.DecodeBody() which is an implementation detail of the backend AFAICT.

And lastly, I find it weird that the backend config struct constructs the backend. AFAICT it is a much more common pattern to first create a config struct for X, then pass the config struct to an X constructor to construct an X struct.

I would focus on having an abstraction which makes sense first, and worry about the duplication second. I feel like we're trying to build an unnatural abstraction just to avoid a few similar lines of code in the switch.

What concrete type does c.NewBackend() return?

It should return Backend interface, as in current PR (or specific type implementing backend interface).

Also, what you're proposing kind of breaks encapsulation: now the caller needs to deal with gohcl.DecodeBody() which is an implementation detail of the backend AFAICT.

This is not the implementation detail of the backend. This is the UI part of the lokoctl. The backend should have no knowledge about HCL (maybe except struct tags).

It should return Backend interface, as in current PR (or specific type implementing backend interface).

I'm asking what concrete type (as opposed to an interface) you expect NewBackend() to return. I understand there is backend.Backend as the return type in the function's signature but you need to return a struct in the end.

This is not the implementation detail of the backend. This is the UI part of the lokoctl. The backend should have no knowledge about HCL (maybe except struct tags).

My point is that it doesn't make sense to me to move gohcl.DecodeBody() to the UI. Does it make sense to you? Why?

My point is that it doesn't make sense to me to move gohcl.DecodeBody() to the UI. Does it make sense to you? Why?

In my opinion, we should isolate entire HCL code into one place, so it's only job is to convert HCL files into the Go struct, so we can operate on Go structs from there. This way, things are easier to test as test do not require HCL parser.

How about something like the following patch? Though still, in my opinion, having render method on config struct and return an error would even simplify it more.

From aeb00d3296deacf753ad5a6a33e64301ff51a47c Mon Sep 17 00:00:00 2001 From: Mateusz Gozdek <mateusz@kinvolk.io> Date: Tue, 25 Aug 2020 18:31:15 +0200 Subject: [PATCH] patch Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io> --- cli/cmd/cluster.go | 44 +++++++++++--------------------------- pkg/backend/backend.go | 5 +++++ pkg/backend/local/local.go | 14 ++++++------ pkg/backend/s3/s3.go | 12 ++++++----- 4 files changed, 33 insertions(+), 42 deletions(-) diff --git a/cli/cmd/cluster.go b/cli/cmd/cluster.go index dd5d191c..9306905a 100644 --- a/cli/cmd/cluster.go +++ b/cli/cmd/cluster.go @@ -17,6 +17,7 @@ package cmd import ( "fmt" + "github.com/hashicorp/hcl/v2/gohcl" "github.com/mitchellh/go-homedir" "github.com/sirupsen/logrus" "github.com/spf13/cobra" @@ -85,44 +86,25 @@ func initialize(ctxLogger *logrus.Entry) (*terraform.Executor, platform.Platform func createBackend(logger *logrus.Entry, config *config.Config) backend.Backend { bn := config.RootConfig.Backend.Name + var backendConfig backend.Config + switch bn { case backend.Local: - bc, diags := local.NewConfig(&config.RootConfig.Backend.Config, config.EvalContext) - if diags.HasErrors() { - for _, diagnostic := range diags { - logger.Error(diagnostic.Error()) - } - - logger.Fatal("Errors found while loading backend configuration") - } - - b, err := local.NewBackend(bc) - if err != nil { - logger.Fatalf("Error constructing backend: %v", err) - } - - return b + backendConfig = &local.Config{} case backend.S3: - bc, diags := s3.NewConfig(&config.RootConfig.Backend.Config, config.EvalContext) - if diags.HasErrors() { - for _, diagnostic := range diags { - logger.Error(diagnostic.Error()) - } - - logger.Fatal("Errors found while loading backend configuration") - } - - b, err := s3.NewBackend(bc) - if err != nil { - logger.Fatalf("Error constructing backend: %v", err) - } + backendConfig = &s3.Config{} + } - return b + if d := gohcl.DecodeBody(config.RootConfig.Backend.Config, config.EvalContext, backendConfig); d.HasErrors() { + logger.Fatalf("bad backend") } - logger.Fatalf("Unknown backend %q", bn) + b, err := backendConfig.New() + if err != nil { + logger.Fatalf("bad backend") + } - return nil + return b } // initializeTerraform initialized Terraform directory using given backend and platform diff --git a/pkg/backend/backend.go b/pkg/backend/backend.go index 7136ae6b..f21e31da 100644 --- a/pkg/backend/backend.go +++ b/pkg/backend/backend.go @@ -26,3 +26,8 @@ type Backend interface { // String returns the Backend's configuration as rendered Terraform code. String() string } + +// Config describes functionality of backend configuration. +type Config interface { + New() (Backend, error) +} diff --git a/pkg/backend/local/local.go b/pkg/backend/local/local.go index 4af828a2..bb472926 100644 --- a/pkg/backend/local/local.go +++ b/pkg/backend/local/local.go @@ -19,7 +19,9 @@ import ( "github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2/gohcl" + "github.com/kinvolk/lokomotive/internal/template" + "github.com/kinvolk/lokomotive/pkg/backend" ) // Config represents the configuration of a local backend. @@ -42,23 +44,23 @@ func NewConfig(b *hcl.Body, ctx *hcl.EvalContext) (*Config, hcl.Diagnostics) { return c, hcl.Diagnostics{} } -// Backend implements the Backend interface for a local backend. -type Backend struct { +// backend implements the Backend interface for a local backend. +type localBackend struct { config *Config // A string containing the rendered Terraform code of the backend. rendered string } -func (b *Backend) String() string { +func (b *localBackend) String() string { return b.rendered } -// NewBackend constructs a Backend based on the provided config and returns a pointer to it. -func NewBackend(c *Config) (*Backend, error) { +// New constructs a Backend based on the provided config and returns a pointer to it. +func (c *Config) New() (backend.Backend, error) { rendered, err := template.Render(backendConfigTmpl, c) if err != nil { return nil, fmt.Errorf("rendering backend: %v", err) } - return &Backend{config: c, rendered: rendered}, nil + return &localBackend{config: c, rendered: rendered}, nil } diff --git a/pkg/backend/s3/s3.go b/pkg/backend/s3/s3.go index 8ab4b2fd..7b450615 100644 --- a/pkg/backend/s3/s3.go +++ b/pkg/backend/s3/s3.go @@ -19,7 +19,9 @@ import ( "github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2/gohcl" + "github.com/kinvolk/lokomotive/internal/template" + "github.com/kinvolk/lokomotive/pkg/backend" ) // Config represents the configuration of an S3 backend. @@ -67,18 +69,18 @@ func NewConfig(b *hcl.Body, ctx *hcl.EvalContext) (*Config, hcl.Diagnostics) { } // Backend implements the Backend interface for an S3 backend. -type Backend struct { +type s3Backend struct { config *Config // A string containing the rendered Terraform code of the backend. rendered string } -func (b *Backend) String() string { +func (b *s3Backend) String() string { return b.rendered } -// NewBackend constructs a Backend based on the provided config and returns a pointer to it. -func NewBackend(c *Config) (*Backend, error) { +// New constructs a Backend based on the provided config and returns a pointer to it. +func (c *Config) New() (backend.Backend, error) { if err := c.validate(); err != nil { return nil, fmt.Errorf("validating backend config: %w", err) } @@ -88,5 +90,5 @@ func NewBackend(c *Config) (*Backend, error) { return nil, fmt.Errorf("rendering backend: %v", err) } - return &Backend{config: c, rendered: rendered}, nil + return &s3Backend{config: c, rendered: rendered}, nil } -- 2.28.0

invidian

Also, there are tests missing. I think part of the refactoring should be adding tests to make sure new code is easily testable.

- Simplify Backend interface. - Rather than relying on the caller to call LoadConfig, expose new functions for creating a backend configuration and a backend struct. HCL unmarshaling happens inside NewConfig now so that it's impossible to construct an "unloaded" backend. - Replace Render() with String(). The actual rendering happens at backend construction time and the result is stored for retrieval by String(). - Remove Validate() from the interface. Instead, validate the the config at creation time. - Make region a required parameter. Ensuring that a region is specified only when no credentials file is specified doesn't make sense. A region is always required by the S3 Terraform backend. In addition, the AWS_SHARED_CREDENTIALS_FILE and AWS_DEFAULT_REGION env vars aren't documented in the S3 backend reference guide. We might as well just get rid of these. - Get rid of the "registration" pattern. Storing all supported backend types in a global variable doesn't seem useful. Instead, each implementation of the Backend interface can construct a concrete type which implements the interface and return it. - Fix whitespace problems in backend file.

- Integrate the backend config into the root module template. - Remove the Backend interface.

The current whitespace trim markers cause the `locals` block to start on the same line as the closing curly bracket of the previous block.

johananl · 2020-09-04T17:57:33Z

I've made an experimental attempt to move the backend templates into the root module templates. I think that overall this simplifies things, however I still have to check how well this plays with refactoring the platform interface. This change introduces some new duplication which we should hopefully be able to get rid of while refactoring the platform interface.

I'm still checking whether this change makes sense. I've made the changes in a new commit which we can discard if we decide this approach isn't good.

invidian

I've made an experimental attempt to move the backend templates into the root module templates. I think that overall this simplifies things, however I still have to check how well this plays with >refactoring the platform interface. This change introduces some new duplication which we should hopefully be able to get rid of while refactoring the platform interface.

I'm still checking whether this change makes sense. I've made the changes in a new commit which we can discard if we decide this approach isn't good.

Hmm, I don't think those changes go in the right direction to be honest. They bring UI changes, empty interfaces, break the encapsulation of Platform code, add duplication and spread HCL loading even more by expanding function which we want to get rid of 😕

@johananl what is wrong with changes I suggested in #828 (comment) ?

invidian · 2020-09-08T07:50:11Z

pkg/backend/backend.go

-	String() string
+type Backend struct {
+	Type   string
+	Config interface{}


Why empty interface if we know all the types we deal with?

invidian · 2020-09-08T07:52:58Z

pkg/platform/aks/aks.go

@@ -71,6 +73,9 @@ type config struct {
 	WorkerPools []workerPool `hcl:"worker_pool,block"`

 	KubernetesVersion string
+
+	// TODO: Transient change - remove when refactoring platform interface.
+	Backend *backend.Backend


This is a noticeable change to the UI, since the block is being moved inside cluster {} block.

invidian · 2020-09-08T07:53:29Z

pkg/platform/aks/aks.go

-	if configBody == nil {
-		emptyConfig := hcl.EmptyBody()
-		configBody = &emptyConfig
+func (c *config) LoadConfig(cc *lkconfig.Config) hcl.Diagnostics {


As we need to get rid of LoadConfig() completely, I'm not sure why we add more code into it now. Also accepting entire configuration here breaks the encapsulation 😕

johananl requested review from ipochi and mauriciovasquezbernal as code owners August 21, 2020 20:49

johananl requested a review from invidian August 21, 2020 20:51

invidian suggested changes Aug 23, 2020

View reviewed changes

invidian reviewed Aug 23, 2020

View reviewed changes

johananl force-pushed the johananl/refactor-backend branch 4 times, most recently from 8c36fdf to 74751c8 Compare August 24, 2020 17:15

johananl added 3 commits September 4, 2020 19:44

Simplify backend handling

332095c

- Integrate the backend config into the root module template. - Remove the Backend interface.

platform/aks: Fix whitespace in template

025ec88

The current whitespace trim markers cause the `locals` block to start on the same line as the closing curly bracket of the previous block.

johananl force-pushed the johananl/refactor-backend branch from 7fc2a99 to 025ec88 Compare September 4, 2020 17:51

invidian suggested changes Sep 8, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor backend handling #828

Refactor backend handling #828

johananl commented Aug 21, 2020

johananl commented Aug 21, 2020

invidian left a comment

invidian Aug 23, 2020

johananl Aug 24, 2020

johananl Aug 24, 2020

invidian Aug 23, 2020

invidian Aug 23, 2020

johananl Aug 24, 2020

invidian Aug 23, 2020

johananl Aug 24, 2020

invidian Aug 23, 2020

invidian Aug 23, 2020

johananl Aug 24, 2020

invidian Aug 24, 2020

johananl Aug 24, 2020

johananl Aug 24, 2020

johananl Aug 25, 2020

johananl Aug 25, 2020 •

edited

invidian Aug 25, 2020

johananl Aug 25, 2020 •

edited

invidian Aug 25, 2020

invidian left a comment

johananl commented Sep 4, 2020

invidian left a comment

invidian Sep 8, 2020

invidian Sep 8, 2020

invidian Sep 8, 2020

Refactor backend handling #828

Are you sure you want to change the base?

Refactor backend handling #828

Conversation

johananl commented Aug 21, 2020

johananl commented Aug 21, 2020

invidian 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johananl Aug 25, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johananl Aug 25, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

invidian left a comment

Choose a reason for hiding this comment

johananl commented Sep 4, 2020

invidian 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

johananl Aug 25, 2020 •

edited

johananl Aug 25, 2020 •

edited