Skip to content
This repository has been archived by the owner on Mar 24, 2023. It is now read-only.

Commit

Permalink
Fix template panic, refactor builder construction
Browse files Browse the repository at this point in the history
What I Did
---------

There were some code paths that would nil-pointer panic because of a `StaticCtx` or a `ConfigCtx` that
was constructed without a `Logger` field. Specifically [this path](https://github.com/replicatedcom/ship/blob/master/pkg/lifecycle/render/config/config_context.go#L147) and [this one](https://github.com/replicatedcom/ship/blob/master/pkg/templates/static_context.go#L87). Things worked fine when all was well,
but if you had an issue with your usage of `{{repl }}`, referencing an unsupported template function, ship would `panic` when trying to print a warning.

I also added a small sleep before exiting so the ui can bring up the "all done" page before we bail.

How I did it
---------

Did a big refactor of how `Builder` and `ConfigCtx` and `StaticCtx` instances are created. Instead of

```go
builder := templates.NewBuilder(...)
```

or

```go
builder := &templates.Builder{}
```

we now instead use a new `BuilderBuilder` struct to manage creation. A BuilderBuilder has
all the dependencies needed by the underlying `Builder`s and `Ctx`s, and can pass them in
during construction.

So now every object that needs to create or manage a `Builder` gets a BuilderBuilder injected. And should use
something like the following to get a Builder

```go
staticCtx := r.BuilderBuilder.NewStaticContext()

configCtx, _ := r.BuilderBuilder.NewConfigContext(
	configGroups,
	updatedValues,
)

builder := r.BuilderBuilder.NewBuilder(
	r.BuilderBuilder.NewStaticContext(),
	configCtx,
)
```

This should help with `Ctx`s not getting the deps they need in some places.

How to verify it
---------

Write a yaml that uses `{{repl NonExistentFunctionABCDEF }}` to reference a template function that does not exist. Ensure that ship prints an informative error about the template function, instead of just panic-ing as in [this error](https://replicated.slack.com/archives/C9QQD9LHK/p1527283748000194).

Description for the changelog
---------

Fixes a bug in template processing that could cause panics when an unknown or unsupported template function was used.
  • Loading branch information
dexhorthy committed May 26, 2018
1 parent bfc36bc commit 03fe21d
Show file tree
Hide file tree
Showing 22 changed files with 509 additions and 332 deletions.
19 changes: 19 additions & 0 deletions .github/pull_request_template.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
What I Did
------------


How I Did it
------------


How to verify it
------------


Description for the Changelog
------------



<!-- (thanks https://github.com/linuxkit/linuxkit for this template) -->

6 changes: 3 additions & 3 deletions BUILD.mill
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ def main(s):
tree = go_create_tree(s, root_package)

j = new_job_set()
# j.add('go_fmt', lambda: go_vet(s))
# j.add('go_vet', lambda: go_fmt(s))
# j.add('go_build', lambda: go_build(tree, package=root_package))
j.add('go_fmt', lambda: go_vet(s))
j.add('go_vet', lambda: go_fmt(s))
j.add('go_build', lambda: go_build(tree, package=root_package))
j.add('go_test', lambda: go_test(tree, package=root_package))
return j
10 changes: 6 additions & 4 deletions pkg/lifecycle/message/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,16 @@ import (
"github.com/pkg/errors"
"github.com/replicatedcom/ship/pkg/api"
"github.com/replicatedcom/ship/pkg/lifecycle/render/config"
"github.com/replicatedcom/ship/pkg/templates"
"github.com/spf13/viper"
)

type DaemonMessenger struct {
Logger log.Logger
UI cli.Ui
Viper *viper.Viper
Daemon config.Daemon
Logger log.Logger
UI cli.Ui
Viper *viper.Viper
Daemon config.Daemon
BuilderBuilder *templates.BuilderBuilder
}

func (m *DaemonMessenger) Execute(ctx context.Context, release *api.Release, step *api.Message) error {
Expand Down
11 changes: 6 additions & 5 deletions pkg/lifecycle/message/cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,10 @@ import (
var _ Messenger = &CLIMessenger{}

type CLIMessenger struct {
Logger log.Logger
UI cli.Ui
Viper *viper.Viper
Logger log.Logger
UI cli.Ui
Viper *viper.Viper
BuilderBuilder *templates.BuilderBuilder
}

func (m *CLIMessenger) WithDaemon(_ config.Daemon) Messenger {
Expand Down Expand Up @@ -48,14 +49,14 @@ func (e *CLIMessenger) Execute(ctx context.Context, release *api.Release, step *
}

func (e *CLIMessenger) getBuilder(release *api.Release) templates.Builder {
builder := templates.NewBuilder(
builder := e.BuilderBuilder.NewBuilder(
templates.NewStaticContext(),
builderContext{
logger: e.Logger,
viper: e.Viper,
},
&templates.InstallationContext{
Meta: release.Metadata,
Meta: release.Metadata,
Viper: e.Viper,
},
)
Expand Down
18 changes: 10 additions & 8 deletions pkg/lifecycle/message/messenger.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,18 @@ type Messenger interface {
func FromViper(v *viper.Viper) Messenger {
if v.GetBool("headless") {
return &CLIMessenger{
Logger: logger.FromViper(v),
UI: ui.FromViper(v),
Viper: v,
Logger: logger.FromViper(v),
UI: ui.FromViper(v),
Viper: v,
BuilderBuilder: templates.BuilderBuilderFromViper(v),
}
}

return &DaemonMessenger{
Logger: logger.FromViper(v),
UI: ui.FromViper(v),
Viper: v,
Logger: logger.FromViper(v),
UI: ui.FromViper(v),
Viper: v,
BuilderBuilder: templates.BuilderBuilderFromViper(v),
}
}

Expand All @@ -38,8 +40,8 @@ func (m *DaemonMessenger) WithDaemon(d config.Daemon) Messenger {
}

func (m *DaemonMessenger) getBuilder(meta api.ReleaseMetadata) templates.Builder {
builder := templates.NewBuilder(
templates.NewStaticContext(),
builder := m.BuilderBuilder.NewBuilder(
m.BuilderBuilder.NewStaticContext(),
builderContext{
logger: m.Logger,
viper: m.Viper,
Expand Down
46 changes: 26 additions & 20 deletions pkg/lifecycle/render/config/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,9 @@ const MissingRequiredValue = "MISSING_REQUIRED_VALUE"

// APIConfigRenderer resolves config values via API
type APIConfigRenderer struct {
Logger log.Logger
Viper *viper.Viper
Logger log.Logger
Viper *viper.Viper
BuilderBuilder *templates.BuilderBuilder
}

type ValidationError struct {
Expand Down Expand Up @@ -87,24 +88,25 @@ func deepCopyMap(original map[string]interface{}) (map[string]interface{}, error
}

// given a set of input values ('liveValues') and the config ('configGroups') returns a map of configItem names to values, with all config option template functions resolved
func resolveConfigValuesMap(liveValues map[string]interface{}, configGroups []libyaml.ConfigGroup, logger log.Logger, viper *viper.Viper) (map[string]interface{}, error) {
func (r *APIConfigRenderer) resolveConfigValuesMap(liveValues map[string]interface{}, configGroups []libyaml.ConfigGroup) (map[string]interface{}, error) {
// make a deep copy of the live values map
updatedValues, err := deepCopyMap(liveValues)
if err != nil {
return nil, err
}

configCtx, err := NewConfigContext(
viper, logger,
configCtx, err := r.BuilderBuilder.NewConfigContext(
configGroups,
updatedValues)
updatedValues,
)

if err != nil {
return nil, err
}

staticCtx := templates.NewStaticContext()
builder := templates.NewBuilder(
templates.NewStaticContext(),
staticCtx := r.BuilderBuilder.NewStaticContext()
builder := r.BuilderBuilder.NewBuilder(
r.BuilderBuilder.NewStaticContext(),
configCtx,
)

Expand All @@ -116,7 +118,9 @@ func resolveConfigValuesMap(liveValues map[string]interface{}, configGroups []li
}

// Build config values in order & add them to the template builder
var deps depGraph
deps := depGraph{
BuilderBuilder: r.BuilderBuilder,
}
deps.ParseConfigGroup(configGroups)
var headNodes []string

Expand Down Expand Up @@ -148,15 +152,15 @@ func resolveConfigValuesMap(liveValues map[string]interface{}, configGroups []li
}

//recalculate builder with new values
newConfigCtx, err := NewConfigContext(
viper, logger,
newConfigCtx, err := r.BuilderBuilder.NewConfigContext(
configGroups,
updatedValues)
updatedValues,
)
if err != nil {
return nil, err
}

builder = templates.NewBuilder(
builder = r.BuilderBuilder.NewBuilder(
staticCtx,
newConfigCtx,
)
Expand Down Expand Up @@ -186,7 +190,7 @@ func (r *APIConfigRenderer) ResolveConfig(
return resolvedConfig, errors.Wrap(err, "deep copy config")
}

updatedValues, err := resolveConfigValuesMap(liveValues, configCopy, r.Logger, r.Viper)
updatedValues, err := r.resolveConfigValuesMap(liveValues, configCopy)

if err != nil {
return resolvedConfig, errors.Wrap(err, "resolve configCopy values map")
Expand Down Expand Up @@ -291,15 +295,17 @@ func (r *APIConfigRenderer) newBuilder(
release *api.Release,
templateContext map[string]interface{},
) (*templates.Builder, error) {
newConfigCtx, err := NewConfigContext(
r.Viper, r.Logger,
release.Spec.Config.V1, templateContext)

newConfigCtx, err := r.BuilderBuilder.NewConfigContext(
release.Spec.Config.V1,
templateContext,
)
if err != nil {
return nil, err
}

builder := templates.NewBuilder(
templates.NewStaticContext(),
builder := r.BuilderBuilder.NewBuilder(
r.BuilderBuilder.NewStaticContext(),
newConfigCtx,
)
return &builder, nil
Expand Down
34 changes: 27 additions & 7 deletions pkg/lifecycle/render/config/api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,11 @@ import (

"strings"

"github.com/go-kit/kit/log"
"github.com/go-test/deep"
"github.com/replicatedcom/ship/pkg/api"
_ "github.com/replicatedcom/ship/pkg/lifecycle/render/config/test-cases/api"
"github.com/replicatedcom/ship/pkg/templates"
"github.com/replicatedcom/ship/pkg/test-mocks/logger"
"github.com/replicatedhq/libyaml"
"github.com/spf13/viper"
"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -72,15 +73,24 @@ type configTestCase struct {
func TestAPIResolver(t *testing.T) {
ctx := context.Background()

resolver := &APIConfigRenderer{
Logger: log.NewNopLogger(),
}

tests := loadAPITestCases(t, filepath.Join("test-cases", "api"))

for _, test := range tests {
t.Run(test.Name, func(t *testing.T) {
req := require.New(t)
v := viper.New()
testLogger := &logger.TestLogger{T: t}

builderBuilder := &templates.BuilderBuilder{
Logger: testLogger,
Viper: v,
}

resolver := &APIConfigRenderer{
Logger: testLogger,
Viper: v,
BuilderBuilder: builderBuilder,
}

release := &api.Release{
Spec: api.Spec{
Expand All @@ -89,7 +99,6 @@ func TestAPIResolver(t *testing.T) {
},
},
}
resolver.Viper = viper.New()

func() {
if test.LiveValues == nil {
Expand Down Expand Up @@ -206,7 +215,18 @@ func TestResolveConfigValuesMap(t *testing.T) {
//build a config to test
groups := buildTestConfigGroups(test.dependencies, test.prefix, test.suffix, false)

output, err := resolveConfigValuesMap(test.input, groups, log.NewNopLogger(), viper.New())
testLogger := &logger.TestLogger{T: t}
v := viper.New()
builderBuilder := &templates.BuilderBuilder{
Logger: testLogger,
Viper: v,
}
renderer := &APIConfigRenderer{
Logger: testLogger,
Viper: v,
BuilderBuilder: builderBuilder,
}
output, err := renderer.resolveConfigValuesMap(test.input, groups)
req.NoError(err)

req.Equal(test.results, output)
Expand Down
25 changes: 8 additions & 17 deletions pkg/lifecycle/render/config/daemon.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,12 @@ type Daemon interface {

// Daemon runs the ship api server.
type ShipDaemon struct {
Logger log.Logger
Fs afero.Afero
Viper *viper.Viper
UI cli.Ui
StateManager *state.StateManager
Logger log.Logger
Fs afero.Afero
Viper *viper.Viper
UI cli.Ui
StateManager *state.StateManager
ConfigRenderer *APIConfigRenderer

sync.Mutex
currentStep *api.Step
Expand Down Expand Up @@ -355,13 +356,8 @@ func (d *ShipDaemon) postAppConfigLive(release *api.Release) gin.HandlerFunc {
liveValues[itemValue.Name] = itemValue.Value
}

resolver := &APIConfigRenderer{
Logger: d.Logger,
Viper: d.Viper,
}

debug.Log("event", "resolveConfig")
resolvedConfig, err := resolver.ResolveConfig(c, release, savedSate, liveValues)
resolvedConfig, err := d.ConfigRenderer.ResolveConfig(c, release, savedSate, liveValues)
if err != nil {
level.Error(d.Logger).Log("event", "resolveconfig failed", "err", err)
c.AbortWithStatus(500)
Expand Down Expand Up @@ -430,13 +426,8 @@ func (d *ShipDaemon) putAppConfig(release *api.Release) gin.HandlerFunc {
liveValues[itemValue.Name] = itemValue.Value
}

resolver := &APIConfigRenderer{
Logger: d.Logger,
Viper: d.Viper,
}

debug.Log("event", "resolveConfig")
resolvedConfig, err := resolver.ResolveConfig(c, release, savedSate, liveValues)
resolvedConfig, err := d.ConfigRenderer.ResolveConfig(c, release, savedSate, liveValues)
if err != nil {
level.Error(d.Logger).Log("event", "resolveconfig failed", "err", err)
c.AbortWithStatus(500)
Expand Down
18 changes: 12 additions & 6 deletions pkg/lifecycle/render/config/dep_graph.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ import (
)

type depGraph struct {
Dependencies map[string]map[string]struct{}
BuilderBuilder *templates.BuilderBuilder
Dependencies map[string]map[string]struct{}
}

//these config functions are used to add their dependencies to the depGraph
Expand Down Expand Up @@ -81,26 +82,31 @@ func (d *depGraph) Copy() (depGraph, error) {
var buf bytes.Buffer
enc := json.NewEncoder(&buf)
dec := json.NewDecoder(&buf)
err := enc.Encode(d)
err := enc.Encode(d.Dependencies)
if err != nil {
return depGraph{}, err
}
var copy depGraph
var copy map[string]map[string]struct{}
err = dec.Decode(&copy)
if err != nil {
return depGraph{}, err
}
return copy, nil

return depGraph{
BuilderBuilder: d.BuilderBuilder,
Dependencies: copy,
}, nil

}

func (d *depGraph) ParseConfigGroup(configGroups []libyaml.ConfigGroup) error {
staticCtx := templates.NewStaticContext()
staticCtx := d.BuilderBuilder.NewStaticContext()
for _, configGroup := range configGroups {
for _, configItem := range configGroup.Items {
// add this to the dependency graph
d.AddNode(configItem.Name)

depBuilder := templates.NewBuilder(staticCtx)
depBuilder := d.BuilderBuilder.NewBuilder(staticCtx)
depBuilder.Functs = d.funcMap(configItem.Name)

// while builder is normally stateless, the functions it uses within this loop are not
Expand Down
Loading

0 comments on commit 03fe21d

Please sign in to comment.